StorageOS attach device before volume attach #69782

darkowlzz · 2018-10-14T13:50:27Z

What this PR does / why we need it:
This fixes an issue in StorageOS volume plugin where volume mount succeeds even if request to mount via StorageOS API fails, by adding an AttachDevice API, which is called before a volume is attached. Also, updates the StorageOS API library.

Which issue(s) this PR fixes:
Fixes #69580

Release note:

StorageOS volume plugin updated to fix an issue where volume mount succeeds even if request to mount via StorageOS API fails.

idealhack · 2018-10-18T15:23:58Z

/kind bug
/ok-to-test

darkowlzz · 2018-10-18T17:35:52Z

Not sure why it failed. Some help would be appreciated.
Running it again.

/retest

croomes · 2018-11-14T13:08:07Z

Hi @rootfs - would you mind having a look when you have time?

Thanks!

saad-ali · 2018-12-06T17:55:30Z

/approve

Please move the volume expand to a separate PR.
Will let @gnufied review and lgtm

/assign @gnufied

idealhack · 2018-12-10T08:33:20Z

/test pull-kubernetes-verify

darkowlzz · 2018-12-10T08:41:19Z

/retest

darkowlzz · 2018-12-10T10:02:58Z

/test pull-kubernetes-e2e-kops-aws

darkowlzz · 2018-12-10T10:46:17Z

/retest

darkowlzz · 2018-12-10T16:31:55Z

I've removed the volume expand related changes from this PR. Will create a separate PR for that.
Thanks!

gnufied

Also the title of the PR still says "Volume expansion" please remove it.

gnufied · 2018-12-10T17:26:58Z

pkg/volume/storageos/storageos.go

