-
Notifications
You must be signed in to change notification settings - Fork 133
SREP-1831 - Fix watch command label #802
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
SREP-1831 - Fix watch command label #802
Conversation
| fmt.Println("Service log sent successfully. Use the following command to track progress of the resize:") | ||
| fmt.Println() | ||
| fmt.Println(`watch -d 'oc get machines -n openshift-machine-api -l machine.openshift.io/cluster-api-machine-role=master && oc get nodes -l node-role.kubernetes.io/control-plane'`) | ||
| fmt.Println(`watch -d 'oc get machines -n openshift-machine-api -l machine.openshift.io/cluster-api-machine-role=master && oc get nodes -l node-role.kubernetes.io/master'`) |
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.
Didn't CPMS switch to "control-plane" at some point? Won't this not work for "newer" instances?
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.
Looking to our other codebases in the openshift org, it seems like critical components such as etcd still rely on this label as of the 4.22 branch: https://github.com/openshift/cluster-etcd-operator/blob/release-4.22/pkg/cmd/render/certs.go#L50
It's not exactly the same thing, but the enhancements repo also specifies the role.kubernetes.io/master taint is still the de-facto source-of-truth, with no plans to switch to a role.kubernetes.io/control-plane equivalent.
From testing on a 4.17.z cluster, I'm happy to report that the role.kubernetes.io/master label does select all instances correctly, while the role.kubernetes.io/control-plane one only returns the newly made control-plane nodes
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.
Gotcha! So the new nodes have both labels? If so, this looks good to me.
/lgtm
|
@tnierman: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. I understand the commands that are listed here. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: joshbranham, tnierman The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
https://issues.redhat.com/browse/SREP-1831
Modifies the label used by the suggested
watchcommand to print all (current and new) control-plane instances when watching control-plane scale-up/down processes.