Skip unused volumes in VolumeManager #81163

jsafrane · 2019-08-08T14:02:07Z

DesiredStateOfWorldPopulator should skip a volume that is not used in any pod. "Used" means either mounted (via volumeMounts) or used as raw block device (via volumeDevices) in a container in the pod.

Especially when block feature is disabled, a block volume must not get into DesiredStateOfWorld, because it would be formatted and mounted there, potentially overwriting any existing raw block data.

/kind bug
Fixes #76044

Does this PR introduce a user-facing change?:

Volumes specified in a pod but not used in it are no longer unnecessarily formatted, mounted and reported in `node.status.volumesInUse`.

Change of Kubernetes behavior
Previously, all volumes were reported in node.status.volumesInUse and they were mounted / mapped as raw block devices in kubelet. With this PR, only the volumes that are actually used will be reported there and mounted/mapped. In most cases this won't make a difference (why would anyone put a volume to a pod and then not use it in any container?)

cc: @wongma7 @msau42 @bertinatto

wongma7 · 2019-08-08T16:50:00Z

My old attempt to fix this looks extremely convoluted... What happens in kubelet-side WaitForAttachAndMount since it also reads spec.volumes?

kubernetes/pkg/kubelet/volumemanager/volume_manager.go

Line 440 in b409073

for _, podVolume := range pod.Spec.Volumes {

mattjmcnaughton · 2019-08-08T19:09:22Z

I believe that the pull-kubernetes-bazel-test failure is legit:

 --- FAIL: TestGetMountedVolumesForPodAndGetVolumesInUse (123.00s)
    volume_manager_test.go:80: Expected success: unmounted volumes=[vol1], unattached volumes=[vol1]: timed out waiting for the condition

My guess is that the other failures are flake... retesting them now to verify.

/test pull-kubernetes-integration
/test pull-kubernetes-kubemark-e2e-gce-big
/test pull-kubernetes-kubemark-e2e-gce-csi-serial

wongma7 · 2019-08-08T20:21:57Z

Yes, test failure is legit, @jsafrane volumemanager needs some kind of fix as well since it gets list of volumes to wait for from spec.volumes instead of desired state. I'm sure you can fix it in a nicer way than I did.

msau42 · 2019-08-09T00:51:36Z

/assign @jingxu97

jsafrane · 2019-08-09T13:30:37Z

volumemanager needs some kind of fix as well since it gets list of volumes to wait for from spec.volumes instead of desired state. I'm sure you can fix it in a nicer way than I did.

You're right, I reworked getExpectedVolumes a bit. I used the simplest solution I could think of, not sure it's nicer.

In addition, I moved most of the code to processPodVolumes(), the code got a bit cleaner, but then I needed to pass fake BlockVolumePathHandler to NewVolumeManager for unit tests, so the PR grew a lot...

/hold

TODO:

Add e2e test with unused volume
Squash the commits

jsafrane · 2019-08-12T13:24:50Z

@wongma7, can you please take a look at the current status? I'll squash the commits if this is the way to go.

I could not find any elegant way how to e2e test this. From storage test I can't get the global mount directory or name to check in node.status.volumesInUse easily. I could compare volumesInUse before and after a test pod was created, but that 1) would imply [Serial], 2) it's quite ugly and 3) I am not sure it would work for all plugins. Any ideas?

wongma7 · 2019-08-12T17:47:14Z

IMO it doesn't need to work for all plugins. Maybe just gcepd, e.g.

kubernetes/test/e2e/storage/pd.go

Line 616 in 1addcf4