+	globalPDPath := makeGlobalPDName(b.plugin.host, b.pvName, b.podNamespace, b.volName)
+
+	// Attach the device to the host.
+	if err := b.manager.AttachDevice(b, globalPDPath); err != nil {


Can the AttachDevice be called from control-plane rather than being called from kubelet?

I don't think there's a need for that. It just creates a virtual volume by calling a StorageOS API.

gnufied · 2018-12-12T16:40:05Z

pkg/volume/storageos/storageos.go

+	// Attach the device to the host.
+	if err := b.manager.AttachDevice(b, globalPDPath); err != nil {
+		klog.Errorf("Failed to attach device at %s: %s", globalPDPath, err.Error())
+		return err


lets not use capitalization for error messages.

Was just being consistent with the existing format in the same file and the other plugins. Should I change all the existing error messages to be non-capitalized?

gnufied · 2018-12-12T16:41:09Z

pkg/volume/storageos/storageos.go

@@ -277,6 +277,8 @@ type storageosManager interface {
 	CreateVolume(provisioner *storageosProvisioner) (*storageosVolume, error)
 	// Attaches the disk to the kubelet's host machine.
 	AttachVolume(mounter *storageosMounter) (string, error)
+	// Attaches the device to the host at a mount path.
+	AttachDevice(mounter *storageosMounter, deviceMountPath string) error


there is also a AttachVolume defined above and it isn't immediately obvious how is AttachDevice different from AttachVolume.

There is another bigger design issue with this plugin which I am not quite sure about is - it implements mounting the device to global path and unmounting from it, without implementing MountDevice and UnmountDevice functions. It is expected that a plugin that mounts the volume to a global path implements these functions (for various reasons).

AttachDevice calls a StorageOS API for the virtual volumes which are created on the host and it doesn't actually attaches a device. The StorageOS virtual volume is then used by AttachVolume to attach the volume to the kubelet.

gnufied · 2019-01-23T23:36:56Z

pkg/volume/storageos/storageos.go

@@ -347,6 +349,14 @@ func (b *storageosMounter) SetUp(fsGroup *int64) error {
 		b.volNamespace = b.podNamespace
 	}

+	globalPDPath := makeGlobalPDName(b.plugin.host, b.pvName, b.podNamespace, b.volName)


How does access pattern of storageOS works on multiple pods using same volume on same node. Can the volume be attached multiple times on the node with different paths?

In general I am not sure why this volume uses pod's namespace in something that it calls "GlobalPDPath". That implies this path can only be reused if another pod within same namespace uses same volume. What happens if another pod but in different namespace uses the volume? I am not super familiar with StorageOS and hence these questions.

Yes, a volume can be used by multiple pods on the same node if they are in the same namespace as the PVC.

b.podNamespace looks to be incorrect there. Should be b.volNamespace. I'll change that.

The variable name globalPDPath can be changed too. It does sounds misleading. It's just the target path where a loop device is mounted and made accessible to a pod.
I'll rename it to target path.

okay, once tests are fixed I will lgtm.

AttachDevice() ensures that the volume device is attached to the host before they are used.

darkowlzz · 2019-01-29T10:34:03Z

/retest

darkowlzz · 2019-01-29T11:58:03Z

/retest

darkowlzz · 2019-01-30T06:32:54Z

/retest

k8s-ci-robot · 2019-01-30T07:11:37Z

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

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-kops-aws	`835183a`	link	`/test pull-kubernetes-e2e-kops-aws`

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.

darkowlzz · 2019-01-30T11:03:41Z

Can someone help with this test failure?
It doesn't look like it's related to this change at all: waiting for ready nodes timed out.

gnufied · 2019-01-30T17:12:09Z

/lgtm

gnufied · 2019-02-07T14:42:07Z

/approve

darkowlzz · 2019-02-07T15:00:45Z

/assign @thockin

thockin · 2019-02-20T18:25:07Z

Punting to newly minted dep-approver @apelisse !

apelisse · 2019-02-20T18:42:24Z

Godeps: Updating StorageOS dependency, no other new dependencies. Seems reasonable to me.

/approve

k8s-ci-robot · 2019-02-20T18:42:50Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: apelisse, darkowlzz, gnufied, 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:

~~Godeps/OWNERS~~ [apelisse]
~~pkg/volume/OWNERS~~ [gnufied,saad-ali]
~~vendor/OWNERS~~ [apelisse]

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

kubernetes#69782 introduced a change to register the device attachment (in the StorageOS API) prior to the volume attachment. The volume attachment code would clear any mount info, causing the StorageOS API to register the mount and then immediately de-register it. The code to clear the mount info on volume attach is no longer needed. It was used to force-mount a volume if StorageOS thought it was already mounted. In practice it was not needed, and administrators have other ways of clearing stale mount information if required.

k8s-ci-robot requested review from brendandburns and rootfs October 14, 2018 13:50

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 Oct 14, 2018

k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Oct 18, 2018

k8s-ci-robot assigned gnufied Dec 6, 2018

darkowlzz force-pushed the storageos-updates branch from 251e61c to 6828692 Compare December 10, 2018 05:47

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 10, 2018

darkowlzz force-pushed the storageos-updates branch 2 times, most recently from a5150f9 to 6b5af9e Compare December 10, 2018 08:05

gnufied reviewed Dec 12, 2018

View reviewed changes

darkowlzz changed the title ~~StorageOS attach device before volume attach & volume expansion support~~ StorageOS attach device before volume attach Dec 12, 2018

gnufied reviewed Jan 23, 2019

View reviewed changes

darkowlzz force-pushed the storageos-updates branch from 6b5af9e to f6b9c57 Compare January 28, 2019 17:26

darkowlzz added 2 commits January 28, 2019 22:57

StorageOS update api library

a4cc754

Add AttachDevice() to attach the device to a host

835183a

AttachDevice() ensures that the volume device is attached to the host before they are used.

darkowlzz force-pushed the storageos-updates branch from f6b9c57 to 835183a Compare January 28, 2019 17:28

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

k8s-ci-robot assigned thockin Feb 7, 2019

thockin assigned apelisse and unassigned thockin Feb 20, 2019

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

k8s-ci-robot merged commit 23a0850 into kubernetes:master Feb 20, 2019

This was referenced May 30, 2019

StorageOS volumes do not register as mounted in StorageOS API #78517

Closed

StorageOS volume driver: Remove call to clear mount info if already set #78522

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

StorageOS attach device before volume attach #69782

StorageOS attach device before volume attach #69782

darkowlzz commented Oct 14, 2018 •

edited

idealhack commented Oct 18, 2018

darkowlzz commented Oct 18, 2018 •

edited

croomes commented Nov 14, 2018

saad-ali commented Dec 6, 2018

idealhack commented Dec 10, 2018

darkowlzz commented Dec 10, 2018

darkowlzz commented Dec 10, 2018

darkowlzz commented Dec 10, 2018

darkowlzz commented Dec 10, 2018

gnufied left a comment

gnufied Dec 10, 2018

darkowlzz Jan 1, 2019

gnufied Dec 12, 2018

darkowlzz Jan 1, 2019 •

edited

gnufied Dec 12, 2018

darkowlzz Jan 1, 2019

gnufied Jan 23, 2019

darkowlzz Jan 28, 2019 •

edited

gnufied Jan 28, 2019

darkowlzz commented Jan 29, 2019

darkowlzz commented Jan 29, 2019

darkowlzz commented Jan 30, 2019

k8s-ci-robot commented Jan 30, 2019

darkowlzz commented Jan 30, 2019

gnufied commented Jan 30, 2019

gnufied commented Feb 7, 2019

darkowlzz commented Feb 7, 2019

thockin commented Feb 20, 2019

apelisse commented Feb 20, 2019

k8s-ci-robot commented Feb 20, 2019

StorageOS attach device before volume attach #69782

StorageOS attach device before volume attach #69782

Conversation

darkowlzz commented Oct 14, 2018 • edited

idealhack commented Oct 18, 2018

darkowlzz commented Oct 18, 2018 • edited

croomes commented Nov 14, 2018

saad-ali commented Dec 6, 2018

idealhack commented Dec 10, 2018

darkowlzz commented Dec 10, 2018

darkowlzz commented Dec 10, 2018

darkowlzz commented Dec 10, 2018

darkowlzz commented Dec 10, 2018

gnufied left a comment

Choose a reason for hiding this comment

gnufied Dec 10, 2018

Choose a reason for hiding this comment

darkowlzz Jan 1, 2019

Choose a reason for hiding this comment

gnufied Dec 12, 2018

Choose a reason for hiding this comment

darkowlzz Jan 1, 2019 • edited

Choose a reason for hiding this comment

gnufied Dec 12, 2018

Choose a reason for hiding this comment

darkowlzz Jan 1, 2019

Choose a reason for hiding this comment

gnufied Jan 23, 2019

Choose a reason for hiding this comment

darkowlzz Jan 28, 2019 • edited

Choose a reason for hiding this comment

gnufied Jan 28, 2019

Choose a reason for hiding this comment

darkowlzz commented Jan 29, 2019

darkowlzz commented Jan 29, 2019

darkowlzz commented Jan 30, 2019

k8s-ci-robot commented Jan 30, 2019

darkowlzz commented Jan 30, 2019

gnufied commented Jan 30, 2019

gnufied commented Feb 7, 2019

darkowlzz commented Feb 7, 2019

thockin commented Feb 20, 2019

apelisse commented Feb 20, 2019

k8s-ci-robot commented Feb 20, 2019

darkowlzz commented Oct 14, 2018 •

edited

darkowlzz commented Oct 18, 2018 •

edited

darkowlzz Jan 1, 2019 •

edited

darkowlzz Jan 28, 2019 •

edited