-
Notifications
You must be signed in to change notification settings - Fork 38.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add DataSourceRef field to PVC spec #103276
Conversation
This PR may require API review. If so, when the changes are ready, complete the pre-review checklist and request an API review. Status of requested reviews is tracked in the API Review project. |
/assign @thockin |
d6eb9a7
to
3bdb330
Compare
/remove-sig api-machinery |
/assign @saad-ali |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll be online off and on today and tomorrow. If you want to make an argument for "the tests work, let's merge it and then fix this" I am open to it, but I really REALLY think it should per made more permanent, since it is now part of the defined semantics.
@thockin Thanks I've been working on refactoring this code and adding more unit tests today. I'll send up another version trying to incorporate all your suggestions. The one tricky one will be the DropDisabledFields() implementation -- I understand it's intended to hold the feature-gated logic, but the existing implementation already doesn't do that. Given the ugly reality of this I will try to hold to the spirit of that goal and you can tell me what you think. I'll ping you when I push another commit this afternoon. |
That last commit represents the changes to DropDisabledFields() and more tests. I still haven't reworded the comments/docs for the new core field. That will come next. |
The most recent commit also updates the doc comments, and should cover all of the feedback so far. Please let me know if the rewrite of DropDisabledFields() is clear enough and follows the spirit of the feature-gate rule. The original logic was flawed so this new version is not only more readable but also more functional. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This actually pushed me farther down the "move it" road. IIUC, updates of PVC can't change the data source, so having this logic in DropDisabled, which has an "old" and "new" param is extra confusing. Putting it in Normalize, which is only called on Create will make it that much more obvious what the heck is going on.
See, there's a story that you need to tell here. 3, 4, 5 years from now, someone will be staring at this and asking "WTF?!", running git blame
and muttering about you. Very possibly that someone will be you.
I think you should move it, and tell the whole story in Normalize. You are swimming upstream.
If you need to merge this and followup, I could buy that, but I REALLY think we will regret not doing it right.
@@ -39,26 +65,21 @@ func dataSourceInUse(oldPVCSpec *core.PersistentVolumeClaimSpec) bool { | |||
if oldPVCSpec == nil { | |||
return false | |||
} | |||
if oldPVCSpec.DataSource != nil { | |||
if oldPVCSpec.DataSource != nil || oldPVCSpec.DataSourceRef != nil { | |||
return true | |||
} | |||
return false | |||
} | |||
|
|||
func dataSourceIsEnabled(pvcSpec *core.PersistentVolumeClaimSpec) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IsEnabled is now a total misnomer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DropDisabled is called by Create and Update, and Normalize is called by Create and Get/List. I'm not opposed to more refactoring, but I think we'll end up with a 3rd function and the unit tests will need a complete rewrite as a result.
I also agree that the existing method names suck, but there's a tension between wanting to clean up bad code and wanting to avoid fixing stuff that's not broken.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is broken, it's just broken in human-space not compiler-space :)
Okay I have split most of the code in DropDisabledFields into another function, so Drop just handles the feature gate dropping. The new function is called EnforceDataSourceBackwardsCompatibility (long, but hopefully not confusing). I don't think it's possible to combine this new function with NormalizeDataSources, because Normalize is also used on the Get/List code path. Lots of unit tests needed updating. And also I realized that updates needed some work so I fixed that code path too. In particular we have to be careful not to break any updates which would have previously worked, given that the validation code prevents mutations of PVC specs, and we're playing a lot of games with automatically modifying these fields. I still want to add a raft of E2E tests that cover the combination of all the changes, to try to smoke out any corner cases, especially around combinations of updates and changing of the feature gate from off to on or on to off. |
/retest |
// to maintain backwards compatibility with old behavior. | ||
// See KEP 1495 for details. | ||
// Specifically, if this is an update of a PVC with no data source, or a creation of a new PVC, | ||
// and either the AnyVolumeDataSource feature gate is disabled or the dataSourceRef field is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitty: I wouldn't mention the feature gate here - it's not actually checked and it is irrelevant. The field is either in use or not. The gate is handled elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed in slack - this is the right place to tell the full story about why it is called on both create and update paths, and how it handles back-rev clients
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM from API POV
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bswartz, thockin 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:
Approvers can indicate their approval by writing |
I'm going to squash the commits into one so it can merge |
Modify the behavior of the AnyVolumeDataSource alpha feature gate to enable a new field, DataSourceRef, rather than modifying the behavior of the existing DataSource field. This allows addition Volume Populators in a way that doesn't risk breaking backwards compatibility, although it will result in eventually deprecating the DataSource field.
Squashed |
/lgtm |
/milestone v1.22 |
/triage accepted |
What type of PR is this?
/kind feature
/kind api-change
What this PR does / why we need it:
Modify the behavior of the AnyVolumeDataSource alpha feature gate to enable
a new field, DataSourceRef, rather than modifying the behavior of the
existing DataSource field. This allows addition Volume Populators in a way
that doesn't risk breaking backwards compatibility, although it will
result in eventually deprecating the DataSource field.
Which issue(s) this PR fixes:
Fixes #103275
Special notes for your reviewer:
A lot of test cases have been added, but I think more could be added. I'm going to keep working on those which the change itself is reviewed.
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: