Fix check for subpath source #105512

gnufied · 2021-10-06T15:20:06Z

I have verified that this fixes CSI/NFS and other volume types and at the same time it keeps older behaviour for configmaps/secrets etc. This code relies on the fact that most volume types create a mount point in /var/lib/kubelet/pods/xxxx/volumes/ when being mounted. The check here won't work for volume types that don't create mount point for main volume (but it is not a regression, at least). For configmaps etc - no bind mount is created unless user specifies subpaths.

For NFS volume with fix:

9660:I1006 18:38:53.488871  766496 subpath_linux.go:122] Skipping bind-mounting subpath /var/lib/kubelet/pods/f7b2da79-86eb-4a66-a9f8-5f3853c708be/volume-subpaths/manual-pv/sandbox/0, volumePath: /var/lib/kubelet/pods/f7b2da79-86eb-4a66-a9f8-5f3853c708be/volumes/kubernetes.io~nfs/manual-pv, path: /var/lib/kubelet/pods/f7b2da79-86eb-4a66-a9f8-5f3853c708be/volumes/kubernetes.io~nfs/manual-pv/foobar

Do not unmount and mount subpath bind mounts during container creation unless bind mount changes

gnufied · 2021-10-06T15:21:19Z

/sig storage

gnufied · 2021-10-06T15:22:12Z

/kind bug

gnufied · 2021-10-06T15:39:02Z

/assign @msau42 @jingxu97

pkg/volume/util/hostutil/hostutil_linux.go

bertinatto · 2021-10-06T19:40:38Z

pkg/volume/util/subpath/subpath_linux.go

+				(volumeMountInfo.Major == mntInfo.Major && volumeMountInfo.Minor == mntInfo.Minor) {
+				klog.V(5).Infof("Skipping bind-mounting subpath %s, volumePath: %s, path: %s", bindPathTarget, subpath.VolumePath, subpath.Path)
+				return true, bindPathTarget, nil
+			}


Here the error is ignored because of ConfigMaps?

I think this could use a comment to explain the reason to exempt these subpath bind mounts from the Unmount call below. IIUC, the configmap and secret subpaths should still trigger unmount, but bind mounts should not?
Does mount.PathWithinBase(subpath.Path, subpath.VolumePath) only return true for bind mounts then?

Would it be feasible to add a unit test for this path?

I have changed this logic to hardcode just the volume types that will trigger unmount and mount. I think apart from that every other volume type should use previously mounted subpath bind mount.

dobsonj · 2021-10-06T21:07:03Z

/retest

jingxu97 · 2021-10-07T21:39:04Z

pkg/volume/util/hostutil/hostutil_linux.go

@@ -164,6 +164,28 @@ func (hu *HostUtil) FindMountInfo(path string) (mount.MountInfo, error) {
 	return findMountInfo(path, procMountInfoPath)
 }

+// FindExactMountInfo returns exact mount point that matches given path


maybe add more details about the difference between FindMountInfo and FindExactMountInfo?

I removed this function. I think the only volume types that really need to trigger mount and unmount is intree ephemeral types.

jingxu97 · 2021-10-07T21:41:02Z

/assigne @fatedier

k8s-ci-robot · 2021-11-09T21:06:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gnufied

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/subpath/OWNERS~~ [gnufied]

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

gnufied · 2021-11-09T21:10:50Z

@msau42 @jsafrane I have updated the code to use os.SameFile to compare bindmount target (i.e subpath bind mount kubelet creates) with subpath directory/file inside the volume. Since a bind mount has same inode number, device numbers as original file - this check should work. If subpath inside volume however changes (but bind mount does not), then they will mismatch and whenever container is restarted, it will cause old subpath bind mount to unmount and mounted again. PTAL.

jsafrane · 2021-11-10T16:45:18Z

pkg/volume/util/subpath/subpath_linux_test.go

