StorageOS configurable device directory and mount options #58816

croomes · 2018-01-25T15:55:30Z

What this PR does / why we need it:
This allows StorageOS volumes to be mounted when the kubelet is running in a container and we are unable to use the default device location (/var/lib/storageos/volumes). With this PR, the node's device location is requested via the StorageOS api, falling back to the current behaviour if not configured. The node's device location can be supplied as an environment variable (DEVICE_DIR) to the StorageOS container. This is backwards-compatible and no changes are needed to existing deployments.

The PR also allows Mount options to be set for StorageOS volumes in the same way they're enabled for other volume plugins.

The StorageOS API dependency was updated to the latest version, but no functionality changes besides adding the DeviceDir property to the Controller object.

There is also a small refactor of the loopback device handling code in storageos_utils.go to capture stderr output.

Release note:

StorageOS volume plugin updated to support mount options and environments where the kubelet runs in a container and the device location should be specified.

Not sure why godep changed the comments of unrelated packages in Godeps.json...

/sig storage

jsafrane · 2018-01-29T13:49:03Z

pkg/volume/storageos/storageos_util.go

@@ -323,27 +346,30 @@ func getLoopDevice(path string, exec mount.Exec) (string, error) {
 	}

 	args := []string{"-j", path}
-	out, err := exec.Run(losetupPath, args...)
+	cmd := exec.Command(losetupPath, args...)


Please add a comment that you intentionally run where kubelet is (e.g. in kubelet's container).

@jsafrane thanks for the review, but quick question as I noticed that I reverted the VolumeHost.GetExec() change you made without fully understanding it.

While we expect the mount utilities (incl losetup) to run where the kubelet runs, we don't require it. We just need the device and mount to be available in the correct place and where the kubelet can see them.

Unless you can think of any gotchas or I'm misunderstanding, I'll revert this bit.

Also fyi: the Block Volume support loop device code seems to have used this as a base, and doesn't use VolumeHost.GetExec(). Not sure if this was intentional, and if it was do I need the same here? https://github.com/kubernetes/kubernetes/blob/master/pkg/volume/util/util_linux.go. Maybe @mtanino can advise?

In 1.9 there is a possibility to run mount utilities (mount.nfs, iscsiadm, mkfs.btrfs, ..., losetup) in a dedicated container instead of the host (which can be a minimal one like CoreOS) or in the container where kubelet runs (which can have just kubelet). The setup is quite elaborate and it is dedicated only for testing in our e2e tests, which run quite minimal GCI image (but it has losetup). VolumeHost.GetExec() is used to execute things in these containers with mount utilities instead of on the host. You probably should use this interface for losetup too, just in case someone has really minimal OS without it.

In addition, it would be great if you could prepare container(s) with the server parts of StorageOS and e2e tests that would deploy it and test the plugin against. This way, we could test our plugin and make sure there are no regressions as the plugin evolves.

Thanks, I'll revert this bit. We're working on the e2e tests separately. I'm hoping we can get a PR for them in the next few weeks.

I still see you use exec.Command directly, did you plan to revert it back to mount.Exec interface?

jsafrane · 2018-01-29T13:50:42Z

pkg/volume/storageos/storageos_util.go

-	out, err := exec.Run(losetupPath, args...)
+func makeLoopDevice(path string) (string, error) {
+	args := []string{"-f", "-P", "--show", path}
+	cmd := exec.Command(losetupPath, args...)


Please add a comment that you intentionally run where kubelet is (e.g. in kubelet's container).

jsafrane · 2018-01-29T13:50:48Z

pkg/volume/storageos/storageos_util.go

 	args := []string{"-d", device}
-	out, err := exec.Run(losetupPath, args...)
+	cmd := exec.Command(losetupPath, args...)


Please add a comment that you intentionally run where kubelet is (e.g. in kubelet's container).

jsafrane · 2018-01-29T14:03:43Z

/ok-to-test

jsafrane · 2018-01-31T09:11:45Z

/retest

jsafrane · 2018-01-31T09:24:25Z

flake: #58975

cblecker · 2018-01-31T17:02:51Z

/retest

croomes · 2018-02-06T00:31:53Z

@jsafrane reverted the exec changes, but just noticed the Godeps conflict, will resolve tomorrow.

This change queries the StorageOS API to retrieve the volume device dir when a volume is attached, rather than relying on the default /var/lib/storageos/volumes directory. This allows all or some nodes to have kubelet running in a container with the StorageOS volume path mounted in, which is required for Azure ACS support. Volume mount options are also supported.

croomes · 2018-02-07T21:34:53Z

@jsafrane I think this is good to go now, PTAL - thanks

jsafrane · 2018-02-09T09:27:10Z

/lgtm

jsafrane · 2018-02-09T09:28:02Z

/assign @thockin
for vendor approval

jsafrane · 2018-02-12T15:12:02Z

/approve no-issue
/retest

croomes · 2018-02-12T17:39:45Z

/test pull-kubernetes-e2e-kubeadm-gce-canary

croomes · 2018-02-13T10:22:59Z

/test pull-kubernetes-e2e-kubeadm-gce-canary

croomes · 2018-02-15T15:05:42Z

/test pull-kubernetes-e2e-kubeadm-gce-canary

k8s-ci-robot · 2018-02-15T15:09:01Z

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

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-kubeadm-gce-canary	`1a91403`	link	`/test pull-kubernetes-e2e-kubeadm-gce-canary`

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.

croomes · 2018-02-19T22:06:32Z

/test pull-kubernetes-e2e-kubeadm-gce-canary

thockin · 2018-02-23T00:42:43Z

In the future, please file bugs so we can xref them in PRs.

/lgtm
/approve no-issue

k8s-ci-robot · 2018-02-23T00:43:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: croomes, jsafrane, thockin

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:

~~Godeps/OWNERS~~ [thockin]
~~pkg/volume/OWNERS~~ [jsafrane,thockin]
~~vendor/OWNERS~~ [thockin]

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

k8s-github-robot · 2018-02-23T01:18:46Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2018-02-23T02:11:34Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

croomes · 2018-02-27T12:01:43Z

/status approved-for-milestone

k8s-ci-robot · 2018-02-27T12:01:44Z

You must be a member of the kubernetes/kubernetes-milestone-maintainers github team to add status labels.

croomes · 2018-02-27T12:03:27Z

@saad-ali are you able to help get this into 1.10 release? Was hoping it would have gone in automatically...

jsafrane · 2018-02-27T13:02:21Z

Don't worry, it's merged into master so it's going to be part of 1.10. It splits from master in upcoming week(s).

croomes · 2018-02-27T13:05:55Z

Fantastic, thanks for letting me know!

k8s-ci-robot requested review from jsafrane and smarterclayton January 25, 2018 15:55

jsafrane reviewed Jan 29, 2018

View reviewed changes

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jan 29, 2018

croomes force-pushed the storageos_containerized_kubelet branch from 3500580 to 81456bf Compare February 5, 2018 17:00

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 6, 2018

croomes force-pushed the storageos_containerized_kubelet branch from 81456bf to 035a080 Compare February 6, 2018 13:36

croomes added 2 commits February 6, 2018 13:52

Update storageos api dependency to 0.3.4

8d0ba4a

croomes force-pushed the storageos_containerized_kubelet branch from 035a080 to 1a91403 Compare February 6, 2018 13:52

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 6, 2018

k8s-ci-robot assigned jsafrane Feb 9, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 9, 2018

k8s-ci-robot assigned thockin Feb 9, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 23, 2018

k8s-github-robot merged commit 9a75b4d into kubernetes:master Feb 23, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

StorageOS configurable device directory and mount options #58816

StorageOS configurable device directory and mount options #58816

croomes commented Jan 25, 2018

jsafrane Jan 29, 2018

croomes Jan 29, 2018

jsafrane Jan 30, 2018

croomes Jan 30, 2018

jsafrane Feb 2, 2018

jsafrane Jan 29, 2018

jsafrane Jan 29, 2018

jsafrane commented Jan 29, 2018

jsafrane commented Jan 31, 2018

jsafrane commented Jan 31, 2018

cblecker commented Jan 31, 2018

croomes commented Feb 6, 2018

croomes commented Feb 7, 2018

jsafrane commented Feb 9, 2018

jsafrane commented Feb 9, 2018

jsafrane commented Feb 12, 2018

croomes commented Feb 12, 2018

croomes commented Feb 13, 2018

croomes commented Feb 15, 2018

k8s-ci-robot commented Feb 15, 2018 •

edited

Loading

croomes commented Feb 19, 2018

thockin commented Feb 23, 2018

k8s-ci-robot commented Feb 23, 2018

k8s-github-robot commented Feb 23, 2018

k8s-github-robot commented Feb 23, 2018

croomes commented Feb 27, 2018

k8s-ci-robot commented Feb 27, 2018

croomes commented Feb 27, 2018

jsafrane commented Feb 27, 2018

croomes commented Feb 27, 2018

StorageOS configurable device directory and mount options #58816

StorageOS configurable device directory and mount options #58816

Conversation

croomes commented Jan 25, 2018

jsafrane Jan 29, 2018

Choose a reason for hiding this comment

croomes Jan 29, 2018

Choose a reason for hiding this comment

jsafrane Jan 30, 2018

Choose a reason for hiding this comment

croomes Jan 30, 2018

Choose a reason for hiding this comment

jsafrane Feb 2, 2018

Choose a reason for hiding this comment

jsafrane Jan 29, 2018

Choose a reason for hiding this comment

jsafrane Jan 29, 2018

Choose a reason for hiding this comment

jsafrane commented Jan 29, 2018

jsafrane commented Jan 31, 2018

jsafrane commented Jan 31, 2018

cblecker commented Jan 31, 2018

croomes commented Feb 6, 2018

croomes commented Feb 7, 2018

jsafrane commented Feb 9, 2018

jsafrane commented Feb 9, 2018

jsafrane commented Feb 12, 2018

croomes commented Feb 12, 2018

croomes commented Feb 13, 2018

croomes commented Feb 15, 2018

k8s-ci-robot commented Feb 15, 2018 • edited Loading

croomes commented Feb 19, 2018

thockin commented Feb 23, 2018

k8s-ci-robot commented Feb 23, 2018

k8s-github-robot commented Feb 23, 2018

k8s-github-robot commented Feb 23, 2018

croomes commented Feb 27, 2018

k8s-ci-robot commented Feb 27, 2018

croomes commented Feb 27, 2018

jsafrane commented Feb 27, 2018

croomes commented Feb 27, 2018

k8s-ci-robot commented Feb 15, 2018 •

edited

Loading