[wip] Thockin hack on deads2k gates branch #9
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
deads2k
commented
Apr 10, 2024
pkg/api/pod/util.go
Outdated
| @@ -685,7 +686,8 @@ func dropDisabledFields( | |||
| // For other types of containers, validateContainers will handle them. | |||
| } | |||
|
|
|||
| if !utilfeature.DefaultFeatureGate.Enabled(features.RecursiveReadOnlyMounts) && !rroInUse(oldPodSpec) { | |||
| if !utilfeature.DefaultFeatureGate.Enabled(features.RecursiveReadOnlyMounts) && | |||
| !feature.Enabled(features.RecursiveReadOnlyMounts3) && !rroInUse(oldPodSpec) { | |||
There was a problem hiding this comment.
I think in my refactor this became !features.RecursiveReadOnlyMounts3.Enabled() . I guess the advantage in this refactor is that every featuregate has the env var assignment? How attached are you to the feature.Enabled ?
deads2k
force-pushed
the
featuregate-rewrite-01-experiment
branch
from
b93f54c
to
e6a5bcd
Compare
deads2k
commented
Apr 10, 2024
| if !utilfeature.DefaultFeatureGate.Enabled(features.StorageVersionMigrator) || | ||
| !clientgofeaturegate.InformerResourceVersion2.Enabled() { | ||
| if !oldutilfeature.DefaultFeatureGate.Enabled(features.StorageVersionMigrator) && | ||
| !feature.Enabled(clientgofeaturegate.InformerResourceVersion3) { |
There was a problem hiding this comment.
I think in my refactor this became !features.RecursiveReadOnlyMounts3.Enabled(). I guess the advantage in this refactor is that every featuregate has the env var assignment? How attached are you to the feature.Enabled ?
There was a problem hiding this comment.
I don't see why we wouldn't want ALL gates to have the same enablement options?
feature.Enabled( syntax was JUST to put the verb at the front of the LOC. I find it easier to read as:
if !feature.Enabled(longpkgname.bigLongVariableName) {
than
if !longpkgname.bigLongVariableName.Enabled() {
That's REALLY the only argument :)
deads2k
commented
Apr 10, 2024
Comment on lines
+33
to
+32
| //FIXME: unclear if we want to make the API in terms of "Gates" or "Features". | ||
| //"Features" is so abstract... |
There was a problem hiding this comment.
I prefer the more explicit FeatureGate and FeatureGateSet (or FeatureSet)
There was a problem hiding this comment.
I was trying to follow the "don't stutter" approach. feature.Gate vs feature.FeatureGate or heaven forbid featuregate.FeatureGate.
deads2k
commented
Apr 10, 2024
| @@ -68,6 +81,11 @@ func LibraryFeatureSet() featuregates.FeatureSet { | |||
| return libraryFeatureSet | |||
| } | |||
|
|
|||
| // FIXME: rename to FeatureGates when other definition is gone. | |||
| func LibFeatureGates() *feature.GateSet { | |||
There was a problem hiding this comment.
I see an advantage to using an interface over a concrete type here so that in composition people can say, "David and Tim won't are too slow to merge my awesome way to set FeatureGates, so I'll provide a compatible implementation myself!" and anything accepting a GateSet will "just work".
There was a problem hiding this comment.
I see that as exceedingly unlikely, or at least not super compelling vs. the complexity. Using interfaces has a few costs, IMO:
- Harder to keep in brain
- MUCH harder to back-track to "where did I get called from"
- Requires everything be methods, meaning the types get more obtuse
- Encourages defining more and more interfaces when a "dumb" struct would do
- Encourages "SimpleFeatureGate" stuff which makes it more difficult again to track
Aside: I looked again at your branch and I honestly have no idea what a private method in a public interface means :)
I won't die on this hill, probably, but this seems like the sort of API where simpler really is better.
I'll try to take a pass over this variation and see if there's a not-so-bad way to have the best of both
…go-rest-client-watch-list add watchlist to client-go rest client
….5.13 etcd: update to v3.5.13
Pinning apidiff to a specific version shouldn't be necessary because, if past experience in klog holds true, the latest version just works. This way we don't have to remember to bump up the revision. The downside of using "latest" is that a compromise of that version would give an attacker the ability to run code in the CI and on developer machines.
Refactor the TestValidateKubeProxyConfiguration by adding a mutating function that adjusts the configuration according to each test case, thereby enhancing readability. Signed-off-by: Daman Arora <aroradaman@gmail.com>
During upgrade apply we had logic to download the kubelet and kubeproxy configs from the cluster as part of the call to: FetchInitConfigurationFromCluster() With the introduction of UpgradeConfiguration there was some refactor in this area and the function no longer had the argument skipComponentConfigs set to false. It is set to 'true', an InitConfiguration is downloaded but it would contain empty / defaulted component configs. - Set the argument to 'false' - Perform minor cleanup of STDOUT messages and comments. that were missed in 1.30.
Allow bookmark events in between delete/modify in testSimpleCRUD
fix e2e loadbalancer test timeouts and assumptions
don't force delete pods on e2e tests
use latest stable version of kube-network-policies for CI
Add wangzhen127 back to approvers and reviewers for NPD
DRA: API cleanup + fix
add coverage tests for probes behavior
…se/t/transformation_tests_parallel Revert "Run `controlplane/transformation` integration tests in parallel"
Change backOffPeriod from const to var, so that it can be set lower during unit test.
feature: add name formats library to CEL
…onfig removed this k8s.io/kubernetes/pkg/apis/componentconfig from verify-…
…-svc Use endpointSlices for describing service endpoints
…containers-lifecycle node_e2e: refactor RunTogether function
Promote RetryGenerateName to beta
Use the generic/typed workqueue throughout
…letion Add completion for kubectl set image
….io-components Include k8s.io components with contextual logging in logcheck.conf
Signed-off-by: Monis Khan <mok@microsoft.com>
scheduler: preallocation for NodeToStatusMap
Sync `go.work.sum` of kms mock plugin
…iration-v1beta4 kubeadm: add support for custom cert validity period in v1beta4
Promote custom resource field selectors to beta
thockin
force-pushed
the
thockin-hack-on-deads2k-gates-branch
branch
2 times, most recently
from
f2920cd
to
56714f9
Compare
thockin
force-pushed
the
thockin-hack-on-deads2k-gates-branch
branch
from
56714f9
to
f7a2360
Compare
$ go list -json k8s.io/utils/ptr | jq .Dir "/home/thockin/src/kubernetes/staging/src/k8s.io/utils/ptr"
thockin
force-pushed
the
thockin-hack-on-deads2k-gates-branch
branch
from
f7a2360
to
a2e702e
Compare
This commit demonstrate how libraries can define and expose Gates and GateSets, and compose GateSets into new GateSets. It also defines a small number of new-style gates, but does not use them yet.
thockin
force-pushed
the
thockin-hack-on-deads2k-gates-branch
branch
from
a2e702e
to
7a5a09e
Compare
TODO: incomplete
thockin
force-pushed
the
thockin-hack-on-deads2k-gates-branch
branch
from
7a5a09e
to
ae307ea
Compare
Notes:
Some of these parse into a map in their config and then call SetFromMap -- need to retain ability to pass in via config.
Option 1: make a `feature.EnableMapControl()`
Option 2: pass map into `feature.EnablePFlagControl()`
Option 3: `feature.GetAllGates(feature.FromFlags(flagName), feature.FromEnv(prefix))`
This is probablay most correct today and makes validation easy,
since we do it exactly once. A little awkward with any dynamic
source (e.g. API).
Should env enablement be part of "AddFlags" logic in these components?
It's a little muddy how apps are configured.
```
$ for f in _output/bin/*; do X=$($f --help 2>&1 | grep gates); if [[ -z "$X" ]]; then X=$($f init --help 2>&1 | grep gates); fi; if [[ -n "$X" ]]; then echo; echo "$f"; echo "$X"; fi; done
_output/bin/apiextensions-apiserver
--feature-gates mapStringBool 4444 A set of key=value pairs that describe feature gates for alpha/experimental features. Options are:
--new-feature-gates mapStringBool A set of key=value pairs that describe feature gates for alpha/experimental features. Options are:
_output/bin/e2e_node.test
--feature-gates mapStringBool 6666 A set of key=value pairs that describe feature gates for alpha/experimental features.
--service-feature-gates mapStringBool 7777 A set of key=value pairs that describe feature gates for alpha/experimental features for API service.
_output/bin/kubeadm
--feature-gates string 2222 A set of key=value pairs that describe feature gates for various features. Options are:
--new-feature-gates mapStringBool A set of key=value pairs that describe feature gates for alpha/experimental features. Options are:
_output/bin/kube-aggregator
--feature-gates mapStringBool 4444 A set of key=value pairs that describe feature gates for alpha/experimental features. Options are:
--new-feature-gates mapStringBool A set of key=value pairs that describe feature gates for alpha/experimental features. Options are:
_output/bin/kube-apiserver
--feature-gates mapStringBool 4444 A set of key=value pairs that describe feature gates for alpha/experimental features. Options are:
--new-feature-gates mapStringBool A set of key=value pairs that describe feature gates for alpha/experimental features. Options are:
_output/bin/kube-controller-manager
--feature-gates mapStringBool 4444 A set of key=value pairs that describe feature gates for alpha/experimental features. Options are:
--new-feature-gates mapStringBool A set of key=value pairs that describe feature gates for alpha/experimental features. Options are:
_output/bin/kubelet
--feature-gates mapStringBool 3333 A set of key=value pairs that describe feature gates for alpha/experimental features. Options are:
--new-feature-gates mapStringBool A set of key=value pairs that describe feature gates for alpha/experimental features. Options are:
_output/bin/kube-proxy
--feature-gates mapStringBool 1111 A set of key=value pairs that describe feature gates for alpha/experimental features. Options are:
--new-feature-gates mapStringBool A set of key=value pairs that describe feature gates for alpha/experimental features. Options are:
_output/bin/kube-scheduler
--feature-gates mapStringBool 4444 A set of key=value pairs that describe feature gates for alpha/experimental features. Options are:
--new-feature-gates mapStringBool A set of key=value pairs that describe feature gates for alpha/experimental features. Options are:
```
thockin
force-pushed
the
thockin-hack-on-deads2k-gates-branch
branch
from
17754df
to
0cdd003
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
opening this because I cannot seem to comment on diffs