Add experimental Alpha API support by xing-yang · Pull Request #417 · container-storage-interface/spec

xing-yang · 2020-02-24T02:17:29Z

Submit the following commit from #365 to provide Alpha API support:

4cf1497

jdef · 2020-02-24T02:24:24Z

jieyu · 2020-02-27T23:35:04Z


+ message AlphaFeature {
+ enum Type {
+ UNKNOWN = 0;


Am I understand correctly that for each new alpha feature capability, a new enum will be added here? If that's the case, another option would be to mark the corresponding enum as alpha, like
message Service { enum Type { UNKNOWN = 0; CONTROLLER_SERVICE = 1; NEW_ALPHA_SERVICE=3 [(alpha_enum) = true]; } Type type; }
I actually don't see a huge value adding this AlphaFeature message here.

you mean alpha_enum_value - right?
PluginCapability lets you pick a Service capability, or a VolumeExpansion capability. It's not clear to me whether all alpha features are a good fit for either. At the same time, PluginCapability.type is a oneof which has pretty strict rules re: backwards compat. If possible it would be nice to make that oneof strictly additive, which my mind ruled out alpha things. So I came up with AlphaFeature as a dumb bucket for things. And if we want to be consistent, people can (should?) just put their alpha features in there while they're iterating .. before things are promoted to stable.
I think that I also remember that someone objected to polluting Service/PluginCapability enums w/ alpha things - and AlphaFeature also seemed to check that box too.
That said, nothing stops someone from dumping an alpha-tagged enum value in the Service bucket. But I tried to avoid that in my examples (see #365) for the sake of consistency.
Also, it might be worth adding a NOTE to the oneof fields that they should not be polluted w/ alpha things. Docs...

At the same time, PluginCapability.type is a oneof which has pretty strict rules re: backwards compat. If possible it would be nice to make that oneof strictly additive, which my mind ruled out alpha things.
I think we can make it strictly additive, if an alpha feature does not make to the end, we can mark it as deprecated.
I think that bag of holding AlphaFeatures makes it hard to GA some fields/enums/... eventually as we'll have to remove it from AlphaFeatures and then add it to Services (or others).

I think that was one of the objections raised, that people didn't want to carry around deprecated or outdated alpha API fields / types / values. we talked about maybe using "reserved" for things like this - as a way to declutter the API w/ respect to outdated names, but we'd still have the baggage of the old identifiers (field/value numbers) hanging around.
the way things have been going, i don't think the above concern is going to be a big problem and i'm happy to remove the AlphaFeature bag if others agree that this is the direction we should go.

Enumerations, fields, messages, methods, and services support `alpha_xxx` designations, indicating that they are part of an experimental feature that may never evolve to "stable" status.

xing-yang · 2020-03-04T18:38:46Z

Removed AlphaFeature bucket.

@jieyu @jdef @saad-ali @julian-hj Please take a look.

jdef · 2020-03-05T02:18:26Z

LGTM

…

On Wed, Mar 4, 2020 at 1:38 PM Xing Yang ***@***.***> wrote: Removed AlphaFeature bucket. @jieyu <https://github.com/jieyu> @jdef <https://github.com/jdef> @saad-ali <https://github.com/saad-ali> @julian-hj <https://github.com/julian-hj> Please take a look. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#417?email_source=notifications&email_token=AAR5KLEHB5HKRRWWQLZ2O63RF2N3RA5CNFSM4K2ADGN2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOENZPZUY#issuecomment-594738387>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAR5KLBEWTFCKOZ4TT2UT23RF2N3RANCNFSM4K2ADGNQ> .

-- James DeFelice 585.241.9488 (voice) 650.649.6071 (fax)

saad-ali · 2020-03-05T05:23:31Z

/lgtm

saad-ali · 2020-03-05T05:24:45Z

Will let @jieyu and @julian-hj LGTM before merge.

julian-hj · 2020-03-05T05:40:26Z

LGTM

…

On Wed, Mar 4, 2020 at 9:24 PM Saad Ali ***@***.***> wrote: Will let @jieyu <https://github.com/jieyu> and @julian-hj <https://github.com/julian-hj> LGTM before merge. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#417?email_source=notifications&email_token=ACAXJ7AVUWI2KDAKGALHBQDRF4ZR5A5CNFSM4K2ADGN2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEN3YISY#issuecomment-595035211>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACAXJ7AAE3KCS6VKJLGDJI3RF4ZR5ANCNFSM4K2ADGNQ> .

jieyu · 2020-03-05T06:02:57Z

@saad-ali i think @xing-yang included this patch in #415

jdef · 2020-03-05T16:54:35Z

Was the plan to merge alpha API support as part of 415? or was 415 just rebased on top of this PR in order to compile, with the expectation that this would be merged first .. and then 415 could rebase to master?

…

On Thu, Mar 5, 2020 at 1:02 AM Jie Yu ***@***.***> wrote: @saad-ali <https://github.com/saad-ali> i think @xing-yang <https://github.com/xing-yang> included this patch in #415 <#415> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#417?email_source=notifications&email_token=AAR5KLGG7DRNJSWXDB7GR7LRF46BFA5CNFSM4K2ADGN2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEN32UGQ#issuecomment-595044890>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAR5KLEX3KPUVURGESE2E53RF46BFANCNFSM4K2ADGNQ> .

-- James DeFelice 585.241.9488 (voice) 650.649.6071 (fax)

xing-yang · 2020-03-05T18:37:20Z

I'm fine either way.

jieyu · 2020-03-05T18:39:18Z

OK, let's merge this first then. @xing-yang

jdef approved these changes Feb 24, 2020
View reviewed changes

xing-yang mentioned this pull request Feb 24, 2020
Add Volume Health Support to CSI #415
Merged

jdef mentioned this pull request Feb 24, 2020
WIP: Add NodeFreeze/UnfreezeVolumeFilesystem Support #407
Open

jieyu reviewed Feb 27, 2020
View reviewed changes

xing-yang force-pushed the alpha branch from cb44c3c to 98b3098Compare March 4, 2020 18:33

spec: added alpha_xxx options
6e109ec
Enumerations, fields, messages, methods, and services support `alpha_xxx` designations, indicating that they are part of an experimental feature that may never evolve to "stable" status.

xing-yang force-pushed the alpha branch from 98b3098 to 6e109ecCompare March 4, 2020 18:36

jieyu approved these changes Mar 5, 2020
View reviewed changes

jieyu merged commit e4404c3 into container-storage-interface:masterMar 5, 2020

Uh oh!

Conversation

xing-yang commented Feb 24, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jdef commented Feb 24, 2020

Uh oh!

jieyuFeb 27, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdefFeb 27, 2020

Choose a reason for hiding this comment

Uh oh!

jieyuFeb 28, 2020

Choose a reason for hiding this comment

Uh oh!

jdefMar 1, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xing-yangMar 4, 2020

Choose a reason for hiding this comment

Uh oh!

xing-yang commented Mar 4, 2020

Uh oh!

jdef commented Mar 5, 2020 via email

Uh oh!

saad-ali commented Mar 5, 2020

Uh oh!

saad-ali commented Mar 5, 2020

Uh oh!

julian-hj commented Mar 5, 2020 via email

Uh oh!

jieyu commented Mar 5, 2020

Uh oh!

jdef commented Mar 5, 2020 via email

Uh oh!

xing-yang commented Mar 5, 2020

Uh oh!

jieyu commented Mar 5, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

xing-yang commented Feb 24, 2020•
edited
Loading

jieyuFeb 27, 2020•
edited
Loading

jdefMar 1, 2020•
edited
Loading