Allow pvc.Selector and pvc.Class both to be set.

[jsafrane]

To simplify initial implementation we made selector and class mutually
exclusive. Now it's time to finish the implementation and let users to
choose e.g. a zone where a volume should be provisioned in pvc.Selector.

@kubernetes/sig-storage-misc

[thockin]

@eparis had a seemingly legit concern that this intersection was not easily discovered, and wanted the class to expose a list of "supported selectors", which was a large part of why this was not done in 1.4.

Has that requirement changed? Where's the discussion?

[eparis]

My concerns about usability have not been "seemingly" addressed....

[jsafrane]

@eparis, @thockin, I added a new commit with a proposal how this discovery could work. It's not set in stone, feel free to suggest a better way.

[liggitt]

this should be a structured, first-class field on the storage class, so that clients can be built to allow users to specify these

straw man type:

type StorageClass struct {
  ...
  SelectorOptions []SelectorComponent
}

type SelectorComponent struct {
  // Label selector key
  Key string

  // If unspecified in the PVC, this value will be used
  DefaultValue string
  // List of all allowed values. Either AllowedValues or DisallowedValues may be specified, not both. If neither are specified, any valid label value is allowed.
  AllowedValues []SelectorValue
  // List of explicitly disallowed values. Either AllowedValues or DisallowedValues may be specified, not both. If neither are specified, any valid label selector value is allowed.
  DisallowedValues []SelectorValue

  // Optional display name. May not contain newlines. If unspecified, Key is used.
  DisplayName string
  // Optional description. May contain newlines and links.
  Description string
}

type SelectorValue struct {
  // Selector value
  Value string
  // Optional display name. May not contain newlines. If unspecified, Key is used.
  DisplayName string
  // Optional description. May contain newlines and links.
  Description string
}

that could look something like:

...
"selectorOptions": [
  {
    "key":"zone",
    "defaultValue":"east",
    "allowedValues": [{"value":"east"},{"value":"west"}],
    "displayName":"Availability zone"
  },
  {
    "key":"type",
    "defaultValue":"hdd",
    "allowedValues": [
      {"value":"ssd", "displayName":"Solid-state"},
      {"value":"hdd", "displayName":"Spinning-disk"}
    ],
    "displayName":"Storage type"
  }
]
...

[thockin]

I like structured form, but I see some difficulties, too.

Who writes these into StorageClass? That is an admin object. If the admin has to write these, the list could potentially get long and tedious. Is that a problem? If the provisioner is going to write this into StorageClass, how do we let the admin have final say over what is and is-not allowed? It seems reasonable that an admin might want to disallow some values or even some whole keys.

displayName and description are awkward. Storing them in the API makes internationalization difficult. Any non-english client will have to i18n them, which means nobody should rely on the API strings in software, or those strings become the i18n keys, which means they can never ever change. Do we have other precedents of this in our API?

What about cases where the default value is "chosen by the implementation" e.g. one of the zones available to the cluster.

[jwforres]

As someone who is going to have to build UI on top of this, I would much rather see structured data here.

The original annotation proposal has the same translation concern you have here, unless you are proposing having a separate annotation for each language?

[derekwaynecarr]

I prefer a structured form, and I hope the admin has a tool to aid in authorship.

[thockin]

My point was that if you;re doing a UI you might be better off to keep a strings-DB rather than embed them in the API objects.

…

On Fri, Jan 6, 2017 at 12:23 PM, Jessica Forrester ***@***.*** > wrote: ***@***.**** commented on this pull request. ------------------------------ In contributors/design-proposals/volume-provisioning.md <#247>: > ``` +StorageClass can have annotation +`volume.beta.kubernetes.io/storage-class-selectors` <http://volume.beta.kubernetes.io/storage-class-selectors> that contains free-form +english text with description of labels that are supported by the provisioner +in pvc.spec.selector when provisioning a new PV for this StorageClass. As someone who is going to have to build UI on top of this, I would much rather see structured data here. The original annotation proposal has the same translation concern you have here, unless you are proposing having a separate annotation for each language? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#247>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFVgVATTZ7DPlUdwnJ48cbsjxV4TJaWaks5rPqLAgaJpZM4LZA4P> .

[liggitt]

