KEP-3333 Retroactive default StorageClass assignment

[RomanBednar]

• One-line PR description:
  Retroactive default StorageClass assignment

• Issue link:
  Retroactive default StorageClass assignement  #3333

• Other comments:

[jsafrane]

We will provide PRR either in a separate commit later (or in separate PR, if this one gets merged)

[xing-yang]

Look mostly good to me. Just some minor comments.

[wojtek-t]

I agree that the newly proposed behavior seems more intuitive, but it's also potentially a breaking change for existing users.

Before approving such change from PRR perspective, I would like to get opinion from API approvers (as even though we're not introducing a new API, we're technically changing the meaning of existing API) if we can even allow such change.

@liggitt - can you please help with evaluating it?

[jsafrane]

@liggitt, please also check Alternatives at the bottom, there are ways how to keep some parts of the existing behavior, but the behavior would be quite complex from user point of view.

[wojtek-t]

It seems Jordan is out - let me ask @thockin about this.

[ddebroy]

nit: how about "bind the PVC to a statically provisioned PV" rather than "provision those PVs statically"

[RomanBednar]

That sounds more accurate - changed.

[ddebroy]

Are there going to be any changes to scenarios involving pre-binding of PVs to PVCs with claimRef field of PV set to a PVC that will be created in the future?

[RomanBednar]

Yes, there will be changes since PVC with nil storage class are no longer bound to PVs with "" and claimRef will not have any effect on this change.

[thockin]

I think we don't need to make this a behavior change - see my above comments. If we WANT this behavior change, I need you to explain why it is valuable (it is a breaking change, strictly speaking).

[thockin]

I agree that retroactively applying a default seems sane and reasonable. I just don't see why we have to include the breaking-change part.

[jsafrane]

We could argue that even retroactively applying a default SC is a breaking change.

See Alternatives at the bottom of this KEP - it's possible (and actually easier) to keep PVC with nil binding to PV with "", until a default SC is created. After a new default SC is created, all PVCs with nil will get the new default SC and never bind to "".

It was actually our first idea, but then when I saw this, I thought it's too complicated for users. What PVC with nil then means? "I want default SC, if it exists at PVC creation, if it does not exist, then I want a PV with "", and when there is no such PV, then wait for it or for a new default SC."

It will be easier if nil always means "I want the default and wait for it if it does not exist".

[jsafrane]

And we're willing to rewrite this KEP to keep binding nil <-> "" if the current version is too breaking.

[thockin]

I'm always extra paranoid about changing really subtle semantics like this, even when they are weird. It's just so hard to know if it will impact anyone. Here's my thinking:

• Almost everyone uses StoirageClass
• Almost everyone uses dynamic provisioning
• Almost everyone who is NOT using dynamic provisioning is using StorageClass
• Result: the vast vast vast majority of clusters do not have and will never have PVs where class = ""
• Inference: anyone who DOES have such PVs probably relies on all the weird semantics of them

So leaving this functionality (PVCs with class=nil can bind to preallocated class="" PVs) is unlikely to have negative impact on people who don't want that (because almost nobody has those), but changing this is likely to have negative impact on that small subset of users who DO depend on it.

It's yucky from a code and docs POV, but that's what compat is all about.

Does my argument hold water for you?

[RomanBednar]

Thank you very much for your feedback @thockin. Those are all great points! We've changed the KEP to describe the behavior you're suggesting and moved the original proposal to the alternatives section. Please review the latest version and let us know if there's anything else we should add/change.

[ddebroy]

Thanks for the additional clarifications, @RomanBednar ! Looks great.

[thockin]

There are 2 things here:

1. apply default SC to pending PVCs with a nil class (sounds good to me)
2. do not match PVCs with nil class to PVS with "" class (sounds bad to me)

explain why 2 is important, please?

[thockin]

We should change this semantic - while there are two, pick one randomly or heuristically. In normal use it's the same thing as losing a race condition.

E.g., in real-time you have one user changing the default and another user creating a PVC at the same time. Depending on network, CPU scheduling, etc (totally non-deterministic) the API server might see either of those operations first. The PVC will either win the race and get the old default or it will lose the race and get the new default.

So whichever one you pick, as long as it is one of the two defaults, it is a correct choice!

[thockin]

Shouldn't we advise them to make a new SC and flip the default as I described above?

[jsafrane]

No, there may be quota on the original SC. Admins may want to re-create a default SC to keep the quota numbers. Re-create means there is no default SC for a short time.

@RomanBednar perhaps you could add a note about this.

[RomanBednar]

Added.

[thockin]

If we are encouraging people to delete a class and recreate it with different params, we might as well allow them to mutate it. Do we ever refer back to the class after the volume is created?

[jsafrane]

If we are encouraging people to delete a class and recreate it with different params, we might as well allow them to mutate it

+1. Do we need a KEP for this?

[thockin]

I'm not convinced this is right. A PVC with nil class means "I don't care". A PV with class = "" should qualify, shouldn't it?

[jsafrane]

The existing behavior is odd. nil means that "I want default, if it exists when PVC is created, or "" forever". "Don't care" would mean that nil can bind to PV with any storageClassName, which is IMO something else.

(I'm further expanding on this below in https://github.com/kubernetes/enhancements/pull/3337/files#r894338701)

[thockin]

If my comment above is wrong (that "" is a valid match), can you explain why this assertion you make here is important? I know there's a lot of history here :)

[jsafrane]

PV with "" is a valid match for PVC with nil today, and yes, we want to change it. See https://github.com/kubernetes/enhancements/pull/3337/files#r894338701.

[thockin]

I think we don't need to make this a behavior change - see my above comments. If we WANT this behavior change, I need you to explain why it is valuable (it is a breaking change, strictly speaking).

[thockin]

I agree that retroactively applying a default seems sane and reasonable. I just don't see why we have to include the breaking-change part.

[thockin]

kubernetes/kubernetes#110514

[thockin]

If we are encouraging people to delete a class and recreate it with different params, we might as well allow them to mutate it. Do we ever refer back to the class after the volume is created?

[thockin]

Thanks!

/lgtm
/approve

[jsafrane]

/approve
the storage parts
/assign @wojtek-t
for PRR (there were very little changes in PRR questionnaire, if any)

[RomanBednar]

Changing KEP status to implementable.

[jsafrane]

/lgtm

[wojtek-t]

I have two comments about graduation criteria. But approving for PRR now and holding to give you a chance to apply those (without waiting for another pass from me).

/approve PRR
/hold

/label tide/merge-method-squash

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jsafrane, RomanBednar, thockin, wojtek-t

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/prod-readiness/OWNERS [wojtek-t]
• keps/sig-storage/OWNERS [jsafrane]

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

[xing-yang]

/milestone v1.25

[wojtek-t]

/lgtm

/hold cancel

Thanks1

[duanwei33]

@RomanBednar I understand it should be changed to:
Such PVC can be bound only to PV with pv.spec.storageClassName="".