Change default behavior to always wait for reconciliation & deletion confirmation

[karlkfi]

The Applier and Destroyer each have WaitTasks used to wait until objects are Current (after apply) or NotFound (after prune/delete), but this behavior is a confusing "wait for some but not others" that is hard to describe and not relative to configuration, but rather the types of resources being applied together.

1. ReconcileTimeout, PruneTimeout, and DeleteTimeout have no explicit default value, which means their implicit default is 0.
   • https://github.com/kubernetes-sigs/cli-utils/blob/master/pkg/apply/applier.go#L282
   • https://github.com/kubernetes-sigs/cli-utils/blob/master/pkg/apply/destroyer.go#L80
2. But then the default of 0 is ignored by the Solver and 1 minute is used instead:
   • https://github.com/kubernetes-sigs/cli-utils/blob/master/pkg/apply/solver/solver.go#L38
3. But the default of 1 minutes is only used if numObjSets > 1, otherwise the WaitTask is skipped.
   • https://github.com/kubernetes-sigs/cli-utils/blob/master/pkg/apply/solver/solver.go#L274
4. This causes the WaitEvent to be skipped:
   • https://github.com/kubernetes-sigs/cli-utils/blob/master/test/e2e/apply_and_destroy_test.go#L213
   • https://github.com/kubernetes-sigs/cli-utils/blob/master/test/e2e/continue_on_error_test.go#L99
   • https://github.com/kubernetes-sigs/cli-utils/blob/master/test/e2e/prune_retrieve_error_test.go#L89

Then in addition to that inconsistency, WaitTasks use DeletePropagationBackground by default, which doesn't wait until child objects are deleted before deleting the parent object. So things like ReplicaSets and Pods may still exist after a Deployment is confirmed to be NotFound.

• https://github.com/kubernetes-sigs/cli-utils/blob/master/pkg/apply/applier.go#L286-L288

As a user of cli-utils, I would rather that the applier/destroyer either waited for every resource to be reconciled/deleted or not wait at all.

In the context of kpt and Config Sync, I think the default behavior should be to wait forever until cancelled by the caller (they can implement their own global timeout using context.WithTimeout or other cancellation using context.WithCancel), now that both the applier and Destroyer accept context as input:

• https://github.com/kubernetes-sigs/cli-utils/blob/master/pkg/apply/applier_test.go#L550
• https://github.com/kubernetes-sigs/cli-utils/blob/master/pkg/apply/destroyer_test.go#L20

I also think the default should be changed to DeletePropagationForeground so that the applier/destroyer don't exit until deletes are fully propagated.

[karlkfi]

Proposed fixes:

• Make the default timeout actually zero in the apply options and destroy options (don't override it in the solver).
• When the timeout is zero, never timeout (unless the whole run is cancelled by the caller)
• Always add the WaitTask, so the WaitEvent is always sent
• Switch to DeletePropagationForeground by default
• (Tech Debt) Clean up the WaitTask timeout to just use a context.WithTimeout that wraps the caller's context, instead of a manual timer. Then we can just use the cancelFunc instead of ClearTimeout
• (Tech Debt) Add an exposed Task method to replace checkCondition & completeIfWaitTask instead of having the baseRunner call multiple private functions on the WaitTask.

[karlkfi]

/assign @karlkfi

[karlkfi]

@ash2k @haiyanmeng @mortent
Can yall comment of how this will affect your usage of cli-utils?
Does it sound acceptable?

[ash2k]

@karlkfi I agree with the list of issues and I agree with the plan to address them 👍

In my case, I don't mind if cli-utils breaks API compatibility (if that would be necessary) to address any issues as long as:

• the change breaks compilation so that I can catch it and/or
• the change is documented in release notes so that I can adjust how I use the library (especially if the breaking change doesn't cause a compilation failure).

[karlkfi]

Related effort to make cancellation better/faster: #440

[mortent]

I agree with the issues and the proposed solution.

I think it would also be useful to be able to specify no-wait, which would apply all resources but not wait for reconciliation. When there are dependencies, it would just skip resources that depends on previous resources that haven't reconciled (i.e. just check once). But we can handle this separately at a later time.

[karlkfi]

I think it would also be useful to be able to specify no-wait, which would apply all resources but not wait for reconciliation.

Yeah, it sounds like @haiyanmeng wants this for Config Sync, but it also sounds like a short-term fix, not a long term solution.

When there are dependencies, it would just skip resources that depends on previous resources that haven't reconciled (i.e. just check once)

This is actually a lot more work. It would involve replacing or augmenting the asynchronous poller with a synchronous mechanism. I think it makes the most sense to do as part of the future async apply effort (which I've talked about but not spec'd out yet). Basically, when moving to a fully asynchronous apply, each apply/prune task needs to check that its dependencies are met before executing. That would mean each task needs to know it's dependencies (which they currently don't) instead of flattening the graph into phases. If we already had that, then making it only check once and skip if not ready would be easy. But that's not a short term solution.