-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Add plugin status plumbing and update server to use it #2041
Merged
Merged
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
8efa713
plugins: Allow plugins to report status to manager
patrick-east 6459080
plugins/discovery: Add plugin status updates
patrick-east 2fbcff0
plugins/bundle: Add plugin status updates
patrick-east 00bb682
plugins/logs: Add plugin status updates
patrick-east a3bc032
plugins/status: Update to report plugin statuses
patrick-east cb05217
server: Update health check to use plugin status
patrick-east f32ea19
server: Add `bundles` option to /health API
patrick-east a9f41c4
docs: Add pointer to Status API in monitoring docs
patrick-east File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an interesting change. The most pressing issue is that readiness checks in K8s don't take into account discovery. This PR fixes that however if bundles are added/updated and fail to download within a certain amount of time, the liveness probe will fail and the OPA pod will get whacked. Maybe that's the right behaviour. Do we document this somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, and I think that is probably the right behavior for some use-cases.. At that point in time the OPA is not in a state that it should be answering queries since its missing bundles, maybe some applications it doesn't matter, some it does.
Specific to the OPA HTTP server reporting a 500 on /health we document that any non-activated and configured bundle will cause the health check to fail. Maybe what i'll do is revert the change to the example deployment to omit the
?plugin
option on the liveness probe to avoid any accidental confusion on this. People should probably opt in to using it versus just copying the default and having this more aggressive behavior.