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
1.25: feature gate cleanup #109435
1.25: feature gate cleanup #109435
Conversation
/triage accepted |
We should also include a presubmit test that ensures these files remain sorted. |
A tool for sorting it would be useful also. |
/lgtm |
APIServerIdentity: {Default: false, PreRelease: featuregate.Alpha}, | ||
APIServerTracing: {Default: false, PreRelease: featuregate.Alpha}, | ||
OpenAPIEnums: {Default: true, PreRelease: featuregate.Beta}, | ||
APIListChunking: {Default: true, PreRelease: featuregate.Beta}, |
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 think we could get rid of the need to list the features here with something like this:
type FeatureSet struct{
features map[Feature]FeatureSpec
}
func (f *FeatureSet) Register(feature Feature, default bool, prerelease prerelease) Feature {
f.features[feature] = FeatureSpec{Default: default, PreRelease: prerelease}
return feature
}
Then the feature declarations just call Register
on the default feature set, e.g.
// owner: @smarterclayton
// alpha: v1.8
// beta: v1.9
//
// Allow API clients to retrieve resource lists in chunks rather than
// all at once.
- APIListChunking featuregate.Feature = "APIListChunking"
+ APIListChunking = defaultKubernetesFeatureGates.Register("APIListChunking", true, featuregate.Beta)
I also like that this keeps the state next to the definition.
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 had the same idea, but didn't pursue it because of one drawback: APIListChunking
has to become a global variable instead of a constant, right?
Is that relevant or can we trust all code to never write to these variables?
The Register
also needs to be a bit more complicated or we need additional flavors of it because some feature gates also have LockToDefault: true
.
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.
@tallclair: ping ^^^
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.
Maybe we should err on the side of splitting sorting the file from refactoring the feature gate mechanism into separate PRs. Hard to review both at once.
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.
Yeah, I'm in favor of moving forward with this change and punting on the larger refactor.
@pohly that's a good point about non-const (I really with go had immutability...). You could probably do something hacky like:
// ...
const APIListChunking featuregate.Feature = "APIListChunking"
var _ = defaultKubernetesFeatureGates.Register(APIListChunking, true, featuregate.Beta)
Note that var _ =
is required to call the function outside a method, hence the hackiness.
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.
That's doable and looks like an improvement over the current approach. I can follow up with that in a second PR once this one has been merged.
Who can provide LGTM and approval?
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.
@liggitt ?? ^
... and we have merge conflicts 😢 I'll wait a bit for merging of the backlog to settled down and then update, but can we then merge this quickly? |
Merge conflicts become less likely when: - features are sorted alphabetically because then changes are more likely to be done in different parts of the files - blank lines separate the hash entries because gofmt then doesn't change the formating of other entries when adding or removing one Merge conflicts where pretty common shortly before a code freeze when everyone added new features at the end of the files.
7dbff2c
to
ea29219
Compare
I've rebased. The first commit is impossible to review manually, so here's some evidence that it really only shuffles lines around. The only non-whitespace changes are the extra comment lines that I added about sorting:
@mtaufen : can you take another look? |
This removes all feature definitions that were marked for removal in 1.25, with some exceptions: - some features were marked for removal in 1.25 although they only graduated to GA in 1.24 - that seems too soon, so the comment was updated instead - CSIVolumeFSGroupPolicy and PodDisruptionBudget are still used in the code, so removing them will be more work and was deferred
ea29219
to
8e56b1e
Compare
/retest |
verified diff:
38,39d37
< CSIServiceAccountToken featuregate.Feature = "CSIServiceAccountToken"
< CSIServiceAccountToken: {Default: true, PreRelease: featuregate.GA, LockToDefault: true}, // remove in 1.23
72,77d69
< EndpointSlice featuregate.Feature = "EndpointSlice"
< EndpointSlice: {Default: true, PreRelease: featuregate.GA, LockToDefault: true}, // remove in 1.25
< EndpointSliceNodeName featuregate.Feature = "EndpointSliceNodeName"
< EndpointSliceNodeName: {Default: true, PreRelease: featuregate.GA, LockToDefault: true}, // remove in 1.25
< EndpointSliceProxying featuregate.Feature = "EndpointSliceProxying"
< EndpointSliceProxying: {Default: true, PreRelease: featuregate.GA, LockToDefault: true}, // remove in 1.25
85c77
< ExpandCSIVolumes: {Default: true, PreRelease: featuregate.GA}, // remove in 1.25
---
> ExpandCSIVolumes: {Default: true, PreRelease: featuregate.GA}, // remove in 1.26
87c79
< ExpandInUsePersistentVolumes: {Default: true, PreRelease: featuregate.GA}, // remove in 1.25
---
> ExpandInUsePersistentVolumes: {Default: true, PreRelease: featuregate.GA}, // remove in 1.26
89c81
< ExpandPersistentVolumes: {Default: true, PreRelease: featuregate.GA}, // remove in 1.25
---
> ExpandPersistentVolumes: {Default: true, PreRelease: featuregate.GA}, // remove in 1.26
96,97d87
< GenericEphemeralVolume featuregate.Feature = "GenericEphemeralVolume"
< GenericEphemeralVolume: {Default: true, PreRelease: featuregate.GA, LockToDefault: true}, // remove in 1.25
108,109d97
< IPv6DualStack featuregate.Feature = "IPv6DualStack"
< IPv6DualStack: {Default: true, PreRelease: featuregate.GA, LockToDefault: true}, // remove in 1.25
130,131d117
< IngressClassNamespacedParams featuregate.Feature = "IngressClassNamespacedParams"
< IngressClassNamespacedParams: {Default: true, PreRelease: featuregate.GA, LockToDefault: true}, // remove in 1.25
173c159
< NonPreemptingPriority: {Default: true, PreRelease: featuregate.GA, LockToDefault: true}, // remove in 1.25
---
> NonPreemptingPriority: {Default: true, PreRelease: featuregate.GA, LockToDefault: true}, // remove in 1.26
187c173
< PreferNominatedNode: {Default: true, PreRelease: featuregate.GA, LockToDefault: true}, // remove in 1.25
---
> PreferNominatedNode: {Default: true, PreRelease: featuregate.GA, LockToDefault: true}, // remove in 1.26
218,219d203
< StorageObjectInUseProtection featuregate.Feature = "StorageObjectInUseProtection"
< StorageObjectInUseProtection: {Default: true, PreRelease: featuregate.GA, LockToDefault: true}, // remove in 1.25
222,223d205
< TTLAfterFinished featuregate.Feature = "TTLAfterFinished"
< TTLAfterFinished: {Default: true, PreRelease: featuregate.GA, LockToDefault: true}, // remove in 1.25
230,231d211
< VolumeSubpath featuregate.Feature = "VolumeSubpath"
< VolumeSubpath: {Default: true, PreRelease: featuregate.GA, LockToDefault: true}, // remove in 1.25
236,237d215
< WindowsEndpointSliceProxying featuregate.Feature = "WindowsEndpointSliceProxying"
< WindowsEndpointSliceProxying: {Default: true, PreRelease: featuregate.GA, LockToDefault: true}, // remove in 1.25
0a1
> // MyFeature featuregate.Feature = "MyFeature" |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: liggitt, pohly 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 |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Sorting is meant to decrease the risk of code conflicts during the period before code freeze.
Removal of GA features is part of the usual maintenance and was due for several features in 1.25.
Special notes for your reviewer:
Discussed here: https://groups.google.com/g/kubernetes-sig-architecture/c/7J6_YYvdugc/m/rr1jde6JBgAJ
Does this PR introduce a user-facing change?