Merge pull request #366 from openshift-cherrypick-robot/cherry-pick-3…

…58-to-release-4.13

[release-4.13] OCPBUGS-11449: Set DNS DaemonSet's maxSurge value to 10%

assets/dns/daemonset.yaml

@@ -2,6 +2,8 @@ kind: DaemonSet
 apiVersion: apps/v1
 # name, namespace and labels are set at runtime
 spec:
+  # minReadySeconds should be 3x the readiness probe's polling interval (i.e. periodSeconds).
+  minReadySeconds: 9
   template:
     metadata:
       annotations:
@@ -33,7 +35,7 @@ spec:
             port: 8181
             scheme: HTTP
           initialDelaySeconds: 10
-          periodSeconds: 3
+          periodSeconds: 3 # Update the daemonset's spec.minReadySeconds above if you change this value!
           successThreshold: 1
           failureThreshold: 3
           timeoutSeconds: 3
@@ -85,8 +87,11 @@ spec:
   updateStrategy:
     type: RollingUpdate
     rollingUpdate:
-      # TODO: Consider setting maxSurge to a positive value.
-      maxSurge: 0
-      # Note: The daemon controller rounds the percentage up
-      # (unlike the deployment controller, which rounds down).
-      maxUnavailable: 10%
+      # Set maxSurge to a positive value so that each node that has a pod
+      # continues to have a local ready pod during a rolling update.  This is
+      # important for topology-aware hints as well as for similar logic in
+      # openshift-sdn and ovn-kubernetes that prefers to use a local ready DNS
+      # pod whenever one exists.
+      maxSurge: 10%
+      # maxUnavailable must be zero when maxSurge is nonzero.
+      maxUnavailable: 0

pkg/operator/controller/controller_dns_daemonset.go

@@ -337,6 +337,11 @@ func daemonsetConfigChanged(current, expected *appsv1.DaemonSet) (bool, *appsv1.
 	changed := false
 	updated := current.DeepCopy()
 
+	if current.Spec.MinReadySeconds != expected.Spec.MinReadySeconds {
+		updated.Spec.MinReadySeconds = expected.Spec.MinReadySeconds
+		changed = true
+	}
+
 	if !cmp.Equal(current.Spec.UpdateStrategy, expected.Spec.UpdateStrategy, cmpopts.EquateEmpty()) {
 		updated.Spec.UpdateStrategy = expected.Spec.UpdateStrategy
 		changed = true

pkg/operator/controller/controller_dns_daemonset_test.go

@@ -27,6 +27,18 @@ func TestDesiredDNSDaemonset(t *testing.T) {
 		t.Errorf("invalid dns daemonset: %v", err)
 	} else {
 		// Validate the daemonset
+		pointerTo := func(ios intstr.IntOrString) *intstr.IntOrString { return &ios }
+		expectedUpdateStrategy := appsv1.DaemonSetUpdateStrategy{
+			Type: appsv1.RollingUpdateDaemonSetStrategyType,
+			RollingUpdate: &appsv1.RollingUpdateDaemonSet{
+				MaxSurge:       pointerTo(intstr.FromString("10%")),
+				MaxUnavailable: pointerTo(intstr.FromInt(0)),
+			},
+		}
+		actualUpdateStrategy := ds.Spec.UpdateStrategy
+		if !reflect.DeepEqual(actualUpdateStrategy, expectedUpdateStrategy) {
+			t.Errorf("unexpected update strategy: expected %#v, got %#v", expectedUpdateStrategy, actualUpdateStrategy)
+		}
 		expectedPodAnnotations := map[string]string{
 			"cluster-autoscaler.kubernetes.io/enable-ds-eviction": "true",
 			"target.workload.openshift.io/management":             "{\"effect\": \"PreferredDuringScheduling\"}",
@@ -469,14 +481,23 @@ func TestDaemonsetConfigChanged(t *testing.T) {
 			expect: true,
 		},
 		{
-			description: "if the update strategy changes",
+			description: "if the update strategy's max unavailable parameter changes",
 			mutate: func(daemonset *appsv1.DaemonSet) {
-				daemonset.Spec.UpdateStrategy = appsv1.DaemonSetUpdateStrategy{
-					Type: appsv1.RollingUpdateDaemonSetStrategyType,
-					RollingUpdate: &appsv1.RollingUpdateDaemonSet{
-						MaxUnavailable: pointerTo(intstr.FromString("10%")),
-					},
-				}
+				daemonset.Spec.UpdateStrategy.RollingUpdate.MaxUnavailable = pointerTo(intstr.FromString("10%"))
+			},
+			expect: true,
+		},
+		{
+			description: "if the update strategy's max surge parameter changes",
+			mutate: func(daemonset *appsv1.DaemonSet) {
+				daemonset.Spec.UpdateStrategy.RollingUpdate.MaxSurge = pointerTo(intstr.FromString("10%"))
+			},
+			expect: true,
+		},
+		{
+			description: "if spec.minReadySeconds changes",
+			mutate: func(daemonset *appsv1.DaemonSet) {
+				daemonset.Spec.MinReadySeconds = 9
 			},
 			expect: true,
 		},