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.

Sign up for GitHub

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

Jump to bottom

support storage class in Ceph RBD volume #31251

Merged
merged 2 commits into from Sep 10, 2016
Merged

support storage class in Ceph RBD volume #31251

merged 2 commits into from Sep 10, 2016

Conversation

rootfs
Copy link
Contributor

@rootfs rootfs commented Aug 23, 2016 •
edited by k8s-oncall

replace WIP PR #30959, using PV annotation idea from @jsafrane

@kubernetes/sig-storage @johscheuer @elsonrodriguez

This change is Reviewable

@rootfs
support storage class in Ceph RBD volume
5445ccf 
Signed-off-by: Huamin Chen <hchen@redhat.com>
@rootfs rootfs mentioned this pull request Aug 23, 2016
Closed
@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Aug 23, 2016
leseb
leseb reviewed Aug 23, 2016
View reviewed changes
pkg/volume/rbd/rbd.go

func selectorToParam(pv *api.PersistentVolume) (string, string, string, error) {
if pv.Annotations == nil {
return "", "", "", fmt.Errorf("PV has no annotation, cannot get Ceph admin cedentials")
Copy link

Choose a reason for hiding this comment

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

s/cedentials/credentials/

@rootfs
Copy link
Contributor Author

rootfs commented Aug 23, 2016

@leseb @elsonrodriguez

I am considering support the following rbd options as part of the provisioning parameters

--object-size B/K/M
Specifies the object size in B/K/M, it will be rounded up the nearest power of two. The default object size is 4 MB, smallest is 4K and maximum is 32M.

--stripe-unit size-in-B/K/M
Specifies the stripe unit size in B/K/M. See striping section (below) for more details.

--stripe-count num
Specifies the number of objects to stripe over before looping back to the first object. See striping section (below) for more details.

@elsonrodriguez
Copy link
Contributor

elsonrodriguez commented Aug 23, 2016 •
edited

@rootfs is it possible to also control the RBD features via the StorageClass? (fast-diff, deep-flatten, object-map, exclusive-lock etc) is StorageClass the right place for those RBD features?

Also, would it make sense to specify multiple pools in the StorageClass, and have provisioning round-robin the pools?

Side note, I'm in the process of reworking the documentation: https://github.com/elsonrodriguez/kubernetes/tree/rbd-prov3-docs/examples/experimental/persistent-volume-provisioning

I'll send a follow-up PR after this is merged.

edit:
Changed URL from commit to branch URL: https://github.com/elsonrodriguez/kubernetes/tree/rbd-prov3-docs/examples/experimental/persistent-volume-provisioning

@elsonrodriguez
Copy link
Contributor

@rootfs This is working pretty good.

Provisioning, Mounting, Remounting after pod deletion, all works.

Going to run through some more scenarios.

@leseb
Copy link

leseb commented Aug 24, 2016 •
edited

@elsonrodriguez I think we can introduce the support of more features later. As discussed with @rootfs krbd doesn't support all the image features yet, it really depends on the kernel you're using.
It's basically playing catchup with librbd.

@rootfs thanks for addressing my comments!

jsafrane
jsafrane reviewed Aug 24, 2016
View reviewed changes
examples/experimental/persistent-volume-provisioning/README.md
provisioner: kubernetes.io/rbd
parameters:
monitors: 10.16.153.105:6789
adminID: kube
Copy link
Member

Choose a reason for hiding this comment

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

adminId, to follow userId below. Or the other way around.

@jsafrane
Copy link
Member

Reviewed 7 of 9 files at r1, 2 of 2 files at r2.
Review status: all files reviewed at latest revision, 16 unresolved discussions, some commit checks failed.

Comments from Reviewable

@jsafrane jsafrane added sig/storage Categorizes an issue or PR as relevant to SIG Storage. team/cluster release-note-none Denotes a PR that doesn't merit a release note. labels Aug 24, 2016
@rootfs
review feedbacks
0c3b2f4 
Signed-off-by: Huamin Chen <hchen@redhat.com>
@jsafrane
Copy link
Member

Reviewed 6 of 6 files at r3.
Review status: all files reviewed at latest revision, 10 unresolved discussions.

Comments from Reviewable

@jsafrane
Copy link
Member

I am happy with overall quality of the PR. @elsonrodriguez, @leseb, can you check please Ceph-related parts?

@leseb
Copy link

leseb commented Aug 26, 2016

@jsafrane LGTM (from a Ceph angle)
Thanks!

@jsafrane jsafrane added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 26, 2016
@elsonrodriguez
Copy link
Contributor

elsonrodriguez commented Aug 26, 2016 •
edited

@jsafrane Tested the latest commit and functionality looks good as before, however there is still no unlocking mechanism for dead kubelets.

I think we need this feature, but if the unlocking is out of scope for this PR(thoughts, @rootfs?), then LGTM.

@apelisse
Copy link
Member

@pwittrock @jsafrane Hi, this is 500+ LOC. Is this a bug fix? Should it be in 1.4? Thanks

@apelisse apelisse added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Aug 26, 2016
@apelisse
Copy link
Member

@pwittrock @saad-ali @matchstick Is this something we should make an exception for?
@goltermann I know there is a link for the exception process, do you have that link?

@matchstick
Copy link
Contributor

Here is the link to the exceptions process for adding features or large refactors post feature freeze date. @rootfs @pmorie @saad-ali @thockin @jsafrane @childsb Is there a case why to add this in the 1.4 window? If so please follow the application for an exception procedures which are outlined in the link above.

@saad-ali
Copy link
Member

Will remove 1.4 milestone from the PR. If exception process determines we want this in v1.4, it can be re-added.

@saad-ali saad-ali removed this from the v1.4 milestone Aug 26, 2016
@k8s-bot
Copy link

k8s-bot commented Aug 30, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message will repeat several times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

jsafrane
jsafrane reviewed Aug 31, 2016
View reviewed changes
pkg/volume/rbd/rbd.go
@@ -262,3 +448,24 @@ func getVolumeSource(

return nil, false, fmt.Errorf("Spec does not reference a RBD volume type")
}

// parseSecretMap locates the secret by key name.
Copy link
Member

Choose a reason for hiding this comment

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

nit: parseSecretMap -> parseSecret

@rootfs
Copy link
Contributor Author

rootfs commented Sep 2, 2016

Will update after #31886

@k8s-bot
Copy link

k8s-bot commented Sep 6, 2016

GCE e2e build/test passed for commit 0c3b2f4.

@rootfs
Copy link
Contributor Author

rootfs commented Sep 6, 2016

@apelisse can you get rid of do-not-merge label so it can merge to 1.5?

@apelisse apelisse removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Sep 6, 2016
@k8s-github-robot
Copy link

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

@k8s-bot
Copy link

k8s-bot commented Sep 10, 2016

GCE e2e build/test passed for commit 0c3b2f4.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 34141a7 into kubernetes:master Sep 10, 2016
@elsonrodriguez elsonrodriguez mentioned this pull request Sep 12, 2016
Merged
pospispa pushed a commit to pospispa/openshift-docs that referenced this pull request Dec 8, 2016
Added Dynamic Provisioning for Ceph RBD
543668b 
Dynamic provisioning for Ceph RBD was implemented in Kubernetes in PR: kubernetes/kubernetes#31251
So it is also a part of OpenShift.

That's why the Ceph RBD dynamic provisioner documentation is added.
pospispa pushed a commit to pospispa/openshift-docs that referenced this pull request Dec 9, 2016
Added Dynamic Provisioning for Ceph RBD
8245db6 
Dynamic provisioning for Ceph RBD was implemented in Kubernetes in PR: kubernetes/kubernetes#31251
So it is also a part of OpenShift.

That's why the Ceph RBD dynamic provisioner documentation is added.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@pmorie pmorie

Labels
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. sig/storage Categorizes an issue or PR as relevant to SIG Storage. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet