Skip to content

Add LimitRange template and Pod LimitRange-defaults note#659

Merged
bergerx merged 2 commits into
masterfrom
worktree-bridge-cse_01PXvTzfoyx1LwrMc8myRfEb
Jul 11, 2026
Merged

Add LimitRange template and Pod LimitRange-defaults note#659
bergerx merged 2 commits into
masterfrom
worktree-bridge-cse_01PXvTzfoyx1LwrMc8myRfEb

Conversation

@bergerx

@bergerx bergerx commented Jul 11, 2026

Copy link
Copy Markdown
Owner

Summary

  • Add LimitRange.tmpl (previously unimplemented embedded template). LimitRange has no status, so this renders spec.limits[] grouped by resource name (cpu/memory/storage/...) across default/defaultRequest/max/min/maxLimitRequestRatio, rather than five separate lists.
  • Add a (matches LimitRange defaults) note next to a Pod container's existing resource line, shown only when every requests/limits value is exactly explained by a single namespace LimitRange's Container-type defaults. Worded as a coincidence, not provenance — the stored Pod spec doesn't retain which fields the API server actually defaulted.
  • Investigated the rest of a proposed ResourceQuota/LimitRange coverage matrix (reverse-match from Deployment/StatefulSet/DaemonSet/ReplicaSet/Job/CronJob/PVC) and found it's already covered by existing behavior:
    • Deployment/ReplicaSet already surface FailedCreate: exceeded quota ... via the ReplicaFailure condition.
    • StatefulSet/Job already surface the same via the default-on events section (verified live against a minikube cluster).
    • PVC creation rejected by quota produces no object at all (API server denies synchronously) — nothing to render.
  • Filed Consider quota-headroom / imminent-rollout-block signal for workloads #658 to track the one signal deliberately left out (quota-headroom / imminent-rollout-block check), which needs more careful wording around shared namespace state and currently has no path to automated test coverage.

Closes/relates to #658 (tracking issue for future quota-headroom work, not closed by this PR).

Test plan

  • go test ./... passes
  • New local fixtures tests/artifacts/limitrange-{container,pod,pvc}.yaml/.out cover LimitRange rendering for Container/Pod/PersistentVolumeClaim limit types
  • Verified live against a minikube cluster: LimitRange render, Pod note firing when resources match namespace defaults, Pod note not firing when resources are explicitly set to different values, and Deployment/ReplicaSet/StatefulSet/Job quota-rejection visibility via existing conditions/events

LimitRange has no status subresource, so the new template renders spec.limits
grouped by resource name rather than as five separate default/defaultRequest/
max/min/maxLimitRequestRatio lists. Pod.tmpl now appends "(matches LimitRange
defaults)" beside a container's existing resource line when its requests/
limits are exactly explained by a namespace LimitRange's Container-type
defaults, worded as a coincidence rather than provenance since the stored Pod
spec doesn't retain per-field defaulting history.

ResourceQuota/LimitRange admission failures on workloads (Deployment/
ReplicaSet/StatefulSet/DaemonSet/Job/CronJob) were found to already surface
via existing conditions and default-on events, so no changes were needed
there. Quota-headroom / imminent-rollout-block signal tracked in #658.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces support for rendering Kubernetes LimitRange resources, including a new template LimitRange.tmpl and corresponding test artifacts. It also adds a feature to Pod.tmpl that appends a note when a container's resource requests and limits match the namespace's LimitRange defaults. Feedback on the PR suggests improving the resource quantity comparison logic in Pod.tmpl by using the quantityToFloat64 helper to avoid false negatives due to different string representations of equivalent quantities (e.g., 1000m vs 1).

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.

Comment on lines +439 to +446
{{- $requestsExplained := eq (len $requests) (len $defaultRequest) }}
{{- range $k, $v := $requests }}
{{- if ne (index $defaultRequest $k) $v }}{{ $requestsExplained = false }}{{ end }}
{{- end }}
{{- $limitsExplained := eq (len $limits) (len $default) }}
{{- range $k, $v := $limits }}
{{- if ne (index $default $k) $v }}{{ $limitsExplained = false }}{{ end }}
{{- end }}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Comparing Kubernetes resource quantities as raw strings or values is fragile because quantities can be represented in different equivalent formats (e.g., 1000m vs 1, 1Gi vs 1024Mi, or 0.5 vs 500m). Using the existing quantityToFloat64 helper ensures semantically identical quantities are correctly matched regardless of their string representation.

                        {{- $requestsExplained := eq (len $requests) (len $defaultRequest) }}
                        {{- range $k, $v := $requests }}
                            {{- $defaultVal := index $defaultRequest $k }}
                            {{- if $defaultVal }}
                                {{- if ne ($defaultVal | toString | quantityToFloat64) ($v | toString | quantityToFloat64) }}{{ $requestsExplained = false }}{{ end }}
                            {{- else }}
                                {{- $requestsExplained = false }}
                            {{- end }}
                        {{- end }}
                        {{- $limitsExplained := eq (len $limits) (len $default) }}
                        {{- range $k, $v := $limits }}
                            {{- $defaultVal := index $default $k }}
                            {{- if $defaultVal }}
                                {{- if ne ($defaultVal | toString | quantityToFloat64) ($v | toString | quantityToFloat64) }}{{ $limitsExplained = false }}{{ end }}
                            {{- else }}
                                {{- $limitsExplained = false }}
                            {{- end }}
                        {{- end }}

Raw string comparison missed equivalent-but-differently-formatted
quantities (e.g. "1000m" vs "1"), producing false negatives on the
"matches LimitRange defaults" note. Use quantityToFloat64 for the
comparison, as done elsewhere in this file.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@bergerx

bergerx commented Jul 11, 2026

Copy link
Copy Markdown
Owner Author

Addressed the quantity-comparison review comment in 4bb9762 — now compares via quantityToFloat64 instead of raw string equality, verified live with a LimitRange/Pod using 1000m vs 1.

@bergerx
bergerx merged commit 38beb7d into master Jul 11, 2026
2 checks passed
@bergerx
bergerx deleted the worktree-bridge-cse_01PXvTzfoyx1LwrMc8myRfEb branch July 11, 2026 12:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant