Distinguish between volume path and mount path #74653

cofyc · 2019-02-27T10:48:47Z

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:

Which issue(s) this PR fixes:

partially addresses #74650

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Because some plugins mount volume on sub-directory of volume path, we need to distinguish between volume path and mount path to fix issue in reconstruction.

cofyc · 2019-02-27T10:49:10Z

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

cofyc · 2019-02-27T11:28:47Z

pkg/volume/util/operationexecutor/operation_executor.go

@@ -917,7 +917,7 @@ func (oe *operationExecutor) CheckVolumeExistenceOperation(
 		if attachable != nil {
 			var isNotMount bool
 			var mountCheckErr error
-			if isNotMount, mountCheckErr = mounter.IsLikelyNotMountPoint(mountPath); mountCheckErr != nil {
+			if isNotMount, mountCheckErr = mounter.IsNotMountPoint(mountPath); mountCheckErr != nil {


~~I guess for safety, we should use mounter.IsNotMountPoint here because mount source may be a directory on the host (bind mount)~~ (reverted, because it checks for attachable volume only).

A very special case here is mount.IsNotMountPoint/IsLikelyNotMountPoint may fail because mount point is broken (e.g. stale NFS file handle). If this happens, reconstruction will fail and never recover because reconstruction happens only once.

I'm not sure if this is expected. If it is not expected, maybe we need to use another way to check volume existence in reconstruction or don't check it at all?

msau42 · 2019-02-27T19:05:31Z

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

msau42 · 2019-02-28T02:10:54Z

pkg/kubelet/volumemanager/reconciler/reconciler.go

+	if checkErr != nil {
+		return nil, checkErr
+	}
+	// If mount or symlink doesn't exist, volume reconstruction should be failed


@jingxu97 can this check be removed? This means that the directory still exists and should be cleaned up.

Also this assumes that all volumes use mounts, which is not necessarily true (esp for ephemeral types).

For reference, this is the PR that introduced this check. Reading through the original bug and discussion, I'm not sure how this fix actually addresses the original issue... cc @chakri-nelluri

#51985

@msau42 PR #51985 adds an extra check to make sure that the volume is mounted on a reboot and return a failure for volume reconstruction, if it is not. Hope this clarifies the need for check.

Ok @jingxu97 and I discussed and I understand now.

If the volume was partially torn down (ie the mount point is gone, but the directory still exists), then we don't want reconstruction to add the volume back to the ASW (because it's actually not mounted) and cause an empty directory to be mounted into the pod. Let's update the comment here to make that reason more obvious.

However, this brings up some other issues:

What happens if the volume plugin doesn't actually use mounts? Maybe this isn't an issue for attachable plugins. I can't think of a real attachable plugin that is not mount-based.

If there was a case of kubelet/node crash in the middle of cleanup, then we may not be able to properly cleanup the directories for CSI because we only try reconstruction once during kubelet initialization, when the CSI plugin may not be registered yet. This is also more of a corner case and we don't need to consider fixing it urgently for now.

@jingxu97 can this check be removed? This means that the directory still exists and should be cleaned up.

Also this assumes that all volumes use mounts, which is not necessarily true (esp for ephemeral types).

@msau42 This code is conditional and was only meant for plugins which support attach and needs to be reattached and mounted on a node reboot.

What happens if the volume plugin doesn't actually use mounts? Maybe this isn't an issue for attachable plugins. I can't think of a real attachable plugin that is not mount-based.

Block devices?

If there was a case of kubelet/node crash in the middle of cleanup, then we may not be able to properly cleanup the directories for CSI because we only try reconstruction once during kubelet initialization, when the CSI plugin may not be registered yet. This is also more of a corner case and we don't need to consider fixing it urgently for now.

@msau42 This might show up if we restart Kubelet to recover for any other issues..

This CheckVolumeExistance function does handle raw block, although I wonder if it possible to the link to change across reboots? I think this is what the losetup is supposed to protect against. cc @wongma7

only kubelet manipulates these links, technically it can change but so can a mount device (we check that a mount point exists only, not if it points to the right thing). (losetup doesn't help here as the failure scenario the losetup code is intended to prevent is something different like: volume at /dev/sda somehow gets attached then another volume gets attached at /dev/sda, silently. The loop device holding onto /dev/sda is supposed to prevent detach from happening at all. If there's a reboot, I don't believe the loop devices persist anyhow.)

We should test the reboot behavior for raw block separately to make sure we're not going to accidentally give the wrong device to container if the device names change on a reboot.

msau42 · 2019-03-04T22:02:18Z

/lgtm
for fixing the immediate issue. There are a few other followup issues that we can investigate/address separately.

saad-ali · 2019-03-05T18:22:30Z

/assign

saad-ali

/approve

/assign @jingxu97
Jing is our reconstruction expert, let's make sure she takes a look at this.

saad-ali · 2019-03-07T02:30:15Z

pkg/kubelet/volumemanager/reconciler/reconciler.go

@@ -516,6 +507,16 @@ func (rc *reconciler) reconstructVolume(volume podVolume) (*reconstructedVolume,
 			newMounterErr)
 	}

+	// Check existence of mount point for filesystem volume or symbolic link for block volume


Can you update comment.

This comment describes how CheckVolumeExistenceOperation works internally to check volume existence. It's correct.

I think @saad-ali means to add a comment about the change? Instead of using volume.volumePath, we need to use volumeMounter.GetPath() because the mount point setup is different for different plugins. We have to be careful about this when adding/modifying code.

oh, sorry, because it's already merged, we can add it later.

k8s-ci-robot · 2019-03-07T02:31:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cofyc, 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/kubelet/volumemanager/OWNERS~~ [saad-ali]
~~pkg/volume/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

tpepper · 2019-03-18T14:58:29Z

/kind bug

…upstream-release-1.13 Automated cherry pick of #74653: Distinguish volume path with mount path

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

k8s-ci-robot requested review from jingxu97 and jsafrane February 27, 2019 10:49

k8s-ci-robot added area/kubelet sig/node Categorizes an issue or PR as relevant to SIG Node. labels Feb 27, 2019

cofyc mentioned this pull request Feb 27, 2019

Delay CSI client initialization #74652

Merged

cofyc commented Feb 27, 2019

View reviewed changes

Distinguish volume path with mount path

8940976

cofyc force-pushed the fix74650-CheckVolumeExistenceOperation branch from e85a1ca to 8940976 Compare February 27, 2019 11:31

msau42 reviewed Feb 28, 2019

View reviewed changes

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

msau42 mentioned this pull request Mar 4, 2019

Investigate CheckVolumeExistence corner cases #74914

Closed

k8s-ci-robot assigned saad-ali Mar 5, 2019

cofyc changed the title ~~Distinguish volume path with mount path~~ Distinguish between volume path and mount path Mar 6, 2019

saad-ali reviewed Mar 7, 2019

View reviewed changes

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

k8s-ci-robot merged commit 8dd6028 into kubernetes:master Mar 7, 2019

cofyc mentioned this pull request Mar 14, 2019

Automated cherry pick of #74653: Distinguish volume path with mount path #75364

Merged

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

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

Merge pull request #75364 from cofyc/automated-cherry-pick-of-#74653-…

20cdc6c

…upstream-release-1.13 Automated cherry pick of #74653: Distinguish volume path with mount path

cofyc deleted the fix74650-CheckVolumeExistenceOperation branch May 4, 2019 07:21

mkimuram mentioned this pull request Jul 30, 2019

Fix volume reconstruction and add e2e tests #75071

Merged

jingxu97 mentioned this pull request Jan 9, 2020

Invalid path to check in volume existence test for CSI volumes #74650

Closed

j4ckstraw mentioned this pull request Aug 14, 2023

Update kubelet_getters.go #119530

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Distinguish between volume path and mount path #74653

Distinguish between volume path and mount path #74653

cofyc commented Feb 27, 2019 •

edited

cofyc commented Feb 27, 2019

cofyc Feb 27, 2019 •

edited

msau42 commented Feb 27, 2019

msau42 Feb 28, 2019

msau42 Mar 4, 2019

chakri-nelluri Mar 4, 2019

msau42 Mar 4, 2019

chakri-nelluri Mar 4, 2019

chakri-nelluri Mar 4, 2019

chakri-nelluri Mar 4, 2019

msau42 Mar 4, 2019

wongma7 Mar 4, 2019

msau42 Mar 4, 2019

msau42 commented Mar 4, 2019

saad-ali commented Mar 5, 2019

saad-ali left a comment

saad-ali Mar 7, 2019

cofyc Mar 7, 2019

jingxu97 Mar 7, 2019

cofyc Mar 7, 2019

k8s-ci-robot commented Mar 7, 2019

tpepper commented Mar 18, 2019

Distinguish between volume path and mount path #74653

Distinguish between volume path and mount path #74653

Conversation

cofyc commented Feb 27, 2019 • edited

cofyc commented Feb 27, 2019

cofyc Feb 27, 2019 • edited

Choose a reason for hiding this comment

msau42 commented Feb 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msau42 commented Mar 4, 2019

saad-ali commented Mar 5, 2019

saad-ali left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Mar 7, 2019

tpepper commented Mar 18, 2019

cofyc commented Feb 27, 2019 •

edited

cofyc Feb 27, 2019 •

edited