Some additional context:

1. The first option implemented -- very easy, but all volumes get a method not relevant to them: Add GetAccessModes to volume plugin interface #5398.
2. PersistentVolumeManager exists today and is part of the PV framework: WIP: Persistent Storage Volumes #5344.
3. PersistentVolumeController (name TBD) is a placeholder idea for dynamic provisioning. I presume all persistent volumes would implement the interface for dynamic provisioning for its volume type.

I prefer option 3 if only for packaging purposes. It will be nice to have just 1 place to define a volume and all its interfaces implemented. This maintains the idea of having an entirely self-contained volume that can be dropped in as a directory with no intimate knowledge of the plugin leaking outside that directory.

More to come after I address the API feedback left for me yesterday and digest this PR more fully.

3 doesn't sound terrible. All nfs code in one place benefits the extender because "copy-and-paste-extension" becomes much more obvious.

On Mar 19, 2015, at 2:38 AM, Tim Hockin notifications@github.com wrote:

With @markturansky adding persistent volumes, it becomes clear that volume plugins are needed in more than one kubernetes process. Specifically (at least, perhaps not complete):

1. PersistentVolumeManager needs to get the access modes for each type of persistent volume.
2. PersistentVolumeController needs to be able to Create/Destroy/Recycle volumes of each type.
3. Kubelet needs to be able to Mount() and Unmount() each type of volume.

Each of these needs different functionality from the plugin. I'm trying to balance the desire to have plugins be mostly opaque to the surrounding system with the desire to not have to sprinkle support for a given volume type all over the place.

Options that I saw:

1. Extend the existing volume.Interface to host a bunch of new methods. Downside is that all plugins (even those that will never support persistent volumes such as secrets) have to implement those functions (even just to panic). Would need to move pkg/kubelet/volume up to pkg/volume. Upside is simple.

2. Make 2 or 3 different plugin interfaces in 2 or 3 different pkg/something dirs. Downside is that adding a new volume type means creating 1, 2, or 3 new pkg dirs in various places in the codebase with related but not overlapping functionality. E.g. there would be 2 or 3 'nfs' packages implementing 2 or 3 different plugin interfaces. Upside is that the are cleanly factored.

3. Make 2 or 3 different plugin interfaces but put them all into a single pkg per volume type. Each caller would use a different entrypoint to the plugin pkg (ProbeNodeVolumePlugins, ProbePersistentVolumePlugins, etc). Downside is that naming gets clumsy. Upside is that all (e.g.) NFS code is in a single place, though each caller still needs to enumerate which plugins it supports.

I figured option 1 was right out. I preferred option 3 to option 2, so I implemented the basis for it. I don't HATE it, but I don't love it. I could be talked into option 2. I think we could limit option 2 to two places and 2 interfaces. Not so bad.

@markturansky @smarterclayton @bgrant0607 @vishh for smart opinions. This may set precedent for plugins that span the whole system, so I want to tread carefully.

This PR was mostly gorename.

You can view, comment on, or merge this pull request online at:

#5642

Commit Summary

Move pkg/kubelet/volume to pkg/volume
Rename all volume symbols to say 'Node'
File Changes

M cmd/integration/integration.go (6)
M cmd/kubelet/app/plugins.go (28)
M cmd/kubelet/app/server.go (9)
M cmd/kubernetes/kubernetes.go (2)
M pkg/kubelet/kubelet.go (9)
M pkg/kubelet/kubelet_test.go (12)
M pkg/kubelet/volumes.go (2)
R pkg/volume/doc.go (0)
R pkg/volume/empty_dir/empty_dir.go (18)
R pkg/volume/empty_dir/empty_dir_linux.go (0)
R pkg/volume/empty_dir/empty_dir_test.go (8)
R pkg/volume/empty_dir/empty_dir_unsupported.go (0)
R pkg/volume/gce_pd/gce_pd.go (14)
R pkg/volume/gce_pd/gce_pd_test.go (14)
R pkg/volume/gce_pd/gce_util.go (0)
R pkg/volume/gce_pd/gce_util_test.go (0)
R pkg/volume/git_repo/git_repo.go (12)
R pkg/volume/git_repo/git_repo_test.go (22)
R pkg/volume/host_path/host_path.go (12)
R pkg/volume/host_path/host_path_test.go (10)
R pkg/volume/plugins.go (27)
R pkg/volume/secret/secret.go (12)
R pkg/volume/secret/secret_test.go (16)
R pkg/volume/testing.go (62)
R pkg/volume/volume.go (8)
Patch Links:

