OCPBUGS-14644: Do not set master node selector if there's no masters #1347
Conversation
This is so oc adm must-gather works by default in hypershift clusters
| } | ||
| for _, node := range nodes.Items { | ||
| pod, err := o.Client.CoreV1().Pods(ns.Name).Create(context.TODO(), o.newPod(node.Name, image), metav1.CreateOptions{}) | ||
| pod, err := o.Client.CoreV1().Pods(ns.Name).Create(context.TODO(), o.newPod(node.Name, image, hasMaster), metav1.CreateOptions{}) |
There was a problem hiding this comment.
Before we start adding weird edge cases, have you considered using --node-selector=node-role.kubernetes.io/worker instead?
There was a problem hiding this comment.
I wouldn't look at Openshift hosted clusters powered by hypershift as a weird use case but rather as the only/default scenario for Red Hat Openshift managed services offering and the increasing choice for self-hosted multi-cluster offering.
The intend was to avoid end user to have to set an additional flag for it work ootb in any of the scenarios above.
Whether this is the right impl approach to satisfy the use case I have no opinions / not familiar with this code base, so feel free to close and request a deeper discussion. I'm curious why we run in a master by default?
There was a problem hiding this comment.
I'm curious why we run in a master by default?
We care about what's going on masters only, even though parts of the data we scrape is reachable through API, significant portion is scraping data directly from the OS running the control plane components. That's why it was natural for us to force running them on masters.
openshift-ci
bot
added
the
do-not-merge/hold
|
cc @soltysh any thoughts on how to move this forward? |
soltysh
left a comment
soltysh
left a comment
There was a problem hiding this comment.
/hold cancel
/lgtm
/approve
I'll have to make a few refactorings to that code, so let's land it as is for now. |
openshift-ci
bot
removed
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: enxebre, soltysh 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 |
openshift-ci
bot
added
the
approved
|
@enxebre: all tests passed! 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. |
|
@soltysh we'd want this in 4.12. Let's discuss if you have any objections, concerns... |
|
@enxebre: new pull request created: #1366 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. |
|
Too late to get this bug linked up to the 4.13 GA errata or anything, but just to make it easier for folks looking at this pull request to find the bug chain it's the tip of: /retitle OCPBUGS-14644: Do not set master node selector if there's no masters |
openshift-ci
bot
changed the title
|
@enxebre: Jira Issue OCPBUGS-14644: All pull requests linked via external trackers have merged: Jira Issue OCPBUGS-14644 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. |
6 participants
This is so oc adm must-gather works by default in hypershift clusters.
Ref https://issues.redhat.com/browse/HOSTEDCP-836