Add recreate procedure for deployment and daemonsets in case of chang…

…ed selectors

Selectors are immutable for Deployments and DaemonSets,
in case if it was changed, delete old and create new one with prior serverside validation

pkg/controllers/clusteroperator_controller_test.go

@@ -676,6 +676,7 @@ var _ = Describe("Apply resources should", func() {
 		}
 
 		recorder = record.NewFakeRecorder(32)
+		recorder.IncludeObject = true
 		reconciler = &CloudOperatorReconciler{
 			ClusterOperatorStatusClient: ClusterOperatorStatusClient{
 				Client:   cl,
@@ -758,7 +759,7 @@ var _ = Describe("Apply resources should", func() {
 		updated, err := reconciler.applyResources(context.TODO(), objects)
 		Expect(err).Should(HaveOccurred())
 		Expect(updated).To(BeFalse())
-		Eventually(recorder.Events).Should(Receive(ContainSubstring("Update failed")))
+		Eventually(recorder.Events).Should(Receive(ContainSubstring("Create failed")))
 	})
 
 	It("Expect no update when resources are applied twice", func() {

pkg/controllers/resourceapply/resourceapply.go

@@ -6,6 +6,7 @@ import (
 	"crypto/sha256"
 	"encoding/json"
 	"fmt"
+	"reflect"
 
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
@@ -137,14 +138,14 @@ func applyDeployment(ctx context.Context, client coreclientv1.Client, recorder r
 		required.Annotations[generationAnnotation] = "1"
 		err := client.Create(ctx, required)
 		if err != nil {
-			recorder.Event(required, corev1.EventTypeWarning, "Update failed", err.Error())
+			recorder.Event(required, corev1.EventTypeWarning, "Create failed", err.Error())
 			return false, err
 		}
-		recorder.Event(required, corev1.EventTypeNormal, "Updated successfully", "Resource was successfully updated")
+		recorder.Event(required, corev1.EventTypeNormal, "Created successfully", "Resource was successfully updated")
 		return true, nil
 	}
 	if err != nil {
-		recorder.Event(required, corev1.EventTypeWarning, "Update failed", err.Error())
+		recorder.Event(required, corev1.EventTypeWarning, "Failed to get resource for update", err.Error())
 		return false, err
 	}
 
@@ -161,6 +162,34 @@ func applyDeployment(ctx context.Context, client coreclientv1.Client, recorder r
 		return false, nil
 	}
 
+	// Check if deployment recreation needed
+	// Currently it is necessary if pod selector was changed
+	needRecreate := false
+	if !reflect.DeepEqual(existingCopy.Spec.Selector, required.Spec.Selector) {
+		needRecreate = true
+	}
+	if needRecreate {
+		recorder.Event(
+			existing, corev1.EventTypeNormal,
+			"Delete existing deployment", "Delete existing deployment to recreate it with new parameters",
+		)
+		// Perform dry run creation in order to validate deployment before deleting existing one
+		requiredCopy := required.DeepCopy()
+		requiredCopy.Name = fmt.Sprintf("%s-dry-run", requiredCopy.Name)
+		dryRunOpts := &coreclientv1.CreateOptions{DryRun: []string{metav1.DryRunAll}}
+		err = client.Create(ctx, requiredCopy, dryRunOpts)
+		if err != nil {
+			recorder.Event(existing, corev1.EventTypeWarning, "New resource validation failed", err.Error())
+			return false, err
+		}
+		err = client.Delete(ctx, existing)
+		if err != nil && !apierrors.IsNotFound(err) {
+			recorder.Event(existing, corev1.EventTypeWarning, "Deletion failed", err.Error())
+			return false, err
+		}
+		return applyDeployment(ctx, client, recorder, required)
+	}
+
 	// at this point we know that we're going to perform a write.  We're just trying to get the object correct
 	toWrite := existingCopy // shallow copy so the code reads easier
 	toWrite.Spec = *required.Spec.DeepCopy()
@@ -189,14 +218,14 @@ func applyDaemonSet(ctx context.Context, client coreclientv1.Client, recorder re
 		required.Annotations[generationAnnotation] = "1"
 		err = client.Create(ctx, required)
 		if err != nil {
-			recorder.Event(required, corev1.EventTypeWarning, "Update failed", err.Error())
+			recorder.Event(required, corev1.EventTypeWarning, "Create failed", err.Error())
 			return false, err
 		}
-		recorder.Event(required, corev1.EventTypeNormal, "Updated successfully", "Resource was successfully updated")
+		recorder.Event(required, corev1.EventTypeNormal, "Created successfully", "Resource was successfully created")
 		return true, nil
 	}
 	if err != nil {
-		recorder.Event(required, corev1.EventTypeWarning, "Update failed", err.Error())
+		recorder.Event(required, corev1.EventTypeWarning, "Failed to get resource for update", err.Error())
 		return false, err
 	}
 
@@ -213,6 +242,34 @@ func applyDaemonSet(ctx context.Context, client coreclientv1.Client, recorder re
 		return false, nil
 	}
 
+	// Check if ds recreation needed
+	// Currently it is necessary if pod selector was changed
+	needRecreate := false
+	if !reflect.DeepEqual(existingCopy.Spec.Selector, required.Spec.Selector) {
+		needRecreate = true
+	}
+	if needRecreate {
+		recorder.Event(
+			existing, corev1.EventTypeNormal,
+			"Delete existing daemonset", "Delete existing daemonset to recreate it with new parameters",
+		)
+		// Perform dry run creation in order to validate ds before deleting existing one
+		requiredCopy := required.DeepCopy()
+		requiredCopy.Name = fmt.Sprintf("%s-dry-run", requiredCopy.Name)
+		dryRunOpts := &coreclientv1.CreateOptions{DryRun: []string{metav1.DryRunAll}}
+		err = client.Create(ctx, requiredCopy, dryRunOpts)
+		if err != nil {
+			recorder.Event(existing, corev1.EventTypeWarning, "New resource validation failed", err.Error())
+			return false, err
+		}
+		err = client.Delete(ctx, existing)
+		if err != nil && !apierrors.IsNotFound(err) {
+			recorder.Event(existing, corev1.EventTypeWarning, "Deletion failed", err.Error())
+			return false, err
+		}
+		return applyDaemonSet(ctx, client, recorder, required)
+	}
+
 	// at this point we know that we're going to perform a write.  We're just trying to get the object correct
 	toWrite := existingCopy // shallow copy so the code reads easier
 	toWrite.Spec = *required.Spec.DeepCopy()
@@ -241,14 +298,14 @@ func applyPodDisruptionBudget(ctx context.Context, client coreclientv1.Client, r
 		required.Annotations[generationAnnotation] = "1"
 		err = client.Create(ctx, required)
 		if err != nil {
-			recorder.Event(required, corev1.EventTypeWarning, "Update failed", err.Error())
+			recorder.Event(required, corev1.EventTypeWarning, "Create failed", err.Error())
 			return false, err
 		}
-		recorder.Event(required, corev1.EventTypeNormal, "Updated successfully", "Resource was successfully updated")
+		recorder.Event(required, corev1.EventTypeNormal, "Created successfully", "Resource was successfully updated")
 		return true, nil
 	}
 	if err != nil {
-		recorder.Event(required, corev1.EventTypeWarning, "Update failed", err.Error())
+		recorder.Event(required, corev1.EventTypeWarning, "Failed to get resource for update", err.Error())
 		return false, err
 	}
 

pkg/controllers/resourceapply/resourceapply_test.go

@@ -270,13 +270,8 @@ func TestApplyDeployment(t *testing.T) {
 			g.Expect(tt.expectedDeployment.Annotations[specHashAnnotation]).Should(BeEquivalentTo(updatedDeployment.Annotations[specHashAnnotation]))
 		})
 	}
-}
 
-func TestApplyDeploymentSelector(t *testing.T) {
-	cl, tearDownFn := setupEnvtest(t)
-	defer tearDownFn(t)
-
-	tests := []struct {
+	updateSelectorTests := []struct {
 		name               string
 		desiredDeployment  *appsv1.Deployment
 		expectedDeployment *appsv1.Deployment
@@ -326,7 +321,7 @@ func TestApplyDeploymentSelector(t *testing.T) {
 			expectError:        true,
 		},
 	}
-	for _, tt := range tests {
+	for _, tt := range updateSelectorTests {
 		t.Run(tt.name, func(t *testing.T) {
 			g := NewWithT(t)
 			eventRecorder := record.NewFakeRecorder(1000)
@@ -525,6 +520,96 @@ func TestApplyDaemonSet(t *testing.T) {
 			g.Expect(tt.expectedDaemonSet.Annotations[specHashAnnotation]).Should(BeEquivalentTo(updatedDaemonSet.Annotations[specHashAnnotation]))
 		})
 	}
+
+	updateSelectorTests := []struct {
+		name              string
+		desiredDaemonSet  *appsv1.DaemonSet
+		expectedDaemonSet *appsv1.DaemonSet
+
+		expectError      bool
+		expectedRecreate bool
+	}{
+		{
+			name: "the daemonset is recreated due to a change in match labels field",
+			desiredDaemonSet: func() *appsv1.DaemonSet {
+				w := workloadDaemonSet()
+				w.Spec.Selector = &metav1.LabelSelector{
+					MatchLabels: map[string]string{
+						"bar": "baz",
+					},
+				}
+				w.Spec.Template.Labels = map[string]string{"bar": "baz"}
+				return w
+			}(),
+			expectedDaemonSet: func() *appsv1.DaemonSet {
+				w := workloadDaemonSet()
+				w.Spec.Selector = &metav1.LabelSelector{
+					MatchLabels: map[string]string{
+						"bar": "baz",
+					},
+				}
+				w.Spec.Template.Labels = map[string]string{"bar": "baz"}
+				w.Annotations[specHashAnnotation] = "ba95dff6a88cc11a6cd80aa8a8d7a5e88793809ad27f9f8c5b7b66c39ce13ee4"
+				return w
+			}(),
+			expectedRecreate: true,
+		},
+
+		{
+			name: "resourceapply should report an error in case if resource is malformed",
+			desiredDaemonSet: func() *appsv1.DaemonSet {
+				w := workloadDaemonSet()
+				w.Spec.Selector = &metav1.LabelSelector{
+					MatchLabels: map[string]string{
+						"bar": "baz",
+					},
+				}
+				w.Spec.Template.Labels = map[string]string{"fiz": "baz"}
+				return w
+			}(),
+			expectedDaemonSet: workloadDaemonSetWithDefaultSpecHash(),
+			expectError:       true,
+		},
+	}
+	for _, tt := range updateSelectorTests {
+		t.Run(tt.name, func(t *testing.T) {
+			g := NewWithT(t)
+			eventRecorder := record.NewFakeRecorder(1000)
+			ctx := context.TODO()
+			defer cleanupResources(t, g, ctx, cl, &appsv1.DaemonSetList{})
+
+			actualDaemonSet := workloadDaemonSetWithDefaultSpecHash()
+			g.Expect(cl.Create(ctx, actualDaemonSet)).To(Succeed())
+			g.Expect(actualDaemonSet.UID).NotTo(BeNil())
+
+			_, err := applyDaemonSet(ctx, cl, eventRecorder, tt.desiredDaemonSet)
+			if tt.expectError {
+				g.Expect(err).To(HaveOccurred(), "expected error")
+			}
+			if !tt.expectError {
+				g.Expect(err).NotTo(HaveOccurred(), "expected no error")
+			}
+
+			updatedDaemonSet := &appsv1.DaemonSet{}
+			deploymentObjectKey := appsclientv1.ObjectKeyFromObject(tt.desiredDaemonSet)
+			g.Expect(cl.Get(ctx, deploymentObjectKey, updatedDaemonSet)).To(Succeed())
+			if tt.expectedRecreate {
+				g.Expect(actualDaemonSet.UID).ShouldNot(BeEquivalentTo(updatedDaemonSet.UID))
+			}
+			if !tt.expectedRecreate {
+				g.Expect(actualDaemonSet.UID).Should(BeEquivalentTo(updatedDaemonSet.UID))
+			}
+
+			if !equality.Semantic.DeepDerivative(tt.expectedDaemonSet.Spec, updatedDaemonSet.Spec) {
+				t.Fatalf("Expected deployment: %+v, got %+v", tt.expectedDaemonSet, updatedDaemonSet)
+			}
+			g.Expect(tt.expectedDaemonSet.Annotations[specHashAnnotation]).To(BeEquivalentTo(updatedDaemonSet.Annotations[specHashAnnotation]))
+
+			dss := &appsv1.DaemonSetList{}
+			g.Expect(cl.List(ctx, dss)).To(Succeed())
+			g.Expect(len(dss.Items)).To(BeEquivalentTo(1))
+		})
+	}
 }
 
 func workloadDaemonSet() *appsv1.DaemonSet {