-
Notifications
You must be signed in to change notification settings - Fork 784
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
added: support for metrics configuration, periodic metrics cleanup and selective namespace whitelisting and blacklisting with respect to metrics registration #2288
Conversation
8bb7a37
to
8cf8f78
Compare
Hi @yashvardhan-kukreja - have you tested it locally? Seems like the Kyverno Pod never enters running & ready state. Can you please verify locally? |
Yes I did for both with a definite metrics refresh and no metrics refresh. Was running perfectly fine on my end. I'll test it again, meanwhile do you mind sending the logs and the output of Also, can you share the values.yaml file against which you are testing this PR? |
Here's the log, I think it uses the direct manifest to install Kyverno, and Pod was in an Error state:
|
Shuting, I also checked around this. So, I installed Kyverno around 10-15 times and only once the issue occured and that was around Kyverno entering "Running" state but still it had 0/1 containers meaning, Kyverno container was failing. And upon describing it, I found that the Kyverno container failed the liveness probe's check in that situation due to which the Kyverno was registered as unhealthy. So, as a resolution I increased the |
Another thing Shuting. I found that although I had programmed the default case scenarios when |
I guess got the issue around here. I forgot to update metrics config related manifests and kustomize definitions under |
All done @realshuting :) |
6f92582
to
d3fa768
Compare
@realshuting once you're done reviewing this PR, please do not merge it. Please review and merge #2351 first. Once it gets merged to |
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.
/lgtm
@yashvardhan-kukreja - do we have issues tracking website updates? We need to document how to customize ConfigMap using Helm and direct manifests.
Yeah Shuting, I'll hop onto documenting that.
So that, by default, Kyverno's metric exporter won't reset and cleanup metrics in its buffer every 24 hrs. |
657c424
to
bfb149a
Compare
@realshuting do you mind running e2e tests corresponding to this branch on your end? I tried running them on my end but they Passed so I am not exactly able to find what's the concern here and why the e2e tests of the github actions are reporting failure. |
@yashvardhan-kukreja - verified locally, all looked good! Can you please rebase the main branch? And sorry for the late response. |
…d selective namespace whitelisting and blacklisting for metrics Signed-off-by: Yashvardhan Kukreja <yash.kukreja.98@gmail.com>
bfb149a
to
fd8b2d9
Compare
Thank you @yashvardhan-kukreja ! |
Hi @yashvardhan-kukreja - following up on the doc update, do we have an issue logged to track it? |
Signed-off-by: Yashvardhan Kukreja yash.kukreja.98@gmail.com
Related issue
closes #2268
Milestone of this PR
What type of PR is this
Proposed Changes
Now,
values.yaml
will encapsulate configuration for metrics exposure as wellnamespaces.include
- specifically, the namespaces for which the metrics will be collected. Default: metrics will be collected for all namespaces.namespaces.exclude
- the namespaces for which the metrics will NOT be collected. Default: none of the namespaces are excluded.metricsRefreshInterval
- The periodic rate at which the metrics of Kyverno will get cleaned. This will help in reducing the memory footprint associated with metrics collection and exposure in Kyverno. And this sort of metrics cleanup doesn't involve any loss of data because the metrics are always going to be persistently getting stored on end-user's Prometheus server.Proof Manifests
Checklist
Further Comments