Bug 1691513: CVO should retry initialization until cancelled to avoid wedging because of failing dependencies

[smarterclayton]

The sync loop currently mixes a combination of:

1. Try forever (top level sync loop)
2. Cancel sync when a timeout is reached
3. Retry individual Apply actions in the sync workers up to a max
   retry
4. Retry within Apply actions

This led to bugs in initialization where transient errors cause a prereq
step to fail and never complete, leading to other operators never going
live.

This commit simplifies the structure of our retry logic to be:

1. Try to sync forever
2. Cancel sync after a certain period of failure and go into backoff
3. Retry individual Applies forever with capped backoff
4. Retry applies as long as context is not cancelled

This implies all top level sync has a context with a deadline which is the
case today - some test cases are changed for it.

This should also reduce the lag between cancel (on user action, for
instance) and the sync loop terminating, since everything is context gated.

Also adds a Makefile because I like consistency.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1691513

[smarterclayton]

@abhinavdahiya with this the sync loop is context aware all the way down and retries until context is cancelled. Retry and bail didn't have much value for us anyway except for status updates and we should gather those on cancel (I need to verify that and we can make apply report partial status if necessary).

[wking]

unit:

E0325 19:56:04.262847    4132 task.go:68] error running apply for test "file-yml" (3 of 3): unable to proceed
SIGQUIT: quit

[abhinavdahiya]

9221dec Also adds a Makefile because I like consistency. :( but ok!
1212873 LGTM

@abhinavdahiya with this the sync loop is context aware all the way down and retries until context is cancelled. Retry and bail didn't have much value for us anyway except for status updates and we should gather those on cancel

The context we have is the global one (leader election ctx), I didn't see any deadlines.

cluster-version-operator/pkg/cvo/cvo.go

Line 219 in 3d7d6bc

go optr.configSync.Start(ctx, 16)

cluster-version-operator/pkg/cvo/sync_worker.go

Line 232 in 3d7d6bc

ctx, cancelFn := context.WithCancel(ctx)

cluster-version-operator/pkg/cvo/sync_worker.go

Line 242 in 3d7d6bc

return w.syncOnce(ctx, work, maxWorkers, reporter)

cluster-version-operator/pkg/cvo/sync_worker.go

Line 402 in 3d7d6bc

return w.apply(ctx, w.payload, work, maxWorkers, reporter)

cluster-version-operator/pkg/cvo/sync_worker.go

Line 455 in 3d7d6bc

err := payload.RunGraph(ctx, graph, maxWorkers, func(ctx context.Context, tasks []*payload.Task) error {

cluster-version-operator/pkg/cvo/sync_worker.go

Line 471 in 3d7d6bc

if err := task.Run(ctx, version, w.builder, work.State); err != nil {

(I need to verify that and we can make apply report partial status if necessary).

Yeah, we will only see status updates on success or global cancel...

[smarterclayton]

The context we have is the global one (leader election ctx), I didn't see any deadlines.

There should have been one, I'm going to add that. Was meaningless while we looped, but is meaningful now.

Also adds a Makefile because I like consistency. :( but ok!

Every other repo has a simple one, it helps orient and define some minimum carry over between them. I don't expect it to grow or be heavy, but it saves time for folks working everywhere.

[smarterclayton]

Yeah, we will only see status updates on success or global cancel...

We'll see a progress percentage for every group that increases. It's only crash loops that will be delayed. I'll tune the sync loop timeout based on that progress upgrade distinct from sync distinct from initializing.

[smarterclayton]

For updates we may want to keep going until we see repeated errors, and then report those up. One thing I think we need is summarizing the errors from multiple nodes in the graph into a stable output message that isn't going back and forth. Reconcile could also accumulate all possible errors that occur during a pass without retries and then summarize any failures in the progressing message, potentially moving to failing if two successive syncs have any errors.

[smarterclayton]

We don't need to exit sync in order to report status, but we should keep in mind that we want to guarantee two things during reconcile:

1. we eventually try every objects
2. we want to guarantee an upper bound on the interval between any two reconcile attempts of an object

[abhinavdahiya]

838794a LGTM

/retest

[abhinavdahiya]

/test e2e-aws

[abhinavdahiya]

/test e2e-aws

[wking]

e2e-aws:

Failing tests:

[sig-storage] In-tree Volumes [Driver: nfs] [Testpattern: Dynamic PV (default fs)] subPath should be able to unmount after the subpath directory is deleted [Suite:openshift/conformance/parallel] [Suite:k8s]
[sig-storage] In-tree Volumes [Driver: nfs] [Testpattern: Inline-volume (default fs)] subPath should be able to unmount after the subpath directory is deleted [Suite:openshift/conformance/parallel] [Suite:k8s]
[sig-storage] In-tree Volumes [Driver: nfs] [Testpattern: Pre-provisioned PV (default fs)] subPath should be able to unmount after the subpath directory is deleted [Suite:openshift/conformance/parallel] [Suite:k8s]

/retest

[wking]

e2e-aws hit openshift/origin#22412 again.

[wking]

openshift/origin#22412 landed.

/retest

[wking]

/lgtm

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: smarterclayton, wking

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [smarterclayton,wking]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[wking]

🎉