Delay CSI client initialization

[cofyc]

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]

/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]

/retest

[msau42]

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

[msau42]

/test pull-kubernetes-e2e-gce

[msau42]

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

[cofyc]

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]

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]

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]

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]

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

[cofyc]

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]

/retest
I’m looking at failing tests.

[k8s-ci-robot]

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

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

[msau42]

/lgtm
/hold cancel

[tedyu]

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

[cofyc]

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

[cofyc]

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.