Spec update of Block volume to support for dynamic provisioning #1595

mtanino · 2018-01-12T20:09:19Z

This PR updates current specification of Block volume to support dynamic provisioning functionality.

Initial design doc proposal for block volume: #1265
Feature issue: kubernetes/enhancements#351

mtanino · 2018-01-12T20:12:46Z

/cc @msau42 @erinboyd @screeley44 @jsafrane
@jingxu97 @rootfs

Please give feedback for the update of dynamic provisioning support, thanks.

k8s-ci-robot · 2018-01-12T20:12:48Z

@mtanino: GitHub didn't allow me to request PR reviews from the following users: erinboyd.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @msau42 @erinboyd @screeley44 @jsafrane
@jingxu97 @rootfs

Please give feedback for the update of dynamic provisioning support, thanks.

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.

jsafrane · 2018-01-15T08:33:55Z

@mtanino, can you please update also "Out of tree provisioning" section in contributors/design-proposals/storage/volume-provisioning.md so we have the whole protocol in one document?

Otherwise this PR is OK.

mtanino · 2018-01-15T15:56:06Z

@jsafrane
Good catch. I'll also update volume-provisioning.md. Thanks.

mtanino · 2018-01-15T19:52:03Z

@jsafrane

@mtanino, can you please update also "Out of tree provisioning" section in contributors/design-proposals/storage/volume-provisioning.md so we have the whole protocol in one document?

Updated.

jsafrane · 2018-01-16T08:18:18Z

Thanks! LGTM, however I'd appreciate if someone else looks at it too. If not, let's merge it say early next week.

msau42 · 2018-01-16T19:50:16Z

contributors/design-proposals/storage/raw-block-pv.md

+provisioners. Therefore, in order to create block volume, provisioners need to support
+`volumeMode` and then create persistent volume with `volumeMode`.
+
+As for the external provisioner case, admin have to carefully provide Kubernetes environment


Can you outline both for internal and external, what happens if the plugin does not support block?

msau42 · 2018-01-16T23:04:44Z

contributors/design-proposals/storage/raw-block-pv.md

+If a storage and plugin don't have an ability to create raw block type of volume,
+then `both internal and external provisioner don't need any update` to support `volumeMode`
+because `volumeMode` in PV and PVC are automatically set to `Filesystem` as a default when
+these volume object are created.


So IIUC, a volume will be created, but because the volumeMode is filesystem, it won't bind to the PVC. So PVC will stay pending forever. And then it's up to the user to manually delete the provisioned PVs? Is there anyway we can propagate some type of error to make it more clear to the user what's wrong?

@msau42 @jsafrane

Good catch. This is a problem, we should provide solution as you mentioned.

Can pv_controller check created volume object after Provision()?
Seems if plugin uses internal provisioner, pv_controller have a chance to inspect whether requested volumeMode in PVC and volumeMode in PV are same or not, so that we can add inspection and show error if they don't match.

But if plugin uses external-provisioner, do we have a chance to inspect PV and show error same as internal provisioner?

so not sure I know of an easy way to catch and handle this other than somehow letting the user know via an event that they supplied volumeMode in their pvc but there are currently no supported plugins or matching volumes.

So in the pv-controller --> index.go, we try to find matchingVolumes based on claimSpecs, etc... We perform this check on misMatchingVolumeModes and maybe we can flag that there is a volumeMode mismatch, if that flag is flipped on and there are no matches meaning we return:

nil, nil (no volume, no error)

maybe we return instead:

nil, <some error type message>

so the caller can see no volume was returned but an error was returned and the caller can handle appropriately

(or another return value indicating that we did find volumeMode being used but did not find a matching volume):

nil, nil, <some message saying plugin may not support volumeMode or no volumes exist">

and maybe then let's put that into an event into the pv and pvc?

Just thinking outloud? @msau42 @jsafrane @mtanino any thoughts?

Yeah for external provisioners, probably the best you can do is generate an event if the PV is prebound to the PVC and if volume mode doesn't match.

@screeley44 @msau42

IIUC, the conclusions are;

For internal provisioner
Check of mismatching volumeMode between PV spec and PVC spec after provision
pv_controller.go

For external provisioner
Check of mismatching volumeMode between PV spec and PVC spec when pv_controller finds matching volumes based on claimSpecs
index.go

+events on the PVC and the PVC is Pending forever (or until admin fixes either internal or external provisioner)

@jsafrane
Which line are you suggesting to add above comment?

It's just for the comment above, the proposal itself is OK.

jsafrane · 2018-01-29T13:43:57Z

@msau42, @screeley44, do you have any more comments or is this good enough to be merged?

msau42 · 2018-01-29T19:22:03Z

lgtm

screeley44 · 2018-01-29T20:02:25Z

lgtm

erinboyd · 2018-01-29T22:09:54Z

lgtm great job @mtanino

erinboyd

In example 2 remove ext4

This commits update current specification of Block volume to support dynamic provisioning functionality.

mtanino · 2018-01-29T23:49:26Z

@erinboyd
Thanks.
To avoid confusion, I removed the line 357 fsType: ext4 from the Example 2) the created PV: of volume-provisioning.md

On the other hands, in the raw-block-pv.md side, we mention below. Therefore I think example of provisioner could have FSType. This makes implementation of provisioner simpler since provisioner doesn't need to change plugin's volumeSource depends on volumeMode.

+Regardless of the volumeMode, provisioner can set `FSType` into the plugin's volumeSource
+but the value will be ignored at the volume plugin side if `volumeMode` is `Block`.

jsafrane · 2018-01-30T07:45:38Z

/lgtm

mtanino · 2018-01-30T18:18:50Z

/assign @saad-ali

saad-ali · 2018-02-01T00:47:19Z

/lgtm
/approve

k8s-ci-robot · 2018-02-01T00:47:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jsafrane, mtanino, saad-ali

The full list of commands accepted by this bot can be found here.

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

~~contributors/design-proposals/storage/OWNERS~~ [saad-ali]

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

mtanino · 2018-02-15T20:12:36Z

kubernetes/kubernetes#59942

Spec update of Block volume to support for dynamic provisioning

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 12, 2018

k8s-ci-robot requested review from childsb and saad-ali January 12, 2018 20:09

k8s-github-robot added the sig/storage Categorizes an issue or PR as relevant to SIG Storage. label Jan 12, 2018

k8s-ci-robot requested review from msau42 and screeley44 January 12, 2018 20:12

k8s-ci-robot requested a review from jsafrane January 12, 2018 20:12

mtanino force-pushed the raw-block-dyn-support branch from fe498b4 to 3b5f713 Compare January 12, 2018 20:26

mtanino force-pushed the raw-block-dyn-support branch from 3b5f713 to 9bf1128 Compare January 15, 2018 19:16

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 15, 2018

mtanino force-pushed the raw-block-dyn-support branch 2 times, most recently from df28078 to 8671dfc Compare January 15, 2018 19:41

msau42 reviewed Jan 16, 2018

View reviewed changes

mtanino force-pushed the raw-block-dyn-support branch from 8671dfc to 5a737af Compare January 16, 2018 22:58

msau42 reviewed Jan 16, 2018

View reviewed changes

mtanino force-pushed the raw-block-dyn-support branch 2 times, most recently from 9745cf9 to 92855d5 Compare January 18, 2018 19:26

mtanino mentioned this pull request Jan 23, 2018

Raw block device using persistent volume source kubernetes/enhancements#351

Closed

erinboyd reviewed Jan 29, 2018

View reviewed changes

Spec update of Block volume to support for dynamic provisioning

c4e1df2

This commits update current specification of Block volume to support dynamic provisioning functionality.

mtanino force-pushed the raw-block-dyn-support branch from 92855d5 to c4e1df2 Compare January 29, 2018 23:41

k8s-ci-robot assigned jsafrane Jan 30, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 30, 2018

k8s-ci-robot assigned saad-ali Jan 30, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 1, 2018

k8s-ci-robot merged commit 2c7ff0d into kubernetes:master Feb 1, 2018

MadhavJivrajani pushed a commit to MadhavJivrajani/community that referenced this pull request Nov 30, 2021

Merge pull request kubernetes#1595 from mtanino/raw-block-dyn-support

5004c8f

Spec update of Block volume to support for dynamic provisioning

Spec update of Block volume to support for dynamic provisioning #1595

Spec update of Block volume to support for dynamic provisioning #1595

Uh oh!

Conversation

mtanino commented Jan 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mtanino commented Jan 12, 2018

Uh oh!

k8s-ci-robot commented Jan 12, 2018

Uh oh!

jsafrane commented Jan 15, 2018

Uh oh!

mtanino commented Jan 15, 2018

Uh oh!

mtanino commented Jan 15, 2018

Uh oh!

jsafrane commented Jan 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

screeley44 Jan 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtanino Jan 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsafrane Jan 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsafrane commented Jan 29, 2018

Uh oh!

msau42 commented Jan 29, 2018

Uh oh!

screeley44 commented Jan 29, 2018

Uh oh!

erinboyd commented Jan 29, 2018

Uh oh!

erinboyd left a comment

Choose a reason for hiding this comment

Uh oh!

mtanino commented Jan 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jsafrane commented Jan 30, 2018

Uh oh!

mtanino commented Jan 30, 2018

Uh oh!

saad-ali commented Feb 1, 2018

Uh oh!

k8s-ci-robot commented Feb 1, 2018

Uh oh!

mtanino commented Feb 15, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

mtanino commented Jan 12, 2018 •

edited

Loading

screeley44 Jan 17, 2018 •

edited

Loading

mtanino Jan 18, 2018 •

edited

Loading

jsafrane Jan 19, 2018 •

edited

Loading

mtanino commented Jan 29, 2018 •

edited

Loading