Add --poll-snapshot-period for periodic readiness requeue check when creating a VolumeSnapshotContent

[pwschuurman]

What type of PR is this?
/kind feature

What this PR does / why we need it:
Today, the csi-snapshotter runs with a reconciler pattern when processing VolumeSnapshotContent resources. This follows the familiar requeue, err pattern that is followed by other controller frameworks, like controller-runtime.

• Error Handling: When an error is encountered, VolumeSnapshotContent resources will be requeued with exponential decay. This behavior is desired, as presumably RPC calls to a CSI driver are due to congestion, and the request rate should be reduced.
• Requeue Processing: In the case of dynamic provisioning, the creation of a VolumeSnapshotContent may require some time until a snapshot is ready in the underlying storage system. This is where the requeue field is useful, as the reconciler can periodically attempt to check the status of a snapshot.

The problem with the current requeue implementation is that it relies on the same exponential backoff rate limiter queue as the error handling scenario. This is a problem, as it can lead to non-deterministic ready times, and can lead to significantly higher effective snapshot ready latencies for snapshots that take longer to process (eg: larger snapshots).

To fix this, the requeue logic should use a constant retry period, to allow for more reliable snapshot reconciliation. This PR adds a new command line argument --poll-snapshot-period to the csi-snapshotter, with a default of 10 seconds.

Which issue(s) this PR fixes:

Fixes #1282

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Requeue VolumeSnapshotContent resources based on --poll-snapshot-period in csi-snapshotter

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: pwschuurman
Once this PR has been reviewed and has the lgtm label, please assign jsafrane for approval. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

• OWNERS

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

[pwschuurman]

/assign @sunnylovestiramisu

[sunnylovestiramisu]

Question:

1. Why is the default 10 seconds? Is any data we looked at that is supporting this value?
2. Do you think it makes sense to separate two queues for this case? One for deterministic ready times and the other for non-deterministic ready times.

[xing-yang]

Can you set the default to 0 so that drivers can opt-in to this new behavior?

[pwschuurman]

Can you set the default to 0 so that drivers can opt-in to this new behavior?

I think changing the default is a benefit for end users here, so I would prefer to set it to a reasonable poll interval. We could increase the poll interval higher, if scalability is a concern. However if we do get a 429 from the APIServer, we'll encounter an error and then leverage exponential backoff.

I can make the default 0, and then refactor the logic to fallback on the exponential backoff when pollSnapshotPeriod is not set (eg: we need to adjust when Forget() is called if we do this).

@xing-yang What would you prefer (either keeping the logic as is but modifying the pollSnapshotPeriod, or refactoring to use the exponential rate limiting poll interval as a fallback).

[pwschuurman]

@sunnylovestiramisu

Why is the default 10 seconds? Is any data we looked at that is supporting this value?

Good call out. I don't have any data for setting a default. A default of a minute is may be more reasonable. This is the CSI connection timeout:

external-snapshotter/cmd/csi-snapshotter/main.go

Line 70 in 7e30553

defaultCSITimeout = time.Minute

I think the CreateSnapshot RPC is unique in that it is one of the RPCs that can return OK, but may require a retry. Other RPCs (eg: CreateVolume) are pass/fail. For CSI Provisioner, it seems that we used to have the default poll interval set at 10 seconds (eg: some reference here kubernetes-csi/external-provisioner#131).

Do you think it makes sense to separate two queues for this case? One for deterministic ready times and the other for non-deterministic ready times.

So you're thinking in the event of an error, we add a PVC to an "error" processing queue (that has a ExponentialRateLimiter), and we have a "retry" processing queue? I think that is more to manage, and there is greater risk of concurrency related issues. We'd need to track resource keys across two queues, and we need to listen to resources available on both queues.