CSI - API Changes for Pod Inline Volume Support #67452

vladimirvivien · 2018-08-15T19:31:55Z

What this PR does / why we need it:
This PR introduces the changes needed in the API to support inline volume for pods using CSI drivers. Detail can be found in feature kubernetes/enhancements#596

CSI support for inline ephemeral volumes that can be specified in pod specs.

⚠️ The will no longer track the implementation changes for CSI Inline Volume. Its content has been combined with PR #68232 (this PR has cherry-picked there). Follow #68232 for all future changes. ⚠️

/sig storage

thockin · 2018-08-28T03:22:19Z

title says WIP - is it still WIP?

thockin

Small things

thockin · 2018-08-28T03:23:54Z

pkg/apis/core/types.go

+	// Optional: The value to pass to ControllerPublishVolumeRequest.
+	// Defaults to false (read/write).
+	// +optional
+	ReadOnly bool


All optional fields should be pointers or nil-able types (maps, slices)

OK thanks, will change.

thockin · 2018-08-28T03:24:40Z

pkg/apis/core/types.go

+	// +optional
+	FSType string
+
+	// Attributes of the volume to publish.


Can you expand this? What is done with them?

Will update the comment.

thockin · 2018-08-28T03:25:03Z

pkg/apis/core/types.go

+	// Filesystem type to mount.
+	// Must be a filesystem type supported by the host operating system.
+	// Ex. "ext4", "xfs", "ntfs".
+	// +optional


"Default is auto-detect. If not formatted the FS will be chosen by the node." ?

"Default is auto-detect. If not formatted the FS will be chosen by the node."

I see this quoted, Where is this from ?
Also, for CSI, fstype is passed as-is delegating its interpretation to the CSI driver.

I meant you need to document what the default is when a user doesn't specify, and that the default shouldn't be defined as a particular FS but as "the system will choose for you".

thockin · 2018-08-28T03:27:02Z

pkg/apis/core/types.go

