[release-4.18] OCPBUGS-59897: Update downloads deployment configuration to use master node selector (#1029)

* Update downloads deployment configuration to use master node selector and modify the tests accordingly

* gofmt

* Update downloads deployment to include master node selector, and keep the linux selector too

* Enhance node selector handling for externalized control plane by ensuring only the master node selector is removed if it exists

* Add Linux OS node selector to console deployment configuration as well

* Fix the unit test

* fix: fix the failing unit tests

* refactor: rename withConsoleNodeSelector to withNodeSelector

* fix: remove Linux OS node selector from console deployment configuration

* fix: fix the failing unit test

* test: add missing fields to deployment test case

---------

Co-authored-by: Leo HC Li <[email protected]>

Author: openshift-cherrypick-robot

bindata/assets/deployments/downloads-deployment.yaml

@@ -26,6 +26,7 @@ spec:
     spec:
       nodeSelector:
         kubernetes.io/os: linux
+        node-role.kubernetes.io/master: ""
       terminationGracePeriodSeconds: 0
       securityContext:
         runAsNonRoot: true

pkg/console/subresource/deployment/deployment.go

@@ -105,7 +105,7 @@ func DefaultDeployment(
 		canMountCustomLogo,
 	)
 	withConsoleContainerImage(deployment, operatorConfig, proxyConfig)
-	withConsoleNodeSelector(deployment, infrastructureConfig)
+	withNodeSelector(deployment, infrastructureConfig)
 	util.AddOwnerRef(deployment, util.OwnerRefFrom(operatorConfig))
 	return deployment
 }
@@ -121,6 +121,7 @@ func DefaultDownloadsDeployment(
 	withAffinity(downloadsDeployment, infrastructureConfig, "downloads")
 	withStrategy(downloadsDeployment, infrastructureConfig)
 	withDownloadsContainerImage(downloadsDeployment)
+	withNodeSelector(downloadsDeployment, infrastructureConfig)
 	util.AddOwnerRef(downloadsDeployment, util.OwnerRefFrom(operatorConfig))
 	return downloadsDeployment
 }
@@ -324,15 +325,18 @@ func withConsoleContainerImage(
 	deployment.Spec.Template.Spec.Containers[0].Image = util.GetImageEnv("CONSOLE_IMAGE")
 }
 
-func withConsoleNodeSelector(
+func withNodeSelector(
 	deployment *appsv1.Deployment,
 	infrastructureConfig *configv1.Infrastructure,
 ) {
 	nodeSelector := deployment.Spec.Template.Spec.NodeSelector
 
-	// If running with an externalized control plane, remove the master node selector
+	// If running with an externalized control plane, remove only the master node selector
 	if infrastructureConfig.Status.ControlPlaneTopology == configv1.ExternalTopologyMode {
-		nodeSelector = map[string]string{}
+		if nodeSelector == nil {
+			nodeSelector = map[string]string{}
+		}
+		delete(nodeSelector, "node-role.kubernetes.io/master")
 	}
 
 	deployment.Spec.Template.Spec.NodeSelector = nodeSelector

pkg/console/subresource/deployment/deployment_test.go

@@ -237,11 +237,8 @@ func TestDefaultDeployment(t *testing.T) {
 						Spec: corev1.PodSpec{
 							ServiceAccountName: "console",
 							// we want to deploy on master nodes
-							NodeSelector: map[string]string{
-								// empty string is correct
-								"node-role.kubernetes.io/master": "",
-							},
-							Affinity: consoleDeploymentAffinity,
+							NodeSelector: map[string]string{"node-role.kubernetes.io/master": ""},
+							Affinity:     consoleDeploymentAffinity,
 							// toleration is a taint override. we can and should be scheduled on a master node.
 							Tolerations:                   consoleDeploymentTolerations,
 							PriorityClassName:             "system-cluster-critical",
@@ -318,11 +315,8 @@ func TestDefaultDeployment(t *testing.T) {
 						Spec: corev1.PodSpec{
 							ServiceAccountName: "console",
 							// we want to deploy on master nodes
-							NodeSelector: map[string]string{
-								// empty string is correct
-								"node-role.kubernetes.io/master": "",
-							},
-							Affinity: consoleDeploymentAffinity,
+							NodeSelector: map[string]string{"node-role.kubernetes.io/master": ""},
+							Affinity:     consoleDeploymentAffinity,
 							// toleration is a taint override. we can and should be scheduled on a master node.
 							Tolerations:                   consoleDeploymentTolerations,
 							PriorityClassName:             "system-cluster-critical",
@@ -399,11 +393,8 @@ func TestDefaultDeployment(t *testing.T) {
 						Spec: corev1.PodSpec{
 							ServiceAccountName: "console",
 							// we want to deploy on master nodes
-							NodeSelector: map[string]string{
-								// empty string is correct
-								"node-role.kubernetes.io/master": "",
-							},
-							Affinity: &corev1.Affinity{},
+							NodeSelector: map[string]string{"node-role.kubernetes.io/master": ""},
+							Affinity:     &corev1.Affinity{},
 							// toleration is a taint override. we can and should be scheduled on a master node.
 							Tolerations:                   consoleDeploymentTolerations,
 							PriorityClassName:             "system-cluster-critical",
@@ -1414,7 +1405,9 @@ func TestWithConsoleNodeSelector(t *testing.T) {
 				Spec: appsv1.DeploymentSpec{
 					Template: corev1.PodTemplateSpec{
 						Spec: corev1.PodSpec{
-							NodeSelector: map[string]string{},
+							NodeSelector: map[string]string{
+								"foo": "bar",
+							},
 						},
 					},
 				},
@@ -1423,7 +1416,7 @@ func TestWithConsoleNodeSelector(t *testing.T) {
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			withConsoleNodeSelector(tt.args.deployment, tt.args.infrastructureConfig)
+			withNodeSelector(tt.args.deployment, tt.args.infrastructureConfig)
 			if diff := deep.Equal(tt.args.deployment, tt.want); diff != nil {
 				t.Error(diff)
 			}
@@ -1486,7 +1479,8 @@ func TestDefaultDownloadsDeployment(t *testing.T) {
 
 	downloadsDeploymentPodSpecSingleReplica := corev1.PodSpec{
 		NodeSelector: map[string]string{
-			"kubernetes.io/os": "linux",
+			"kubernetes.io/os":               "linux",
+			"node-role.kubernetes.io/master": "",
 		},
 		Affinity: &corev1.Affinity{},
 		Tolerations: []corev1.Toleration{