Fix translation of Cinder storage classess to CSI #98311

jsafrane · 2021-01-22T14:38:02Z

/kind bug

What this PR does / why we need it:
In-tree Cinder storage class must be translated to CSI:

sc.parameters["fsType"] -> sc.parameters["csi.storage.k8s.io/fstype"]
~~sc.parameters["availability"] -> sc.allowedTopology (with CSI topology keys)~~
sc.allowedTopology (with in-tree topology keys) -> sc.allowedTopology (with CSI topology keys)

Found by running e2e tests with CSI migration enabled. One topology test failed:

[sig-storage] In-tree Volumes [Driver: cinder] [Testpattern: Dynamic PV (delayed binding)] topology should provision a volume and schedule a pod with AllowedTopologies [Suite:openshift/conformance/parallel] [Suite:k8s]

...

ProvisioningFailed: failed to provision volume with StorageClass "e2e-topology-1939-cinder-scf2rsm": error generating accessibility requirements: topology map[topology.cinder.csi.openstack.org/zone:nova] from selected node "jsafrane-5x4gw-worker-0-vc2z9" is not in requisite: [map[failure-domain.beta.kubernetes.io/zone:nova]]

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Fixed provisioning of Cinder volumes migrated to CSI when StorageClass with AllowedTopologies was used.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

jsafrane · 2021-01-22T14:38:32Z

/sig storage
/priority important-soon

jsafrane · 2021-01-25T10:02:57Z

/retest

ialidzhikov · 2021-01-25T18:55:31Z

/test pull-kubernetes-integration

jsafrane · 2021-01-26T08:57:09Z

cc @kubernetes/provider-openstack-pr-reviews, PTAL.

msau42 · 2021-01-27T18:18:03Z

/approve
/triage accepted

Will let openstack folks give final review

ramineni · 2021-01-28T05:08:56Z

@jsafrane sc.parameters["availability"] is still supported in CSI driver and takes priority over other topology keys if specified. So we dont need to translate the same?

Fedosin · 2021-01-28T08:45:05Z

staging/src/k8s.io/csi-translation-lib/plugins/openstack_cinder_test.go

