delete old applied work after its new work is applied (open-cluster-m…

…anagement-io#172) Signed-off-by: Wei Liu <liuweixa@redhat.com> Signed-off-by: Wei Liu <liuweixa@redhat.com>
zhujian7 · Jan 3, 2023 · 5417cc4 · 5417cc4
1 parent 5b74046
commit 5417cc4
Show file tree

Hide file tree

Showing 5 changed files with 125 additions and 5 deletions.
diff --git a/pkg/spoke/controllers/finalizercontroller/unmanaged_appliedmanifestwork_controller.go b/pkg/spoke/controllers/finalizercontroller/unmanaged_appliedmanifestwork_controller.go
@@ -23,8 +23,9 @@ import (
 
 const byWorkNameAndAgentID = "UnManagedAppliedManifestWork-byWorkNameAndAgentID"
 
-// ManifestWorkFinalizeController handles cleanup of manifestwork resources before deletion is allowed.
+// UnManagedAppliedWorkController deletes unmanaged applied works.
 type UnManagedAppliedWorkController struct {
+	manifestWorkLister         worklister.ManifestWorkNamespaceLister
 	appliedManifestWorkClient  workv1client.AppliedManifestWorkInterface
 	appliedManifestWorkLister  worklister.AppliedManifestWorkLister
 	appliedManifestWorkIndexer cache.Indexer
@@ -34,12 +35,15 @@ type UnManagedAppliedWorkController struct {
 
 func NewUnManagedAppliedWorkController(
 	recorder events.Recorder,
+	manifestWorkInformer workinformer.ManifestWorkInformer,
+	manifestWorkLister worklister.ManifestWorkNamespaceLister,
 	appliedManifestWorkClient workv1client.AppliedManifestWorkInterface,
 	appliedManifestWorkInformer workinformer.AppliedManifestWorkInformer,
 	hubHash, agentID string,
 ) factory.Controller {
 
 	controller := &UnManagedAppliedWorkController{
+		manifestWorkLister:         manifestWorkLister,
 		appliedManifestWorkClient:  appliedManifestWorkClient,
 		appliedManifestWorkLister:  appliedManifestWorkInformer.Lister(),
 		appliedManifestWorkIndexer: appliedManifestWorkInformer.Informer().GetIndexer(),
@@ -55,6 +59,10 @@ func NewUnManagedAppliedWorkController(
 	}
 
 	return factory.New().
+		WithInformersQueueKeyFunc(func(obj runtime.Object) string {
+			accessor, _ := meta.Accessor(obj)
+			return fmt.Sprintf("%s-%s", hubHash, accessor.GetName())
+		}, manifestWorkInformer.Informer()).
 		WithFilteredEventsInformersQueueKeyFunc(func(obj runtime.Object) string {
 			accessor, _ := meta.Accessor(obj)
 			return accessor.GetName()
@@ -71,6 +79,26 @@ func (m *UnManagedAppliedWorkController) sync(ctx context.Context, controllerCon
 		// work not found, could have been deleted, do nothing.
 		return nil
 	}
+	if err != nil {
+		return err
+	}
+
+	// We delete the old applied work until the work of this applied work is applied.
+	// This will avoid to delete the old applied work prematurely, if an applied work is an
+	// owner of its applied resource, if we delete the old applied work prematurely, this will
+	// casue to delete the applied resource.
+	manifestWork, err := m.manifestWorkLister.Get(appliedManifestWork.Spec.ManifestWorkName)
+	if errors.IsNotFound(err) {
+		// work not found, could have been deleted, do nothing.
+		return nil
+	}
+	if err != nil {
+		return err
+	}
+	if !meta.IsStatusConditionTrue(manifestWork.Status.Conditions, workapiv1.WorkApplied) {
+		// the work is not applied, do nothing.
+		return nil
+	}
 
 	unManagedAppliedWorks, err := m.getUnManagedAppliedManifestWorksByIndex(appliedManifestWork.Spec.ManifestWorkName, appliedManifestWork.Spec.AgentID)
 	if err != nil {

diff --git a/pkg/spoke/controllers/finalizercontroller/unmanaged_appliedmanifestwork_controller_test.go b/pkg/spoke/controllers/finalizercontroller/unmanaged_appliedmanifestwork_controller_test.go
@@ -21,6 +21,7 @@ func TestSyncUnamanagedAppliedWork(t *testing.T) {
 		workName                           string
 		hubHash                            string
 		agentID                            string
+		works                              []runtime.Object
 		appliedWorks                       []runtime.Object
 		validateAppliedManifestWorkActions func(t *testing.T, actions []clienttesting.Action)
 	}{
@@ -29,6 +30,22 @@ func TestSyncUnamanagedAppliedWork(t *testing.T) {
 			workName: "test",
 			hubHash:  "hubhash1",
 			agentID:  "test-agent",
+			works: []runtime.Object{
+				&workapiv1.ManifestWork{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "test",
+						Namespace: "test",
+					},
+					Status: workapiv1.ManifestWorkStatus{
+						Conditions: []metav1.Condition{
+							{
+								Type:   workapiv1.WorkApplied,
+								Status: metav1.ConditionTrue,
+							},
+						},
+					},
+				},
+			},
 			appliedWorks: []runtime.Object{
 				&workapiv1.AppliedManifestWork{
 					ObjectMeta: metav1.ObjectMeta{
@@ -59,11 +76,64 @@ func TestSyncUnamanagedAppliedWork(t *testing.T) {
 				spoketesting.AssertAction(t, actions[0], "delete")
 			},
 		},
+		{
+			name:     "no action if the work is not applied",
+			workName: "test",
+			hubHash:  "hubhash1",
+			agentID:  "test-agent",
+			works: []runtime.Object{
+				&workapiv1.ManifestWork{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "test",
+						Namespace: "test",
+					},
+				},
+			},
+			appliedWorks: []runtime.Object{
+				&workapiv1.AppliedManifestWork{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "hubhash-test",
+					},
+					Spec: workapiv1.AppliedManifestWorkSpec{
+						ManifestWorkName: "test",
+						HubHash:          "hubhash",
+						AgentID:          "test-agent",
+					},
+				},
+				&workapiv1.AppliedManifestWork{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "hubhash1-test",
+					},
+					Spec: workapiv1.AppliedManifestWorkSpec{
+						ManifestWorkName: "test",
+						HubHash:          "hubhash1",
+						AgentID:          "test-agent",
+					},
+				},
+			},
+			validateAppliedManifestWorkActions: noAction,
+		},
 		{
 			name:     "no action for different AgentID",
 			workName: "test",
 			hubHash:  "hubhash1",
 			agentID:  "test-agent",
+			works: []runtime.Object{
+				&workapiv1.ManifestWork{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "test",
+						Namespace: "test",
+					},
+					Status: workapiv1.ManifestWorkStatus{
+						Conditions: []metav1.Condition{
+							{
+								Type:   workapiv1.WorkApplied,
+								Status: metav1.ConditionTrue,
+							},
+						},
+					},
+				},
+			},
 			appliedWorks: []runtime.Object{
 				&workapiv1.AppliedManifestWork{
 					ObjectMeta: metav1.ObjectMeta{
@@ -93,6 +163,22 @@ func TestSyncUnamanagedAppliedWork(t *testing.T) {
 			workName: "test",
 			hubHash:  "hubhash1",
 			agentID:  "test-agent",
+			works: []runtime.Object{
+				&workapiv1.ManifestWork{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "test",
+						Namespace: "test",
+					},
+					Status: workapiv1.ManifestWorkStatus{
+						Conditions: []metav1.Condition{
+							{
+								Type:   workapiv1.WorkApplied,
+								Status: metav1.ConditionTrue,
+							},
+						},
+					},
+				},
+			},
 			appliedWorks: []runtime.Object{
 				&workapiv1.AppliedManifestWork{
 					ObjectMeta: metav1.ObjectMeta{
@@ -129,13 +215,19 @@ func TestSyncUnamanagedAppliedWork(t *testing.T) {
 			if err != nil {
 				t.Fatal(err)
 			}
+			for _, work := range c.works {
+				if err := informerFactory.Work().V1().ManifestWorks().Informer().GetStore().Add(work); err != nil {
+					t.Fatal(err)
+				}
+			}
 			for _, appliedWork := range c.appliedWorks {
 				if err := informerFactory.Work().V1().AppliedManifestWorks().Informer().GetStore().Add(appliedWork); err != nil {
 					t.Fatal(err)
 				}
 			}
 
 			controller := &UnManagedAppliedWorkController{
+				manifestWorkLister:         informerFactory.Work().V1().ManifestWorks().Lister().ManifestWorks("test"),
 				appliedManifestWorkClient:  fakeClient.WorkV1().AppliedManifestWorks(),
 				appliedManifestWorkLister:  informerFactory.Work().V1().AppliedManifestWorks().Lister(),
 				appliedManifestWorkIndexer: informerFactory.Work().V1().AppliedManifestWorks().Informer().GetIndexer(),

diff --git a/pkg/spoke/spokeagent.go b/pkg/spoke/spokeagent.go
@@ -161,6 +161,8 @@ func (o *WorkloadAgentOptions) RunWorkloadAgent(ctx context.Context, controllerC
 	)
 	unmanagedAppliedManifestWorkController := finalizercontroller.NewUnManagedAppliedWorkController(
 		controllerContext.EventRecorder,
+		workInformerFactory.Work().V1().ManifestWorks(),
+		workInformerFactory.Work().V1().ManifestWorks().Lister().ManifestWorks(o.SpokeClusterName),
 		spokeWorkClient.WorkV1().AppliedManifestWorks(),
 		spokeWorkInformerFactory.Work().V1().AppliedManifestWorks(),
 		hubhash, agentID,

diff --git a/test/integration/suite_test.go b/test/integration/suite_test.go
@@ -1,7 +1,6 @@
 package integration
 
 import (
-	"io/ioutil"
 	"os"
 	"path"
 	"path/filepath"
@@ -60,7 +59,7 @@ var _ = ginkgo.BeforeSuite(func() {
 	gomega.Expect(cfg).ToNot(gomega.BeNil())
 
 	// create kubeconfig file for hub in a tmp dir
-	tempDir, err = ioutil.TempDir("", "test")
+	tempDir, err = os.MkdirTemp("", "test")
 	gomega.Expect(err).ToNot(gomega.HaveOccurred())
 	gomega.Expect(tempDir).ToNot(gomega.BeEmpty())
 	hubKubeconfigFileName = path.Join(tempDir, "kubeconfig")

diff --git a/test/integration/unmanaged_appliedwork_test.go b/test/integration/unmanaged_appliedwork_test.go
@@ -3,7 +3,6 @@ package integration
 import (
 	"context"
 	"fmt"
-	"io/ioutil"
 	"os"
 	"path"
 	"path/filepath"
@@ -65,7 +64,7 @@ var _ = ginkgo.Describe("Unmanaged ApplieManifestWork", func() {
 		newCfg, err := newHub.Start()
 		gomega.Expect(err).ToNot(gomega.HaveOccurred())
 
-		newHubTempDir, err = ioutil.TempDir("", "unmanaged_work_test")
+		newHubTempDir, err = os.MkdirTemp("", "unmanaged_work_test")
 		gomega.Expect(err).ToNot(gomega.HaveOccurred())
 
 		newHubKubeConfigFile = path.Join(newHubTempDir, "kubeconfig")