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

storageclass ceph add imageformat parameter #45805

Merged
merged 3 commits into from Jun 23, 2017

Conversation

weiwei04
Copy link
Contributor

@weiwei04 weiwei04 commented May 15, 2017

What this PR does / why we need it:
Add a imageformat parameter for StorageClass(ceph rbd)

k8s hard coded ceph imageformat 1, according to ceph manual, imageformat 1 was deprecated, we should add an extra ceph parameter to set ceph rbd imageformat. Ceph rbd imageformat can only be 1 or 2, set the default value to 1.

Release note:

Allow StorageClass Ceph RBD to specify image format and image features.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 15, 2017
@k8s-github-robot k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note-label-needed labels May 15, 2017
@k8s-ci-robot
Copy link
Contributor

Hi @weiwei04. 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-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 15, 2017
@weiwei04
Copy link
Contributor Author

weiwei04 commented May 17, 2017

/assign @saad-ali please take a look, thanks.

@weiwei04
Copy link
Contributor Author

@saad-ali any comments on this pr?

@weiwei04
Copy link
Contributor Author

weiwei04 commented Jun 2, 2017

/assign @rootfs would you please take a look, thanks.

default:
return nil, fmt.Errorf("invalid option %q for volume plugin %s", k, r.plugin.GetPluginName())
}
}
// sanity check
if imageFormat != rbdImageFormat1 && imageFormat != rbdImageFormat2 {
return nil, fmt.Errorf("invalid ceph imageformat %s, neither 1 or 2", imageFormat)
Copy link
Contributor

Choose a reason for hiding this comment

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

fmt.Error("invalid ..., expecting %s or %s", imageFormat1, imageFormat2)

@rootfs
Copy link
Contributor

rootfs commented Jun 2, 2017

@k8s-bot ok to test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 2, 2017
@rootfs
Copy link
Contributor

rootfs commented Jun 2, 2017

Though image format 1 is deprecated, it is still used by krbd (which rbd volume is based on). Newer kernel starts to support format 2 but many linux distros don't have them yet. The plan is to wait till they pick up the newer kernel and we add image format options.

@weiwei04
Copy link
Contributor Author

weiwei04 commented Jun 4, 2017

@k8s-bot pull-kubernetes-federation-e2e-gce test this

@weiwei04
Copy link
Contributor Author

weiwei04 commented Jun 4, 2017

@rootfs modified the error log, please take a look, thanks.

@rootfs
Copy link
Contributor

rootfs commented Jun 4, 2017

@leseb

@rootfs
Copy link
Contributor

rootfs commented Jun 4, 2017

@weiwei04 please update readme and more diagnostic messages when older krbd failes to map v2 image

@k8s-ci-robot k8s-ci-robot removed the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 16, 2017
@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. 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. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Jun 16, 2017
@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 16, 2017
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jun 16, 2017
@cofyc
Copy link
Member

cofyc commented Jun 16, 2017

Bump to verify CLA.

@@ -192,6 +193,10 @@ parameters:
* `pool`: Ceph RBD pool. Default is "rbd".
* `userId`: Ceph client ID that is used to map the RBD image. Default is the same as `adminId`.
* `userSecretName`: The name of Ceph Secret for `userId` to map RBD image. It must exist in the same namespace as PVCs. It is required.
* `imageFormat`: Ceph RBD image format, "1" or "2". Default is "1".
* `imageFeatures`: Ceph RBD image format 2 features, comma delimited. This is optional, and only be used if you set `imageFormat` to "2". For a complete list of available image features, please refer to [RBD docs](http://docs.ceph.com/docs/master/man/8/rbd/). By default, all features (except for striping) will be enabled.
Copy link
Contributor

Choose a reason for hiding this comment

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

Not all features are supported and unfortunately RBD docs are not well updated.

@rootfs
Copy link
Contributor

rootfs commented Jun 16, 2017

@weiwei04 @cofyc The readme and feature validation need some work, see my closed PR here
#43816

We cannot turn on exclusive lock feature for now, exclusive lock and advisory lock cannot work together. For now, just turn on layering feature and we'll revisit exclusive lock later.

@weiwei04
Copy link
Contributor Author

/release-note

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Jun 19, 2017
Copy link

@leseb leseb left a comment

Choose a reason for hiding this comment

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

@rootfs lgtm

@cofyc
Copy link
Member

cofyc commented Jun 22, 2017

@rootfs
Is there anything else I need to fix? Are you ok with RBD new parameters, and theirs default behaviors?

@rootfs
Copy link
Contributor

rootfs commented Jun 22, 2017

/lgtm

thanks for contributing!

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

rootfs commented Jun 22, 2017

/approve

@eparis
Copy link
Contributor

eparis commented Jun 22, 2017

/approve no-issue

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: eparis, rootfs, weiwei04

No associated issue. Update pull-request body to add a reference to an issue, or get approval with /approve no-issue

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

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 k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 22, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: eparis, rootfs, weiwei04

Associated issue requirement bypassed by: eparis

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

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

3 similar comments
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: eparis, rootfs, weiwei04

Associated issue requirement bypassed by: eparis

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

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: eparis, rootfs, weiwei04

Associated issue requirement bypassed by: eparis

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

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: eparis, rootfs, weiwei04

Associated issue requirement bypassed by: eparis

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

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

@ianchakeres
Copy link
Contributor

/sig storage

@k8s-ci-robot k8s-ci-robot added the sig/storage Categorizes an issue or PR as relevant to SIG Storage. label Jun 23, 2017
@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit e554dd6 into kubernetes:master Jun 23, 2017
@@ -209,6 +209,7 @@ parameters:
pool: kube
userId: kube
userSecretName: ceph-secret-user
imageFormat: "1"
Copy link
Contributor

Choose a reason for hiding this comment

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

Does "1" need to be in double quotes?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it need to be in double quotes, because parameters is a map of string to string.

@ianchakeres
Copy link
Contributor

Will there ever be a case when imageFormat will be nil or ""? I'm not sure exactly where in the lifecycle of a PV Provision() is called.

For example, if a PV were created before this code was merged.

@cofyc
Copy link
Member

cofyc commented Jun 26, 2017

@ianchakeres

hi,

imageFormat type is string, so it will never to nil. And it will never be empty string (""), because imageFormat is invalid option before this code is merged.

After this code is merged, imageFormat can be missing (not configured) or one of possible values ("1" or "2"). If imageFormat is configured, Provisioner will use configured value to create PVs. If imageFormat is missing, Provisioner will use default value ("1") to create PVs, so old RBD StorageClasses which do not (and can not) specify imageFormat parameter will continue work as before.

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 Denotes a PR that will be considered when it comes time to generate release notes. sig/storage Categorizes an issue or PR as relevant to SIG Storage. 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.

None yet

10 participants