+		}
+
+		if !reflect.DeepEqual(got, tc.expSc) {
+			t.Errorf("Got parameters: %v, expected :%v", got, tc.expSc)


nit: typo

Suggested change

t.Errorf("Got parameters: %v, expected :%v", got, tc.expSc)

t.Errorf("Got parameters: %v, expected: %v", got, tc.expSc)

jsafrane · 2021-01-28T09:48:01Z

@ramineni, you're right, I removed availability translation and added explicit unit tests it's not translated.

Still, migration to CSI was a great opportunity to drop the hack we had in Kubernetes StorageClasses and stop supporting two conflicting ways how to place volumes into zones. I did not find a clear statement if the CSI driver is GA or beta or alpha, is it too late to remove availability support from there? (If it's at least beta, then the answer is yes, it's too late).

Fedosin · 2021-01-28T10:14:26Z

/test pull-kubernetes-e2e-kind-ipv6

Fedosin · 2021-01-28T10:30:42Z

/test pull-kubernetes-verify

mandre · 2021-01-28T10:30:47Z

@ramineni, you're right, I removed availability translation and added explicit unit tests it's not translated.

Can you update the PR description and commit message to reflect the change you just made?

ramineni · 2021-01-28T10:38:21Z

@ramineni, you're right, I removed availability translation and added explicit unit tests it's not translated.

Still, migration to CSI was a great opportunity to drop the hack we had in Kubernetes StorageClasses and stop supporting two conflicting ways how to place volumes into zones. I did not find a clear statement if the CSI driver is GA or beta or alpha, is it too late to remove availability support from there? (If it's at least beta, then the answer is yes, it's too late).

@jsafrane we did consider once before, but there was some issue (something with volume AZ , node AZ can be different). Dont remember exactly, I suppose, I can do some digging again if we could remove the redundancy. Thanks

jsafrane · 2021-01-28T10:50:39Z

Ok, this PR does not touch availability now. I updated the commit message with the last push. PTAL

ramineni · 2021-01-28T10:56:45Z

Thanks.
/lgtm

fejta-bot · 2021-01-28T14:01:06Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

In-tree Cinder storage class must be translated to CSI: - sc.params["fsType"] -> sc.params["csi.storage.k8s.io/fstype"] - sc.allowedTopology (with in-tree topology keys) -> sc.allowedTopology (with CSI topology keys)

jsafrane · 2021-01-28T15:22:11Z

Fixed gofmt

Fedosin

/lgtm

k8s-ci-robot · 2021-01-29T09:21:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Fedosin, jsafrane, msau42

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~staging/src/k8s.io/csi-translation-lib/OWNERS~~ [msau42]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

…98311-upstream-release-1.20 Automated cherry pick of #98311: Fix translation of Cinder storage classess to CSI

…98311-upstream-release-1.18 Automated cherry pick of #98311: Fix translation of Cinder storage classess to CSI

…98311-upstream-release-1.19 Automated cherry pick of #98311: Fix translation of Cinder storage classess to CSI

k8s-ci-robot requested review from davidz627 and leakingtapan January 22, 2021 14:39

jsafrane force-pushed the fix-cinder-topology-translation branch from 04450b7 to 1d34a35 Compare January 25, 2021 09:24

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 25, 2021

jsafrane force-pushed the fix-cinder-topology-translation branch from 1d34a35 to 0333480 Compare January 25, 2021 11:19

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jan 27, 2021

Fedosin reviewed Jan 28, 2021

View reviewed changes

jsafrane force-pushed the fix-cinder-topology-translation branch 2 times, most recently from 3caacfe to 4010c88 Compare January 28, 2021 09:50

jsafrane force-pushed the fix-cinder-topology-translation branch from 4010c88 to 1070cbe Compare January 28, 2021 10:49

k8s-ci-robot assigned ramineni Jan 28, 2021

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

Fix translation of Cinder storage classess to CSI

9032f5e

In-tree Cinder storage class must be translated to CSI: - sc.params["fsType"] -> sc.params["csi.storage.k8s.io/fstype"] - sc.allowedTopology (with in-tree topology keys) -> sc.allowedTopology (with CSI topology keys)

jsafrane force-pushed the fix-cinder-topology-translation branch from 1070cbe to 9032f5e Compare January 28, 2021 15:22

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 28, 2021

jsafrane mentioned this pull request Jan 29, 2021

Mark openstack/cinder csi migrations are on by default #98538

Merged

Fedosin approved these changes Jan 29, 2021

View reviewed changes

k8s-ci-robot assigned Fedosin Jan 29, 2021

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

k8s-ci-robot merged commit 5cd694b into kubernetes:master Jan 29, 2021

k8s-ci-robot added this to the v1.21 milestone Jan 29, 2021

chrigl mentioned this pull request Feb 2, 2021

[cinder-csi-plugin] Tracking CSIMigration with Topology Support kubernetes/cloud-provider-openstack#1397

Closed

github-actions bot mentioned this pull request Feb 3, 2021

Week Ending January 31, 2021 dev-obs/actus#339

Open

k8s-ci-robot added a commit that referenced this pull request Feb 4, 2021

Merge pull request #98608 from ialidzhikov/automated-cherry-pick-of-#…

542316f

…98311-upstream-release-1.20 Automated cherry pick of #98311: Fix translation of Cinder storage classess to CSI

k8s-ci-robot added a commit that referenced this pull request Feb 4, 2021

Merge pull request #98610 from ialidzhikov/automated-cherry-pick-of-#…

4608d7f

…98311-upstream-release-1.18 Automated cherry pick of #98311: Fix translation of Cinder storage classess to CSI

k8s-ci-robot added a commit that referenced this pull request Feb 4, 2021

Merge pull request #98609 from ialidzhikov/automated-cherry-pick-of-#…

4a94417

…98311-upstream-release-1.19 Automated cherry pick of #98311: Fix translation of Cinder storage classess to CSI

chrigl mentioned this pull request Feb 16, 2021

Make release with latest translation-lib kubernetes-csi/external-provisioner#563

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix translation of Cinder storage classess to CSI #98311

Fix translation of Cinder storage classess to CSI #98311

jsafrane commented Jan 22, 2021 •

edited

jsafrane commented Jan 22, 2021

jsafrane commented Jan 25, 2021

ialidzhikov commented Jan 25, 2021

jsafrane commented Jan 26, 2021

msau42 commented Jan 27, 2021

ramineni commented Jan 28, 2021

Fedosin Jan 28, 2021

jsafrane commented Jan 28, 2021

Fedosin commented Jan 28, 2021

Fedosin commented Jan 28, 2021

mandre commented Jan 28, 2021

ramineni commented Jan 28, 2021

jsafrane commented Jan 28, 2021

ramineni commented Jan 28, 2021

fejta-bot commented Jan 28, 2021

jsafrane commented Jan 28, 2021

Fedosin left a comment

k8s-ci-robot commented Jan 29, 2021

	t.Errorf("Got parameters: %v, expected :%v", got, tc.expSc)
	t.Errorf("Got parameters: %v, expected: %v", got, tc.expSc)

Fix translation of Cinder storage classess to CSI #98311

Fix translation of Cinder storage classess to CSI #98311

Conversation

jsafrane commented Jan 22, 2021 • edited

jsafrane commented Jan 22, 2021

jsafrane commented Jan 25, 2021

ialidzhikov commented Jan 25, 2021

jsafrane commented Jan 26, 2021

msau42 commented Jan 27, 2021

ramineni commented Jan 28, 2021

Fedosin Jan 28, 2021

Choose a reason for hiding this comment

jsafrane commented Jan 28, 2021

Fedosin commented Jan 28, 2021

Fedosin commented Jan 28, 2021

mandre commented Jan 28, 2021

ramineni commented Jan 28, 2021

jsafrane commented Jan 28, 2021

ramineni commented Jan 28, 2021

fejta-bot commented Jan 28, 2021

jsafrane commented Jan 28, 2021

Fedosin left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jan 29, 2021

jsafrane commented Jan 22, 2021 •

edited