Kubernetes CSI - Persistent Volume Source Type #55204

vladimirvivien · 2017-11-07T03:37:46Z

What this PR does / why we need it:
This PR is to track the addition of new API type CSIPersistentVolumeSource that will be used as PersistentVolume for storage sources managed by CSI drivers.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
xref kubernetes/enhancements#178

Special notes for your reviewer:

Implements API PersistentVolume type CSIPersistentVolumeSource
Part of implementation for Out-of-tree CSI Volume Plugins enhancements#178
Designed at CSI Volume Plugins in Kubernetes Design Doc community#1258

Other CSI Volume Plugin PRs:

Plugin Mounter/Unmounter Kubernetes CSI - in-tree Plugin Implementation #54529
Plugin Attacher/Detacher Kubernetes CSI - in-tree plugin Attacher/Detacher API #55809

Release note:

NONE

dixudx

@vladimirvivien I didn't see MountInterface was implemented for csi in this PR.

@jsafrane @rootfs PTAL.

dixudx · 2017-11-07T09:56:35Z

pkg/api/types.go

+
+	// VolumeHandle is the unique volume name returned by the CSI volume
+	// plugin’s CreateVolume to refer to the volume on all subsequent calls.
+	VolumeHandle string


Add // Required for VolumeHandle.

@dixudx will add // Required

dixudx · 2017-11-07T10:00:02Z

pkg/api/validation/validation.go

+	if len(csi.VolumeHandle) == 0 {
+		allErrs = append(allErrs, field.Required(fldPath.Child("volumeHandle"), ""))
+	}
+


Also need to validate MountSecretRef and AttachSecretRef if they are not nil.

Like,

if csi.MountSecretRef != nil { if len(csi.MountSecretRef.Name) == 0 { allErrs = append(allErrs, field.Required(fldPath.Child("MountSecretRef", "name"), "")) } ... }

@dixudx ok good catch. Will look into this as well. Thanks.

@dixudx I dbl checked, MountSecretRef is no longer part of the API type.

vladimirvivien · 2017-11-07T16:18:01Z

@dixudx thanks for the review. What do you mean by MountInterface? (Are you referring to the volume.Mounter interface ?) Please point me where I can do it. Thank you!

I am also working on #54529 which is the actual volume plugin impl for CSI.

jsafrane · 2017-11-08T15:46:01Z

/ok-to-test

jsafrane · 2017-11-08T15:46:33Z

cmd/kube-controller-manager/app/plugins.go

@@ -105,6 +107,7 @@ func ProbeExpandableVolumePlugins(config componentconfig.VolumeConfiguration) []
 	allPlugins = append(allPlugins, scaleio.ProbeVolumePlugins()...)
 	allPlugins = append(allPlugins, storageos.ProbeVolumePlugins()...)
 	allPlugins = append(allPlugins, fc.ProbeVolumePlugins()...)
+	allPlugins = append(allPlugins, fc.ProbeVolumePlugins()...)


jsafrane · 2017-11-08T15:48:22Z

pkg/api/validation/validation_test.go

+			errfield: "driver",
+		},
+		{
+			name:     "missing volume handle",


some successful test would be nice here, both of the current ones are error cases.

added couple of successful tests.

jsafrane · 2017-11-08T15:49:22Z

pkg/printers/internalversion/describe.go

@@ -1073,6 +1073,14 @@ func printFlockerVolumeSource(flocker *api.FlockerVolumeSource, w PrefixWriter)
 		flocker.DatasetName, flocker.DatasetUUID)
 }

