Skip to content

Commit ee2ffd6

Browse files
ingvagabundopenshift-cherrypick-robot
ingvagabund
authored and
openshift-cherrypick-robot
committed
fix(must-gather): do not set node affinity if nodename is set
1 parent 0581d70 commit ee2ffd6

File tree

2 files changed

+207
-28
lines changed

2 files changed

+207
-28
lines changed

‎pkg/cli/admin/mustgather/mustgather.go‎

Lines changed: 57 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,9 @@ const (
5757
unreachableTaintKey = "node.kubernetes.io/unreachable"
5858
notReadyTaintKey = "node.kubernetes.io/not-ready"
5959
controlPlaneNodeRoleLabel = "node-role.kubernetes.io/control-plane"
60+
defaultMustGatherCommand = "/usr/bin/gather"
61+
defaultVolumePercentage = 70
62+
defaultSourceDir = "/must-gather/"
6063
)
6164

6265
var (
@@ -172,10 +175,10 @@ func NewMustGatherCommand(f kcmdutil.Factory, streams genericiooptions.IOStreams
172175

173176
func NewMustGatherOptions(streams genericiooptions.IOStreams) *MustGatherOptions {
174177
opts := &MustGatherOptions{
175-
SourceDir: "/must-gather/",
178+
SourceDir: defaultSourceDir,
176179
IOStreams: streams,
177180
Timeout: 10 * time.Minute,
178-
VolumePercentage: 70,
181+
VolumePercentage: defaultVolumePercentage,
179182
}
180183
opts.LogOut = opts.newPrefixWriter(streams.Out, "[must-gather ] OUT", false, true)
181184
opts.RawOut = opts.newPrefixWriter(streams.Out, "", false, false)
@@ -619,7 +622,7 @@ func (o *MustGatherOptions) Run() error {
619622
}
620623
var hasMaster bool
621624
for _, node := range nodes.Items {
622-
if _, ok := node.Labels["node-role.kubernetes.io/control-plane"]; ok {
625+
if _, ok := node.Labels[controlPlaneNodeRoleLabel]; ok {
623626
hasMaster = true
624627
break
625628
}
@@ -1073,38 +1076,21 @@ func newClusterRoleBinding(ns *corev1.Namespace) *rbacv1.ClusterRoleBinding {
10731076
}
10741077
}
10751078

1076-
// newPod creates a pod with 2 containers with a shared volume mount:
1077-
// - gather: init containers that run gather command
1078-
// - copy: no-op container we can exec into
1079-
func (o *MustGatherOptions) newPod(node, image string, hasMaster bool, affinity *corev1.Affinity) *corev1.Pod {
1080-
1079+
func defaultMustGatherPod(image string) *corev1.Pod {
10811080
zero := int64(0)
10821081

1083-
nodeSelector := map[string]string{
1084-
corev1.LabelOSStable: "linux",
1085-
}
1086-
if node == "" && hasMaster {
1087-
nodeSelector["node-role.kubernetes.io/control-plane"] = ""
1088-
}
1082+
cleanedSourceDir := path.Clean(defaultSourceDir)
1083+
volumeUsageChecker := fmt.Sprintf(volumeUsageCheckerScript, cleanedSourceDir, defaultVolumePercentage)
1084+
podCmd := buildPodCommand(volumeUsageChecker, defaultMustGatherCommand)
10891085

1090-
executedCommand := "/usr/bin/gather"
1091-
if len(o.Command) > 0 {
1092-
executedCommand = strings.Join(o.Command, " ")
1093-
}
1094-
1095-
cleanedSourceDir := path.Clean(o.SourceDir)
1096-
volumeUsageChecker := fmt.Sprintf(volumeUsageCheckerScript, cleanedSourceDir, o.VolumePercentage)
1097-
podCmd := buildPodCommand(volumeUsageChecker, executedCommand)
1098-
1099-
ret := &corev1.Pod{
1086+
return &corev1.Pod{
11001087
ObjectMeta: metav1.ObjectMeta{
11011088
GenerateName: "must-gather-",
11021089
Labels: map[string]string{
11031090
"app": "must-gather",
11041091
},
11051092
},
11061093
Spec: corev1.PodSpec{
1107-
NodeName: node,
11081094
// This pod is ok to be OOMKilled but not preempted. Following the conventions mentioned at:
11091095
// https://github.com/openshift/enhancements/blob/master/CONVENTIONS.md#priority-classes
11101096
// so setting priority class to system-cluster-critical
@@ -1166,8 +1152,9 @@ func (o *MustGatherOptions) newPod(node, image string, hasMaster bool, affinity
11661152
},
11671153
},
11681154
},
1169-
HostNetwork: o.HostNetwork,
1170-
NodeSelector: nodeSelector,
1155+
NodeSelector: map[string]string{
1156+
corev1.LabelOSStable: "linux",
1157+
},
11711158
TerminationGracePeriodSeconds: &zero,
11721159
Tolerations: []corev1.Toleration{
11731160
{
@@ -1177,9 +1164,51 @@ func (o *MustGatherOptions) newPod(node, image string, hasMaster bool, affinity
11771164
Operator: "Exists",
11781165
},
11791166
},
1180-
Affinity: affinity,
11811167
},
11821168
}
1169+
}
1170+
1171+
// newPod creates a pod with 2 containers with a shared volume mount:
1172+
// - gather: init containers that run gather command
1173+
// - copy: no-op container we can exec into
1174+
func (o *MustGatherOptions) newPod(node, image string, hasMaster bool, affinity *corev1.Affinity) *corev1.Pod {
1175+
executedCommand := defaultMustGatherCommand
1176+
if len(o.Command) > 0 {
1177+
executedCommand = strings.Join(o.Command, " ")
1178+
}
1179+
1180+
cleanedSourceDir := path.Clean(o.SourceDir)
1181+
volumeUsageChecker := fmt.Sprintf(volumeUsageCheckerScript, cleanedSourceDir, o.VolumePercentage)
1182+
podCmd := buildPodCommand(volumeUsageChecker, executedCommand)
1183+
1184+
ret := defaultMustGatherPod(image)
1185+
ret.Spec.Containers[0].Command = []string{"/bin/bash", "-c", podCmd}
1186+
ret.Spec.Containers[0].VolumeMounts = []corev1.VolumeMount{{
1187+
Name: "must-gather-output",
1188+
MountPath: cleanedSourceDir,
1189+
ReadOnly: false,
1190+
}}
1191+
ret.Spec.Containers[1].VolumeMounts = []corev1.VolumeMount{{
1192+
Name: "must-gather-output",
1193+
MountPath: cleanedSourceDir,
1194+
ReadOnly: false,
1195+
}}
1196+
ret.Spec.HostNetwork = o.HostNetwork
1197+
if node == "" && hasMaster {
1198+
ret.Spec.NodeSelector[controlPlaneNodeRoleLabel] = ""
1199+
}
1200+
1201+
if node != "" {
1202+
ret.Spec.NodeName = node
1203+
} else {
1204+
// Set affinity towards the most suitable nodes. E.g. to exclude
1205+
// nodes that if unreachable could cause the must-gather pod to stay
1206+
// in Pending state indefinitely. Please check getCandidateNodeNames
1207+
// function for more details about how the selection of the most
1208+
// suitable nodes is performed.
1209+
ret.Spec.Affinity = affinity
1210+
}
1211+
11831212
if o.HostNetwork {
11841213
// If a user specified hostNetwork he might have intended to perform
11851214
// packet captures on the host, for that we need to set the correct

‎pkg/cli/admin/mustgather/mustgather_test.go‎

Lines changed: 150 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package mustgather
33
import (
44
"context"
55
"fmt"
6+
"path"
67
"reflect"
78
"testing"
89
"time"
@@ -478,3 +479,152 @@ sync && echo 'Caches written to disk'`,
478479
})
479480
}
480481
}
482+
483+
func TestNewPod(t *testing.T) {
484+
generateMustGatherPod := func(image string, update func(pod *corev1.Pod)) *corev1.Pod {
485+
pod := defaultMustGatherPod(image)
486+
update(pod)
487+
return pod
488+
}
489+
490+
tests := []struct {
491+
name string
492+
options *MustGatherOptions
493+
hasMasters bool
494+
node string
495+
affinity *corev1.Affinity
496+
expectedPod *corev1.Pod
497+
}{
498+
{
499+
name: "node set, affinity provided, affinity not set",
500+
options: &MustGatherOptions{
501+
VolumePercentage: defaultVolumePercentage,
502+
SourceDir: defaultSourceDir,
503+
},
504+
node: "node1",
505+
affinity: buildNodeAffinity([]string{"node2"}),
506+
expectedPod: generateMustGatherPod("image", func(pod *corev1.Pod) {
507+
pod.Spec.NodeName = "node1"
508+
}),
509+
},
510+
{
511+
name: "node not set, affinity provided, affinity set",
512+
options: &MustGatherOptions{
513+
VolumePercentage: defaultVolumePercentage,
514+
SourceDir: defaultSourceDir,
515+
},
516+
affinity: buildNodeAffinity([]string{"node2"}),
517+
expectedPod: generateMustGatherPod("image", func(pod *corev1.Pod) {
518+
pod.Spec.Affinity = buildNodeAffinity([]string{"node2"})
519+
}),
520+
},
521+
{
522+
name: "custom source dir",
523+
options: &MustGatherOptions{
524+
VolumePercentage: defaultVolumePercentage,
525+
SourceDir: "custom-source-dir",
526+
},
527+
expectedPod: generateMustGatherPod("image", func(pod *corev1.Pod) {
528+
volumeUsageChecker := fmt.Sprintf(volumeUsageCheckerScript, "custom-source-dir", defaultVolumePercentage)
529+
podCmd := buildPodCommand(volumeUsageChecker, defaultMustGatherCommand)
530+
pod.Spec.Containers[0].Command = []string{"/bin/bash", "-c", podCmd}
531+
pod.Spec.Containers[0].VolumeMounts = []corev1.VolumeMount{{
532+
Name: "must-gather-output",
533+
MountPath: "custom-source-dir",
534+
}}
535+
pod.Spec.Containers[1].VolumeMounts = []corev1.VolumeMount{{
536+
Name: "must-gather-output",
537+
MountPath: "custom-source-dir",
538+
}}
539+
}),
540+
},
541+
{
542+
name: "host network",
543+
options: &MustGatherOptions{
544+
VolumePercentage: defaultVolumePercentage,
545+
SourceDir: defaultSourceDir,
546+
HostNetwork: true,
547+
},
548+
expectedPod: generateMustGatherPod("image", func(pod *corev1.Pod) {
549+
pod.Spec.HostNetwork = true
550+
pod.Spec.Containers[0].SecurityContext = &corev1.SecurityContext{
551+
Capabilities: &corev1.Capabilities{
552+
Add: []corev1.Capability{
553+
corev1.Capability("CAP_NET_RAW"),
554+
},
555+
},
556+
}
557+
}),
558+
},
559+
{
560+
name: "with control plane nodes",
561+
options: &MustGatherOptions{
562+
VolumePercentage: defaultVolumePercentage,
563+
SourceDir: defaultSourceDir,
564+
},
565+
hasMasters: true,
566+
expectedPod: generateMustGatherPod("image", func(pod *corev1.Pod) {
567+
pod.Spec.NodeSelector[controlPlaneNodeRoleLabel] = ""
568+
}),
569+
},
570+
{
571+
name: "with custom command",
572+
options: &MustGatherOptions{
573+
VolumePercentage: defaultVolumePercentage,
574+
SourceDir: defaultSourceDir,
575+
Command: []string{"custom_command", "with_params"},
576+
},
577+
expectedPod: generateMustGatherPod("image", func(pod *corev1.Pod) {
578+
cleanSourceDir := path.Clean(defaultSourceDir)
579+
volumeUsageChecker := fmt.Sprintf(volumeUsageCheckerScript, cleanSourceDir, defaultVolumePercentage)
580+
podCmd := buildPodCommand(volumeUsageChecker, "custom_command with_params")
581+
pod.Spec.Containers[0].Command = []string{"/bin/bash", "-c", podCmd}
582+
pod.Spec.Containers[0].VolumeMounts = []corev1.VolumeMount{{
583+
Name: "must-gather-output",
584+
MountPath: cleanSourceDir,
585+
}}
586+
pod.Spec.Containers[1].VolumeMounts = []corev1.VolumeMount{{
587+
Name: "must-gather-output",
588+
MountPath: cleanSourceDir,
589+
}}
590+
}),
591+
},
592+
{
593+
name: "with since",
594+
options: &MustGatherOptions{
595+
VolumePercentage: defaultVolumePercentage,
596+
SourceDir: defaultSourceDir,
597+
Since: 2 * time.Minute,
598+
},
599+
expectedPod: generateMustGatherPod("image", func(pod *corev1.Pod) {
600+
pod.Spec.Containers[0].Env = append(pod.Spec.Containers[0].Env, corev1.EnvVar{
601+
Name: "MUST_GATHER_SINCE",
602+
Value: "2m0s",
603+
})
604+
}),
605+
},
606+
{
607+
name: "with sincetime",
608+
options: &MustGatherOptions{
609+
VolumePercentage: defaultVolumePercentage,
610+
SourceDir: defaultSourceDir,
611+
SinceTime: "2023-09-24T15:30:00Z",
612+
},
613+
expectedPod: generateMustGatherPod("image", func(pod *corev1.Pod) {
614+
pod.Spec.Containers[0].Env = append(pod.Spec.Containers[0].Env, corev1.EnvVar{
615+
Name: "MUST_GATHER_SINCE_TIME",
616+
Value: "2023-09-24T15:30:00Z",
617+
})
618+
}),
619+
},
620+
}
621+
622+
for _, test := range tests {
623+
t.Run(test.name, func(t *testing.T) {
624+
tPod := test.options.newPod(test.node, "image", test.hasMasters, test.affinity)
625+
if !cmp.Equal(test.expectedPod, tPod) {
626+
t.Errorf("Unexpected pod command was generated: \n%s\n", cmp.Diff(test.expectedPod, tPod))
627+
}
628+
})
629+
}
630+
}

0 commit comments

Comments
 (0)