Promote CSIPersistentVolume feature to GA #69929

jsafrane · 2018-10-17T12:26:31Z

What this PR does / why we need it:
We want CSIPersistentVolume GA in 1.13, kubernetes/enhancements#178

/milestone 1.13

CSI has many sub-features, this is only about the PersistentVolume part we consider stable enough. Inline volumes and block volumes and similar features have / will have their own alpha features and are / will be tracked separately.

Special notes for your reviewer:
Originally, I filled separate PRs for individual commits here, now I open this PR with everything on one place.

Release note:

CSIPersistentVolume feature, i.e. PersistentVolumes with CSIPersistentVolumeSource, is GA. 
CSIPersistentVolume feature gate is now deprecated and will be removed according to deprecation policy.

/sig storage
/sig api-machinery
cc @saad-ali, @liggitt

k8s-ci-robot · 2018-10-17T12:26:33Z

@jsafrane: You must be a member of the kubernetes/kubernetes-milestone-maintainers github team to set the milestone.

In response to this:

What this PR does / why we need it:
We want CSIPersistentVolume GA in 1.13, kubernetes/enhancements#178

/milestone 1.13

CSI has many sub-features, this is only about the PersistentVolume part we consider stable enough. Inline volumes and block volumes and similar features have / will have their own alpha features and are / will be tracked separately.

Special notes for your reviewer:
Originally, I filled separate PRs for individual commits here, now I open this PR with everything on one place.

Release note:
CSIPersistentVolume feature, i.e. PersistentVolumes with CSIPersistentVolumeSource, is GA. 
CSIPersistentVolume feature gate is now deprecated and will be removed according to deprecation policy.
/sig storage
/sig api-machinery
cc @saad-ali, @LiGgit

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 · 2018-10-17T12:28:37Z

cc @liggitt, I created one PR for all CSIPersistentVolume GA changes.

staging/src/k8s.io/api/storage/v1/types.go

kacole2 · 2018-10-17T13:30:39Z

/milestone v1.13

lavalamp · 2018-10-18T20:52:00Z

/remove-sig api-machinery

msau42 · 2018-10-29T17:47:09Z

/assign @msau42 @saad-ali

saad-ali

Approved from SIG Storage side.

/lgtm
/approve

msau42 · 2018-10-31T04:00:36Z

/hold

We should wait until we get the CSI 1.0 spec changes approved and merged before moving the feature to GA.

msau42 · 2018-10-31T04:01:19Z

(Let's continue to review so that everything is ready to go)

jsafrane · 2018-11-14T13:53:02Z

Added validation for spec.Attacher and spec.Source.PersistentVolumeName

pkg/apis/core/validation/validation.go

liggitt · 2018-11-14T14:51:42Z

pkg/apis/storage/validation/validation.go

@@ -163,6 +163,12 @@ func validateVolumeAttachmentSource(source *storage.VolumeAttachmentSource, fldP
 	if source.PersistentVolumeName == nil || len(*source.PersistentVolumeName) == 0 {
 		allErrs = append(allErrs, field.Required(fldPath, ""))
 	}
+	if source.PersistentVolumeName != nil {
+		for _, msg := range apivalidation.ValidatePersistentVolumeName(*source.PersistentVolumeName, false) {


same comment about only adding this validation in volumeAttachmentStrategy#Validate, and only when the api request is not coming from the v1beta1 API

I think this was fixed by commit below.

liggitt · 2018-11-14T14:52:10Z

needs a bazel update as well

msau42 · 2018-11-14T18:46:44Z

/retest

msau42 · 2018-11-14T18:50:50Z

pkg/apis/core/validation/validation.go

+	allErrs := field.ErrorList{}
+
+	if !utilfeature.DefaultFeatureGate.Enabled(features.CSIPersistentVolume) {
+		allErrs = append(allErrs, field.Forbidden(fldPath, "CSIPersistentVolume disabled by feature-gate"))


does this have the same issue where you can't delete the Pod object after downgrading to a version where the feature is disabled?

well, not the pod, since this is in PV, but the same issue, yes

pushed a commit to https://github.com/liggitt/kubernetes/commits/csi-ga to resolve this field for this object. I think that looks like what we'll want to do more broadly (consider the existing field on update, and not block in validation based on feature enablement, since it can break update/delete scenarios)

thanks. I will pull your commit and update this branch.

ddebroy · 2018-11-14T21:51:04Z

/retest

msau42 · 2018-11-15T00:01:17Z

/test pull-kubernetes-e2e-gce-100-performance

msau42 · 2018-11-15T02:00:53Z

This is ready for final review

liggitt · 2018-11-15T02:33:41Z

pkg/registry/storage/volumeattachment/storage/storage.go

+// StatusREST implements the REST endpoint for changing the status of a VolumeAttachment
+type StatusREST struct {
+	store *genericregistry.Store
+}


follow-up: please add a type assertion to ensure this implements enough to patch status:

var _ = rest.Patcher(&StatusREST{})

liggitt · 2018-11-15T02:47:07Z

/lgtm
/approve

k8s-ci-robot · 2018-11-15T02:47:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jsafrane, liggitt, saad-ali

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:

~~api/OWNERS~~ [liggitt]
~~docs/api-reference/OWNERS~~ [liggitt]
~~pkg/api/OWNERS~~ [liggitt]
~~pkg/apis/OWNERS~~ [liggitt]
~~pkg/features/OWNERS~~ [liggitt,saad-ali]
~~pkg/registry/OWNERS~~ [liggitt]
~~pkg/volume/csi/OWNERS~~ [jsafrane,saad-ali]
~~staging/src/k8s.io/api/OWNERS~~ [liggitt]
~~staging/src/k8s.io/client-go/OWNERS~~ [liggitt]
~~test/integration/etcd/OWNERS~~ [liggitt,saad-ali]

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

liggitt · 2018-11-15T02:48:23Z

staging/src/k8s.io/api/storage/v1/generated.proto

+  optional k8s.io.apimachinery.pkg.apis.meta.v1.Time time = 1;
+
+  // String detailing the error encountered during Attach or Detach operation.
+  // This string maybe logged, so it should not contain sensitive


follow-up: s/maybe/may be/ (in existing versions as well)

liggitt · 2018-11-15T02:49:51Z

please open a PR with the follow-ups so we don't forget, even if it sits until 1.14 opens:

gnufied · 2018-11-15T03:05:18Z

@liggitt github issue for follow up items - #71056 . I have assigned it to both myself and @jsafrane . Whoever gets to it first.

k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Oct 17, 2018

k8s-ci-robot requested review from deads2k and gmarek October 17, 2018 12:27

This was referenced Oct 17, 2018

Promote VolumeAttachment to GA #69688

Closed

Promote CSIPersistentVolumeSource to GA #69690

Closed

liggitt reviewed Oct 17, 2018

View reviewed changes

staging/src/k8s.io/api/storage/v1/types.go Show resolved Hide resolved

k8s-ci-robot added this to the v1.13 milestone Oct 17, 2018

jsafrane force-pushed the csi-ga branch from 3e7f5d2 to 0c7adf2 Compare October 18, 2018 07:47

k8s-ci-robot removed the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Oct 18, 2018

k8s-ci-robot assigned msau42 and saad-ali Oct 29, 2018

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

saad-ali approved these changes Oct 29, 2018

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 31, 2018

Add volumeattachment status subresource

498cd61

jsafrane force-pushed the csi-ga branch from ab6d0af to ceb1664 Compare November 14, 2018 13:52

liggitt reviewed Nov 14, 2018

View reviewed changes

pkg/apis/core/validation/validation.go Show resolved Hide resolved

liggitt reviewed Nov 14, 2018

View reviewed changes

Tighten up validation of VolumeAttachment fields

8cfce0a

jsafrane force-pushed the csi-ga branch 2 times, most recently from d784ba1 to 8cfce0a Compare November 14, 2018 16:41

msau42 reviewed Nov 14, 2018

View reviewed changes

Tolerate existing CSI fields in PVs when the feature is disabled

eaff4fd

gnufied force-pushed the csi-ga branch from b5e5a39 to eaff4fd Compare November 14, 2018 22:58

gnufied mentioned this pull request Nov 15, 2018

Extract common label/annotation reset functions while setting status into a common utility #71056

Closed

liggitt reviewed Nov 15, 2018

View reviewed changes

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

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

liggitt reviewed Nov 15, 2018

View reviewed changes

k8s-ci-robot merged commit 726c07e into kubernetes:master Nov 15, 2018

This was referenced Nov 19, 2018

Refactor status PrepareForUpdate into standalone method #71211

Merged

CSI v1 leftovers #71217

Merged

liggitt mentioned this pull request Apr 7, 2019

Unit test failure: k8s.io/kubernetes/pkg/registry/storage/csidriver/storage.TestUpdate #76236

Closed

cwdsuzhou mentioned this pull request Jan 4, 2020

Patch status of volumeAttachment(StorageV1) does not work #86835

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Promote CSIPersistentVolume feature to GA #69929

Promote CSIPersistentVolume feature to GA #69929

jsafrane commented Oct 17, 2018 •

edited

k8s-ci-robot commented Oct 17, 2018

jsafrane commented Oct 17, 2018

kacole2 commented Oct 17, 2018

lavalamp commented Oct 18, 2018

msau42 commented Oct 29, 2018

saad-ali left a comment

msau42 commented Oct 31, 2018

msau42 commented Oct 31, 2018

jsafrane commented Nov 14, 2018

liggitt Nov 14, 2018

gnufied Nov 14, 2018

liggitt commented Nov 14, 2018

msau42 commented Nov 14, 2018

msau42 Nov 14, 2018

liggitt Nov 14, 2018

liggitt Nov 14, 2018

liggitt Nov 14, 2018

gnufied Nov 14, 2018

ddebroy commented Nov 14, 2018

msau42 commented Nov 15, 2018

msau42 commented Nov 15, 2018

liggitt Nov 15, 2018

liggitt commented Nov 15, 2018

k8s-ci-robot commented Nov 15, 2018

liggitt Nov 15, 2018

liggitt commented Nov 15, 2018

gnufied commented Nov 15, 2018

Promote CSIPersistentVolume feature to GA #69929

Promote CSIPersistentVolume feature to GA #69929

Conversation

jsafrane commented Oct 17, 2018 • edited

k8s-ci-robot commented Oct 17, 2018

jsafrane commented Oct 17, 2018

kacole2 commented Oct 17, 2018

lavalamp commented Oct 18, 2018

msau42 commented Oct 29, 2018

saad-ali left a comment

Choose a reason for hiding this comment

msau42 commented Oct 31, 2018

msau42 commented Oct 31, 2018

jsafrane commented Nov 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented Nov 14, 2018

msau42 commented Nov 14, 2018

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

ddebroy commented Nov 14, 2018

msau42 commented Nov 15, 2018

msau42 commented Nov 15, 2018

Choose a reason for hiding this comment

liggitt commented Nov 15, 2018

k8s-ci-robot commented Nov 15, 2018

Choose a reason for hiding this comment

liggitt commented Nov 15, 2018

gnufied commented Nov 15, 2018

jsafrane commented Oct 17, 2018 •

edited