MGMT-9797: Determine number of replicas according to `DefaultPlacement` #728

omertuc · 2022-03-24T16:57:13Z

Implementation of the https://github.com/openshift/enhancements/blob/master/enhancements/single-node/single-node-openshift-with-workers.md
enhancement.

The installation process now sets the new DefaultPlacement
API field in the Ingress Config CR's status (openshift/installer#5746
still unmerged at the time of writing this message).

This PR introduces two new changes -

While creating the default IngressController, or while creating
deployments corresponding to IngressController resources which
don't have their number of replicas set, we determine the number
of replicas according to the value of the new DefaultPlacement
parameter.
When creating Deployments out of IngressController resources
which don't have their nodeSelector set, we determine the default
nodeSelector according to the new DefaultPlacement API field.

See the enhancement, new API documentation, implementation and code
comments in the diff for the exact details on how, why and when we
set the fields and to which values.

omertuc · 2022-04-05T11:48:04Z

/cc @Miciah

Miciah

I have a few stylistic concerns, but overall the changes look good. You need to fix calls to desiredRouterDeployment in unit tests though. Also, please add unit test coverage for the changes to desiredRouterDeployment.

pkg/operator/controller/ingress/deployment.go

pkg/operator/controller/ingress/replicas.go

pkg/operator/operator.go

Miciah · 2022-04-05T14:28:42Z

I have a few stylistic concerns, but overall the changes look good. You need to fix calls to desiredRouterDeployment in unit tests though. Also, please add unit test coverage for the changes to desiredRouterDeployment.

By the way, #712 refactors the test code. Please define a new TestDesiredRouterDeploymentDefaultPlacement test function rather than modifying TestDesiredRouterDeployment.

ETA: I have something like the following in mind (this is an example and may need more test cases or other tweaks).

Example unit test

func TestDesiredRouterDeploymentDefaultPlacement(t *testing.T) {
	var (
		workerNodeSelector = map[string]string{
			"kubernetes.io/os":               "linux",
			"node-role.kubernetes.io/worker": "",
		}
		controlPlaneNodeSelector = map[string]string{
			"kubernetes.io/os":               "linux",
			"node-role.kubernetes.io/master": "",
		}
	)
	testCases := []struct {
		name                 string
		ingressConfig        *configv1.Ingress
		infraConfig          *configv1.Infrastructure
		expectedNodeSelector map[string]string
		expectedReplicas     int
	}{
		{
			name:          "empty ingress/infra config",
			ingressConfig: &configv1.Ingress{},
			infraConfig: &configv1.Infrastructure{
				Status: configv1.InfrastructureStatus{
					PlatformStatus: &configv1.PlatformStatus{
						Type: configv1.AWSPlatformType,
					},
				},
			},
			expectedNodeSelector: workerNodeSelector,
			expectedReplicas:     2,
		},
		{
			name:          "empty ingress config, single-node, worker",
			ingressConfig: &configv1.Ingress{},
			infraConfig: &configv1.Infrastructure{
				Status: configv1.InfrastructureStatus{
					InfrastructureTopology: configv1.SingleReplicaTopologyMode,
					PlatformStatus: &configv1.PlatformStatus{
						Type: configv1.AWSPlatformType,
					},
				},
			},
			expectedNodeSelector: workerNodeSelector,
			expectedReplicas:     1,
		},
		{
			name: "empty ingress config, single-node, control-plane",
			ingressConfig: &configv1.Ingress{
				Status: configv1.IngressStatus{
					DefaultPlacement: configv1.DefaultPlacementControlPlane,
				},
			},
			infraConfig: &configv1.Infrastructure{
				Status: configv1.InfrastructureStatus{
					ControlPlaneTopology: configv1.SingleReplicaTopologyMode,
					PlatformStatus: &configv1.PlatformStatus{
						Type: configv1.AWSPlatformType,
					},
				},
			},
			expectedNodeSelector: controlPlaneNodeSelector,
			expectedReplicas:     1,
		},
	}
	for _, tc := range testCases {
		t.Run(tc.name, func(t *testing.T) {
			var (
				ic = &operatorv1.IngressController{
					ObjectMeta: metav1.ObjectMeta{
						Name: "default",
					},
					Spec: operatorv1.IngressControllerSpec{},
					Status: operatorv1.IngressControllerStatus{
						EndpointPublishingStrategy: &operatorv1.EndpointPublishingStrategy{
							Type: operatorv1.PrivateStrategyType,
						},
					},
				}
				ingressControllerImage = "quay.io/openshift/router:latest"
				apiConfig              = &configv1.APIServer{}
				networkConfig          = &configv1.Network{}
			)

			proxyNeeded, err := IsProxyProtocolNeeded(ic, tc.infraConfig.Status.PlatformStatus)
			if err != nil {
				t.Fatal(err)
			}

			deployment, err := desiredRouterDeployment(ic, ingressControllerImage, tc.ingressConfig, tc.infraConfig, apiConfig, networkConfig, proxyNeeded, false, nil)
			if err != nil {
				t.Error(err)
			}

			checkDeploymentHash(t, deployment)

			expectedReplicas := int32(tc.expectedReplicas)
			if !reflect.DeepEqual(deployment.Spec.Replicas, &expectedReplicas) {
				t.Errorf("expected replicas to be %v, got %v", expectedReplicas, *deployment.Spec.Replicas)
			}

			if !reflect.DeepEqual(deployment.Spec.Template.Spec.NodeSelector, tc.expectedNodeSelector) {
				t.Errorf("expected node selector to be %v, got %v", tc.expectedNodeSelector, deployment.Spec.Template.Spec.NodeSelector)
			}
			checkDeploymentHash(t, deployment)
		})
	}

}

Miciah · 2022-04-05T16:39:43Z

The verify job found some fmt issues:

 !!! gofmt needs to be run on the listed files
./pkg/operator/controller/ingress/deployment.go
./pkg/operator/operator.go

pkg/operator/controller/ingress/deployment.go

sferich888 · 2022-04-05T21:25:12Z

/label px-approved

omertuc · 2022-04-06T10:21:10Z

Force push includes running make generate so the ci/prow/verify job can pass

omertuc · 2022-04-06T13:11:39Z

/retest-required

omertuc · 2022-04-06T13:11:45Z

Closed by mistake

omertuc · 2022-04-06T17:01:47Z

/retest

openshift-bot · 2022-04-22T07:02:15Z

/retest-required