Merge pull request #169 from syntasso/stateFileFlake

fix: delete kratix statefile last
syntasso · Jun 19, 2024 · efb1298 · efb1298
2 parents 8372aee + d1735f7
commit efb1298
Show file tree

Hide file tree

Showing 4 changed files with 85 additions and 42 deletions.
diff --git a/controllers/workplacement_controller.go b/controllers/workplacement_controller.go
@@ -51,7 +51,10 @@ type WorkPlacementReconciler struct {
 	VersionCache map[string]string
 }
 
-const repoCleanupWorkPlacementFinalizer = "finalizers.workplacement.kratix.io/repo-cleanup"
+const (
+	repoCleanupWorkPlacementFinalizer       = "finalizers.workplacement.kratix.io/repo-cleanup"
+	kratixFileCleanupWorkPlacementFinalizer = "finalizers.workplacement.kratix.io/kratix-dot-files-cleanup"
+)
 
 //+kubebuilder:rbac:groups=platform.kratix.io,resources=workplacements,verbs=get;list;watch;create;update;patch;delete
 //+kubebuilder:rbac:groups=platform.kratix.io,resources=workplacements/status,verbs=get;update;patch
@@ -95,12 +98,13 @@ func (r *WorkPlacementReconciler) Reconcile(ctx context.Context, req ctrl.Reques
 		return ctrl.Result{}, err
 	}
 
+	filepathMode := destination.GetFilepathMode()
 	if !workPlacement.DeletionTimestamp.IsZero() {
-		return r.deleteWorkPlacement(ctx, writer, workPlacement, destination.GetFilepathMode(), logger)
+		return r.deleteWorkPlacement(ctx, writer, workPlacement, filepathMode, logger)
 	}
 
