Merge pull request #623 from sdowell/fix-retain-reconcile-fail

fix: retain objects which failed to reconcile

Author: k8s-ci-robot

pkg/apply/task/delete_inv_task_test.go

@@ -8,7 +8,6 @@ import (
 
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/runtime/schema"
-	"sigs.k8s.io/cli-utils/pkg/apis/actuation"
 	"sigs.k8s.io/cli-utils/pkg/apply/cache"
 	"sigs.k8s.io/cli-utils/pkg/apply/event"
 	"sigs.k8s.io/cli-utils/pkg/apply/taskrunner"
@@ -24,6 +23,7 @@ func TestDeleteInvTask(t *testing.T) {
 	id3 := object.UnstructuredToObjMetadata(obj3)
 	testCases := map[string]struct {
 		prevInventory     object.ObjMetadataSet
+		deletedObjs       object.ObjMetadataSet
 		failedDeletes     object.ObjMetadataSet
 		failedReconciles  object.ObjMetadataSet
 		timeoutReconciles object.ObjMetadataSet
@@ -54,20 +54,23 @@ func TestDeleteInvTask(t *testing.T) {
 		},
 		"inventory is updated instead of deleted in case of reconcile failure": {
 			prevInventory:    object.ObjMetadataSet{id1, id2, id3},
+			deletedObjs:      object.ObjMetadataSet{id1, id2, id3},
 			failedReconciles: object.ObjMetadataSet{id1},
 			err:              nil,
 			isError:          false,
 			expectedObjs:     object.ObjMetadataSet{id1},
 		},
 		"inventory is updated instead of deleted in case of reconcile timeout": {
 			prevInventory:     object.ObjMetadataSet{id1, id2, id3},
+			deletedObjs:       object.ObjMetadataSet{id1, id2, id3},
 			timeoutReconciles: object.ObjMetadataSet{id1},
 			err:               nil,
 			isError:           false,
 			expectedObjs:      object.ObjMetadataSet{id1},
 		},
 		"inventory is updated instead of deleted in case of pruning/reconcile failure": {
 			prevInventory:    object.ObjMetadataSet{id1, id2, id3},
+			deletedObjs:      object.ObjMetadataSet{id1, id2, id3},
 			failedReconciles: object.ObjMetadataSet{id1},
 			failedDeletes:    object.ObjMetadataSet{id2},
 			err:              nil,
@@ -82,34 +85,20 @@ func TestDeleteInvTask(t *testing.T) {
 			eventChannel := make(chan event.Event)
 			resourceCache := cache.NewResourceCacheMap()
 			context := taskrunner.NewTaskContext(eventChannel, resourceCache)
+			im := context.InventoryManager()
+			for _, deleteObj := range tc.deletedObjs {
+				im.AddSuccessfulDelete(deleteObj, "unused-uid")
+			}
 			for _, failedDelete := range tc.failedDeletes {
-				context.InventoryManager().AddFailedDelete(failedDelete)
+				im.AddFailedDelete(failedDelete)
 			}
 			for _, failedReconcile := range tc.failedReconciles {
-				context.InventoryManager().SetObjectStatus(actuation.ObjectStatus{
-					ObjectReference: actuation.ObjectReference{
-						Group:     failedReconcile.GroupKind.Group,
-						Kind:      failedReconcile.GroupKind.Kind,
-						Name:      failedReconcile.Name,
-						Namespace: failedReconcile.Namespace,
-					},
-				})
-				context.AddInvalidObject(failedReconcile)
-				if err := context.InventoryManager().SetFailedReconcile(failedReconcile); err != nil {
+				if err := im.SetFailedReconcile(failedReconcile); err != nil {
 					t.Fatal(err)
 				}
 			}
 			for _, timeoutReconcile := range tc.timeoutReconciles {
-				context.InventoryManager().SetObjectStatus(actuation.ObjectStatus{
-					ObjectReference: actuation.ObjectReference{
-						Group:     timeoutReconcile.GroupKind.Group,
-						Kind:      timeoutReconcile.GroupKind.Kind,
-						Name:      timeoutReconcile.Name,
-						Namespace: timeoutReconcile.Namespace,
-					},
-				})
-				context.AddInvalidObject(timeoutReconcile)
-				if err := context.InventoryManager().SetTimeoutReconcile(timeoutReconcile); err != nil {
+				if err := im.SetTimeoutReconcile(timeoutReconcile); err != nil {
 					t.Fatal(err)
 				}
 			}

pkg/apply/task/inv_set_task.go

@@ -131,6 +131,18 @@ func (i *DeleteOrUpdateInvTask) updateInventory(taskContext *taskrunner.TaskCont
 	klog.V(4).Infof("keep in inventory %d skipped prunes", len(pruneSkips))
 	invObjs = invObjs.Union(pruneSkips)
 
+	// If an object failed to reconcile and was previously stored in the inventory,
+	// then keep it in the inventory so it can be waited on next time.
+	reconcileFailures := i.PrevInventory.Intersection(im.FailedReconciles())
+	klog.V(4).Infof("set inventory %d failed reconciles", len(reconcileFailures))
+	invObjs = invObjs.Union(reconcileFailures)
+
+	// If an object timed out reconciling and was previously stored in the inventory,
+	// then keep it in the inventory so it can be waited on next time.
+	reconcileTimeouts := i.PrevInventory.Intersection(im.TimeoutReconciles())
+	klog.V(4).Infof("keep in inventory %d timeout reconciles", len(reconcileTimeouts))
+	invObjs = invObjs.Union(reconcileTimeouts)
+
 	// If an object is abandoned, then remove it from the inventory.
 	abandonedObjects := taskContext.AbandonedObjects()
 	klog.V(4).Infof("remove from inventory %d abandoned objects", len(abandonedObjects))

pkg/apply/task/inv_set_task_test.go

@@ -29,15 +29,18 @@ func TestInvSetTask(t *testing.T) {
 	idInvalid := object.UnstructuredToObjMetadata(objInvalid)
 
 	tests := map[string]struct {
-		prevInventory  object.ObjMetadataSet
-		appliedObjs    object.ObjMetadataSet
-		failedApplies  object.ObjMetadataSet
-		failedDeletes  object.ObjMetadataSet
-		skippedApplies object.ObjMetadataSet
-		skippedDeletes object.ObjMetadataSet
-		abandonedObjs  object.ObjMetadataSet
-		invalidObjs    object.ObjMetadataSet
-		expectedObjs   object.ObjMetadataSet
+		prevInventory     object.ObjMetadataSet
+		appliedObjs       object.ObjMetadataSet
+		deletedObjs       object.ObjMetadataSet
+		failedApplies     object.ObjMetadataSet
+		failedDeletes     object.ObjMetadataSet
+		skippedApplies    object.ObjMetadataSet
+		skippedDeletes    object.ObjMetadataSet
+		failedReconciles  object.ObjMetadataSet
+		timeoutReconciles object.ObjMetadataSet
+		abandonedObjs     object.ObjMetadataSet
+		invalidObjs       object.ObjMetadataSet
+		expectedObjs      object.ObjMetadataSet
 	}{
 		"no apply objs, no prune failures; no inventory": {
 			expectedObjs: object.ObjMetadataSet{},
@@ -160,6 +163,30 @@ func TestInvSetTask(t *testing.T) {
 			invalidObjs:   object.ObjMetadataSet{idInvalid},
 			expectedObjs:  object.ObjMetadataSet{id3},
 		},
+		"applied object failed to reconcile": {
+			prevInventory:    object.ObjMetadataSet{},
+			appliedObjs:      object.ObjMetadataSet{id3},
+			failedReconciles: object.ObjMetadataSet{id3},
+			expectedObjs:     object.ObjMetadataSet{id3},
+		},
+		"deleted object failed to reconcile": {
+			prevInventory:    object.ObjMetadataSet{id3},
+			deletedObjs:      object.ObjMetadataSet{id3},
+			failedReconciles: object.ObjMetadataSet{id3},
+			expectedObjs:     object.ObjMetadataSet{id3},
+		},
+		"applied object timed out to reconcile": {
+			prevInventory:     object.ObjMetadataSet{},
+			appliedObjs:       object.ObjMetadataSet{id3},
+			timeoutReconciles: object.ObjMetadataSet{id3},
+			expectedObjs:      object.ObjMetadataSet{id3},
+		},
+		"deleted object timed out to reconcile": {
+			prevInventory:     object.ObjMetadataSet{id3},
+			deletedObjs:       object.ObjMetadataSet{id3},
+			timeoutReconciles: object.ObjMetadataSet{id3},
+			expectedObjs:      object.ObjMetadataSet{id3},
+		},
 	}
 
 	for name, tc := range tests {
@@ -180,6 +207,9 @@ func TestInvSetTask(t *testing.T) {
 			for _, applyObj := range tc.appliedObjs {
 				im.AddSuccessfulApply(applyObj, "unusued-uid", int64(0))
 			}
+			for _, deleteObj := range tc.deletedObjs {
+				im.AddSuccessfulDelete(deleteObj, "unused-uid")
+			}
 			for _, applyFailure := range tc.failedApplies {
 				im.AddFailedApply(applyFailure)
 			}
@@ -198,6 +228,16 @@ func TestInvSetTask(t *testing.T) {
 			for _, invalidObj := range tc.invalidObjs {
 				context.AddInvalidObject(invalidObj)
 			}
+			for _, failedReconcile := range tc.failedReconciles {
+				if err := im.SetFailedReconcile(failedReconcile); err != nil {
+					t.Fatal(err)
+				}
+			}
+			for _, timeoutReconcile := range tc.timeoutReconciles {
+				if err := im.SetTimeoutReconcile(timeoutReconcile); err != nil {
+					t.Fatal(err)
+				}
+			}
 			if taskName != task.Name() {
 				t.Errorf("expected task name (%s), got (%s)", taskName, task.Name())
 			}

test/e2e/destroy_reconciliation_failure_test.go

@@ -61,20 +61,40 @@ func destroyReconciliationFailureTest(ctx context.Context, c client.Client, invC
 
 	options := apply.DestroyerOptions{
 		InventoryPolicy: inventory.PolicyAdoptIfNoInventory,
-		DeleteTimeout:   15 * time.Second, // one pod is expected to be not pruned, so set a shorter timeout
+		DeleteTimeout:   10 * time.Second, // one pod is expected to be not pruned, so set a short timeout
 	}
-	_ = e2eutil.RunCollect(destroyer.Run(ctx, inv, options))
-
-	By("Verify pod1 is deleted")
-	e2eutil.AssertUnstructuredDoesNotExist(ctx, c, podObject)
+	// we should be able to run destroy multiple times and continue tracking the
+	// object in the inventory
+	expectedCounts := []struct {
+		specCount   int
+		statusCount int
+	}{
+		{
+			specCount:   1, // one object failed to reconcile, so is retained
+			statusCount: 2, // status for two objects, one deleted successfully
+		},
+		{
+			specCount:   1,
+			statusCount: 1, // only one object in inventory now, still failing to reconcile
+		},
+		{
+			specCount:   1,
+			statusCount: 1,
+		},
+	}
+	for _, ec := range expectedCounts {
+		_ = e2eutil.RunCollect(destroyer.Run(ctx, inv, options))
 
-	By("Verify podWithFinalizerObject is not deleted but has deletion timestamp")
-	podWithFinalizerObject = e2eutil.AssertHasDeletionTimestamp(ctx, c, podWithFinalizerObject)
+		By("Verify pod1 is deleted")
+		e2eutil.AssertUnstructuredDoesNotExist(ctx, c, podObject)
 
-	By("Verify inventory")
-	// The inventory should still have the Pod with the finalizer.
-	invConfig.InvSizeVerifyFunc(ctx, c, inventoryName, namespaceName, inventoryID, 0, 2)
+		By("Verify podWithFinalizerObject is not deleted but has deletion timestamp")
+		podWithFinalizerObject = e2eutil.AssertHasDeletionTimestamp(ctx, c, podWithFinalizerObject)
 
+		By("Verify inventory")
+		// The inventory should still have the Pod with the finalizer.
+		invConfig.InvSizeVerifyFunc(ctx, c, inventoryName, namespaceName, inventoryID, ec.specCount, ec.statusCount)
+	}
 	// remove the finalizer
 	podWithFinalizerObject = e2eutil.WithoutFinalizers(podWithFinalizerObject)
 	e2eutil.ApplyUnstructured(ctx, c, podWithFinalizerObject)