Improve controller logic + fix tests flakiness

controllers/extendeddaemonset/controller.go

@@ -277,14 +277,8 @@ func (r *Reconciler) updateInstanceWithCurrentRS(logger logr.Logger, daemonset *
 				logger.Error(err, "Failed to update ExtendedDaemonSet")
 				return newDaemonsetCopy, reconcile.Result{}, err
 			}
+			// Not need to update the status, `r.client.Update()` update both
 
-			// This ensures that the first client update respects the desired new status
-			newDaemonsetCopy.Status = newDaemonset.Status
-			logger.Info("Updating ExtendedDaemonSet status")
-			if err := r.client.Status().Update(context.TODO(), newDaemonsetCopy); err != nil {
-				logger.Error(err, "Failed to update ExtendedDaemonSet status")
-				return newDaemonsetCopy, reconcile.Result{}, err
-			}
 			return newDaemonsetCopy, reconcile.Result{}, nil
 		}
 

controllers/extendeddaemonset/controller_test.go

@@ -16,6 +16,7 @@ import (
 	test "github.com/DataDog/extendeddaemonset/api/v1alpha1/test"
 	commontest "github.com/DataDog/extendeddaemonset/pkg/controller/test"
 	"github.com/DataDog/extendeddaemonset/pkg/controller/utils/comparison"
+	"github.com/google/go-cmp/cmp"
 
 	"github.com/go-logr/logr"
 	corev1 "k8s.io/api/core/v1"
@@ -643,7 +644,7 @@ func TestReconcileExtendedDaemonSet_updateInstanceWithCurrentRS(t *testing.T) {
 	}
 	daemonsetWithCanaryFailedNewStatus := daemonsetWithCanaryFailedOldStatus.DeepCopy()
 	{
-		daemonsetWithCanaryFailedNewStatus.ResourceVersion = "3"
+		daemonsetWithCanaryFailedNewStatus.ResourceVersion = "2"
 		daemonsetWithCanaryFailedNewStatus.Status.State = datadoghqv1alpha1.ExtendedDaemonSetStatusStateCanaryFailed
 		daemonsetWithCanaryFailedNewStatus.Status.Canary = nil
 	}
@@ -776,11 +777,11 @@ func TestReconcileExtendedDaemonSet_updateInstanceWithCurrentRS(t *testing.T) {
 				t.Errorf("ReconcileExtendedDaemonSet.updateInstanceWithCurrentRS() error = %v, wantErr %v", err, tt.wantErr)
 				return
 			}
-			if !apiequality.Semantic.DeepEqual(got, tt.want) {
-				t.Errorf("ReconcileExtendedDaemonSet.updateInstanceWithCurrentRS() got = %#v, \n want %#v", got, tt.want)
+			if diff := cmp.Diff(tt.want, got); diff != "" {
+				t.Errorf("ReconcileExtendedDaemonSet.updateInstanceWithCurrentRS() (-want +got):\n%s", diff)
 			}
-			if !reflect.DeepEqual(got1, tt.wantResult) {
-				t.Errorf("ReconcileExtendedDaemonSet.updateInstanceWithCurrentRS() gotResult = %v, \n wantResult %v", got1, tt.wantResult)
+			if diff := cmp.Diff(tt.wantResult, got1); diff != "" {
+				t.Errorf("ReconcileExtendedDaemonSet.updateInstanceWithCurrentRS().result (-want +got):\n%s", diff)
 			}
 		})
 	}

controllers/extendeddaemonset_e2e_test.go