+func printCSIPersistentVolumeSource(csi *api.CSIPersistentVolumeSource, w PrefixWriter) {
+	w.Write(LEVEL_2, "Type:\tCSIVolume (a generic volume resource that represents a volume managed by an external CSI plugin)\n"+


CSI -> Container Storage Interface? At least for now, before it gets well-known.

Added full CSI name

jsafrane · 2017-11-08T15:49:46Z

pkg/api/types.go

@@ -391,6 +391,9 @@ type PersistentVolumeSource struct {
 	// More info: https://releases.k8s.io/HEAD/examples/volumes/storageos/README.md
 	// +optional
 	StorageOS *StorageOSPersistentVolumeSource
+	// CSI represents storage that handled by an external CSI driver


CSI -> Container Storage Interface?

Agreed. Added full name.

jsafrane · 2017-11-13T18:06:34Z

pkg/api/types.go

+	// This may be empty if no secret is required. If the secret object contains
+	// more than one secret, all secrets are passed.
+	// +optional
+	MountSecretRef *SecretReference


did we remove secret refs in the proposal?

Yes. Master branch does not have it : https://github.com/kubernetes/community/blob/master/contributors/design-proposals/storage/container-storage-interface.md#proposed-api

vladimirvivien · 2017-11-14T10:14:49Z

/test pull-kubernetes-verify

vladimirvivien · 2017-11-14T10:56:12Z

/test pull-kubernetes-unit

vladimirvivien · 2017-11-14T12:17:34Z

/test pull-kubernetes-unit

jsafrane · 2017-11-14T13:07:40Z

pkg/apis/core/types.go

+	// This may be empty if no secret is required. If the secret object contains
+	// more than one secret, all secrets are passed.
+	// +optional
+	MountSecretRef *SecretReference


Secrets were postponed to future release, please remove them.

Hmm, github still shows that MountSecretRef is present in the first commit and at the same time it shows this review remark has been resolved / relevant code has been modified.

Oh, I see, you put it into the commit with generated code. Sorry for the noise.

Still, IMO it should be squashed into the first commit for further review. Nobody looks at the generated stuff.

@jsafrane ah ok, that commit slipped in to the generated stuff. I will fix that. Thanks.

jsafrane · 2017-11-15T13:07:12Z

lgtm from sig-storage point of view

/assign @thockin @liggitt
for API review

saad-ali · 2017-11-16T00:28:09Z

/lgtm
/approve

@thockin for API review.

saad-ali · 2017-11-16T18:28:08Z

/lgtm
/approve

jsafrane · 2017-11-17T09:31:28Z

@thockin, can you please approve? It's blocking subsequent PRs.

saad-ali · 2017-11-17T19:03:21Z

pkg/apis/core/validation/validation.go

@@ -1318,6 +1318,19 @@ func validateStorageOSPersistentVolumeSource(storageos *core.StorageOSPersistent
 	return allErrs
 }

+func validateCSIPersistentVolumeSource(csi *core.CSIPersistentVolumeSource, fldPath *field.Path) field.ErrorList {
+	allErrs := field.ErrorList{}


Please add a feature gate check:

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

This commit tracks source code update to support the CSI volume source type additionn.

This commit tracks all auto-generated sources.

saad-ali · 2017-11-18T00:23:14Z

/lgtm
/approve

thockin · 2017-11-18T00:35:05Z

/approve

k8s-github-robot · 2017-11-18T00:35:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: saad-ali, thockin, vladimirvivien

Associated issue: 178

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

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

~~api/OWNERS~~ [thockin]
~~docs/api-reference/OWNERS~~ [thockin]
~~pkg/OWNERS~~ [thockin]
~~staging/src/k8s.io/api/OWNERS~~ [thockin]

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

k8s-github-robot · 2017-11-18T00:43:52Z

[MILESTONENOTIFIER] Milestone Pull Request Needs Approval

@erictune @liggitt @saad-ali @thockin @vladimirvivien @yujuhong @kubernetes/sig-storage-misc

Action required: This pull request must have the status/approved-for-milestone label applied by a SIG maintainer. If the label is not applied within 6 days, the pull request will be moved out of the v1.9 milestone.

Pull Request Labels

sig/storage: Pull Request will be escalated to these SIGs if needed.
priority/important-soon: Escalate to the pull request owners and SIG owner; move out of milestone after several unsuccessful escalation attempts.
kind/feature: New functionality.

Help

saad-ali · 2017-11-18T00:52:06Z

/test pull-kubernetes-node-e2e

saad-ali · 2017-11-18T01:35:08Z

/test pull-kubernetes-node-e2e

k8s-github-robot · 2017-11-18T05:37:42Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2017-11-18T06:18:29Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

Automatic merge from submit-queue (batch tested with PRs 54529, 53765). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Kubernetes CSI - in-tree Plugin Implementation **What this PR does / why we need it**: This PR is part of the internal Kubernetes CSI Volume plugin. It implements the Attach/Detach/Mount/Unmount API. **Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: kubernetes/enhancements#178 **Special notes for your reviewer**: - Implements feature kubernetes/enhancements#178 - Designed kubernetes/community#1258 Other CSI Volume plugin PRs - CSI Persistent Volume Source - #55204 **Release note**: ```release-note NONE ```

k8s-github-robot assigned yujuhong and erictune Nov 7, 2017

k8s-github-robot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Nov 7, 2017

dixudx reviewed Nov 7, 2017

View reviewed changes

vladimirvivien changed the title ~~K8s csi volume source~~ Kubernetes CSI - Adding Persistent Volume Source Type Nov 7, 2017

vladimirvivien changed the title ~~Kubernetes CSI - Adding Persistent Volume Source Type~~ Kubernetes CSI - Persistent Volume Source Type Nov 7, 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 Nov 8, 2017

dixudx mentioned this pull request Nov 9, 2017

Kubernetes CSI - in-tree Plugin Implementation #54529

Merged

jsafrane reviewed Nov 9, 2017

View reviewed changes

clintkitson mentioned this pull request Nov 9, 2017

Kubernetes CSI implementation thecodeteam/roadmap#200

Closed

6 tasks

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

jsafrane reviewed Nov 13, 2017

View reviewed changes

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

jsafrane reviewed Nov 14, 2017

View reviewed changes

k8s-ci-robot assigned liggitt and thockin Nov 15, 2017

vladimirvivien mentioned this pull request Nov 15, 2017

Kubernetes CSI - in-tree plugin Attacher/Detacher API #55809

Closed

k8s-ci-robot assigned saad-ali Nov 16, 2017

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

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 16, 2017

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

saad-ali reviewed Nov 17, 2017

View reviewed changes

vladimirvivien added 2 commits November 17, 2017 19:17

CSI - API change for CSI volume source type

af0c2bd

This commit tracks source code update to support the CSI volume source type additionn.

CSI - Generated source code

c4a1730

This commit tracks all auto-generated sources.

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 18, 2017

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

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

saad-ali added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Nov 18, 2017

saad-ali added this to the v1.9 milestone Nov 18, 2017

k8s-github-robot added the milestone/incomplete-labels label Nov 18, 2017

saad-ali added kind/feature Categorizes issue or PR as related to a new feature. sig/storage Categorizes an issue or PR as relevant to SIG Storage. labels Nov 18, 2017

k8s-github-robot added milestone/needs-approval and removed milestone/incomplete-labels labels Nov 18, 2017

k8s-github-robot merged commit 928c85f into kubernetes:master Nov 18, 2017

Kubernetes CSI - Persistent Volume Source Type #55204

Kubernetes CSI - Persistent Volume Source Type #55204

Conversation

vladimirvivien commented Nov 7, 2017 • edited by thockin

dixudx left a comment • edited

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

vladimirvivien commented Nov 7, 2017 • edited

jsafrane commented Nov 8, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladimirvivien commented Nov 14, 2017

vladimirvivien commented Nov 14, 2017

vladimirvivien commented Nov 14, 2017

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

jsafrane commented Nov 15, 2017

saad-ali commented Nov 16, 2017

saad-ali commented Nov 16, 2017

jsafrane commented Nov 17, 2017

Choose a reason for hiding this comment

saad-ali commented Nov 18, 2017

thockin commented Nov 18, 2017

k8s-github-robot commented Nov 18, 2017

k8s-github-robot commented Nov 18, 2017

saad-ali commented Nov 18, 2017

saad-ali commented Nov 18, 2017

k8s-github-robot commented Nov 18, 2017

k8s-github-robot commented Nov 18, 2017

vladimirvivien commented Nov 7, 2017 •

edited by thockin

dixudx left a comment •

edited

vladimirvivien commented Nov 7, 2017 •

edited