Skip to content

OCPBUGS-14644: Do not set master node selector if there's no masters #1347

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Feb 28, 2023
Merged

OCPBUGS-14644: Do not set master node selector if there's no masters #1347

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 20 additions & 4 deletions pkg/cli/admin/mustgather/mustgather.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,6 +307,22 @@ func (o *MustGatherOptions) Run() error {
defer cleanupNamespace()
}

// Prefer to run in master if there's any but don't be explicit otherwise.
// This enables the command to run by default in hypershift where there's no masters.
nodes, err := o.Client.CoreV1().Nodes().List(context.TODO(), metav1.ListOptions{
LabelSelector: o.NodeSelector,
})
if err != nil {
return err
}
var hasMaster bool
for _, node := range nodes.Items {
if _, ok := node.Labels["node-role.kubernetes.io/master"]; ok {
hasMaster = true
break
}
}

// ... and create must-gather pod(s)
var pods []*corev1.Pod
for _, image := range o.Images {
Expand All @@ -328,7 +344,7 @@ func (o *MustGatherOptions) Run() error {
return err
}
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{})
Copy link
Contributor

@soltysh soltysh Feb 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before we start adding weird edge cases, have you considered using --node-selector=node-role.kubernetes.io/worker instead?

Copy link
Member Author

@enxebre enxebre Feb 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@soltysh soltysh Feb 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if err != nil {
// ensure the errors bubble up to BackupGathering method for display
errs = []error{err}
Expand All @@ -345,7 +361,7 @@ func (o *MustGatherOptions) Run() error {
return err
}
}
pod, err := o.Client.CoreV1().Pods(ns.Name).Create(context.TODO(), o.newPod(o.NodeName, image), metav1.CreateOptions{})
pod, err := o.Client.CoreV1().Pods(ns.Name).Create(context.TODO(), o.newPod(o.NodeName, image, hasMaster), metav1.CreateOptions{})
if err != nil {
// ensure the errors bubble up to BackupGathering method for display
errs = []error{err}
Expand Down Expand Up @@ -705,13 +721,13 @@ func newClusterRoleBinding(ns string) *rbacv1.ClusterRoleBinding {
// newPod creates a pod with 2 containers with a shared volume mount:
// - gather: init containers that run gather command
// - copy: no-op container we can exec into
func (o *MustGatherOptions) newPod(node, image string) *corev1.Pod {
func (o *MustGatherOptions) newPod(node, image string, hasMaster bool) *corev1.Pod {
zero := int64(0)

nodeSelector := map[string]string{
corev1.LabelOSStable: "linux",
}
if node == "" {
if node == "" && hasMaster {
nodeSelector["node-role.kubernetes.io/master"] = ""
}

Expand Down