Remove work deletion from spoke, will be moved to hub GC

Signed-off-by: Ben Perry <[email protected]>

Author: bhperry

pkg/registration/hub/manifests/rbac/managedcluster-work-clusterrole.yaml

@@ -9,10 +9,10 @@ rules:
 - apiGroups: ["", "events.k8s.io"]
   resources: ["events"]
   verbs: ["create", "patch", "update"]
-# Allow work agent to get/list/watch/update/delete manifestworks
+# Allow work agent to get/list/watch/update manifestworks
 - apiGroups: ["work.open-cluster-management.io"]
   resources: ["manifestworks"]
-  verbs: ["get", "list", "watch", "update", "patch", "delete"]
+  verbs: ["get", "list", "watch", "update", "patch"]
 # Allow work agent to update the status of manifestwork
 - apiGroups: ["work.open-cluster-management.io"]
   resources: ["manifestworks/status"]

pkg/work/spoke/controllers/manifestcontroller/manifestwork_controller.go

@@ -136,8 +136,8 @@ func (m *ManifestWorkController) sync(ctx context.Context, controllerContext fac
 	}
 
 	// work that is completed does not receive any updates
-	if isComplete, err := m.manifestWorkCompletion(ctx, controllerContext, manifestWork); isComplete || err != nil {
-		return err
+	if meta.IsStatusConditionTrue(manifestWork.Status.Conditions, workapiv1.WorkComplete) {
+		return nil
 	}
 
 	// Apply appliedManifestWork
@@ -215,45 +215,3 @@ func (m *ManifestWorkController) applyAppliedManifestWork(ctx context.Context, w
 	_, err = m.appliedManifestWorkPatcher.PatchSpec(ctx, appliedManifestWork, requiredAppliedWork.Spec, appliedManifestWork.Spec)
 	return appliedManifestWork, err
 }
-
-func (m *ManifestWorkController) manifestWorkCompletion(
-	ctx context.Context, controllerContext factory.SyncContext, manifestWork *workapiv1.ManifestWork,
-) (bool, error) {
-	complete := meta.FindStatusCondition(manifestWork.Status.Conditions, workapiv1.WorkComplete)
-	if complete != nil && complete.Status == metav1.ConditionTrue {
-		var err error
-		// check if work has TTL set and it has elapsed since completion
-		if ttl, ok := remainingTtl(complete.LastTransitionTime, manifestWork.Spec.DeleteOption); ok {
-			if ttl <= 0 {
-				// Only delete if resourceVersion matches in case complete condition has changed
-				deleteOpts := metav1.DeleteOptions{Preconditions: &metav1.Preconditions{ResourceVersion: &manifestWork.ResourceVersion}}
-				err = m.manifestWorkClient.Delete(ctx, manifestWork.Name, deleteOpts)
-				if err == nil {
-					controllerContext.Recorder().Eventf(
-						"ManifestWorkDeleted", "Deleted ManifestWork %s because its TTL elapsed after completion", manifestWork.Name,
-					)
-				}
-			} else {
-				// Requeue after TTL to delete the manifestwork
-				controllerContext.Queue().AddAfter(manifestWork.Name, ttl)
-			}
-		}
-		return true, err
-	}
-	return false, nil
-}
-
-// remainingTtl returns the remaining duration before a completed manifestwork should be deleted, if configured
-func remainingTtl(completedAt metav1.Time, deleteOption *workapiv1.DeleteOption) (time.Duration, bool) {
-	if deleteOption == nil || deleteOption.TTLSecondsAfterFinished == nil {
-		return 0, false
-	}
-
-	ttl := *deleteOption.TTLSecondsAfterFinished
-	if ttl <= 0 {
-		return 0, true
-	}
-
-	remaining := time.Second*time.Duration(ttl) - time.Since(completedAt.Time)
-	return remaining, true
-}

pkg/work/spoke/controllers/manifestcontroller/manifestwork_reconciler_test.go

@@ -19,7 +19,6 @@ import (
 	fakedynamic "k8s.io/client-go/dynamic/fake"
 	fakekube "k8s.io/client-go/kubernetes/fake"
 	clienttesting "k8s.io/client-go/testing"
-	"k8s.io/utils/ptr"
 
 	fakeworkclient "open-cluster-management.io/api/client/work/clientset/versioned/fake"
 	workinformers "open-cluster-management.io/api/client/work/informers/externalversions"
@@ -166,11 +165,6 @@ func (t *testCase) withManifestConfig(configs ...workapiv1.ManifestConfigOption)
 	return t
 }
 
-func (t *testCase) withDeleteOption(deleteOption *workapiv1.DeleteOption) *testCase {
-	t.deleteOption = deleteOption
-	return t
-}
-
 func (t *testCase) withSpokeObject(objects ...runtime.Object) *testCase {
 	t.spokeObject = objects
 	return t
@@ -370,33 +364,10 @@ func TestSync(t *testing.T) {
 				expectedCondition(workapiv1.ManifestApplied, metav1.ConditionTrue),
 				expectedCondition(workapiv1.ManifestApplied, metav1.ConditionTrue)).
 			withExpectedWorkCondition(expectedCondition(workapiv1.WorkApplied, metav1.ConditionTrue)),
-		newTestCase("ignore completed manifestwork with no TTL").
+		newTestCase("ignore completed manifestwork").
 			withWorkManifest(testingcommon.NewUnstructured("v1", "Secret", "ns1", "test")).
 			withSpokeObject(spoketesting.NewSecret("test", "ns1", "value2")).
 			withExistingWorkCondition(metav1.Condition{Type: workapiv1.WorkComplete, Status: metav1.ConditionTrue}),
-		newTestCase("ignore completed manifestwork with unsatisfied TTL").
-			withWorkManifest(testingcommon.NewUnstructured("v1", "Secret", "ns1", "test")).
-			withSpokeObject(spoketesting.NewSecret("test", "ns1", "value2")).
-			withDeleteOption(&workapiv1.DeleteOption{TTLSecondsAfterFinished: ptr.To[int64](30)}).
-			withExistingWorkCondition(
-				metav1.Condition{
-					Type:               workapiv1.WorkComplete,
-					Status:             metav1.ConditionTrue,
-					LastTransitionTime: metav1.NewTime(time.Now().Add(-1 * time.Second)),
-				},
-			),
-		newTestCase("delete completed manifestwork with satisfied TTL").
-			withWorkManifest(testingcommon.NewUnstructured("v1", "Secret", "ns1", "test")).
-			withSpokeObject(spoketesting.NewSecret("test", "ns1", "value2")).
-			withDeleteOption(&workapiv1.DeleteOption{TTLSecondsAfterFinished: ptr.To[int64](10)}).
-			withExistingWorkCondition(
-				metav1.Condition{
-					Type:               workapiv1.WorkComplete,
-					Status:             metav1.ConditionTrue,
-					LastTransitionTime: metav1.NewTime(time.Now().Add(-11 * time.Second)),
-				},
-			).
-			withExpectedWorkAction("delete"),
 	}
 
 	for _, c := range cases {

test/integration/work/work_test.go

@@ -17,7 +17,6 @@ import (
 	"k8s.io/apimachinery/pkg/util/rand"
 	"k8s.io/client-go/dynamic"
 	"k8s.io/client-go/util/retry"
-	"k8s.io/utils/ptr"
 
 	workapiv1 "open-cluster-management.io/api/work/v1"
 	"open-cluster-management.io/sdk-go/pkg/cloudevents/clients/common"
@@ -879,17 +878,13 @@ var _ = ginkgo.Describe("ManifestWork", func() {
 		})
 	})
 
-	ginkgo.Context("With completion TTL", func() {
+	ginkgo.Context("Work completion", func() {
 		ginkgo.BeforeEach(func() {
 			manifests = []workapiv1.Manifest{
 				util.ToManifest(util.NewConfigmap(commOptions.SpokeClusterName, cm1, map[string]string{"a": "b"}, nil)),
 				util.ToManifest(util.NewConfigmap(commOptions.SpokeClusterName, cm2, map[string]string{"c": "d"}, nil)),
 			}
 			workOpts = append(workOpts, func(work *workapiv1.ManifestWork) {
-				work.Spec.DeleteOption = &workapiv1.DeleteOption{
-					TTLSecondsAfterFinished: ptr.To[int64](min(5, eventuallyTimeout/2)),
-					PropagationPolicy:       "Foreground",
-				}
 				work.Spec.ManifestConfigs = []workapiv1.ManifestConfigOption{
 					{
 						ResourceIdentifier: workapiv1.ResourceIdentifier{
@@ -937,29 +932,20 @@ var _ = ginkgo.Describe("ManifestWork", func() {
 				context.Background(), updatedWork.Name, types.MergePatchType, pathBytes, metav1.PatchOptions{})
 			gomega.Expect(err).ToNot(gomega.HaveOccurred())
 
-			// ManifestWork should be marked completed and eventually be deleted
+			// ManifestWork should be marked completed
 			gomega.Eventually(func() error {
 				work, err := hubWorkClient.WorkV1().ManifestWorks(commOptions.SpokeClusterName).Get(context.Background(), work.Name, metav1.GetOptions{})
 				if err != nil {
-					if errors.IsNotFound(err) {
-						return nil
-					}
 					return err
 				}
 
-				// If ManifestWork still exists we expect it to be marked complete
 				if err := util.CheckExpectedConditions(work.Status.Conditions, metav1.Condition{
 					Type:   workapiv1.WorkComplete,
 					Status: metav1.ConditionTrue,
 					Reason: "ConditionRulesAggregated",
 				}); err != nil {
 					return fmt.Errorf("%s: %v", work.Name, err)
 				}
-
-				// If marked complete and still exists, we expect deletion timestamp to be set
-				if work.DeletionTimestamp.IsZero() {
-					return fmt.Errorf("Expected work %s to be deleted", work.Name)
-				}
 				return nil
 			}, eventuallyTimeout, eventuallyInterval).Should(gomega.Succeed())
 		})