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
Bug 1811834: Sync jsonnet dependencies #722
Conversation
@paulfantom: This pull request references Bugzilla bug 1811834, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
In response to this:
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. |
e0ea276
to
9923716
Compare
/hold |
Its been merged it seems. |
/unhold prometheus/node_exporter#1649 is merged and propagated here. |
@@ -9,8 +9,19 @@ metadata: | |||
namespace: openshift-monitoring | |||
spec: | |||
endpoints: | |||
- honorLabels: true | |||
port: http | |||
- bearerTokenFile: /var/run/secrets/kubernetes.io/serviceaccount/token |
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.
why do we have 2 endpoint entries?
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.
Good catch! Fixing.
63c1fef
to
78836ef
Compare
/test generate |
e276c10
to
ad4ef92
Compare
/retest |
/retest should be back now |
/retest |
… certificates on /metrics endpoints
Rebased to solve merge conflicts |
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.
Couple of questions, otherwise looks good
is configured on this host. | ||
summary: Clock not synchronising. | ||
expr: | | ||
min_over_time(node_timex_sync_status[5m]) == 0 |
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 don't need the job label selector here, like we had for ClockSkewDetected?
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.
That metric comes only from node_exporter which is deployed as DaemonSet, so I don't see why we would need it.
@@ -87,3 +87,15 @@ rules: | |||
- get | |||
- list | |||
- watch | |||
- apiGroups: |
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.
Do we know why this was added for prometheus-operator in kube-prom, can't seem to find it in the prometheus-operator repo itself? https://github.com/coreos/prometheus-operator/blob/master/example/rbac/prometheus-operator/prometheus-operator-cluster-role.yaml should this be updated there?
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.
Without those kube-rbac-proxy didn't work and metrics couldn't be scraped.
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.
okay that explains that, thanks!
} | ||
} | ||
return errors.Wrap(err, "updating deployment object failed") | ||
return errors.Wrap(err, "updating Deployment object failed") |
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 :)
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.
Follow up task to us, maybe tech-debt, unrelated to this PR: we should include the name of these objects in the logs. WDYT?
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.
good idea, could you create JIRA?
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.
aws fails with 🤔:
|
argh, yes. We are checking if prometheus-operator is available over http and not https -.- Source: https://github.com/openshift/origin/blob/master/test/extended/prometheus/prometheus.go#L222 |
/retest |
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
/hold
In case simon wants to review as well.
/lgtm /hold cancel |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lilic, paulfantom, 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 |
@paulfantom: All pull requests linked via external trackers have merged: openshift/cluster-monitoring-operator#722. Bugzilla bug 1811834 has been moved to the MODIFIED state. In response to this:
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. |
/cherry-pick release-4.4 |
@sferich888: #722 failed to apply on top of branch "release-4.4":
In response to this:
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. |
@sferich888 that won't work as the lock file needs to be generated for each release branch, what are you trying to do backport? :) |
Multiple things in one PR (sorry):
/metrics
endpoint for prometheus-operators, JIRA MON-990Some other changes include ones described in prometheus-operator/kube-prometheus#470
Note:
jsonnetfile.lock.json
was updated in stages to check what is changing, but in the end it came down to runningjb update
on all./cc @openshift/openshift-team-monitoring