Increase timeout of csi-provisioner and csi attacher #93

awels · 2023-09-21T20:00:44Z

What this PR does / why we need it:
Increased timeout of csi-provisioner and csi-attacher. This fixes random failures in the k8s e2e test. Cleaned up some linter issues with using deprecated functions.
Ignore server rejection errors from kubevirt addvolume/removevolume and retry them.
Put proper log levels on messages so it is easier to control the
logging in the csi-driver pod. Added some extra logging to easier identify issues once the log level is set high enough.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:
See these links for what the arguments do
https://github.com/kubernetes-csi/external-provisioner
https://github.com/kubernetes-csi/external-attacher

Release note:

Increased timeout for csi-provisioner and csi-attacher

csi-attacher. This fixes random failures in the k8s e2e test. Cleaned up some linter issues with using deprecated functions. Ignore server rejection errors from kubevirt addvolume/removevolume and retry them. Signed-off-by: Alexander Wels <awels@redhat.com>

awels · 2023-09-21T20:07:33Z

/test pull-csi-driver-linter

davidvossel · 2023-09-21T20:18:47Z

cmd/kubevirt-csi-driver/kubevirt-csi-driver.go

@@ -60,7 +58,6 @@ func init() {

 func main() {
 	flag.Parse()
-	rand.Seed(time.Now().UnixNano())


why was this removed?

Because it is not needed anymore, rand.Seed is deprecated, and according to the documentation if you don't initialize the Random with a particular seed, it gets a random seed on startup.

Okay so the automatic seed population doesn't happen automatically until golang 1.20, I will restore this until we update golang to be 1.20

davidvossel · 2023-09-21T20:29:31Z

pkg/service/controller.go

+	if attached, err := c.isVolumeAttached(dvName, vmName); err != nil {
+		return nil, err


I don't think you need this check. The c.addVolumeToVm() invocation immediately checks isVolumeAttached() before attempting to add the volume

Good point, removed.

davidvossel · 2023-09-21T20:30:55Z

pkg/service/controller.go

+		retryCount := 6
+		for retryCount > 0 {
+			retryCount--


typically we use wait.PollImmediate for loops like this.

Good point, added, less code also.

davidvossel · 2023-09-21T20:32:09Z

pkg/service/controller.go

+	if attached, err := c.isVolumeAttached(dvName, vmName); err != nil {
+		return nil, err
+	} else if attached {


the removeVolumeFromVM() already checks to see if the volume is attached before attempting to remove it.

Good point, removed.

davidvossel · 2023-09-21T20:32:19Z

pkg/service/controller.go

+		for retryCount > 0 {
+			retryCount--


another candidate for wait.PollImmediate

Aligned retry-interval-max between csi-provisioner and csi-attacher Signed-off-by: Alexander Wels <awels@redhat.com>

awels · 2023-09-22T13:50:02Z

/retest-required

Signed-off-by: Alexander Wels <awels@redhat.com>

awels · 2023-09-22T18:29:21Z

/test pull-csi-driver-sanity-test

davidvossel

/lgtm
/approve

kubevirt-bot · 2023-09-22T19:53:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: davidvossel

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

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

kubevirt/csi-driver#93 updated the timeouts of the csi-provisioner and csi-attacher to be 3 minutes instead of 15 seconds to avoid the timeout happening while waiting for disk to hotplug. This commit modifies the controller deployment and daemonset to match that PR Signed-off-by: Alexander Wels <awels@redhat.com>

* Increased timeout of csi-provisioner and csi-attacher. This fixes random failures in the k8s e2e test. Cleaned up some linter issues with using deprecated functions. Ignore server rejection errors from kubevirt addvolume/removevolume and retry them. Signed-off-by: Alexander Wels <awels@redhat.com> * Address review comments. Aligned retry-interval-max between csi-provisioner and csi-attacher Signed-off-by: Alexander Wels <awels@redhat.com> * Use exponetial backoff instead of poll immediate Signed-off-by: Alexander Wels <awels@redhat.com> --------- Signed-off-by: Alexander Wels <awels@redhat.com>

kubevirt/csi-driver#93 updated the timeouts of the csi-provisioner and csi-attacher to be 3 minutes instead of 15 seconds to avoid the timeout happening while waiting for disk to hotplug. This commit modifies the controller deployment and daemonset to match that PR Signed-off-by: Alexander Wels <awels@redhat.com>

kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Sep 21, 2023

kubevirt-bot requested review from mhenriks and ydayagi September 21, 2023 20:00

kubevirt-bot added the size/L label Sep 21, 2023

awels changed the title ~~Increased timeout of csi-provisioner and~~ Increased timeout of csi-provisioner and csi attacher Sep 21, 2023

awels changed the title ~~Increased timeout of csi-provisioner and csi attacher~~ Increase timeout of csi-provisioner and csi attacher Sep 21, 2023

davidvossel reviewed Sep 21, 2023

View reviewed changes

Address review comments.

1c92791

Aligned retry-interval-max between csi-provisioner and csi-attacher Signed-off-by: Alexander Wels <awels@redhat.com>

awels force-pushed the retry_invalid_request branch from d1d6bd3 to 1c92791 Compare September 22, 2023 13:31

Use exponetial backoff instead of poll immediate

f7994a8

Signed-off-by: Alexander Wels <awels@redhat.com>

davidvossel reviewed Sep 22, 2023

View reviewed changes

kubevirt-bot assigned davidvossel Sep 22, 2023

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Sep 22, 2023

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 22, 2023

kubevirt-bot merged commit 8250632 into kubevirt:main Sep 22, 2023
7 of 8 checks passed

awels mentioned this pull request Sep 25, 2023

Update kubevirt csi driver deployment with proper timeouts openshift/hypershift#3044

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Increase timeout of csi-provisioner and csi attacher #93

Increase timeout of csi-provisioner and csi attacher #93

awels commented Sep 21, 2023 •

edited

Loading

awels commented Sep 21, 2023

davidvossel Sep 21, 2023

awels Sep 21, 2023

awels Sep 22, 2023

davidvossel Sep 21, 2023

awels Sep 22, 2023

davidvossel Sep 21, 2023

awels Sep 22, 2023

davidvossel Sep 21, 2023

awels Sep 22, 2023

davidvossel Sep 21, 2023

awels Sep 22, 2023

awels commented Sep 22, 2023

awels commented Sep 22, 2023

davidvossel left a comment

kubevirt-bot commented Sep 22, 2023

		if attached, err := c.isVolumeAttached(dvName, vmName); err != nil {
		return nil, err

Increase timeout of csi-provisioner and csi attacher #93

Increase timeout of csi-provisioner and csi attacher #93

Conversation

awels commented Sep 21, 2023 • edited Loading

awels commented Sep 21, 2023

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

Choose a reason for hiding this comment

awels commented Sep 22, 2023

awels commented Sep 22, 2023

davidvossel left a comment

Choose a reason for hiding this comment

kubevirt-bot commented Sep 22, 2023

awels commented Sep 21, 2023 •

edited

Loading