only the volume provisioner or cluster admin knows the meaning of a particular selector key/value and can provide a display name or description. I agree that getting potentially translatable data via the API is awkward, but it's more awkward to try to convey human readable names or descriptions to multiple clients (web, CLI, etc) without using the API.

That said, I'd still be far happier with structured data even if it didn't have display name or description.

[thockin]

wrt description and display stuff, I'm inclined to leave it out until we have a greater sense of requirements. All my other questions stand. Also the examples should probably use namespace keys, like ebs.volume.kubernetes.io/type

…

On Fri, Jan 6, 2017 at 9:08 PM, Jordan Liggitt ***@***.***> wrote: only the volume provisioner or cluster admin knows the meaning of a particular selector key/value and can provide a display name or description. I agree that getting potentially translatable data via the API is awkward, but it's more awkward to try to convey human readable names or descriptions to multiple clients (web, CLI, etc) without using the API. That said, I'd still be far happier with structured data even if it didn't have display name or description. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#247 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFVgVBB8d7zZ_zQWxzyS0OEx4nuIcJowks5rPx3igaJpZM4LZA4P> .

[jsafrane]

  // List of all allowed values. Either AllowedValues or DisallowedValues may be specified, not both. If neither are specified, any valid label value is allowed.
 AllowedValues []SelectorValue
 // List of explicitly disallowed values. Either AllowedValues or DisallowedValues may be specified, not both. If neither are specified, any valid label selector value is allowed.
 DisallowedValues []SelectorValue

By default, AWS provisions a volume in a random zone/region where it has a node. As nodes can come and go dynamically, value of AllowedValues can change too. I.e. there needs to be a controller that keeps it in sync and I'd like to avoid that. Too much to care about.

Also, if we allow e.g. configuration of IOPS, then AllowedValues can be a range, not enumeration. IMO, it gets complicated very quickly.

Who writes these into StorageClass?

Initially I proposed an admission plugin to add it to newly created StorageClasses, however it can't be admission plugin if we need to update the value in time, see above.

If the provisioner is going to write this into StorageClass, how do we let the admin have final say over what is and is-not allowed? It seems reasonable that an admin might want to disallow some values or even some whole keys.

Currently, I consider only region/zone to be configurable with a selector. And it's already parameter of AWS/GCE/OpenStack StorageClass. So, admin can configure list of zones where a PV can be provisioned (via SC.parameters) and user can pick a specific one from these using a PVC.selector. Other selectors could follow the same pattern.

[liggitt]

By default, AWS provisions a volume in a random zone/region where it has a node. As nodes can come and go dynamically, value of AllowedValues can change too. I.e. there needs to be a controller that keeps it in sync and I'd like to avoid that. Too much to care about.

