Call SetupDevice for Block Volume only if Volume is not globally Mounted #112403

akankshakumari393 · 2022-09-12T17:32:16Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

For raw block CSI volumes on Kubernetes, kubelet was incorrectly calling CSI NodeStageVolume for every single "map" (i.e. raw block "mount") operation for a volume already attached to the node. This PR ensures it is only called once per volume per node.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2022-09-12T17:32:25Z

Hi @akankshakumari393. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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 · 2022-09-12T17:34:49Z

/ok-to-test

saad-ali · 2022-09-12T19:42:50Z

Do we have sufficient E2E raw block test coverage? I am wondering if there are other bugs that were hidden that will surface once this is fixed.

mattcary · 2022-09-12T20:47:15Z

Do we have sufficient E2E raw block test coverage? I am wondering if there are other bugs that were hidden that will surface once this is fixed.

Most of the testsuites seem to have a block pattern. So the coverage is probably on par with file---I don't know if that's enough though. There are tests for a volume being used multiple times on the same node which are actually run, so I think we might be okay?

$ gfind test/e2e storageframework.BlockVol
test/e2e/storage/testsuites/ephemeral.go:71:		storageframework.BlockVolModeGenericEphemeralVolume,
test/e2e/storage/testsuites/volumes.go:88:		storageframework.BlockVolModePreprovisionedPV,
test/e2e/storage/testsuites/volumes.go:89:		storageframework.BlockVolModeDynamicPV,
test/e2e/storage/testsuites/disruptive.go:58:		storageframework.BlockVolModePreprovisionedPV,
test/e2e/storage/testsuites/disruptive.go:59:		storageframework.BlockVolModeDynamicPV,
test/e2e/storage/testsuites/volumemode.go:78:		storageframework.BlockVolModePreprovisionedPV,
test/e2e/storage/testsuites/volumemode.go:79:		storageframework.BlockVolModeDynamicPV,
test/e2e/storage/testsuites/provisioning.go:94:		storageframework.BlockVolModeDynamicPV,
test/e2e/storage/testsuites/volume_expand.go:79:		storageframework.BlockVolModeDynamicPV,
test/e2e/storage/testsuites/volume_expand.go:81:		storageframework.BlockVolModeDynamicPVAllowExpansion,
test/e2e/storage/testsuites/multivolume.go:66:		storageframework.BlockVolModePreprovisionedPV,
test/e2e/storage/testsuites/multivolume.go:67:		storageframework.BlockVolModeDynamicPV,
test/e2e/storage/testsuites/multivolume.go:432:		if l.driver.GetDriverInfo().Name == "vsphere" && pattern == storageframework.BlockVolModeDynamicPV {
test/e2e/storage/testsuites/volume_stress.go:78:		storageframework.BlockVolModeDynamicPV,

saad-ali · 2022-09-12T22:24:37Z

Thanks @mattcary. Coverage looks promising. And we are early enough in the release, hopefully if there are any issues, they can be fixed before the 1.26 release.

saad-ali · 2022-09-12T22:25:29Z

/lgtm
/approve

/hold

Adding hold to give others an opportunity to review. e.g. @jsafrane @xing-yang @jingxu97 @msau42

k8s-ci-robot · 2022-09-12T22:25:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: akankshakumari393, 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/util/operationexecutor/OWNERS~~ [saad-ali]

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

jingxu97 · 2022-09-12T23:51:22Z

/lgtm

saad-ali · 2022-09-13T00:25:21Z

/triage accepted
/priority important-soon

saad-ali · 2022-09-13T00:32:59Z

@akankshakumari393 Would be nice to add a test to prevent regression for this case (ok to do in a follow up PR).

akankshakumari393 · 2022-09-13T03:20:39Z

@saad-ali Sure will add a test in a follow up PR.

msau42 · 2022-09-13T04:20:53Z

pkg/volume/util/operationexecutor/operation_generator.go

@@ -1114,7 +1114,7 @@ func (og *operationGenerator) GenerateMapVolumeFunc(

 		}
 		// Call SetUpDevice if blockVolumeMapper implements CustomBlockVolumeMapper
-		if customBlockVolumeMapper, ok := blockVolumeMapper.(volume.CustomBlockVolumeMapper); ok {
+		if customBlockVolumeMapper, ok := blockVolumeMapper.(volume.CustomBlockVolumeMapper); ok && actualStateOfWorld.GetDeviceMountState(volumeToMount.VolumeName) != DeviceGloballyMounted {


Can you also test the reconstruction scenario (restart kubelet while the volume is already "mounted")?

Also the scenario where two pods share the same volume on the same node.

There is are tests with [Testpattern: Pre-provisioned PV (block volmode)] multiVolume [Slow] should concurrently access the single volume from pods on the same node that passed in pull-kubernetes-e2e-gce-storage-slow.

jsafrane · 2022-09-13T09:41:10Z

/test help
checking if we have anything that runs [Disruptive] tests.

k8s-ci-robot · 2022-09-13T09:41:16Z

@jsafrane: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test pull-kubernetes-conformance-kind-ga-only-parallel
/test pull-kubernetes-coverage-unit
/test pull-kubernetes-dependencies
/test pull-kubernetes-dependencies-go-canary
/test pull-kubernetes-e2e-gce
/test pull-kubernetes-e2e-gce-100-performance
/test pull-kubernetes-e2e-gce-big-performance
/test pull-kubernetes-e2e-gce-canary
/test pull-kubernetes-e2e-gce-network-proxy-http-connect
/test pull-kubernetes-e2e-gce-no-stage
/test pull-kubernetes-e2e-gce-scale-performance-manual
/test pull-kubernetes-e2e-gce-ubuntu-containerd
/test pull-kubernetes-e2e-gce-ubuntu-containerd-canary
/test pull-kubernetes-e2e-kind
/test pull-kubernetes-e2e-kind-ipv6
/test pull-kubernetes-files-remake
/test pull-kubernetes-integration
/test pull-kubernetes-integration-go-canary
/test pull-kubernetes-kubemark-e2e-gce-scale
/test pull-kubernetes-node-e2e-containerd
/test pull-kubernetes-typecheck
/test pull-kubernetes-unit
/test pull-kubernetes-unit-go-canary
/test pull-kubernetes-update
/test pull-kubernetes-verify
/test pull-kubernetes-verify-go-canary
/test pull-kubernetes-verify-govet-levee

The following commands are available to trigger optional jobs:

/test check-dependency-stats
/test pull-ci-kubernetes-unit-windows
/test pull-kubernetes-conformance-image-test
/test pull-kubernetes-conformance-kind-ga-only
/test pull-kubernetes-conformance-kind-ipv6-parallel
/test pull-kubernetes-cross
/test pull-kubernetes-e2e-capz-azure-disk
/test pull-kubernetes-e2e-capz-azure-disk-vmss
/test pull-kubernetes-e2e-capz-azure-file
/test pull-kubernetes-e2e-capz-azure-file-vmss
/test pull-kubernetes-e2e-capz-conformance
/test pull-kubernetes-e2e-capz-ha-control-plane
/test pull-kubernetes-e2e-capz-windows-containerd
/test pull-kubernetes-e2e-containerd-gce
/test pull-kubernetes-e2e-gce-alpha-features
/test pull-kubernetes-e2e-gce-correctness
/test pull-kubernetes-e2e-gce-csi-serial
/test pull-kubernetes-e2e-gce-device-plugin-gpu
/test pull-kubernetes-e2e-gce-kubetest2
/test pull-kubernetes-e2e-gce-network-proxy-grpc
/test pull-kubernetes-e2e-gce-storage-disruptive
/test pull-kubernetes-e2e-gce-storage-slow
/test pull-kubernetes-e2e-gce-storage-snapshot
/test pull-kubernetes-e2e-gce-ubuntu-containerd-serial
/test pull-kubernetes-e2e-gci-gce-autoscaling
/test pull-kubernetes-e2e-gci-gce-ingress
/test pull-kubernetes-e2e-gci-gce-ipvs
/test pull-kubernetes-e2e-kind-canary
/test pull-kubernetes-e2e-kind-dual-canary
/test pull-kubernetes-e2e-kind-ipv6-canary
/test pull-kubernetes-e2e-kind-ipvs-dual-canary
/test pull-kubernetes-e2e-kind-multizone
/test pull-kubernetes-e2e-kops-aws
/test pull-kubernetes-e2e-ubuntu-gce-network-policies
/test pull-kubernetes-kubemark-e2e-gce-big
/test pull-kubernetes-local-e2e
/test pull-kubernetes-node-crio-cgrpv2-e2e
/test pull-kubernetes-node-crio-cgrpv2-e2e-kubetest2
/test pull-kubernetes-node-crio-dra
/test pull-kubernetes-node-crio-e2e
/test pull-kubernetes-node-crio-e2e-kubetest2
/test pull-kubernetes-node-e2e-containerd-alpha-features
/test pull-kubernetes-node-e2e-containerd-features
/test pull-kubernetes-node-e2e-containerd-features-kubetest2
/test pull-kubernetes-node-e2e-containerd-kubetest2
/test pull-kubernetes-node-kubelet-credential-provider
/test pull-kubernetes-node-kubelet-serial-containerd
/test pull-kubernetes-node-kubelet-serial-containerd-kubetest2
/test pull-kubernetes-node-kubelet-serial-cpu-manager
/test pull-kubernetes-node-kubelet-serial-cpu-manager-kubetest2
/test pull-kubernetes-node-kubelet-serial-crio-cgroupv1
/test pull-kubernetes-node-kubelet-serial-crio-cgroupv2
/test pull-kubernetes-node-kubelet-serial-hugepages
/test pull-kubernetes-node-kubelet-serial-memory-manager
/test pull-kubernetes-node-kubelet-serial-topology-manager
/test pull-kubernetes-node-kubelet-serial-topology-manager-kubetest2
/test pull-kubernetes-node-memoryqos-cgrpv2
/test pull-kubernetes-node-swap-fedora
/test pull-kubernetes-node-swap-fedora-serial
/test pull-kubernetes-node-swap-ubuntu-serial
/test pull-kubernetes-unit-experimental
/test pull-publishing-bot-validate

Use /test all to run the following jobs that were automatically triggered:

pull-kubernetes-conformance-kind-ga-only-parallel
pull-kubernetes-dependencies
pull-kubernetes-e2e-gce-100-performance
pull-kubernetes-e2e-gce-csi-serial
pull-kubernetes-e2e-gce-storage-slow
pull-kubernetes-e2e-gce-storage-snapshot
pull-kubernetes-e2e-gce-ubuntu-containerd
pull-kubernetes-e2e-kind
pull-kubernetes-e2e-kind-ipv6
pull-kubernetes-integration
pull-kubernetes-node-e2e-containerd
pull-kubernetes-typecheck
pull-kubernetes-unit
pull-kubernetes-verify
pull-kubernetes-verify-govet-levee

In response to this:

/test help
checking if we have anything that runs [Disruptive] tests.

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.

jsafrane · 2022-09-13T09:41:42Z

/test pull-kubernetes-e2e-gce-storage-disruptive

jsafrane · 2022-09-13T14:32:24Z

Half of block volume disruptive tests succeeded and second half failed. It seems that these tests were not run for quite same time.
Trying to get "baseline flakiness" in #111367 and compare with this PR.
/test pull-kubernetes-e2e-gce-storage-disruptive

msau42 · 2022-09-13T17:47:54Z

It looks like CSI raw block reconstruction tests passed. The failing raw block tests are related to rbd and iscsi which is potentially an environmental issue

k8s-ci-robot · 2022-09-13T18:48:06Z

@akankshakumari393: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubernetes-e2e-gce-storage-disruptive	`86fa479`	link	false	`/test pull-kubernetes-e2e-gce-storage-disruptive`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

msau42 · 2022-09-13T20:37:55Z

/hold cancel
/skip
the failing tests look environmental. The CSI ones passed.

jingxu97 · 2022-09-13T23:26:42Z

The change is merged!
I think we should cherrypick this change to all supporting versions.

saad-ali · 2022-09-14T21:31:49Z

Thanks folks! @akankshakumari393 please link regression test PR when ready here.

…03-upstream-release-1.24 Automated cherry pick of #112403: Call SetupDevice only if Volume is not globally Mounted

…03-upstream-release-1.25 Automated cherry pick of #112403: Call SetupDevice only if Volume is not globally Mounted

…03-upstream-release-1.23 Automated cherry pick of #112403: Call SetupDevice only if Volume is not globally Mounted

Call SetupDevice only if Volume is not globally Mounted

86fa479

k8s-ci-robot added needs-priority Indicates a PR lacks a `priority/foo` label and requires one. sig/storage Categorizes an issue or PR as relevant to SIG Storage. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Sep 12, 2022

k8s-ci-robot requested review from Jiawei0227 and mattcary September 12, 2022 17:33

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Sep 12, 2022

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 12, 2022

k8s-ci-robot assigned saad-ali Sep 12, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 12, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 12, 2022

k8s-ci-robot assigned jingxu97 Sep 12, 2022

msau42 reviewed Sep 13, 2022

View reviewed changes

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 13, 2022

k8s-ci-robot merged commit ea4c28c into kubernetes:master Sep 13, 2022

k8s-ci-robot added this to the v1.26 milestone Sep 13, 2022

k8s-ci-robot added a commit that referenced this pull request Sep 22, 2022

Merge pull request #112494 from msau42/automated-cherry-pick-of-#1124…

cdf0e92

…03-upstream-release-1.24 Automated cherry pick of #112403: Call SetupDevice only if Volume is not globally Mounted

k8s-ci-robot added a commit that referenced this pull request Sep 22, 2022

Merge pull request #112493 from msau42/automated-cherry-pick-of-#1124…

611af0a

…03-upstream-release-1.25 Automated cherry pick of #112403: Call SetupDevice only if Volume is not globally Mounted

k8s-ci-robot added a commit that referenced this pull request Sep 30, 2022

Merge pull request #112495 from msau42/automated-cherry-pick-of-#1124…

932fab4

…03-upstream-release-1.23 Automated cherry pick of #112403: Call SetupDevice only if Volume is not globally Mounted

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Call SetupDevice for Block Volume only if Volume is not globally Mounted #112403

Call SetupDevice for Block Volume only if Volume is not globally Mounted #112403

akankshakumari393 commented Sep 12, 2022 •

edited by saad-ali

k8s-ci-robot commented Sep 12, 2022

xing-yang commented Sep 12, 2022

saad-ali commented Sep 12, 2022

mattcary commented Sep 12, 2022

saad-ali commented Sep 12, 2022

saad-ali commented Sep 12, 2022

k8s-ci-robot commented Sep 12, 2022

jingxu97 commented Sep 12, 2022

saad-ali commented Sep 13, 2022

saad-ali commented Sep 13, 2022

akankshakumari393 commented Sep 13, 2022

msau42 Sep 13, 2022

msau42 Sep 13, 2022

jsafrane Sep 13, 2022

jsafrane commented Sep 13, 2022

k8s-ci-robot commented Sep 13, 2022

jsafrane commented Sep 13, 2022

jsafrane commented Sep 13, 2022 •

edited

msau42 commented Sep 13, 2022 •

edited

k8s-ci-robot commented Sep 13, 2022

msau42 commented Sep 13, 2022

jingxu97 commented Sep 13, 2022

saad-ali commented Sep 14, 2022

Call SetupDevice for Block Volume only if Volume is not globally Mounted #112403

Call SetupDevice for Block Volume only if Volume is not globally Mounted #112403

Conversation

akankshakumari393 commented Sep 12, 2022 • edited by saad-ali

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented Sep 12, 2022

xing-yang commented Sep 12, 2022

saad-ali commented Sep 12, 2022

mattcary commented Sep 12, 2022

saad-ali commented Sep 12, 2022

saad-ali commented Sep 12, 2022

k8s-ci-robot commented Sep 12, 2022

jingxu97 commented Sep 12, 2022

saad-ali commented Sep 13, 2022

saad-ali commented Sep 13, 2022

akankshakumari393 commented Sep 13, 2022

msau42 Sep 13, 2022

Choose a reason for hiding this comment

msau42 Sep 13, 2022

Choose a reason for hiding this comment

jsafrane Sep 13, 2022

Choose a reason for hiding this comment

jsafrane commented Sep 13, 2022

k8s-ci-robot commented Sep 13, 2022

jsafrane commented Sep 13, 2022

jsafrane commented Sep 13, 2022 • edited

msau42 commented Sep 13, 2022 • edited

k8s-ci-robot commented Sep 13, 2022

msau42 commented Sep 13, 2022

jingxu97 commented Sep 13, 2022

saad-ali commented Sep 14, 2022

akankshakumari393 commented Sep 12, 2022 •

edited by saad-ali

jsafrane commented Sep 13, 2022 •

edited

msau42 commented Sep 13, 2022 •

edited