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
[MON-1950] Remove UI access from the Thanos routes #1512
[MON-1950] Remove UI access from the Thanos routes #1512
Conversation
raptorsun
commented
Dec 13, 2021
- I added CHANGELOG entry for this change.
- No user facing changes, so no entry in CHANGELOG was needed.
744b907
to
8b5fd89
Compare
/retest-required |
8b5fd89
to
9d2be2d
Compare
2c620d6
to
85b5584
Compare
About test "[sig-instrumentation] Prometheus when installed on the cluster should start and expose a secured proxy and unsecured metrics": |
/test e2e-agnostic-operator |
/test e2e-agnostic-operator |
8389e5a
to
aefb9e9
Compare
scheme: HTTPS | ||
initialDelaySeconds: 5 | ||
periodSeconds: 5 | ||
resources: | ||
requests: | ||
cpu: 1m | ||
memory: 20Mi | ||
memory: 15Mi |
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 would leave the requests as-is
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, I will change it to 20MiB.
From which criteria do we know this kube-rbac-proxy should require 20MiB memory while its neighbor container kube-rbac-proxy for tenancy requires 15MiB? 🤔
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.
https://github.com/openshift/enhancements/blob/master/CONVENTIONS.md#resources-and-limits describes how to determine resource requests for OpenShift components. The other kube-rbac-proxy is only for serving /metrics while this container would serve the /api/v1/* requests so I would expect that the latter uses more resources. I think it is wiser to leave the requests untouched for now and check later if we can decrease the values.
aefb9e9
to
0fe5805
Compare
here is the PR on openshift/origin: https://github.com/openshift/origin/pull/26695/files |
76f43b6
to
433a337
Compare
433a337
to
4694bd1
Compare
/test e2e-agnostic-operator |
I think that the |
3c2daf7
to
ed17f06
Compare
ed17f06
to
74db4fc
Compare
/test e2e-agnostic-operator |
@raptorsun: The following tests failed, say
Full PR test history. Your PR dashboard. 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/test-infra repository. I understand the commands that are listed here. |
Route is restricted to /api for Thanos Ruler now :) |
Hello @kyoto , Is there some reference to Thanos Ruler API in openshift/console that needs to be tested? |
/skip @raptorsun the console backend only uses the Thanos query API. |
/label docs-approved The associated epic (https://issues.redhat.com/browse/MON-1962) has the acks from docs and PX. |
Thank you :) No additional test will be required for Thanos Ruler then. |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: raptorsun, simonpasquier 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 |
Hello @juzhao , this PR is ready for QE testing. |
@lihongyan1 will test it |
The PR on openshift/origin is no longer required for this PR. We can safely ignore that now. |
@raptorsun Yes, all the Observe > Alerting pages will need to be tested for both the Administrator and Developer Consoles as well as the alert notifications (at the top right of the Console UI). CC @lihongyan1 |
Tested the pr following steps in case Everything looks well |
/label qe-approved |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
/retest-required Please review the full test history for this PR and help us cut down flakes. |