Skip to content
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

Kubelet config: Validate new config against future feature gates #63409

Merged
merged 1 commit into from May 22, 2018

Conversation

@mtaufen
Copy link
Contributor

mtaufen commented May 3, 2018

This fixes an issue with KubeletConfiguration validation, where the
feature gates set by the new config were not taken into account.

Also fixes a validation issue with dynamic Kubelet config, where flag
precedence was not enforced prior to dynamic config validation in the
controller; this prevented rejection of dynamic configs that don't merge
well with values set via legacy flags.

Fixes #63305

NONE
@@ -43,9 +43,16 @@ const (
configTrialDuration = 10 * time.Minute
)

type TransformFunc func(kc *kubeletconfig.KubeletConfiguration) error

This comment has been minimized.

@mtaufen

mtaufen May 16, 2018

Author Contributor

I'm not 100% sure if this is the pattern I want, because it looks like a generic utility but we have a very specific use-case: enforce flag precedence so the final config combination is validated before choosing a config.

At the very least, we should warn people in a comment, and maybe change the name to something less generic.

This comment has been minimized.

@dashpole

dashpole May 18, 2018

Contributor

Alternatively, we could make it something generic, and use it in the e2e tests. https://github.com/kubernetes/kubernetes/blob/master/test/e2e_node/util.go#L106 is essentially the same function.

This comment has been minimized.

@mtaufen

mtaufen May 18, 2018

Author Contributor

It's more about how it's used in the controller; it's an extension point that we only want to use as a last resort.

This comment has been minimized.

@mtaufen

mtaufen May 20, 2018

Author Contributor

Though I don't see how we get away from it as long as we have to enforce flag precedence.

This comment has been minimized.

@mtaufen

mtaufen May 20, 2018

Author Contributor

I think we'll go with this for now. I added a warning to the comment above the field on the controller and to the NewController constructor.

@mtaufen mtaufen changed the title WIP Kubelet config: Validate new config against future feature gates Kubelet config: Validate new config against future feature gates May 16, 2018

@mtaufen mtaufen force-pushed the mtaufen:kc-validation-feature-gates branch 2 times, most recently from d7721eb to cc02d3c May 16, 2018

@mtaufen mtaufen added this to the v1.11 milestone May 16, 2018

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented May 16, 2018

[MILESTONENOTIFIER] Milestone Pull Request: Up-to-date for process

@dashpole @dchen1107 @liggitt @mtaufen

Pull Request Labels
  • sig/cluster-lifecycle sig/node: Pull Request will be escalated to these SIGs if needed.
  • priority/important-soon: Escalate to the pull request owners and SIG owner; move out of milestone after several unsuccessful escalation attempts.
  • kind/feature: New functionality.
Help
@@ -31,6 +31,11 @@ import (
func ValidateKubeletConfiguration(kc *kubeletconfig.KubeletConfiguration) error {
allErrors := []error{}

// Make a local copy of the global feature gates and combine it with the gates set by this configuration.
// This allows us to validate the config against the set of gates it will actually run against.
localFeatureGate := utilfeature.DefaultFeatureGate.DeepCopy()

This comment has been minimized.

@liggitt

liggitt May 18, 2018

Member

why are we starting with a copy of the global gates? wouldn't kc.FeatureGates be the merged args+config at this point?

edit: hmm, because we don't want to overwrite the global here, and don't have a good way to construct a new default one from scratch

This comment has been minimized.

@mtaufen

mtaufen May 18, 2018

Author Contributor

yup, exactly

@mtaufen

This comment has been minimized.

Copy link
Contributor Author

mtaufen commented May 18, 2018

TODO: After this merges, double check validation in case concurrent PRs add more feature gate checks (e.g. #63912).

@mtaufen mtaufen force-pushed the mtaufen:kc-validation-feature-gates branch 2 times, most recently from 934caf8 to 97aa235 May 20, 2018

Kubelet config: Validate new config against future feature gates
This fixes an issue with KubeletConfiguration validation, where the
feature gates set by the new config were not taken into account.

Also fixes a validation issue with dynamic Kubelet config, where flag
precedence was not enforced prior to dynamic config validation in the
controller; this prevented rejection of dynamic configs that don't merge
well with values set via legacy flags.

@mtaufen mtaufen force-pushed the mtaufen:kc-validation-feature-gates branch from 97aa235 to 647e903 May 20, 2018

@dashpole

This comment has been minimized.

Copy link
Contributor

dashpole commented May 21, 2018

/lgtm
I like all of the warning comments.
Just because I am curious, do we plan to deprecate the feature gates flag in favor of including it in the component configuration? Will we be able to remove the TransformFunc after that, or will we need it to validate flags that may not be in the kubelet configuration object?

@k8s-ci-robot k8s-ci-robot added the lgtm label May 21, 2018

@mtaufen

This comment has been minimized.

Copy link
Contributor Author

mtaufen commented May 21, 2018

@dashpole yes, the feature-gates flag will go away in favor of componentconfig. The transform func will probably be around until we can stop enforcing flag precedence, which is likely a-long-time™.

@mtaufen

This comment has been minimized.

Copy link
Contributor Author

mtaufen commented May 21, 2018

/retest

@dchen1107

This comment has been minimized.

Copy link
Member

dchen1107 commented May 21, 2018

/lgtm

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented May 21, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dashpole, dchen1107, mtaufen

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented May 22, 2018

Automatic merge from submit-queue (batch tested with PRs 63881, 64046, 63409, 63402, 63221). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 6d510f5 into kubernetes:master May 22, 2018

18 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation mtaufen authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.