RFC: wait for deletion of namespaces

[mhoffm-aiven]

What this PR does / why we need it:

Fixes #314

[kensipe]

hey @mhoffm-aiven, thanks for the PR. I'm curious around the motivation... I could see value in certain circumstances but a nuisance in others. generally a test is in a namespace and is deleted after that test, followed by another test in another namespace. It is common to not care when the namespace removal is complete.

If we should optionally care, it seems like it should be configurable (and likely at the test level). It seems like there would be value in creating a KEP with acceptance criteria and use case. Could we get the use-case you are targetting?

[kensipe]

it also seems valuable to control timeouts around the wait

[mhoffm-aiven]

it also seems valuable to control timeouts around the wait

That should be controlled by the context now.

[mhoffm-aiven]

hey @mhoffm-aiven, thanks for the PR. I'm curious around the motivation... I could see value in certain circumstances but a nuisance in others. generally a test is in a namespace and is deleted after that test, followed by another test in another namespace. It is common to not care when the namespace removal is complete.

If we should optionally care, it seems like it should be configurable (and likely at the test level). It seems like there would be value in creating a KEP with acceptance criteria and use case. Could we get the use-case you are targetting?

Hey @kensipe,

First of all, thanks for looking into the PR!

I'm trying to test a controller that needs to clean up finalizers before the namespace can be deleted ( if it does not it will cause the namespace deletion to hang ), the problem now is that when i run the controller out of cluster as a command the kuttl test suite will kill -9 it after the final assertion. If we make namespace deletion blocking the command will not be killed before we got a chance to remove finalizers and cleanup will work ( i tested it with my fork ).

Should there be an error in your reconcile logic where finalizers are not removed for some reason, the test will fail, so it also strengthens the tests.

Not killing it is probably no solution because the controller cannot know if some deletions are still queued so it cannot wait itself for a warm shutdown.

I think that making it configurable (maybe a waitForCleanup flag with false as default) is a good compromise. What do you think?

[kensipe]

yeah.. sorry for delays. I'm aligned with the last message posted here. I will review the details later today.

[kensipe]

oh... @mhoffm-aiven the changes don't appear to be here... you were looking to talk it thru. I'm aligned with a configurable option. Do you plan to add that to this PR?

[mhoffm-aiven]

Hey @kensipe

I have not changed anything yet! If the changes sound good to you, i will implement them shortly though!

[mhoffm-aiven]

Hey @kensipe

I made the blocking deletion configurable. What do you think?

[mhoffm-aiven]

Hey @kensipe,

Is there something to be done still?

[kensipe]

shame on me... apologies. I will dig into this today. and thanks so much!

[kensipe]

a quick pass looks good. I will take a deeper look later today

[mhoffm-aiven]

Hey @kensipe ,

No worries! Thanks for looking into it!

[kensipe]

@mhoffm-aiven ok... thanks for the ping and for think time. my initial hesitant was on the name "block" vs "wait"... then landed on the crux of the issue. what would you think of a --delete-timeout or --ns-delete-timeout. Instead of assuming forever is good. The default of 0 is no wait (value without a flag set). then each project could determine what a reasonable timeout is.... perhaps a -1 is forever. rough thoughts at this point. I agree with the value of having a wait on ns finalizers. Wanted to get your thoughts before proceeding. In general this is good... the question is could we do better? Looking forward to your thoughts

[mhoffm-aiven]

Hey @kensipe

bounding the wait time seems like a good thing to do, lets go with your suggestions, to summarize:

• calling the flag --ns-delete-timeout or --ns-delete-timeout-seconds maybe
• 0 means no wait, -1 means indefinite wait, N means wait n seconds

If you agree i would see when i can find some time to implement it, probably on the weekend though!

[kensipe]

@mhoffm-aiven love it

[kensipe]

I would like to include this into the next release. we will wait for the updates for it's inclusion.