Inline CSI volumes support KEP

[vladimirvivien]

Currently, volumes that are backed by CSI drivers can only be used with the PersistentVolume and PersistentVolumeClaim objects. This proposal is to implement support for the ability to nest CSI volume declarations within pod specs:

apiVersion: v1
kind: Pod
metadata:
  name: testpod
spec:
  containers:
    ...
  volumes:
      - name: vol
        csi:
          driver: mock.storage.kubernetes.io
          volumeAttributes:
              name: "Mock Volume 1"
          volumeHandle: "1"

This would put CSI drivers on feature parity with existing in-tree storage plugins which already support this volume deployment mode.

Related feature entry - #596
This KEP started life as feature #2273.

/sig storage

[kfox1111]

This has a few issues in decreasing levels of importance:

1. The user is in control of it. This means the user can not specify it, give it the same volume handle as another user's ephemeral volume and it would mount their volume. This is a security issue.
2. Because of the restricted api, a given driver may malfunction if switched to ephemeral style when the driver doesn't support it.
3. The user needs to know to specify it.
4. It takes more effort for the user to specify.

Due to all of these, I recommend we change it to a CSIDriver CR flag. Then the driver is flaged as either ephemeral or normal by the driver packager. A driver can not be made to be both normal and ephemeral. We drop the lifecycle flag.

[vladimirvivien]

@kfox1111 thank you for the input. I think it escaped me that a driver should either be persistent or ephemeral, but never both. In that case, the setting can be introduced in the CSIDriver object.

[kfox1111]

lifecycle flag still here. should be removed.

[msau42]

should this lifecycle field be removed then?

[vladimirvivien]

ah ok. Fixing.

[kfox1111]

Oh. One more thing. There needs to be a section covering the PodSecurityPolicy stuff around needing a way to limit which drivers can be used PodInline. As an Operator I need to restrict my users only to the safe subset of ephemeral drivers I want to support for them.

[vladimirvivien]

@kfox1111 thanks for the input. I will address.

[saad-ali]

AllowedInlineCSIDrivers? Or at least mention in the comment that this refers only to volumes referenced inline not via PVC?

[saad-ali]

It probably should apply to both inline and PVC right? We should be clear about that. If it applies to both, does it need to be part of this inline design or should it be a new standalone design?

[kfox1111]

pvc's cant specify a driver to use. pv's do, but normal users can't create those at all. So I don't think it matters to apply there. i don't think the other volume restrictions in podsecuritypolicy apply to pv's either.

[liggitt]

PSP only applies to pod objects... it doesn't have visibility to transitive information in PVC->PV or PVC->StorageClass

[liggitt]

I don't think Inline is needed... most of the other names have counterparts in PV sources. PSP is specifically for things in pod specs

[vladimirvivien]

@saad-ali thanks for the review.

[vladimirvivien]

@msau42 thank you for the review.

[kfox1111]

Looks good to me I think. down to a few simple typo's now. Nothing that effects the actual implementation.

[timothysc]

It would help the reader to know what the use cases are for out of tree ephemeral CSI volumes. From a user story perspective, I'm missing the "why", or a reference/link to the why.

[vladimirvivien]

Adding to the storage provider user story, why ephemeral CSI drivers would be useful. Thanks.

[timothysc]

Is there a tracking issue, or reference, that outlines the issues with moving * out of tree?

The grand promise of CSI is ~= CNI, it's a post bootstrap addition, and this KEP is a step in that direction. I guess I'm looking for a roadmap or umbrella issue that outlines this.

[kfox1111]

I don't know of a complete issue/roadmap.

I do know about this one:
kubernetes/community#2199

[vladimirvivien]

@timothysc CSI has similar aspiration of moving in-tree drivers out via CSI drivers. There is a separate set of tasks and roadmap (here) with design to track the exodus.

[pohly]

I'm still confused about this. So now here it states that it should work "with existing CSI drivers".

But then further down it says "A CSI driver must indicate that it supports either persistent or ephemeral inline mode." and explains how a driver in ephemeral mode must "delay or combine any provisioning/deprovisioning operation".

In other words, none of the "existing drivers" will work - at least not for ephemeral mode.

Or is the goal merely that "Inline persistent volumes" should work without modifications?

[kfox1111]

existing (normal) drivers should work without changes with pod inline.

There isn't such a thing as ephemeral drivers yet. those need to be specifically coded to work as they don't support the entire csi workflow and need to self provision/delete backing stores.

[pohly]

Can we change the wording to make this obvious? I propose:

• Inline persistent CSI volumes should work with existing CSI drivers

[kfox1111]

sounds good to me.

[vladimirvivien]

@pohly sounds good. I will make the change.

[pohly]

Above it says "A CSI driver must indicate that it supports either persistent or ephemeral inline mode." and here it says that persistent is the default.

So if CSIDriver.lifecycle is not set, does that mean that support for persistent inline mode is enabled (because that is the default)?

[kfox1111]

yes

[vladimirvivien]

@pohly in order for existing persistent driver to work automatically, when that new flag is introduced in CSIDriver object, if not provided or not set, the code must assume it is working as a persistent driver.

[vladimirvivien]

I clarified the language anyway.

[pohly]

Given @kfox1111's clarification, I propose to change this into "A CSI driver must indicate that it supports ephemeral inline mode. This setting will be stored...".

What I'm missing in the document is an explicit statement that a driver can only be used in one mode or the other, and an explanation why that decision was made.

At first glance that decision seems to make it harder to deploy a driver that can operate in both modes. My colleagues are working on https://github.com/intel/pmem-csi, which manages local storage. Exposing that as ephemeral local storage is one way of using that storage, but we have ideas how we can make persistent mode also useful. It would be nice if we didn't have to run two driver daemons on each node just to support both. I can imagine doing it via two different CSI sockets provided by the same process, but if we could have a single socket and a parameter on NodePublishVolume for "this is an ephemeral volume" then that might be simpler.

