-
Notifications
You must be signed in to change notification settings - Fork 39.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
StorageOS attach device before volume attach #69782
Conversation
/kind bug |
Not sure why it failed. Some help would be appreciated. /retest |
Hi @rootfs - would you mind having a look when you have time? Thanks! |
251e61c
to
6828692
Compare
a5150f9
to
6b5af9e
Compare
/test pull-kubernetes-verify |
/retest |
/test pull-kubernetes-e2e-kops-aws |
/retest |
I've removed the volume expand related changes from this PR. Will create a separate PR for that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also the title of the PR still says "Volume expansion" please remove it.
pkg/volume/storageos/storageos.go
Outdated
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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can the AttachDevice
be called from control-plane rather than being called from kubelet?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there's a need for that. It just creates a virtual volume by calling a StorageOS API.
// 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets not use capitalization for error messages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
@@ -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 was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
pkg/volume/storageos/storageos.go
Outdated
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, once tests are fixed I will lgtm.
6b5af9e
to
f6b9c57
Compare
AttachDevice() ensures that the volume device is attached to the host before they are used.
f6b9c57
to
835183a
Compare
/retest |
2 similar comments
/retest |
/retest |
@darkowlzz: The following test failed, say
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. |
Can someone help with this test failure? |
/lgtm |
/approve |
/assign @thockin |
Punting to newly minted dep-approver @apelisse ! |
Godeps: Updating StorageOS dependency, no other new dependencies. Seems reasonable to me. /approve |
[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:
Approvers can indicate their approval by writing |
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.
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: