ScaleIO Volume Plugin - Volume attribute fixes and updates #48999

vladimirvivien · 2017-07-16T22:57:37Z

What this PR does / why we need it:
This is a housekeeping PR for small enhancements and fixes to the ScaleIO volume plugin to address issues:

Enforcement of fsGroup
Enable ScaleIO multiple-instance volume mapping
Tighter validation of PVC parameters
Injection of default PVC capacity when omitted
Better alignment of PVC, PV, and volume names for dynamic provisioning

Special notes for your reviewer:

Release note:

Enforcement of fsGroup; enable ScaleIO multiple-instance volume mapping; default PVC capacity; alignment of PVC, PV, and volume names for dynamic provisioning

k8s-ci-robot · 2017-07-16T22:57:44Z

Hi @vladimirvivien. 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 /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.

I understand the commands that are listed here.

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.

vladimirvivien · 2017-07-17T21:06:56Z

/ok-to-test

k8s-ci-robot · 2017-07-17T21:07:10Z

@vladimirvivien: you can't request testing unless you are a kubernetes member.

In response to this:

/ok-to-test

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 · 2017-07-18T07:30:21Z

@kubernetes/sig-storage-pr-reviews

jsafrane · 2017-07-18T07:31:03Z

/ok-to-test

jsafrane · 2017-07-18T09:40:01Z

I0718 07:51:22.017] Verifying hack/make-rules/../../hack/verify-govet.sh
W0718 07:55:16.078] pkg/volume/scaleio/sio_volume_test.go:425: no formatting directive in Fatalf call

vladimirvivien · 2017-07-18T22:14:12Z

/test pull-kubernetes-federation-e2e-gce

jsafrane · 2017-07-19T10:18:17Z

pkg/volume/scaleio/sio_util.go

