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

Promote CSIPersistentVolume feature to GA #69929

Merged
merged 7 commits into from Nov 15, 2018

Conversation

@jsafrane
Copy link
Member

jsafrane commented Oct 17, 2018

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

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Oct 17, 2018

@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

This comment has been minimized.

Copy link
Member

jsafrane commented Oct 17, 2018

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

@kacole2

This comment has been minimized.

Copy link

kacole2 commented Oct 17, 2018

/milestone v1.13

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

@jsafrane jsafrane force-pushed the jsafrane:csi-ga branch from 3e7f5d2 to 0c7adf2 Oct 18, 2018

@lavalamp

This comment has been minimized.

Copy link
Member

lavalamp commented Oct 18, 2018

/remove-sig api-machinery

@msau42

This comment has been minimized.

Copy link
Member

msau42 commented Oct 29, 2018

/assign @msau42 @saad-ali

@saad-ali
Copy link
Member

saad-ali left a comment

Approved from SIG Storage side.

/lgtm
/approve

@msau42

This comment has been minimized.

Copy link
Member

msau42 commented Oct 31, 2018

/hold

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

@msau42

This comment has been minimized.

Copy link
Member

msau42 commented Oct 31, 2018

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

jsafrane added some commits Nov 14, 2018

@jsafrane jsafrane force-pushed the jsafrane:csi-ga branch from ab6d0af to ceb1664 Nov 14, 2018

@jsafrane

This comment has been minimized.

Copy link
Member

jsafrane commented Nov 14, 2018

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

@@ -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) {

This comment has been minimized.

@liggitt

liggitt Nov 14, 2018

Member

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

This comment has been minimized.

@gnufied

gnufied Nov 14, 2018

Member

I think this was fixed by commit below.

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Nov 14, 2018

needs a bazel update as well

@jsafrane jsafrane force-pushed the jsafrane:csi-ga branch 2 times, most recently from d784ba1 to 8cfce0a Nov 14, 2018

@msau42

This comment has been minimized.

Copy link
Member

msau42 commented Nov 14, 2018

/retest

allErrs := field.ErrorList{}

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

This comment has been minimized.

@msau42

msau42 Nov 14, 2018

Member

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

This comment has been minimized.

@liggitt

This comment has been minimized.

@liggitt

liggitt Nov 14, 2018

Member

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

This comment has been minimized.

@liggitt

liggitt Nov 14, 2018

Member

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)

This comment has been minimized.

@gnufied

gnufied Nov 14, 2018

Member

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

@ddebroy

This comment has been minimized.

Copy link
Member

ddebroy commented Nov 14, 2018

/retest

@gnufied gnufied force-pushed the jsafrane:csi-ga branch from b5e5a39 to eaff4fd Nov 14, 2018

@msau42

This comment has been minimized.

Copy link
Member

msau42 commented Nov 15, 2018

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

@msau42

This comment has been minimized.

Copy link
Member

msau42 commented Nov 15, 2018

This is ready for final review

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

This comment has been minimized.

@liggitt

liggitt Nov 15, 2018

Member

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

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

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Nov 15, 2018

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm label Nov 15, 2018

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Nov 15, 2018

[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:

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

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

This comment has been minimized.

@liggitt

liggitt Nov 15, 2018

Member

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

@gnufied

This comment has been minimized.

Copy link
Member

gnufied commented Nov 15, 2018

@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 k8s-ci-robot merged commit 726c07e into kubernetes:master Nov 15, 2018

18 checks passed

cla/linuxfoundation liggitt authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-e2e-kubeadm-gce Skipped
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
tide In merge pool.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment