address comments

pkg/controllers/work/applied_work_syncer_test.go

@@ -185,21 +185,21 @@ func TestCalculateNewAppliedWork(t *testing.T) {
 			}
 			newRes, staleRes, err := r.generateDiff(context.Background(), &tt.inputWork, &tt.inputAppliedWork)
 			if len(tt.expectedNewRes) != len(newRes) {
-				t.Errorf("Testcase %s: get newRes contains different number of elements than the expected newRes.", testName)
+				t.Errorf("Testcase %s: get newRes contains different number of elements than the want newRes.", testName)
 			}
 			for i := 0; i < len(newRes); i++ {
 				diff := cmp.Diff(tt.expectedNewRes[i].WorkResourceIdentifier, newRes[i].WorkResourceIdentifier)
 				if len(diff) != 0 {
-					t.Errorf("Testcase %s: get newRes is different from the expected newRes, diff = %s", testName, diff)
+					t.Errorf("Testcase %s: get newRes is different from the want newRes, diff = %s", testName, diff)
 				}
 			}
 			if len(tt.expectedStaleRes) != len(staleRes) {
-				t.Errorf("Testcase %s: get staleRes contains different number of elements than the expected staleRes.", testName)
+				t.Errorf("Testcase %s: get staleRes contains different number of elements than the want staleRes.", testName)
 			}
 			for i := 0; i < len(staleRes); i++ {
 				diff := cmp.Diff(tt.expectedStaleRes[i].WorkResourceIdentifier, staleRes[i].WorkResourceIdentifier)
 				if len(diff) != 0 {
-					t.Errorf("Testcase %s: get staleRes is different from the expected staleRes, diff = %s", testName, diff)
+					t.Errorf("Testcase %s: get staleRes is different from the want staleRes, diff = %s", testName, diff)
 				}
 			}
 			if tt.hasErr {

pkg/controllers/work/apply_controller_helper_test.go

@@ -20,6 +20,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 
 	fleetv1beta1 "go.goms.io/fleet/apis/placement/v1beta1"
+	"go.goms.io/fleet/pkg/utils/condition"
 )
 
 // createWorkWithManifest creates a work given a manifest
@@ -95,11 +96,12 @@ func waitForWorkToBeAvailable(workName, workNS string) *fleetv1beta1.Work {
 			return false
 		}
 		applyCond := meta.FindStatusCondition(resultWork.Status.Conditions, fleetv1beta1.WorkConditionTypeAvailable)
