Kubelet responds to ConfigMap mutations for dynamic Kubelet config#63221

Merged

k8s-github-robot merged 1 commit intokubernetes:masterfrom

mtaufen:dkcfg-live-configmap

May 22, 2018

Contributor

mtaufen commented Apr 27, 2018 •

edited

Loading

This PR makes dynamic Kubelet config easier to reason about by leaving less room for silent skew scenarios. The new behavior is as follows:

ConfigMap does not exist: Kubelet reports error status due to missing source
ConfigMap is created: Kubelet starts using it
ConfigMap is updated: Kubelet respects the update (but we discourage this pattern, in favor of incrementally migrating to a new ConfigMap)
ConfigMap is deleted: Kubelet keeps using the config (non-disruptive), but reports error status due to missing source
ConfigMap is recreated: Kubelet respects any updates (but, again, we discourage this pattern)

This PR also makes a small change to the config checkpoint file tree structure, because ResourceVersion is now taken into account when saving checkpoints. The new structure is as follows:

- dir named by --dynamic-config-dir (root for managing dynamic config)
| - meta
  | - assigned (encoded kubeletconfig/v1beta1.SerializedNodeConfigSource object, indicating the assigned config)
  | - last-known-good (encoded kubeletconfig/v1beta1.SerializedNodeConfigSource object, indicating the last-known-good config)
| - checkpoints
  | - uid1 (dir for versions of object identified by uid1)
    | - resourceVersion1 (dir for unpacked files from resourceVersion1)
    | - ...
  | - ...

fixes: #61643

The dynamic Kubelet config feature will now update config in the event of a ConfigMap mutation, which reduces the chance for silent config skew. Only name, namespace, and kubeletConfigKey may now be set in Node.Spec.ConfigSource.ConfigMap. The least disruptive pattern for config management is still to create a new ConfigMap and incrementally roll out a new Node.Spec.ConfigSource.

mtaufen added priority/important-soon area/kubelet area/kubelet-api sig/node kind/feature labels

mtaufen self-assigned this

k8s-ci-robot added do-not-merge/work-in-progress release-note size/XXL cncf-cla: yes labels

k8s-ci-robot requested review from brendandburns, caesarxuchao, dchen1107, deads2k, ericchiang, feiskyer, luxas, ncdc, roberthbailey, sttts, tallclair and tmrts

April 27, 2018 01:15

k8s-ci-robot added area/kubeadm sig/cluster-lifecycle labels

k8s-github-robot added the kind/api-change label

mtaufen commented

View reviewed changes

pkg/kubelet/kubeletconfig/checkpoint/download.go Outdated

