Add --pvc-annotation-mappings arg to pass pvc annotations to parameters

[mkimuram]

What this PR does / why we need it:
As discussed in #86, current csi external-provisioner doesn't pass any pvc annotations to a csi driver. This PR add command line option for csi external provisioner so that it can pass specific set of pvc annotations to a csi driver.

Which issue(s) this PR fixes
Fixes: #86

Special notes for your reviewer:

• This PR add --pvc-annotation-mappings command line option for csi external provisioner,
• User will specify set of mapping from pvc annotation to parameters with key-value pairs, like --pvc-annotation-mappings=annotation1=parameter1,annotation2=annotation2,
• PVC annotations specified with the key in the option will be mapped as value for parameters:
  • ex) If annotations in PVC are as below and above option is specified, parameters will be passed to csi driver as below:

[Annotations]

    annotation1: aaa
    annotation2: bbb

[Parameters]

    parameter1: aaa
    annotation2: bbb

• No behaviour changes if --pvc-annotation-mappings isn't specified,
• No behaviour changes for the pvc annotations which aren't specified in --pvc-annotation-mappings,
• Annotation names can be changed to avoid conflict with storage parameter if needed. (Name remains the same if key and value are the same in the option.)

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mkimuram
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: jsafrane

If they are not already assigned, you can assign the PR to them by writing /assign @jsafrane in a comment when ready.

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:

• OWNERS

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

[mkimuram]

@fatih @orainxiong @kfox1111

Could you check if this PR fit to your use cases?

@jsafrane @princerachit @saad-ali

PTAL
I believe that this PR meets the requirement to:

• have ability to selectively pass pvc annotation to csi driver,
• avoid name conflict to storage parameters,
• be harmless to existing behaviour.

[fatih]

@mkimuram I quickly glanced over, but I believe this only allows us to pass a fixed set of parameters, defined as an argument to the external provisioner. This still doesn't allow me to pass custom, dynamic data per volume ?

[mkimuram]

@fatih

Thank you for your feedback.

This still doesn't allow me to pass custom, dynamic data per volume ?

Do you intend to pass different annotation keys per volume, instead of different annotation values per volume?
If different annotation values per volume are only required, this PR should meet the requirement, because values can be different as seen in the example below.

ex)

1. If --pvc-annotation-mappings=annotation1=annotation1 is specified as csi external-provisioner's argument.

• if pvc has annotation annotation1: aaa, parameter is passed to csi driver as annotation1: aaa,
• if pvc has annotation annotation1: bbb, parameter is passed to csi driver as annotation1: bbb.

2. If --pvc-annotation-mappings=annotation1=parameter1 is specified as csi external-provisioner's argument.

• if pvc has annotation annotation1: aaa, parameter is passed to csi driver as parameter1: aaa,
• if pvc has annotation annotation1: bbb, parameter is passed to csi driver as parameter1: bbb.

In other word, value for --pvc-annotation-mappings is used just to rename the parameter to avoid conflict with storage class parameter, not for the value for the parameter itself.
(If the rename is not required, we might make it just a list of string, not key-value pairs.)

If different annotation keys per volume is required and they aren't decided at the time of csi external-provisioner start up, this PR won't meet the requirement, for all the set of annotation keys can't be passed as csi external-provisioner's argument.

[princerachit]

maybe a UT for this?

[mkimuram]

Thank you for your comment.

I will add unit tests for this, once we reach to the agreement that this specification is good to everyone.

[mkimuram]

@princerachit

I rebased and add unit tests. PTAL

@davidz627

I hope that you could also review this, because you recently changed the related codes, so you have knowledge on how csi external provisioner should pass parameters from k8s to CSI drivers.

[davidz627]

code change mostly lgtm.

[davidz627]

I think this description is a little confusing since its not really key=value, it's actually more like pvcannotationkey=csidriverparameterkey (<- this isn't actually good I urge you to reword so that this concept is more clear though)

[davidz627]

add to existing param map instead of creating a new one?

[davidz627]

@mkimuram I am a little concerned with the overall design of this feature though. But also I don't think I fully understand the use case.

