Add ability to customize VolumeSnapshotContent workqueue

[humblec]

Two new timeout values ( retryIntervalStart & retryIntervalMax )
have been added to set the ratelimiter for volumesnapshotcontent queue.

Fixes: #463

Signed-off-by: Humble Chirammal hchiramm@redhat.com

 `retry-interval-start` and `retry-interval-max` arguments are added to csi-snapshotter sidecar which controls retry interval of failed volume snapshot creation and deletion. These values set the ratelimiter for volumesnapshotcontent queue.

[humblec]

@xing-yang @yuxiangqian ptal . thanks

[xing-yang]

@humblec Thanks for adding this. Requeue currently does not work properly so adding these options won't really do anything. I'm working on a fix here: #230. After that we can get your fix in.

[humblec]

@humblec Thanks for adding this. Requeue currently does not work properly so adding these options won't really do anything. I'm working on a fix here: #230. After that we can get your fix in.

@xing-yang sure. Will wait for other PR to go in first.

[xing-yang]

@humblec can you rebase?

[humblec]

@humblec can you rebase?

@xing-yang Sure, it is done. 👍 ptal.

[xing-yang]

Please add a release note as this PR adds 2 user facing config parameters.

[humblec]

@xing-yang comments are addressed and also added release notes. Please let me know if any changes are required in any of it.

[yuxiangqian]

I am a little concerned for this change as it would change the behavior of the rateLimiter slightly.

1. how should this be effectively tested? I understand there are no existing tests to cover, but can we consider covering in this PR?
2. the original implementation uses the default controller rate limiter, which under the hood is just a combination of exponential failure rate limiter and a bucket rate limiter. For the exponential failure rate limiter, it uses 1 ms as the base. In this PR, the default base is 1s.
   What are the main purposes of this change? is it just to allow users to specify the base and max-retry interval?

[humblec]

@yuxiangqian regarding the name of these parameters: While I agree that we could change or use some strings like retry-interval-base ..etc, we are actually deviating from the same parameters available in other sidecars and the parity between those. For ex: external provisioner/attacher (https://github.com/kubernetes-csi/external-attacher) too have these parameters. Regarding the default value, we could change that to 1ms or whatever preferred value , Is 1ms good enough?

[humblec]

@yuxiangqian regarding the name of these parameters: While I agree that we could change or use some strings like retry-interval-base ..etc, we are actually deviating from the same parameters available in other sidecars and the parity between those. For ex: external provisioner/attacher (https://github.com/kubernetes-csi/external-attacher) too have these parameters. Regarding the default value, we could change that to 1ms or whatever preferred value , Is 1ms good enough?

@xing-yang can you share your thoughts.. I would like to get some consensus and hopefully finish this PR .

[xing-yang]

@yuxiangqian regarding the name of these parameters: While I agree that we could change or use some strings like retry-interval-base ..etc, we are actually deviating from the same parameters available in other sidecars and the parity between those. For ex: external provisioner/attacher (https://github.com/kubernetes-csi/external-attacher) too have these parameters. Regarding the default value, we could change that to 1ms or whatever preferred value , Is 1ms good enough?

@xing-yang can you share your thoughts.. I would like to get some consensus and hopefully finish this PR .

I agree that the naming in externa-snapshotter should be consistent with that in other sidecars. @yuxiangqian are you okay with this PR?

[yuxiangqian]

@yuxiangqian regarding the name of these parameters: While I agree that we could change or use some strings like retry-interval-base ..etc, we are actually deviating from the same parameters available in other sidecars and the parity between those. For ex: external provisioner/attacher (https://github.com/kubernetes-csi/external-attacher) too have these parameters. Regarding the default value, we could change that to 1ms or whatever preferred value , Is 1ms good enough?

@humblec dropped the ball a bit, my apologies. If its consistent with others, I am good with the name.
I still have concerns over the default value of 1s as users likely would just rely on the default. Snapshot is latency sensitive and that's different from attacher. I'd suggest add some tests to that and default to the original 1ms starting. and in framework_test.go, should we also use the same ratelimiter as in main.go.

[fejta-bot]

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-contributor-experience at kubernetes/community.
/close

[k8s-ci-robot]

@fejta-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-contributor-experience at kubernetes/community.
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[xing-yang]

@humblec can you reference this issue #463 in the top section of this PR?

[humblec]

@humblec can you reference this issue #463 in the top section of this PR?

Sure thing @xing-yang and thanks for reopening this PR, let me rebase and finish it off 👍

[humblec]

The default value of the ratelimiter has been set to 1s. Apart from that, one thing to note here is, framework testing does not make use of this new rate limiter yet ie it use the defaultRateLimiter. If required, I can change framework, otherwise we are good with the change .

[xing-yang]

/remove-lifecycle rotten

[humblec]

@xing-yang ptal.. Thanks !

[xing-yang]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

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

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