+              // NewRemoteConfigSource constructs a RemoteConfigSource from a v1/NodeConfigSource object.
+              // We assume the source has been validated beforehand - e.g. by an API server.
+              func NewRemoteConfigSource(source *apiv1.NodeConfigSource) RemoteConfigSource {
+              	return &remoteConfigMap{source}

Contributor Author

mtaufen Apr 27, 2018

actually still need to check not all known fields nil, wrt old clients against a new API server that allows a new config source type failing cleanly (i.e. not crashing due to nil pointer dereference).

mtaufen mentioned this pull request

explicit kubelet config key in Node.Spec.ConfigSource.ConfigMap #59847

Merged

mtaufen force-pushed the dkcfg-live-configmap branch from 87f5ade to 7ec2708 Compare

April 30, 2018 23:34

k8s-github-robot added the kind/new-api label

mtaufen force-pushed the dkcfg-live-configmap branch from 7ec2708 to fd0b29e Compare

May 1, 2018 17:20

mtaufen force-pushed the dkcfg-live-configmap branch 10 times, most recently from 849c93e to a7eb158 Compare

May 18, 2018 22:47

dashpole reviewed

View reviewed changes

Contributor

dashpole left a comment

Looks good with a couple of nits.

pkg/kubelet/kubeletconfig/checkpoint/download.go Outdated

Contributor

dashpole May 21, 2018

nit: It seems like the interface for (r *remoteConfigMap) Informer() could be a bit cleaner, as addFunc, updateFunc, and deleteFunc arent really well-defined in the interface. Rather than defining them, I would suggest either replacing these inputs with a cache.ResourceEventHandlerFuncs parameter, or assuming the caller of Informer() will add their desired ResourceEventHanderFuncs to the informer returned by Informer().

Contributor Author

mtaufen May 21, 2018

Makes sense, I'll replace them with cache.ResourceEventHandlerFuncs.

Contributor Author

mtaufen May 21, 2018

done

pkg/kubelet/kubeletconfig/checkpoint/download.go Outdated

Contributor

dashpole May 21, 2018

nit: Prefer describing what is returned, rather than behavior: e.g. ResourceVersion returns the resource version of the most recently downloaded payload targeted by the source.

Contributor Author

mtaufen May 21, 2018

done

pkg/kubelet/kubeletconfig/checkpoint/download.go Outdated

Contributor

dashpole May 21, 2018

nit: similar to above about describing internal behavior

Contributor

dashpole May 21, 2018

Actually, I think Payload() may be a better interface. The fact that it downloads and caches it on disk can be an implementation detail?

Contributor

dashpole May 21, 2018

Scratch that. Downloading the payload locally is an important part of the function.

pkg/kubelet/kubeletconfig/checkpoint/download.go Outdated

Contributor

dashpole May 21, 2018

This is slightly confusing to me. Why are we overwriting the UID and ResourceVersion of one ConfigMap with the UID and ResourceVersion of another ConfigMap? Do we use the rest of the Stored ConfigMap, or just these fields? If we only use those two fields, why store the whole thing, and if we use the whole configmap, it seems potentially confusing to insert a different UID and ResourceVersion...

Contributor Author

mtaufen May 21, 2018

r.source.ConfigMap is a ConfigMapNodeConfigSource, we are resolving the reference (namespace, name, kubeletConfigKey) by adding the additional details (uid, resourceVersion) that identify a version of that resource

Contributor

dashpole May 21, 2018

oh, right. I forgot it wasn't an actual configmap.

Member

liggitt commented May 21, 2018

just looking at the description, for the current and last-known-good configs, where is the resolved uid/resourceVersion recorded (to be able to look up the correct dir in checkpoints)?

is there any expectation that multiple objects will need to be pulled in/checkpointed in the future for a node config (e.g. a configmap and some set of secrets)? should that inform our config/checkpoint disk structure?

Contributor Author

mtaufen commented May 21, 2018

just looking at the description, for the current and last-known-good configs, where is the resolved uid/resourceVersion recorded (to be able to look up the correct dir in checkpoints)?

@liggitt in the SerializedNodeConfigSources written to --dynamic-config-dir/meta/assigned and --dynamic-config-dir/meta/last-known-good.

is there any expectation that multiple objects will need to be pulled in/checkpointed in the future for a node config (e.g. a configmap and some set of secrets)? should that inform our config/checkpoint disk structure?

Not yet. If we added this, I'd expect NodeConfigSource to be extended to represent a request for multiple objects (which would transitively ensure --dynamic-config-dir/meta can record the same), and since UID and ResourceVersion are consistent concepts across all objects, the structure of --dynamic-config-dir/checkpoints should work for additional object types.

Member

liggitt commented May 21, 2018

@liggitt in the SerializedNodeConfigSources written to --dynamic-config-dir/meta/assigned and --dynamic-config-dir/meta/last-known-good.

ah, so those are enhanced versions of what is in the Node API object

Not yet. If we added this, I'd expect NodeConfigSource to be extended to represent a request for multiple objects (which would transitively ensure --dynamic-config-dir/meta can record the same), and since UID and ResourceVersion are consistent concepts across all objects, the structure of --dynamic-config-dir/checkpoints should work for additional object types.

That makes sense


          dynamic Kubelet config reconciles ConfigMap updates

b5648c3

dashpole reviewed

View reviewed changes

pkg/kubelet/kubeletconfig/status/status.go

		@@ -142,6 +145,8 @@ func (s *nodeConfigStatus) Sync(client clientset.Interface, nodeName string) {
		return

Contributor

dashpole May 21, 2018

I realize this isn't part of this PR, but is there a reason you chose to use a periodic, jittered check rather than just a long-running loop?
for _ := range s.syncCh {
utillog.Infof("updating Node.Status.Config")
...
}

Contributor

dashpole commented May 21, 2018

/lgtm
comment above doesn't need to be dealt with in this PR.
Great work, especially on the test coverage for this.

Contributor Author

mtaufen commented May 21, 2018

/retest

Member

dchen1107 commented May 21, 2018

/approve

Approve the pr based on the offline discussions with @mtaufen on the high level design and @dashpole' detail review.

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

Details

Needs approval from an approver in each of these files:

~~cmd/kubelet/OWNERS~~ [dchen1107]
~~pkg/apis/core/OWNERS~~ [dchen1107]
~~pkg/kubelet/kubeletconfig/OWNERS~~ [dchen1107,mtaufen]
~~test/OWNERS~~ [dchen1107]

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

mtaufen mentioned this pull request

increase ci-kubernetes-node-kubelet-serial timeout kubernetes/test-infra#8102

Merged

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.

Contributor

dashpole commented May 22, 2018

I think we may need to update the node e2e framework. Tests using dynamic kubelet config are failing with the error: Forbidden: uid must not be set in spec

Contributor Author

mtaufen commented May 23, 2018

That's surprising, bad merge?

Contributor Author

mtaufen commented May 23, 2018

Ah, misread, you said tests using dynamic config, not the dynamic config tests.

Contributor Author

mtaufen commented May 23, 2018

Yup, probably have to do that.

Contributor Author

mtaufen commented May 23, 2018

wojtek-t mentioned this pull request

Pod-startup performance related failures on 2k/5k-node tests #62808

Closed

mtaufen mentioned this pull request

remove unused status per TODO #64486

Merged

mtaufen mentioned this pull request

featureGates: DynamicKubeletConfig fails in Kubeadm v1.11 kubernetes/kubeadm#921

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

brendandburns Awaiting requested review from brendandburns

caesarxuchao Awaiting requested review from caesarxuchao

dchen1107 Awaiting requested review from dchen1107

deads2k Awaiting requested review from deads2k

ericchiang Awaiting requested review from ericchiang

feiskyer Awaiting requested review from feiskyer

luxas Awaiting requested review from luxas

ncdc Awaiting requested review from ncdc

roberthbailey Awaiting requested review from roberthbailey

sttts Awaiting requested review from sttts

tallclair Awaiting requested review from tallclair

tmrts Awaiting requested review from tmrts

1 more reviewer

dashpole dashpole left review comments

Labels

approved area/kubeadm area/kubelet area/kubelet-api cncf-cla: yes kind/feature lgtm priority/important-soon release-note sig/cluster-lifecycle sig/node size/XXL