Merge release branch v0.8 (#143)

api/v1alpha1/const.go

@@ -24,10 +24,6 @@ const (
 	ExtendedDaemonSetCanaryPausedReasonAnnotationKey = "extendeddaemonset.datadoghq.com/canary-paused-reason"
 	// ExtendedDaemonSetCanaryUnpausedAnnotationKey annotation key used on ExtendedDaemonset in order to detect if a canary deployment is manually unpaused.
 	ExtendedDaemonSetCanaryUnpausedAnnotationKey = "extendeddaemonset.datadoghq.com/canary-unpaused"
-	// ExtendedDaemonSetCanaryFailedAnnotationKey annotation key used on ExtendedDaemonset in order to detect if a canary deployment has failed.
-	ExtendedDaemonSetCanaryFailedAnnotationKey = "extendeddaemonset.datadoghq.com/canary-failed"
-	// ExtendedDaemonSetCanaryFailedReasonAnnotationKey annotation key used on ExtendedDaemonset in order to provide a reason that the a canary deployment is failed.
-	ExtendedDaemonSetCanaryFailedReasonAnnotationKey = "extendeddaemonset.datadoghq.com/canary-failed-reason"
 	// ExtendedDaemonSetOldDaemonsetAnnotationKey annotation key used on ExtendedDaemonset in order to inform the controller that old Daemonset's pod.
 	// should be taken into consideration during the initial rolling-update.
 	ExtendedDaemonSetOldDaemonsetAnnotationKey = "extendeddaemonset.datadoghq.com/old-daemonset"

api/v1alpha1/extendeddaemonset_validate_test.go

@@ -7,6 +7,7 @@ package v1alpha1
 
 import (
 	"testing"
+	"time"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
@@ -50,6 +51,13 @@ func TestValidateExtendedDaemonSetSpec(t *testing.T) {
 	*validAutoPauseNoAutoFail.Strategy.Canary.AutoFail.Enabled = false
 	*validAutoPauseNoAutoFail.Strategy.Canary.AutoFail.MaxRestarts = 1
 
+	invalidCanaryTimeout := validWithCanary.DeepCopy()
+	*invalidCanaryTimeout.Strategy.Canary.AutoPause.Enabled = true
+	*invalidCanaryTimeout.Strategy.Canary.AutoFail.Enabled = true
+	invalidCanaryTimeout.Strategy.Canary.AutoFail.CanaryTimeout = &metav1.Duration{
+		Duration: 1 * time.Minute,
+	}
+
 	validManualValidationMode := validWithCanaryManualValidationMode.DeepCopy()
 	validManualValidationMode.Strategy.Canary.ValidationMode = ExtendedDaemonSetSpecStrategyCanaryValidationModeManual
 
@@ -83,6 +91,11 @@ func TestValidateExtendedDaemonSetSpec(t *testing.T) {
 			spec: invalidAutoFail,
 			err:  ErrInvalidAutoFailRestarts,
 		},
+		{
+			name: "invalid autoFail canaryTimeout",
+			spec: invalidCanaryTimeout,
+			err:  ErrInvalidCanaryTimeout,
+		},
 		{
 			name: "valid autoFail no autoPause",
 			spec: validAutoFailNoAutoPause,

bundle.Dockerfile

@@ -6,9 +6,9 @@ LABEL operators.operatorframework.io.bundle.manifests.v1=manifests/
 LABEL operators.operatorframework.io.bundle.metadata.v1=metadata/
 LABEL operators.operatorframework.io.bundle.package.v1=extendeddaemonset
 LABEL operators.operatorframework.io.bundle.channels.v1=alpha
-LABEL operators.operatorframework.io.metrics.mediatype.v1=metrics+v1
 LABEL operators.operatorframework.io.metrics.builder=operator-sdk-v1.4.0+git
 LABEL operators.operatorframework.io.metrics.project_layout=go.kubebuilder.io/v2
+LABEL operators.operatorframework.io.metrics.mediatype.v1=metrics+v1
 
 # Labels for testing.
 LABEL operators.operatorframework.io.test.mediatype.v1=scorecard+v1

bundle/manifests/datadoghq.com_extendeddaemonsets.yaml

@@ -105,27 +105,32 @@ spec:
                     autoFail:
                       description: ExtendedDaemonSetSpecStrategyCanaryAutoFail defines the canary deployment AutoFail parameters of the ExtendedDaemonSet.
                       properties:
+                        canaryTimeout:
+                          description: CanaryTimeout defines the maximum duration of a Canary, after which the Canary deployment is autofailed. This is a safeguard against lengthy Canary pauses. There is no default value.
+                          type: string
                         enabled:
+                          description: Enabled enables AutoFail. Default value is true.
                           type: boolean
                         maxRestarts:
-                          description: MaxRestarts defines the number of tolerable (per pod) Canary pod restarts after which the Canary deployment is autofailed.
+                          description: MaxRestarts defines the number of tolerable (per pod) Canary pod restarts after which the Canary deployment is autofailed. Default value is 5.
                           format: int32
                           type: integer
                         maxRestartsDuration:
-                          description: MaxRestartsDuration defines the maximum duration of tolerable Canary pod restarts after which the Canary deployment is autofailed.
+                          description: MaxRestartsDuration defines the maximum duration of tolerable Canary pod restarts after which the Canary deployment is autofailed. There is no default value.
                           type: string
                       type: object
                     autoPause:
                       description: ExtendedDaemonSetSpecStrategyCanaryAutoPause defines the canary deployment AutoPause parameters of the ExtendedDaemonSet.
                       properties:
                         enabled:
+                          description: Enabled enables AutoPause. Default value is true.
                           type: boolean
                         maxRestarts:
-                          description: MaxRestarts defines the number of tolerable (per pod) Canary pod restarts after which the Canary deployment is autopaused.
+                          description: MaxRestarts defines the number of tolerable (per pod) Canary pod restarts after which the Canary deployment is autopaused. Default value is 2.
                           format: int32
                           type: integer
                         maxSlowStartDuration:
-                          description: MaxSlowStartDuration defines the maximum slow start duration for a pod (stuck in Creating state) after which the. Canary deployment is autopaused
+                          description: MaxSlowStartDuration defines the maximum slow start duration for a pod (stuck in Creating state) after which the Canary deployment is autopaused. There is no default value.
                           type: string
                       type: object
                     duration:

bundle/manifests/extendeddaemonset.clusterserviceversion.yaml

@@ -73,7 +73,7 @@ metadata:
     capabilities: Full Lifecycle
     operators.operatorframework.io/builder: operator-sdk-v1.4.0+git
     operators.operatorframework.io/project_layout: go.kubebuilder.io/v2
-  name: extendeddaemonset.v0.7.0
+  name: extendeddaemonset.v0.8.0
   namespace: placeholder
 spec:
   apiservicedefinitions: {}
@@ -327,4 +327,4 @@ spec:
   provider:
     name: Datadog
     url: https://github.com/DataDog/extendeddaemonset
-  version: 0.7.0
+  version: 0.8.0

bundle/metadata/annotations.yaml

@@ -5,9 +5,9 @@ annotations:
   operators.operatorframework.io.bundle.metadata.v1: metadata/
   operators.operatorframework.io.bundle.package.v1: extendeddaemonset
   operators.operatorframework.io.bundle.channels.v1: alpha
-  operators.operatorframework.io.metrics.mediatype.v1: metrics+v1
   operators.operatorframework.io.metrics.builder: operator-sdk-v1.4.0+git
   operators.operatorframework.io.metrics.project_layout: go.kubebuilder.io/v2
+  operators.operatorframework.io.metrics.mediatype.v1: metrics+v1
 
   # Annotations for testing.
   operators.operatorframework.io.test.mediatype.v1: scorecard+v1

controllers/extendeddaemonset/controller.go

@@ -238,7 +238,7 @@ func (r *Reconciler) updateInstanceWithCurrentRS(logger logr.Logger, now time.Ti
 	if daemonset.Spec.Strategy.Canary != nil {
 		metaNow := metav1.NewTime(now)
 		isCanaryPaused, pausedReason := IsCanaryDeploymentPaused(daemonset.GetAnnotations(), upToDate)
-		isCanaryFailed := IsCanaryDeploymentFailed(daemonset.GetAnnotations(), upToDate)
+		isCanaryFailed := IsCanaryDeploymentFailed(upToDate)
 		isCanaryActive := isCanaryActive(daemonset, current.GetName(), upToDate.GetName(), isCanaryFailed)
 		logger.V(1).Info("canary state", "isCanaryActive", isCanaryActive, "isCanaryFailed", isCanaryFailed, "isCanaryPaused", isCanaryPaused, "pausedReason", pausedReason)
 
@@ -415,7 +415,9 @@ func (r *Reconciler) selectNodes(logger logr.Logger, daemonsetSpec *datadoghqv1a
 				antiAffinityKeysValues[antiAffinityKeysValue]++
 			}
 
-			currentNodes = append(currentNodes, node.Name)
+			if scheduler.CheckNodeFitness(logger, newPod, &node) {
+				currentNodes = append(currentNodes, node.Name)
+			}
 			// All nodes are found. We can exit now!
 			if len(currentNodes) == nbCanaryPod {
 				logger.V(1).Info("All nodes were found")
@@ -538,7 +540,7 @@ func newReplicaSetFromInstance(daemonset *datadoghqv1alpha1.ExtendedDaemonSet) (
 	return rs, err
 }
 
-func (r *Reconciler) cleanupReplicaSet(logger logr.Logger, now time.Time, rsList *datadoghqv1alpha1.ExtendedDaemonSetReplicaSetList, current, updatetodate *datadoghqv1alpha1.ExtendedDaemonSetReplicaSet) error {
+func (r *Reconciler) cleanupReplicaSet(logger logr.Logger, now time.Time, rsList *datadoghqv1alpha1.ExtendedDaemonSetReplicaSetList, current, upToDate *datadoghqv1alpha1.ExtendedDaemonSetReplicaSet) error {
 	var errs []error
 	for id, rs := range rsList.Items {
 		if current == nil {
@@ -547,7 +549,7 @@ func (r *Reconciler) cleanupReplicaSet(logger logr.Logger, now time.Time, rsList
 		if rs.Name == current.Name {
 			continue
 		}
-		if updatetodate != nil && rs.Name == updatetodate.Name {
+		if upToDate != nil && rs.Name == upToDate.Name {
 			continue
 		}
 		if rs.DeletionTimestamp != nil {
@@ -591,8 +593,6 @@ func clearCanaryAnnotations(eds *datadoghqv1alpha1.ExtendedDaemonSet) bool {
 		datadoghqv1alpha1.ExtendedDaemonSetCanaryPausedAnnotationKey,
 		datadoghqv1alpha1.ExtendedDaemonSetCanaryPausedReasonAnnotationKey,
 		datadoghqv1alpha1.ExtendedDaemonSetCanaryUnpausedAnnotationKey,
-		datadoghqv1alpha1.ExtendedDaemonSetCanaryFailedAnnotationKey,
-		datadoghqv1alpha1.ExtendedDaemonSetCanaryFailedReasonAnnotationKey,
 	}
 	var updated bool
 	for _, key := range keysToDelete {

controllers/extendeddaemonset/controller_test.go

@@ -710,17 +710,11 @@ func TestReconcileExtendedDaemonSet_updateInstanceWithCurrentRS(t *testing.T) {
 		}
 	}
 	daemonsetWithCanaryFailedOldStatus := daemonsetWithCanaryFailedOldWithoutAnnotationsStatus.DeepCopy()
-	{
-		daemonsetWithCanaryFailedOldStatus.Annotations[datadoghqv1alpha1.ExtendedDaemonSetCanaryFailedAnnotationKey] = "true"
-	}
-
 	daemonsetWithCanaryFailedWithoutAnnotationsWanted := daemonsetWithCanaryFailedOldStatus.DeepCopy()
 	{
 		// When the canary fails, the number of "Updated" replicas should equal
 		// the number of current ones.
 		daemonsetWithCanaryFailedWithoutAnnotationsWanted.Status.UpToDate = replicassetCurrent.Status.Current
-		delete(daemonsetWithCanaryFailedWithoutAnnotationsWanted.Annotations, datadoghqv1alpha1.ExtendedDaemonSetCanaryFailedAnnotationKey)
-		delete(daemonsetWithCanaryFailedWithoutAnnotationsWanted.Annotations, datadoghqv1alpha1.ExtendedDaemonSetCanaryFailedReasonAnnotationKey)
 		daemonsetWithCanaryFailedWithoutAnnotationsWanted.ResourceVersion = "3"
 		daemonsetWithCanaryFailedWithoutAnnotationsWanted.Status.Canary = nil
 		daemonsetWithCanaryFailedWithoutAnnotationsWanted.Status.State = datadoghqv1alpha1.ExtendedDaemonSetStatusStateCanaryFailed
@@ -885,28 +879,6 @@ func TestReconcileExtendedDaemonSet_updateInstanceWithCurrentRS(t *testing.T) {
 			wantResult: reconcile.Result{Requeue: false},
 			wantErr:    false,
 		},
-		{
-			now:  now,
-			name: "canary failed => update",
-			fields: fields{
-				client: fake.NewClientBuilder().WithObjects(daemonsetWithCanaryFailedOldStatus, replicassetCurrent, replicassetUpToDate).Build(),
-				scheme: s,
-			},
-			args: args{
-				logger:    log,
-				daemonset: daemonsetWithCanaryFailedOldStatus,
-				current:   replicassetCurrent,
-				upToDate:  replicassetUpToDate,
-				podsCounter: podsCounterType{
-					Current:   3,
-					Ready:     2,
-					Available: 1,
-				},
-			},
-			want:       daemonsetWithCanaryFailedWithoutAnnotationsWanted,
-			wantResult: reconcile.Result{Requeue: false},
-			wantErr:    false,
-		},
 		{
 			now:  now,
 			name: "canary failed, ers-condition-failed => update",

controllers/extendeddaemonset/utils.go

@@ -104,16 +104,11 @@ func IsCanaryDeploymentValid(dsAnnotations map[string]string, rsName string) boo
 }
 
 // IsCanaryDeploymentFailed checks if the Canary deployment has been failed.
-func IsCanaryDeploymentFailed(dsAnnotations map[string]string, ers *datadoghqv1alpha1.ExtendedDaemonSetReplicaSet) bool {
+func IsCanaryDeploymentFailed(ers *datadoghqv1alpha1.ExtendedDaemonSetReplicaSet) bool {
 	// Check ERS status to detect if a Canary failed
 	if ers != nil && conditions.IsConditionTrue(&ers.Status, datadoghqv1alpha1.ConditionTypeCanaryFailed) {
 		return true
 	}
 
-	// Check also failed annotations if a user wanted to force a canary failure
-	if value, found := dsAnnotations[datadoghqv1alpha1.ExtendedDaemonSetCanaryFailedAnnotationKey]; found {
-		return value == datadoghqv1alpha1.ValueStringTrue
-	}
-
 	return false
 }

controllers/extendeddaemonset/utils_test.go

@@ -12,6 +12,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
 	datadoghqv1alpha1 "github.com/DataDog/extendeddaemonset/api/v1alpha1"
+	"github.com/DataDog/extendeddaemonset/controllers/extendeddaemonsetreplicaset/conditions"
 )
 
 func TestIsCanaryDeploymentPaused(t *testing.T) {
@@ -282,8 +283,10 @@ func TestIsCanaryDeploymentValid(t *testing.T) {
 }
 
 func TestIsCanaryDeploymentFailed(t *testing.T) {
+	now := time.Now()
+
 	type args struct {
-		dsAnnotations map[string]string
+		rs *datadoghqv1alpha1.ExtendedDaemonSetReplicaSet
 	}
 
 	tests := []struct {
@@ -292,45 +295,56 @@ func TestIsCanaryDeploymentFailed(t *testing.T) {
 		want bool
 	}{
 		{
-			name: "annotation found - correct rs name",
+			name: "nil RS",
 			args: args{
-				dsAnnotations: map[string]string{
-					"extendeddaemonset.datadoghq.com/canary-failed": "true",
-				},
-			},
-			want: true,
-		},
-		{
-			name: "annotation found - incorrect rs name",
-			args: args{
-				dsAnnotations: map[string]string{
-					"extendeddaemonset.datadoghq.com/canary-failed": "false",
-				},
+				rs: &datadoghqv1alpha1.ExtendedDaemonSetReplicaSet{},
 			},
 			want: false,
 		},
 		{
-			name: "annotation not found",
+			name: "rs not failed",
 			args: args{
-				dsAnnotations: map[string]string{
-					"extendeddaemonset.datadoghq.com/canary-failed": "random",
+				rs: &datadoghqv1alpha1.ExtendedDaemonSetReplicaSet{
+					ObjectMeta: metav1.ObjectMeta{
+						CreationTimestamp: metav1.NewTime(now.Add(-2 * time.Hour)),
+					},
+					Status: datadoghqv1alpha1.ExtendedDaemonSetReplicaSetStatus{
+						Conditions: []datadoghqv1alpha1.ExtendedDaemonSetReplicaSetCondition{
+							{
+								Type:           datadoghqv1alpha1.ConditionTypeCanaryFailed,
+								LastUpdateTime: metav1.NewTime(now.Add(-15 * time.Minute)),
+								Status:         conditions.BoolToCondition(false),
+							},
+						},
+					},
 				},
 			},
 			want: false,
 		},
 		{
-			name: "annotation not found",
+			name: "rs failed",
 			args: args{
-				dsAnnotations: map[string]string{
-					"extendeddaemonset.datadoghq.com/canary-something-else": "true",
+				rs: &datadoghqv1alpha1.ExtendedDaemonSetReplicaSet{
+					ObjectMeta: metav1.ObjectMeta{
+						CreationTimestamp: metav1.NewTime(now.Add(-2 * time.Hour)),
+					},
+					Status: datadoghqv1alpha1.ExtendedDaemonSetReplicaSetStatus{
+						Conditions: []datadoghqv1alpha1.ExtendedDaemonSetReplicaSetCondition{
+							{
+								Type:           datadoghqv1alpha1.ConditionTypeCanaryFailed,
+								LastUpdateTime: metav1.NewTime(now),
+								Status:         conditions.BoolToCondition(true),
+							},
+						},
+					},
 				},
 			},
-			want: false,
+			want: true,
 		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			if got := IsCanaryDeploymentFailed(tt.args.dsAnnotations, nil); got != tt.want {
+			if got := IsCanaryDeploymentFailed(tt.args.rs); got != tt.want {
 				t.Errorf("IsCanaryDeploymentFailed() = %v, want %v", got, tt.want)
 			}
 		})

controllers/extendeddaemonset_e2e_test.go

@@ -221,11 +221,6 @@ var _ = Describe("ExtendedDaemonSet e2e updates and recovery", func() {
 			Eventually(withEDS(key, eds, func() bool {
 				return eds.Status.State == datadoghqv1alpha1.ExtendedDaemonSetStatusStateRunning
 			}), timeout, interval).Should(BeTrue())
-
-			Eventually(withEDS(key, eds, func() bool {
-				_, found := eds.Annotations[datadoghqv1alpha1.ExtendedDaemonSetCanaryFailedAnnotationKey]
-				return found
-			}), timeout, interval).Should(BeFalse())
 		})
 
 		It("Should delete EDS", func() {
@@ -798,8 +793,6 @@ func clearCanaryAnnotations(eds *datadoghqv1alpha1.ExtendedDaemonSet) bool {
 		datadoghqv1alpha1.ExtendedDaemonSetCanaryPausedAnnotationKey,
 		datadoghqv1alpha1.ExtendedDaemonSetCanaryPausedReasonAnnotationKey,
 		datadoghqv1alpha1.ExtendedDaemonSetCanaryUnpausedAnnotationKey,
-		datadoghqv1alpha1.ExtendedDaemonSetCanaryFailedAnnotationKey,
-		datadoghqv1alpha1.ExtendedDaemonSetCanaryFailedReasonAnnotationKey,
 	}
 	var updated bool
 	for _, key := range keysToDelete {

controllers/extendeddaemonsetreplicaset/controller.go

@@ -223,11 +223,6 @@ func (r *Reconciler) applyStrategy(logger logr.Logger, daemonset *datadoghqv1alp
 		conditions.UpdateExtendedDaemonSetReplicaSetStatusCondition(strategyParams.NewStatus, now, datadoghqv1alpha1.ConditionTypeActive, corev1.ConditionFalse, "", "", false, false)
 		logger.Info("manage canary deployment")
 		strategyResult, err = strategy.ManageCanaryDeployment(r.client, daemonset, strategyParams)
-	case strategy.ReplicaSetStatusCanaryFailed:
-		logger.Info("manage canary failed deployment")
-		strategyResult = &strategy.Result{
-			NewStatus: strategyParams.NewStatus.DeepCopy(),
-		}
 	case strategy.ReplicaSetStatusUnknown:
 		conditions.UpdateExtendedDaemonSetReplicaSetStatusCondition(strategyParams.NewStatus, now, datadoghqv1alpha1.ConditionTypeCanary, corev1.ConditionFalse, "", "", false, false)
 		conditions.UpdateExtendedDaemonSetReplicaSetStatusCondition(strategyParams.NewStatus, now, datadoghqv1alpha1.ConditionTypeActive, corev1.ConditionFalse, "", "", false, false)
@@ -460,6 +455,9 @@ func retrieveOwnerReference(obj *datadoghqv1alpha1.ExtendedDaemonSetReplicaSet)
 	return "", fmt.Errorf("unable to retrieve the owner reference name")
 }
 
+// This method has the effect of deciding if the ERS should be ignored or something should be updated. Note that this
+// never returns strategy.ReplicaSetStatusCanaryFailed because that case does not need to be managed (if it is Failed, it has
+// already been processed).
 func retrieveReplicaSetStatus(daemonset *datadoghqv1alpha1.ExtendedDaemonSet, replicassetName string) strategy.ReplicaSetStatus {
 	switch daemonset.Status.ActiveReplicaSet {
 	case "":

controllers/extendeddaemonsetreplicaset/strategy/canary.go

@@ -48,7 +48,8 @@ func manageCanaryStatus(annotations map[string]string, params *Parameters, now t
 	result.NewStatus = params.NewStatus.DeepCopy()
 	result.NewStatus.Status = string(ReplicaSetStatusCanary)
 
-	result.IsFailed = eds.IsCanaryDeploymentFailed(annotations, params.Replicaset)
+	// TODO: these are set here as well as in manageCanaryPodFailures. Consider simplifying.
+	result.IsFailed = eds.IsCanaryDeploymentFailed(params.Replicaset)
 	result.IsPaused, result.PausedReason = eds.IsCanaryDeploymentPaused(annotations, params.Replicaset)
 	result.IsUnpaused = eds.IsCanaryDeploymentUnpaused(annotations)