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
Remove finalizers #115
Merged
Merged
Remove finalizers #115
Conversation
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
fhielpos
approved these changes
Jun 8, 2022
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.
Nice 🚀
@giantswarm/team-atlas as our resident metric gurus, if one of you has a quick moment today/tomorrow to see if any of this is completely backwards I'd appreciate it. I'll merge this tomorrow |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
To support sharding metrics across exporters, it would be nice to get rid of the finalizers added by each controller to their watched aqua resources. The finalizers were only needed because it isn't possible to delete a metric from the golang prometheus client without specifying all of the labels for that metric -- in other words, we had to reconstruct the entire metric (so vulnerability details, etc.) in order to delete it. Finalizers were added to give the exporters an opportunity to clear the metrics before the report is deleted.
I've since implemented the functionality needed in the prometheus client, but this change has not been released yet.
With sharded metrics, the shard owner is responsible for clearing the finalizer after it has cleared its metrics for a given report. However, even the finalizer does not handle all cases where we might miss metrics, for example, when a statefulset is updated as described by #48. So, I'm committing a minor sin by using an untagged version of the prometheus client in order to a. solve the issue with stale metrics and b. hopefully get us closer to caching fewer reports per instance.
Checklist
values.yaml
andvalues.schema.json
are valid.version
andappVersion
in Chart.yaml.