For context, here is a re-print of the discussion on the kubeletconfig-to-beta PR:

liggitt 2 days ago Member
if this is only used for dynamic config, doesn't it make more sense in the Node ConfigSource?

mtaufen 2 days ago Member
I think this is a question of whether we should associate the measure of risk tolerance with the config being applied to a Node (KubeletConfiguration.ConfigTrialDuration), or with the Node the config is being applied to (Node.ConfigSource.ConfigTrialDuration). Now that you bring it up, I'm not sure I have a clear winner:

KubeletConfiguration.ConfigTrialDuration:

• + Directly associates idea of "risk" with a configuration, which makes some sense because whatever is in the configuration probably determines how risky it is.
• - Feels weird because, as you mentioned, it only affects config from a specific delivery mechanism.
• +/- Simpler, in some ways, but less flexible (see below).

Node.ConfigSource.ConfigTrialDuration:

• + Can set the baseline for how "skeptical" a Node is of new configs, without having to determine the "risk" of each configuration.
• +/- Can technically achieve the same effect as KubeletConfiguration.ConfigTrialDuration by changing the assigned config and trial duration simultaneously, though you'd have to maintain that (config, duration) association independent of the config.
• - Makes the NodeConfigSource API more complex, e.g. what does it mean to change the trial duration without changing the assigned config? I wonder if it would be confusing for a Kubelet to say: "Ah, config passed trial, now it's my last-known-good. Oh, you increased the trial? Guess now it's not my last known good anymore." This scenario requires the Kubelet to remember current and previous last-known-good configs, so that it can go back and forth when the trial duration changes. I worry this could be a confusing API (more implicit state and a parallel timeline).
• + Makes some sense when you think about higher-level declarative APIs that control config across pools of nodes; you probably want to allow these APIs to control config policy without having to edit serialized payloads.

As third option, we could consider Node.ConfigSource.LastKnownGood:

• +/- Elevates the problem of last-know-good selection to higher-level APIs. Inversion of control = inversion of responsibility.
• + Completely explicit.
• - Kubelet is maybe less robust by default, though default fallback to "provisioned with" config is probably plenty good in circumstances where people have fungible VMs.

As a fourth option, we could remove the external knob altogether, have the Kubelet just use 10 minutes as an internal default, and decide where to expose control of last-known-good later on (I don't really like kicking the can down the road, but it's an option).

WDYT?

liggitt 12 hours ago • edited Member
I guess I have a hard time understanding how I would choose a value for this. It doesn't seem like something you'd tune super carefully... whatever gets you past the "I'm crash looping on config X, better roll back", which I'm not sure is best represented by a duration (something like "completed startup operations and N successful sync loops" might be more meaningful)

liggitt 12 hours ago Member
Without a clear picture of how we'd recommend someone tune this, I'd probably omit it and keep it internal and low(ish) for now, but happy to hear opposing views.

mtaufen 7 hours ago Member
I think that's probably what we should do for now, simply because it would be silly to block Kubelet's component config going beta while we argue this out.

moving internally for now LGTM

as an aside, there are normal conditions where the kubelet exits/restarts (waiting on client credential grants, or an available API server, etc). how does this interact with crashloops caused by things like that? could that trigger rolling back config because the kubelet thinks the config was responsible for the crashloop?

/lgtm

[APPROVALNOTIFIER] This PR is APPROVED

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

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

The pull request process is described here

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

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

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

Right now, this is just a grace-period after which the last-known-good updates. We removed crash-loop detection over the summer because we weren't confident the implementation covered the full spectrum of edge cases. So right now the only thing that can cause a fallback to last-known good is a failure to load the assigned config (corrupt checkpoint, invalid configuration, etc.).

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