@@ -107,6 +109,12 @@ func validateConfigs(config map[string]string) error {
 	if config[confKey.system] == "" {
 		return systemNotProvidedErr
 	}
+	if config[confKey.storagePool] == "" {


Doesn't this break backward compatibility? A config file without storagePool and protectionDomain was accepted before and now it is not. This needs at least a release note.

@jsafrane You are correct, existing/deployed config are ok, but newer configs will require these values to be provided in subsequent deployments. I will add that to the release notes and the documentations will also be updated (separate PR).

@jsafrane another clarification on that point. The code was substituting "default" for those values when they were not provided. However, production deployments are most likely not using that value anyway. Backward compat is ok. Will still add it to release note.

jsafrane · 2017-07-19T10:25:05Z

pkg/volume/scaleio/sio_volume.go

@@ -248,6 +248,10 @@ func (v *sioVolume) Provision() (*api.PersistentVolume, error) {
 	// setup volume attrributes
 	name := v.generateVolName()
 	capacity := v.options.PVC.Spec.Resources.Requests[api.ResourceName(api.ResourceStorage)]
+	if capacity.Value() == 0 {


This patch chunk is removed in subsequent patch and IMO it should not be - PVC with requesting 0 bytes is wrong and should be probably rejected.

jsafrane · 2017-07-19T16:00:17Z

pkg/volume/scaleio/sio_volume.go

@@ -248,12 +248,12 @@ func (v *sioVolume) Provision() (*api.PersistentVolume, error) {
 	// setup volume attrributes
 	name := v.generateVolName()
 	capacity := v.options.PVC.Spec.Resources.Requests[api.ResourceName(api.ResourceStorage)]
-	if capacity.Value() == 0 {


I liked this code, refusing wrong PVCs is IMO better than defaulting to magic 8GiB.

jsafrane · 2017-07-19T16:04:34Z

This PR fixes things here and there, I'd appreciate separate PRs next time.

The biggest change is multipleMappings - it was false, now its true. Is that a change that should users worry about? Can it break an existing Kubernetes installation? And will there be a PR that actually makes it configurable? Because this PR adds a new param to AttachVolume, but it is still hardcoded by the caller.

vladimirvivien · 2017-07-19T16:37:28Z

@jsafrane
My attempt was to group related small changes in one PR. I will keep that in mind for the next set of changes and submit smaller PRs.

The multipleMapping change though it touched several files (including tests) is the most inconsequential. The change is for new deployment of PVC/PV and will automatically tell scaleio backend to allow a volume to be allow multiple mapping. So, no it does not effect Kubernetes itself, it is for the ScaleIO backend.

jsafrane · 2017-07-20T07:37:52Z

/retest

vladimirvivien · 2017-07-20T18:16:20Z

/test pull-kubernetes-federation-e2e-gce

jsafrane · 2017-07-21T11:48:08Z

I commented it twice already, perhaps it got lost somewhere. I don't like 8GiB as default volume size if the user does not specify any. This allows user to bypass storage quota - user asks for 0 and is accounted for it, but he gets 8. That's IMO wrong and PVCs with capacity == 0 should be rejected (or default to nearest supported value, which is IMO 1 GB).

vladimirvivien · 2017-07-21T21:31:23Z

@jsafrane Thanks for the feedback. I agree, a request for 0 capacity is an erroneous request. While the minimum size for ScaleIO is 8gig, I will change the code to reject it as you suggested. Thanks.

vladimirvivien · 2017-07-26T03:29:09Z

/test pull-kubernetes-kubemark-e2e-gce

vladimirvivien · 2017-07-30T21:28:27Z

/retest

jsafrane · 2017-07-31T09:53:05Z

@vladimirvivien

I0730 21:37:23.542] Run ./hack/update-bazel.sh

This commit introduces the following updates and fixes: - Enable scaleIO volume multip-mapping based on accessMode - No longer uses "default" as default values for storagepool & protection domain - validates capacity when capacity is zero - Better naming for PV and volume - make mount ro when accessModes contains ROM

vladimirvivien · 2017-07-31T21:38:13Z

/pull-kubernetes-unit

vladimirvivien · 2017-07-31T22:13:49Z

/pull-kubernetes-unit

vladimirvivien · 2017-07-31T23:00:47Z

/test pull-kubernetes-unit

jsafrane · 2017-08-01T12:57:52Z

/lgtm
/approve no-issue

jsafrane · 2017-08-01T12:57:57Z

/retest

k8s-github-robot · 2017-08-01T12:58:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jsafrane, vladimirvivien

Associated issue requirement bypassed by: jsafrane

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

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

~~pkg/volume/OWNERS~~ [jsafrane]

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

vladimirvivien · 2017-08-01T13:53:24Z

Thanks @jsafrane

vladimirvivien · 2017-08-01T14:10:05Z

/test pull-kubernetes-e2e-kops-aws

vladimirvivien · 2017-08-01T15:44:36Z

/test pull-kubernetes-e2e-kops-aws

k8s-github-robot · 2017-08-02T06:35:57Z

Automatic merge from submit-queue (batch tested with PRs 49871, 49422, 49092, 49858, 48999)

…f-#48999-upstream-release-1.7 Automatic merge from submit-queue ScaleIO Volume Plugin - volume attribute updates This commit introduces the following updates and fixes: - Enable scaleIO volume multip-mapping based on accessMode - No longer uses "default" as default values for storagepool & protection domain - validates capacity when capacity is zero - Better naming for PV and volume - make mount ro when accessModes contains ROM **Special notes for your reviewer**: - Related bug - #50794 - This is being cherry-picked for PR #48999 Fixes: #50794 **Release note**: ```release-note ScaleIO: fixed enforcement of fsGroup, enabled multiple-instance volume mapping, adjusted alignment of PVC, PV, and volume names for dynamic provisioning ```

k8s-cherrypick-bot · 2017-09-20T16:09:08Z

Commit found in the "release-1.7" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 16, 2017

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 Jul 16, 2017

k8s-github-robot assigned thockin and deads2k Jul 16, 2017

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jul 16, 2017

vladimirvivien mentioned this pull request Jul 16, 2017

Kubernetes ScaleIO Volume Update (3Q2017) thecodeteam/roadmap#171

Closed

18 tasks

deads2k assigned jsafrane and unassigned deads2k Jul 17, 2017

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 Jul 17, 2017

k8s-ci-robot added the sig/storage Categorizes an issue or PR as relevant to SIG Storage. label Jul 18, 2017

jsafrane reviewed Jul 19, 2017

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 1, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 1, 2017

vladimirvivien mentioned this pull request Aug 1, 2017

ScaleIO Volume Plugin - volume attribute updates #49973

Merged

k8s-github-robot merged commit 0cb5ec7 into kubernetes:master Aug 2, 2017

wojtek-t added this to the v1.7 milestone Aug 21, 2017

wojtek-t added cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. cherrypick-candidate labels Aug 21, 2017

k8s-cherrypick-bot removed the cherrypick-candidate label Sep 20, 2017

vladimirvivien deleted the scaleio-vol-attribs-update branch September 21, 2017 21:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ScaleIO Volume Plugin - Volume attribute fixes and updates #48999

ScaleIO Volume Plugin - Volume attribute fixes and updates #48999

vladimirvivien commented Jul 16, 2017

k8s-ci-robot commented Jul 16, 2017

vladimirvivien commented Jul 17, 2017

k8s-ci-robot commented Jul 17, 2017

jsafrane commented Jul 18, 2017

jsafrane commented Jul 18, 2017

jsafrane commented Jul 18, 2017

vladimirvivien commented Jul 18, 2017

jsafrane Jul 19, 2017

vladimirvivien Jul 19, 2017

vladimirvivien Jul 19, 2017

jsafrane Jul 19, 2017

jsafrane Jul 19, 2017

jsafrane commented Jul 19, 2017

vladimirvivien commented Jul 19, 2017

jsafrane commented Jul 20, 2017

vladimirvivien commented Jul 20, 2017

jsafrane commented Jul 21, 2017

vladimirvivien commented Jul 21, 2017

vladimirvivien commented Jul 26, 2017

vladimirvivien commented Jul 30, 2017

jsafrane commented Jul 31, 2017

vladimirvivien commented Jul 31, 2017

vladimirvivien commented Jul 31, 2017

vladimirvivien commented Jul 31, 2017

jsafrane commented Aug 1, 2017

jsafrane commented Aug 1, 2017

k8s-github-robot commented Aug 1, 2017

vladimirvivien commented Aug 1, 2017

vladimirvivien commented Aug 1, 2017

vladimirvivien commented Aug 1, 2017

k8s-github-robot commented Aug 2, 2017

k8s-cherrypick-bot commented Sep 20, 2017

ScaleIO Volume Plugin - Volume attribute fixes and updates #48999

ScaleIO Volume Plugin - Volume attribute fixes and updates #48999

Conversation

vladimirvivien commented Jul 16, 2017

k8s-ci-robot commented Jul 16, 2017

vladimirvivien commented Jul 17, 2017

k8s-ci-robot commented Jul 17, 2017

jsafrane commented Jul 18, 2017

jsafrane commented Jul 18, 2017

jsafrane commented Jul 18, 2017

vladimirvivien commented Jul 18, 2017

jsafrane Jul 19, 2017

Choose a reason for hiding this comment

vladimirvivien Jul 19, 2017

Choose a reason for hiding this comment

vladimirvivien Jul 19, 2017

Choose a reason for hiding this comment

jsafrane Jul 19, 2017

Choose a reason for hiding this comment

jsafrane Jul 19, 2017

Choose a reason for hiding this comment

jsafrane commented Jul 19, 2017

vladimirvivien commented Jul 19, 2017

jsafrane commented Jul 20, 2017

vladimirvivien commented Jul 20, 2017

jsafrane commented Jul 21, 2017

vladimirvivien commented Jul 21, 2017

vladimirvivien commented Jul 26, 2017

vladimirvivien commented Jul 30, 2017

jsafrane commented Jul 31, 2017

vladimirvivien commented Jul 31, 2017

vladimirvivien commented Jul 31, 2017

vladimirvivien commented Jul 31, 2017

jsafrane commented Aug 1, 2017

jsafrane commented Aug 1, 2017

k8s-github-robot commented Aug 1, 2017

vladimirvivien commented Aug 1, 2017

vladimirvivien commented Aug 1, 2017

vladimirvivien commented Aug 1, 2017

k8s-github-robot commented Aug 2, 2017

k8s-cherrypick-bot commented Sep 20, 2017