Delay CSI client initialization #74652

cofyc · 2019-02-27T10:45:14Z

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change
/kind bug
/kind cleanup
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:

#72500 (comment)

Which issue(s) this PR fixes:

Fixes #72500

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Delay CSI client initialization to make reconstruction of CSI volume possible because clients may not be available on kubelet restart.

cofyc · 2019-02-27T11:13:40Z

/priority important-soon
/sig storage
/assign @msau42 @jingxu97

Note that reconstructVolume will still fail because operationExecutor.CheckVolumeExistenceOperation checks on an invalid path, which can be fixed by this PR: #74653.

msau42 · 2019-02-27T19:00:20Z

/retest

msau42 · 2019-02-27T19:00:52Z

/test pull-kubernetes-e2e-gce-csi-serial

msau42 · 2019-02-27T20:17:48Z

/test pull-kubernetes-e2e-gce

msau42 · 2019-02-28T01:26:53Z

/test pull-kubernetes-e2e-gce-csi-serial

cofyc · 2019-02-28T03:06:15Z

This PR should work with #74653 to fix [sig-storage] CSI Volumes [Driver: pd.csi.storage.gke.io][Serial] [Testpattern: Dynamic PV (default fs)] subPath should unmount if pod is force deleted while kubelet is down [Disruptive][Slow] flakiness.

cofyc · 2019-02-28T03:10:09Z

How about review and merge #74653 first? I guess the issue #74653 trying to resolve is obvious (that's why I separated them into two PRs).

msau42 · 2019-02-28T22:07:51Z

I noticed that the skip attach check is also called early. Would this have any problem during reconstruction? I guess during reconstruction we only call mount/unmount/map/unmap. And this attachable check will end up failing later during reconciliation.

msau42 · 2019-03-01T01:46:04Z

cc @jsafrane to think about the skip attach case when the driver/kubelet restarts and is the driver is not available by the time VolumesAreAttached/WaitForAttach is called.

msau42 · 2019-03-01T02:10:17Z

Maybe skip attach is ok because it doesn't call the csi driver, it gets the K8s api object.

cofyc · 2019-03-01T02:12:09Z