https://github.com/GoogleCloudPlatform/kubernetes/pull/5642.patch
https://github.com/GoogleCloudPlatform/kubernetes/pull/5642.diff
—
Reply to this email directly or view it on GitHub.

Is each new volume type really going to need a new implementation of all three interfaces? e.g. I would imagine that several different volume types would be mountable by a single common kubelet plugin based around *nix 'mount', perhaps? Similarly an iscsi-initiator based plugin might be able to mount volumes from multiple different storage vendors (NetApp, EMC etc), whereas the vendor interfaces for creation, deletion etc of volumes are mostly different (NetApp OnTAP, EMC Atmos etc).

Let me go and read and digest Mark's PV and plugin stuff, after which I hope to be able to provide more intelligent input :-)

@quinton-hoole I think you're right in that many volumes will mount similarly. We have a few people working on other storage plugins (iscsci and gluster, currently, and i've heard ceph bandied about). We've already agreed that we will likely find commonality among them and will take on the refactoring task of making them work well and adhere to whatever patterns we discover.

The move and rename LGTM. Also, "gore name" is the worst (or best) name for a Go extension.

As a slight counter to my own proposal, which do you find least fugly?

volume.{NodePlugin, NodeHost, NodeVolume, NodePluginMgr},
volume.{PersistentPlugin, PersistentHost, PersistentVolume,
PersistentPluginMgr}

or

volume.{Plugin, Host, Volume, PluginMgr}, volume.{PersistentPlugin,
PersistentHost, PersistentVolume, PersistentPluginMgr}

The latter leaves all of the node types "undecorated", since every volume
plugin has to implement those, but only some will implement Persistent*

I want to make this decision soon, polish off the PR, commit, and let you
proceed.

On Thu, Mar 19, 2015 at 12:15 PM, Mark Turansky notifications@github.com
wrote:

The move and rename LGTM. Also, "gore name" is the worst (or best) name
for a Go extension.

—
Reply to this email directly or view it on GitHub
#5642 (comment)
.

+1 for the 3rd option. Although a bit tedious with naming, it makes it easy to manage all the plugins.

As for NodePlugin vs Plugin I prefer the latter.

/sub

As to your two choices above, I think I prefer the latter ("volume.{Plugin, Host, Volume, PluginMgr}, volume.{PersistentPlugin, PersistentHost, PersistentVolume, PersistentPluginMgr}") but I'll know with certainty when I start using it!

updated PR is coming soon

Third choice - it's more verbose but I think it is most consistent.

volume.{VolumePlugin, VolumeHost, Volume, VolumePluginMgr} and
volume.{PersistentVolumePlugin, PersistentVolumeHost, PersistentVolume, PersistentVolumePluginMgr}

I'm pushing an updated PR with 2 commits. First commit is the minimal
(Plugin, Host, Volume, PluginMgr). 2nd commit just does the extra renames
(VolumePlugin, VolumeHost, Volume, VolumePluginMgr)

On Thu, Mar 19, 2015 at 1:36 PM, Mark Turansky notifications@github.com
wrote:

As to your two choices above, I think I prefer the latter
("volume.{Plugin, Host, Volume, PluginMgr}, volume.{PersistentPlugin,
PersistentHost, PersistentVolume, PersistentPluginMgr}") but I'll know with
certainty when I start using it!

—
Reply to this email directly or view it on GitHub
#5642 (comment)
.

I can take or leave the last commit. I sort of prefer it, so if nobody objects strongly, I'd say go with it.

LGTM. I like the 2nd commit because VolumePluginMgr (for example) is clearer than PluginMgr.

Overall, the renames and moves make sense.

If pkg/volume is now a top-level thing, does it make sense to move pkg/util/mount functionality to pkg/volume?

Needs a rebase @thockin. LGTM otherwise.

rebased

@markturansky re: util/mount - I'd rather leave it as it's own thing - it's potentially useful and has no other kubernetes tie-ins (whcih is what util/ should be for IMO)

LGTM