Pass VolumeAttributesClass Parameters as MutableParameters to CreateVolume #1068

ConnorJC3 · 2023-10-26T19:54:02Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Implements the external-provisioner component of https://kep.k8s.io/3751

Which issue(s) this PR fixes:

Fixes #952

Special notes for your reviewer:

There are several places where I was unsure of the best action, marked with "TODO" comments, please answer those if you have a better answer than I do.

Breakdown by commit:

Update to go1.21
Required because the Kubernetes dependency that will be upgrading in the following commit references the go1.21-only slices library.
[TODO FIX AFTER PRS MERGE] Bump CSI spec, csi-test, and Kubernetes dependencies
- Upgrades all k8s dependencies directly in k/k to the versions from [KEP-3751] introduce the VolumeAttributesClass API kubernetes/kubernetes#121104
- Upgrades csi-test to the version from Update mock for ControllerModifyVolume csi-test#492
- Upgrades the CSI spec to v1.9.0
Add VolumeAttributesClass feature flag
Adds a default-off VAC feature flag.
Pass VolumeAttributesClass Parameters as MutableParameters to CreateVolume
The meat of the PR: Adds the funcionality for VACs, as well as associated unit tests.

Does this PR introduce a user-facing change?:

Pass VolumeAttributesClass Parameters as MutableParameters to CreateVolume

ConnorJC3 · 2023-10-26T19:56:48Z

/assign @sunnylovestiramisu @msau42

ConnorJC3 · 2023-10-26T20:57:27Z

Chasing down CI failures, but I believe they are largely (if not entirely) false positives caused by issues with the go version - this PR should be reasonably safe to review now, I don't anticipate any large or architectural changes. make test passes locally for me.

sunnylovestiramisu · 2023-10-26T22:50:46Z

pkg/controller/controller.go

@@ -596,6 +604,14 @@ func (p *csiProvisioner) prepareProvision(ctx context.Context, claim *v1.Persist
 		}
 	}

+	vacName := claim.Spec.VolumeAttributesClassName
+	// TODO: For reviewers - Should we reject volumes that have a VAC specified if the VAC feature gate is disabled?


The csi spec states that "A Controller plugin MUST implement this RPC call if the plugin has the MODIFY_VOLUME controller capability. "

To consider backward compatibility I do not think we should reject the call. Maybe if there is a way to send a warning to users indicate that the VAC is not applied because of feature gate etc.

Why would it not be backwards compatible? A user would have to opt-in to specifying the VAC in their PVC.

What if they downgrade their cluster and turn off the vac feature but the VAC is applied?

Kubernetes doesn't support downgrades, although I'm not 100% sure what would happen if you disabled the API Server feature gate on a cluster with VACs already in it

Kubernetes supports downgrades.

However, even with the help of the folks on #sig-architecture the best link that I could find was only the underlying design for API storage: https://kubernetes.io/docs/reference/using-api/deprecation-policy/

Maybe "doesn't support" is too strong of wording, but Kubernetes does not document any support for downgrading and it is widely considered unsafe: kubernetes/website#12327

That said, I'm not sure it really matters for the purpose of this question: My question is basically, if the volume does specify a VolumeAttributeClassName (i.e. the feature flag is enabled at the API level and the user is using it) but the local external-provisioner feature flag is disabled, what should we do?

Should we either (1) provision the volume as normal - ignoring the fact it has a VAC or (2) refuse to provision the volume and demand the user turns on the VAC feature flag or remove the VAC from the volume?

KEP says that "API server gets requests of modify parameters in VolumeAttributesClass but does not act on it", which I read as the external-provisioner / resizer should not act on it and thus ignore vacName.

Perhaps a little refactoring could help:

var vacName string if utilfeature.DefaultFeatureGate.Enabled(features.VolumeAttributesClass) { if claim.Spec.VolumeAttributesClassName != nil { vacName = *claim.Spec.VolumeAttributesClassName } } if vacName != "" { ... }

Much better than my version, I stole it wholesale for the next revision

pkg/features/features.go

pkg/controller/controller.go

carlory · 2023-11-01T01:32:56Z

@ConnorJC3 API changes is merged now, please use upstream kubernetes repo instead.

ConnorJC3 · 2023-11-06T19:15:43Z

Bumped to use k8s 1.29.0-alpha.3 version. CI still broken because it uses go 1.20

pkg/controller/controller.go

ConnorJC3 · 2023-11-13T16:32:52Z

Rebased on latest master branch as well as incorporated the most recent feedback

jsafrane · 2023-11-14T10:12:27Z

Makefile

+# TODO: Change in csi-release-tools before PR merge?
+export CSI_PROW_GO_VERSION_BUILD=1.21


Yes, please send a PR for csi-release-tools. All sidecar releases for 1.29 will need a new go.

Working on a PR, for now I modified the directory directly to hopefully unblock CI, although this will cause make test to fail because it detects non-upstreamed changes

pkg/features/features.go

jsafrane · 2023-11-14T10:36:21Z

pkg/controller/controller.go

@@ -596,6 +604,14 @@ func (p *csiProvisioner) prepareProvision(ctx context.Context, claim *v1.Persist
 		}
 	}

+	vacName := claim.Spec.VolumeAttributesClassName
+	// TODO: For reviewers - Should we reject volumes that have a VAC specified if the VAC feature gate is disabled?


KEP says that "API server gets requests of modify parameters in VolumeAttributesClass but does not act on it", which I read as the external-provisioner / resizer should not act on it and thus ignore vacName.

Perhaps a little refactoring could help:

var vacName string if utilfeature.DefaultFeatureGate.Enabled(features.VolumeAttributesClass) { if claim.Spec.VolumeAttributesClassName != nil { vacName = *claim.Spec.VolumeAttributesClassName } } if vacName != "" { ... }

pkg/controller/controller.go

go.mod

pkg/controller/controller_test.go

torredil · 2023-11-16T18:20:35Z

/lgtm

k8s-ci-robot · 2023-11-16T18:20:37Z

@torredil: changing LGTM is restricted to collaborators

In response to this:

/lgtm

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 · 2023-11-20T11:03:22Z

/approve
LGTM-ish except for the release-tools hack.

k8s-ci-robot · 2023-11-20T11:04:10Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

~~OWNERS~~ [jsafrane]

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

ConnorJC3 · 2023-11-20T18:45:44Z

csi-release-tools PR here: kubernetes-csi/csi-release-tools#238

jsafrane · 2023-11-22T12:39:12Z

release-tools have go 1.21 now

Signed-off-by: Connor Catlett <conncatl@amazon.com>

…olume Signed-off-by: Connor Catlett <conncatl@amazon.com>

jsafrane · 2023-11-30T07:37:29Z

/lgtm

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. labels Oct 26, 2023

k8s-ci-robot requested review from ggriffiths and humblec October 26, 2023 19:54

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 26, 2023

ConnorJC3 force-pushed the kep3751 branch from e72843d to fdea7b0 Compare October 26, 2023 19:56

k8s-ci-robot assigned msau42 and sunnylovestiramisu Oct 26, 2023

sunnylovestiramisu reviewed Oct 26, 2023

View reviewed changes

pkg/features/features.go Show resolved Hide resolved

sunnylovestiramisu reviewed Oct 26, 2023

View reviewed changes

pkg/controller/controller.go Show resolved Hide resolved

ConnorJC3 force-pushed the kep3751 branch 2 times, most recently from 3deff1e to e467de1 Compare October 30, 2023 15:25

ConnorJC3 force-pushed the kep3751 branch from e467de1 to 1bb375e Compare November 6, 2023 19:04

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 8, 2023

carlory reviewed Nov 8, 2023

View reviewed changes

pkg/controller/controller.go Outdated Show resolved Hide resolved

carlory reviewed Nov 8, 2023

View reviewed changes

pkg/controller/controller.go Outdated Show resolved Hide resolved

carlory reviewed Nov 8, 2023

View reviewed changes

pkg/controller/controller.go Show resolved Hide resolved

pacoxu mentioned this pull request Nov 10, 2023

Kubernetes VolumeAttributesClass ModifyVolume kubernetes/enhancements#3751

Open

20 tasks

ConnorJC3 force-pushed the kep3751 branch from 1bb375e to b827fcc Compare November 13, 2023 16:30

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 13, 2023

jsafrane reviewed Nov 14, 2023

View reviewed changes

go.mod Outdated Show resolved Hide resolved

jsafrane reviewed Nov 14, 2023

View reviewed changes

pkg/controller/controller_test.go Show resolved Hide resolved

ConnorJC3 force-pushed the kep3751 branch from b827fcc to a9d5c82 Compare November 16, 2023 17:31

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 20, 2023

ConnorJC3 mentioned this pull request Nov 20, 2023

Bump go build version to 1.21 kubernetes-csi/csi-release-tools#238

Closed

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 22, 2023

ConnorJC3 force-pushed the kep3751 branch from a9d5c82 to b83288b Compare November 29, 2023 16:22

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 29, 2023

ConnorJC3 added 4 commits November 29, 2023 19:02

Update to go1.21

29a1f5c

Signed-off-by: Connor Catlett <conncatl@amazon.com>

Bump CSI spec, csi-test, and Kubernetes dependencies

6e12110

Signed-off-by: Connor Catlett <conncatl@amazon.com>

Add VolumeAttributesClass feature flag

eb14cdb

Signed-off-by: Connor Catlett <conncatl@amazon.com>

Pass VolumeAttributesClass Parameters as MutableParameters to CreateV…

cf60a4b

…olume Signed-off-by: Connor Catlett <conncatl@amazon.com>

ConnorJC3 force-pushed the kep3751 branch from b83288b to cf60a4b Compare November 29, 2023 19:02

pacoxu mentioned this pull request Nov 30, 2023

iops limit for pod/pvc/pv kubernetes/kubernetes#92287

Closed

k8s-ci-robot assigned jsafrane Nov 30, 2023

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

k8s-ci-robot merged commit b022523 into kubernetes-csi:master Nov 30, 2023

mpatlasov mentioned this pull request Mar 21, 2024

STOR-1573: Rebase external-provisioner to v4.0.1 for OCP 4.16 openshift/csi-external-provisioner#95

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass VolumeAttributesClass Parameters as MutableParameters to CreateVolume #1068

Pass VolumeAttributesClass Parameters as MutableParameters to CreateVolume #1068

ConnorJC3 commented Oct 26, 2023 •

edited by sunnylovestiramisu

Loading

ConnorJC3 commented Oct 26, 2023

ConnorJC3 commented Oct 26, 2023 •

edited

Loading

sunnylovestiramisu Oct 26, 2023

msau42 Oct 27, 2023

sunnylovestiramisu Oct 27, 2023

ConnorJC3 Oct 27, 2023

pohly Oct 27, 2023

ConnorJC3 Oct 30, 2023

jsafrane Nov 14, 2023

ConnorJC3 Nov 16, 2023

carlory commented Nov 1, 2023

ConnorJC3 commented Nov 6, 2023

ConnorJC3 commented Nov 13, 2023

jsafrane Nov 14, 2023

ConnorJC3 Nov 16, 2023

jsafrane Nov 14, 2023

torredil commented Nov 16, 2023

k8s-ci-robot commented Nov 16, 2023

jsafrane commented Nov 20, 2023 •

edited

Loading

k8s-ci-robot commented Nov 20, 2023

ConnorJC3 commented Nov 20, 2023

jsafrane commented Nov 22, 2023

jsafrane commented Nov 30, 2023

		# TODO: Change in csi-release-tools before PR merge?
		export CSI_PROW_GO_VERSION_BUILD=1.21

Pass VolumeAttributesClass Parameters as MutableParameters to CreateVolume #1068

Pass VolumeAttributesClass Parameters as MutableParameters to CreateVolume #1068

Conversation

ConnorJC3 commented Oct 26, 2023 • edited by sunnylovestiramisu Loading

ConnorJC3 commented Oct 26, 2023

ConnorJC3 commented Oct 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlory commented Nov 1, 2023

ConnorJC3 commented Nov 6, 2023

ConnorJC3 commented Nov 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

torredil commented Nov 16, 2023

k8s-ci-robot commented Nov 16, 2023

jsafrane commented Nov 20, 2023 • edited Loading

k8s-ci-robot commented Nov 20, 2023

ConnorJC3 commented Nov 20, 2023

jsafrane commented Nov 22, 2023

jsafrane commented Nov 30, 2023

ConnorJC3 commented Oct 26, 2023 •

edited by sunnylovestiramisu

Loading

ConnorJC3 commented Oct 26, 2023 •

edited

Loading

jsafrane commented Nov 20, 2023 •

edited

Loading