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

Attacher/Detacher refactor for local storage #66884

Merged
merged 3 commits into from Aug 15, 2018

Conversation

@NickrenREN
Member

NickrenREN commented Aug 2, 2018

Proposal link: kubernetes/community#2438

What this PR does / why we need it:

Attacher/Detacher refactor for the plugins which just need to mount device, but do not need to attach, such as local storage plugin.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Attacher/Detacher refactor for local storage

/sig storage
/kind feature

@NickrenREN

This comment has been minimized.

Show comment
Hide comment
@NickrenREN
Member

NickrenREN commented Aug 2, 2018

/retest

/assign @msau42 @jsafrane @saad-ali

@@ -294,6 +294,10 @@ func (plugin *TestPlugin) NewAttacher() (volume.Attacher, error) {
return &attacher, nil
}
func (plugin *TestPlugin) NewDeviceMounter() (volume.DeviceMounter, error) {
return plugin.NewAttacher()

This comment has been minimized.

@msau42

msau42 Aug 10, 2018

Member

Hm, I wonder if this would cause any issues for attachable plugins if they had been keeping some state in the Attacher object.

@msau42

msau42 Aug 10, 2018

Member

Hm, I wonder if this would cause any issues for attachable plugins if they had been keeping some state in the Attacher object.

This comment has been minimized.

@NickrenREN

NickrenREN Aug 11, 2018

Member

The refactor move MountDevice() and GetDeviceMountPath() from Attacher to DeviceMounter.
The difference is:
Before this change: we will create an Attacher and let it call WaitForAttach, GetDeviceMountPath() and MountDevice()
After this change: we will create an Attacher and a DeviceMounter, let Attacher call WaitForAttach , let DeviceMounter call GetDeviceMountPath() and MountDevice().

Here, It seems that GetDeviceMountPath() and MountDevice() will not change the state kept in Attacher object(which is attachedVolumeMap), and also the ErrorEncountered will not affect the logic, since we will return directly if error occurs

BTW: for now, we will create a new Attacher in both GenerateMountVolumeFunc and GenerateAttachVolumeFunc which are called by different components. It seems that we should avoid keeping state in Attacher object.

please correct me if i am wrong.

@NickrenREN

NickrenREN Aug 11, 2018

Member

The refactor move MountDevice() and GetDeviceMountPath() from Attacher to DeviceMounter.
The difference is:
Before this change: we will create an Attacher and let it call WaitForAttach, GetDeviceMountPath() and MountDevice()
After this change: we will create an Attacher and a DeviceMounter, let Attacher call WaitForAttach , let DeviceMounter call GetDeviceMountPath() and MountDevice().

Here, It seems that GetDeviceMountPath() and MountDevice() will not change the state kept in Attacher object(which is attachedVolumeMap), and also the ErrorEncountered will not affect the logic, since we will return directly if error occurs

BTW: for now, we will create a new Attacher in both GenerateMountVolumeFunc and GenerateAttachVolumeFunc which are called by different components. It seems that we should avoid keeping state in Attacher object.

please correct me if i am wrong.

This comment has been minimized.

@msau42

msau42 Aug 11, 2018

Member

I'm worried about situations where a plugin may have kept some state to serialize between WaitForAttach and MountDevice, like a lock or some cached data.

@msau42

msau42 Aug 11, 2018

Member

I'm worried about situations where a plugin may have kept some state to serialize between WaitForAttach and MountDevice, like a lock or some cached data.

This comment has been minimized.

@NickrenREN

NickrenREN Aug 11, 2018

Member

Got it, for now, i can not find the state kept between WaitForAttach and MountDevice through the plugins.
But should we cache the attacher info for the situations you are worried about ? e.g. if the attacher is created before, we do not need to create a new one in NewDeviceMounter...

/cc @jsafrane @saad-ali

@NickrenREN

NickrenREN Aug 11, 2018

Member

Got it, for now, i can not find the state kept between WaitForAttach and MountDevice through the plugins.
But should we cache the attacher info for the situations you are worried about ? e.g. if the attacher is created before, we do not need to create a new one in NewDeviceMounter...

/cc @jsafrane @saad-ali

This comment has been minimized.

@jsafrane

jsafrane Aug 13, 2018

Member

I went through volume plugins, none of them really passes information from WaitForAttach to MountDevice via the attacher instance.

@jsafrane

jsafrane Aug 13, 2018

Member

I went through volume plugins, none of them really passes information from WaitForAttach to MountDevice via the attacher instance.

This comment has been minimized.

@msau42

msau42 Aug 13, 2018

Member

Thanks guys for searching! This sounds fine to me then.

@msau42

msau42 Aug 13, 2018

Member

Thanks guys for searching! This sounds fine to me then.

@k8s-ci-robot k8s-ci-robot requested review from jsafrane and saad-ali Aug 11, 2018

@jsafrane

This comment has been minimized.

Show comment
Hide comment
@jsafrane

jsafrane Aug 13, 2018

Member

I quickly went through the PR and I like it.
/approve
(this does not mean lgtm, I need more time or someone else may lgtm it)

Member

jsafrane commented Aug 13, 2018

I quickly went through the PR and I like it.
/approve
(this does not mean lgtm, I need more time or someone else may lgtm it)

// DeviceMounter can mount a block volume to a global path.

This comment has been minimized.

@gnufied

gnufied Aug 13, 2018

Member

Can we drop block volume word from here? it seems iffy - there are block volumes which don't necessarily use DeviceMounter and there could be non-block volumes that use the interface?

@gnufied

gnufied Aug 13, 2018

Member

Can we drop block volume word from here? it seems iffy - there are block volumes which don't necessarily use DeviceMounter and there could be non-block volumes that use the interface?

This comment has been minimized.

@NickrenREN

NickrenREN Aug 14, 2018

Member

there are block volumes which don't necessarily use DeviceMounter

which is true, for block mode.

there could be non-block volumes that use the interface

Actually, for now, local storage is the only use case that we only implement DeviceMounter( not implement Attacher interface).
We will only mount block device to a global path. If the local storage is a fs dir, we will do nothing, just set global path to its Path in pv spec.

@NickrenREN

NickrenREN Aug 14, 2018

Member

there are block volumes which don't necessarily use DeviceMounter

which is true, for block mode.

there could be non-block volumes that use the interface

Actually, for now, local storage is the only use case that we only implement DeviceMounter( not implement Attacher interface).
We will only mount block device to a global path. If the local storage is a fs dir, we will do nothing, just set global path to its Path in pv spec.

@gnufied

This comment has been minimized.

Show comment
Hide comment
@gnufied

gnufied Aug 13, 2018

Member

minor nit but otherwise lgtm!

Member

gnufied commented Aug 13, 2018

minor nit but otherwise lgtm!

@@ -294,6 +294,10 @@ func (plugin *TestPlugin) NewAttacher() (volume.Attacher, error) {
return &attacher, nil
}
func (plugin *TestPlugin) NewDeviceMounter() (volume.DeviceMounter, error) {
return plugin.NewAttacher()

This comment has been minimized.

@msau42

msau42 Aug 13, 2018

Member

Thanks guys for searching! This sounds fine to me then.

@msau42

msau42 Aug 13, 2018

Member

Thanks guys for searching! This sounds fine to me then.

Show outdated Hide outdated pkg/volume/util/operationexecutor/operation_generator.go
@@ -478,35 +478,45 @@ func (og *operationGenerator) GenerateMountVolumeFunc(
volumeAttacher, _ = attachableVolumePlugin.NewAttacher()
}
// get deviceMounter, if possible
deviceMountableVolumePlugin, _ := og.volumePluginMgr.FindDeviceMountablePluginBySpec(volumeToMount.VolumeSpec)

This comment has been minimized.

@msau42

msau42 Aug 13, 2018

Member

Can you add unit tests for these new plugin scenarios?

@msau42

msau42 Aug 13, 2018

Member

Can you add unit tests for these new plugin scenarios?

This comment has been minimized.

@NickrenREN

NickrenREN Aug 14, 2018

Member

I am not sure i understand the scenarios. Just added some UTs for Concurrent Mount operations for devicemountable plugins in the third commit. PTAL, thanks

@NickrenREN

NickrenREN Aug 14, 2018

Member

I am not sure i understand the scenarios. Just added some UTs for Concurrent Mount operations for devicemountable plugins in the third commit. PTAL, thanks

This comment has been minimized.

@msau42

msau42 Aug 14, 2018

Member

I mean adding test cases for DeviceMountable-only plugins for Mount and Unmount To ensure that we have correctly decoupled the attacher

@msau42

msau42 Aug 14, 2018

Member

I mean adding test cases for DeviceMountable-only plugins for Mount and Unmount To ensure that we have correctly decoupled the attacher

This comment has been minimized.

@NickrenREN

NickrenREN Aug 14, 2018

Member

We haven't implemented any DeviceMountable-only plugins for now, this PR is just interface refactor. We can add these UTs in my another PR: #63011
WDYT ?

@NickrenREN

NickrenREN Aug 14, 2018

Member

We haven't implemented any DeviceMountable-only plugins for now, this PR is just interface refactor. We can add these UTs in my another PR: #63011
WDYT ?

@NickrenREN

This comment has been minimized.

Show comment
Hide comment
@NickrenREN

NickrenREN Aug 14, 2018

Member

/retest

Member

NickrenREN commented Aug 14, 2018

/retest

@NickrenREN

This comment has been minimized.

Show comment
Hide comment
@NickrenREN

NickrenREN Aug 14, 2018

Member

Errors seem not related to this change

/retest

Member

NickrenREN commented Aug 14, 2018

Errors seem not related to this change

/retest

@msau42

This comment has been minimized.

Show comment
Hide comment
@msau42

msau42 Aug 14, 2018

Member

/lgtm

Member

msau42 commented Aug 14, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Aug 14, 2018

@saad-ali

/lgtm

@k8s-ci-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-ci-robot

k8s-ci-robot Aug 14, 2018

Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jsafrane, msau42, NickrenREN, 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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Contributor

k8s-ci-robot commented Aug 14, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jsafrane, msau42, NickrenREN, 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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@msau42

This comment has been minimized.

Show comment
Hide comment
@msau42

msau42 Aug 14, 2018

Member

/retest

Member

msau42 commented Aug 14, 2018

/retest

@k8s-merge-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-merge-robot

k8s-merge-robot Aug 15, 2018

Contributor

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

Contributor

k8s-merge-robot commented Aug 15, 2018

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

@k8s-merge-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-merge-robot

k8s-merge-robot Aug 15, 2018

Contributor

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

Contributor

k8s-merge-robot commented Aug 15, 2018

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

@k8s-merge-robot k8s-merge-robot merged commit c5e74d1 into kubernetes:master Aug 15, 2018

16 of 18 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-kubemark-e2e-gce-big
Details
pull-kubernetes-local-e2e-containerized Job triggered.
Details
cla/linuxfoundation NickrenREN authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-e2e-kubeadm-gce Skipped
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

@NickrenREN NickrenREN deleted the NickrenREN:attacher-detacher-refactor branch Aug 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment