Add VerticalPodAutoscaler support with HPA-style workload reverse-matching#656
Conversation
…ching Adds a standalone VerticalPodAutoscaler.tmpl (target ref, update mode, per-container min/max policy, applied recommendation) and reverse-matches VPAs onto their target workload the same way HorizontalPodAutoscaler already does, via a new matching_vpas template in common.tmpl gated by a vpaTargetable flag (Deployment/StatefulSet/ReplicaSet/DaemonSet -- VPA, unlike HPA, can also target DaemonSet directly). install-e2e-deps now installs the VPA controllers via Helm (CRDs alone aren't enough since the new e2e scenario exercises the updater actually evicting/recreating a Pod to apply a recommendation), and a new e2e test covers the full loop with fully anchored fixtures. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request adds support for rendering VerticalPodAutoscaler (VPA) resources, including reverse-matching them to their target workloads (Deployments, StatefulSets, ReplicaSets, and DaemonSets). It also introduces end-to-end tests that deploy VPA controllers via Helm to verify recommendation and eviction behavior. The review feedback highlights several opportunities to prevent potential nil pointer dereferences during template evaluation by defensively guarding .Spec and .Status fields with default empty dictionaries. Additionally, the reviewer suggests renaming misleading labels, such as changing 'Scales' to 'Targets' and clarifying the 'Min replicas to evict' label.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| {{- $obj := $.KubeGetFirst $.Namespace (.kind | default "Deployment") .name }} | ||
| {{- if $obj.Object }}{{ $.IncludeRenderableObject $obj | nindent 2 }}{{ end }} | ||
| {{- else }} | ||
| {{- "Scales" | bold | nindent 2 }}: {{ $.Include "resource_ref" (dict "kind" (.kind | default "Deployment") "name" .name "namespace" $.Namespace) }} |
There was a problem hiding this comment.
The label 'Scales' is misleading for a VerticalPodAutoscaler because VPA does not scale the number of replicas (like an HPA does); instead, it autoscales the resource requests and limits of the target workload. Consider using 'Targets' or 'Target workload' to accurately describe this relationship.
{{- \"Targets\" | bold | nindent 2 }}: {{ $.Include \"resource_ref\" (dict \"kind\" (.kind | default \"Deployment\") \"name\" .name \"namespace\" $.Namespace) }}
| {{- $updateMode := (.Spec.updatePolicy | default dict).updateMode | default "Auto" }} | ||
| {{- if ne $updateMode "Auto" }} | ||
| {{- "Update mode" | bold | nindent 2 }}: {{ $updateMode | cyan }} | ||
| {{- end }} | ||
| {{- with (.Spec.updatePolicy | default dict).minReplicas }} | ||
| {{- "Min replicas to evict" | bold | nindent 2 }}: {{ . | toString | cyan }} | ||
| {{- end }} | ||
| {{- range (.Spec.resourcePolicy | default dict).containerPolicies }} |
There was a problem hiding this comment.
This block has two issues:\n1. Defensive Programming: If .Spec is nil (which can happen for newly created resources or when rendering local files with missing fields), accessing .Spec.updatePolicy or .Spec.resourcePolicy directly will cause a template evaluation error (nil pointer dereference). Guarding .Spec with a default empty dictionary prevents this.\n2. Misleading Label: The label 'Min replicas to evict' is misleading. In the VPA updatePolicy, minReplicas defines the minimum number of replicas that must be running for the updater to be allowed to perform any evictions. It does not mean the minimum number of replicas that will be evicted. Using 'Min replicas for eviction' or 'Min replicas required to evict' is much more accurate.
{{- $updateMode := ((.Spec | default dict).updatePolicy | default dict).updateMode | default \"Auto\" }}\n {{- if ne $updateMode \"Auto\" }}\n {{- \"Update mode\" | bold | nindent 2 }}: {{ $updateMode | cyan }}\n {{- end }}\n {{- with ((.Spec | default dict).updatePolicy | default dict).minReplicas }}\n {{- \"Min replicas for eviction\" | bold | nindent 2 }}: {{ . | toString | cyan }}\n {{- end }}\n {{- range ((.Spec | default dict).resourcePolicy | default dict).containerPolicies }}
| {{- end }} | ||
| {{- end }} | ||
| {{- end }} | ||
| {{- with (.Status.recommendation | default dict).containerRecommendations }} |
There was a problem hiding this comment.
If .Status is nil (e.g., when rendering local files or resources that haven't been reconciled yet), accessing .Status.recommendation directly will cause a template evaluation error. Guard .Status with a default empty dictionary to prevent nil pointer dereferences.
{{- with ((.Status | default dict).recommendation | default dict).containerRecommendations }}
| {{- $updateMode := (.Spec.updatePolicy | default dict).updateMode | default "Auto" }} | ||
| {{- if ne $updateMode "Auto" }}, {{ $updateMode | cyan }}{{ end }} | ||
| {{- range (.Status.recommendation | default dict).containerRecommendations }} |
There was a problem hiding this comment.
If .Spec or .Status is nil, accessing .Spec.updatePolicy or .Status.recommendation directly will cause a template evaluation error. Use defensive programming to guard these fields with default empty dictionaries.
{{- $updateMode := ((.Spec | default dict).updatePolicy | default dict).updateMode | default \"Auto\" }}\n {{- if ne $updateMode \"Auto\" }}, {{ $updateMode | cyan }}{{ end }}\n {{- range ((.Status | default dict).recommendation | default dict).containerRecommendations }}
| {{- if not ($ctx.Config.GetBool "shallow") }} | ||
| {{- $vpas := list }} | ||
| {{- range $ctx.KubeGet $namespace "VerticalPodAutoscalers" }} | ||
| {{- $ref := .Spec.targetRef | default dict }} |
ReplicaSets/ControllerRevisions were sorted by creationTimestamp, which only has second resolution: revisions created in the same second (e.g. in a fast e2e run) could tie and render rollout diffs out of order. Sort by the revision annotation/field instead for a reliable chronological order in Deployment, StatefulSet and DaemonSet rollout rendering. Also renames VerticalPodAutoscaler's "Scales" label to "Targets" (VPA doesn't scale replica count, unlike HPA) and clarifies "Min replicas to evict" to "Min replicas for eviction", per gemini-code-assist review on PR #656. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Summary
VerticalPodAutoscaler.tmpl(target ref, update mode, per-container min/max policy, applied recommendation) built from the liveautoscaling.k8s.io/v1CRD schema.HorizontalPodAutoscaleralready does (matching_hpas), via a newmatching_vpastemplate incommon.tmpl, gated by avpaTargetableflag separate from HPA'sscalablegate since VPA can also target DaemonSet directly (unlike HPA).install-e2e-depsnow installs the real VPA controllers (recommender/updater/admission-controller) via thecowboysysopHelm chart, since the new e2e scenario exercises the updater actually evicting/recreating a Pod to apply a recommendation, not just rendering a static object.\A...\z) fixtures.Test plan
go build ./...,go vet ./...,staticcheck ./...go test ./...(unit suite)TestE2ERegexFixturesAreAnchoredpasses for both new fixturesTestE2EDynamicManifests/VerticalPodAutoscaler...passes against real minikube (verified on two independent fresh runs)--deepterminates cleanly (mutual-recursion guard, same as HPA) and DaemonSet reverse-matching works🤖 Generated with Claude Code