Fix volume reconstruction and add e2e tests #75071

mkimuram · 2019-03-06T23:41:30Z

What type of PR is this?
/kind bug

What this PR does / why we need it:
This PR fixes a bug that volume reconstruction does not work properly for csi, by failure in handling filesystem volume and block volume with a proper logic. To get the path to check for existence of volume, Mounter 's GetPath() should be used for file system and BlockVolumeMapper's GetPodDeviceMapPath() should be used for block volume. In addition, Mounter should be created only for file system volume and BlockVolumeMapper should be created only for block volume, because csi plugin creates a new directory + json on Mounter and BlockVolumeMapper creation, leading to even more orphaned files/directories.
(This PR was originally intended to add e2e tests for block volume as discussed in #74545 , and found the bug and fix it.)

Which issue(s) this PR fixes:

Special notes for your reviewer:
/sig storage
cc @bswartz @wongma7 @msau42

Does this PR introduce a user-facing change?:

NONE

k8s-ci-robot · 2019-03-06T23:41:37Z

Hi @mkimuram. 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.

Details

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.

mkimuram · 2019-03-06T23:44:36Z

@msau42 @wongma7

Some of the fixes and additions to the existing test framework are borrowed from #74693. Also, changes that are discussed in #65248 are also included.

msau42 · 2019-03-06T23:46:19Z

/ok-to-test

msau42 · 2019-03-06T23:51:33Z

BTW, CSI Reconstruction is flaky in general: #72500

mkimuram · 2019-03-07T16:19:03Z

/retest

mkimuram · 2019-03-07T16:31:23Z

@msau42

Is it possible to manually arrange tests in this PR, because they aren't tested by regular CI, due to the fact that these tests have [Disruptive] tag?

Note that below three tests are added for both filesystem and block volume in this PR.

"Should test that pv written before kubelet restart is readable after restart."
"Should test that pv used in a pod that is deleted while the kubelet is down cleans up when the kubelet returns."
"Should test that pv used in a pod that is force deleted while the kubelet is down cleans up when the kubelet returns."

For filesystem, all tests passed in my environment. (As they almost just call existing tests from framework.)
However, for block, only first test passes, and rest of the tests fail. (Maybe due to the issue discussed #74914)

Can this PR go ahead by disabling these failing tests for another PR that fix #74914 to utilize this test?
Or should we need to include fix to this PR?

msau42 · 2019-03-07T16:44:14Z

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

Is an optional job that runs serial CSI tests

mkimuram · 2019-03-07T18:06:31Z

@msau42

BTW, CSI Reconstruction is flaky in general: #72500

I found that the path is eventually deleted if manually checked after the test failure. Therefore, we might at least reduce the flake by adding rather longer wait before here.

(In my environment, test with iscsi block volume fails more often in non-force delete case than in force delete case, so we might just need ~30sec wait in general and even more for CSI as a workaround?)

msau42 · 2019-03-07T18:16:22Z

In the graceful delete case, kubelet should wait until volume manager finishes unmounting the volume before deleting the Pod object. So we shouldn't need to add a timeout for the graceful deletion case.

mkimuram · 2019-03-07T20:23:06Z

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

mkimuram · 2019-03-08T02:52:19Z

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

msau42 · 2019-05-06T17:17:34Z

/assign @bswartz

msau42 · 2019-07-22T20:29:01Z

/approve

k8s-ci-robot · 2019-07-22T20:29:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jsafrane, mkimuram, msau42

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~pkg/kubelet/volumemanager/OWNERS~~ [jsafrane,msau42]
~~pkg/volume/util/operationexecutor/OWNERS~~ [jsafrane,msau42]
~~test/OWNERS~~ [msau42]

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

jingxu97 · 2019-07-23T18:57:14Z

pkg/kubelet/volumemanager/reconciler/reconciler.go

-	var volumeMapper volumepkg.BlockVolumeMapper
 	if utilfeature.DefaultFeatureGate.Enabled(features.BlockVolume) && volume.volumeMode == v1.PersistentVolumeBlock {
 		var newMapperErr error
 		if mapperPlugin != nil {


It is not your change, but I don't see the possibility of mapperPlugin is nil?

@jingxu97

Thank you for your comment.

You're right that this path won't be reached, beause ReconstructVolumeOperation returns error when mapperPlugin == nil just after FindMapperPluginByName returns nil for mapperPlugin when plugin doesn't support block volume.

However, it doesn't seem to be so obvious, because it depends on other function's implementation.
In this case, do you mean to delete this check here or any other ideas for improve this?

It just seems strange to check this quite late here.

I guess @jingxu97 points checkPath is always set if we can remove if mapperPlugin != nil { condition and that would make CheckVolumeExistenceOperation() call safe.

@jingxu97 @oomichi

I've moved nil check for mapperPlugin earlier in reconstructVolume in "Move nil check for mapperPlugin earlier in reconstructVolume" commit. PTAL

(I will merge this change with the previous commit if this looks good.)

jsafrane · 2019-07-30T10:33:04Z

pkg/kubelet/volumemanager/reconciler/reconciler.go

+	}
+
+	// Check existence of mount point for filesystem volume or symbolic link for block volume
+	isExist, checkErr := rc.operationExecutor.CheckVolumeExistenceOperation(volumeSpec, volume.volumePath, volumeSpec.Name(), rc.mounter, uniqueVolumeName, volume.podName, pod.UID, attachablePlugin)


volume.volumePath is used incorrectly here. Before this PR, volumeMounter.GetPath() was used. This breaks CSI reconstruction, as CheckVolumeExistenceOperation checks the pod directory [1] and not the directory where the volume should be mounted [2]

1: /var/lib/kubelet/pods/4ec8e3aa-f83e-4d49-96ea-3859f09f7dd7/volumes/kubernetes.io~csi/pvc-873e54a8-a2c1-4872-8a3c-b68b71a81e42
2: /var/lib/kubelet/pods/4ec8e3aa-f83e-4d49-96ea-3859f09f7dd7/volumes/kubernetes.io~csi/pvc-873e54a8-a2c1-4872-8a3c-b68b71a81e42/mount

jsafrane · 2019-07-30T10:35:35Z

@mkimuram, I fixed CSI volume reconstruction in the last commit of this PR: #80743 (running tests there).

Can you please include it in this PR? I am sorry I broke it in the first place.

mkimuram · 2019-07-30T15:20:14Z

@jsafrane

Oh, I should have read through the original code, too. Sorry for that.
Previous code seems to revert #74653 work (@jingxu97 was right) and now it is taken into account.
Also, checking GetPodDeviceMapPath for block volume should be the right place to check.

I've re-organized the commits to move the fix into the right commits.
(Deleted the test disable commit and merged the fix commit into one.)

PTAL @jsafrane @msau42 @jingxu97

jingxu97 · 2019-07-26T20:31:14Z

pkg/kubelet/volumemanager/reconciler/reconciler.go

Maybe in MountedVolume definition, we can add the comment to make it clear that mounter is required for file system volumes, but not required for BlockVolumes, and BlockVolumeMapper is the opposite.

jingxu97 · 2019-07-26T20:34:41Z

pkg/kubelet/volumemanager/reconciler/reconciler.go

-		return nil, fmt.Errorf("Volume: %q is not mounted", uniqueVolumeName)
-	}
+	var volumeMapper volumepkg.BlockVolumeMapper
+	var volumeMounter volumepkg.Mounter


Can you indicate this change into the PR's title and comment since this is a code change, not just test change?

@jingxu97

Thank you for your review.

I've changed the PR's title and comments and added comments in MountedVolume definition.
(New code change is merged to "Don't create mounter when reconstructing block volume" commit.)

PTAL

oomichi · 2019-07-31T02:44:36Z

pkg/kubelet/volumemanager/reconciler/reconciler.go

-	var volumeMapper volumepkg.BlockVolumeMapper
 	if utilfeature.DefaultFeatureGate.Enabled(features.BlockVolume) && volume.volumeMode == v1.PersistentVolumeBlock {
 		var newMapperErr error
 		if mapperPlugin != nil {


I guess @jingxu97 points checkPath is always set if we can remove if mapperPlugin != nil { condition and that would make CheckVolumeExistenceOperation() call safe.

oomichi · 2019-07-31T03:05:17Z

test/e2e/storage/generic_persistent_volume-disruptive.go

Why do we need to change here?
I cannot find the reason from the PR message.

framework.CreateNginxPod can't handle block volume and inline volume, but framework.CreateSecPod can.
Current tests in generic_persistent_volume-disruptive.go doesn't actually test them, but it would be good to use the similar logic to testsuites/disruptive.go to prepare for future imporvement and maintenance.

oomichi · 2019-07-31T03:22:47Z

test/e2e/storage/utils/utils.go

We can replace here with framework.ExpectEqual(result.Code, 0, fmt.Sprintf("Expected grep exit code of 0, got %d", result.Code))

Fixed as suggested.

oomichi · 2019-07-31T03:23:53Z

test/e2e/storage/utils/utils.go

We can remove if err != nil { because of framework.ExpectNoError(err, "Expected pod to be not found.")

Fixed as suggested.

CSI mounter will create a new directory + json for a filesystem volume, leading to even more orphaned files/directories.

mkimuram

@oomichi

Thank you for your comment. I've fixed as suggested. PTAL

mkimuram · 2019-08-01T21:09:24Z

test/e2e/storage/generic_persistent_volume-disruptive.go

framework.CreateNginxPod can't handle block volume and inline volume, but framework.CreateSecPod can.
Current tests in generic_persistent_volume-disruptive.go doesn't actually test them, but it would be good to use the similar logic to testsuites/disruptive.go to prepare for future imporvement and maintenance.

mkimuram · 2019-08-01T21:10:30Z

test/e2e/storage/utils/utils.go

Fixed as suggested.

mkimuram · 2019-08-01T21:10:42Z

test/e2e/storage/utils/utils.go

Fixed as suggested.

jsafrane · 2019-08-02T08:16:13Z

/retest

jingxu97 · 2019-08-05T20:08:18Z

thanks for the work! /lgtm

jsafrane · 2019-08-06T11:40:08Z

Just reformating Jing's lgtm
/lgtm

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Mar 6, 2019

k8s-ci-robot requested review from copejon and janetkuo March 6, 2019 23:42

k8s-ci-robot added the sig/testing Categorizes an issue or PR as relevant to SIG Testing. label Mar 6, 2019

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 6, 2019

mkimuram force-pushed the issue/74552e2e branch from 76be029 to 616dcda Compare March 6, 2019 23:52

mkimuram force-pushed the issue/74552e2e branch from 616dcda to 4c82c25 Compare March 8, 2019 00:12

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

k8s-ci-robot assigned bswartz May 6, 2019

mkimuram force-pushed the issue/74552e2e branch from 4c82c25 to fd511ed Compare May 17, 2019 20:41

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

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

jingxu97 reviewed Jul 23, 2019

View reviewed changes

jsafrane mentioned this pull request Jul 30, 2019

WIP: Fix reconstruction #80743

Closed

jsafrane reviewed Jul 30, 2019

View reviewed changes

mkimuram force-pushed the issue/74552e2e branch from c5b4ccf to e84d3b0 Compare July 30, 2019 15:17

jingxu97 reviewed Jul 30, 2019

View reviewed changes

mkimuram changed the title ~~Move disruptive tests to testsuites and add ones for block volume~~ Fix volume reconstruction and add e2e tests Jul 30, 2019

k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Jul 30, 2019

mkimuram force-pushed the issue/74552e2e branch from e84d3b0 to 4cd0029 Compare July 30, 2019 19:18

oomichi reviewed Jul 31, 2019

View reviewed changes

jsafrane mentioned this pull request Jul 31, 2019

Use internal node address for SSH in volume tests #80806

Closed

mkimuram and others added 5 commits August 1, 2019 20:04

Move disruptive tests to testsuites and add ones for block volume

f1d2d9d

Don't create mounter when reconstructing block volume

bab81b8

CSI mounter will create a new directory + json for a filesystem volume, leading to even more orphaned files/directories.

Add in-line filesystem volumes to MakeSecPod

d79b7c7

Refactor e2e tests

2306d2f

Move nil check for mapperPlugin earlier in reconstructVolume

c130b77

mkimuram force-pushed the issue/74552e2e branch from 4cd0029 to c130b77 Compare August 1, 2019 21:10

mkimuram commented Aug 1, 2019

View reviewed changes

k8s-ci-robot assigned jsafrane Aug 6, 2019

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

k8s-ci-robot merged commit 345e58b into kubernetes:master Aug 6, 2019

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

bertinatto mentioned this pull request Aug 13, 2019

[Failing Test] ci-kubernetes-e2e-gci-gce-serial is timing out #81197

Closed

Fix volume reconstruction and add e2e tests #75071

Fix volume reconstruction and add e2e tests #75071

Uh oh!

Conversation

mkimuram commented Mar 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Mar 6, 2019

Uh oh!

mkimuram commented Mar 6, 2019

Uh oh!

msau42 commented Mar 6, 2019

Uh oh!

msau42 commented Mar 6, 2019

Uh oh!

mkimuram commented Mar 7, 2019

Uh oh!

mkimuram commented Mar 7, 2019

Uh oh!

msau42 commented Mar 7, 2019

Uh oh!

mkimuram commented Mar 7, 2019

Uh oh!

msau42 commented Mar 7, 2019

Uh oh!

mkimuram commented Mar 7, 2019

Uh oh!

mkimuram commented Mar 8, 2019

Uh oh!

msau42 commented May 6, 2019

Uh oh!

msau42 commented Jul 22, 2019

Uh oh!

k8s-ci-robot commented Jul 22, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsafrane commented Jul 30, 2019

Uh oh!

mkimuram commented Jul 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkimuram left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

mkimuram commented Mar 6, 2019 •

edited

Loading

mkimuram commented Jul 30, 2019 •

edited

Loading