"Who" is setting the PVC annotation (end user)? Is this a different user than the one creating the storage class (an admin)?
If so, then why is the entity creating PVC (an end user) concerning themselves with driver specific details such a parameters passed in to create volume. This seems like not a great way for providing information to the driver as the PVC should not really be concerned with the underlying storage provider. I believe this reaches through and breaks the abstraction layer created between PV-PVC.
/cc @saad-ali

If these users are actually the same person (end user + admin), then why do we need the mapping, this user can just make sure that the names are not colliding and we can throw an error if the annotations collide with storage class parameters. But the concern about PVC providing information specific to a driver implementation is still present here.

[saad-ali]

I strongly agree with @davidz627. The Kubernetes PVC object is intended for application portability. When we start leaking cluster specific details in to it we are violating that principle. And explicitly passing PVC annotations encourages that pattern.

Let's discuss the specific use cases you have in mind, and see if we can come up with better solutions for each of those uses cases (for example, what we did for passing pod workload information to drivers on NodePublishVolume calls) rather then opening up a hole in the API.

/hold

[mkimuram]

@davidz627

"Who" is setting the PVC annotation (end user)? Is this a different user than the one creating the storage class (an admin)?

Thank you for your confirmation. This should be the point that we all have the same understanding.
There are some people requesting this feature, so I'm not sure that they are requesting for the same use case to me. (If anyone assumes different use cases, please let me know them.)

As for my use case, it is end user who sets PVC annotation and the user is different from admin.
The reason that PVC annotation need to be passed to driver is that user would like to change the behavior per volume, not per StorageClass.

One of the example use cases was cloning the volume from another volume like it is done in Gluster driver. I know that we are adding snapshot and cloning feature, so this feature won't be necessary for this particular use case.

However, there will be other use cases that could be solved by this feature until k8s become available, like setting QOS, tiering policy, and so on, per volume basis. These kinds of parameters can also be defined to StorageClass, but number of StorageClasses will explode, if the number of combinations increase. Therefore, per volume parameter will be useful in such a situation.

@saad-ali

The Kubernetes PVC object is intended for application portability. When we start leaking cluster specific details in to it we are violating that principle. And explicitly passing PVC annotations encourages that pattern.

I agree that above use of annotation would cause the situation that you mentioned and it potentially causes compatibility issues between cluster. However, I believe that PVC annotation was intended to allow enabling these kinds of things, so it will be more useful than harmful, if it is properly warned to end users.
(Many end users will be happy if they have some ways to achieve what they need with some restrictions than there are no way to achieve it.)

[saad-ali]

However, I believe that PVC annotation was intended to allow enabling these kinds of things, so it will be more useful than harmful, if it is properly warned to end users.
(Many end users will be happy if they have some ways to achieve what they need with some restrictions than there are no way to achieve it.)

Workload portability is a corner stone of Kubernetes. It is one of the major reasons for the success of the project. We must be very careful when we violate it. And Driver developer velocity, to me, is not sufficient justification. When we add anything to the CSI external-provisioner it gives it a Kubernetes "seal of approval/authority" that this is the right thing to do. And adding cluster specific parameters or annotations on a PVC is behavior we should strongly discourage as it makes a portable app developer facing API object non-portable. If someone really wants to do this, I want them to have to do extra work to do so (instead of making it easier for them) -- I want to incentivize working on standards solutions that improve Kubernetes. More specifically I want to incentivize working with the community to develop appropriate APIs to enable new use cases in alignment with Kubernetes principles.

I am happy to discuss this more at the SIG Storage meeting if folks want.

[mkimuram]

@saad-ali

Thank you for your comment.

If someone really wants to do this, I want them to have to do extra work to do so (instead of making it easier for them) -- I want to incentivize working on standards solutions that improve Kubernetes.

I understand that discussion is going back to this comment and
"write your own external provisioner which knows what params to pass for your specific driver"
is also required to pvc annotation.

Workload portability is a corner stone of Kubernetes. It is one of the major reasons for the success of the project. We must be very careful when we violate it. And Driver developer velocity, to me, is not sufficient justification.

I will wait for that someone provide better use cases or reasons that this feature should be prioritized over workload portability. Until then, anyone who need this feature will need to port this PR to their own external provisioner. (Or do their own implementation.)