Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CA-393199: Disable external auth should clean pbis cache #5642

Merged
merged 1 commit into from
Jun 5, 2024

Conversation

liulinC
Copy link
Collaborator

@liulinC liulinC commented May 21, 2024

The domain information is cached by PBIS. Such cache can help to accelerate query of domain information.

However, the cache data should be cleaned during disable external auth to make sure

  • release cache as the domain is left
  • rejoin domain refresh all the domain information


(* Clean pbis cache as we are disabling external auth *)
( try pbis_common "/opt/pbis/bin/ad-cache" ["--delete-all"] |> ignore
with _ -> debug "Got error cleaning pbis cache, ignore the error"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The exception should be logged to help diagnose the problem.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The exception should be logged to help diagnose the problem.

I ignore the exception by intention as we do not really care about the error, (and ignored)
this is best to have clean.
But I have updated the code anyway as it always better to log errors.

Copy link
Contributor

@Vincent-lau Vincent-lau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Grammar issue, but I think this is confusing:

in the commit message you mean:
"release cache as the domain has left" right?

Copy link
Member

@psafont psafont left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes since linding's comment is waiting

@liulinC
Copy link
Collaborator Author

liulinC commented May 23, 2024

Grammar issue, but I think this is confusing:

in the commit message you mean: "release cache as the domain has left" right?

well, leave domain is part of disable external auth.
Thanks for the comments, I have updated the comment to clarify this.

@liulinC liulinC force-pushed the private/linl/yangtze branch from 4c0c6ba to 31226e8 Compare May 23, 2024 02:10
@liulinC
Copy link
Collaborator Author

liulinC commented May 23, 2024

Sorry, I accidentally create a new branch in this repo, (instead of my private repo).
Will delete the branch after this merge.

@liulinC liulinC force-pushed the private/linl/yangtze branch from 31226e8 to 9b67707 Compare May 27, 2024 05:55
@lindig
Copy link
Contributor

lindig commented May 30, 2024

1060 |         |> debug "Got error cleaning pbis cache, ignore the error %s"
                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Error: This expression has type string -> unit
       but an expression was expected of type (exn -> string) -> 'a
       Type string is not compatible with type exn -> string 
make: *** [Makefile:10: build] Error 1

@lindig
Copy link
Contributor

lindig commented May 30, 2024

This was not tested

The domain information is cached by PBIS. Such cache can
help to accelerate query of domain information.

However, the cache data should be cleaned during disable
external auth to make sure
- release cache as the domain is left
- rejoin domain refresh all the domain information

Signed-off-by: Lin Liu <[email protected]>
@liulinC liulinC force-pushed the private/linl/yangtze branch from 9b67707 to c932279 Compare May 31, 2024 03:27
@liulinC
Copy link
Collaborator Author

liulinC commented May 31, 2024

tested

The original code was test in my local env,
I did not re-test Printexc.to_string as it is low risk.
Thanks compiler detected the grammar issue.

@robhoes
Copy link
Member

robhoes commented Jun 5, 2024

I believe @psafont 's request has been addressed, so I'll dismiss his review and merge the PR.

@robhoes robhoes dismissed psafont’s stale review June 5, 2024 12:47

Feedback has been addressed.

@robhoes robhoes merged commit d26cf25 into 1.249-lcm Jun 5, 2024
8 checks passed
@robhoes robhoes deleted the private/linl/yangtze branch June 5, 2024 12:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants