Move MountsInGlobalPDPath from mount pkg to volume #74734

codenrhoden · 2019-02-28T17:34:46Z

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
Since pkg/util/mount is going to move out of k/k, this exported constant
that is Kubernetes specific needs to move somewhere else. Made sense to
move it to pkg/volume/util.

For the volume plugins that call mounter.GetDeviceNameFromMount(),
rather than changing the signature of mounter.GetDeviceNameFromMount()
just have the volume plugins append MountsInGlobalPDPath to the volume
pluginDir themselves rather than having mounter.GetDeviceNameFromMount()
do it since that code no longer has access to that constant.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:
This is another step in making #68513 be a lot simpler.

This PR does not attempt to address #74548 at all, just maintain current functionality.

Does this PR introduce a user-facing change?:

NONE

codenrhoden · 2019-02-28T17:35:46Z

/sig storage
/assign @msau42 @saad-ali

msau42 · 2019-02-28T21:27:31Z

pkg/util/mount/mount_windows.go

 	for _, ref := range refs {
-		if strings.Contains(ref, basemountPath) {
-			volumeID, err := filepath.Rel(normalizeWindowsPath(basemountPath), ref)
+		if strings.Contains(ref, pluginDir) {


does plugindir need to be normalized first?

Yes, it does. Good catch.

msau42 · 2019-02-28T21:28:40Z

pkg/volume/awsebs/aws_ebs.go

@@ -250,7 +250,7 @@ func getVolumeSource(

 func (plugin *awsElasticBlockStorePlugin) ConstructVolumeSpec(volName, mountPath string) (*volume.Spec, error) {
 	mounter := plugin.host.GetMounter(plugin.GetPluginName())
-	pluginDir := plugin.host.GetPluginDir(plugin.GetPluginName())
+	pluginDir := filepath.Join(plugin.host.GetPluginDir(plugin.GetPluginName()), util.MountsInGlobalPDPath)


does it seem like enough plugins use this to warrant some new "GetPluginMountDir" interface?

I'm not sure it would need to be an interface, rather just an exported function perhaps in pkg/volume/util?

func GetPluginMountDir(host volume.VolumeHost, name string) string { pluginDir := filepath.Join(host.GetPluginDir(name), MountsInGlobalPDPath) return pluginDir }

Something like that?

This sounds reasonable to me

pkg/volume/azure_dd/azure_common.go

bertinatto · 2019-03-05T11:24:27Z

pkg/util/mount/mount_linux.go

-		if strings.HasPrefix(ref, basemountPath) {
-			volumeID, err := filepath.Rel(basemountPath, ref)
+		if strings.HasPrefix(ref, pluginDir) {
+			volumeID, err := filepath.Rel(pluginDir, ref)


Seems like pluginDir, that comes from the plugin's ConstructVolumeSpec, is no longer only the plugin directory. Now it has the MountsInGlobalPDPath const appended to it, right?

We might have to change the mount.Interface as well.

That was what I was going for here, yes. I was trying to make as few changes as possible, without having to change mount.Interface.

An alternative would be to go ahead and change the interface from:

GetDeviceNameFromMount(mountPath, pluginDir string)

to

GetDeviceNameFromMount(mountPath, pluginDir, mountDir string)

And then append the mount dir in the function, which is what it did before.

The overall goal here is not have anything in pkg/util/mount depend on access to that constant, which needs to live in another package (I had put it in pkg/volume/util).

I'm fine with either method.

I prefer changing it to the first

Sorry I wasn't very clear. I preferred only having two arguments to GetDeviceNameFromMount, and having some helper method that appends the constant to the plugin dir.

doh. okay, I can do that. 😆

codenrhoden · 2019-03-06T19:32:10Z

@msau42 @bertinatto

Thanks for the reviews, and apologies for the slow response. Both of my kids have been sick this week, so I've been unable to come back to this.

If I have any chance of getting this in before code freeze, I'd like to try for that.

I think the main question is whether or not to keep mount.Interface the way it is now, which requires callers of mounter.GetDeviceNameFromMount() to append the mountDir constant themselves before the call (which could be done with a common function), or to change the interface to take the mountDir as a third parameter.

I don't have a preference one way or the other. I had leaned the way I did because I didn't want to change the interface if I could avoid it in this PR.

pkg/volume/photon_pd/photon_pd.go

pkg/volume/storageos/storageos.go

pkg/volume/rbd/rbd_util.go

codenrhoden · 2019-03-29T23:28:03Z

@msau42 Okay, I went ahead and did this again after changing the interface. That commit is probably a bit easier to review now.

I ran with @andyzhangx's guidance to always use filepath.Join, and went ahead and cleaned up all usage in pkg/util/mount and pkg/util/volume in a second/separate commit.

codenrhoden · 2019-03-30T15:56:54Z

/test pull-kubernetes-e2e-gce

codenrhoden · 2019-04-03T17:35:15Z

/test pull-kubernetes-bazel-test

msau42 · 2019-04-03T21:54:32Z

/lgtm

@andyzhangx @jingxu97 would you be able to help test this change on windows? Especially the configmap/secret/emptydir/projected/atomic writer changes?

msau42 · 2019-04-25T16:19:05Z

cc @yujuhong

Run: ('kubetest', '--dump=/logs/artifacts', '--gcp-service-account=/etc/service-account/service-account.json', '--build=gce-windows-bazel', '--up', '--down', '--provider=gce', '--cluster=e2e-cfe9c0aea2-95646', '--gcp-network=e2e-cfe9c0aea2-95646', '--check-leaked-resources', '--extract=local', '--gcp-zone=us-west1-b', '--ginkgo-parallel=8', '--gcp-nodes=2', '--test-cmd=$GOPATH/src/k8s.io/gce-k8s-windows-testing/run-e2e.sh', '--test-cmd-args=--ginkgo.focus=\\[Conformance\\]|\\[NodeConformance\\]|\\[sig-windows\\] --ginkgo.skip=\\[LinuxOnly\\]|\\[Serial\\]|\\[Feature:.+\\] --minStartupPods=8', '--test-cmd-args=--node-os-distro=windows', '--timeout=120m')
2019/04/25 16:15:55 main.go:275: Flag parse failed: invalid argument "gce-windows-bazel" for "--build" flag: bad build strategy: gce-windows-bazel (use: bazel, e2e, host-go, quick, release)

codenrhoden · 2019-04-25T16:20:39Z

Thanks @msau42, you beat me to it. Whatever the test is, the test is broken. =)

yujuhong · 2019-04-25T20:19:38Z

Thanks @msau42, you beat me to it. Whatever the test is, the test is broken. =)

The test requires a new kubetest image to pick up the change adding the flag. I filed an issue in test-infra to build and push a new image: kubernetes/test-infra#12340

jsafrane · 2019-04-29T09:07:39Z

/approve

k8s-ci-robot · 2019-04-29T09:08:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: codenrhoden, 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/util/mount/OWNERS~~ [jsafrane]
~~pkg/volume/OWNERS~~ [jsafrane]

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

Since pkg/util/mount is going to move out of k/k, this exported constant that is Kubernetes specific needed to move somewhere else. Made sense to move it to pkg/volume/util. Update GetDeviceNameFromMount in the mount interface to now take a pluginMountDir argument, which is volume plugin dir with the global mount path appended to it already.

This patch cleans up pkg/util/mount/* and pkg/util/volume/* to always use filepath.Join instead of path.Join. filepath.Join is preferred because path.Join can have issues on Windows.

codenrhoden · 2019-04-29T17:05:11Z

/test pull-kubernetes-bazel-test

yujuhong · 2019-04-29T22:09:53Z

/test pull-kubernetes-e2e-windows-gce

codenrhoden · 2019-04-29T23:15:21Z

/test pull-kubernetes-e2e-windows-gce

yujuhong · 2019-04-30T00:26:13Z

/test pull-kubernetes-e2e-windows-gce

We are trying to get the test job working, so I'm hijacking this PR to run some tests

yujuhong · 2019-04-30T00:53:18Z

/test pull-kubernetes-e2e-windows-gce

codenrhoden · 2019-04-30T01:22:32Z

@yujuhong Hijack away, and good luck. =)

codenrhoden · 2019-04-30T01:23:48Z

I doubt it is helpful, but the main thing that jumps out to me from the logs to me is the linux reference, here:

--config=cross:linux_amd64

For a windows test, that raises red flags for me.

yujuhong · 2019-04-30T20:36:09Z

/test pull-kubernetes-e2e-windows-gce

yujuhong · 2019-04-30T20:36:39Z

For a windows test, that raises red flags for me.

We need to build both windows and linux artifacts since the master control plane runs only on linux :-)

k8s-ci-robot · 2019-04-30T20:56:13Z

@codenrhoden: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-windows-gce	`78d109e`	link	`/test pull-kubernetes-e2e-windows-gce`

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.

codenrhoden · 2019-05-01T20:28:43Z

Anything I can do to move this along?

msau42 · 2019-05-01T22:52:00Z

windows pull job still needs a bit of work to get passing
/skip

msau42 · 2019-05-01T22:52:33Z

/lgtm

yujuhong · 2019-05-01T23:50:49Z

windows pull job still needs a bit of work to get passing

It's not required, so you can safely ignore it.

Our build/stage process have poor support of handling multiple platforms simultaneously. I made some progress for building them, but more changes are needed for staging...

k8s-ci-robot assigned msau42 and saad-ali Feb 28, 2019

k8s-ci-robot added sig/storage Categorizes an issue or PR as relevant to SIG Storage. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Feb 28, 2019

k8s-ci-robot requested review from divyenpatel and feiskyer February 28, 2019 17:36

msau42 reviewed Feb 28, 2019

View reviewed changes

bertinatto reviewed Mar 5, 2019

View reviewed changes

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

andyzhangx reviewed Mar 27, 2019

View reviewed changes

pkg/volume/photon_pd/photon_pd.go Outdated Show resolved Hide resolved

pkg/volume/storageos/storageos.go Outdated Show resolved Hide resolved

pkg/volume/rbd/rbd_util.go Outdated Show resolved Hide resolved

codenrhoden force-pushed the move-mountspath branch from 78d77ae to 008d789 Compare March 29, 2019 23:25

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 29, 2019

codenrhoden force-pushed the move-mountspath branch from 008d789 to e05b8a7 Compare April 3, 2019 16:01

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

codenrhoden force-pushed the move-mountspath branch from e05b8a7 to 0780ddc Compare April 9, 2019 17:26

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 9, 2019

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

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

codenrhoden added 2 commits April 29, 2019 09:56

Always use filepath.Join instead of path.Join

78d109e

This patch cleans up pkg/util/mount/* and pkg/util/volume/* to always use filepath.Join instead of path.Join. filepath.Join is preferred because path.Join can have issues on Windows.

codenrhoden force-pushed the move-mountspath branch from 4faface to 78d109e Compare April 29, 2019 15:57

k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 29, 2019

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

k8s-ci-robot merged commit b5c34d0 into kubernetes:master May 2, 2019

codenrhoden deleted the move-mountspath branch May 2, 2019 00:29

codenrhoden mentioned this pull request May 10, 2019

Refactor util/mount interface in prep for moving out of k/k #68513

Merged

Move MountsInGlobalPDPath from mount pkg to volume #74734

Move MountsInGlobalPDPath from mount pkg to volume #74734

Conversation

codenrhoden commented Feb 28, 2019

codenrhoden commented Feb 28, 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

codenrhoden commented Mar 6, 2019

codenrhoden commented Mar 29, 2019

codenrhoden commented Mar 30, 2019

codenrhoden commented Apr 3, 2019

msau42 commented Apr 3, 2019

msau42 commented Apr 25, 2019

codenrhoden commented Apr 25, 2019

yujuhong commented Apr 25, 2019

jsafrane commented Apr 29, 2019

k8s-ci-robot commented Apr 29, 2019

codenrhoden commented Apr 29, 2019

yujuhong commented Apr 29, 2019

codenrhoden commented Apr 29, 2019

yujuhong commented Apr 30, 2019

yujuhong commented Apr 30, 2019

codenrhoden commented Apr 30, 2019

codenrhoden commented Apr 30, 2019

yujuhong commented Apr 30, 2019

yujuhong commented Apr 30, 2019

k8s-ci-robot commented Apr 30, 2019

codenrhoden commented May 1, 2019

msau42 commented May 1, 2019

msau42 commented May 1, 2019

yujuhong commented May 1, 2019