Add DisableGarbageCollect annotation #2858

phillebaba · 2020-02-19T17:53:14Z

This change implements an annotation to disable garbage collection for specific resources.

squaremo

A couple of nits (one misleading log message; one test that could do more). I have a longer-form comment which I'll get to in a sec ..

squaremo · 2020-02-20T13:38:17Z

pkg/cluster/kubernetes/sync_test.go

@@ -464,6 +485,18 @@ metadata:
 		test(t, kube, "", ns1+defs1+defs2+ns3+defs3, false)
 	})

+	t.Run("sync adds and GCs skips resources", func(t *testing.T) {


This test does verify what it claims to; but I think it should also verify that resources are updated (i.e., not ignored) when they have a gc_disabled annotation.

squaremo · 2020-02-20T13:39:03Z

pkg/cluster/kubernetes/sync.go

@@ -145,6 +145,11 @@ func (c *Cluster) collectGarbage(

 		switch {
 		case !ok: // was not recorded as having been staged for application
+			if res.Policies().Has(policy.DisableGarbageCollect) {
+				c.logger.Log("info", "not deleting resource; garbage colelction annotation in file", "resource", res.ResourceID())


Suggested change

c.logger.Log("info", "not deleting resource; garbage colelction annotation in file", "resource", res.ResourceID())

c.logger.Log("info", "not deleting resource; garbage collection disabled by annotation", "resource", res.ResourceID())

(typo, and the annotation is in the cluster resource, not the file)

squaremo · 2020-02-20T13:53:28Z

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" | "no_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 · 2020-02-23T10:38:33Z

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 · 2020-02-24T15:59:36Z

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 · 2020-02-25T11:35:57Z

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 · 2020-02-25T14:59:12Z

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 · 2020-02-25T15:05:51Z

Cool, no worries @phillebaba, any time you lend to this project is appreciated :-)

phillebaba · 2020-02-29T16:53:36Z

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 #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

Out of date

squaremo

I have quibbles only -- this is a neat and incisive PR, thank you. If you have a moment, account for the quibbles as you see fit, and rebase on master. (I'll do the latter in a day or two -- it'll be mechanical anyway).

squaremo · 2020-03-02T11:00:11Z

docs/faq.md

@@ -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


Suggested change

Additionally when garbage collection is enabled. Flux will not garbage collect resources in the cluster

Additionally, when garbage collection is enabled, Flux will not garbage collect resources in the cluster

squaremo · 2020-03-02T11:00:31Z

docs/faq.md

+### 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.


squaremo · 2020-03-02T11:01:21Z

pkg/cluster/kubernetes/sync.go

+			}
+
+			v, ok := res.Policies().Get(policy.Ignore)
+			if ok && v == "sync_only" {


Could do with a constant for this

squaremo · 2020-03-02T11:03:33Z

pkg/cluster/kubernetes/sync_test.go

+		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)


Nice and simple test case 👍

Change GC behavior to skip GC of resources with ignore policy

squaremo previously requested changes Feb 20, 2020

View reviewed changes

squaremo approved these changes Mar 2, 2020

View reviewed changes

phillebaba added 5 commits March 2, 2020 22:42

Add DisableGarbageCollect annotation

8696876

Add support for sync_only value on ignore policy

1f5649a

Change GC behavior to skip GC of resources with ignore policy

Update docs with new annotation value

e383949

Make sync only value constant

7b2d98a

Fix docs grammar

c0f7160

squaremo merged commit e45cf58 into fluxcd:master Mar 5, 2020

hiddeco added this to the 1.19.0 milestone Mar 26, 2020

stefanprodan mentioned this pull request Mar 30, 2020

Disable garbage collection for specific resource #2841

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add DisableGarbageCollect annotation #2858

Add DisableGarbageCollect annotation #2858

phillebaba commented Feb 19, 2020

squaremo left a comment

squaremo Feb 20, 2020

squaremo Feb 20, 2020

squaremo commented Feb 20, 2020 •

edited

Loading

phillebaba commented Feb 23, 2020

squaremo commented Feb 24, 2020

squaremo commented Feb 25, 2020

phillebaba commented Feb 25, 2020

squaremo commented Feb 25, 2020

phillebaba commented Feb 29, 2020

squaremo left a comment

squaremo Mar 2, 2020

squaremo Mar 2, 2020

squaremo Mar 2, 2020

squaremo Mar 2, 2020

	c.logger.Log("info", "not deleting resource; garbage colelction annotation in file", "resource", res.ResourceID())
	c.logger.Log("info", "not deleting resource; garbage collection disabled by annotation", "resource", res.ResourceID())

	Additionally when garbage collection is enabled. Flux will not garbage collect resources in the cluster
	Additionally, when garbage collection is enabled, Flux will not garbage collect resources in the cluster

Add DisableGarbageCollect annotation #2858

Add DisableGarbageCollect annotation #2858

Conversation

phillebaba commented Feb 19, 2020

squaremo left a comment

Choose a reason for hiding this comment

squaremo Feb 20, 2020

Choose a reason for hiding this comment

squaremo Feb 20, 2020

Choose a reason for hiding this comment

squaremo commented Feb 20, 2020 • edited Loading

phillebaba commented Feb 23, 2020

squaremo commented Feb 24, 2020

squaremo commented Feb 25, 2020

phillebaba commented Feb 25, 2020

squaremo commented Feb 25, 2020

phillebaba commented Feb 29, 2020

squaremo left a comment

Choose a reason for hiding this comment

squaremo Mar 2, 2020

Choose a reason for hiding this comment

squaremo Mar 2, 2020

Choose a reason for hiding this comment

squaremo Mar 2, 2020

Choose a reason for hiding this comment

squaremo Mar 2, 2020

Choose a reason for hiding this comment

squaremo commented Feb 20, 2020 •

edited

Loading