-		if applyCond == nil || applyCond.Status != metav1.ConditionTrue || applyCond.ObservedGeneration != resultWork.Generation {
+		if !condition.IsConditionStatusTrue(applyCond, resultWork.Generation) {
 			return false
 		}
 		for _, manifestCondition := range resultWork.Status.ManifestConditions {
-			if !meta.IsStatusConditionTrue(manifestCondition.Conditions, fleetv1beta1.WorkConditionTypeAvailable) {
+			applyCond := meta.FindStatusCondition(manifestCondition.Conditions, fleetv1beta1.WorkConditionTypeAvailable)
+			if !condition.IsConditionStatusTrue(applyCond, resultWork.Generation) {
 				return false
 			}
 		}

pkg/controllers/work/apply_controller_integration_test.go

@@ -42,7 +42,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 
 	fleetv1beta1 "go.goms.io/fleet/apis/placement/v1beta1"
-	"go.goms.io/fleet/pkg/utils/condition"
+	"go.goms.io/fleet/test/utils/controller"
 )
 
 const timeout = time.Second * 10
@@ -120,7 +120,7 @@ var _ = Describe("Work Controller", func() {
 					Reason: string(manifestNotTrackableAction),
 				},
 			}
-			Expect(condition.CompareConditions(expected, resultWork.Status.ManifestConditions[0].Conditions)).Should(BeEmpty())
+			Expect(controller.CompareConditions(expected, resultWork.Status.ManifestConditions[0].Conditions)).Should(BeEmpty())
 			expected = []metav1.Condition{
 				{
 					Type:   fleetv1beta1.WorkConditionTypeApplied,
@@ -133,7 +133,7 @@ var _ = Describe("Work Controller", func() {
 					Reason: workNotTrackableReason,
 				},
 			}
-			Expect(condition.CompareConditions(expected, resultWork.Status.Conditions)).Should(BeEmpty())
+			Expect(controller.CompareConditions(expected, resultWork.Status.Conditions)).Should(BeEmpty())
 
 			By("Check applied config map")
 			var configMap corev1.ConfigMap

pkg/controllers/work/apply_controller_test.go

@@ -53,7 +53,8 @@ import (
 
 	fleetv1beta1 "go.goms.io/fleet/apis/placement/v1beta1"
 	"go.goms.io/fleet/pkg/utils"
-	"go.goms.io/fleet/pkg/utils/condition"
+	"go.goms.io/fleet/pkg/utils/controller"
+	controller2 "go.goms.io/fleet/test/utils/controller"
 )
 
 var (
@@ -286,14 +287,14 @@ func TestIsManifestManagedByWork(t *testing.T) {
 
 func TestBuildManifestCondition(t *testing.T) {
 	tests := map[string]struct {
-		err      error
-		action   applyAction
-		expected []metav1.Condition
+		err    error
+		action applyAction
+		want   []metav1.Condition
 	}{
 		"TestNoErrorManifestCreated": {
 			err:    nil,
 			action: manifestCreatedAction,
-			expected: []metav1.Condition{
+			want: []metav1.Condition{
 				{
 					Type:   fleetv1beta1.WorkConditionTypeApplied,
 					Status: metav1.ConditionTrue,
@@ -309,7 +310,7 @@ func TestBuildManifestCondition(t *testing.T) {
 		"TestNoErrorManifestServerSideApplied": {
 			err:    nil,
 			action: manifestServerSideAppliedAction,
-			expected: []metav1.Condition{
+			want: []metav1.Condition{
 				{
 					Type:   fleetv1beta1.WorkConditionTypeApplied,
 					Status: metav1.ConditionTrue,
@@ -325,7 +326,7 @@ func TestBuildManifestCondition(t *testing.T) {
 		"TestNoErrorManifestThreeWayMergePatch": {
 			err:    nil,
 			action: manifestThreeWayMergePatchAction,
-			expected: []metav1.Condition{
+			want: []metav1.Condition{
 				{
 					Type:   fleetv1beta1.WorkConditionTypeApplied,
 					Status: metav1.ConditionTrue,
@@ -341,7 +342,7 @@ func TestBuildManifestCondition(t *testing.T) {
 		"TestNoErrorManifestNotAvailable": {
 			err:    nil,
 			action: manifestNotAvailableYetAction,
-			expected: []metav1.Condition{
+			want: []metav1.Condition{
 				{
 					Type:   fleetv1beta1.WorkConditionTypeApplied,
 					Status: metav1.ConditionTrue,
@@ -357,7 +358,7 @@ func TestBuildManifestCondition(t *testing.T) {
 		"TestNoErrorManifestNotTrackableAction": {
 			err:    nil,
 			action: manifestNotTrackableAction,
-			expected: []metav1.Condition{
+			want: []metav1.Condition{
 				{
 					Type:   fleetv1beta1.WorkConditionTypeApplied,
 					Status: metav1.ConditionTrue,
@@ -373,7 +374,7 @@ func TestBuildManifestCondition(t *testing.T) {
 		"TestNoErrorManifestAvailableAction": {
 			err:    nil,
 			action: manifestAvailableAction,
-			expected: []metav1.Condition{
+			want: []metav1.Condition{
 				{
 					Type:   fleetv1beta1.WorkConditionTypeApplied,
 					Status: metav1.ConditionTrue,
@@ -389,7 +390,7 @@ func TestBuildManifestCondition(t *testing.T) {
 		"TestWithError": {
 			err:    errors.New("test error"),
 			action: errorApplyAction,
-			expected: []metav1.Condition{
+			want: []metav1.Condition{
 				{
 					Type:   fleetv1beta1.WorkConditionTypeApplied,
 					Status: metav1.ConditionFalse,
@@ -407,7 +408,7 @@ func TestBuildManifestCondition(t *testing.T) {
 	for name, tt := range tests {
 		t.Run(name, func(t *testing.T) {
 			conditions := buildManifestCondition(tt.err, tt.action, 1)
-			diff := condition.CompareConditions(tt.expected, conditions)
+			diff := controller2.CompareConditions(tt.want, conditions)
 			assert.Empty(t, diff, "buildManifestCondition() test %v failed, (-want +got):\n%s", name, diff)
 		})
 	}
@@ -772,7 +773,7 @@ func TestGenerateWorkCondition(t *testing.T) {
 	for name, tt := range tests {
 		t.Run(name, func(t *testing.T) {
 			conditions := buildWorkCondition(tt.manifestConditions, 1)
-			diff := condition.CompareConditions(tt.expected, conditions)
+			diff := controller2.CompareConditions(tt.expected, conditions)
 			assert.Empty(t, diff, "buildWorkCondition() test %v failed, (-want +got):\n%s", name, diff)
 		})
 	}
@@ -785,6 +786,31 @@ func TestTrackResourceAvailability(t *testing.T) {
 		expected applyAction
 		err      error
 	}{
+		"Test a mal-formated object": {
+			gvr: utils.DeploymentGVR,
+			obj: &unstructured.Unstructured{
+				Object: map[string]interface{}{
+					"apiVersion": "apps/v1",
+					"kind":       "Deployment",
+					"metadata": map[string]interface{}{
+						"generation": 1,
+						"name":       "test-deployment",
+					},
+					"spec": "wrongspec",
+					"status": map[string]interface{}{
+						"observedGeneration": 1,
+						"conditions": []interface{}{
+							map[string]interface{}{
+								"type":   "Available",
+								"status": "True",
+							},
+						},
+					},
+				},
+			},
+			expected: errorApplyAction,
+			err:      controller.ErrUnexpectedBehavior,
+		},
 		"Test Deployment available": {
 			gvr: utils.DeploymentGVR,
 			obj: &unstructured.Unstructured{
@@ -905,6 +931,30 @@ func TestTrackResourceAvailability(t *testing.T) {
 			expected: manifestNotAvailableYetAction,
 			err:      nil,
 		},
+		"Test StatefulSet observed old generation": {
+			gvr: utils.StatefulSettGVR,
+			obj: &unstructured.Unstructured{
+				Object: map[string]interface{}{
+					"apiVersion": "apps/v1",
+					"kind":       "StatefulSet",
+					"metadata": map[string]interface{}{
+						"generation": 3,
+						"name":       "test-statefulset",
+					},
+					"spec": map[string]interface{}{
+						"replicas": 3,
+					},
+					"status": map[string]interface{}{
+						"observedGeneration": 2,
+						"availableReplicas":  2,
+						"currentReplicas":    3,
+						"updatedReplicas":    3,
+					},
+				},
+			},
+			expected: manifestNotAvailableYetAction,
+			err:      nil,
+		},
 		"Test DaemonSet Available": {
 			gvr: utils.DaemonSettGVR,
 			obj: &unstructured.Unstructured{
@@ -947,6 +997,27 @@ func TestTrackResourceAvailability(t *testing.T) {
 			expected: manifestNotAvailableYetAction,
 			err:      nil,
 		},
+		"Test DaemonSet not observe current generation": {
+			gvr: utils.DaemonSettGVR,
+			obj: &unstructured.Unstructured{
+				Object: map[string]interface{}{
+					"apiVersion": "apps/v1",
+					"kind":       "DaemonSet",
+					"metadata": map[string]interface{}{
+						"generation": 2,
+					},
+					"status": map[string]interface{}{
+						"observedGeneration":     1,
+						"numberAvailable":        0,
+						"desiredNumberScheduled": 1,
+						"currentNumberScheduled": 1,
+						"updatedNumberScheduled": 1,
+					},
+				},
+			},
+			expected: manifestNotAvailableYetAction,
+			err:      nil,
+		},
 		"Test Job available with succeeded pod": {
 			gvr: utils.JobGVR,
 			obj: &unstructured.Unstructured{
@@ -1020,7 +1091,7 @@ func TestTrackResourceAvailability(t *testing.T) {
 		t.Run(name, func(t *testing.T) {
 			action, err := trackResourceAvailability(tt.gvr, tt.obj)
 			assert.Equal(t, tt.expected, action, "action not matching in test %s", name)
-			assert.Equal(t, tt.err, err, "applyErr not matching in test %s", name)
+			assert.Equal(t, errors.Is(err, tt.err), true, "applyErr not matching in test %s", name)
 		})
 	}
 }

pkg/controllers/workgenerator/controller.go

@@ -662,10 +662,9 @@ func (r *Reconciler) SetupWithManager(mgr controllerruntime.Manager) error {
 				oldAvailableStatus := meta.FindStatusCondition(oldWork.Status.Conditions, fleetv1beta1.WorkConditionTypeAvailable)
 				newAvailableStatus := meta.FindStatusCondition(newWork.Status.Conditions, fleetv1beta1.WorkConditionTypeAvailable)
 
-				// we only need to handle the case the applied or available conditions is flipped between true and NOT true between the
+				// we only need to handle the case the applied or available condition is changed between the
 				// new and old work objects. Otherwise, it won't affect the binding applied condition
-				if condition.IsConditionStatusTrue(oldAppliedStatus, oldWork.GetGeneration()) == condition.IsConditionStatusTrue(newAppliedStatus, newWork.GetGeneration()) &&
-					condition.IsConditionStatusTrue(oldAvailableStatus, oldWork.GetGeneration()) == condition.IsConditionStatusTrue(newAvailableStatus, newWork.GetGeneration()) {
+				if condition.EqualCondition(oldAppliedStatus, newAppliedStatus) && condition.EqualCondition(oldAvailableStatus, newAvailableStatus) {
 					klog.V(2).InfoS("The work applied or available condition didn't flip between true and false, no need to reconcile", "oldWork", klog.KObj(oldWork), "newWork", klog.KObj(newWork))
 					return
 				}

pkg/utils/condition/condition.go

@@ -7,10 +7,6 @@ Licensed under the MIT license.
 package condition
 
 import (
-	"sort"
-
-	"github.com/google/go-cmp/cmp"
-	"github.com/google/go-cmp/cmp/cmpopts"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
@@ -53,16 +49,3 @@ func IsConditionStatusTrue(cond *metav1.Condition, latestGeneration int64) bool
 func IsConditionStatusFalse(cond *metav1.Condition, latestGeneration int64) bool {
 	return cond != nil && cond.Status == metav1.ConditionFalse && cond.ObservedGeneration == latestGeneration
 }
-
-// CompareConditions compares two condition slices and returns a string with the differences.
-func CompareConditions(wantConditions, gotConditions []metav1.Condition) string {
-	ignoreOption := cmpopts.IgnoreFields(metav1.Condition{}, "LastTransitionTime", "ObservedGeneration", "Message")
-	// we need to sort each condition slice by type before comparing
-	sort.SliceStable(wantConditions, func(i, j int) bool {
-		return wantConditions[i].Type < wantConditions[j].Type
-	})
-	sort.SliceStable(gotConditions, func(i, j int) bool {
-		return gotConditions[i].Type < gotConditions[j].Type
-	})
-	return cmp.Diff(wantConditions, gotConditions, ignoreOption)
-}

test/e2e/rollout_test.go

@@ -21,9 +21,9 @@ import (
 
 	placementv1beta1 "go.goms.io/fleet/apis/placement/v1beta1"
 	"go.goms.io/fleet/pkg/utils"
-	"go.goms.io/fleet/pkg/utils/condition"
 	"go.goms.io/fleet/test/e2e/framework"
 	testutils "go.goms.io/fleet/test/e2e/v1alpha1/utils"
+	"go.goms.io/fleet/test/utils/controller"
 )
 
 var (
@@ -122,7 +122,7 @@ var _ = Describe("placing wrapped resources using a CRP", Ordered, func() {
 									ObservedGeneration: 1,
 								},
 							}
-							diff := condition.CompareConditions(wantConditions, work.Status.Conditions)
+							diff := controller.CompareConditions(wantConditions, work.Status.Conditions)
 							if len(diff) != 0 {
 								return diff
 							}
@@ -143,7 +143,7 @@ var _ = Describe("placing wrapped resources using a CRP", Ordered, func() {
 									ObservedGeneration: 1,
 								},
 							}
-							diff := condition.CompareConditions(wantConditions, work.Status.Conditions)
+							diff := controller.CompareConditions(wantConditions, work.Status.Conditions)
 							if len(diff) != 0 {
 								return diff
 							}

test/utils/controller/controller.go

@@ -8,7 +8,12 @@ package controller
 
 import (
 	"context"
+	"sort"
 	"sync"
+
+	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
+	v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
 // FakeController is a fake controller which only stores one key.
@@ -46,3 +51,16 @@ func (f *FakeController) Key() string {
 	defer f.mu.RUnlock()
 	return f.key
 }
+
+// CompareConditions compares two condition slices and returns a string with the differences.
+func CompareConditions(wantConditions, gotConditions []v1.Condition) string {
+	ignoreOption := cmpopts.IgnoreFields(v1.Condition{}, "LastTransitionTime", "ObservedGeneration", "Message")
+	// we need to sort each condition slice by type before comparing
+	sort.SliceStable(wantConditions, func(i, j int) bool {
+		return wantConditions[i].Type < wantConditions[j].Type
+	})
+	sort.SliceStable(gotConditions, func(i, j int) bool {
+		return gotConditions[i].Type < gotConditions[j].Type
+	})
+	return cmp.Diff(wantConditions, gotConditions, ignoreOption)
+}