Move to a structured status for dynamic kubelet config #63314

mtaufen · 2018-04-30T19:42:48Z

This PR updates dynamic Kubelet config to use a structured status, rather than a node condition. This makes the status machine-readable, and thus more useful for config orchestration.

Fixes: #56896

The status of dynamic Kubelet config is now reported via Node.Status.Config, rather than the KubeletConfigOk node condition.

ixdy · 2018-05-10T18:51:47Z

/retest

dashpole · 2018-05-10T21:31:40Z

/assign

k8s-github-robot · 2018-05-10T21:32:34Z

[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

dashpole · 2018-05-10T23:15:48Z

pkg/apis/core/types.go

+	// taking further action. The node tries to make the Assigned config the Active config
+	// by downloading, checkpointing, validating, and attempting to use the referenced source.
+	// +optional
+	Assigned *NodeConfigSource


From our discussion offline: I think having transition times, and heartbeat times on each of these, showing when they were last updated, and when it last changed would be useful. It would allow distinguishing between roll-backs to LKG and when the node config hasn't yet taken effect because if assigned has transitioned more recently than active, it hasn't taken effect, and if the opposite, it was rolled back. Even though we have well defined errors, this seems a better way to transmit this information. It can also help with correlating config changes with other, non-fatal behavior changes (e.g. lots of evictions start happening after enabling local storage). Or, for example if the kubelet wasn't updating LKG after 10 minutes, or if the kubelet was in an infinite loop during loading of config, and heartbeat hadn't been updated in a couple minutes, these would be obvious. I agree that having well defined objects in the status is helpful for this, but I think we should model this as a "typed condition", and keep the heartbeat and transition times for each of these Sources.

distinguishing between roll-backs to LKG and when the node config hasn't yet taken effect

To clarify, this is the scenario where we're on LKG, then ConfigSource is updated by a user, then Assigned is updated by the Kubelet, but we haven't tried the new config yet, so the status looks inconsistent (and it's unclear if Error refers to the new Assigned or the previous Assigned)?

And the transition times would clarify this by allowing you to compare the transition time for Assigned with the transition time for Error?

It can also help with correlating config changes with other, non-fatal behavior changes

I wonder if some of these debugging scenarios aren't better covered by monitoring events or timeseries derived from metrics. We could send events at every status transition, rather than just when the Kubelet restarts to try a new config.

I want to be careful with heartbeats, as they do impact scalability (every update, including heartbeats, requires a full rewrite of the Node object in etcd). But I think transition times could provide some value.

if the kubelet was in an infinite loop during loading of config

I think you'd already get a NodeNotReady in this case.

I spent the afternoon drawing up a state machine diagram, to help clarify what we should think about for status reporting: https://docs.google.com/drawings/d/1Xk_uiDFY0Y3pN6gualoy9wDPnja9BAuT-i5JYuAZ6wE/edit?usp=sharing

I'm thinking that rather than having Assigned in the status be an early acknowledgement of ConfigSource, it might be clearer to make it a direct reflection of the fully explicit on-disk record of assigned config (like LKG is), and then ensure the error messages clearly differentiate between errors related to the Spec.ConfigSource vs errors related to the already-downloaded config payload.

In general, it's probably clearer if the status simply maps to the state machine.

And if we do report timestamps, the simplest option might just be to report the modification times of the on-disk records for Assigned and LastKnownGood. (Active is a little trickier, since this is determined at Kubelet startup and only applies to the runtime; though the NodeReady heartbeat might be a decent proxy for whether Active is up to date or not).

@dashpole and I had an offline meeting and decided to leave timestamps out for now, and potentially add them later if a controller can justify that it needs them to reason about the state of the world.

dashpole · 2018-05-10T23:53:40Z

cmd/kubelet/app/server.go

-				if err := utilfeature.DefaultFeatureGate.SetFromMap(kubeletConfig.FeatureGates); err != nil {
-					glog.Fatal(err)
+				// If we should just use our existing, local config, the controller will return a nil config
+				if dynamicKubeletConfig != nil {


nit: i'm generally not a fan of giving nil an implicit meaning. I would rather have an explicit return value indicating if we should use local or remote.

I'm not sure I agree in this case.
The controller bootstrap returns the dynamic config if it exists.
If there's no dynamic config to use it returns nil (nil => Nothing is a common idiom in Go).
In the case that there's no dynamic config to use, we just move on and use the local.

I think adding a fourth return value to tag the result is unnecessary, given the ubiquity of that idiom.

mtaufen · 2018-05-11T20:58:16Z

pkg/apis/core/types.go

+	// taking further action. The node tries to make the Assigned config the Active config
+	// by downloading, checkpointing, validating, and attempting to use the referenced source.
+	// +optional
+	Assigned *NodeConfigSource


TODO: Think through this case more:

API server is upgraded to version w/ new source subtype, Kubelets not

User sets new source type

Kubelet sets Assigned in runtime status manager, intending to ack, but had unmarshaled a config with all nil subfields, as far as it could see. So the status sync will fail API server validation.

Kubelet sees AllNilSubfieldsError when it tries to produce a config source a little after updating runtime status manager, and updates the status manager with this error. But since Assigned was already set to an invalid value in the status manager, all status sync attempts will fail API server validation until this is corrected.

Now that we report the checkpointed intent in Assigned, rather than using it as an early ack, this isn't a concern. AllNilSubfieldsError would be encountered prior to checkpointing the record of Assigned config.

dashpole · 2018-05-14T23:23:48Z

pkg/kubelet/kubeletconfig/status/status.go

+
+func (s *nodeConfigStatus) ClearSyncError() {
+	s.transact(func() {
+		s.syncError = ""


how does this get set in the status? It looks like we ignore syncError unless len() > 0.

When the status is constructed, SyncError dominates Error (e.g. Node.Status.Config.Error = nodeConfigStatus.syncError || nodeConfigStatus.status.Error ).

For example:

Config fails to validate, you see a ValidateError.

You update config source, but you get AllNilSubfieldsError; this is a syncError (overlay), but the Kubelet is still internally aware of the ValidateError.

You revert config source, Kubelet knows it doesn't need a restart, so it just clears the syncError, and you see the ongoing ValidateError again.

oh, duh, I get it.

dashpole · 2018-05-14T23:25:39Z

pkg/registry/core/node/strategy.go

 	// Nodes allow *all* fields, including status, to be set on create.
+
+	if !utilfeature.DefaultFeatureGate.Enabled(features.DynamicKubeletConfig) {


since I am not familiar with the pgk/registry code-base, why is this required? Same question for addition in PrepareForUpdate.

We strip alpha fields when the corresponding feature gate is turned off, so that they can't be written unless the feature gate is turned on. See similar code in func (nodeStrategy) PrepareForCreate/PrepareForUpdate, similarly see DropDisabledAlphaFields in pkg/api/pod/util.go.

great, thanks

dchen1107 · 2018-05-15T00:00:02Z

I approve the pr but rely on dashpole@ for a throughout review.

/approve

dashpole · 2018-05-15T16:27:23Z

pkg/kubelet/kubeletconfig/status/status.go

+	// SetLastKnownGood sets the last-known-good source in the status
+	SetLastKnownGood(source *apiv1.NodeConfigSource)
+	// SetError sets the error associated with the status
+	SetError(err string)


I would prefer having Error and SyncError functions behave similarly, and I prefer qualifying the error. I.E.
SetLoadError
ClearLoadError
SetSyncError
ClearSyncError
Or just Set...Error functions.

The fact that we store the Load error in the status, and override it with the sync error is an implementation detail. (You dont need to call it load error. It is just what I picked since it comes from loadConfig(assigned).)

I could see renaming SetSyncError to something like SetTmpError. I don't think the status manager should assume anything about the context of the base error, which is why I just stuck with SetError.

Updated naming, and simplified to just SetError and SetErrorOverride.

Updates dynamic Kubelet config to use a structured status, rather than a node condition. This makes the status machine-readable, and thus more useful for config orchestration. Fixes: kubernetes#56896

dashpole · 2018-05-15T18:42:21Z

/lgtm
good work

k8s-ci-robot · 2018-05-15T18:42:31Z

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

~~api/OWNERS~~ [dchen1107]
~~cmd/kubelet/OWNERS~~ [dchen1107]
~~docs/api-reference/OWNERS~~ [dchen1107]
~~pkg/apis/core/OWNERS~~ [dchen1107]
~~pkg/kubelet/kubeletconfig/OWNERS~~ [dchen1107,mtaufen]
~~pkg/registry/OWNERS~~ [dchen1107]
~~staging/src/k8s.io/api/OWNERS~~ [dchen1107]
~~test/e2e_node/OWNERS~~ [dchen1107]

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 · 2018-05-16T01:51:09Z

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

k8s-github-robot · 2018-05-16T02:41:35Z

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

mtaufen added this to the v1.11 milestone Apr 30, 2018

mtaufen assigned liggitt and dchen1107 Apr 30, 2018

k8s-ci-robot requested review from deads2k, fabriziopandini, jbeda, justinsb, lavalamp, liggitt, ncdc, Random-Liu, tallclair, thockin, vishh, wojtek-t, yifan-gu and yujuhong April 30, 2018 19:43

mtaufen force-pushed the dkcfg-structured-status branch 3 times, most recently from 3ac818a to 4460b42 Compare May 10, 2018 21:08

k8s-ci-robot assigned dashpole May 10, 2018

dashpole reviewed May 10, 2018

View reviewed changes

mtaufen commented May 11, 2018

View reviewed changes

mtaufen mentioned this pull request May 12, 2018

Kubelet responds to ConfigMap mutations for dynamic Kubelet config #63221

Merged

mtaufen force-pushed the dkcfg-structured-status branch 3 times, most recently from a56f36b to 2c60104 Compare May 14, 2018 22:47

dashpole reviewed May 14, 2018

View reviewed changes

mtaufen force-pushed the dkcfg-structured-status branch from 2c60104 to 4eb3059 Compare May 15, 2018 00:23

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 15, 2018

dashpole reviewed May 15, 2018

View reviewed changes

mtaufen force-pushed the dkcfg-structured-status branch from 4eb3059 to a0d7300 Compare May 15, 2018 17:42

Move to a structured status for dynamic Kubelet config

fcc1f8e

Updates dynamic Kubelet config to use a structured status, rather than a node condition. This makes the status machine-readable, and thus more useful for config orchestration. Fixes: kubernetes#56896

mtaufen force-pushed the dkcfg-structured-status branch from a0d7300 to fcc1f8e Compare May 15, 2018 18:25

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 15, 2018

k8s-github-robot merged commit 2fcac6a into kubernetes:master May 16, 2018

Pingan2017 mentioned this pull request Oct 30, 2018

remove node condition ConfigOK kubernetes/website#10772

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move to a structured status for dynamic kubelet config #63314

Move to a structured status for dynamic kubelet config #63314

mtaufen commented Apr 30, 2018 •

edited

ixdy commented May 10, 2018

dashpole commented May 10, 2018

k8s-github-robot commented May 10, 2018

dashpole May 10, 2018

mtaufen May 11, 2018

mtaufen May 12, 2018

mtaufen May 14, 2018

mtaufen May 14, 2018

mtaufen May 14, 2018

dashpole May 10, 2018

mtaufen May 11, 2018 •

edited

dashpole May 14, 2018

mtaufen May 11, 2018

mtaufen May 14, 2018

dashpole May 14, 2018

mtaufen May 15, 2018

dashpole May 15, 2018

dashpole May 14, 2018

mtaufen May 15, 2018

dashpole May 15, 2018

dchen1107 commented May 15, 2018

dashpole May 15, 2018

mtaufen May 15, 2018

mtaufen May 15, 2018

dashpole commented May 15, 2018

k8s-ci-robot commented May 15, 2018

k8s-github-robot commented May 16, 2018

k8s-github-robot commented May 16, 2018

		// Nodes allow all fields, including status, to be set on create.

		if !utilfeature.DefaultFeatureGate.Enabled(features.DynamicKubeletConfig) {

Move to a structured status for dynamic kubelet config #63314

Move to a structured status for dynamic kubelet config #63314

Conversation

mtaufen commented Apr 30, 2018 • edited

ixdy commented May 10, 2018

dashpole commented May 10, 2018

k8s-github-robot commented May 10, 2018

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 May 11, 2018 • edited

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

dchen1107 commented May 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dashpole commented May 15, 2018

k8s-ci-robot commented May 15, 2018

k8s-github-robot commented May 16, 2018

k8s-github-robot commented May 16, 2018

mtaufen commented Apr 30, 2018 •

edited

mtaufen May 11, 2018 •

edited