I could see the allowedValues and disallowedValues being optional. If either is specified, it can be used by a client builder to present options or indicate disallowed values. If neither is specified, then a client would have to allow arbitrary values, and a human-readable description of what the values need to be becomes important (though I'd still want that description associated with a particular selector key)

Also, if we allow e.g. configuration of IOPS, then AllowedValues can be a range, not enumeration. IMO, it gets complicated very quickly.

Does a range fit well with normal use of label selectors? I thought enumerability was typically expected.

admin can configure list of zones where a PV can be provisioned (via SC.parameters) and user can pick a specific one from these using a PVC.selector.

how does the user know which ones they can pick from?

[jsafrane]

admin can configure list of zones where a PV can be provisioned (via SC.parameters) and user can pick a specific one from these using a PVC.selector.

how does the user know which ones they can pick from?

If SC.SelectorOptions were added by an admission plugin then it could ask appropriate (internal) provisioner to give a list of them. Assuming that the list is static.

[thockin]

By default, AWS provisions a volume in a random zone/region where it has

a node. As nodes can come and go dynamically, value of AllowedValues can change too. I.e. there

needs to be a controller that keeps it in sync and I'd like to avoid

that. Too much to care about.

Also, if we allow e.g. configuration of IOPS, then AllowedValues can be a

range, not enumeration. IMO, it gets complicated very quickly. Agree - this gets into DSL territory very quickly if we try to make this anything more than advisory.

> Who writes these into StorageClass? Initially I proposed an admission plugin to add it to newly created

StorageClasses, however it can't be admission plugin if we need to update the value in time, see above.

> If the provisioner is going to write this into StorageClass, how do we

let the admin have final say over what is and is-not allowed? It seems reasonable that an admin might

> want to disallow some values or even some whole keys. Currently, I consider only region/zone to be configurable with a

selector. And it's already parameter of AWS/GCE/OpenStack StorageClass. So, admin can configure list of

zones where a PV can be provisioned (via SC.parameters) and user can pick

a specific one from these using a PVC.selector. Other selectors could follow the same pattern. I don't follow. How do you "consider only region/zone to be configurable" when you're defining the API to make arbitrary anything be configurable? My point was that the admin needs final say, not the provisioner. Provisioner might support values A, B, C, but admin only wants to allow A and B. How does that work? Admin configures provisioner by flag, provisioner writes to storage class? That's very roundabout, and will be very confusing.

Does a range fit well with normal use of label selectors? I thought

enumerability was typically expected. This is a good point. I don't think users should be saying "I want 6924 IOPS". They should be saying "I want tier-1" or something.

…

On Mon, Jan 9, 2017 at 7:06 AM, Jan Šafránek ***@***.***> wrote: admin can configure list of zones where a PV can be provisioned (via SC.parameters) and user can pick a specific one from these using a PVC.selector. how does the user know which ones they can pick from? If SC.SelectorOptions were added by an admission plugin then it could ask appropriate (internal) provisioner to give a list of them. Assuming that the list is static. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#247 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFVgVBfw58EB4i3_onX_b66bABzs3m5zks5rQkz_gaJpZM4LZA4P> .

[jsafrane]

Does a range fit well with normal use of label selectors? I thought enumerability was typically expected.

So far, generic selectors (e.g. in a service) were used to choose a subset of a finite set of existing objects (e.g. pods). By definition, all values can be easily enumerated by listing all objects (pods). Now, we misuse selectors to create a new object (PV) that matches the selector. The domain from which we create the object may be very large (it's probably finite though).

My use case was only to allow users to choose a region or zone. Do we really see demand for a generic selectors? Shouldn't be IOPS specificaly part of StorageClass parameters, not configurable by users?

[jsafrane]

I don't follow. How do you "consider only region/zone to be configurable" when you're defining the API to make arbitrary anything be configurable?

The only use case I have is to allow users to choose a region or zone. I personally do not want API for selectors, IMHO link to docs could be enough. You (and @eparis) requested that, I tried something very simple. And then it exploded with @liggitt's generic API.

My point was that the admin needs final say, not the provisioner.
Provisioner might support values A, B, C, but admin only wants to allow A
and B. How does that work? Admin configures provisioner by flag,
provisioner writes to storage class? That's very roundabout, and will be
very confusing.

I agree it's confusing, however don't see a better way. Following your example, if admin does not say anything, something should fill into the SC that values "A,B,C" are allowed. Only appropriate volume plugin knows this, so it must be involved at some point. And IMO the right place when/where to fill it into SC is an admission plugin.

Similarly, if admin chooses "A,B,D" and the provisioner supports only "A,B,C", something should fill only "A,B" as supported selectors (or return an error).

Admin has the final say, roundabout through volume plugins just make the admin's decision discoverable via generic API [which I still think is not worth the effort...]

[liggitt]

requiring consumers to read and understand a natural language description of valid selectors and hand-craft a matching selector doesn't seem usable to me

[derekwaynecarr]

events get lost. is there any thought about having a PersistentVolumeClaimCondition in status that could report things like this now or in the future?

[jsafrane]

I've rewritten the proposal completely to accommodate @liggitt's suggestion for full API for supported selector keys with some simplification - I don't think that we need struct for SelectorValue, simple string with allowed value could be IMO enough.

I created this as a separate proposal so volume-provisioning.md can be declared stable in 1.6.

Contrary to previous proposal, no admission plugin is expected and admin is responsible for filling all supported selector keys/values into StorageClasses.

In addition, I expect the new StorageClass field to go trhough alpha-beta-stable cycle, starting as an alpha annotation as suggested by https://github.com/kubernetes/community/blob/master/contributors/devel/api_changes.md#adding-unstable-features-to-stable-versions.

[liggitt]

remove the human-readable fields displayName/description, since there were concerns about translation, etc

[jsafrane]

removed

[liggitt]

if it defaults to false, why make it a *bool?

[jsafrane]

good catch, it's plain bool now.

[jsafrane]

fixed @liggitt's comments

[jsafrane]

@liggitt @thockin, PTAL, I think I addressed all comments, is it ready to be implemented in 1.7 (as alpha annotations)?

[msau42]

Do we want to have a description field?

[jsafrane]

I am not sure, I can leave it out (and it will be out in the initial alpha implementation). I think it was @liggitt who asked for it.

[msau42]

I think this can co-exist with the topology constraints feature. This feature is allowing specific label values to be passed through to the PV, and the topology constraints is specifying what label keys on the PV the scheduler should consider.

[msau42]

After some discussion with @vishh and @saad-ali, there were two things we were concerned with:

1. Specifying/validating allowed values in StorageClass.

Given that we will be working towards integrating storage availability constraints into scheduling decisions, this adds a further dimension of constraints that the scheduler would need to be aware of. What happens if the scheduler decides the pod can only fit in zones a and b, but the storageclass is constrained to zones c and d? Or what if the storageclass contraints cause conflicts with the pod zonal spreading policies? Availability constraints could be better integrated with the scheduler if it was driven down from the pod level instead of driven up from the storage level.

What reasons would a storage administrator want to limit zones? Could the limits be controlled via quotas?

In general, validation errors can be returned by the provisioner as an event.

2. Passing a selector to the provisioner so that users can specify a range of values to pick from.

I can imagine a user wanting to pick a specific zone, but is there a use case for a user wanting to pick from a range of zones?

This adds a lot of complexity to all the provisioners to be able to understand the selector, and now the provisioners need logic to make a decision from a variety of choices. For the zone example, the scheduler should be the one making the decision of which zone a pod would best fit in.

If only specifying single values is good enough, then it could be a map instead of a selector.

[vishh]

At a meta level, I'd prefer controlling access to zones, racks, nodes, etc. from the pod level instead of from the resource level (storage in this case). If a pod cannot land in a zone, is provisioning storage in that zone a concern at all?

…

On Tue, Apr 4, 2017 at 5:52 PM, Michelle Au ***@***.***> wrote: After some discussion with @vishh <https://github.com/vishh> and @saad-ali <https://github.com/saad-ali>, there were two things we were concerned with: 1. Specifying/validating allowed values in StorageClass. Given that we will be working towards integrating storage availability constraints into scheduling decisions, this adds a further dimension of constraints that the scheduler would need to be aware of. What happens if the scheduler decides the pod can only fit in zones a and b, but the storageclass is constrained to zones c and d? Or what if the storageclass contraints cause conflicts with the pod zonal spreading policies? Availability constraints could be better integrated with the scheduler if it was driven down from the pod level instead of driven up from the storage level. What reasons would a storage administrator want to limit zones? Could the limits be controlled via quotas? In general, validation errors can be returned by the provisioner as an event. 1. Passing a selector to the provisioner so that users can specify a range of values to pick from. I can imagine a user wanting to pick a specific zone, but is there a use case for a user wanting to pick from a range of zones? This adds a lot of complexity to all the provisioners to be able to understand the selector, and now the provisioners need logic to make a decision from a variety of choices. For the zone example, the scheduler should be the one making the decision of which zone a pod would best fit in. If only specifying single values is good enough, then it could be a map instead of a selector. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#247 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGvIKIFZ0nBqwMacnftiPSwaBjPoFa6Yks5rsuXUgaJpZM4LZA4P> .

[davidopp]

ref/ kubernetes/kubernetes#41442

[lpabon]

This looks complicated to me. Probably easier to do a AllowNone or AllowAll style.

[lpabon]

@jsafrane I agree that using Selectors seems weird since they are normally used to pick something that already exists, not something that would be created. You could use annotations to a PVC, maybe that could be the route. As compared to kubernetes/enhancements#244 , I like the model because it describes something to be created, not something to be selected which does not exist.

[jsafrane]

I am closing this proposal, we as sig-storage have other priorities and we did not find people to lead this effort. Volunteers are welcome at kubernetes/enhancements#244 !