At before, reconciler.reconstructVolume will fail in NewMounter and NewBlockVolumeMapper because driver plugin may not be registered by plugin watcher (so I decided to delay CSI client initialization in this PR) and in CheckVolumeExistenceOperation because it checks on an invalid path (#74653).

If it succeeds reconstructing CSI volume information from the host, CSI volumes will be put into the actual and desired state of the world.

After this, reconciliation main loop may still fail because CSI driver CRD is not synced or CSI driver client is not discovered by plugin watcher yet. But the difference is it will retry again now. When CSI driver CRD is synced and CSI client is discovered, the operation will succeed.

cofyc · 2019-03-08T14:04:04Z

/retest
I’m looking at failing tests.

k8s-ci-robot · 2019-03-09T05:47:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cofyc, msau42, saad-ali

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:

~~pkg/volume/csi/OWNERS~~ [msau42,saad-ali]

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

msau42 · 2019-03-11T05:14:22Z

/test pull-kubernetes-e2e-gce-csi-serial

msau42 · 2019-03-11T15:28:30Z

/lgtm
/hold cancel

tedyu · 2019-03-12T02:28:59Z

pkg/volume/csi/csi_client.go

+	if c.csiClient != nil {
+		return c.csiClient, nil
+	}
+	csi, err := newCsiDriverClient(c.driverName)


Can c.csiClient be assigned directly (we have the write lock) ?

you mean c.csiClient, err := newCsiDriverClient(c.driverName)?
Yes, it is equal, no much difference.

there is one difference, with the second style newCsiDriverClient(c.driverName) must return nil client on error, with the first style we do not need make such assumption.

…upstream-release-1.13 Automated cherry pick of #74652: Delay CSI client initialization

cofyc mentioned this pull request Feb 27, 2019

CSI plugin registration problem after kublet restarts #72500

Closed

k8s-ci-robot requested review from saad-ali and verult February 27, 2019 10:45

k8s-ci-robot added sig/storage Categorizes an issue or PR as relevant to SIG Storage. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Feb 27, 2019

cofyc changed the title ~~Delay CSI client initialization~~ WIP: Delay CSI client initialization Feb 27, 2019

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 27, 2019

cofyc changed the title ~~WIP: Delay CSI client initialization~~ Delay CSI client initialization Feb 27, 2019

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 27, 2019

k8s-ci-robot assigned jingxu97 and msau42 Feb 27, 2019

k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Feb 27, 2019

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 8, 2019

cofyc force-pushed the fix72500 branch from a0d2c59 to 268d5ee Compare March 8, 2019 11:47

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

cofyc force-pushed the fix72500 branch from 268d5ee to 126e4cd Compare March 9, 2019 05:37

Delay CSI client initialization

fa926ed

cofyc force-pushed the fix72500 branch from 126e4cd to fa926ed Compare March 9, 2019 05:45

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 9, 2019

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Mar 11, 2019

k8s-ci-robot merged commit 8477c48 into kubernetes:master Mar 11, 2019

krmayankk mentioned this pull request Mar 12, 2019

[Umbrella issue for tracking] ci-kubernetes-e2e-gce-new-master-upgrade-cluster is flaky in 1.14 #75239

Closed

tedyu reviewed Mar 12, 2019

View reviewed changes

cofyc mentioned this pull request Mar 13, 2019

[Flaking Test] [sig-storage] CSI Volumes [Driver: csi-hostpath-v0] [Testpattern: Dynamic PV (default fs)] subPath should unmount if pod is gracefully deleted while kubelet is down [Disruptive][Slow] #75196

Closed

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Mar 14, 2019

cofyc mentioned this pull request Mar 14, 2019

Automated cherry pick of #74652: Delay CSI client initialization #75366

Merged

k8s-ci-robot added a commit that referenced this pull request Mar 19, 2019

Merge pull request #75366 from cofyc/automated-cherry-pick-of-#74652-…

693b636

…upstream-release-1.13 Automated cherry pick of #74652: Delay CSI client initialization

cofyc deleted the fix72500 branch May 4, 2019 07:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delay CSI client initialization #74652

Delay CSI client initialization #74652

cofyc commented Feb 27, 2019 •

edited

Loading

cofyc commented Feb 27, 2019 •

edited

Loading

msau42 commented Feb 27, 2019

msau42 commented Feb 27, 2019

msau42 commented Feb 27, 2019

msau42 commented Feb 28, 2019

cofyc commented Feb 28, 2019

cofyc commented Feb 28, 2019

msau42 commented Feb 28, 2019

msau42 commented Mar 1, 2019

msau42 commented Mar 1, 2019

cofyc commented Mar 1, 2019 •

edited

Loading

cofyc commented Mar 8, 2019

k8s-ci-robot commented Mar 9, 2019

msau42 commented Mar 11, 2019

msau42 commented Mar 11, 2019

tedyu Mar 12, 2019

cofyc Mar 12, 2019

cofyc Mar 12, 2019

Delay CSI client initialization #74652

Delay CSI client initialization #74652

Conversation

cofyc commented Feb 27, 2019 • edited Loading

cofyc commented Feb 27, 2019 • edited Loading

msau42 commented Feb 27, 2019

msau42 commented Feb 27, 2019

msau42 commented Feb 27, 2019

msau42 commented Feb 28, 2019

cofyc commented Feb 28, 2019

cofyc commented Feb 28, 2019

msau42 commented Feb 28, 2019

msau42 commented Mar 1, 2019

msau42 commented Mar 1, 2019

cofyc commented Mar 1, 2019 • edited Loading

cofyc commented Mar 8, 2019

k8s-ci-robot commented Mar 9, 2019

msau42 commented Mar 11, 2019

msau42 commented Mar 11, 2019

tedyu Mar 12, 2019

Choose a reason for hiding this comment

cofyc Mar 12, 2019

Choose a reason for hiding this comment

cofyc Mar 12, 2019

Choose a reason for hiding this comment

cofyc commented Feb 27, 2019 •

edited

Loading

cofyc commented Feb 27, 2019 •

edited

Loading

cofyc commented Mar 1, 2019 •

edited

Loading