fix: wait namespace is actually deleted

[eddycharly]

What this PR does / why we need it:

This PR waits the test case namespace is actually deleted when deleting a namespace.
This can take some time until finalisers run, returning before the actual deletion can disturb other tests.

[erikgb]

I think this is a good change. One comment, and I also wonder if we need to add the import alias for K8s errors.

[eddycharly]

I also wonder if we need to add the import alias for K8s errors

I can remove the alias if you want but we have a variable named errors.

[erikgb]

/lgtm

Thanks for this!

[kensipe]

/lgtm

I was tempted to get your take on the use of this projects kubernetes.go::WaitForDelete which is generic... but I actually have a strong preference for passing ctx which it is missing. However when the generic function was created, most of k8s code didn't have ctx... I believe that was added post 1.16 (hard to remember now). It would still be interesting to get your take on the value of modifying the generic function or removing it. The WaitForDelete doesn't currently have any dependencies and if we don't have interest in it... I will likely delete it.

[kensipe]

1 sec... This code does look good! however just prior to merging I realized the potential issue. This is valuable for specific use case(s)... however for a use case of parallel tests or tests isolated by namespaces, you don't need to wait for the deletion of a previous namespace to continue working in another namespace. I need to review 1 more time zoomed out to the larger picture. It is possible this is fine. I would like to reason thru the code from larger perspective.

[kensipe]

the first thing I noticed is:

if the test is successful, however the namespace isn't deleted (perhaps a timeout) then the success turns to a failure. Do we want a failure for this? I can actually see both cases... we could see a timeout on wait and the cluster is at fault for which it is a false negative. I could see a timeout on deletion which is caused by namespace finalizers for which you do want a failure to be reported.

It would be good IMO to discuss all scenarios and reason thru expectations. It may lead to another test configuration to wait for deletion or to declaratively express if deletion should raise a failure.

[eddycharly]

I'm not sure why we wouldn't want to wait.

I discovered the issue when running kuttl twice rapidly, first time all worked fine, second time tests failed because a namespace created during the first run was not yet deleted (not waiting actual namespace deletion can have an impact across multiple kuttl runs).

In this sense I feel like failing to cleanup should lead to a test failure, cleaning up correctly sounds important to me, especially when running kuttl on non ephemeral clusters. If the namespace fails to cleanup because of finalisers, the test can probably be improved to perform resources deletion in the correct order.

[eddycharly]

I didn't know about the WaitForDelete function, I will look at it.

I actually have a strong preference for passing ctx

💯

[kensipe]

actually with the ns deletion moving to the test.Cleanup it makes it much easier to reason about... (thanks for that). I think this makes sense! all other tests will be separate tests parallel or not. This makes sense as long as we are all good with ns deletion timeouts are a test failure.

[eddycharly]

I'm not sure why we wouldn't want to wait.

IMHO it's not just about namespaces but all resources a kuttl test creates, the deletion order matters too, we should not try to delete everything concurrently, this can lead to serious problems.

Take the following case:

• create a CRD
• deploy a controller that uses finalisers
• create a CR

If we delete the controller before the CR we're stuck, the CR will never be deleted because the controller will never perform finalisation. If this happens I would expect a test failure.

[kensipe]

I'm not confident in the error message we will receive for this... worth a look... looking...

[eddycharly]

This makes sense as long as we are all good with ns deletion timeouts are a test failure

I'm fine with this. If it happens it's probably a sign my test scenario has holes. We could/should document that.

[eddycharly]

I'm not confident in the error message we will receive for this

Let me know if you want me to change the message.

[eddycharly]

@kensipe WaitForDelete is not used anymore right ?
I feel like it's a nice helper, we could add context support to it and start using it more, WDYT ?