Skip to content
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

Add support for flex volume. #13840

Merged
merged 1 commit into from Dec 29, 2015

Conversation

@chakri-nelluri
Copy link
Contributor

commented Sep 10, 2015

This change list adds support for a new type of volume, 'flex volume'.

The goal of this change is to enable third-party providers to provide their own executable to setup and teardown volumes in Kubernetes.

Plugin takes the following parameters:

  • Provider: Name of the plugin provider
  • Command: Executable to setup or teardown volume
  • VolumeID: ID or name of the volume
  • FSType: File system type
  • Readonly : Mount read-only
  • Options: Extra options to pass to the plugin. Options are passed as a json encoded map[string]string

The executable command is invoked from kubelet to setup & teardown the volumes.

Setup Path:
Executable is invoked with "setup", "volumeID" and "json encoded options" as arguments. Setup returns the block device where the volume is setup, like /dev/mapper/vg-vol1

Teardown Path:
Executable is invoked with "teardown" and "block device" as arguments.

Example flex volume definition:
{
"provider": "blue",
"command": "setup.sh",
"volumeID": "vol1",
"fsType": "ext4",
"options": {
"size": "10g",
"replicas": 3,
"qos": "high"
}
}

@googlebot

This comment has been minimized.

Copy link

commented Sep 10, 2015

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@googlebot googlebot added the cla: no label Sep 10, 2015

@k8s-bot

This comment has been minimized.

Copy link

commented Sep 10, 2015

Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist")

If this message is too spammy, please complain to ixdy.

@chakri-nelluri chakri-nelluri force-pushed the chakri-nelluri:master branch from fc489de to ecab812 Sep 10, 2015

@chakri-nelluri

This comment has been minimized.

Copy link
Contributor Author

commented Sep 10, 2015

@markturansky

This comment has been minimized.

Copy link
Member

commented Sep 11, 2015

Hi @NELCY thanks for the PR.

What kind of storage do you envision a FlexVolume provisioning/attaching/mounting? Can you describe the use case for this volume?

@pmorie

This comment has been minimized.

Copy link
Member

commented Sep 11, 2015

Looks very interesting

On Thursday, September 10, 2015, Chakravarthy Nelluri <
notifications@github.com> wrote:

This change list adds support for a new type of volume, 'flex volume'.

The goal of this change is to enable third-party providers to provide
their own executable to setup and teardown volumes in Kubernetes.

Plugin takes the following parameters:

  • Provider: Name of the plugin provider
  • Command: Executable to setup or teardown volume
  • VolumeID: ID or name of the volume
  • FSType: File system type
  • Readonly : Mount read-only
  • Options: Extra options to pass to the plugin. Options are passed as
    a json encoded map[string]string

The executable command is invoked from kubelet to setup & teardown the
volumes.

Setup Path:
Executable is invoked with "setup", "volumeID" and "json encoded options"
as arguments. Setup returns the block device where the volume is setup,
like /dev/mapper/vg-vol1

Teardown Path:
Executable is invoked with "teardown" and "block device" as arguments.

Example flex volume definition:
{
"provider": "blue",
"command": "setup.sh",
"volumeID": "vol1",
"fsType": "ext4",
"options": {
"size": "10g",
"replicas": 3,
"qos": "high"
}

}

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

#13840
Commit Summary

  • Add support for flex volume. Flex volume adds support for
    thirdparty(provider)

File Changes

Patch Links:


Reply to this email directly or view it on GitHub
#13840.

@chakri-nelluri

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2015

Hi Mark, We are planning to use this plugin to support our own volume type. But this is generic enough to support any kind of volume, with out requiring code changes in Kubernetes.

@chakri-nelluri chakri-nelluri force-pushed the chakri-nelluri:master branch 2 times, most recently from 49e35c5 to 6d2f755 Sep 11, 2015

@chakri-nelluri

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2015

I signed it! CLA Please verify.

@googlebot

This comment has been minimized.

Copy link

commented Sep 11, 2015

CLAs look good, thanks!

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2015

Labelling this PR as size/XL


