external-snapshotter constantly retrying CreateSnapshot calls on erro…

[zhucan]

…r w/o backoff

Signed-off-by: zhucan [email protected]

What type of PR is this?
/kind bug

What this PR does / why we need it:
external-snapshotter constantly retrying CreateSnapshot calls on error w/o backoff

Which issue(s) this PR fixes:
Fixes #533

Special notes for your reviewer:
@xing-yang

Does this PR introduce a user-facing change?:

Fix a problem in CSI snapshotter sidecar that constantly retries CreateSnapshot call on error without exponential backoff.

[pohly]

/skip

I don't know this code.

[zhucan]

This is the test results. WIth the default retry interval times, The retry interval times will be 1s , 2s, 4s, 8s, 16s, 32s, 64s, 128s, 256s, 5m.

[zhucan]

/retest

[zhucan]

/retest-required

[xing-yang]

@zhucan Can you change the release note to the following?

Fix a problem in CSI snapshotter sidecar that constantly retries CreateSnapshot call on error without exponential backoff.

[xing-yang]

/lgtm

[xing-yang]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: xing-yang, zhucan

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 [xing-yang]

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

[anshulahuja98]

@xing-yang can we take a release of the external snapshotter with this fix included?

[pwschuurman]

I think this change has the unintended effect of not updating if the CSI driver returns success. I don't think that an update which removes the AnnVolumeSnapshotBeingCreated annotation in indicative of a failure. It can be removed even when the CSI driver CreateSnapshot does not return a failure: https://github.com/kubernetes-csi/external-snapshotter/blob/master/pkg/sidecar-controller/snapshot_controller.go#L352

This PR is currently breaking some tests for the PD CSI driver (which @amacaskill is planning to bring to testgrid). The effect is that the CSI controller does not get resynced after the AnnVolumeSnapshotBeingCreated annotation is removed, which makes the snapshotter update at the next resync period (default 15 minutes). This time period is longer than how long the test waits (5 minutes). It could also affect a production controller, if using the default resync period.

[xing-yang]

@pwschuurman Do you see a 15 minutes delay for a VolumeSnapshot to be created successfully? Does this happen all the time? I wonder why CI didn't catch this.

[pwschuurman]

@xing-yang For PD CSI driver, volume snapshot typically takes <1 minute for a snapshot to be ready. However with this change, a call to syncContent won't happen until the next controller resync interval, which is 15 minutes by default.

I chatted with @mattcary, and we think a short term fix would be to reintroduce the call to enqueueContentWork in the success case, but keep the logic as is for the error case (by checking the status on the new VolumeSnapshotContent).

[xing-yang]

@pwschuurman That makes sense. Do you want to submit a fix?

[pwschuurman]

@xing-yang Yes, I can send out a PR to fix.