1.25: feature gate cleanup #109435

pohly · 2022-04-12T14:32:02Z

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?

Feature gates that graduated to GA in 1.23 or earlier and were unconditionally enabled have been removed: CSIServiceAccountToken, ConfigurableFSGroupPolicy, EndpointSlice, EndpointSliceNodeName, EndpointSliceProxying, GenericEphemeralVolume, IPv6DualStack, IngressClassNamespacedParams, StorageObjectInUseProtection, TTLAfterFinished, VolumeSubpath, WindowsEndpointSliceProxying

fedebongio · 2022-04-12T20:10:37Z

/sig architecture
/remove-sig api-machinery
/cc @deads2k @thockin @liggitt

mtaufen · 2022-04-12T20:42:41Z

/triage accepted
/priority important-longterm

mtaufen · 2022-04-12T20:44:31Z

We should also include a presubmit test that ensures these files remain sorted.

mtaufen · 2022-04-12T20:46:33Z

A tool for sorting it would be useful also.

mtaufen · 2022-04-12T20:46:38Z

/lgtm

tallclair · 2022-04-13T21:58:12Z

staging/src/k8s.io/apiserver/pkg/features/kube_features.go

-	APIServerIdentity:                   {Default: false, PreRelease: featuregate.Alpha},
-	APIServerTracing:                    {Default: false, PreRelease: featuregate.Alpha},
-	OpenAPIEnums:                        {Default: true, PreRelease: featuregate.Beta},
+	APIListChunking: {Default: true, PreRelease: featuregate.Beta},


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.

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.

@tallclair: ping ^^^

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.

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.

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?

@liggitt ?? ^

pohly · 2022-05-04T05:53:22Z

... 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.

pohly · 2022-05-05T06:48:29Z

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:

$ for i in $(find . -name kube_features.go); do echo $i:; diff --ignore-all-space <(git show origin/master:$i | sort -u)  <(git show HEAD~:$i | sort -u); done
./pkg/features/kube_features.go:
6a7
> //
10a12
> 	// across the file.
244a247
> // Entries are separated from each other with blank lines to avoid sweeping gofmt changes
259a263
> 	// Feature gates should be listed in alphabetical, case-sensitive
413a418
> 	// of code conflicts because changes are more likely to be scattered
581a587
> 	// (upper before any lower case character) order. This reduces the risk
588a595
> // when adding or removing one entry.
./staging/src/k8s.io/component-base/logs/kube_features.go:
./staging/src/k8s.io/controller-manager/pkg/features/kube_features.go:
6a7
> 	// across the file.
20a22
> 	// Feature gates should be listed in alphabetical, case-sensitive
30c32,33
< 	// MyFeature() bool
---
> 	// MyFeature featuregate.Feature = "MyFeature"
> 	// of code conflicts because changes are more likely to be scattered
39a43
> 	// (upper before any lower case character) order. This reduces the risk
./staging/src/k8s.io/apiextensions-apiserver/pkg/features/kube_features.go:
./staging/src/k8s.io/apiserver/pkg/features/kube_features.go:
6a7
> 	// across the file.
72a74
> 	// Feature gates should be listed in alphabetical, case-sensitive
89c91,92
< 	// MyFeature() bool
---
> 	// MyFeature featuregate.Feature = "MyFeature"
> 	// of code conflicts because changes are more likely to be scattered
135a139
> 	// (upper before any lower case character) order. This reduces the risk

@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

pohly · 2022-05-05T08:34:39Z

/retest

liggitt · 2022-05-05T14:08:09Z

verified diff:

diff <(git show upstream/master:pkg/features/kube_features.go | egrep "featuregate.Feature =|Default:" | sed -E 's/\s+/ /g' | sort) \
     <(git show HEAD:pkg/features/kube_features.go | egrep "featuregate.Feature =|Default:" | sed -E 's/\s+/ /g' | sort)

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

diff <(git show upstream/master:staging/src/k8s.io/apiserver/pkg/features/kube_features.go | egrep "featuregate.Feature =|Default:" | sed -E 's/\s+/ /g' | sort) \
     <(git show HEAD:staging/src/k8s.io/apiserver/pkg/features/kube_features.go | egrep "featuregate.Feature =|Default:" | sed -E 's/\s+/ /g' | sort)

0a1
>  // MyFeature featuregate.Feature = "MyFeature"

liggitt · 2022-05-05T14:08:38Z

/lgtm
/approve

k8s-ci-robot · 2022-05-05T14:08:56Z

[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:

~~pkg/features/OWNERS~~ [liggitt]
~~staging/src/k8s.io/apiserver/pkg/features/OWNERS~~ [liggitt]
~~staging/src/k8s.io/controller-manager/pkg/features/OWNERS~~ [liggitt]

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

k8s-ci-robot requested review from andrewsykim and smarterclayton April 12, 2022 14:32

k8s-ci-robot requested review from deads2k, liggitt and thockin April 12, 2022 20:10

k8s-ci-robot added sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. and removed sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. labels Apr 12, 2022

k8s-ci-robot assigned mtaufen Apr 12, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 12, 2022

tallclair reviewed Apr 13, 2022

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 15, 2022

pohly force-pushed the feature-gates-sorting branch from 7dbff2c to ea29219 Compare May 5, 2022 06:47

k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 5, 2022

pohly force-pushed the feature-gates-sorting branch from ea29219 to 8e56b1e Compare May 5, 2022 07:39

k8s-ci-robot assigned liggitt May 5, 2022

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels May 5, 2022

k8s-ci-robot merged commit 0bd2847 into kubernetes:master May 5, 2022

k8s-ci-robot added this to the v1.25 milestone May 5, 2022

ialidzhikov mentioned this pull request May 5, 2022

Remove the IngressClassNamespacedParams feature gate #108375

Closed

pohly mentioned this pull request May 6, 2022

feature gates: define name and spec next to each other #109865

Closed

jackfrancis mentioned this pull request May 9, 2022

chore: remove GA IPv6DualStack feature-gate kubernetes-sigs/cloud-provider-azure#1701

Merged

github-actions bot mentioned this pull request May 11, 2022

Week Ending May 8, 2022 dev-obs/actus#434

Open

aojea mentioned this pull request Jun 2, 2022

Implement KEP-3178 "iptables cleanup" in kubelet #110291

Merged

willzhang mentioned this pull request Nov 9, 2022

unrecognized feature gate: TTLAfterFinished kubesphere/kubekey#1593

Closed

BenTheElder mentioned this pull request Nov 11, 2022

Config with EndpointSlice fails kubernetes-sigs/kind#2997

Closed

This was referenced Aug 1, 2023

"Ephemeral Volumes" page references the usage of a deprecated feature gate GenericEphemeralVolume kubernetes/website#42259

Closed

Update ephemeral volumes feature state kubernetes/website#42317

Merged

liggitt mentioned this pull request Dec 22, 2023

Fix feature gates #122442

Closed

nprokopic mentioned this pull request Jan 24, 2024

Add podSecurityStandards.enforced giantswarm/cluster#44

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

1.25: feature gate cleanup #109435

1.25: feature gate cleanup #109435

pohly commented Apr 12, 2022 •

edited by liggitt

fedebongio commented Apr 12, 2022

mtaufen commented Apr 12, 2022

mtaufen commented Apr 12, 2022

mtaufen commented Apr 12, 2022

mtaufen commented Apr 12, 2022

tallclair Apr 13, 2022

pohly Apr 14, 2022

pohly May 4, 2022

mtaufen May 4, 2022 •

edited

tallclair May 4, 2022

pohly May 5, 2022

aojea May 5, 2022

pohly commented May 4, 2022

pohly commented May 5, 2022

pohly commented May 5, 2022

liggitt commented May 5, 2022

liggitt commented May 5, 2022

k8s-ci-robot commented May 5, 2022

1.25: feature gate cleanup #109435

1.25: feature gate cleanup #109435

Conversation

pohly commented Apr 12, 2022 • edited by liggitt

What type of PR is this?

What this PR does / why we need it:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

fedebongio commented Apr 12, 2022

mtaufen commented Apr 12, 2022

mtaufen commented Apr 12, 2022

mtaufen commented Apr 12, 2022

mtaufen commented Apr 12, 2022

tallclair Apr 13, 2022

Choose a reason for hiding this comment

pohly Apr 14, 2022

Choose a reason for hiding this comment

pohly May 4, 2022

Choose a reason for hiding this comment

mtaufen May 4, 2022 • edited

Choose a reason for hiding this comment

tallclair May 4, 2022

Choose a reason for hiding this comment

pohly May 5, 2022

Choose a reason for hiding this comment

aojea May 5, 2022

Choose a reason for hiding this comment

pohly commented May 4, 2022

pohly commented May 5, 2022

pohly commented May 5, 2022

liggitt commented May 5, 2022

liggitt commented May 5, 2022

k8s-ci-robot commented May 5, 2022

pohly commented Apr 12, 2022 •

edited by liggitt

mtaufen May 4, 2022 •

edited