Replace `csiTimeout` to `timeout` param for resizer #82

humblec · 2020-05-21T17:54:05Z

At present the resizer has CSI call timeout value
taken as arg in form of --csiTimeout , however all
other sidecars have this parameter as --timeout.
This patch replaces csiTimeout to timeout value

NOTE: This is a breaking change, so we need a new release

The `--csiTimeout` flag of external-resizer is obsoleted with this release. Instead `--timeout` flag has been introduced for the same purpose. The   timeout value  accommodates majority of `ControllerExpandVolume` calls. `timeout` value defaults to 10 seconds. From now on, the deployment  templates or  artifacts has to make use of `--timeout` flag instead of obsoleted `--csiTimeout`  flag.

Signed-off-by: Humble Chirammal [email protected]

Madhu-1 · 2020-05-22T04:20:33Z

Fixes #72

cmd/csi-resizer/main.go

gnufied · 2020-08-05T19:35:09Z

@humblec can you update this PR and rebase it?

humblec · 2020-08-06T08:58:10Z

@humblec can you update this PR and rebase it?

Sure. rebased @gnufied . PTAL.

gnufied · 2020-08-06T12:29:56Z

cmd/csi-resizer/main.go

@@ -99,7 +102,7 @@ func main() {

 	csiResizer, err := resizer.NewResizer(
 		*csiAddress,
-		*csiTimeout,
+		*timeout,


this means that if someone was using deprecated parameter - csiTimeout, it will simply be ignored. Can you fix it?

Since in this release we are only deprecating the older csiTimeout parameter (as evident from CLI help flags), but if we remove the flag in the same release we will have problems.

alternately - if we are not going to support csiTimeout flag anymore, we should remove it alogether. But I am in favour of deprecating the flag for now and removing it later. cc @msau42

If someone sets csiTimeout, can we have it set timeout?

I'm also ok with just removing the flag because we're doing a major release anyway.

Ok.. let me try to explain what I was thinking. Being this is a major release, we could definitely mark csitimeout param as a "deprecated" one , also we could mention in the release note that if someone set this value , it will be NO-OP and the default timeout value in the external resizer will be used. The advantage with this approach is that, the older templates ( which set csiTimeout ) in some customer/user setup could also work , but its a NO-OP . If they set timeout while using new templates in parity with this release, it will be effective as well. Isnt it a good idea to address this way? @gnufied @msau42 wdyt?

that would be problematic. It would seem like we still support csiTimeout option but in truth we don't. If we are going to not support csiTimeout option then we should just remove it.

@gnufied @msau42 ok.. are we in agreement to completely remove csiTimeout ?

@gnufied ptal on current version.. completely removed csiTimeout.

hmm, I think what @msau42 meant was 15s itself is too low of a default (and hence 10s will be even lower) and hence is it worth forcing COs/cluster-admins to specify a value rather than choosing a non-ideal default?

I see @msau42 point but I kinda like the idea of defaults, but I admit 15s is not good default for volume expansion. I wonder if we can increase the default to something like 80s which would work in most cases and if certain drivers have problems they can tune it.

Also this PR will require fixing all our e2e jobs btw.

gnufied · 2020-08-07T12:58:25Z

/lgtm

gnufied · 2020-08-07T13:10:27Z

@humblec can you add a release note plz?

humblec · 2020-08-07T14:02:17Z

@gnufied does this looks good as release note for this?


The `--csiTimeout` flag of external-resizer is obsoleted with this release. Instead `--timeout` flag has been introduced for the same purpose. The   timeout value  accommodates majority of `ControllerExpandVolume` calls. `timeout` value defaults to 10 seconds. From now on, the deployment  templates or  artifacts has to make use of `--timeout` flag instead of obsoleted `--csiTimeout`  flag.

msau42 · 2020-08-07T16:19:50Z

cmd/csi-resizer/main.go

-	csiAddress  = flag.String("csi-address", "/run/csi/socket", "Address of the CSI driver socket.")
-	csiTimeout  = flag.Duration("csiTimeout", 15*time.Second, "Timeout for waiting for CSI driver socket.")
+	csiAddress = flag.String("csi-address", "/run/csi/socket", "Address of the CSI driver socket.")
+	timeout    = flag.Duration("timeout", 15*time.Second, "Timeout for waiting for CSI driver socket.")


How come 15? All the other sidecars are 10 seconds. Actually, do we want to default a timeout value? I find 10 seconds is quite short for real drivers. xref: kubernetes-csi/external-provisioner#451 (comment)

@msau42 sure we can keep in parity between sidecars ie 10 seconds. I kept it 15 seconds as obsoleted csitimeout default was "15s".

ok.. I changed to 10s default @msau42 @gnufied

Also - I kind of dislike, renaming the parameter and making it required in same release. It causes problems for downstream users. So, if we choose to make this parameter mandatory, we should do this in 2.0 IMO.

@msau42 does 80s as a default sound good to you? The reason I picked this is because, from my experience this is "good enough" default for most in-tree plugins for which there is a equivalent CSI driver. If we all agree @humblec can you update the PR to use 80s as default timeout?

sure @gnufied , so once @msau42 ack on 80s , I will change the default value to the same 👍

Also, I do second to 2.0.0 with this new parameter instead of changing it in the minor release.

we spoke in CSI implementation call today. It was decided that, 10s is a good default because it exposes driver idempotency issues and specific drivers can tune this value.

Thanks for the update @gnufied ! 👍

At present the resizer has CSI call timeout value taken as arg in form of `--csiTimeout` , however all other sidecars have this parameter as `--timeout`. This patch replaces `csiTimeout` to `timeout` value NOTE: This is a breaking change, so we need a new release Signed-off-by: Humble Chirammal <[email protected]>

gnufied · 2020-08-10T17:25:06Z

/lgtm

gnufied · 2020-08-10T18:56:03Z

/approve

k8s-ci-robot · 2020-08-10T18:56:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gnufied, humblec

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~~ [gnufied]

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

msau42 · 2020-08-10T20:42:33Z

cmd/csi-resizer/main.go

@@ -44,8 +45,9 @@ var (
 	resyncPeriod = flag.Duration("resync-period", time.Minute*10, "Resync period for cache")
 	workers      = flag.Int("workers", 10, "Concurrency to process multiple resize requests")

-	csiAddress  = flag.String("csi-address", "/run/csi/socket", "Address of the CSI driver socket.")
-	csiTimeout  = flag.Duration("csiTimeout", 15*time.Second, "Timeout for waiting for CSI driver socket.")
+	csiAddress = flag.String("csi-address", "/run/csi/socket", "Address of the CSI driver socket.")


Since we're going to ignore this csiAddress flag, should we use the deprecatedflag library?

wait, why is csiAddress being ignored?

sorry this got merged before this has chance to get addressed. but we can do a follow up.

humblec · 2020-08-11T15:08:28Z

/test pull-kubernetes-csi-external-resizer-1-16-on-kubernetes-1-16

We are using a really old csi resizer image, that doesn't yet support the new `timeout` param, instead it relies on an old `csiTimeout` param. Decided against updating the csi images and instead will do a full update of all the csi images for 1.2. See kubernetes-csi/external-resizer#82 Longhorn longhorn#2347 Signed-off-by: Joshua Moody <[email protected]>

We are using a really old csi resizer image, that doesn't yet support the new `timeout` param, instead it relies on an old `csiTimeout` param. Decided against updating the csi images and instead will do a full update of all the csi images for 1.2. See kubernetes-csi/external-resizer#82 Longhorn #2347 Signed-off-by: Joshua Moody <[email protected]>

k8s-ci-robot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 21, 2020

k8s-ci-robot requested review from gnufied and msau42 May 21, 2020 17:54

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label May 21, 2020

Madhu-1 reviewed May 22, 2020

View reviewed changes

cmd/csi-resizer/main.go Outdated Show resolved Hide resolved

humblec force-pushed the new branch from 250910b to f8a0c27 Compare May 22, 2020 13:04

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 28, 2020

humblec force-pushed the new branch from f8a0c27 to 3dfa126 Compare August 6, 2020 08:57

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 6, 2020

gnufied reviewed Aug 6, 2020

View reviewed changes

humblec force-pushed the new branch from 3dfa126 to d482993 Compare August 7, 2020 12:22

k8s-ci-robot assigned gnufied Aug 7, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 7, 2020

msau42 reviewed Aug 7, 2020

View reviewed changes

humblec force-pushed the new branch from d482993 to 77f4ef8 Compare August 7, 2020 16:41

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 10, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 10, 2020

msau42 reviewed Aug 10, 2020

View reviewed changes

k8s-ci-robot merged commit ae8826a into kubernetes-csi:master Aug 11, 2020

msau42 mentioned this pull request Sep 10, 2020

Arg "--csiTimeout" is not consistent with other sidecars #72

Closed

joshimoo mentioned this pull request Mar 15, 2021

Fix csi resizer timeout param usage longhorn/longhorn-manager#847

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace `csiTimeout` to `timeout` param for resizer #82

Replace `csiTimeout` to `timeout` param for resizer #82

humblec commented May 21, 2020 •

edited

Loading

Madhu-1 commented May 22, 2020

gnufied commented Aug 5, 2020

humblec commented Aug 6, 2020

gnufied Aug 6, 2020

gnufied Aug 6, 2020

gnufied Aug 6, 2020

msau42 Aug 6, 2020

humblec Aug 6, 2020

gnufied Aug 6, 2020

humblec Aug 7, 2020

gnufied Aug 7, 2020

humblec Aug 7, 2020

gnufied Aug 10, 2020

gnufied commented Aug 7, 2020

gnufied commented Aug 7, 2020

humblec commented Aug 7, 2020 •

edited

Loading

msau42 Aug 7, 2020

humblec Aug 7, 2020

humblec Aug 7, 2020

gnufied Aug 10, 2020

humblec Aug 10, 2020

humblec Aug 10, 2020

gnufied Aug 10, 2020

humblec Aug 10, 2020

gnufied commented Aug 10, 2020

gnufied commented Aug 10, 2020

k8s-ci-robot commented Aug 10, 2020

msau42 Aug 10, 2020

gnufied Aug 11, 2020

gnufied Aug 11, 2020

humblec commented Aug 11, 2020

Replace csiTimeout to timeout param for resizer #82

Replace csiTimeout to timeout param for resizer #82

Conversation

humblec commented May 21, 2020 • edited Loading

Madhu-1 commented May 22, 2020

gnufied commented Aug 5, 2020

humblec commented Aug 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnufied commented Aug 7, 2020

gnufied commented Aug 7, 2020

humblec commented Aug 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnufied commented Aug 10, 2020

gnufied commented Aug 10, 2020

k8s-ci-robot commented Aug 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humblec commented Aug 11, 2020

Replace `csiTimeout` to `timeout` param for resizer #82

Replace `csiTimeout` to `timeout` param for resizer #82

humblec commented May 21, 2020 •

edited

Loading

humblec commented Aug 7, 2020 •

edited

Loading