func waitForPDInVolumesInUse(

.

Initially I was thinking to ssh into the node and parse the mounts. Then you could e.g. make sure that the test pod UID has 0 (non secret/token etc.)* mounts.

wongma7 · 2019-08-12T17:53:30Z

pkg/kubelet/volumemanager/volume_manager.go


-	for _, podVolume := range pod.Spec.Volumes {
-		expectedVolumes = append(expectedVolumes, podVolume.Name)
+	for _, container := range pod.Spec.Containers {


what about initContainers?

Good catch!

I copied the code from VolumeManager and it observes only .containers, not .initContainers. Filed #81343.

I think the best would be to fix makeVolumeMap in #81343 first (to have a small patch), backport it everywhere and then move it to pkg/volume/utils that will be used also here.

Wrote too early, the issue is reproducible only in this PR.

I extracted GetPodVolumeNames and used it consistently in VolumeManager and DSWP.

wongma7 · 2019-08-12T18:02:47Z

pkg/kubelet/volumemanager/volume_manager_test.go

+			pvMode:      v1.PersistentVolumeBlock,
+			podMode:     v1.PersistentVolumeFilesystem,
+			expectMount: false,
+			expectError: true,


where does the error come from, is it a timeout or won't expectedVolumes have len 0?

It's not timeout, DSWP has stored error into DSW and WaitForAttachAndMount asynchronously picked it up within ~podAttachAndMountRetryInterval (300ms)

jsafrane · 2019-08-13T13:25:47Z

Initially I was thinking to ssh into the node and parse the mounts. Then you could e.g. make sure that the test pod UID has 0 (non secret/token etc.)* mounts.

Ha, we mount unused volumes even to pod directory. I can test that.

The function will be handy in subsequent patches. Also change custom maps into sets.String.

DesiredStateOfWorldPopulator should skip a volume that is not used in any pod. "Used" means either mounted (via volumeMounts) or used as raw block device (via volumeDevices). Especially when block feature is disabled, a block volume must not get into DesiredStateOfWorld, because it would be formatted and mounted there.

jsafrane · 2019-08-15T09:07:58Z

Added e2e test. Now it should be complete.

jsafrane · 2019-08-15T10:47:43Z

e2e test added, commits squashed a bit
/hold cancel

jsafrane · 2019-08-15T10:48:06Z

/retest

msau42 · 2019-08-15T16:03:42Z

pkg/volume/util/util.go

+	mounts = sets.NewString()
+	devices = sets.NewString()
+
+	addContainerVolumes(pod.Spec.Containers, mounts, devices)


This may need to be updated again after #59484

wongma7 · 2019-08-15T17:25:37Z

/lgtm
beautiful!

jsafrane · 2019-08-16T08:29:03Z

/assign @tallclair @derekwaynecarr
for approval from node point of view

Note that there is a small behavior change - before this PR, kubelet mounted / mapped as raw block everything that was in pod.spec.volumes, now it mounts / maps only things that are in pod.spec.containers[*].volumeMounts / volumeDevices.

derekwaynecarr · 2019-08-16T15:13:18Z

kubelet changes lgtm.

/lgtm
/approve

k8s-ci-robot · 2019-08-16T15:19:12Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: derekwaynecarr, jsafrane

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/kubelet/OWNERS~~ [derekwaynecarr]
~~pkg/volume/util/OWNERS~~ [jsafrane]
~~test/e2e/storage/OWNERS~~ [jsafrane]

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

fejta-bot · 2019-08-16T19:36:56Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

k8s-ci-robot requested review from msau42 and rootfs August 8, 2019 14:03

k8s-ci-robot assigned jingxu97 Aug 9, 2019

jsafrane force-pushed the skip-unused-volumes branch from 7506c53 to fb4c781 Compare August 9, 2019 13:25

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

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

jsafrane force-pushed the skip-unused-volumes branch from fb4c781 to 46589a0 Compare August 9, 2019 13:42

wongma7 reviewed Aug 12, 2019

View reviewed changes

jsafrane mentioned this pull request Aug 13, 2019

Pod with a volume used only in initContainers can't start #81343

Closed

jsafrane force-pushed the skip-unused-volumes branch from 46589a0 to a572ff0 Compare August 13, 2019 10:49

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 13, 2019

k8s-ci-robot added area/e2e-test-framework Issues or PRs related to refactoring the kubernetes e2e test framework area/test sig/storage Categorizes an issue or PR as relevant to SIG Storage. sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Aug 13, 2019

jsafrane force-pushed the skip-unused-volumes branch from a572ff0 to 7a9d142 Compare August 13, 2019 11:04

jsafrane added 3 commits August 15, 2019 10:48

Refactor makeMountsMap into GetPodVolumeNames

8d58026

The function will be handy in subsequent patches. Also change custom maps into sets.String.

Add test for unused volumes

2c79ffe

jsafrane force-pushed the skip-unused-volumes branch from 7a9d142 to 2c79ffe Compare August 15, 2019 09:07

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

msau42 reviewed Aug 15, 2019

View reviewed changes

k8s-ci-robot assigned wongma7 Aug 15, 2019

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

k8s-ci-robot assigned derekwaynecarr and tallclair Aug 16, 2019

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

k8s-ci-robot merged commit e319abf into kubernetes:master Aug 17, 2019

k8s-ci-robot added this to the v1.16 milestone Aug 17, 2019

haircommander mentioned this pull request Aug 20, 2019

WIP: test reverting f22b67dd8f9f4c1ce7 cri-o/cri-o#2740

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Skip unused volumes in VolumeManager #81163

Skip unused volumes in VolumeManager #81163

jsafrane commented Aug 8, 2019 •

edited

Loading

wongma7 commented Aug 8, 2019

mattjmcnaughton commented Aug 8, 2019

wongma7 commented Aug 8, 2019

msau42 commented Aug 9, 2019

jsafrane commented Aug 9, 2019 •

edited

Loading

jsafrane commented Aug 12, 2019

wongma7 commented Aug 12, 2019 •

edited

Loading

wongma7 Aug 12, 2019

jsafrane Aug 13, 2019 •

edited

Loading

jsafrane Aug 13, 2019

wongma7 Aug 12, 2019

jsafrane Aug 13, 2019

jsafrane commented Aug 13, 2019

jsafrane commented Aug 15, 2019

jsafrane commented Aug 15, 2019

jsafrane commented Aug 15, 2019

msau42 Aug 15, 2019

wongma7 commented Aug 15, 2019

jsafrane commented Aug 16, 2019

derekwaynecarr commented Aug 16, 2019

k8s-ci-robot commented Aug 16, 2019

fejta-bot commented Aug 16, 2019

Skip unused volumes in VolumeManager #81163

Skip unused volumes in VolumeManager #81163

Conversation

jsafrane commented Aug 8, 2019 • edited Loading

wongma7 commented Aug 8, 2019

mattjmcnaughton commented Aug 8, 2019

wongma7 commented Aug 8, 2019

msau42 commented Aug 9, 2019

jsafrane commented Aug 9, 2019 • edited Loading

jsafrane commented Aug 12, 2019

wongma7 commented Aug 12, 2019 • edited Loading

wongma7 Aug 12, 2019

Choose a reason for hiding this comment

jsafrane Aug 13, 2019 • edited Loading

Choose a reason for hiding this comment

jsafrane Aug 13, 2019

Choose a reason for hiding this comment

wongma7 Aug 12, 2019

Choose a reason for hiding this comment

jsafrane Aug 13, 2019

Choose a reason for hiding this comment

jsafrane commented Aug 13, 2019

jsafrane commented Aug 15, 2019

jsafrane commented Aug 15, 2019

jsafrane commented Aug 15, 2019

msau42 Aug 15, 2019

Choose a reason for hiding this comment

wongma7 commented Aug 15, 2019

jsafrane commented Aug 16, 2019

derekwaynecarr commented Aug 16, 2019

k8s-ci-robot commented Aug 16, 2019

fejta-bot commented Aug 16, 2019

jsafrane commented Aug 8, 2019 •

edited

Loading

jsafrane commented Aug 9, 2019 •

edited

Loading

wongma7 commented Aug 12, 2019 •

edited

Loading

jsafrane Aug 13, 2019 •

edited

Loading