// NewBuilder is the builder routine to build the volume.
func (plugin *flexVolumePlugin) NewBuilder(spec *volume.Spec, pod *api.Pod, _ volume.VolumeOptions, mounter mount.Interface) (volume.Builder, error) {
return plugin.newBuilderInternal(spec, pod, &flexVolumeUtil{}, mounter, exec.New())

This comment has been minimized.

Copy link
@pmorie

pmorie Sep 13, 2015

Member

Are you planning to support SELinux with this volume type? Currently the SELinux context of the volumes directory is passed to plugins via the VolumeOptions arg to NewBuilder.

You might also be interested in: #12944

This comment has been minimized.

Copy link
@chakri-nelluri

chakri-nelluri Sep 14, 2015

Author Contributor

Thanks @pmore.
Yes. Went through 12944. Looks very interesting. Would extend this plugin and add support for SELinux after your PR is in.

Thinking more, the provider plugin can verify whether the provider implementation of filesystem supports SELinux or not. For example: Some NFS implementations do not support SELinux and some do.

@smarterclayton

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2015

How does an admin manage security of custom volumes? A volume can potentially gain root access, so we need some way of limiting or granting access to the volume type. In addition, command is highly insecure and would have to be controlled.

For network plugins we require them to be registered on each kubelet. Is there a reason not to enforce that for these sorts of volume plugins as well?

@chakri-nelluri

This comment has been minimized.

Copy link
Contributor Author

commented Sep 14, 2015

Thanks @smarterclayton for the feedback.

Volumes still need to be authenticated with the storage server to be attached to a Kubelet node. The authentication parameters are plugin specific and are passed to the plugin using options.

The custom plugin executable still needs to be installed on each Kubelet node in the plugin path. My current assumption is this path is secure and only admin supported plugins are available with the restricted set of permissions. If we want to control it more, I can add support for adding a 'white list of volume types' controlled by cluster admin. Let me know WDYT?

The network plugins are probed today during Kubelet bootstrap. I moved away from this to be able to support dynamically installed plugins with out requiring Kubelet restart. If this is a concern and we deliberately want to restrict the list, I will add support for it.
Also for network plugins today we support only one provider plugin passed with Kubelet command line. My understanding is this is done as there is no way to pass which network plugin to use from pod spec. So I did not consider it for volume plugin.

@markturansky

This comment has been minimized.

Copy link
Member

commented Sep 14, 2015

I'm curious to hear what Tim and Brian think.

No doubt you've seen plugins.go where each binary includes which plugins are compiled into it. The comments there deliberately say there's no magic or dynamic loading of plugins. This is purposeful.

I tend to think this PR breaks that pattern. If this can work for any volume, as you say, then why have volume plugins at all? We just need this one.

That's why I was asking for your use case and type of storage you want to use. Why not a plugin like the rest?

@chakri-nelluri

This comment has been minimized.

Copy link
Contributor Author

commented Sep 14, 2015

Hi Mark,

The intent is not to replace the existing plugins. It is not practical to add all proprietary volume types. The plugin is intended to add flexibility to support other proprietary volumes.

There are so many other good things with the current plugins, like argument validation etc, which are tough to cover using flexible volume plugin.

We wanted to support our own volume type using this plugin. We can add our volume type to Kubelet code like the rest, but our volume type is not a generic volume type and can only be used with our storage. Testing, validating is going to be a problem.

I initially started with HTTP plugin, but ended up with exec based plugin after hearing from @bgrant0607 that we want to follow one model for external plugins and we already use exec based plugin model for networking.

@thockin @bgrant0607 PTAL

@bgrant0607

This comment has been minimized.

Copy link
Member

commented Sep 14, 2015

I doubt I'll have time to look at this in the 1.1 timeframe -- the code-complete deadline is Friday. Extensibility is likely to be a priority for 1.2. I'd like to consider this then, together with several other extension proposals.

@bgrant0607 bgrant0607 added this to the v1.2-candidate milestone Sep 14, 2015

@smarterclayton

This comment has been minimized.

Copy link
Contributor

commented Sep 14, 2015

For an exec based plugin, I would recommend not allowing anything to be
customized except the name of the plugin (which should be a suitably
restricted name like [a-z0-9-]). The admin then has to make sure there is
a binary in that location, and the location can be suitably restricted.
I'm mostly concerned about allowing any client to specify anything that
could possibly be a remote filename / path

On Mon, Sep 14, 2015 at 1:20 PM, Brian Grant notifications@github.com
wrote:

I doubt I'll have time to look at this in the 1.1 timeframe -- the
code-complete deadline is Friday. Extensibility is likely to be a priority
for 1.2. I'd like to consider this then, together with several other
extension proposals.


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

Clayton Coleman | Lead Engineer, OpenShift

@chakri-nelluri

This comment has been minimized.

Copy link
Contributor Author

commented Sep 14, 2015

Hi Clayton,
The plugins are expected to be a from a trusted source installed by the admin. The assumption is trusted plugins should not open any security holes with the extra options.

There are a multitude of options specific to each volume from different providers like number of replicas, snapshot interval etc and my concern we cannot cover all of them in a single spec with out exposing a generic key value pair, like our annotations today.

@k8s-bot

This comment has been minimized.

Copy link

commented Dec 23, 2015

GCE e2e build/test failed for commit fa76de7.

@saad-ali

This comment has been minimized.

Copy link
Member

commented Dec 23, 2015

@k8s-bot test this

@k8s-bot

This comment has been minimized.

Copy link

commented Dec 24, 2015

GCE e2e test build/test passed for commit fa76de7.

@saad-ali

This comment has been minimized.

Copy link
Member

commented Dec 24, 2015

@k8s-bot unit test this

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

commented Dec 24, 2015

The author of this PR is not in the whitelist for merge, can one of the admins add the 'ok-to-merge' label?

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

commented Dec 24, 2015

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot

This comment has been minimized.

Copy link

commented Dec 24, 2015

GCE e2e test build/test passed for commit fa76de7.

@chakri-nelluri

This comment has been minimized.

Copy link
Contributor Author

commented Dec 27, 2015

@k8s-bot unit test this

@wojtek-t

This comment has been minimized.

Copy link
Member

commented Dec 28, 2015

@k8s-bot test this please

@k8s-bot

This comment has been minimized.

Copy link

commented Dec 28, 2015

GCE e2e test build/test passed for commit fa76de7.

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

commented Dec 28, 2015

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot

This comment has been minimized.

Copy link

commented Dec 28, 2015

GCE e2e test build/test passed for commit fa76de7.

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

commented Dec 28, 2015

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot

This comment has been minimized.

Copy link

commented Dec 28, 2015

GCE e2e test build/test passed for commit fa76de7.

@wojtek-t

This comment has been minimized.

Copy link
Member

commented Dec 28, 2015

@k8s-bot unit test this please

2 similar comments
@chakri-nelluri

This comment has been minimized.

Copy link
Contributor Author

commented Dec 29, 2015

@k8s-bot unit test this please

@wojtek-t

This comment has been minimized.

Copy link
Member

commented Dec 29, 2015

@k8s-bot unit test this please

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

commented Dec 29, 2015

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot

This comment has been minimized.

Copy link

commented Dec 29, 2015

GCE e2e test build/test passed for commit fa76de7.

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

commented Dec 29, 2015

Automatic merge from submit-queue

k8s-github-robot added a commit that referenced this pull request Dec 29, 2015
Merge pull request #13840 from nelcy/master
Auto commit by PR queue bot

@k8s-github-robot k8s-github-robot merged commit fec5206 into kubernetes:master Dec 29, 2015

5 checks passed

Jenkins GCE e2e 212 tests run, 92 skipped, 0 failed.
Details
Jenkins unit/integration 2537 tests run, 9 skipped, 0 failed.
Details
Submit Queue The e2e tests are failing. The entire submit queue is blocked.
Details
cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@markturansky

This comment has been minimized.

Copy link
Member

commented Dec 30, 2015

@NELCY lots of code in this PR. nice job on getting it merged.

@chakri-nelluri

This comment has been minimized.

Copy link
Contributor Author

commented Dec 31, 2015

Thanks @markturansky. Really appreciate the support from @saad-ali, you and @thockin.

dims pushed a commit to dims/kubernetes that referenced this pull request Feb 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.