Add --poll-snapshot-period for periodic readiness requeue check when creating a VolumeSnapshotContent #1284
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…creating a VolumeSnapshotContent
k8s-ci-robot
added
kind/feature
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: pwschuurman The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
cncf-cla: yes
|
/assign @sunnylovestiramisu |
|
Question:
|
xing-yang
reviewed
Apr 23, 2025
| @@ -93,6 +93,7 @@ var ( | |||
| metricsAddress = flag.String("metrics-address", "", "(deprecated) The TCP network address where the prometheus metrics endpoint will listen (example: `:8080`). The default is empty string, which means metrics endpoint is disabled. Only one of `--metrics-address` and `--http-endpoint` can be set.") | |||
| httpEndpoint = flag.String("http-endpoint", "", "The TCP network address where the HTTP server for diagnostics, including metrics and leader election health check, will listen (example: `:8080`). The default is empty string, which means the server is disabled. Only one of `--metrics-address` and `--http-endpoint` can be set.") | |||
| metricsPath = flag.String("metrics-path", "/metrics", "The HTTP path where prometheus metrics will be exposed. Default is `/metrics`.") | |||
| pollSnapshotPeriod = flag.Duration("poll-snapshot-period", 10*time.Second, "Poll interval to check if a snapshot is ready. Default is 10 seconds.") | |||
There was a problem hiding this comment.
Can you set the default to 0 so that drivers can opt-in to this new behavior?
# for free
to join this conversation on GitHub.
Already have an account?
# to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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, errpattern that is followed by other controller frameworks, like controller-runtime.requeuefield 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-periodto the csi-snapshotter, with a default of10 seconds.Which issue(s) this PR fixes:
Fixes #1282
Special notes for your reviewer:
Does this PR introduce a user-facing change?: