If checkpoint state is nil in non-error cases (like standalone kubelet) we can't log errors on every pod sync. We'll flood the error log with these otherwise.

Is there a way we can make the VPA feature completely inactive/inert for standalone kubelets, rather than sprinkling nil checks throughout, since a kubelet running that way won't be getting pod updates from the API that change resources?

Thanks for the review and catching this issue. I think I can add a couple of Get interfaces to status manager (similar pattern to GetPodStatus) and avoid these nil checks. I'll work on it.

BTW, how did you find this issue? Is this it: https://github.com/kelseyhightower/standalone-kubelet-tutorial ?

how did you find this issue?

we (GKE) run standalone kubelets, and our CI started failing once the VPA change merged to master

I think I can add a couple of Get interfaces to status manager (similar pattern to GetPodStatus) and avoid these nil checks

Does the VPA feature make sense for a standalone kubelet? I don't think it does. It would be easier to reason about if standalone kubelets didn't ever hit VPA code paths.

maybe something like this:

mkdir -p /var/run/kubernetes/static-pods

echo "
kind: Pod
apiVersion: v1
metadata:
  name: busybox
spec:
  containers:
  - name: busybox
    image: busybox
" > /var/run/kubernetes/static-pods/busybox.yaml

START_MODE=kubeletonly FEATURE_GATES=InPlacePodVerticalScaling=true hack/local-up-cluster.sh

but I bet local-up-cluster.sh has atrophied and START_MODE=kubeletonly won't work quite right

@SergeyKanzhelev @smarterclayton looks like we have a general test gap for standalone kubelet that regularly causes problems:

• kubelet: fix nil pointer in startReflector for standalone mode #113501
• Managing nil pointer in VolumeManager #108442
• standalone kubelet panic because of nil pointer in VolumeManager #108063
• Standalone kubelet demands pki/kubelet.key #87558
• kubelet: Observed a panic: "invalid memory address or nil pointer dereference" #77174
• ignore kubeclient nil in csi plugin init #75308 (this actually seems similar to this PR in keeping code paths active and trying to add nil checks, which exposed later code to unexpected nils)

START_MODE=kubeletonly FEATURE_GATES=InPlacePodVerticalScaling=true hack/local-up-cluster.sh

but I bet local-up-cluster.sh has atrophied and START_MODE=kubeletonly won't work quite right

Thanks, that works. Needs one additional envvar:
POD_MANIFEST_PATH=/var/run/kubernetes/static-pods START_MODE=kubeletonly FEATURE_GATES=InPlacePodVerticalScaling=true hack/local-up-cluster.sh

Anyways, I have another commitment I need to get to. I'll verify this with the above suggested optimization later in the day today.

Thanks Jordan for raising this, AFAIU I don't think we have good coverage of standalone kubelet in CI today. I think we could think about extending node e2e to also support a mode where we spin up kubelet in standalone mode specifically and only run static pods as a few tests.

In general, it may help as well to have an environment we can dedicate to static pod related tests since we have a few issues there that @smarterclayton and others been trying to tackle recently.

Looking more closely, kubeletonly mode conveniently brings up just kubelet (POD_MANIFEST_PATH default works) but kubeClient is created. But, I was able to set kubeDeps.KubeClient = nil just before NewMainKubelet and get a repro to verify the before & after.

We don't need a new check for kubeClient==nil before invoking handlePodResourcesResize because !kubetypes.IsStaticPod(pod) check achieves the same goal.

check nil and return not found before locking?

yes.

state will still be nil in all of these functions if the feature gate is disabled.

sweeping all the callers, it's pretty twisty to tell that all of the call sites are guarded by feature gate checks (these are sometimes called from functions which are only called under feature gate guard, or are called from declared closures which are then invoked inside a feature gate guard)

this PR is an improvement on current state of master, but this still seems like a sharp edge to improve before 1.27 cut

True. I can add feature gates check in these functions as a conservative check that makes it easier for future reviews vs. adding a nil check and forgetting to remove at GA. It is one extra compare and branch instruction we don't need on every pod sync (if we can count on pull & CI tests).

this PR is an improvement on current state of master, but this still seems like a sharp edge to improve before 1.27 cut

What other things do you want to see fixed? I initially wanted to move away from node local checkpoint in favor of relying on values persisted in PodStatus (if it is legit use of KEP 2527), but that wouldn't fly in standalone kubelet scenario. I feel kubelet could use a more generic KV abstraction that better decouples the checkpointing implementation details, but that is a bit larger scope work.. jmho.

probably not leaving those manager methods NPE landmines if the feature-gate is disabled? either doing nil checks in those functions, or assigning a dummy/no-op impl to state when the feature gate is off

but a follow-up is fine... I'm more concerned with getting alpha standalone kubelets green again at this point

no-op is a good idea. Since this fix is urgent and already LTGM'd, I'll create a separate PR follow-up or tack it on to an existing follow-up PR.

Does PR #116351 get it done?