Fix spec comparison in applyWork (#89)

Signed-off-by: Jian Qiu <jqiu@redhat.com>

pkg/addonmanager/controllers/agentdeploy/controller.go

@@ -166,7 +166,7 @@ func (c *addonDeployController) sync(ctx context.Context, syncCtx factory.SyncCo
 	c.setStatusFeedbackRule(work, agentAddon)
 
 	// apply work
-	work, err = applyWork(c.workClient, c.workLister, c.cache, c.eventRecorder, ctx, work)
+	work, err = applyWork(ctx, c.workClient, c.workLister, c.cache, c.eventRecorder, work)
 	if err != nil {
 		meta.SetStatusCondition(&managedClusterAddonCopy.Status.Conditions, metav1.Condition{
 			Type:    "ManifestApplied",

pkg/addonmanager/controllers/agentdeploy/hookcontroller.go

@@ -176,7 +176,7 @@ func (c *addonHookDeployController) sync(ctx context.Context, syncCtx factory.Sy
 
 	// apply hookWork when addon is deleting
 	hookWork.OwnerReferences = []metav1.OwnerReference{*owner}
-	hookWork, applyErr := applyWork(c.workClient, c.workLister, c.cache, c.eventRecorder, ctx, hookWork)
+	hookWork, applyErr := applyWork(ctx, c.workClient, c.workLister, c.cache, c.eventRecorder, hookWork)
 	completed := hookWorkIsCompleted(hookWork)
 	if completed && hasFinalizer(managedClusterAddonCopy.Finalizers, constants.PreDeleteHookFinalizer) {
 		finalizer := removeFinalizer(managedClusterAddonCopy.Finalizers, constants.PreDeleteHookFinalizer)

pkg/addonmanager/controllers/agentdeploy/util_test.go

@@ -0,0 +1,73 @@
+package agentdeploy
+
+import (
+	"context"
+	"testing"
+	"time"
+
+	"k8s.io/apimachinery/pkg/runtime"
+	"open-cluster-management.io/addon-framework/pkg/addonmanager/addontesting"
+	fakework "open-cluster-management.io/api/client/work/clientset/versioned/fake"
+	workinformers "open-cluster-management.io/api/client/work/informers/externalversions"
+	workapiv1 "open-cluster-management.io/api/work/v1"
+)
+
+func TestApplyWork(t *testing.T) {
+	cache := newWorkCache()
+	fakeWorkClient := fakework.NewSimpleClientset()
+	workInformerFactory := workinformers.NewSharedInformerFactory(fakeWorkClient, 10*time.Minute)
+	syncContext := addontesting.NewFakeSyncContext(t, "test")
+
+	work, _, _ := buildManifestWorkFromObject("cluster1", "addon1", []runtime.Object{addontesting.NewUnstructured("batch/v1", "Job", "default", "test")})
+
+	_, err := applyWork(context.TODO(), fakeWorkClient, workInformerFactory.Work().V1().ManifestWorks().Lister(), cache, syncContext.Recorder(), work)
+	if err != nil {
+		t.Errorf("failed to apply work with err %v", err)
+	}
+
+	addontesting.AssertActions(t, fakeWorkClient.Actions(), "create")
+
+	// IF work is not changed, we should not update
+	newWorkCopy := work.DeepCopy()
+	fakeWorkClient.ClearActions()
+	workInformerFactory.Work().V1().ManifestWorks().Informer().GetStore().Add(work)
+	_, err = applyWork(context.TODO(), fakeWorkClient, workInformerFactory.Work().V1().ManifestWorks().Lister(), cache, syncContext.Recorder(), newWorkCopy)
+	if err != nil {
+		t.Errorf("failed to apply work with err %v", err)
+	}
+	addontesting.AssertNoActions(t, fakeWorkClient.Actions())
+
+	// Update work spec to update it
+	newWork, _, _ := buildManifestWorkFromObject("cluster1", "addon1", []runtime.Object{addontesting.NewUnstructured("batch/v1", "Job", "default", "test")})
+	newWork.Spec.DeleteOption = &workapiv1.DeleteOption{PropagationPolicy: workapiv1.DeletePropagationPolicyTypeOrphan}
+	fakeWorkClient.ClearActions()
+	_, err = applyWork(context.TODO(), fakeWorkClient, workInformerFactory.Work().V1().ManifestWorks().Lister(), cache, syncContext.Recorder(), newWork)
+	if err != nil {
+		t.Errorf("failed to apply work with err %v", err)
+	}
+	addontesting.AssertActions(t, fakeWorkClient.Actions(), "update")
+
+	// Do not update if generation is not changed
+	work.Spec.DeleteOption = &workapiv1.DeleteOption{PropagationPolicy: workapiv1.DeletePropagationPolicyTypeForeground}
+	workInformerFactory.Work().V1().ManifestWorks().Informer().GetStore().Update(work)
+
+	fakeWorkClient.ClearActions()
+	workInformerFactory.Work().V1().ManifestWorks().Informer().GetStore().Update(work)
+	_, err = applyWork(context.TODO(), fakeWorkClient, workInformerFactory.Work().V1().ManifestWorks().Lister(), cache, syncContext.Recorder(), newWork)
+	if err != nil {
+		t.Errorf("failed to apply work with err %v", err)
+	}
+	addontesting.AssertNoActions(t, fakeWorkClient.Actions())
+
+	// change generation will cause update
+	work.Generation = 1
+	workInformerFactory.Work().V1().ManifestWorks().Informer().GetStore().Update(work)
+
+	fakeWorkClient.ClearActions()
+	workInformerFactory.Work().V1().ManifestWorks().Informer().GetStore().Update(work)
+	_, err = applyWork(context.TODO(), fakeWorkClient, workInformerFactory.Work().V1().ManifestWorks().Lister(), cache, syncContext.Recorder(), newWork)
+	if err != nil {
+		t.Errorf("failed to apply work with err %v", err)
+	}
+	addontesting.AssertActions(t, fakeWorkClient.Actions(), "update")
+}

pkg/addonmanager/controllers/agentdeploy/utils.go

@@ -160,11 +160,11 @@ func buildManifestWorkFromObject(
 }
 
 func applyWork(
+	ctx context.Context,
 	workClient workv1client.Interface,
 	workLister worklister.ManifestWorkLister,
 	cache *workCache,
 	eventRecorder events.Recorder,
-	ctx context.Context,
 	required *workapiv1.ManifestWork) (*workapiv1.ManifestWork, error) {
 	existingWork, err := workLister.ManifestWorks(required.Namespace).Get(required.Name)
 	existingWork = existingWork.DeepCopy()
@@ -190,7 +190,7 @@ func applyWork(
 		return existingWork, nil
 	}
 
-	existingWork.Spec.Workload = required.Spec.Workload
+	existingWork.Spec = required.Spec
 	existingWork, err = workClient.WorkV1().ManifestWorks(existingWork.Namespace).Update(ctx, existingWork, metav1.UpdateOptions{})
 	if err == nil {
 		cache.updateCache(required, existingWork)