Merge pull request #529 from weaveworks/fix-daemonset-ready-cond

pkg/canary/daemonset: fix ready condition according to kubectl
fluxcd · Mar 27, 2020 · 61fd505 · 61fd505
2 parents 7821bc6 + 4242bf0
commit 61fd505
Show file tree

Hide file tree

Showing 3 changed files with 67 additions and 37 deletions.
diff --git a/go.mod b/go.mod
@@ -7,6 +7,7 @@ require (
 	github.com/aws/aws-sdk-go v1.29.29
 	github.com/davecgh/go-spew v1.1.1
 	github.com/google/go-cmp v0.4.0
+	github.com/pkg/errors v0.9.1
 	github.com/prometheus/client_golang v1.5.1
 	github.com/stretchr/testify v1.5.1
 	go.uber.org/zap v1.14.1

diff --git a/pkg/canary/daemonset_ready.go b/pkg/canary/daemonset_ready.go
@@ -44,21 +44,31 @@ func (c *DaemonSetController) IsCanaryReady(cd *flaggerv1.Canary) (bool, error)
 }
 
 // isDaemonSetReady determines if a daemonset is ready by checking the number of old version daemons
+// reference: https://github.com/kubernetes/kubernetes/blob/5232ad4a00ec93942d0b2c6359ee6cd1201b46bc/pkg/kubectl/rollout_status.go#L110
 func (c *DaemonSetController) isDaemonSetReady(cd *flaggerv1.Canary, daemonSet *appsv1.DaemonSet) (bool, error) {
-	if diff := daemonSet.Status.DesiredNumberScheduled - daemonSet.Status.UpdatedNumberScheduled; diff > 0 || daemonSet.Status.NumberUnavailable > 0 {
+	if daemonSet.Generation <= daemonSet.Status.ObservedGeneration {
+		// calculate conditions
+		newCond := daemonSet.Status.UpdatedNumberScheduled < daemonSet.Status.DesiredNumberScheduled
+		availableCond := daemonSet.Status.NumberAvailable < daemonSet.Status.DesiredNumberScheduled
+		if !newCond && !availableCond {
+			return true, nil
+		}
+
+		// check if deadline exceeded
 		from := cd.Status.LastTransitionTime
 		delta := time.Duration(cd.GetProgressDeadlineSeconds()) * time.Second
-		dl := from.Add(delta)
-		if dl.Before(time.Now()) {
+		if from.Add(delta).Before(time.Now()) {
 			return false, fmt.Errorf("exceeded its progressDeadlineSeconds: %d", cd.GetProgressDeadlineSeconds())
-		} else {
-			return true, fmt.Errorf(
-				"waiting for rollout to finish: desiredNumberScheduled=%d, updatedNumberScheduled=%d, numberUnavailable=%d",
-				daemonSet.Status.DesiredNumberScheduled,
-				daemonSet.Status.UpdatedNumberScheduled,
-				daemonSet.Status.NumberUnavailable,
-			)
+		}
+
+		// retryable
+		if newCond {
+			return true, fmt.Errorf("waiting for rollout to finish: %d out of %d new pods have been updated",
+				daemonSet.Status.UpdatedNumberScheduled, daemonSet.Status.DesiredNumberScheduled)
+		} else if availableCond {
+			return true, fmt.Errorf("waiting for rollout to finish: %d of %d updated pods are available",
+				daemonSet.Status.NumberAvailable, daemonSet.Status.DesiredNumberScheduled)
 		}
 	}
-	return true, nil
+	return true, fmt.Errorf("waiting for rollout to finish: observed daemonset generation less then desired generation")
 }
diff --git a/pkg/canary/daemonset_ready_test.go b/pkg/canary/daemonset_ready_test.go
@@ -1,6 +1,7 @@
 package canary
 
 import (
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -24,39 +25,57 @@ func TestDaemonSetController_IsReady(t *testing.T) {
 }
 
 func TestDaemonSetController_isDaemonSetReady(t *testing.T) {
-	ds := &appsv1.DaemonSet{
-		Status: appsv1.DaemonSetStatus{
-			DesiredNumberScheduled: 1,
-			UpdatedNumberScheduled: 1,
-		},
-	}
-
+	mocks := newDaemonSetFixture()
 	cd := &flaggerv1.Canary{}
-	cd.Spec.ProgressDeadlineSeconds = int32p(1e5)
-	cd.Status.LastTransitionTime = metav1.Now()
 
-	// ready
-	mocks := newDaemonSetFixture()
-	_, err := mocks.controller.isDaemonSetReady(cd, ds)
+	// observed generation is less than desired generation
+	ds := &appsv1.DaemonSet{Status: appsv1.DaemonSetStatus{}}
+	ds.Status.ObservedGeneration--
+	retyable, err := mocks.controller.isDaemonSetReady(cd, ds)
+	require.Error(t, err)
+	require.True(t, retyable)
+
+	// succeeded
+	ds = &appsv1.DaemonSet{Status: appsv1.DaemonSetStatus{
+		UpdatedNumberScheduled: 1,
+		DesiredNumberScheduled: 1,
+		NumberAvailable:        1,
+	}}
+	retyable, err = mocks.controller.isDaemonSetReady(cd, ds)
 	require.NoError(t, err)
+	require.True(t, retyable)
 
-	// not ready but retriable
-	ds.Status.NumberUnavailable++
-	retrieable, err := mocks.controller.isDaemonSetReady(cd, ds)
+	// deadline exceeded
+	ds = &appsv1.DaemonSet{Status: appsv1.DaemonSetStatus{
+		UpdatedNumberScheduled: 0,
+		DesiredNumberScheduled: 1,
+	}}
+	cd.Status.LastTransitionTime = metav1.Now()
+	cd.Spec.ProgressDeadlineSeconds = int32p(-1e6)
+	retyable, err = mocks.controller.isDaemonSetReady(cd, ds)
 	require.Error(t, err)
-	require.True(t, retrieable)
-	ds.Status.NumberUnavailable--
+	require.False(t, retyable)
 
-	ds.Status.DesiredNumberScheduled++
-	retrieable, err = mocks.controller.isDaemonSetReady(cd, ds)
+	// only newCond not satisfied
+	ds = &appsv1.DaemonSet{Status: appsv1.DaemonSetStatus{
+		UpdatedNumberScheduled: 0,
+		DesiredNumberScheduled: 1,
+		NumberAvailable:        1,
+	}}
+	cd.Spec.ProgressDeadlineSeconds = int32p(1e6)
+	retyable, err = mocks.controller.isDaemonSetReady(cd, ds)
 	require.Error(t, err)
-	require.True(t, retrieable)
+	require.True(t, retyable)
+	require.True(t, strings.Contains(err.Error(), "new pods"))
 
-	// not ready and not retriable
-	cd.Status.LastTransitionTime = metav1.Now()
-	cd.Spec.ProgressDeadlineSeconds = int32p(-1e5)
-	retrieable, err = mocks.controller.isDaemonSetReady(cd, ds)
+	// only availableCond not satisfied
+	ds = &appsv1.DaemonSet{Status: appsv1.DaemonSetStatus{
+		UpdatedNumberScheduled: 1,
+		DesiredNumberScheduled: 1,
+		NumberAvailable:        0,
+	}}
+	retyable, err = mocks.controller.isDaemonSetReady(cd, ds)
 	require.Error(t, err)
-	require.False(t, retrieable)
-
+	require.True(t, retyable)
+	require.True(t, strings.Contains(err.Error(), "available"))
 }