-
Notifications
You must be signed in to change notification settings - Fork 71
Add parameter enableAdminAPI to prometheus configuration #909
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
base: main
Are you sure you want to change the base?
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: morenod The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Hi @morenod. Thanks for your PR. I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
cd6b11b to
29cce1b
Compare
|
/ok-to-test |
| }, | ||
| RemoteWrite: config.RemoteWrite, | ||
| ExternalLabels: config.ExternalLabels, | ||
| EnableAdminAPI: config.EnableAdminAPI, |
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.
After a deeper look, this just won't work. If you check in https://github.com/rhobs/observability-operator/blob/main/pkg/controllers/monitoring/monitoring-stack/components.go#L150 you'll notice that there's a mapping fom this API to the prometheus-operator one. As prom-op doesn't have anything about that, it'll just fail. You'd need to couple this change with another one adding the feature to prometheus-operator (specifically, to the downstream fork used here)
2e42e2b to
6130562
Compare
6130562 to
8ca0b03
Compare
8be8e95 to
3552875
Compare
go.mod
Outdated
|
|
||
| replace github.com/openshift/api => github.com/openshift/api v0.0.0-20240404200104-96ed2d49b255 | ||
|
|
||
| replace github.com/rhobs/observability-operator/pkg/apis => ./pkg/apis |
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.
Note that this may bring issues as there's a mapping in between the apis and the observability operator ones. Let's see if the images build but I'll take a deeper look.
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.
We've just discussed it with @danielmellado. Since we (ok it was me) split the API (into a new Go module), I think we need to split this into two PRs (so that this replacement is not needed):
- introducing/updating the API changes
- introducing the "impl" changes with API dependency update (previous commit updating the API)
Does it make sense?
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.
Ok, so I have updated the files here to only contain the changes on API
|
After checking this a bit more in detail,two things here. Yes, you can rely on the fork but I'd split this into two. Get the api in first so you can get the new pseudo commit API version there in. Thanks! |
3552875 to
012e3cb
Compare
CI is passing now just with the changes in the API |
|
@tremes @danielmellado anything else missing to merge this? |
Add the EnableAdminAPI boolean field to the PrometheusConfig struct to allow users to configure whether the Prometheus Admin API should be enabled. This commit only adds the API definition without implementing its usage in the controller. The field includes appropriate validation and documentation warning about security implications. Signed-off-by: morenod <[email protected]>
012e3cb to
181d599
Compare
|
Since #941 we don't need to split an API change into 2 PRs. Can you rebase on main?
|
|
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
As part of the openstack
[watcher](https://opendev.org/openstack/watcher)tests we are injecting metrics on prometheus to simulate CPU and memory load on openstack instances.we need to delete old metrics to avoid errors calculating node resource utilizations.
To inject metrics, only remotewritter is needed, but to delete them, we need the adminAPI.
We are enabling it by patching prometheus on post_scripting, after controlplane is created, using:
oc patch prometheuses.monitoring.rhobs metric-storage --namespace=openstack --type=merge -p '{"spec":{"enableAdminAPI":true}}'hereHaving the possibility of adding this parameter to the control-plane kustomize variables at the installation time would avoid to need the post scripting, making edpm jobs easier to maintain (no need to patch, undeploy prometheus, waiting for it to be available...)
I have followed the same logic than used to add
enableRemoteWriteReceiverparameter, which is also used by our tests to inject metrics.I'm also creating a similar PR on telemetry-operator project, not sure which will require the other to be first merged