+		},
+	}
+	for _, test := range tests {
+		klog.V(4).Infof("test %q", test.name)


Just a suggestion for temp dirs + defer for os.RemoveAll

t.Run(test.name, func (t *testing.T){ base, err = ioutil.TempDir() if err {...} defer os.RemoveAll(base)

jsafrane · 2021-11-10T17:54:22Z

lgtm. Giving @msau42 a chance to review.

gnufied · 2021-11-10T23:07:48Z

/hold cancel

msau42 · 2021-11-11T01:08:09Z

/lgtm
/hold
can you update the release note?

gnufied · 2021-11-11T11:35:35Z

done.
/hold cancel

gnufied · 2021-11-11T12:13:19Z

Unrelated failure:

goroutine 676 [running]:
k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/util/runtime.logPanic({0x50c15a0, 0x89ad100})
	/home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/util/runtime/runtime.go:74 +0xe6
k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/util/runtime.HandleCrash({0x0, 0x0, 0x461107})
	/home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/util/runtime/runtime.go:48 +0xb0
panic({0x50c15a0, 0x89ad100})
	/usr/local/go/src/runtime/panic.go:1047 +0x266
k8s.io/kubernetes/pkg/kubelet.TestSyncLabels.func1.2()

/retest

…512-upstream-release-1.22 Automated cherry pick of #105512: Add check for subpaths

…512-upstream-release-1.21 Automated cherry pick of #105512: Add check for subpaths

…512-upstream-release-1.20 Automated cherry pick of #105512: Add check for subpaths

k8s-ci-robot requested review from andyzhangx and jingxu97 October 6, 2021 15:20

k8s-ci-robot added 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 Oct 6, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 6, 2021

k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. and removed do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Oct 6, 2021

k8s-ci-robot assigned jingxu97 and msau42 Oct 6, 2021

gnufied force-pushed the add-subpath-mount-source-check branch from 1b13067 to c2cf45e Compare October 6, 2021 18:59

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 6, 2021

bertinatto reviewed Oct 6, 2021

View reviewed changes

jingxu97 reviewed Oct 7, 2021

View reviewed changes

Check subpath file

7a73168

gnufied force-pushed the add-subpath-mount-source-check branch from ad66bc7 to f68c948 Compare November 9, 2021 21:05

gnufied force-pushed the add-subpath-mount-source-check branch from f68c948 to e3d5b31 Compare November 10, 2021 16:12

jsafrane reviewed Nov 10, 2021

View reviewed changes

Add tests for checking bind mounts

467bcd8

gnufied force-pushed the add-subpath-mount-source-check branch from e3d5b31 to 467bcd8 Compare November 10, 2021 16:53

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

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Nov 11, 2021

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

gnufied mentioned this pull request Nov 11, 2021

subpath configmap always remount, when container restarts #103836

Closed

k8s-ci-robot merged commit ba94497 into kubernetes:master Nov 11, 2021

k8s-ci-robot added this to the v1.23 milestone Nov 11, 2021

This was referenced Nov 11, 2021

Automated cherry pick of #105512: Add check for subpaths #106350

Merged

Automated cherry pick of #105512: Add check for subpaths #106351

Merged

Automated cherry pick of #105512: Add check for subpaths #106352

Merged

k8s-ci-robot added a commit that referenced this pull request Nov 12, 2021

Merge pull request #106350 from gnufied/automated-cherry-pick-of-#105…

f939cb1

…512-upstream-release-1.22 Automated cherry pick of #105512: Add check for subpaths

k8s-ci-robot added a commit that referenced this pull request Nov 12, 2021

Merge pull request #106351 from gnufied/automated-cherry-pick-of-#105…

81eb60a

…512-upstream-release-1.21 Automated cherry pick of #105512: Add check for subpaths

k8s-ci-robot added a commit that referenced this pull request Nov 12, 2021

Merge pull request #106352 from gnufied/automated-cherry-pick-of-#105…

15467f3

…512-upstream-release-1.20 Automated cherry pick of #105512: Add check for subpaths

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix check for subpath source #105512

Fix check for subpath source #105512

gnufied commented Oct 6, 2021 •

edited

gnufied commented Oct 6, 2021

gnufied commented Oct 6, 2021

gnufied commented Oct 6, 2021

bertinatto Oct 6, 2021

dobsonj Oct 6, 2021

dobsonj Oct 6, 2021

gnufied Oct 8, 2021

dobsonj commented Oct 6, 2021

jingxu97 Oct 7, 2021

gnufied Oct 8, 2021

jingxu97 commented Oct 7, 2021

k8s-ci-robot commented Nov 9, 2021

gnufied commented Nov 9, 2021

jsafrane Nov 10, 2021 •

edited

gnufied Nov 10, 2021

jsafrane commented Nov 10, 2021

gnufied commented Nov 10, 2021

msau42 commented Nov 11, 2021

gnufied commented Nov 11, 2021

gnufied commented Nov 11, 2021

Fix check for subpath source #105512

Fix check for subpath source #105512

Conversation

gnufied commented Oct 6, 2021 • edited

gnufied commented Oct 6, 2021

gnufied commented Oct 6, 2021

gnufied commented Oct 6, 2021

bertinatto Oct 6, 2021

Choose a reason for hiding this comment

dobsonj Oct 6, 2021

Choose a reason for hiding this comment

dobsonj Oct 6, 2021

Choose a reason for hiding this comment

gnufied Oct 8, 2021

Choose a reason for hiding this comment

dobsonj commented Oct 6, 2021

jingxu97 Oct 7, 2021

Choose a reason for hiding this comment

gnufied Oct 8, 2021

Choose a reason for hiding this comment

jingxu97 commented Oct 7, 2021

k8s-ci-robot commented Nov 9, 2021

gnufied commented Nov 9, 2021

jsafrane Nov 10, 2021 • edited

Choose a reason for hiding this comment

gnufied Nov 10, 2021

Choose a reason for hiding this comment

jsafrane commented Nov 10, 2021

gnufied commented Nov 10, 2021

msau42 commented Nov 11, 2021

gnufied commented Nov 11, 2021

gnufied commented Nov 11, 2021

gnufied commented Oct 6, 2021 •

edited

jsafrane Nov 10, 2021 •

edited