[kfox1111]

The issue is, Mount/Unmount of an ephemeral driver will do the creation of the backing store and the deletion of the backing store in the driver itself. The driver isn't handed any info to decide if it is an ephemeral driver or a normal one.

We might be able to relay this information in the future. I'd suggest though we make them totally separate for now and we can revisit having a driver that can do both modes in the future. We're really close to the deadline now.

[vladimirvivien]

@pohly thanks for the feedback. So I think a bit of clarification is in order (thanks for pointing that out). A driver should be able to support both at the same time. The lifecycle flag is consequential depending on how the volume is originated. When volume is being used inline, in a pod, the lifecyle flag is used to determine how it should function. When a volume is originated by a PV/PVC, it should automatically be assumed to be persistent.

The problem is, if a volume originates from PV/PVC and the driver does not in fact support persistent volume, then there will be issues.

[kfox1111]

The protocol we came up with, for how an ephemeral driver will work does not have enough information to switch between being ephemeral and not being ephemeral.I say we only support one or the other explicitly for now, as this is alpha, and add that functionally later. perhaps as an enhancement to the pod info passing feature.

[pohly]

FWIW, I'm fine with supporting just one mode for now. I just wish the document would elaborate a bit more about the reasoning that led to the proposal. But that shouldn't stop it from getting merge as-is.

[vladimirvivien]

@pohly I updated the doc to address the language and clarify your concerns.

[msau42]

/lgtm

[kfox1111]

@saad-ali how is this looking? Good enough?

[msau42]

@vladimirvivien I think you need to update the NEXT_KEP_NUMBER file
/hold

[msau42]

Nevermind. apparently kep numbers are not used anymore. So you should actually rename the file and remove the kep number from it.
/hold cancel

[saad-ali]

/approve

Will let @liggitt lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

• keps/sig-storage/OWNERS [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]

not sure how this got to implementable with TBD approvers. moving @thockin (for API) and @saad-ali (for storage) from reviewers to approvers would probably make sense

[liggitt]

is this enforced by API validation, or by failing the pod at runtime if no volumeHandle is provided and the CSIDriver.lifecycle is not ephemeral?

[kfox1111]

runtime's pretty easy to implement. api validation would be harder? can an api validator lookup cr's to validate against? can we add to the existing api validation logic or would that need to be an external webhook thing?

[vladimirvivien]

@liggitt this will have to happen at runtime since volumeHandle can support two modes one of which does not require it when ephemeral. The api validators don't usually call the API server (or at least i haven't seen one), but if possible or doable, then the validator will check the CRD for proper application.

[liggitt]

Runtime makes sense. Clarify that pods targeting ephemeral inline drivers can specify a volumeHandle that will be ignored, and that pods targeting persisting inline drivers can omit a volumeHandle which will result in an error (and describe where in the lifecycle that error occurs, and how it surfaces to the user... I'd guess when the volumeattachment was attempted to be created, since no name would be able to be computed)

[liggitt]

We'll need to iron out the implementation details when multiple pods in different namespaces share the same volume.

just got here... sketching out the lifecycle of a volumeattachment object with multiple pods involved would help understand if there are information flow gaps. (who creates the object, based on what info? who detaches the volume, based on what signal? etc)

[liggitt]

reporting errors about missing secrets to user

using what mechanism?

[kfox1111]

pod events?

[vladimirvivien]

@liggitt this will have to be done with events and logging.

[liggitt]

if there's an error attaching a volume, is it reported as an event into all namespaces that have pods waiting for that attachment, or just into the namespace referenced by the VolumeAttachment object?

[liggitt]

is this section only relevant for persistent inline CSI volumes? see question below at the "Ephemeral inline drivers must ignore attachment requests" line

[kfox1111]

yeah. Not needed for ephemeral I think.

[vladimirvivien]

@liggitt that is correct. Only persistent inline drivers will participate in that phase.

[liggitt]

if so, clarify that at the top... if VolumeAttachment objects are not created for ephemeral inline volumes, that answers #716 (comment) and partially answers the ephemeral part of #716 (comment)

[vladimirvivien]

Clarified.

[liggitt]

this is a little confusing...
does that mean VolumeAttachment objects are not created for ephemeral inline volumes?
does that mean the CSI driver is not invoked with attach/detach requests, or that it is invoked and should ignore the invocations?

[kfox1111]

I think it means the three of:

• volumeattachment's wont be created
• csi attach/detach requests will not be issued to the driver
• the csi driver should not implement attach/detach

The latter allows us to potentially add them at some point and not break existing ephemeral drivers. I don't foresee ever needing them, but its always good to give yourself room to make enhancements later to cover your own lack of foresight. :)

[vladimirvivien]

@liggitt one of the major reason ephemeral won't participate in attach/detach is because of the volumeHandle auto-generation strategy which will be based on the podUID for guaranteed uniqueness. During Attach, there is no suitable podInfo that I was able to find. So, the ephemeral drivers are restricted to mount/unmount which is when all info needed for form handle is available.

[liggitt]

that seems reasonable... clarifying the whole attach/detach/volumeattachment section only applies to persistent inline drivers would be good

[liggitt]

I found the * Ephemeral volumes will only make CSI calls for *mount* and *unmount* volume operations. statement, which I should have connected to the volume attachment bits not applying, but reiterating it for slow readers like me would be nice :)

[vladimirvivien]

Will re-read KEP and make sure that point is clear.

[liggitt]

to read Secrets in any namespace

clarify this only attempts to read secrets referenced by CSI volume sources