Repository navigation
Keep hand-raised memory limits across oap install - #52
josephschorr wants to merge 1 commit into
Conversation
oap install used to reset every component's memory limit to the bundled value, so a limit raised by hand to stop an OOM crashloop was undone by the next upgrade. Install now reads the live Deployment/StatefulSet and carries its container memory limits forward.
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
samkim
left a comment
samkim
left a comment
There was a problem hiding this comment.
Review findings from a /code-review medium pass (read-only; findings not independently verified or tested).
The two most important are on line 60: the code can't distinguish a limit a person raised from one ap-install wrote or a mutator injected. Test gaps: no case for a live value written by an older bundle, a live limit lower than the bundle, multiple containers, or initContainers.
| c, _ := raw.(map[string]any) | ||
| name, _, _ := unstructured.NestedString(c, "name") | ||
| if limit, found, _ := unstructured.NestedString(c, "resources", "limits", "memory"); found && limit != "" { | ||
| liveLimits[name] = limit |
There was a problem hiding this comment.
Any differing live limit is kept, including one a previous oap install wrote, so a new release can't change a bundled limit on an existing cluster.
Release N ships the operator at 512Mi; release N+1 bumps config/manager/deployment.yaml to 1Gi. On every cluster upgraded from N, the live 512Mi (written by ap-install, not a person) differs from the bundle, gets carried forward, and is narrated as "kept live memory limit". The fix never lands and the OOM crashloop comes back.
Same root cause, second symptom: a limit set by an admission mutator or policy engine (GKE Autopilot resource adjustment, Kyverno/Gatekeeper) is treated as hand-raised and pinned, even when it's lower than the bundle. ap-install then force-owns the downgraded value.
Suggested fix: only keep a limit owned by a field manager other than ap-install (check managedFields), and/or only when it's larger than the bundled value. Alternatively take overrides as an explicit install input (see the comment on line 36).
There was a problem hiding this comment.
This is expected - once the limit is set, it should be maintained
| // no live memory limit, keeps the bundled value. A missing live object (a fresh | ||
| // install) is a no-op. | ||
| // | ||
| // The applied payload now depends on the live object, not only the bundle, but |
There was a problem hiding this comment.
This conflicts with the repo rule in CLAUDE.md: "Keep the SSA payload a pure function of its inputs. The fields a client applies must derive solely from its inputs (the bundle, the flags)". The applied resources.limits.memory now depends on a live GET. An explicit override input (install flag or values override) would satisfy the rule and also avoid the release-bump and mutator problems flagged on line 60.
There was a problem hiding this comment.
That would break if someone forgets it, which is opposite the intention - once its set, it should remain overridden
| } | ||
|
|
||
| liveLimits := map[string]string{} | ||
| liveContainers, _, _ := unstructured.NestedSlice(live.Object, "spec", "template", "spec", "containers") |
There was a problem hiding this comment.
Errors from NestedSlice, NestedString and the raw.(map[string]any) assertion are discarded. If the live containers field has an unexpected shape, liveLimits stays empty and the bundled limit overwrites the hand-raised one with no log line, which is the regression this PR exists to prevent. Per CLAUDE.md "Never silently drop errors", return or log these.
| return nil | ||
| } | ||
|
|
||
| containers, found, err := unstructured.NestedSlice(d.Object, "spec", "template", "spec", "containers") |
There was a problem hiding this comment.
Only spec.template.spec.containers is handled. A raised limit on an initContainers entry (including native sidecars with restartPolicy: Always) is still reset on every install.
| return fmt.Errorf("%s %s/%s container %q: live memory limit %q: %w", d.GetKind(), d.GetNamespace(), d.GetName(), name, limit, err) | ||
| } | ||
| bundled, _, _ := unstructured.NestedString(c, "resources", "limits", "memory") | ||
| if bundled == limit { |
There was a problem hiding this comment.
String comparison, not quantity comparison: if the bundle says 1024Mi and the apiserver returns 1Gi, every re-install prints "kept live memory limit" though nothing changed. limitQty is already parsed; compare with resource.Quantity.Cmp.
| return err | ||
| } | ||
| if req, found, _ := unstructured.NestedString(c, "resources", "requests", "memory"); found { | ||
| if reqQty, err := resource.ParseQuantity(req); err == nil && reqQty.Cmp(limitQty) > 0 { |
There was a problem hiding this comment.
A bundled request that fails to parse is skipped silently (err == nil &&). The request isn't lowered, and the failure surfaces later as an opaque apply rejection that doesn't mention the preserved limit. Return the parse error with context.
Related: only the limit is carried. A request raised alongside it (the usual kubectl set resources --requests=memory=2Gi --limits=memory=4Gi) is reset to the bundled 256Mi, undoing the scheduling guarantee.
| } | ||
| containers[i] = c | ||
| if info != nil { | ||
| info("kept live memory limit %s on %s %s/%s container %s (bundle ships %s)", limit, d.GetKind(), d.GetNamespace(), d.GetName(), name, bundledOrNone(bundled)) |
There was a problem hiding this comment.
When the request is lowered to the kept limit (e.g. bundled request 256Mi, live limit 128Mi), nothing is printed. Consider narrating the request change too.
Re-running
oap installno longer resets a component's memory limit to the bundled default. If an operator has raised a limit on the cluster (for example, to stop the operator from being OOM-killed on a busy install), install keeps that value and says so in its output. Fresh installs are unchanged.