Make feature gates loadable from a map[string]bool #53025

mtaufen · 2017-09-25T23:26:14Z

Command line flag API remains the same. This allows ComponentConfig
structures (e.g. KubeletConfiguration) to express the map structure
behind feature gates in a natural way when written as JSON or YAML.

For example:

KubeletConfiguration Before:

apiVersion: kubeletconfig/v1alpha1
kind: KubeletConfiguration
featureGates: "DynamicKubeletConfig=true,Accelerators=true"

KubeletConfiguration After:

apiVersion: kubeletconfig/v1alpha1
kind: KubeletConfiguration
featureGates:
  DynamicKubeletConfig: true
  Accelerators: true

Fixes: #53024

The Kubelet's feature gates are now specified as a map when provided via a JSON or YAML KubeletConfiguration, rather than as a string of key-value pairs.

/cc @mikedanese @jlowdermilk @smarterclayton

mtaufen · 2017-09-25T23:29:39Z

whoops, forgot to update bazel, one sec

mtaufen · 2017-09-25T23:38:12Z

/cc @ncdc

mtaufen · 2017-09-26T03:21:37Z

/retest

ncdc

Do you also want to make a similar change to the feature gates field in the kube proxy config?

ncdc · 2017-09-27T14:08:01Z

staging/src/k8s.io/apiserver/pkg/util/flag/map_string_bool.go

+		if v {
+			vstr = "true"
+		}
+		pairs = append(pairs, fmt.Sprintf("%s=%s", k, vstr))


Can't you use %t to format the bool?

Oh, yeah! Thanks!

ncdc · 2017-09-27T14:09:48Z

staging/src/k8s.io/apiserver/pkg/util/flag/map_string_bool_test.go