-	if !controllerutil.ContainsFinalizer(workPlacement, repoCleanupWorkPlacementFinalizer) {
-		return addFinalizers(opts, workPlacement, []string{repoCleanupWorkPlacementFinalizer})
+	if missingFinalizers := checkWorkPlacementFinalizers(workPlacement, filepathMode); len(missingFinalizers) > 0 {
+		return addFinalizers(opts, workPlacement, missingFinalizers)
 	}
 
 	versionID, err := r.writeWorkloadsToStateStore(writer, *workPlacement, *destination, logger)
@@ -129,42 +133,54 @@ func (r *WorkPlacementReconciler) Reconcile(ctx context.Context, req ctrl.Reques
 }
 
 func (r *WorkPlacementReconciler) deleteWorkPlacement(ctx context.Context, writer writers.StateStoreWriter, workPlacement *v1alpha1.WorkPlacement, filePathMode string, logger logr.Logger) (ctrl.Result, error) {
-	if !controllerutil.ContainsFinalizer(workPlacement, repoCleanupWorkPlacementFinalizer) {
-		return ctrl.Result{}, nil
-	}
-	logger.Info("cleaning up work on repository", "workplacement", workPlacement.Name)
+	pendingRepoCleanup := controllerutil.ContainsFinalizer(workPlacement, repoCleanupWorkPlacementFinalizer)
+	pendingKratixFileCleanup := controllerutil.ContainsFinalizer(workPlacement, kratixFileCleanupWorkPlacementFinalizer)
 
 	var err error
+	kratixFilePath := fmt.Sprintf(".kratix/%s-%s.yaml", workPlacement.Namespace, workPlacement.Name)
 
-	var dir = getDir(*workPlacement) + "/"
-	var workloadsToDelete []string
+	var dir string
+	switch filePathMode {
+	case v1alpha1.FilepathModeNestedByMetadata:
+		dir = getDir(*workPlacement) + "/"
+	}
 
-	if filePathMode == v1alpha1.FilepathModeNone {
-		var kratixFile []byte
-		kratixFilePath := fmt.Sprintf(".kratix/%s-%s.yaml", workPlacement.Namespace, workPlacement.Name)
-		if kratixFile, err = writer.ReadFile(kratixFilePath); err != nil {
-			logger.Error(err, "failed to read .kratix state file")
-			return defaultRequeue, err
-		}
-		stateFile := StateFile{}
-		if err = yaml.Unmarshal(kratixFile, &stateFile); err != nil {
-			logger.Error(err, "failed to unmarshal .kratix state file")
-			return defaultRequeue, err
+	if pendingRepoCleanup {
+		logger.Info("cleaning up work on repository", "workplacement", workPlacement.Name)
+		var workloadsToDelete []string
+		if filePathMode == v1alpha1.FilepathModeNone {
+			var kratixFile []byte
+			if kratixFile, err = writer.ReadFile(kratixFilePath); err != nil {
+				logger.Error(err, "failed to read .kratix state file", "file path", kratixFilePath)
+				return ctrl.Result{}, err
+			}
+			stateFile := StateFile{}
+			if err = yaml.Unmarshal(kratixFile, &stateFile); err != nil {
+				logger.Error(err, "failed to unmarshal .kratix state file")
+				return defaultRequeue, err
+			}
+			workloadsToDelete = stateFile.Files
 		}
-		dir = ""
-		workloadsToDelete = append(stateFile.Files, kratixFilePath)
+
+		return r.delete(ctx, writer, dir, workPlacement, workloadsToDelete, repoCleanupWorkPlacementFinalizer, logger)
 	}
-	_, err = writer.UpdateFiles(dir, workPlacement.Name, nil, workloadsToDelete)
 
-	if err != nil {
+	if pendingKratixFileCleanup {
+		logger.Info("cleaning up .kratix state file", "workplacement", workPlacement.Name)
+		return r.delete(ctx, writer, "", workPlacement, []string{kratixFilePath}, kratixFileCleanupWorkPlacementFinalizer, logger)
+	}
+	return ctrl.Result{}, nil
+}
+
+func (r *WorkPlacementReconciler) delete(ctx context.Context, writer writers.StateStoreWriter, dir string, workPlacement *v1alpha1.WorkPlacement, workloadsToDelete []string, finalizerToRemove string, logger logr.Logger) (ctrl.Result, error) {
+	if _, err := writer.UpdateFiles(dir, workPlacement.Name, nil, workloadsToDelete); err != nil {
 		logger.Error(err, "error removing work from repository, will try again in 5 seconds")
-		return defaultRequeue, err
+		return ctrl.Result{}, err
 	}
 
-	controllerutil.RemoveFinalizer(workPlacement, repoCleanupWorkPlacementFinalizer)
-	err = r.Client.Update(ctx, workPlacement)
-	if err != nil {
-		return defaultRequeue, err
+	controllerutil.RemoveFinalizer(workPlacement, finalizerToRemove)
+	if err := r.Client.Update(ctx, workPlacement); err != nil {
+		return ctrl.Result{}, err
 	}
 	return fastRequeue, nil
 }
@@ -280,3 +296,14 @@ func (r *WorkPlacementReconciler) SetupWithManager(mgr ctrl.Manager) error {
 		For(&v1alpha1.WorkPlacement{}).
 		Complete(r)
 }
+
+func checkWorkPlacementFinalizers(workPlacement *v1alpha1.WorkPlacement, filepathMode string) []string {
+	var missingFinalizers []string
+	if !controllerutil.ContainsFinalizer(workPlacement, repoCleanupWorkPlacementFinalizer) {
+		missingFinalizers = append(missingFinalizers, repoCleanupWorkPlacementFinalizer)
+	}
+	if filepathMode == v1alpha1.FilepathModeNone && !controllerutil.ContainsFinalizer(workPlacement, kratixFileCleanupWorkPlacementFinalizer) {
+		missingFinalizers = append(missingFinalizers, kratixFileCleanupWorkPlacementFinalizer)
+	}
+	return missingFinalizers
+}
diff --git a/controllers/workplacement_controller_test.go b/controllers/workplacement_controller_test.go
@@ -191,7 +191,10 @@ var _ = Describe("WorkplacementReconciler", func() {
 				workplacement := &v1alpha1.WorkPlacement{}
 				Expect(fakeK8sClient.Get(ctx, types.NamespacedName{Name: workplacementName, Namespace: "default"}, workplacement)).
 					To(Succeed())
-				Expect(workplacement.GetFinalizers()).To(ContainElement("finalizers.workplacement.kratix.io/repo-cleanup"))
+				Expect(workplacement.GetFinalizers()).To(ConsistOf(
+					"finalizers.workplacement.kratix.io/repo-cleanup",
+					"finalizers.workplacement.kratix.io/kratix-dot-files-cleanup",
+				))
 			})
 
 			When("deleting a workplacement", func() {
@@ -210,14 +213,21 @@ files:
 					Expect(err).NotTo(HaveOccurred())
 					Expect(result).To(Equal(ctrl.Result{}))
 
-					Expect(fakeWriter.UpdateFilesCallCount()).To(Equal(2))
+					kratixStateFile := fmt.Sprintf(".kratix/%s-%s.yaml", workPlacement.Namespace, workPlacement.Name)
+					Expect(fakeWriter.UpdateFilesCallCount()).To(Equal(3))
 					Expect(fakeWriter.ReadFileCallCount()).To(Equal(2))
-					Expect(fakeWriter.ReadFileArgsForCall(1)).To(Equal(fmt.Sprintf(".kratix/%s-%s.yaml", workPlacement.Namespace, workPlacement.Name)))
+					Expect(fakeWriter.ReadFileArgsForCall(1)).To(Equal(kratixStateFile))
 
 					dir, workPlacementName, workloadsToCreate, workloadsToDelete := fakeWriter.UpdateFilesArgsForCall(1)
 					Expect(workPlacementName).To(Equal(workPlacement.Name))
 					Expect(workloadsToCreate).To(BeNil())
-					Expect(workloadsToDelete).To(ConsistOf("fruit.yaml", ".kratix/default-test-workplacement.yaml"))
+					Expect(workloadsToDelete).To(ConsistOf("fruit.yaml"))
+					Expect(dir).To(Equal(""))
+
+					dir, workPlacementName, workloadsToCreate, workloadsToDelete = fakeWriter.UpdateFilesArgsForCall(2)
+					Expect(workPlacementName).To(Equal(workPlacement.Name))
+					Expect(workloadsToCreate).To(BeNil())
+					Expect(workloadsToDelete).To(ConsistOf(kratixStateFile))
 					Expect(dir).To(Equal(""))
 				})
 			})

diff --git a/lib/writers/s3.go b/lib/writers/s3.go
@@ -102,7 +102,6 @@ func (b *S3Writer) UpdateFiles(subDir string, _ string, workloadsToCreate []v1al
 func (b *S3Writer) update(subDir string, workloadsToCreate []v1alpha1.Workload, workloadsToDelete []string) (string, error) {
 	ctx := context.Background()
 	logger := b.Log.WithValues("bucketName", b.BucketName, "path", b.path)
-
 	objectsToDeleteMap := map[string]minio.ObjectInfo{}
 
 	//Get a list of all the old workload files, we delete any that aren't part of the new workload at the end of this function.

diff --git a/test/system/system_test.go b/test/system/system_test.go
@@ -712,30 +712,37 @@ var _ = Describe("Kratix", func() {
 				return listFilesInMinIOStateStore(resourceDestName)
 			}, shortTimeout, interval).Should(ContainElements(
 				"configmap.yaml",
-				fmt.Sprintf("%s.yaml", rrNameOne),
-				fmt.Sprintf("%s.yaml", rrNameTwo)))
+				ContainSubstring(fmt.Sprintf("%s.yaml", rrNameOne)),
+				ContainSubstring(fmt.Sprintf("%s.yaml", rrNameTwo)),
+			))
 
 			By("removing only files associated with the resource request at deletion")
 			platform.kubectl("delete", crd.Name, rrNameOne)
 			Eventually(func() []string {
 				return listFilesInMinIOStateStore(resourceDestName)
 			}, shortTimeout, interval).ShouldNot(ContainElements(
 				fmt.Sprintf("%s.yaml", rrNameOne)))
-			Expect(listFilesInMinIOStateStore(resourceDestName)).To(ContainElements(fmt.Sprintf("%s.yaml", rrNameTwo)))
+			Expect(listFilesInMinIOStateStore(resourceDestName)).To(ContainElements(
+				ContainSubstring(fmt.Sprintf("%s.yaml", rrNameTwo)),
+			))
 
 			By("cleaning up files from state store at deletion")
 			platform.eventuallyKubectlDelete("promise", bashPromiseName)
 			Eventually(func() []string {
 				return listFilesInGitStateStore(promiseDestName)
-			}, shortTimeout, interval).ShouldNot(ContainElements("configmap.yaml", ".kratix/"))
+			}, shortTimeout, interval).ShouldNot(ContainElements(
+				"configmap.yaml",
+				ContainSubstring(".kratix"),
+			))
 
 			Eventually(func() []string {
 				return listFilesInMinIOStateStore(resourceDestName)
 			}, shortTimeout, interval).ShouldNot(ContainElements(
 				"configmap.yaml",
-				".kratix/",
-				fmt.Sprintf("%s.yaml", rrNameOne),
-				fmt.Sprintf("%s.yaml", rrNameTwo)))
+				ContainSubstring(".kratix"),
+				ContainSubstring(fmt.Sprintf("%s.yaml", rrNameOne)),
+				ContainSubstring(fmt.Sprintf("%s.yaml", rrNameTwo)),
+			))
 		})
 	})
 })