@@ -120,11 +120,11 @@ var _ = Describe("ExtendedDaemonSet e2e updates and recovery", func() {
 				NoRestartsDuration: &metav1.Duration{Duration: 1 * time.Minute},
 				AutoPause: &datadoghqv1alpha1.ExtendedDaemonSetSpecStrategyCanaryAutoPause{
 					Enabled:     datadoghqv1alpha1.NewBool(true),
-					MaxRestarts: datadoghqv1alpha1.NewInt32(2),
+					MaxRestarts: datadoghqv1alpha1.NewInt32(1),
 				},
 				AutoFail: &datadoghqv1alpha1.ExtendedDaemonSetSpecStrategyCanaryAutoFail{
 					Enabled:     datadoghqv1alpha1.NewBool(true),
-					MaxRestarts: datadoghqv1alpha1.NewInt32(3),
+					MaxRestarts: datadoghqv1alpha1.NewInt32(2),
 				},
 			}
 			eds.Spec.Template.Spec.Containers[0].Image = fmt.Sprintf("gcr.io/google-containers/alpine-with-bash:1.0")

controllers/extendeddaemonset_test.go

@@ -48,9 +48,6 @@ var _ = Describe("ExtendedDaemonSet Controller", func() {
 		}
 
 		It("Should handle EDS ", func() {
-			nodeList := &corev1.NodeList{}
-			Expect(k8sClient.List(ctx, nodeList)).Should(Succeed())
-
 			edsOptions := &testutils.NewExtendedDaemonsetOptions{
 				CanaryStrategy: &datadoghqv1alpha1.ExtendedDaemonSetSpecStrategyCanary{
 					Duration: &metav1.Duration{Duration: 1 * time.Minute},
@@ -76,9 +73,10 @@ var _ = Describe("ExtendedDaemonSet Controller", func() {
 				Name:      eds.Status.ActiveReplicaSet,
 			}
 			Eventually(withERS(ersKey, ers, func() bool {
-				// Info: we use ers.Status.Desired or ers.Status.Current because the pod status is not updated with the test FWK
+				// Info: we use ers.Status.Desired and ers.Status.Current because the pod status is not updated with the test FWK
 				// and so available and ready will never be updated
-				return ers.Status.Status == "active" && int(ers.Status.Desired) == len(nodeList.Items)
+				fmt.Fprintf(GinkgoWriter, "ERS status:\n%s\n", spew.Sdump(ers.Status))
+				return ers.Status.Status == "active" && int(ers.Status.Desired) == int(ers.Status.Current)
 			}), timeout, interval).Should(BeTrue())
 		})
 
@@ -104,6 +102,10 @@ var _ = Describe("ExtendedDaemonSet Controller", func() {
 				},
 			}
 			Eventually(withList(listOptions, canaryPods, "canary pods", func() bool {
+<<<<<<< HEAD
+=======
+				fmt.Fprintf(GinkgoWriter, "canary pods nb: %d ", len(canaryPods.Items))
+>>>>>>> 690a702... Improve controller logic + fix tests flakiness
 				return len(canaryPods.Items) == 1
 			}), timeout, interval).Should(BeTrue())
 		})

controllers/extendeddaemonsetreplicaset/controller.go

@@ -81,8 +81,16 @@ func (r *Reconciler) Reconcile(ctx context.Context, request reconcile.Request) (
 	}
 
 	if !datadoghqv1alpha1.IsDefaultedExtendedDaemonSet(daemonsetInstance) {
-		reqLogger.Info("Parent ExtendedDaemonSet is not defaulted, requeuing")
-		return reconcile.Result{RequeueAfter: time.Second}, nil
+		message := "Parent ExtendedDaemonSet is not defaulted, requeuing"
+		reqLogger.Info(message)
+		err = fmt.Errorf("parent ExtendedDaemonSet is not defaulted")
+		newStatus := replicaSetInstance.Status.DeepCopy()
+		// Updating the status with a new condition will trigger a new Event on the ExtendedDaemonSet-controller
+		// and so the ExtendedDamonset will be defaulted.
+		// It is better to only update a Resource Kind from its own controller, to avoid conccurent update.
+		conditions.UpdateErrorCondition(newStatus, now, err, message)
+		err = r.updateReplicaSet(replicaSetInstance, newStatus)
+		return reconcile.Result{RequeueAfter: time.Second}, err
 	}
 
 	lastResyncTimeStampCond := conditions.GetExtendedDaemonSetReplicaSetStatusCondition(&replicaSetInstance.Status, datadoghqv1alpha1.ConditionTypeLastFullSync)
@@ -123,9 +131,14 @@ func (r *Reconciler) Reconcile(ctx context.Context, request reconcile.Request) (
 	conditions.UpdateExtendedDaemonSetReplicaSetStatusCondition(newStatus, now, datadoghqv1alpha1.ConditionTypeUnschedule, status, desc, false, false)
 
 	// start actions on pods
+	requeueAfter := 5 * time.Second
+	if daemonsetInstance.Spec.Strategy.ReconcileFrequency != nil {
+		requeueAfter = daemonsetInstance.Spec.Strategy.ReconcileFrequency.Duration
+	}
+
 	lastPodDeletionCondition := conditions.GetExtendedDaemonSetReplicaSetStatusCondition(newStatus, datadoghqv1alpha1.ConditionTypePodDeletion)
-	if lastPodDeletionCondition != nil && now.Sub(lastPodDeletionCondition.LastUpdateTime.Time) < 5*time.Second {
-		result.RequeueAfter = 5 * time.Second
+	if lastPodDeletionCondition != nil && now.Sub(lastPodDeletionCondition.LastUpdateTime.Time) < requeueAfter {
+		result.RequeueAfter = requeueAfter
 	} else {
 		errs = append(errs, deletePods(reqLogger, r.client, strategyParams.PodByNodeName, strategyResult.PodsToDelete)...)
 		if len(strategyResult.PodsToDelete) > 0 {
@@ -267,6 +280,7 @@ func (r *Reconciler) updateReplicaSet(replicaset *datadoghqv1alpha1.ExtendedDaem
 		newRS.Status = *newStatus
 		return r.client.Status().Update(context.TODO(), newRS)
 	}
+
 	return nil
 }