+type CSIVolumeSource struct {
+	// Driver is the name of the driver to use for this volume.
+	// Required.
+	Driver string


do we need to add quota (ok as followup) on this?

Quota is not currently part of CSI but can certainly be a follow up item.

vladimirvivien

Replies to Tim comments.

vladimirvivien · 2018-08-28T15:14:37Z

pkg/apis/core/types.go

+type CSIVolumeSource struct {
+	// Driver is the name of the driver to use for this volume.
+	// Required.
+	Driver string


Quota is not currently part of CSI but can certainly be a follow up item.

vladimirvivien · 2018-08-28T15:15:25Z

pkg/apis/core/types.go

+	// Optional: The value to pass to ControllerPublishVolumeRequest.
+	// Defaults to false (read/write).
+	// +optional
+	ReadOnly bool


OK thanks, will change.

vladimirvivien · 2018-08-28T16:01:04Z

pkg/apis/core/types.go

+	// Filesystem type to mount.
+	// Must be a filesystem type supported by the host operating system.
+	// Ex. "ext4", "xfs", "ntfs".
+	// +optional


"Default is auto-detect. If not formatted the FS will be chosen by the node."

I see this quoted, Where is this from ?
Also, for CSI, fstype is passed as-is delegating its interpretation to the CSI driver.

thockin · 2018-08-31T15:38:27Z

I'm trying to keep track of PRs that need re-review. This one has unresolved comments.

saad-ali · 2018-09-01T01:14:26Z

FYI @vladimirvivien #67803 has merged. Feel free to rebase. That PR did not include the generateVolumeHandle API. We can do that in a follow up PR (and slip it to next release, if needed).

vladimirvivien · 2018-09-01T05:44:23Z

@thockin I addressed the comments and update the code.

saad-ali · 2018-09-02T16:14:42Z

pkg/apis/core/types.go

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


@liggitt Ok with inline CSI volumes being able to specify namespace and name of a secret?

I'd expect references inside a namespaced object to only be in that namespace (like all other secret references). Same comment applies for NodeStageSecretRef and NodePublishSecretRef)

other inline volume sources (like ScaleIOVolumeSource, StorageOSVolumeSource, CephFSVolumeSource, etc) use *LocalObjectReference for this, where the namespace is implicitly the namespace of the pod

saad-ali · 2018-09-02T16:19:40Z

pkg/apis/core/types.go

@@ -1570,6 +1573,59 @@ type CSIPersistentVolumeSource struct {
 	NodePublishSecretRef *SecretReference
 }

+// Represents a source location of a volume to mount, managed by an external CSI driver
+type CSIVolumeSource struct {


@vladimirvivien is the code implementing this API going to be in a separate PR?

@saad-ali yes, will publish a separate PR link for impl. Apologies for additional work.

Ok I see it: #68232

Hey guys. So, I think this is a pattern we need to do less of. Getting an API in with no implementation is always going to be riskier than putting them together. Getting a review on API before implementing is fine, but I think we want to merge them together.

vladimirvivien · 2018-09-04T15:41:00Z

@liggitt @thockin @saad-ali PTAL

msau42 · 2018-09-04T22:32:59Z

pkg/apis/policy/types.go

@@ -232,6 +232,10 @@ type PodSecurityPolicySpec struct {
 	// Empty or nil indicates that only the DefaultProcMountType may be used.
 	// +optional
 	AllowedProcMountTypes []api.ProcMountType
+	// AllowedCSIDrivers is a whitelist of allowed CSI drivers.  This slice will be checked


Should this clarify that it's only limiting inline csi drivers (and not pv.csi drivers)?

This will be a way to whitelist any CSI driver, not just inline.

If that's the case, then I think we may need two separate fields, one for inline and one for PV. For example, we want to allow gce PD CSI driver as a PV, but disallow it as inline because a lot of functionality won't work inline.

whether it will work or not is not the point of PSP. you can specify driver names that don't even exist in a CSI PV today, and it fails at the point of calling the driver, correct?

PSP is about explicitly whitelisting which drivers a user is allowed to reference

@msau42 to reduce complexity, the meaning of the PSP is simply a list of allowed CSI drivers here. Capabilities such as inline/attached etc is exposed in the CSIInfo object. We can refine its interpretation in upcoming release.

Is PSP validated at admission time and/or runtime? I thought that today we cannot have PV validation in PSP because of cross-object validation and the PV may not exist yet

Admission time. PSP only applies to pods, so this would only restrict inline CSI volume sources

msau42

Since this is an alpha feature, it needs a feature gate, and the api fields need to be dropped when the feature gate is not enabled.

See https://github.com/kubernetes/community/blob/master/contributors/devel/api_changes.md#alpha-field-in-existing-api-version

saad-ali · 2018-09-10T21:08:14Z

pkg/apis/core/validation/validation.go

+				allErrs = append(allErrs, validateCSIVolumeSource(source.CSI, fldPath.Child("csi"))...)
+			}
+		}
+	}


If feature is disabled, should validation reject the object? Accepting it without validation seems like a bad idea.

@saad-ali not quite following you.

If feature is enabled, only then the object is validated because when the feature is not enabled, the object (VolumeSource.CSI) is cleared as nil (see https://github.com/kubernetes/kubernetes/blob/19be94c88bec6b3cd5983e3f7a468b1e7b30c8bb/pkg/api/pod/util.go#L342)

that is correct, but as described in #67452 (comment), also checking in validation ensured that if the PrepareForCreate/PrepareForUpdate check is removed or skipped, validation prevents alpha data from being persisted when the feature is not enabled.

Just in case the field drop doesn't work, it would be good to return an error in validation if CSI is also set and the feature is off.

@liggitt I misread @saad-ali concerns. Fixed cc @msau42

PTAL

vladimirvivien · 2018-09-12T11:57:15Z

/test pull-kubernetes-e2e-gke

vladimirvivien · 2018-09-12T14:07:50Z

/test pull-kubernetes-e2e-gce-device-plugin-gpu

vladimirvivien · 2018-09-12T14:56:37Z

/test pull-kubernetes-e2e-gke

vladimirvivien · 2018-09-12T17:45:09Z

@lavalamp @liggitt @saad-ali @msau42 PTAL

vladimirvivien · 2018-09-12T18:14:47Z

/test pull-kubernetes-e2e-gke

vladimirvivien · 2018-09-12T19:18:35Z

/test pull-kubernetes-e2e-gke

vladimirvivien · 2018-09-13T01:53:45Z

/test pull-kubernetes-e2e-gke

vladimirvivien · 2018-09-13T14:51:43Z

/test pull-kubernetes-e2e-gke

vladimirvivien · 2018-09-13T14:53:08Z

@thockin @saad-ali @liggitt PTAL. Anything else this PR needs?

vladimirvivien · 2018-09-13T16:45:14Z

/test pull-kubernetes-e2e-gke

vladimirvivien · 2018-09-13T18:49:01Z

/test pull-kubernetes-integration

saad-ali

LGTM

saad-ali · 2018-09-13T19:06:26Z

pkg/apis/storage/types.go

+	// Namespace of the pod with in-line volume. It is used to resolve
+	// references to Secrets in VolumeSource.
+	// Required.
+	Namespace string


Sorry if I missed the conversation around this. But just to verify, specifying the namespace here is considered safe because VolumeAttachment should only be created by kube-controller-manager right? CC @liggitt

just to verify, specifying the namespace here is considered safe because VolumeAttachment should only be created by kube-controller-manager right? CC @liggitt

Yes, InlineVolumeSource is only used in VolumeAttachmentSource inside VolumeAttachment, which is a cluster-scoped object. Normal namespace-constrained users don't have write permission to VolumeAttachment objects.

It contains a denormalized copy of the VolumeSource field from a pod, along with the recorded namespace of the pod (since all the resource references inside the v1.VolumeSource implicitly refer to the namespace of the pod, but that information is not available when the volume source is copied up the a cluster-level object).

vladimirvivien · 2018-09-13T19:54:07Z

/test pull-kubernetes-integration

vladimirvivien · 2018-09-14T00:11:57Z

/test pull-kubernetes-e2e-gke

vladimirvivien · 2018-09-14T10:13:56Z

/test pull-kubernetes-e2e-gke

MrHohn · 2018-09-18T17:04:46Z

/test pull-kubernetes-e2e-gke

thockin · 2018-09-18T23:18:18Z

pkg/apis/core/types.go

@@ -1570,6 +1573,59 @@ type CSIPersistentVolumeSource struct {
 	NodePublishSecretRef *SecretReference
 }

+// Represents a source location of a volume to mount, managed by an external CSI driver
+type CSIVolumeSource struct {


Hey guys. So, I think this is a pattern we need to do less of. Getting an API in with no implementation is always going to be riskier than putting them together. Getting a review on API before implementing is fine, but I think we want to merge them together.

thockin · 2018-09-18T23:22:43Z

pkg/apis/policy/validation/validation.go

@@ -39,6 +40,13 @@ import (
 	utilfeature "k8s.io/apiserver/pkg/util/feature"
 )

+const (
+	csiDriverNameRexpFmt    = `^[a-zA-Z0-9][-a-zA-Z0-9_.]{0,61}[a-zA-Z-0-9]$`


This requires at least 2 letters - is that the correct spec? Or is it something like

^[a-zA-Z0-9]([-a-zA-Z0-9_.]{0,61}[a-zA-Z-0-9])?$

thockin · 2018-09-18T23:31:57Z

pkg/apis/storage/validation/validation.go

@@ -164,8 +164,37 @@ func validateAttacher(attacher string, fldPath *field.Path) field.ErrorList {
 // validateSource tests if the source is valid for VolumeAttachment.
 func validateVolumeAttachmentSource(source *storage.VolumeAttachmentSource, fldPath *field.Path) field.ErrorList {
 	allErrs := field.ErrorList{}
-	if source.PersistentVolumeName == nil || len(*source.PersistentVolumeName) == 0 {


Strictly speaking there's no way to convert a field from required to optional and be rollback-safe. This is kind of a problem. Any creative ideas here?

vladimirvivien · 2018-09-21T16:15:30Z

@thockin Thanks for continuing to review. As far as implementation, I do have another PR (based on this one) with the implementation. If that is the preferred way of moving forward, I don't see a problem -

#68232

I will mark this as paused and redirect future reviews (above).

k8s-ci-robot · 2018-09-29T21:26:34Z

@vladimirvivien: PR needs rebase.

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.

vladimirvivien · 2018-10-09T22:59:36Z

⚠️ This PR will no longer track the implementation for inline volume. It will be closed. Its content has been rebased and combined to PR #68232 for future changes. ⚠️

spiffxp · 2018-10-17T00:34:25Z

/close
closing per the above comment

k8s-ci-robot · 2018-10-17T00:34:25Z

@spiffxp: Closing this PR.

In response to this:

/close
closing per the above comment

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.

k8s-ci-robot requested review from lavalamp and pwittrock August 15, 2018 19:32

k8s-ci-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 15, 2018

k8s-ci-robot added sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 25, 2018

thockin reviewed Aug 28, 2018

View reviewed changes

vladimirvivien commented Aug 28, 2018

View reviewed changes

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

vladimirvivien changed the title ~~[WIP] CSI - API Changes for Pod Inline Volume Support~~ CSI - API Changes for Pod Inline Volume Support Sep 1, 2018

k8s-ci-robot removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 1, 2018

saad-ali reviewed Sep 2, 2018

View reviewed changes

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 4, 2018

vladimirvivien mentioned this pull request Sep 4, 2018

CSI - Implementation for Inline Volume Support #68232

Closed

msau42 reviewed Sep 4, 2018

View reviewed changes

msau42 suggested changes Sep 4, 2018

View reviewed changes

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

saad-ali reviewed Sep 10, 2018

View reviewed changes

vladimirvivien added 2 commits September 13, 2018 11:23

CSI Inline Volumne - API Changes

0c9ae71

CSI Inline Volume - Generated code

458860b

saad-ali reviewed Sep 13, 2018

View reviewed changes

spiffxp mentioned this pull request Sep 17, 2018

pull-kubernetes-e2e-gke is continuously failing #68755

Closed

thockin reviewed Sep 18, 2018

View reviewed changes

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

k8s-ci-robot closed this Oct 17, 2018

This was referenced Feb 14, 2019

CSI Inline Volume Implementation #74086

Merged

CSI Inline Volume - Kubelet/driver implementation #74152

Closed

CSI - API Changes for Pod Inline Volume Support #67452

CSI - API Changes for Pod Inline Volume Support #67452

Conversation

vladimirvivien commented Aug 15, 2018 • edited

thockin commented Aug 28, 2018

thockin left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladimirvivien left a 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

thockin commented Aug 31, 2018

saad-ali commented Sep 1, 2018

vladimirvivien commented Sep 1, 2018 • edited

Choose a reason for hiding this comment

liggitt Sep 2, 2018 • edited

Choose a reason for hiding this comment

liggitt Sep 5, 2018 • 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

vladimirvivien commented Sep 4, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msau42 left a 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 Sep 12, 2018

vladimirvivien commented Sep 12, 2018

vladimirvivien commented Sep 12, 2018

vladimirvivien commented Sep 12, 2018

vladimirvivien commented Sep 12, 2018

vladimirvivien commented Sep 12, 2018

vladimirvivien commented Sep 13, 2018

vladimirvivien commented Sep 13, 2018

vladimirvivien commented Sep 13, 2018

vladimirvivien commented Sep 13, 2018

vladimirvivien commented Sep 13, 2018

saad-ali left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt Sep 13, 2018 • edited

Choose a reason for hiding this comment

vladimirvivien commented Sep 13, 2018

vladimirvivien commented Sep 14, 2018

vladimirvivien commented Sep 14, 2018

MrHohn commented Sep 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladimirvivien commented Sep 21, 2018

k8s-ci-robot commented Sep 29, 2018

vladimirvivien commented Oct 9, 2018

spiffxp commented Oct 17, 2018

k8s-ci-robot commented Oct 17, 2018

vladimirvivien commented Aug 15, 2018 •

edited

vladimirvivien commented Sep 1, 2018 •

edited

liggitt Sep 2, 2018 •

edited

liggitt Sep 5, 2018 •

edited

liggitt Sep 13, 2018 •

edited