-
Notifications
You must be signed in to change notification settings - Fork 455
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
[GEP-19] Migrate seed Prometheus deployment and configuration #9180
[GEP-19] Migrate seed Prometheus deployment and configuration #9180
Conversation
6e6f01c
to
e3f5d16
Compare
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.
Thanks a lot for this nicely structured improvement.
pkg/component/monitoring/charts/bootstrap/charts/monitoring/templates/config.yaml
Show resolved
Hide resolved
pkg/component/monitoring/charts/bootstrap/charts/monitoring/templates/config.yaml
Show resolved
Hide resolved
pkg/component/monitoring/charts/bootstrap/charts/monitoring/templates/config.yaml
Show resolved
Hide resolved
e3f5d16
to
013d45e
Compare
013d45e
to
40c4510
Compare
6dbd32c
to
b99221f
Compare
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.
Thanks a lot for the explanations.
/lgtm
LGTM label has been added. Git tree hash: 1b19ef1289dd91a58938c9b91d7a148d83fc7361
|
/assign |
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 PR, thanks 🚀
@@ -43,11 +44,15 @@ func (p *prometheus) vpa() *vpaautoscalingv1.VerticalPodAutoscaler { | |||
}, | |||
ResourcePolicy: &vpaautoscalingv1.PodResourcePolicy{ | |||
ContainerPolicies: []vpaautoscalingv1.ContainerResourcePolicy{ | |||
{ | |||
ContainerName: "*", |
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.
Does VPA merge ContainerPolicies
? Otherwise, there won't be a match. Afaik prometheus runs 2 container only (prometheus, config-reloader) which have already dedicated policies.
I suggest to include this setting into prometheus
container, config-reloader
is not scaled anyway.
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.
Interesting, we just talked about this a few hours ago with @voelzmo 😄 Actually, we don't know for sure. I was just reflecting the change from #9175 so that it also gets applied when Prometheus is managed via prometheus-operator
. In the scope of this PR, this is correct/what we should do. If we think this VPA definition is wrong, then let's fix it separately (my goal is just to translate/refactor the Helm charts to "mgmt via prom-operator", not to change/add/remove/reinvent things). :) OK?
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.
I'm pretty sure now, that VPA does not merge ContainerPolicy (ref).
Anyway, we can fix it in an own PR too.
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.
Thanks for checking @oliver-goetz!
@voelzmo @nickytd Would you like to open a PR that addresses these concerns?
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.
done: #9192
b99221f
to
1bc2821
Compare
I added a new commit which adapts the |
/lgtm |
LGTM label has been added. Git tree hash: 9dd09ae768678b31d23323dd896e0bd9837d3390
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: oliver-goetz The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
We don't use a ServiceMonitor here because the `kubernetes_sd_configs` uses `role=service` which is only supported via `ScrapeConfig` resources.
1bc2821
to
1e29231
Compare
/lgtm |
LGTM label has been added. Git tree hash: e2e301c6b81b4932a1b893c3b8424fc7cd409a02
|
How to categorize this PR?
/area dev-productivity monitoring
/kind enhancement
What this PR does / why we need it:
Similar to #9128, this PR migrates management of the seed Prometheus deployment and its configuration to
prometheus-operator
.As part of this, I adapted the
VerticalPodAutoscaler
spec for Prometheis in thepkg/componen/tmonitoring/prometheus
package to reflect the changes from #9175.Which issue(s) this PR fixes:
Part of #9065
Special notes for your reviewer:
/cc @oliver-goetz @ScheererJ
FYI @istvanballok @rickardsjp
Release note:
It is now possible to provide configuration for the seed Prometheus running in seed clusters' `garden` namespaces. Read all about it [here](https://github.com/gardener/gardener/tree/master/docs/extensions/logging-and-monitoring.md#seed-prometheus).