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 1884430: Move kube-rbac-proxy to general ovnkube DeamonSet #778
Conversation
Hi @bond95. Thanks for your PR. I'm waiting for a openshift 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/test-infra repository. |
/retest |
080d5e9
to
b7f8cc4
Compare
/ok-to-test |
@@ -286,63 +343,6 @@ spec: | |||
- name: ovn-cert | |||
secret: | |||
secretName: ovn-cert |
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 believe the volume
entry in both ovnkube-node.yaml
and ovnkube-master.yaml
needs to be made optional: true
(as it is in bindata/network/openshift-sdn/sdn.yaml
), because the secret can't be created until after the network is already up, but if the volume is required then the network won't come up without the secret having already been created.
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.
Tests are failing precisely because of this
b7f8cc4
to
661f1d5
Compare
/retest |
/lgtm |
/retest |
1 similar comment
/retest |
661f1d5
to
20d9f23
Compare
/lgtm |
-H "Authorization: Bearer $(cat /var/run/secrets/kubernetes.io/serviceaccount/token)" \ | ||
"https://${KUBERNETES_SERVICE_HOST}:${KUBERNETES_SERVICE_PORT}/api/v1/namespaces/ovn-kubernetes/services/ovnkube-master" | | ||
python -c 'import json,sys; print(json.load(sys.stdin)["metadata"]["creationTimestamp"])' | ||
) |
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.
replace this line with
) || :
So that we don't get the error treated as a fatal. And the same with the node one.
20d9f23
to
518118b
Compare
/retest |
@abhat this includes PRs #751 and #786 . I've verified it works on a cluster manually. Can you PTAL? Logs of the cluster where we see it working as intended
|
/retest |
/retest Please review the full test history for this PR and help us cut down flakes. |
5 similar comments
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/bugzilla refresh |
@juanluisvaladas: No Bugzilla bug is referenced in the title of this pull request. 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. |
/retitle Bug 1884430: Move kube-rbac-proxy to general ovnkube DeamonSet |
/bugzilla refresh |
@juanluisvaladas: No Bugzilla bug is referenced in the title of this pull request. 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. |
@bond95: This pull request references Bugzilla bug 1884430, 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. |
/test e2e-vsphere-ovn |
/help |
@juanluisvaladas : can we please make sure we don't break that alerts-job thing again ? We have already bounced around this label change quiet a few times: |
@tssurya what needs to be changed? I see tests are passing so it should be fine |
/retest Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
/retest Please review the full test history for this PR and help us cut down flakes. |
@bond95: 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. |
/retest Please review the full test history for this PR and help us cut down flakes. |
@bond95: All pull requests linked via external trackers have merged: Bugzilla bug 1884430 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. |
With PR openshift#778 (commit 518118b) we changed the service monitor definition which broke the alert definition for OVN.
With PR openshift#778 (commit 518118b) we changed the service monitor definition which broke the alert definition for OVN.
With PR openshift#778 (commit 518118b) we changed the service monitor definition, and the daemonset serviceaccount. With this PR we fix the serverName in the serviceMonitors, the serviceAccount permissions and the prometheusRule alert expression.
With PR openshift#778 (commit 518118b) we changed the service monitor definition, and the daemonset serviceaccount. With this PR we fix the serverName in the serviceMonitors, the serviceAccount permissions and the prometheusRule alert expression.
Follow up of this PR: #751
Basically it's fixing same issue as for SDN. To avoid deploying metric's DeamonSet before OVN, metrics was moved to OVN DeamonSet.