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

Update flex volume 1.6 documentation. #43326

Merged
merged 3 commits into from
Mar 23, 2017

Conversation

chakri-nelluri
Copy link
Contributor

What this PR does / why we need it:
Update documentation for 1.6 flex volume changes.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 17, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@k8s-ci-robot
Copy link
Contributor

Hi @chakri-nelluri. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with @k8s-bot ok to test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

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.

@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Mar 17, 2017
@chakri-nelluri
Copy link
Contributor Author

@saad-ali @rootfs PTAL

Flexvolume enables users to mount vendor volumes into kubernetes. It expects vendor drivers are installed in the volume plugin path on every kubelet node.

It allows for vendors to develop their own drivers to mount volumes on nodes.
Flexvolume enables users to write their own drivers and add support for their volumes in Kubernetes. Vendor drivers should be installed in the volume plugin path on every Kubelet node and on master node(s) if "-enable-controller-attach-detach" Kubelet option is enabled.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing hyphen (-) --enable-controller-attach-detach?

<driver executable> waitfordetach <mount device> <json options>
```

#### Volume is Attached:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only relevant if --enable-controller-attach-detach is set correct?


### Driver invocation model:

Init:
#### Init:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When is this called by kubernetes? From what component (kubelet?)? What is it expected to do?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These questions should be answered for each callout.

#### Mount device:
Mount device mounts the device to a global path which individual pods can then bind mount.

This call-out does not pass "secrets" specified in Flexvolume spec. If your driver requires secrets, do not implement this call-out and instead use "mount" call-out and implement attach and mount in that call-out.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the mount call?

```

#### Attach:
Attach the volume specified by the given spec on the given host. On success, returns the device path where the device is attached on the node. Nodename param is only valid if "-enable-controller-attach-detach" Kubelet option is enabled.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nodename param is only valid if "-enable-controller-attach-detach" Kubelet option is enabled.

When attach/detach controller is disabled, kubelet calls attach, and I believe still provides the node name, no?

@chakri-nelluri
Copy link
Contributor Author

Review status: 0 of 1 files reviewed at latest revision, 5 unresolved discussions.


examples/volumes/flexvolume/README.md, line 3 at r1 (raw file):

Previously, klausenbusk (Kristian Klausen) wrote…

Missing hyphen (-) --enable-controller-attach-detach?

Done.


examples/volumes/flexvolume/README.md, line 18 at r1 (raw file):

Previously, saad-ali (Saad Ali) wrote…

These questions should be answered for each callout.

Added "Called from Kubelet & Controller manager" for relevant callouts


examples/volumes/flexvolume/README.md, line 33 at r1 (raw file):

Previously, saad-ali (Saad Ali) wrote…

Nodename param is only valid if "-enable-controller-attach-detach" Kubelet option is enabled.

When attach/detach controller is disabled, kubelet calls attach, and I believe still provides the node name, no?

Trying to signify it still provides but nodename but it not valid/relevant.


examples/volumes/flexvolume/README.md, line 54 at r1 (raw file):

Previously, klausenbusk (Kristian Klausen) wrote…

Only relevant if --enable-controller-attach-detach is set correct?

It is getting called from both Kubelet & Controller manager right.


examples/volumes/flexvolume/README.md, line 64 at r1 (raw file):

Previously, saad-ali (Saad Ali) wrote…

This is the mount call?

this is the mountdevice call-out.


Comments from Reviewable

@devin-donnelly
Copy link

@saad-ali Does this need to be documented on kubernetes.io (the web documentation)?

@saad-ali
Copy link
Member

@saad-ali Does this need to be documented on kubernetes.io (the web documentation)?

Good question. Intended audience for Flex volume plugin is Kubernetes volume plugin developers. So maybe not?

@saad-ali
Copy link
Member

Other than the concern's @devin-donnelly raised about the correct place for this doc, LGTM

@chakri-nelluri
Copy link
Contributor Author

Thanks @saad-ali

@devin-donnelly I agree with @saad-ali. This is only intended for volume plugin drivers. I don't think we need to update documentation on kubernetes.io.

@saad-ali
Copy link
Member

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 22, 2017
@saad-ali saad-ali added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Mar 22, 2017
@k8s-github-robot k8s-github-robot added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Mar 22, 2017
@saad-ali saad-ali added this to the v1.6 milestone Mar 22, 2017
@saad-ali saad-ali added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. labels Mar 22, 2017
@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 22, 2017
@saad-ali
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 23, 2017
@brendandburns
Copy link
Contributor

/approve

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brendandburns, chakri-nelluri, saad-ali

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 5d7ff2a into kubernetes:master Mar 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants