docs(dha): resolve SSA-handoff open question — client-side apply, not omit - #66
docs(dha): resolve SSA-handoff open question — client-side apply, not omit#66scooby87 wants to merge 14 commits into
Conversation
… not omit The PoC settled the one migration detail §Upgrade left open. On the current platform (helm-controller v1.5.0, server-side apply by default) the chart-only omit handoff is not safe: the HPA claims the /scale field-manager only when it actually writes, so a cluster idle at its floor never owns .spec.instances and the phase-2 omit prunes it (CNPG defaults to 1, quorum-fatal). The implementation keeps a constant seed and forces client-side apply per app via release.cozystack.io/helm-server-side-apply, under which the seed is a merge no-op and KEDA's live value survives. Records the finding in §Upgrade and marks the Open Questions item resolved. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
VerdictNOT LGTM The resolution this document records does not work: forcing client-side apply does not make a rendered constant a merge no-op, because Helm's three-way merge computes its add-and-change delta from the live object to the new manifest, not from the old manifest to the new one. Findings[CRITICAL] The new bullet's load-bearing sentence is that the chart renders a constant seed and, "under [forced client-side apply] the constant render is a merge no-op and KEDA's live value survives". That is Helm 2 behaviour. Helm 3 changed it deliberately, and Helm 4 (embedded in helm-controller v1.5.0) keeps the change. The add-and-change half of the patch is computed against live state: // k8s.io/apimachinery@v0.35.0/pkg/util/jsonmergepatch/patch.go:29-42
// Create a 3-way merge patch based-on JSON merge patch.
// Calculate addition-and-change patch between current and modified.
// Calculate deletion patch between original and modified.
func CreateThreeWayJSONMergePatch(original, modified, current []byte, fns ...mergepatch.PreconditionFunc) ([]byte, error) {
...
addAndChangePatch, err := jsonpatch.CreateMergePatch(current, modified)
Concretely: seed renders The irony is that the document already states the correct rule three lines up, in the bullet at What this needs before it can be recorded as settled: either a mechanism that genuinely leaves What would change my mind: a demonstration that helm-controller v1.5.0 skips the object entirely when the rendered manifest is byte-identical to the previous revision, rather than computing a patch against live. I did not find that path in Helm v4's client-side apply; the patch-is-empty short circuit compares the computed patch, and the computed patch is non-empty precisely when live has drifted. [MAJOR] The bullet presents
cozystack/cozystack#3954 does not add it. Its head is // internal/operator/package_reconciler_test.go, TestBuildHelmReleaseSpecNoForceOrDriftDetection
// (Server-side apply is not a HelmRelease knob — the
// helm-controller applies releases with Helm's own three-way merge — so there is no SSA
// field to assert here; that merge behavior IS the invariant these fields would break.)Server-side apply is a HelmRelease knob, three of them, and the version cozystack pins carries all three: // github.com/fluxcd/helm-controller/api@v1.5.1/v2/helmrelease_types.go:593-596, 840-846, 1111-1117
// ServerSideApply enables server-side apply for resources during install.
// Defaults to true (or false when UseHelm3Defaults feature gate is enabled).
ServerSideApply *bool `json:"serverSideApply,omitempty"`
// ServerSideApply enables server-side apply for resources during upgrade.
// Can be "enabled", "disabled", or "auto".
// Defaults to "auto".
ServerSideApply ServerSideApplyMode `json:"serverSideApply,omitempty"`So the two artifacts disagree with each other, by the same author, a day apart, and neither is reconciled. A design proposal that names a mechanism nobody has built, in present tense, sends the next implementer looking for an annotation that returns no grep hits. [MAJOR] The version pin checks out:
The resolution reads as // helm-controller v1.5.0 internal/action/upgrade.go:60-77
if upgrade.ServerSideApply == "auto" {
lastRelease, err := config.Releases.Last(releaseName)
...
serverSideApply = lastReleaseTyped.ApplyMethod == "ssa"Since cozystack sets neither field, the platform's behaviour splits by install date. A postgres database that predates the Flux 2.8 upgrade is already on client-side apply and the proposed annotation would be a no-op for it; one created afterwards is on SSA. §Upgrade is the section about enabling autoscaling on an existing database, which is exactly the population the premise is false for. Two databases in the same cluster running the same chart behave differently here, and the document describes one uniform behaviour. [MAJOR] The bullet immediately after the insertion says: "Steady state after migration is correct. With the field absent from both the previous and the current render, three-way merge leaves the HPA-set Four more sites still teach omit, none of them touched:
The append-only shape is what produces this. A targeted rewrite of §3, §Testing, Appendix finding 4 and the two §Upgrade bullets is smaller than the note that now sits on top of text contradicting it, and it leaves the document saying one thing. [MAJOR] PR-level, a merged proposal is revised with a design change and no decision record
The "when to write one" list at This is not paperwork. Two of the three findings above are visible only because the reasoning behind the change is compressed into one bullet: the SSA premise and the merge-no-op premise each get one clause, and neither is separable from the other in the current text. A one-page record with the alternatives it beat is where they would have had to stand on their own. [MINOR] Independent of the finding above, and it survives even if that one is wrong. The bullet claims safety "across enable / disable / dry-run", which enumerates three transitions and omits this fourth one. Raising the floor after watching autoscaling work is an ordinary operator action, not an exotic one. #3954's values documentation acknowledges the dryRun and disable arms of the same hazard; neither it nor this document names the raise-the-floor arm. [MINOR]
Worth naming because SSA is load-bearing for that kind on its own account. Claim mismatches[MISSING] "forces CLIENT-SIDE (three-way-merge) apply for the tenant HelmRelease via a per-app annotation ( [MISSING] "under which the constant render is a merge no-op and KEDA's live value survives", refuted. [PARTIAL] "The platform's helm-controller (v1.5.0) defaults to server-side apply", the version pin and the install default are correct; the upgrade default is [PARTIAL] "Implemented in cozystack/cozystack#3954", the constant seed is implemented; the forced client-side apply and the annotation are not. #3954 is open, unmerged, and depends on #3951. [PARTIAL] "deterministic and safe across enable / disable / dry-run with no handoff race", the three named transitions are the ones #3954 handles; an edit to [UNVERIFIABLE] "Validated live on dev9", this review is hermetic and does not touch clusters, so what ran on dev9 cannot be confirmed or refuted here. The two observations recorded do not span the enable / disable / dry-run matrix the same sentence claims safety across. The second one ("under the forced client-side apply the constant seed is a no-op that preserves the Caveats
Recommended follow-ups
|
…n 0001 Address review on #66: the append-only "Implementation update" note asserted the new seed+client-side design on top of §3/§Testing/Appendix text that still taught the omit design, so the document said both things at once. Replace it with a targeted rewrite that leaves the proposal saying one thing — §3 now describes the constant seed applied client-side, §Upgrade reframes the enable/steady-state bullets around it (no field-deletion handoff), §Testing asserts the seed render plus the client-side apply invariant, the Open-questions item and Appendix finding 4 point at the record, and Alternatives lists the omit design as rejected. Add decision record 0001, which is where the reasoning now lives: why chart-only omit does not survive helm-controller v1.5.0 (SSA force-revert of rendered fields; idle-at-floor prune to CNPG's default of 1), and why forcing client-side apply makes a constant seed safe — refuting the review objection that Helm's three-way merge re-asserts the constant from live. Helm v4 routes unstructured/CRD objects through a two-way CreateMergePatch(original, modified) unless threeWayMergeForUnstructured is set, which helm-controller v1.5.0 never sets; verified empirically on Helm 4.0.4. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>
|
Thanks for the depth here — this is exactly the kind of review that's worth answering carefully. I dug into each point; the summary is that the CRITICAL does not hold, but several of the surrounding findings do, and I've acted on all of them. On the CRITICAL — client-side apply does preserve the seedThe objection rests on
helm-controller v1.5.0 never sets that flag — there is no I verified this empirically rather than trusting the read, using Helm 4.0.4 (the same SDK helm-controller v1.5.0 embeds) against a real custom resource (the JSON-merge path, not a built-in strategic-merge type):
So the second row is precisely the scenario in your finding, and the constant survived. Only SSA+force reverts — which is why the fix is to force client-side, not to rely on SSA. This is written up as decision record 0001 in this PR, with the source citations and the matrix, and it's called out in "Revisit if": if helm-controller ever flips that flag's default, the no-op breaks and your analysis becomes correct. On the findings that were right
Not settled hereThe apply-strategy matrix on a dev stand (install-era × annotation) that you recommend is the right verification and belongs against #3954, not this doc PR; the empirical run above covers the load-bearing cell (client-side preserves under a chart change). Happy to keep the Open-questions item open if you'd still prefer, but with the two-way path established at the source I've marked it resolved and pointed it at the record. |
|
Re-reviewed at Withdrawing the CRITICALYou are right about the upgrade path, and my CRITICAL was wrong. I traced the merge to the wrong arm of the branch and the verdict rested on it. Where the misread was: I quoted Re-verified against the version actually shipped. helm-controller v1.5.0 pins
So on the upgrade action an unchanged rendered seed produces an empty patch and the live value survives. The second row of your table is right and the decision is sound. I'm not reopening it. The rewrite also closes what I raised about the document asserting both designs. I checked each site rather than taking the summary: §3 heading and code block, §3 prose, the mermaid Two things remain. Both are text, not design. [MAJOR] "never from live state" is unqualified, and it is false on the path this decision switches onThe document states the invariant absolutely in four places:
That holds for
Since #3954 renders Record 0001 carries the consequence in one sentence. "Revisit if" says the no-op is "gated today only by that flag's default". The flag's default is not the gate: What I haven't established, and am not claiming: how often helm-controller reaches an install action against an existing [MAJOR] §5's dry-run bullet still describes the mechanism the implementation rejected
#3954 rejects both halves of that, and says why in
So it stamps Two related gaps, same fix:
Verified and correct, so it does not get re-litigated
[MINOR] SDK version in record 0001"Confirmed empirically on Helm 4.0.4 (the same SDK helm-controller v1.5.0 embeds)": the embedded SDK is VerdictNOT LGTM, two text fixes away. The design decision is sound and I'm not reopening it. What blocks is that record 0001 freezes on merge while its "Revisit if" names the wrong gate, and that §5 still defines dry-run by the mechanism #3954 discarded. Qualify the four "never from live state" sentences, correct "Revisit if", rewrite the §5 dry-run bullet and add dry-run to §Upgrade's staging list, and I have nothing further. |
Reconcile four review findings on #66 against the mechanism actually implemented in cozystack/cozystack#3954: - §5 dry-run bullet described KEDA's full `paused` annotation, which deletes the HPA and leaves no recommendation to observe. The implementation pauses both directions via `paused-scale-in`/ `paused-scale-out`, keeping the HPA up and serving its metric while freezing desiredReplicas; the bullet now says so. - The four "does not/never consult live state" sentences now carry the qualifier that the no-op holds only under helm-controller's current two-way-merge default -- the same gate recorded in decision 0001's "Revisit if" -- so a future default flip does not silently falsify the prose. - Decision 0001's freeze note exempted nothing, contradicting the post-merge "Revisit if" trigger. It now marks "Revisit if" an active trigger acted on by superseding the record via the maintained header, not by editing the frozen body. - §Upgrade phase-1 names the pause mechanism precisely and adds an optional dry-run observation stage before actuation is handed over. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>
Round-2 review follow-up on the same four findings: - §Upgrade's resolved Open-questions item carried a fifth, unqualified "never re-asserted from the live count" -- the same claim the other four sentences now gate. Qualify it with helm-controller's current two-way-merge default so the whole set is consistent. - Adding the optional dry-run stage left "two-phase order ... both phases" describing three bolded stages. Note the observation window is between the two phases and count all three stages in the same-count invariant. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>
The "PoC first" bullet gated the whole plan ("gates everything else")
on a claim that was both wrong and CNPG-only:
- It named the bare `spec.subresources.scale` as the assumption, but
the scale subresource existed even on the 1.27.x the PoC ran on --
what the HPA actually requires is `status.selector`, absent below
the CNPG 1.28.4 floor.
- It asserted the assumption is "present in the currently vendored
CNPG", contradicting §Scope/§PoC (vendored 1.28.2 is below the
1.28.4 floor).
- It wrote the gate as CNPG-specific, but §"Engine scope of the MVP"
covers MariaDB (`MariaDB.spec.replicas`) too, and the selector
requirement is a stock-HPA property against any scale subresource --
the same gate MariaDB must clear and the one Redis/MongoDB fail
outright.
Restate it per-engine: the scale subresource must serve
`status.selector`; CNPG >= 1.28.4 for the shipping PostgreSQL, re-clear
for MariaDB, N/A for the subresource-less engines.
Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>
The proposal still described the version floor as unmet: "currently vendors CNPG 1.28.2, two patch releases below the floor", "the bump is a separate cozystack PR", "currently ships 1.28.2 ... does not serve it". That bump has merged -- cozystack/cozystack#3951 raised CNPG to 1.30.0 on 2026-08-26, so main already clears the >= 1.28.4 floor. Update the three present-tense spots (§PoC Resolution, §Scope Precondition, §Testing) to state the floor is satisfied and reference #3951, so the PoC gate reads as "confirm the running operator serves the selector", not "wait for a pending dependency". Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>
The illustrative §3 template had only two arms (enabled -> seed, else -> replicas), but §Upgrade phase-1 and §5 dry-run both rely on a third rendering mode: enabled=true yet rendering the raw replica count via a `transition`/`dryRun` sub-flag. A reader could not reconcile the two sections. Gate the seed arm on `enabled AND not transition AND not dryRun`, note that the two staging modes fall to the static-count arm, and align the prose that described the branch as keying off `autoscaling.enabled` alone. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>
…ersion Findings from an independent three-lens re-review, all verified: - [MAJOR] The "one residual write path" was scoped to three tenant value edits, but a platform chart upgrade that changes how the seed is rendered (effectiveMin/seed helper or a schema default) rebases an HPA-grown .spec.instances with no tenant edit. Generalize the caveat in §Upgrade and decision 0001, and add an upgrade-of-active-cluster case to Testing. - [MINOR] Decision 0001 Status was Accepted while the proposal PR is open and unreviewed; set it to Proposed (flips on merge, per the record's own convention). - [MINOR] Decision 0001 cited "Helm 4.0.4 (the same SDK helm-controller v1.5.0 embeds)"; v1.5.0 pins helm.sh/helm/v4 v4.1.1. State both and note the createPatch branch is identical. - [MINOR] "suspend-and-pin recipe in ... §Upgrade" pointed at a recipe §Upgrade did not carry; attribute it to values.yaml and name the stage/transition procedure §Upgrade actually documents. - [MINOR] Note that MariaDB does not inherit Postgres's basis for the per-kind client-side-apply trade and must re-justify it. - [NIT] "per application" -> "per ApplicationDefinition (per kind)". Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict
NOT LGTM
The record's central refutation holds only for the upgrade path: Helm itself turns on three-way merge for unstructured objects during install-with-adoption, and the enabling condition is the very annotation this decision introduces.
Findings
[MAJOR] design-proposals/database-horizontal-autoscaling/decisions/0001-client-side-apply-preserves-the-autoscaler-seed.md:27, the "refuted at the source" argument covers upgrade only; Helm enables three-way merge for unstructured objects itself on install-with-adoption
The bullet concludes: "helm-controller v1.5.0 never sets it (no reference anywhere in the controller), so the default two-way path applies." The first half is right, and I confirmed it (zero hits for threeWayMerge across the v1.5.0 tree). The conclusion does not follow, because helm-controller is not the only party that can set the flag. Helm's own install action sets it, and both of its conjuncts are satisfied by this design:
// helm/helm @ v4.1.1, pkg/action/install.go:503 (branch taken when len(toBeAdopted) > 0)
updateThreeWayMergeForUnstructured := i.TakeOwnership && !i.ServerSideApply // Use three-way merge when taking ownership (and not using server-side apply)
_, err = i.cfg.KubeClient.Update(
toBeAdopted, resources,
kube.ClientUpdateOptionServerSideApply(i.ServerSideApply, i.ForceConflicts),
kube.ClientUpdateOptionThreeWayMergeForUnstructured(updateThreeWayMergeForUnstructured),// fluxcd/helm-controller @ v1.5.0, internal/action/install.go:91
install.TakeOwnership = !obj.GetInstall().DisableTakeOwnership // default trueTakeOwnership defaults to true, and Install.ServerSideApply=false is exactly what the annotation in the Decision paragraph forces. toBeAdopted is populated from live objects, not from release history:
// helm/helm @ v4.1.1, pkg/action/validate.go:41
// requireAdoption returns the subset of resources that already exist in the cluster.And on that path the add-and-change half of the patch is computed from the live object toward the rendered one, which is precisely the reading the bullet calls refuted:
// k8s.io/apimachinery @ v0.35.0, pkg/util/jsonmergepatch/patch.go:28-41
// Create a 3-way merge patch based-on JSON merge patch.
// Calculate addition-and-change patch between current and modified.
// Calculate deletion patch between original and modified.
func CreateThreeWayJSONMergePatch(original, modified, current []byte, ...) ([]byte, error) {
addAndChangePatch, err := jsonpatch.CreateMergePatch(current, modified)// helm/helm @ v4.1.1, pkg/kube/client.go:1023 and :1047-1055
currentObj, err := helper.Get(target.Namespace, target.Name) // the live GET
...
if isUnstructured || isCRD {
if threeWayMergeForUnstructured {
patch, err := jsonmergepatch.CreateThreeWayJSONMergePatch(oldData, newData, currentData, preconditions...)So a Cluster the HPA has grown to 6, adopted by an install that renders the constant seed 2, receives {"spec":{"instances":2}}. Adoption during install is reachable whenever the object outlives the release record: release history lost while objects remain, an uninstall on a resource carrying helm.sh/resource-policy: keep, or a chart split that moves the Cluster under a new release name, which is a recurring shape in this codebase.
The Revisit if line at :39 locates this risk in a future upstream change ("helm-controller starts enabling threeWayMergeForUnstructured by default"). One of its two conjuncts is already set by helm-controller today, and the other is set by this decision, so the condition is satisfiable now on the install path rather than later.
Concretely: scope the claim. State that the two-way guarantee holds on upgrade, name the install-with-adoption path as the exception with the TakeOwnership && !ServerSideApply condition, and say what the design does about it. DisableTakeOwnership: true on the emitted HelmRelease is the one knob that removes the conjunct, and if it is rejected the residual path belongs in Consequences, not in a future-tense trigger. Whatever the resolution, this record freezes its prose on merge, so the scoping has to land before then.
[MAJOR] design-proposals/database-horizontal-autoscaling/README.md:178, migration phase 1 is not inert on a cluster below the rendered floor, and minReplicaCount is not pinned to N
The bullet says the ScaledObject is rendered "paused in both directions (autoscaling.keda.sh/paused-scale-in/paused-scale-out, minReplicaCount pinned to N)", that KEDA "begins observing load while actuating neither way", and that the phase-1b window "is inert on the running cluster". Two separate problems.
The pause is a behavior policy, and the HPA controller only consults behavior policies on the metrics branch:
// kedacore/keda @ v2.20.2, controllers/keda/hpa.go:86-99
if scaledObject.NeedToPauseScaleIn() {
disabledPolicy := autoscalingv2.DisabledPolicySelect
behavior.ScaleDown.SelectPolicy = &disabledPolicy// kubernetes/kubernetes @ v1.33.4, pkg/controller/podautoscaler/horizontal.go:811-824, 868-871
rescale := true
...
} else if currentReplicas < minReplicas {
rescaleReason = "Current number of replicas below Spec.MinReplicas"
desiredReplicas = minReplicas
} else {
... normalizeDesiredReplicasWithBehaviors(...) // only here
rescale = desiredReplicas != currentReplicas // only here
}
if rescale {
scale.Spec.Replicas = desiredReplicas
_, err = a.scaleNamespacer.Scales(hpa.Namespace).Update(ctx, targetGR, scale, metav1.UpdateOptions{})rescale starts true and is only recomputed inside the metrics branch. When currentReplicas < minReplicas the controller writes /scale without ever reaching normalizeDesiredReplicasWithBehaviors, so SelectPolicy: Disabled does not apply.
The second problem: the implementation this PR points at renders the bound unconditionally from the quorum helper, with no transition override:
# cozystack/cozystack#3954, packages/apps/postgres/templates/scaledobject.yaml
"minReplicaCount" (int (include "postgres.autoscaling.effectiveMin" .))
"maxReplicaCount" (int (include "postgres.autoscaling.effectiveMax" .))That matches §5's own formula at README.md:123, which carries no transition exception either. So "pinned to N" describes neither the implementation nor §5.
The narrative: a tenant with a single-instance Postgres follows phase 1 as written. It stages replicas = 1 (the live count), sets enabled: true with transition: true. The chart renders instances: 1; KEDA creates an HPA with minReplicas: 2 (max(minReplicas, maxSyncReplicas+1, 2)) and both pause annotations. The HPA sees currentReplicas(1) < minReplicas(2), takes the below-min branch, and writes /scale = 2. CNPG provisions a second instance with its PVC and DRBD volume during the window the document says is safe to hold in. That is the same footprint change README.md:182 insists must be "a conscious enablement decision, not a surprise", arriving during the phase whose stated purpose is to observe before committing.
Fix either the claim or the mechanism. If phase 1 is meant to be non-actuating, the transition branch has to render minReplicaCount at the live count and the document has to say so and reconcile it against §5's "quorum wins, never clamp below a safe quorum" rule for the N < floor case. If it is not meant to be non-actuating, drop "inert" and "actuating neither way" and state the scale-up as part of phase 1. Either way the bracketed "minReplicaCount pinned to N" should stop asserting something no template does.
[MAJOR] design-proposals/database-horizontal-autoscaling/README.md:126, the branch switch into the static-count path is a third write path on .spec.instances, and its target is not floor-clamped
decisions/0001-...md:34 enumerates the residual write paths as "Two triggers": a tenant edit to the seed's inputs, and a platform upgrade that changes how the seed is rendered. Both move the seed while autoscaling stays active, so both land on max(replicas, effectiveMin) and the parenthetical guarantee "never below the quorum floor, never a collapse to 1" holds for them.
There is a third. Setting dryRun or transition on a live autoscaled cluster switches the render from the seed expression to the raw one at README.md:99:
instances: {{ .Values.replicas }} # static count — off, or the transition / dry-run staging modes (§Upgrade, §5).Values.replicas is not clamped to effectiveMin, so this write path can go below the quorum floor, unlike the two the record lists. §5's dry-run bullet presents the mode as observation before actuation and says nothing about it touching the live count at all.
The implementation already documents what the proposal and the record do not:
# cozystack/cozystack#3954, packages/apps/postgres/values.yaml, @field dryRun
Like disabling, flipping a LIVE autoscaled cluster into dryRun re-renders `instances: replicas`
(the raw static value, which can be below the autoscaling floor); if `replicas` is below the
live count, stage it to the live count first (see the enablement note) or the cluster sheds
down to `replicas`.
The chart also carries a {{- fail }} on the static branch for replicas <= maxSyncReplicas, which catches the wedge case but not the general shed. So the documents under review are less accurate about this than the code they describe. Add the branch switch as a third trigger in the record's Consequences, note that its target is the unclamped value, and give §5's dry-run bullet the same "stage replicas to the live count first" precondition the enable and disable paths already carry.
[MINOR] design-proposals/database-horizontal-autoscaling/README.md:180, the disable bullet still describes the omit mechanism
"Setting autoscaling.enabled: false re-introduces instances: {{ .Values.replicas }}" and "so Flux reasserts the current value rather than dropping to the default" both belong to the design §3 no longer has. Under the new §3 the field is present in every branch, so nothing is re-introduced; and when the previous and new renders agree the two-way patch is empty, so Flux asserts nothing rather than reasserting the current value. The prescribed steps still produce the right outcome, but their stated rationale is the old one. This also narrows the PR body's claim that the supersession touches "the apply-strategy detail only": this bullet's correctness argument rested on three-way delete semantics and was left untouched.
[MINOR] design-proposals/database-horizontal-autoscaling/decisions/0001-client-side-apply-preserves-the-autoscaler-seed.md:15, the Context conflates the ForceConflicts assignment with the apply-strategy default for upgrade and rollback
The cited line is exact:
// fluxcd/helm-controller @ v1.5.0, internal/action/install.go:53
install.ForceConflicts = install.ServerSideApply // We always force conflicts on server-side apply.upgrade.go:77 and rollback.go:93 do carry the same ForceConflicts assignment, so "same in upgrade.go/rollback.go" is right about that. The default is not the same:
// internal/action/upgrade.go:103
upgrade.ServerSideApply = "auto" // This must be the upgrade default regardless of UseHelm3Defaults.
// internal/action/upgrade.go:74
serverSideApply = lastReleaseTyped.ApplyMethod == "ssa"Upgrade and rollback default to auto, which inherits the previous release's apply method, and the install default is itself behind a controller-wide feature gate (internal/features/features.go:118-120, UseHelm3Defaults: false). "Defaults new HelmReleases to server-side apply" is accurate for a new release; the parenthetical reads as though the upgrade path defaults to SSA too. It matters for the record's own reasoning: a Postgres release predating v1.5.0 was already applying client-side on every upgrade, so the "reverted on every apply" failure never touched that population. That is the same split the last Consequences bullet handles from the other direction, and stating it here would make the two consistent.
[MINOR] design-proposals/database-horizontal-autoscaling/README.md:107, autoscaling.transition is load-bearing in the render condition but absent from the document's own values contract
The seed branch keys off "enabled, and neither transition nor dryRun", and the phase-1 runbook at :178 turns on transition. The tenant-facing block at :137-147 lists enabled, minReplicas, maxReplicas, target, maxReplicationLagSeconds and dryRun, with no transition. §5 states that block is "validated by values.schema.json, like every other cozystack knob", so as documented the migration procedure sets a key the contract does not admit. The implementation does declare it (values.schema.json lists transition in required, plus a ## @field {bool} transition), so this is the proposal trailing the code.
[MINOR] design-proposals/database-horizontal-autoscaling/README.md:126, no KEDA version floor for the per-direction pause annotations
The annotations are real, and I verified them:
// kedacore/keda @ v2.20.2, apis/keda/v1alpha1/scaledobject_types.go:58-61
const PausedScaleInAnnotation = "autoscaling.keda.sh/paused-scale-in"
const PausedScaleOutAnnotation = "autoscaling.keda.sh/paused-scale-out"They were introduced in v2.18.0 (CHANGELOG.md:309-310, "Add support for pause scale in annotation (#6902)", "Add support for pause scale out annotation (#7022)"). Below that version KEDA ignores unknown annotations, so a ScaledObject the document calls non-actuating would actuate immediately, with nothing reporting the difference. This PR is what made the two annotations load-bearing in both §5 and phase 1, and it is where the "or use KEDA's pause annotation" hedge became specific. The proposal states the CNPG floor precisely twice and leaves KEDA's version to ## Open questions. Give it a floor of KEDA >= 2.18.0 the same way, especially since README.md:222 no longer lists this as open.
[MINOR] design-proposals/database-horizontal-autoscaling/decisions/0001-client-side-apply-preserves-the-autoscaler-seed.md:5, Status: Proposed is outside the documented enumeration
design-proposals/decision-template.md gives Status: Accepted | Superseded by NNNN | Reverted, and design-proposals/README.md:103 lists "A decision that has not been made yet" among the things not to write a record for. Proposed (flips to Accepted on merge of the proposal PR) invents a fourth state. If a record has to exist before its own approval, the state belongs in the template rather than in one record, so the next author does not have to guess.
Claim mismatches
[PARTIAL] "forces client-side (three-way-merge) apply for the tenant HelmRelease" (PR body). The attached record's entire refutation is that the client-side path for unstructured objects is a two-way merge (helm/helm @ v4.1.1, pkg/kube/client.go:1061, jsonpatch.CreateMergePatch(oldData, newData)). The body names the mechanism the record spends a paragraph refuting.
[PARTIAL] "This supersedes §3's 'omit the field' on the apply-strategy detail only; the ownership goal is unchanged" (PR body). §Upgrade's disable bullet at README.md:180 and §5's dry-run bullet at :126 also rested on omit-era semantics and were not brought forward (findings above).
[PARTIAL] "deterministic and safe across enable/disable/dry-run with no handoff race" (PR body). Enable is covered. Dry-run on a live autoscaled cluster has an undocumented write path to the unclamped replicas, and disable's stated mechanism no longer describes what happens.
Operational risks
decisions/0001-...md:34names a platform chart upgrade that changes the seed helper as a residual write path that rebases an HPA-grown count "with no tenant edit at all", then prescribes staging.Values.replicasto the live count as the mitigation. In this platform the two are owned by different actors: the tenant owns the application values, the platform release schedules the chart bump.README.md:179's "treat a chart bump of an autoscaling-active database the same way rather than applying it blind" does not say who performs it or how a platform operator identifies which tenant databases are autoscaling-active before rolling a release. As written the mitigation is not reachable by the actor who would need to apply it. Either name the mechanism that makes it reachable (a release-note gate, a pre-upgrade check that enumerates autoscaling-active databases, or a seed helper versioned so a bump cannot move it silently), or record that the trigger is accepted unmitigated.
Caveats
- Hermetic review, no cluster contacted. Nothing was rendered: this repository contains no charts. The configuration-corner analysis is against the §3 snippet plus the templates read in
cozystack/cozystack#3954, which is OPEN and unmerged (verified:state: OPEN,mergedAt: null). To turn that reading into a render, the command ishelm template r packages/apps/postgres --set autoscaling.enabled=true --set autoscaling.transition=true --set replicas=1on that branch, plushelm unittest packages/apps/postgres. - Corners walked, and where each is addressed:
enabled=false(:180, stale mechanism);enabledalone (:103,:179, covered);enabled+transition(:178, finding 2);enabled+dryRun(:126, findings 2 and 3);enabled+transition+dryRun(not addressed, same static branch, no harm found). Transitions: off to transition (both renderreplicas, empty patch); transition to active (moves only whenreplicas < effectiveMin, the document states this parenthetically); active to dryRun and active to off (finding 3); active to active under a changed seed helper (Operational risks). - §Testing at
:207-208adds the client-side invariant assertion and the chart-bump rebase exercise. Three corners of this design's own mechanism have no test named: install-with-adoption re-asserting the seed (finding 1), flippingdryRunon a live autoscaled cluster (finding 3), and a paused ScaledObject whoseminReplicaCountexceeds the live count (finding 2). - Verified sound, so these should not need re-litigating.
Install.ServerSideApply *boolandUpgrade.ServerSideApplyas enumenabled;disabled;auto(fluxcd/helm-controller @ v1.5.0, api/v2/helmrelease_types.go:593-596and:840-846), so §Testing'sInstall.ServerSideApply=false/Upgrade.ServerSideApply=disabledpair is exactly right, asymmetry included. The annotation is real and threaded as described (pkg/config/config.go:18523in the PR diff,HelmServerSideApplyAnnotation = "release.cozystack.io/helm-server-side-apply", consumed inrest.goconvertApplicationToHelmRelease). Two-way as the unstructured default with nothreeWayMergereference anywhere in helm-controller v1.5.0.helm.sh/helm/v4 v4.1.1ingo.mod:44. HPA writes/scaleonly whendesiredReplicas != currentReplicas(horizontal.go:866) and setsInvalidSelectorbefore any metric-type branching (horizontal.go:305,:386-392). CNPGspec.instancesdefaults to 1 (api/v1/cluster_types.go:256-259@ v1.30.0) and the webhook rejectsmaxSyncReplicas >= instances(internal/webhook/v1/cluster_webhook.go:1582-1587). cloudnative-pg#8996 merged 2026-06-04;labelSelectorPath: .status.selectorpresent in the v1.28.4, v1.29.2 and v1.30.0 CRDs and absent from v1.28.3, v1.29.1 and v1.27.4, so the stated floor and the no-backport claim both check out. cozystack#3951 merged 2026-08-26, andpackages/system/postgres-operator/charts/cloudnative-pg/Chart.yamlon main readsappVersion: 1.30.0. KEDA's fullpausedannotation does delete the HPA (controllers/keda/scaledobject_controller.go:361-363), so the contrast drawn at:126is correct. - The record's reproduction was run on Helm 4.0.4 while the platform embeds v4.1.1, and the record says so and argues the carry-over from source. I re-read v4.1.1's
createPatchand the two-way arm is identical, so the carry-over holds on the upgrade path. It is finding 1's install path that the reproduction does not reach, not the version gap. - The annotation is applied only on the aggregated-apps-API path (
rest.go, guarded byif r.releaseConfig.HelmServerSideApply != nil). A HelmRelease for the postgres chart authored directly rather than through the apps API keeps the platform default and would revert the seed. The record's "per-kind, so it reaches every Postgres" scope statement at:33reads wider than the code path. - Per Phase 0 I did not read the PR conversation or any prior review. The reviewer objection quoted in the record's Context is taken from the record's own account of it, not from the thread.
Recommended follow-ups
- The record's footer at
:41-49restates the shared template's frozen-prose rule with an added clarification aboutRevisit if. On re-reading it agrees withdesign-proposals/README.md:83rather than contradicting it, so nothing is wrong here, but per-record divergence in the format boilerplate is how the template stops being the reference. If the clarification is worth keeping, it belongs indecision-template.md. - The reviewer objection that the record exists to answer is attributed at
:17with a link to the pull request root rather than to the comment.design-proposals/README.md:118asks that every rejected alternative be sourced to "the comment, review or pull request it came from", and here the objection is the pivot of the whole record, so a comment permalink would make it checkable in one click instead of a thread scroll. dryRunandtransitionare independent booleans that select the same render branch and the same paused ScaledObject, differing only in intent (permanent versus migration phase 1). The composition is well defined and I found no failure in the both-set corner, but two flags for one rendered outcome is a schema shape worth revisiting before it reaches the values contract for a second engine.
…n staging, ignoreNullValues review findings
Prose-only fixes to the Database Horizontal Autoscaler proposal from
cozy-review. The seed + client-side apply mechanism is unchanged.
- Install-with-adoption reads live today. Forcing ServerSideApply=false
with TakeOwnership defaulting true makes Helm's install action compute
a three-way patch (helm v4.1.1 install.go:503), so a recovery reinstall
against a surviving Cluster rebases an HPA-grown count back to the seed.
Qualify the "does not consult live state" invariant to the upgrade /
steady-state path (README sec.3, Upgrade, Open questions, Appendix and
decision 0001 Decision); add it as a residual write path in
Consequences; fix the "Revisit if" gate (it is live by design, not a
future upstream flag flip).
- Single-instance phase-1 is not inert. The HPA's currentReplicas <
minReplicaCount branch writes /scale unconditionally (k8s v1.33.4
horizontal.go), bypassing the pause annotations, so a single-instance
source is bumped to effectiveMin immediately. Add the caveat to the
Upgrade phase-1 note and the Enablement constraint.
- Third residual write trigger. A standalone dryRun/transition flip on a
grown cluster renders the static instances: replicas (not floor-clamped)
and sheds the live count. Add it to decision 0001 Consequences ("Three
triggers") and to the Upgrade staging guidance.
- Missing ignoreNullValues: "false" in the rendered ScaledObject. KEDA's
default reads an empty/stale PromQL result as 0 and scales to the floor,
inverting the "never scale blind" fail-safe. Add the field to the render
and explain it in the fail-safe note.
Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>
…e rebase path Verification of the round-1 fix surfaced three text-only gaps left by the same absolute-claim cleanup; the mechanism is unchanged. - §5 dry-run/recommendation now carries the single-instance caveat already in phase-1b: the both-directions pause does not freeze a source at N=1, since the HPA floor branch writes /scale=effectiveMin unconditionally, bypassing the pause annotations. dryRun is a tenant-settable schema flag, so the caveat is stated at its canonical definition, not only phase-1. - backup/restore (cozystack#3959 purgeExistingCluster) is documented as a fourth residual write path: it deletes the Cluster+PVCs and re-creates from bootstrap.recovery, so Helm takes the Create arm and the cluster comes up at the seed, not the HPA-grown count. The staging recipe is inapplicable by construction — the purge erases live state on purpose. Recorded in decision 0001 Consequences, README §Upgrade, and §Testing. - the §Decisions index bullet gets the same install-with-adoption exception the five other absolute-claim sites already carry. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>
|
Thanks — you're right on all three, and I've corrected the text rather than the mechanism. Summary of what changed and why. 1. install-with-adoption reads live today (the main one)I traced the chain you pointed at and it holds exactly as you described. On the install action (not upgrade), helm v4.1.1 computes So the decision 0001 invariant was stated too absolutely. It is true for the upgrade / steady-state path (the two-way merge for unstructured objects, live never read), but not for the narrow install-with-adoption recovery path — a The rebase is bounded — the seed is What I changed:
2. single-instance phase-1 is not "inert"Also correct. For a single-instance source, What I changed: added the caveat to §Upgrade phase-1 (no longer calling that window "inert" for single-instance) and cross-linked it from the "Enablement constraint — 3. third residual write trigger (dryRun/transition flip)Added. A standalone Also folded in (minor)
Commit Round 2 (commit
|
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict
NOT LGTM
Re-reviewed at 2614afdd. Two of the three blockers from my last round are properly closed, and the install-with-adoption fix in particular is more thorough than what I asked for. What blocks now is a different set: the version floor the proposal restates as a hard precondition admits two CNPG releases where the mechanism provably does not work, the newly qualified "paused observation window" is not inert in the mirror case the qualification skipped, and record 0001's only tripwire watches a setting the named component does not have.
Closed since my last round
- install-with-adoption reads live state today. The invariant is now scoped to upgrade/steady-state in all its restatements, the exception is in Consequences with the condition, the file, the line and the version, and the bounding premise checks out:
grep -rn resource-policyacross all fifteen files ofpackages/apps/postgres/templatesatdb-autoscaler-kedareturns nothing, so an uninstall/reinstall remediation genuinely cannot reach it. TheRevisit ifline is a separate matter and is the third finding below. - The staging-mode flip as a third write path. Now enumerated in both documents, with the unclamped target stated in bold in the same §Upgrade bullet that carries the runbook, and the same warning duplicated on
dryRunin the implementation'svalues.schema.json. Nothing left here.
Findings
[MAJOR] design-proposals/database-horizontal-autoscaling/README.md:37, the CNPG >= 1.28.4 floor is not monotone: 1.29.0 and 1.29.1 do not serve status.selector
Lines 29 and 37, both rewritten in this revision, and line 208 state the precondition as a lower bound: "On any CNPG >= 1.28.4 the approved mechanism ... works byte-for-byte", and "Precondition — CNPG >= 1.28.4. The Cluster scale subresource must serve status.selector, or the HPA rejects it with InvalidSelector and never scales". The selector landed on the 1.28 patch branch and on main, but the 1.29.0 and 1.29.1 releases were cut without it and it only returns in 1.29.2. Two versions that satisfy the documented floor fail the property the floor exists to guarantee, and by this proposal's own §Testing wording the HPA then reports ScalingActive=False, reason=InvalidSelector before any metric branching, with no other signal.
Verified on two independent artefacts per tag, the kubebuilder marker and the CRD that actually ships:
$ for t in v1.28.3 v1.28.4 v1.29.0 v1.29.1 v1.29.2 v1.30.0; do
gh api "repos/cloudnative-pg/cloudnative-pg/contents/api/v1/cluster_types.go?ref=$t" \
--header 'Accept: application/vnd.github.raw' | grep -m1 'kubebuilder:subresource:scale'; done
v1.28.3 (no selectorpath)
v1.28.4 selectorpath=.status.selector
v1.29.0 (no selectorpath)
v1.29.1 (no selectorpath)
v1.29.2 selectorpath=.status.selector
v1.30.0 selectorpath=.status.selector
$ # config/crd/bases/postgresql.cnpg.io_clusters.yaml, count of labelSelectorPath:
v1.29.0 0
v1.29.1 0
v1.29.2 1
Nothing ships broken today, since main vendors 1.30.0, so this is a wrong gate rather than a live break. But the gate is what a reader applies to an external CNPG, to a pin on the 1.29 line, or to a future downgrade, and it is repeated at lines 7, 15, 23, 29, 37 and 208. Either state it as >= 1.28.4 excluding 1.29.0 and 1.29.1 (equivalently >= 1.29.2 on the 1.29 line), or make the gate behavioural rather than numeric, which is how §Testing already frames it: the check is whether .../scale serves a non-empty status.selector. A version number is a proxy, and it has now been shown to leak.
[MAJOR] design-proposals/database-horizontal-autoscaling/README.md:179, the phase-1 window is not inert for a source above maxReplicaCount either, and staging cannot prevent that one
This is the same defect I raised last round, in its mirror case. The bullet now says the paused window "is inert on the running cluster for a multi-instance source (N >= the floor)" and carves out exactly one exception, N = 1, on the correct grounds that the HPA's below-min branch writes /scale without consulting the pause annotations. The above-max branch behaves identically and is not carved out:
// kubernetes/kubernetes@v1.33.4 pkg/controller/podautoscaler/horizontal.go:790,811-871
currentReplicas := scale.Spec.Replicas
...
rescale := true
} else if currentReplicas > hpa.Spec.MaxReplicas {
rescaleReason = "Current number of replicas above Spec.MaxReplicas"
desiredReplicas = hpa.Spec.MaxReplicas
} else if currentReplicas < minReplicas {
desiredReplicas = minReplicas
} else {
... desiredReplicas = a.normalizeDesiredReplicasWithBehaviors(...)
rescale = desiredReplicas != currentReplicas
}
if rescale {
scale.Spec.Replicas = desiredReplicas
_, err = a.scaleNamespacer.Scales(hpa.Namespace).Update(...)
}Both pause annotations are implemented as behavior.Scale{Up,Down}.SelectPolicy = Disabled (kedacore/keda@v2.20.2 controllers/keda/hpa.go:85-118), and SelectPolicy is only read inside normalizeDesiredReplicasWithBehaviors, which both out-of-range branches skip while rescale keeps its initialised true. currentReplicas comes straight off scale.Spec.Replicas, so the branch fires on the spec value at the first HPA sync.
autoscaling.maxReplicas defaults to 6 (values.schema.json at db-autoscaler-keda). So an existing 8-instance Postgres taken through the documented phase 1 gets minReplicaCount: 2, maxReplicaCount: 6, both pause annotations, and instances: 8. The operator stages .Values.replicas = 8 exactly as instructed, the Helm patch is empty, and the HPA still writes /scale = 6 the moment it comes up, shedding two standbys and their PVCs inside the window this document calls an observation window. §5's "at most one standby is removed per period" does not hold here either, for the same reason: the above-max branch never reaches the scaleDown.policies the chart renders.
Unlike the seed-branch rebases, the staging recipe is structurally unable to help, because the trigger is maxReplicaCount against the live count, not the rendered instances. There is no render guard to catch it either: the chart fails on effectiveMin > maxReplicas and on replicas <= maxSyncReplicas, but nothing rejects or warns on replicas > maxReplicas.
You have most of the prose for this already. The implementation's values.yaml documents the branch under @field maxReplicas: "HAZARD: lowering maxReplicas below the live count makes the HPA cut straight to the new maximum in one step (the current>max branch bypasses the scale-down pacing)". That note covers the deliberate-lowering entry point, not this one, where the live count is above the default maximum before anything is lowered. Qualify the phase-1 and §5 dry-run claims for N > maxReplicaCount the way they were just qualified for N < effectiveMin, and add to the enable recipe that autoscaling.maxReplicas has to be raised to at least the live count before the flip.
[MAJOR] design-proposals/database-horizontal-autoscaling/decisions/0001-client-side-apply-preserves-the-autoscaler-seed.md:41, the Revisit-if trigger watches a helm-controller setting that does not exist
The record designates one trigger for its central invariant: "helm-controller starts enabling threeWayMergeForUnstructured by default, which would make the upgrade-path patch read live state and re-assert the constant". helm-controller has no such setting to enable or to default, which this record establishes correctly two paragraphs earlier at :27 ("helm-controller v1.5.0 never sets it (no reference anywhere in the controller)"). The two statements cannot both be load-bearing.
The lever lives in the embedded Helm SDK, and it is an asymmetry between two actions rather than a flag anyone flips:
$ gh api "repos/helm/helm/contents/pkg/action/upgrade.go?ref=v4.1.1" --header 'Accept: application/vnd.github.raw' \
| grep -c ThreeWayMergeForUnstructured
0
$ # ... and the three options upgrade.go does pass, at :469-471:
kube.ClientUpdateOptionForceReplace(u.ForceReplace)
kube.ClientUpdateOptionServerSideApply(serverSideApply, u.ForceConflicts)
kube.ClientUpdateOptionUpgradeClientSideFieldManager(upgradeClientSideFieldManager)
$ # install.go computes it; rollback.go pins it off explicitly:
install.go:503 updateThreeWayMergeForUnstructured := i.TakeOwnership && !i.ServerSideApply
rollback.go:231 kube.ClientUpdateOptionThreeWayMergeForUnstructured(false)
$ # and on the controller side, for completeness:
$ gh api "search/code?q=threeWayMerge+repo:fluxcd/helm-controller" --jq .total_count
0
upgrade.go omits the option, so it falls to the Go zero value, while install.go:503 computes it and helm-controller sets TakeOwnership true by default on both actions (internal/action/install.go:91, internal/action/upgrade.go:113). A Helm release that makes upgrade consistent with install flips this invariant for every client-side release, arrives through an ordinary Flux image bump, and changes nothing a maintainer watching helm-controller settings would see. Since no repeatable test holds the behaviour, the trigger is the only protection there is, which is why aiming it at the wrong component matters more here than the wording suggests.
Restate it against helm.sh/helm/v4 pkg/action/upgrade.go gaining ClientUpdateOptionThreeWayMergeForUnstructured, and pin the SDK version the record was verified against so the next bump has something to diff. What would change my mind: evidence that helm-controller acquired such a knob after v1.5.0, which would make the sentence forward-looking rather than misdirected.
[MINOR] design-proposals/database-horizontal-autoscaling/README.md:123, §5 still documents a maxReplicaCount auto-raise that the implementation replaced with a hard render failure
"When the floor would exceed maxReplicas, the helper raises maxReplicaCount to the floor too (quorum wins, never clamp below a safe quorum) and the alert rules flag that the configured maximum was overridden." The implementation fails the render instead, deliberately, and says so in its own guard:
postgres: the effective minimum instance count (2 = max(minReplicas 2, maxSyncReplicas+1 1, 2))
exceeds autoscaling.maxReplicas (1); raise maxReplicas to at least the quorum floor.
The line sits outside this diff, but reconciling the proposal to the implementation is what this PR is for and §5 is a section it revised. Two schema-valid inputs now behave the opposite of what §5 promises.
[MINOR] design-proposals/database-horizontal-autoscaling/README.md:126, dryRun on its own renders nothing at all, with no signal
This revision adds "dryRun is an ordinary values-schema flag a tenant may set directly, not only a phase-1 step, so this caveat applies wherever it is used", which invites use outside the enable flow. templates/scaledobject.yaml:1 gates the whole file on .Values.autoscaling.enabled, so dryRun (and transition) without enabled: true renders output identical to the default: no ScaledObject, no HPA, no dashboard recommendation, and no render-time message saying why. Say in §5 that both staging flags require enabled: true, and ideally add a render-time fail for the combination on the implementation side.
[MINOR] design-proposals/database-horizontal-autoscaling/decisions/0001-client-side-apply-preserves-the-autoscaler-seed.md:1, the record is about twice the length the convention sets, and grew that way across the review rounds
design-proposals/README.md gives decision records a length of "one page" and warns that "A record that grows into a second design document will not get read". This one is 1801 words against 997 for the only existing example, and the growth is monotone across this PR's own rounds: 999 at 1d77dc8, 1061 at 0edc72f, 1191 at fb0f55f, 1603 at ab4ce85, 1801 at 2614afd. It landed at the precedent's length, and each round answered a review point by appending another qualifier paragraph to a document the convention says is frozen once merged. Consequences now carries three residual write paths at paragraph length each. Most of that detail already exists, better placed, in packages/apps/postgres/values.yaml and the _autoscaling.tpl header comment, which the convention explicitly prefers: "Where a decision's content is a fact about a type, a field or an invariant that code must respect, its primary home is a comment at that site plus a test". Link out to those and keep the record to the decision and the reason the alternatives lost. I am partly responsible for this growth, so treat it as a request to consolidate rather than to add.
Still open from my earlier rounds
These are not new. Each was raised in an earlier round on this PR and is still present at 2614afdd; a separate pass that saw only the checklist and the code, not the diff or the earlier verdicts, reached the same conclusion on each.
[MINOR] README.md:179, "minReplicaCount pinned to N" contradicts the same bullet and the chart. Raised as part of a MAJOR last round; the mechanism half is now fixed, so what remains is the stale parenthetical and I am dropping it to MINOR. It still reads "(autoscaling.keda.sh/paused-scale-in/paused-scale-out, minReplicaCount pinned to N)", and phase 1b leans on it ("because the count is pinned to N throughout"), while four sentences later the same bullet says the opposite, correctly: "the quorum floor forces minReplicaCount to effectiveMin >= 2". scaledobject.yaml:42 passes minReplicaCount as effectiveMin with no transition or dry-run branch, and tests/autoscaling_test.yaml:256-290 asserts spec.instances and both pause annotations for phase 1 but never minReplicaCount, so nothing pins the parenthetical's reading either. An operator who skims the parenthetical and stops there reads the false half. Drop it, or write effectiveMin.
[MINOR] README.md:141, autoscaling.transition is still absent from the document's own values contract. It gates the render condition printed at :96, it is the flag the phase-1 runbook at :179 tells the operator to set, and :121 claims the block is "validated by values.schema.json, like every other cozystack knob". The block at :136-148 lists enabled, minReplicas, maxReplicas, target, maxReplicationLagSeconds and dryRun. The real schema carries transition as a required property, so the document's contract is narrower than the thing it says it is showing.
[MINOR] README.md:182, the disable bullet still carries its omit-era rationale. "re-introduces instances: {{ .Values.replicas }}" and "so Flux reasserts the current value rather than dropping to the default" both describe a design §3 no longer has: the field is present in both arms, so nothing is re-introduced, and with replicas staged to M the phase-2 render is identical and the two-way patch is empty, so Flux asserts nothing. Two bullets earlier, at :180, the document states that correctly: "re-asserts nothing". The steps are right; the reasoning contradicts the mechanism the rest of the section relies on.
[MINOR] decisions/0001-...md:5, Status: Proposed is still outside the enumeration. decision-template.md:8 gives Accepted | Superseded by NNNN | Reverted, the status table in design-proposals/README.md lists the same three, the string "Proposed" appears in neither file, and the one in-tree precedent lands as Accepted. The added parenthetical also relies on a manual post-merge edit of the one block the same README says must track reality. Land it as Accepted.
[MINOR] decisions/0001-...md:15, the Context still conflates the ForceConflicts assignment with the apply-strategy default. "defaults new HelmReleases to server-side apply and hardcodes ForceConflicts = ServerSideApply (internal/action/install.go:53, same in upgrade.go/rollback.go)" is right about ForceConflicts, which really is identical in all three. The defaults are not: upgrade.go:103 and rollback.go:102 both set the literal auto, which resolves by reading the previous release's ApplyMethod (upgrade.go:74), and the source annotates that as deliberate ("regardless of UseHelm3Defaults"), while the install default is the one behind that gate. Neither document mentions auto, the inheritance, or the gate anywhere. It matters for the record's own argument: a Postgres release predating v1.5.0 was already applying client-side on every upgrade, so the "reverted on every apply" failure never touched that population.
[MINOR] README.md:126 and :179, still no KEDA version floor for the per-direction pause annotations. This revision made them more load-bearing, not less: both sections now reason in detail about their behaviour. They were introduced in KEDA v2.18.0 (CHANGELOG.md, "Add support for pause scale in annotation (#6902)" and "... scale out annotation (#7022)", both under the v2.18.0 heading; declared at apis/keda/v1alpha1/scaledobject_types.go:60-61 in v2.20.2). Below that version KEDA ignores unknown annotations, so a ScaledObject this document calls non-actuating would actuate immediately with nothing reporting the difference. The CNPG floor is stated precisely in six places; KEDA's version is still parked in §Open questions as a packaging question with no owner. Give it the same treatment.
Claim mismatches
[PARTIAL] "Under client-side apply the constant seed is a merge no-op, so KEDA's live value survives, deterministic and safe across enable/disable/dry-run with no handoff race" (PR body). The merge no-op is verified: helm.sh/helm/v4@v4.1.1 pkg/kube/client.go:1012-1064 routes unstructured objects to jsonpatch.CreateMergePatch(original, modified) unless threeWayMergeForUnstructured, pkg/action/upgrade.go:466-471 never passes it, original is built from the stored release manifest, and an identical render short-circuits with no API call. "Safe across enable/disable/dry-run" does not survive: the documents' own carve-outs shed the live count when .Values.replicas is unstaged, and per the second finding the paused window sheds it even when replicas is staged, whenever the live count exceeds maxReplicaCount.
[PARTIAL] "any field the chart renders is force-owned by Flux and reverted from the HPA's live value on every apply" (PR body, and 0001:15). The force-own half is verified (install.go:53, upgrade.go:76, rollback.go:92, all ForceConflicts = ServerSideApply). "On every apply" is right, but the record's phrasing at :21, "the upgrade action every steady-state reconcile takes", is not: DetermineReleaseState returns ReleaseStatusInSync for an unchanged release and no Helm action runs on that tick. The invariant is safer than stated, so this is imprecision rather than risk, but it is the sentence a future reader will quote.
Caveats
- Hermetic review, no cluster contacted. The configuration-corner work is against the §3 snippet plus renders of the chart in
cozystack/cozystack#3954(branchdb-autoscaler-keda, still OPEN and unmerged), not against a live apply. - Checked and sound, so these should not need re-litigating: helm-controller v1.5.0 pinning
helm.sh/helm/v4 v4.1.1; the install-with-adoption three-way rebase behaving exactly as the record now describes, includingrequireAdoptionputting the rendered object inoriginalsso only the three-way arm reads live;TakeOwnershiptrue by default on both actions;Install.ServerSideApply=falseplusUpgrade.ServerSideApply=disabledthreaded throughpkg/registry/apps/application/rest.goand pinned byTestConvertApplicationToHelmRelease_ServerSideApplyOverride; the annotation name itself; CNPG's+kubebuilder:default:=1onInstancesand the webhook rejectingmaxSyncReplicas >= instances;ignoreNullValues: "false"genuinely pinned incozy-lib/templates/_keda.tpland present in the render; cozystack#3951 merged andmainvendoring CNPG 1.30.0; cozy-lib stamping the two per-direction annotations rather than KEDA's fullpaused, which would delete the HPA; every anchor and relative link in the touched README resolving. - Two write paths the record does not name are unreachable rather than missing, so they are not findings:
rollback, because cozystack emitsUpgrade.Strategy.Name=RetryOnFailureand never setsUpgrade.Remediation, so the retry path returns before a rollback remediation is selected; and drift correction, becauseDriftDetection.GetMode()defaults todisabledand the HelmRelease builder never sets it. Worth noting the annotation coversInstallandUpgradeonly, so "forced client-side" is exact for the two reachable actions and inherited from release history for the third. - The annotation is applied only on the aggregated-apps-API path. A HelmRelease for the postgres chart authored directly rather than through the apps API keeps the platform default and would revert the seed, so the record's per-kind scope statement still reads wider than the code path. Carried from an earlier round as a note, not a finding, because no such HelmRelease exists today.
Implemented in: cozystack/cozystack#3954points at an open, unmerged PR. The template permits a PR link, but the header needs revisiting if that PR changes shape.- Per Phase 0 the reviewing pass did not read this PR's conversation or any earlier review; the reconciliation above was done separately, after the verdict was formed, so convergence between the two is not anchoring.
Recommended follow-ups
- §5's lag brake specifies the write gate as
rate(cnpg_pg_stat_replication_sent_diff_bytes[5m]) > 0while the implementation usesrate(cnpg_collector_wal_records[5m])and its comment asserts the proposal's metric "does not" exist. That assertion is wrong:sent_diff_bytesis defined incloudnative-pg@v1.30.0 config/manager/default-monitoring.yaml. It is declaredusage: "GAUGE"though, a bytes-behind figure rather than a monotonic counter, sorate()over it is not a write-rate signal whatever its availability. §Open questions still lists the lag-clamp shape as unsettled, so this belongs wherever that lands, not in this PR. - The record contemplates re-enabling SSA for non-autoscaled Postgres. Moving a live
Clusterfrom server-side to client-side apply leaves the earlierhelmApply-type entry inmanagedFieldsuntouched, because helm only migrates in the other direction (upgradeClientSideFieldManagerrequiresserverSideApply). Re-enabling SSA later would reactivate that stale field set against an object whose.spec.instanceshas since been written by something else. Not exercisable without a cluster, so it is a note for whoever builds the per-instance apply strategy. - Once the two floor findings are settled, a live run is the only way to close the rest: replay a chart bump against an autoscaling-active grown cluster and confirm
.spec.instancesis untouched, and run the enable flow on a cluster whose live count exceedsautoscaling.maxReplicas.
… a version floor IvanHunters round 2 [MAJOR]: `CNPG >= 1.28.4` is not monotone. The `status.selector` the HPA requires shipped per release line (1.28.4, 1.29.2, 1.30.0) and is absent from 1.29.0 and 1.29.1, which were cut before the 1.29-line backport. A `>= 1.28.4` bound admits two releases where the mechanism provably does not work. Restate the gate behaviourally in all six places (frontmatter PoC line, Overview note, PoC-finding intro, Resolution, Scope Precondition, Testing): the precondition is that the `Cluster` /scale subresource serves a non-empty `status.selector`, and the version is a proxy that leaks. The full carrying matrix (1.28.4+, 1.29.2+, 1.30.0+; not 1.29.0/1.29.1) lives once in Scope Precondition; the other spots defer to it. cozystack vendors 1.30.0 (#3951), which serves it. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>
Two [MAJOR] findings from IvanHunters round 2: - Above-max shed (mirror of the covered N<effectiveMin case). The HPA's currentReplicas>maxReplicaCount branch writes /scale=maxReplicaCount unconditionally, bypassing the pause annotations and the scale-down pacing, so an 8-instance cluster under the default maxReplicas 6 is cut to 6 on cold start, shedding standbys during the "observation" window. Staging replicas cannot help (trigger is maxReplicaCount vs live count). Add the mirror to phase-1, the §5 dry-run bullet and the §5 scale-down pacing bullet; add "raise autoscaling.maxReplicas to at least the live count before the flip" to the recipe; note the chart has no replicas>maxReplicas render guard today (follow-up for #3954). - Revisit-if trigger. It named "helm-controller starts enabling threeWayMergeForUnstructured", a setting helm-controller does not have (the record says so two paragraphs up). Retarget it to the real lever: helm.sh/helm/v4 pkg/action/upgrade.go gaining/passing ClientUpdateOptionThreeWayMergeForUnstructured to match install.go:503, and pin the verified SDK version (v4.1.1) for the next bump to diff. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>
- §5 quorum floor: the helper fails the render (not an auto-raise of
maxReplicaCount) when the floor would exceed maxReplicas, matching the
implementation's guard.
- Tenant values block: add the schema-required `transition` sub-flag.
- §5 dry-run: both `dryRun` and `transition` require `enabled: true`,
else scaledobject.yaml renders nothing with no signal.
- KEDA version floor: the per-direction pause annotations need KEDA
>= 2.18.0 (below it they are ignored and the "paused" ScaledObject
actuates); stated at §5 and settled in the Open-questions KEDA item.
- Disable bullet: replace the omit-era rationale ("re-introduces
instances"/"Flux reasserts") with the constant-seed/empty-two-way-patch
reality.
- ADR: Status Proposed -> Accepted (template enumeration); correct the
ForceConflicts-vs-apply-default conflation (upgrade/rollback default to
auto, install behind UseHelm3Defaults); consolidate the verbose prose
and point field-level detail at values.yaml / _autoscaling.tpl, without
dropping any citation, version pin, or finding response.
Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>
Independent cozy-review pass on 977f003: - [MINOR] Phase-1 parenthetical said the paused ScaledObject pins `minReplicaCount` to N, but the chart renders it as `effectiveMin` unconditionally (no transition/dryRun override), and the same bullet says so four sentences later. Correct the parenthetical to `minReplicaCount = effectiveMin`. - [NIT] decision 0001 Decision: "the upgrade action every steady-state reconcile takes" overstates — an unchanged release short-circuits to InSync and runs no Helm action. Qualify to "any applying reconcile". - [NIT] §5 replication-lag brake: note it is opt-in (maxReplicationLagSeconds defaults to 0) and the clamp query shape is a still-open item. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>
2 participants
Resolves the one open migration detail in §Upgrade / Open questions of the database-horizontal-autoscaling proposal.
Finding (PoC, 2026-09-02, live on dev9). The platform runs helm-controller v1.5.0, which defaults to server-side apply. Under SSA the chart-only omit handoff the proposal preferred is not safe: (1) any field the chart renders is force-owned by Flux and reverted from the HPA's live /scale value on every apply; (2) the omit handoff needs the HPA to own
.spec.instancesbefore the phase-2 apply, but the HPA claims the /scale field-manager only when it actually writes (desired != current) — a cluster idle at its floor never writes, never owns the field, and the phase-2 omit prunes it → CNPG defaults to 1 (quorum-fatal). That prune is unavoidable chart-only and would require a runtime actor this design deleted.Resolution. The implementation does not omit under active autoscaling; it renders a constant seed
instances: max(replicas, effectiveMin)and forces client-side (three-way-merge) apply for the tenant HelmRelease via a per-app annotationrelease.cozystack.io/helm-server-side-apply: "false". Under client-side apply the constant seed is a merge no-op, so KEDA's live value survives — deterministic and safe across enable/disable/dry-run with no handoff race, and uniform for the multi-engine roadmap. This supersedes §3's "omit the field" on the apply-strategy detail only; the ownership goal (Flux does not contend with the HPA) is unchanged.Implemented in cozystack/cozystack#3954. Reviewers: Timofei Larkin (@lllamnyp) IvanHunters.