+		{"two keys", MapStringBool{"one": true, "two": false}, "one=true,two=false"},
+	}
+	for _, c := range cases {
+		str := c.m.String()


I'd recommend using subtests so you don't have to print the case name in failures

Oh that looks like quite a nice way to do things, hadn't heard about those before.

ncdc · 2017-09-27T14:10:04Z

staging/src/k8s.io/apiserver/pkg/util/flag/map_string_bool_test.go

+	}
+	for _, c := range cases {
+		str := c.m.String()
+		if str != c.expect {


Use testify's assert.Equal?

ncdc · 2017-09-27T14:10:22Z

staging/src/k8s.io/apiserver/pkg/util/flag/map_string_bool_test.go

+		{"non-boolean value", "one=foo", MapStringBool{}, `invalid value of one: foo, err: strconv.ParseBool: parsing "foo": invalid syntax`},
+	}
+	for _, c := range cases {
+		m := MapStringBool{}


Again recommend subtests

ncdc · 2017-09-27T14:14:45Z

staging/src/k8s.io/apiserver/pkg/util/flag/map_string_bool_test.go

+	for _, c := range cases {
+		m := MapStringBool{}
+		err := m.Set(c.val)
+		if err != nil {


I frequently will do something like this:

if c.err != "" { require.EqualError(t, err, c.err) } else { require.NoError(t, err) }

ncdc · 2017-09-27T14:14:54Z

staging/src/k8s.io/apiserver/pkg/util/flag/map_string_bool_test.go

+			}
+			continue
+		}
+		if !reflect.DeepEqual(m, c.expect) {


Could use assert.Equal

mtaufen · 2017-09-27T16:04:50Z

Thanks, that test looks a lot cleaner!
Let's make the change to kube-proxy in a follow-up PR.

ncdc · 2017-09-27T16:07:39Z

LGTM but would like 1 more reviewer

dixudx · 2017-09-28T03:30:41Z

staging/src/k8s.io/apiserver/pkg/util/flag/map_string_bool.go

+	return strings.Join(pairs, ",")
+}
+
+func (m MapStringBool) Set(value string) error {


Add comment "// Set function of flag.Value" ?

mikedanese · 2017-09-29T23:21:02Z

staging/src/k8s.io/apiserver/pkg/util/flag/map_string_bool_test.go

+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"


I really dislike assert libraries. I want to spend a week and remove these from our dependencies. Generic assert helpers are against the internal style guide. https://github.com/golang/go/wiki/CodeReviewComments#useful-test-failures

It may be tempting to write a bunch of assertFoo helpers, but be sure your helpers produce useful error messages.

It doesn't sound against the style guide to me, as long as the assert helpers produce useful errors. I'm easy either way though.

They are against the internal style guide, not the external one. This wouldn't get past a go readability review.

@mikedanese I would strongly prefer keeping them. These helpers make writing test code so much cleaner (as long as the error messages are useful).

All of the assertions in this test can be written in plain go with equivalent LOC. Golang has fine support for everything here. What value does a seperate testing sublanguage add? You inevitably duplicate a bunch of features to write imprecise tests that omit useful information from error messages.

https://golang.org/doc/faq#testing_framework

Compare something that's fairly common in our codebase:

if e, a := "someExpectedValue", someObj.someField; e != a { t.Errorf("someField: expected %v, got %v", e, a) }

with

assert.Equal(t, "someExpectedValue", someObj.someField, "unexpected someField value")

I find the latter nicer, since it's 1 line instead of 3. How do you do that in 1 without an assertion library?

I am not going to fight tooth and nail over this - there are more important things to argue over 😄 . Given that some tests do use testify, and others don't, I think this is probably something that needs to be decided by more than just 1 or 2 of us.

mikedanese · 2017-09-29T23:22:51Z

cmd/kubelet/app/options/options.go

@@ -317,9 +317,8 @@ func AddKubeletConfigFlags(fs *pflag.FlagSet, c *kubeletconfig.KubeletConfigurat
 	fs.Int32Var(&c.ImageGCLowThresholdPercent, "image-gc-low-threshold", c.ImageGCLowThresholdPercent, "The percent of disk usage before which image garbage collection is never run. Lowest disk usage to garbage collect to.")
 	fs.DurationVar(&c.VolumeStatsAggPeriod.Duration, "volume-stats-agg-period", c.VolumeStatsAggPeriod.Duration, "Specifies interval for kubelet to calculate and cache the volume disk usage for all pods and volumes.  To disable volume calculations, set to 0.")
 	fs.StringVar(&c.VolumePluginDir, "volume-plugin-dir", c.VolumePluginDir, "<Warning: Alpha feature> The full path of the directory in which to search for additional third party volume plugins")
-	fs.StringVar(&c.FeatureGates, "feature-gates", c.FeatureGates, "A set of key=value pairs that describe feature gates for alpha/experimental features. "+
+	fs.Var(flag.MapStringBool(c.FeatureGates), "feature-gates", "A set of key=value pairs that describe feature gates for alpha/experimental features. "+


Does this actually work? If so then why do we do the stuff above (L312)?

Does this work because of the defaulting?

Yeah, it works, I can run the dynamic kubelet config tests with this patch, with the feature gate set via the command line. I pinged @smarterclayton about L312 a few days ago, he said it had been too long since he wrote it to really remember, but that it might have either been an early go convention before we switched to plfag/cobra or that we may have been initializing it lazily at one point, then refactored it out (but probably the former).

I'm fairly certain if you removed the defaulting, you would get a nil pointer dereference and that's why that is there. But sounds good.

Interesting point. I think that might be true. But shouldn't be an issue, since we'll always default before applying the flags.

mikedanese · 2017-09-29T23:27:26Z

staging/src/k8s.io/apiserver/pkg/util/feature/feature_gate.go

+	f.lock.Lock()
+	defer f.lock.Unlock()
+
+	for k, v := range m {


Does order matter in our implementation?

Since you can only set a given key once from a single map, I'm not sure it matters for SetFromMap.

With Set, which takes a string, I could see a bunch of parameters being concatenated while someone builds a command line, and some later ones overriding some previous ones. But that matters for CLI only.

I don't think order would matter for anything else. Feature gates are stored internally as a map, so any ordering information would have to be considered by Set, which it doesn't currently do, even with the special feature handlers (previously set fields are ignored by setUnsetAlphaGates, and subsequently set fields override, so allAlphaGate has the same effect no matter where it appears in the iteration order).

Command line flag API remains the same. This allows ComponentConfig structures (e.g. KubeletConfiguration) to express the map structure behind feature gates in a natural way when written as JSON or YAML. For example: KubeletConfiguration Before: ``` apiVersion: kubeletconfig/v1alpha1 kind: KubeletConfiguration featureGates: "DynamicKubeletConfig=true,Accelerators=true" ``` KubeletConfiguration After: ``` apiVersion: kubeletconfig/v1alpha1 kind: KubeletConfiguration featureGates: DynamicKubeletConfig: true Accelerators: true ```

mikedanese · 2017-10-11T06:30:16Z

/lgtm
/approve

liggitt · 2017-10-11T13:58:46Z

/approve

k8s-github-robot · 2017-10-11T13:59:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, mikedanese, mtaufen

Associated issue: 53024

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~cmd/kubelet/OWNERS~~ [mikedanese]
~~pkg/kubelet/apis/kubeletconfig/OWNERS~~ [mtaufen]
~~staging/src/k8s.io/apiserver/OWNERS~~ [liggitt]
~~test/e2e_node/OWNERS~~ [liggitt,mikedanese]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-10-11T16:02:18Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2017-10-11T16:05:33Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@thockin

Automatic merge from submit-queue (batch tested with PRs 59353, 59905, 53833). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Graduate kubeletconfig API group to beta Regarding kubernetes/enhancements#281, this PR moves the kubeletconfig API group to beta. After #53088, the KubeletConfiguration type should not contain any deprecated or experimental fields, and we should not have to remove any more fields from the type before graduating it to beta. We need the community to double check for two things, however: 1. Are there any fields currently in the KubeletConfiguration type that you were going to mark deprecated this quarter, but haven't yet? 2. Are there any fields currently in the KubeletConfiguration type that are experimental or alpha, but were not explicitly denoted as such? Please comment on this PR if you can answer "yes" to either of those two questions. Please cc anyone with a stake in the kubeletconfig API, so we get as much coverage as possible. /cc @thockin @dchen1107 @Random-Liu @yujuhong @dashpole @tallclair @vishh @abw @freehan @dnardo @bowei @MrHohn @luxas @liggitt @ncdc @derekwaynecarr @mikedanese @kubernetes/sig-network-pr-reviews, @kubernetes/sig-node-pr-reviews ```release-note action required: The `kubeletconfig` API group has graduated from alpha to beta, and the name has changed to `kubelet.config.k8s.io`. Please use `kubelet.config.k8s.io/v1beta1`, as `kubeletconfig/v1alpha1` is no longer available. ``` **TODO:** - [x] Move experimental/non-gated-alpha/soon-to-be-deprecated fields to `KubeletFlags` - [x] #53088 - [x] #54154 - [x] #54160 - [x] #55562 - [x] #55983 - [x] #57851 - [x] Lift embedded structure out of strings - [x] #53025 - [x] #54643 - [x] #54823 - [x] #55254 - [x] Resolve relative paths against the location config files are loaded from - [x] #55648 - [x] Rename to `kubelet.config.k8s.io` - [x] Comments - [x] Make sure existing comments at least read sensibly. - [x] Note default values in comments on the versioned struct. - [x] Remove any reference to default values in comments on the internal struct. - [x] Most fields should be `+optional` and `omitempty`. Add where necessary. ~Where omitted, explicitly comment.~ Edit: We should not distinguish between nil and empty, see below items. - [x] Ensure defaults are specified via `pkg/kubelet/apis/kubelet.config.k8s.io/v1beta1/defaults.go`, not `cmd/kubelet/app/options/options.go`. - [x] #57770 - [x] Ensure kubeadm does not persist v1alpha1 KubeletConfiguration objects (or feature-gates this functionality) - [x] Don't make a distinction between empty and nil, because of #43203. - [x] #59515 - [x] #59681 - [x] Take the opportunity to fix insecure Kubelet defaults @tallclair - [x] #59666 - [x] Remove CAdvisorPort from KubeletConfiguration wrt #56523. - [x] #59580 - [x] Hide `ConfigTrialDuration` until we're more sure what to do with it. - [x] #59628 - [x] Fix `// default: x` comments after rebasing on recent changes.

@luxas

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Migrate FeatureGates type of kube-proxy from string to map[string]bool **What this PR does / why we need it**: Migration of FeatureGates type. This is a follow-up of #53025. **Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*: ref: #53025 #57754 (comment) **Special notes for your reviewer**: /cc @luxas @mtaufen @ncdc **Release note**: ```release-note action required: kube-proxy: feature gates are now specified as a map when provided via a JSON or YAML KubeProxyConfiguration, rather than as a string of key-value pairs. ```

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Update test framework featuregates type **What this PR does / why we need it**: A cleanup following #53025 and #57962. **Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*: ref: #53025 and #57962. **Special notes for your reviewer**: but yeah, not sure if it's worthy to do this :) **Release note**: ```release-note NONE ```

mtaufen added this to the v1.9 milestone Sep 25, 2017

mtaufen assigned smarterclayton, mikedanese and j3ffml Sep 25, 2017

k8s-ci-robot requested review from j3ffml, mikedanese and smarterclayton September 25, 2017 23:26

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 25, 2017

k8s-github-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Sep 25, 2017

mtaufen force-pushed the feature-gate-map branch from a6d96aa to 6b3a5b4 Compare September 25, 2017 23:29

k8s-ci-robot requested a review from ncdc September 25, 2017 23:38

ncdc reviewed Sep 27, 2017

View reviewed changes

mtaufen force-pushed the feature-gate-map branch from 6b3a5b4 to e288a75 Compare September 27, 2017 16:03

mtaufen force-pushed the feature-gate-map branch 3 times, most recently from 27a9ab6 to 22f55a9 Compare September 27, 2017 22:50

dixudx reviewed Sep 28, 2017

View reviewed changes

mtaufen force-pushed the feature-gate-map branch from 22f55a9 to f5d82f3 Compare September 28, 2017 19:01

mikedanese reviewed Sep 29, 2017

View reviewed changes

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 10, 2017

mtaufen force-pushed the feature-gate-map branch from f5d82f3 to db99e3f Compare October 10, 2017 16:27

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 10, 2017

mtaufen force-pushed the feature-gate-map branch from db99e3f to 131b419 Compare October 10, 2017 16:38

mtaufen assigned liggitt and unassigned smarterclayton Oct 10, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 11, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 11, 2017

k8s-github-robot merged commit df072ca into kubernetes:master Oct 11, 2017

mtaufen mentioned this pull request Nov 14, 2017

Graduate kubeletconfig API group to beta #53833

Merged

33 tasks

This was referenced Jan 8, 2018

Migrate FeatureGates type of kube-proxy from string to map[string]bool #57962

Merged

Update test framework featuregates type #57965

Merged

Make feature gates loadable from a map[string]bool #53025

Make feature gates loadable from a map[string]bool #53025

Conversation

mtaufen commented Sep 25, 2017

mtaufen commented Sep 25, 2017

mtaufen commented Sep 25, 2017

mtaufen commented Sep 26, 2017

ncdc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtaufen commented Sep 27, 2017

ncdc commented Sep 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikedanese commented Oct 11, 2017

liggitt commented Oct 11, 2017

k8s-github-robot commented Oct 11, 2017

k8s-github-robot commented Oct 11, 2017

k8s-github-robot commented Oct 11, 2017