Repository: fluxcd/flux
Pull request #2858 (2020-02-19 17:53:14)

Add DisableGarbageCollect annotation
This change implements an annotation to disable garbage collection for specific resources.

https://github.com/fluxcd/flux/issues/2841

squaremo: Thinking aloud: I wonder if this could be value of "ignore", instead of its own annotation.

Here are the combinations of annotations and effects, when there are both:

| ignore \ disable_gc | false | true |
|---------------------------|--------|--------|
| **false**                  | sync and gc | sync, no gc |
| **true**                    | no sync, no gc | no sync, no gc |

There's only three different behaviours there, so they don't operate independently -- one of them is redundant. So you could represent those three possibilities as three values:

    fluxcd.io/ignore: "true" | "false" | "gc"

One counter-argument is that it's weird to have a three-valued boolean like that (albeit they are only stringly pictures of booleans).

wdyt @phillebaba ?
phillebaba: There would be some benefit with reducing the number of annotations, the risk is that having three values would confuse people. So documentation of the behavior is important.

The `ingore: true` + `disable_gc: false` combination would only disable gc if we assume ignore works as it was intended from the beginning.

Does these descriptions match the intended behavior of the different values? Assuming that the end user has enabled GC.
| value | description | 
| --- | --- | 
| false | Resources are synced when added to git and garbage collected when removed from git |
| no_gc | Resources are synced when added to git but will not be garbage collected when removed from git. |
| true | Resources are not synced when added to git. Additionally if the value is set manually outside of flux the resource will no longer be synced by flux, and it will not be garbage collected if removed from git. |

Am I missing something in the description?

squaremo: > There would be some benefit with reducing the number of annotations, the risk is that having three values would confuse people. So documentation of the behavior is important.

Yes, I agree on both points. There's an opportunity to change the formulation slightly, so long as it's backward-compatible -- say, `ignore: {false,none}|sync_only|{true,sync_and_gc}`. If that would mitigate the confusion ..

> The `ingore: true` + `disable_gc: false` combination would only disable gc if we assume ignore works as it was intended from the beginning.

Right, that's presupposed. I find it hard to imagine that someone is relying on it _not_ working like that (even I had difficulty getting my head around the fact that it didn't).

> Resources are synced when added to git but will not be garbage collected when removed from git.

I would say "synced _from_ git", since updates in git will still be applied to the cluster. Similarly, in the description of "true".

squaremo: OK so I think the goal for this PR should be to make it work as we've settled on above -- are you happy to adjust it, @phillebaba? Alternatively, I could make ignore mean ignore-for-gc-too in another PR, then you could work on top of that.
phillebaba: I can implement both parts, don't think that is an issue. You are right about going with the multi value annotation solution as it would simplify logic.

I have a lot to do at work this week so I can't get this done until the weekend. 
squaremo: Cool, no worries @phillebaba, any time you lend to this project is appreciated :-)
phillebaba: I change the behavior so that you disable GC by setting `sync_only` as the value for the ignore annotation.

I also added some more info in the FAQ, but i think that getting a single page that documents all annotations as described in https://github.com/fluxcd/flux/issues/2671 would be great!

A table like this would describe the ignore annotations behavior pretty well.
| Value | Sync | GC |
| --- | --- | --- |
| false | x | x |
| sync_only | x |  |
| true |  |  |

Patch:
diff --git a/docs/faq.md b/docs/faq.md
index 7affd90d..8aaee294 100644
--- a/docs/faq.md
+++ b/docs/faq.md
@@ -234,7 +234,7 @@ Now restart `fluxd` to re-read the k8s secret (if it is running):
 
 `kubectl delete $(kubectl get pod -o name -l name=flux)`
 
-If you have installed flux through Helm, make sure to pass 
+If you have installed flux through Helm, make sure to pass
 `--set git.secretName=flux-git-deploy` when installing/upgrading the chart.
 
 ### How do I use a private git host (or one that's not github.com, gitlab.com, bitbucket.org, dev.azure.com, or vs-ssh.visualstudio.com)?
@@ -341,6 +341,18 @@ how/where to undo the change (cf [flux#1211](https://github.com/fluxcd/flux/issu
 annotation exists in either the cluster or in git, it will be respected, so you may need to remove
 it from both places.
 
+Additionally, when garbage collection is enabled, Flux will not garbage collect resources in the cluster
+with the ignore annotation if the resource is removed from git.
+
+### Can I disable garbage collection for a specific resource?
+
+Yes. By adding the annotation below to a resource Flux will sync updates from git, but it will not
+garbage collect when the resource is removed from git.
+
+```yaml
+    fluxcd.io/ignore: sync_only
+```
+
 ### How can I prevent Flux overriding the replicas when using HPA?
 
 When using a horizontal pod autoscaler you have to remove the `spec.replicas` from your deployment definition.
diff --git a/pkg/cluster/kubernetes/sync.go b/pkg/cluster/kubernetes/sync.go
index 5883e228..2bd718a8 100644
--- a/pkg/cluster/kubernetes/sync.go
+++ b/pkg/cluster/kubernetes/sync.go
@@ -145,6 +145,17 @@ func (c *Cluster) collectGarbage(
 
 		switch {
 		case !ok: // was not recorded as having been staged for application
+			if res.Policies().Has(policy.Ignore) {
+				c.logger.Log("info", "skipping GC of cluster resource; resource has ignore policy true", "dry-run", dryRun, "resource", resourceID)
+				continue
+			}
+
+			v, ok := res.Policies().Get(policy.Ignore)
+			if ok && v == policy.IgnoreSyncOnly {
+				c.logger.Log("info", "skipping GC of cluster resource; resource has ignore policy sync_only ", "dry-run", dryRun, "resource", resourceID)
+				continue
+			}
+
 			c.logger.Log("info", "cluster resource not in resources to be synced; deleting", "dry-run", dryRun, "resource", resourceID)
 			if !dryRun {
 				orphanedResources.stage("delete", res.ResourceID(), "<cluster>", res.IdentifyingBytes())
diff --git a/pkg/policy/policy.go b/pkg/policy/policy.go
index c9ec95f3..93aea64f 100644
--- a/pkg/policy/policy.go
+++ b/pkg/policy/policy.go
@@ -14,6 +14,8 @@ const (
 	TagAll     = Policy("tag_all")
 )
 
+const IgnoreSyncOnly = "sync_only"
+
 // Policy is an string, denoting the current deployment policy of a service,
 // e.g. automated, or locked.
 type Policy string


Test patch:
diff --git a/pkg/cluster/kubernetes/sync_test.go b/pkg/cluster/kubernetes/sync_test.go
index e276cfe3..7ea44532 100644
--- a/pkg/cluster/kubernetes/sync_test.go
+++ b/pkg/cluster/kubernetes/sync_test.go
@@ -353,6 +353,7 @@ metadata:
   name: dep3
   namespace: other
 `
+
 	// checkSame is a check that a result returned from the cluster is
 	// the same as an expected.  labels and annotations may be altered
 	// by the sync process; we'll look at the "spec" field as an
@@ -607,7 +608,7 @@ metadata:
 		test(t, kube, ns1+defs1invalid, ns1+defs1invalid, true)
 	})
 
-	t.Run("sync doesn't apply or delete manifests marked with ignore", func(t *testing.T) {
+	t.Run("sync doesn't apply or GC manifests marked with ignore: 'true'", func(t *testing.T) {
 		kube, _, cancel := setup(t)
 		defer cancel()
 		kube.GC = true
@@ -651,7 +652,7 @@ spec:
 		test(t, kube, ns1+dep1ignored+dep2, ns1+dep1, false)
 	})
 
-	t.Run("sync doesn't update a cluster resource marked with ignore", func(t *testing.T) {
+	t.Run("sync doesn't update a cluster resource marked with ignore: 'true'", func(t *testing.T) {
 		const dep1 = `---
 apiVersion: apps/v1
 kind: Deployment
@@ -703,7 +704,74 @@ spec:
 		test(t, kube, ns1+mod1, ns1+dep1, false)
 	})
 
-	t.Run("sync doesn't update or delete a pre-existing resource marked with ignore", func(t *testing.T) {
+	t.Run("sync doesn't GC resources annotated with ignore: 'sync_only'", func(t *testing.T) {
+		kube, _, cancel := setup(t)
+		defer cancel()
+		kube.GC = true
+
+		const dep1 = `---
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: dep1
+  namespace: foobar
+  annotations: {flux.weave.works/ignore: "sync_only"}
+`
+
+		// sync namespace and deployment
+		test(t, kube, ns1+dep1, ns1+dep1, false)
+
+		// sync namespace only but expect deployment to not be GC
+		test(t, kube, ns1, ns1+dep1, false)
+	})
+
+	t.Run("sync doesn't GC resources annotated with ignore: 'true'", func(t *testing.T) {
+		kube, _, cancel := setup(t)
+		defer cancel()
+		kube.GC = true
+
+		const dep1 = `---
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: dep1
+  namespace: foobar
+`
+		const dep2 = `---
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: dep2
+  namespace: foobar
+  annotations: {flux.weave.works/ignore: "false"}
+`
+
+		// dep1 is created
+		test(t, kube, ns1+dep1+dep2, ns1+dep1+dep2, false)
+
+		// add ignore: 'true' annotation outside of sync loop
+		dc := kube.client.dynamicClient
+		rc := dc.Resource(schema.GroupVersionResource{
+			Group:    "apps",
+			Version:  "v1",
+			Resource: "deployments",
+		})
+		res, err := rc.Namespace("foobar").Get("dep1", metav1.GetOptions{})
+		if err != nil {
+			t.Fatal(err)
+		}
+		annots := res.GetAnnotations()
+		annots["flux.weave.works/ignore"] = "true"
+		res.SetAnnotations(annots)
+		if _, err = rc.Namespace("foobar").Update(res, metav1.UpdateOptions{}); err != nil {
+			t.Fatal(err)
+		}
+
+		// only sync ns1 but expect nothing to be GC
+		test(t, kube, ns1, ns1+dep1, false)
+	})
+
+	t.Run("sync doesn't update or delete a pre-existing resource marked with ignore: 'true'", func(t *testing.T) {
 		kube, _, cancel := setup(t)
 		defer cancel()