🔴 Hot News: sgl project/rbg/pull/

diw-zw · 2026-08-29T14:38:21Z

Ⅰ. Motivation

Every existing e2e suite installs the version under test from scratch, so none of them
can catch an upgrade-only regression: the class of bug where helm upgrade disturbs
workloads that were already running. Recent fixes (#433, #439) were exactly that class
and were found by hand, not by CI.

Four plausible causes exist in this codebase:

the inputs to the revision hash change (e.g. the RoleInstanceSet template now carries
restartPolicyConfig where v0.7.0 carried the restartPolicy string), making the new
controller read every stored spec as modified;
a new CRD default, which the apiserver applies on read and so silently changes the spec
the new controller sees;
the v1alpha1 conversion webhook writing a different v1alpha2 shape than v0.7.0's did;
a controller-side default changing, which rewrites derived resources without touching
a pod at all — KEP 260's sharedServiceSelection is the live example: it narrows a
shared Service selector to the leader, dropping worker endpoints from a Service that
keeps its name, UID and cluster IP.

The first three surface as pod churn; the fourth does not, which is why the assertions
reach past pod identity to pod labels, the Services in front of them and the endpoints
behind those.

Ⅱ. Modifications

New suite: test/e2e/upgrade (Ordered, self-contained)

preflight.go — package doc stating what the suite proves and what it does not,
plus resolution of the from/to versions. The upgrade target defaults to whatever the
chart in the working tree ships, i.e. the images a user actually gets; RBGS_TO_*
overrides that for a local build. One value is deliberately not a chart default:
portAllocator is enabled, on the v0.7.0 install and on the upgrade alike, which is how
the other e2e workflows install rbgs. So this is a feature-enabled upgrade rather
than a stock one — what it is not is a feature being toggled by the hop, which would
put a configuration change inside the interval every assertion here attributes to the
upgrade.
install.go — the suite installs the from-release itself, taking that chart from a
git worktree of the tag (rbgs publishes no Helm repo, and the in-tree chart's values
layout changed after v0.7.0), verifies the running image really is that release, waits
until its webhooks actually serve (a dry-run probe, since --wait only means Available),
then removes everything on teardown. A requireCleanCluster precondition fails fast with
cleanup instructions: teardown deletes the workloads.x-k8s.io CRDs, so proving nothing
of those kinds pre-existed is what makes that safe, and running against a cluster already
on the new version would make every assertion pass vacuously.
fixtures.go — 13 fixtures covering standalone / Deployment / StatefulSet / LWS /
leader-worker RoleInstanceSet / inter-role dependencies / ScalingAdapter / custom
components / RoleBasedGroupSet / v1alpha1 RBG / v1alpha1 RBGSet, plus one fixture stuck
Pending and one held half-rolled by a partition, so the upgrade does not land on
a fully converged cluster.
snapshot.go — the detectors: pod UID/creation time/node/per-container restart
counts/owner UIDs/phase/full labels and annotations/readiness gates, owner
generations, ControllerRevision names, Services and their EndpointSlices,
readiness, and Killing events. Known-and-attributed changes are folded through an explicit
recordedRewrites allowlist (the LWS rolloutStrategy re-patch on every controller start;
the KEP 260 leader-only narrowing of one fixture's shared Service) — each entry names the
change it stands for, and the narrowing is checked exactly rather than waved through:
being on the allowlist is a permission, and the narrowing has to actually show up in that
Service's selector diff before the endpoints it removes are folded.
upgrade.go — drives helm upgrade from inside the suite so the pre-upgrade
snapshot stays in memory, and gates on the CRD-upgrade hook Job, the controller image,
and the webhook caBundles being patched before asserting.
specs.go — 15 specs in 4 phases: create on v0.7.0 and snapshot → upgrade → assert
nothing moved (re-run after a controller restart from cold and after a second identical
helm upgrade) → assert the upgraded controller still works (pod recreation, scaling a
v0.7.0-created role, rolling only the role whose template changed, v1alpha1 round-trips,
the new Warmup API). Each post-upgrade comparison waits a 30s settle window so that
"the controller had not looked yet" cannot be read as "the controller left it alone".

CI / build

.github/workflows/release-test.yml: new e2e-test-upgrade job, mirrored into the PR
merge box alongside the existing two. UPGRADE_FROM_GIT_TAG / UPGRADE_FROM_IMAGE_TAG
pin the from-release in one place. Because the target is the chart's own default images,
the job pre-pulls every image the rendered chart references and turns "this release was
never published" into a named failure instead of an ImagePullBackOff discovered when
helm times out. Failure collects helm history, controller logs (incl. --previous), hook
Job logs, RBG state, ControllerRevisions, webhook caBundles, events and kind logs.
Makefile: make test-e2e-upgrade, 60m timeout (one run installs v0.7.0, starts the
fixtures, waits out a full helm upgrade, then settles — a go-test timeout firing first
would report a panic instead of the gate's own message).

No go.mod or go.sum change.

This lands in the release gate rather than the per-PR e2e workflow on purpose: the
upgrade target is the chart's default images, which point at a real published release only
once a release PR bumps them — and testing the upgrade with exactly the values being
shipped is the point.

Ⅲ. Does this pull request fix one issue?

NONE

Ⅳ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.

This PR is entirely test infrastructure. 15 specs in test/e2e/upgrade:

phase 1 — rejects fields the v0.7.0 CRDs do not know; creates RBGs on v0.7.0 and waits for
every role to be ready; records the running pods.
phase 2 — starts a rollout that halts at a partition, then upgrades the release to the
version under test.
phase 3 — leaves every already-running pod untouched; leaves the stored spec of v0.7.0
objects as written; leaves the half-rolled workload alone too and still finishes its
rollout once the partition is lifted; changes nothing when the upgraded controller starts
from cold; changes nothing when the same helm upgrade is run again.
phase 4 — still recreates a deleted pod; still scales a role v0.7.0 created; rolls only the
role whose template actually changed; does not disturb a v1alpha1 RBG updated through
v1alpha1; same for a v1alpha1 RoleBasedGroupSet; accepts the RoleBasedGroupWarmup API the
upgrade introduced.

Ⅴ. Describe how to verify it

CI: comment /release-test on the PR — e2e-test-upgrade now reports into the merge box.

Locally, against a kind cluster with no rbgs install (LWS CRDs, scheduler-plugins and
Volcano installed as the other e2e suites require):

# upgrade to the images the chart ships (what CI does; needs them published)
make test-e2e-upgrade

# upgrade to a local build instead. Both images, because RBGS_TO_TAG also picks the
# CRD-upgrade hook's tag -- building only the controller leaves the pre-upgrade hook
# unable to pull.
make docker-build-controller TAG=e2e-upgrade
make docker-build-crd-upgrader TAG=e2e-upgrade
kind load docker-image rolebasedgroup/rbgs-controller:e2e-upgrade
kind load docker-image rolebasedgroup/rbgs-upgrade-crd:e2e-upgrade
RBGS_TO_TAG=e2e-upgrade make test-e2e-upgrade

The suite installs v0.7.0, upgrades it in place and removes it again. If the cluster is not
clean it refuses to run and prints the cleanup commands.

Overrides: RBGS_NAMESPACE, RBGS_RELEASE, RBGS_HELM_TIMEOUT, RBGS_FROM_GIT_TAG,
RBGS_FROM_TAG, RBGS_FROM_REPO, RBGS_FROM_CRD_UPGRADE_REPO, RBGS_TO_REPO,
RBGS_TO_TAG, RBGS_CRD_UPGRADE_REPO, RBGS_CRD_UPGRADE_TAG.

VI. Special notes for reviews

The assertions are meant to be strict. They are detectors: when one fires, the finding
belongs in the controller, and the check should not be loosened to accommodate the
behavior it just caught. recordedRewrites is the only escape hatch, and an entry is
only admissible with the change it stands for named — a difference nobody can attribute
is a finding to report, not an entry to add.
The 30s settle window in phase 3 is the single load-bearing detail; without it a
pod-rolling regression passes whenever the sample lands before the new controller's first
reconcile.
The half-rolled fixture is held there by a partition, not by a stretched readiness
probe racing helm: how long a helm upgrade takes is not something a test controls, so a
timing-based overlap could only be reported afterwards, never relied on. The halt is also
what lets that fixture be held to the same "nothing churned" standard as the quiet ones.
What this does not reproduce is a rollout that is actively progressing across the hop.
The suite installs and uninstalls a release, and its teardown deletes CRDs. Please review
requireCleanCluster and teardownFromRelease with that in mind.
Known scope limits are listed in the package doc: single hop v0.7.0 -> current only,
nothing about storage-version migration, single-node/single-replica controller (no leader
election handover), only the field combinations in fixtures.go (no gang scheduling, GPU
or PVC-backed roles), observation bounded by the settle window, and nothing about
downgrade.

Checklist

Format your code make fmt.
Add unit tests or integration tests.
Update the documentation related to the change.

The fixture that is meant to be rolling when the upgrade lands relied on a readiness probe to stretch its rollout past the duration of `helm upgrade`. That is a race the suite cannot win, because how long helm takes is not something a test controls, so the overlap could only be reported after the fact. It was reported as missed on a real run: the rollout took roughly 60-75s and helm 87s. Phase 2 now sets rollingUpdate.partition together with the template change, in a single update so the controller can never see the new template while the partition is still zero. The rollout advances to the partition and stops, which holds the fixture at mixed revisions for as long as the upgrade needs. The partition is introduced here rather than at creation because newVersionedInstance builds every ordinal below it from the current revision, which during initial creation is not yet settled. Two assertions replace the printf. The upgrade must leave every pod of the halted workload alone -- the same standard the quiet fixtures are held to, and previously unreachable because this one was genuinely moving -- and lifting the partition must still let the upgraded controller finish the rollout the old one started.

Copilot

Pull request overview

Adds a release-gated v0.7.0-to-current upgrade suite to detect workload churn and compatibility regressions.

Changes:

Adds upgrade fixtures, snapshots, readiness gates, and post-upgrade checks.
Automates old-release installation, Helm upgrade, and cleanup.
Integrates the suite into release CI and Make targets.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
`test/e2e/upgrade/upgrade.go`	Executes and validates upgrades.
`test/e2e/upgrade/suite_test.go`	Defines suite lifecycle.
`test/e2e/upgrade/specs.go`	Implements phased upgrade tests.
`test/e2e/upgrade/snapshot.go`	Detects workload and service changes.
`test/e2e/upgrade/preflight.go`	Resolves versions and validates prerequisites.
`test/e2e/upgrade/install.go`	Installs and removes the source release.
`test/e2e/upgrade/fixtures.go`	Defines compatibility fixtures.
`Makefile`	Adds the upgrade test target.
`go.sum`	Adds module graph checksums.
`.github/workflows/release-test.yml`	Adds the release-gate upgrade job.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

diw-zw · 2026-08-29T16:21:51Z

+			narrowed := leaderOnly[name]
+			for _, diff := range stringMapDiff(beforeSvc.Selector, afterSvc.Selector) {
+				if narrowed && diff == leaderComponentSelectorAdded {
+					continue
+				}
+				problems = append(problems, where+" selector "+diff)
+			}
+
+			added := missingFrom(afterSvc.Endpoints, beforeSvc.Endpoints)
+			removed := missingFrom(beforeSvc.Endpoints, afterSvc.Endpoints)
+			if narrowed {
+				removed = leaderEndpoints(removed, beforeSnap)
+			}


Agreed, and fixed in 3f854c1.

narrowed came from the allowlist alone, so on this one Service any lost worker endpoint was dropped from the report even when the selector had not moved. That is the one place where it matters: a pod that disappears is caught by checkNoPodChurn and a relabelled pod by checkPodMetadataStable, which leaves an endpoint that vanished on its own with no other detector behind it.

The field's own doc on recordedRewrites.leaderOnlyServices already promised the tighter contract — the code just did not implement it. It now requires the narrowing to be visible in that Service's selector diff before folding the endpoints it removes. Selector-level reporting is unchanged, since that part already matched one exact diff line and reported everything else.

Re-ran the suite after the change: 15/15, no Service or endpoint findings. For what it is worth, the fold does still have something to tolerate — an earlier run that failed for an unrelated reason dumped both snapshots and shows the narrowing happening for real, same Service UID and clusterIP:

before: endpoints=[up-lwp-lwp-0-0=... ready=true up-lwp-lwp-0-1=... ready=true] after: endpoints=[up-lwp-lwp-0-0=... ready=true]

diw-zw · 2026-08-29T16:22:05Z

+		"--set", "crdUpgrade.image.repository=" + crdUpgradeRepo(),
+		"--set", "crdUpgrade.image.tag=" + crdUpgradeTag(),
+		"--set", "crdUpgrade.image.pullPolicy=IfNotPresent",
+		"--set", "controller.features.portAllocator.enabled=true",


The fact is right and the wording was wrong, so I took the second remedy. The first one would not work: portAllocator is enabled on the v0.7.0 side too (install.go:147), which is how the other e2e workflows install rbgs (e2e-test.yml:166, release-test.yml:189). Dropping only the upgrade-side --set would make the hop turn a feature off, putting a configuration change inside the very interval every assertion here attributes to the upgrade.

So the claim is what changed. preflight.go's package doc no longer says the suite runs a helm upgrade "with no overrides at all"; it now states that this is a feature-enabled upgrade rather than a stock one, and that what matters is the feature not being toggled by the hop. There is a note at the --set itself as well, so the non-default is not unexplained at the call site. The PR description says the same thing now.

diw-zw · 2026-08-29T16:22:17Z

+// crdUpgradeTag follows RBGS_TO_TAG before the chart default, because a local build
+// tags both images together: setting only RBGS_TO_TAG must not leave the hook Job on a
+// published image while the controller runs the local one.
+func crdUpgradeTag() string {
+	return envOr("RBGS_CRD_UPGRADE_TAG", envOr("RBGS_TO_TAG", chartDefault(func(v *chartValues) string {


The finding is real, but it is in the PR description rather than here. The fallback is deliberate and documented at preflight.go:235-237: a local build tags both images together, so setting only RBGS_TO_TAG must not leave the hook Job on a published image while the controller runs the local one.

The in-code recipe was already correct (install.go:370-373):

make docker-build-controller TAG=e2e-upgrade make docker-build-crd-upgrader TAG=e2e-upgrade RBGS_TO_TAG=e2e-upgrade make test-e2e-upgrade

The stale copy was the one in the PR description, now fixed to build and kind load both images. You are right about IMG too: it is not a variable this Makefile reads — only RBG_CONTROLLER_IMG, CRD_UPGRADER_IMG and TAG (Makefile:4-5,12) — so make docker-build IMG=... would have silently built the default tag instead. Thanks.

checkServicesStable read its allowlist as the fold itself, so on the one Service recorded as narrowing to the leader, any lost worker endpoint was dropped from the report even when the selector had not moved. That is the one place it matters: a pod that disappears is caught by the churn check and a relabelled pod by the metadata check, leaving an endpoint that vanished on its own with no other detector behind it. The field's own doc already promised the tighter contract. Require the narrowing to appear in that Service's selector diff before folding what it removes. Also correct the package doc: the suite enables portAllocator on both sides of the hop, so it is not the override-free helm upgrade the doc claimed.

Copilot

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.

Suppressed comments (1)

test/e2e/upgrade/snapshot.go:828

The snapshot records each RoleBasedGroup's generation, but this detector checks only its UID; Generation is otherwise used only in debug output. A controller rewrite of the RBG spec can therefore pass whenever it does not also alter a derived owner or pod. Assert the generation delta for every non-skipped RBG.

		if afterSnap.RBGUID != beforeSnap.RBGUID {
			problems = append(problems, fmt.Sprintf(
				"%s: RoleBasedGroup was recreated (UID %s -> %s)",
				rbgName, beforeSnap.RBGUID, afterSnap.RBGUID))
		}

diw-zw · 2026-08-30T03:55:18Z

+			gomega.Expect(
+				countSurvivors(midRollPodsAtPartition, listPodFactsForRole(gomega.Default, f, target, midRollRole)),
+			).To(
+				gomega.Equal(midRollReplicas),
+				"the upgrade churned pods of a workload whose rollout was halted at a partition",
+			)


Fixed in 125a226.

The half-rolled fixture is now held to the same pod-level standard as every quiet one: specs.go:386-388 runs checkNoPodChurn, checkNoRestarts and checkPodMetadataStable on the partitioned role, against the snapshot taken at the moment the partition halted the rollout. An added pod, an in-place restart and a label/annotation/readiness-gate rewrite are all reported now, not merely a count of surviving UIDs.

Two things are deliberately still out, both stated at specs.go:369-372: the object-level detectors, because setting the partition rewrote the RoleInstanceSet spec on purpose so its generation and revisions moved for a reason that has nothing to do with the upgrade; and the event search, because the deliberate half-rollout produces its own killing events.

diw-zw · 2026-08-30T03:55:23Z

+		WithRoles([]workloadsv1alpha1.RoleSpec{
+			wrappersv1.BuildBasicRole("legacy").WithReplicas(1).Obj(),
+			wrappersv1.BuildLwsRole(v1alpha1LwsRole).WithReplicas(1).Obj(),
+		}).Obj()


The finding is right and the fix is a documentation correction, in 125a226, because no such fixture can run.

A v1alpha1 role that reaches the len(src.Components) > 0 branch cannot be reconciled by either version. Workload has an object-level CRD default (api/workloads/v1alpha1/rolebasedgroup_types.go:341), so the apiserver fills in apps/v1 StatefulSet; the conversion then always writes RoleWorkloadTypeAnnotationKey from it (rolebasedgroup_conversion.go:142-147); the StatefulSet reconciler asks for a role-level template, and GetTemplate() returns nil for a CustomComponentsPattern role, so GetResolvedTemplate fails with "role has no template or templateRef set". v0.7.0 behaves the same way, so the fixture would simply hang phase 1 with no pods for this suite to watch.

fixtures.go now says that, and leaves the branch to the conversion unit tests, instead of implying it was merely overlooked.

diw-zw · 2026-08-30T03:55:26Z

+//   - Single-node, single-replica controller: nothing about leader election
+//     handover, multi-node scheduling, or rollout behavior under real load.


Correct, and reworded in 125a226. The limitation now says what it actually is: the chart ships controller.replicaCount 2 with --leader-elect on both versions, so the upgrade does replace a two-replica Deployment — but nothing here observes which replica holds the lease, and the per-start rewrite accounting in recordedRewrites assumes one leader start per rollout. preflight.go:91-96.

diw-zw · 2026-08-30T03:55:28Z

+				if afterFacts.NodeName != beforeFacts.NodeName {
+					problems = append(problems, fmt.Sprintf(
+						"%s/%s: pod %s moved node (%s -> %s)",
+						rbgName, role, podName, beforeFacts.NodeName, afterFacts.NodeName))
+				}


Fixed in 125a226. Both are compared now: OwnerUIDs in checkNoPodChurn (snapshot.go:610), which is the only place a re-parented pod shows up at all since it keeps its UID, node and creationTimestamp; and Phase in checkStillReady (snapshot.go:1192). Phase is compared rather than required to be Running on purpose — one fixture is deliberately Pending, and the question is whether the upgrade moved a pod out of the phase it was in.

cheyang · 2026-08-29T23:05:19Z

+			quietFirst, quietAfter := exclude(first, mutated...), exclude(after, mutated...)
+			checkSameRBGSet(fs, quietFirst, quietAfter)
+			checkNoPodChurn(fs, quietFirst, quietAfter)
+			checkNoRestarts(fs, quietFirst, quietAfter)


The settle comparison stops at three detectors — checkSameRBGSet, checkNoPodChurn, checkNoRestarts — while runDetectors two lines below runs the full set including checkPodMetadataStable. The point of this first-vs-after sample is to prove the controller has stopped moving things, but a controller that keeps rewriting pod labels (or a selector, or an owner generation) on every reconcile passes these three and the suite blames the upgrade instead of saying "still moving". Wiring checkPodMetadataStable in here — ideally the whole runDetectors set — closes it. Checked with a unit harness: the three calls miss a pure label drift, checkPodMetadataStable catches the same one.

Fixed in 125a226, and then the guard itself was reworked.

First half: the hand-picked list is gone. Both the guard and runDetectors now go through checkSnapshotDiff (snapshot.go:540), so a controller still rewriting pod labels, a Service selector or an owner generation no longer passes it, and a detector added later cannot be forgotten in one of the two places.

Second half: running the full set surfaced that the fixed window was wrong. helm upgrade --wait returns once the new controller pod is available, which is before it holds the leader-election lease and before its first reconcile of every fixture has landed, so the run reported that first reconcile — one LeaderWorkerSet patch plus the KEP-260 shared-Service narrowing — as churn caused by the upgrade. Phase 3 now calls waitQuiesced (snapshot.go:498), which samples repeatedly until two samples settleDuration apart agree, bounded at 5 minutes, and fails saying the controller is still moving. Nothing is folded and no detector is relaxed: whatever happened before it went quiet still has to be accounted for by the comparison against the pre-upgrade snapshot, where only the recorded rewrites are allowed.

On a real cluster (v0.7.0 -> v0.8.0-97fea1a2) the wait took three rounds before the samples agreed, and all 15 specs then pass.

cheyang · 2026-08-29T23:05:19Z

+	waitControllerRolledOut(f)
+	// The new process mints its certificate and patches the webhook config again on
+	// startup, and until it has, every RBG write is rejected.
+	waitValidatingWebhookCABundle(f)


restartController waits on the validating-webhook caBundle but not the CRD conversion one — compare waitForUpgradeReady above, which calls waitCRDConversionCABundle too. A cold restart rotates the CA, and if the conversion caBundle hasn't been re-patched by the time phase 4 reads a v1alpha1 object, the Get fails with a TLS error that reads like a product bug rather than a test race. The suite's own comment flags exactly this ("a caBundle present but wrong is a real failure mode"). One-liner: add waitCRDConversionCABundle(f) here. I corroborated on a live cluster that the conversion caBundle is real and populated (and shares the validating caBundle's CA), so the wait is sound.

Fixed in 125a226. restartController now waits on the same three gates waitForUpgradeReady does — waitValidatingWebhookCABundle, waitCRDConversionCABundle and waitConversionActuallyWorks (upgrade.go:281-283) — and it takes the probe object name so the round-trip is read through a real object. That last one is what separates a caBundle that is present from one that works.

cheyang · 2026-08-29T23:05:19Z

+	for _, name := range names {
+		crd := newCRDObject()
+		crd.SetName(name)
+		if err := f.Client.Delete(f.Ctx, crd); err != nil && !apierrors.IsNotFound(err) {


teardown deletes the CRDs but doesn't wait for them to reach NotFound. helm uninstall --wait doesn't cover them — the chart doesn't own the CRDs, the crd-upgrade hook applies them — so the cascade finishes after teardown returns. On a back-to-back CI run requireCleanCluster then trips on Terminating CRDs. An Eventually on listOwnedCRDs -> empty before returning would close it. (You asked reviewers to eyeball teardown; this is the gap in it.)

Fixed in 125a226. teardownFromRelease now re-lists the owned CRDs until none are left, bounded by gateTimeout and reporting what is still registered if it runs out (install.go:299-319). A CRD delete is accepted long before the apiserver has reaped every object of that kind, so without this the next run's requireCleanCluster could either report a leak that is not one or race the reaping.

cheyang · 2026-08-29T23:05:19Z

+				if !found {
+					continue // reported by checkNoPodChurn
+				}
+				for container, beforeCount := range beforeFacts.RestartCounts {


This only iterates before's containers, so a container that appears in after (an upgrade-injected sidecar, say) is never reported — invisible if the pod wasn't recreated. A reverse loop over afterFacts.RestartCounts reporting names absent from before fixes it. Reproduced: a unit harness feeding {app} before / {app, sidecar} after gets no finding from checkNoRestarts; the reverse-loop patch flips it green.

Fixed in 125a226. checkNoRestarts walks both directions now, and a container only the after snapshot reports is called out as one the upgrade added to a pod that survived (snapshot.go:663-668).

cheyang · 2026-08-29T23:05:19Z

+	// signal that catches it.
+	RestartCounts map[string]int32
+	OwnerUIDs     []types.UID
+	Phase         corev1.PodPhase


Phase (and OwnerUIDs) get captured and dumped but no detector compares them between snapshots. A pod that flips Running -> Failed under the same UID is invisible to checkNoPodChurn. The field is already here, so asserting it is a few lines.

Fixed in 125a226. Both are compared now: OwnerUIDs in checkNoPodChurn (snapshot.go:610), which is the only place a re-parented pod shows up at all since it keeps its UID, node and creationTimestamp; and Phase in checkStillReady (snapshot.go:1192). Phase is compared rather than required to be Running on purpose — one fixture is deliberately Pending, and the question is whether the upgrade moved a pod out of the phase it was in.

cheyang · 2026-08-29T23:05:19Z

+	}
+}
+
+func hasDefaultRestartDelays(config map[string]any) bool {


hasDefaultRestartDelays treats an absent field as matching the default, so it returns true for an empty config too — and the restartPolicyConfig fold then strips restartPolicyConfig from the stored spec, hiding a conversion bug that wrote an empty one. Requiring both fields present and matching (rather than "absent or matching") would be safer.

Fixed in 125a226. hasDefaultRestartDelays now requires both keys to be present and to hold exactly the values the v1alpha2 getters default the deprecated string to; an absent key returns false (snapshot.go:829-838). The fold is only sound because the pair says the same thing, so an empty or partial config is a difference worth reporting rather than one to strip.

cheyang

Requesting changes — four of these gaps are silent false-passes (a real upgrade regression can go green), and I reproduced three of them with a unit harness that calls the real package-internal detectors on synthetic snapshots, no cluster. The harness, the observed-vs-expected table, and the bites-checks are on my fork: https://github.com/cheyang/rbg/tree/verify/pr444-e2e-upgrade-suite — see docs/verification/pr444-e2e-upgrade-suite/README.md. Production code under review is untouched; the only added file is the harness.

Blocking (majors):

A (specs.go:322) — the mid-rollout fixture is excluded from every detector; only countSurvivors (UID) runs on it. Reproduced (H5): a surviving pod with a bumped restartCount and a rewritten label passes green.
B (snapshot.go:167) — RBGSnapshot.Generation is captured but never compared; a stored-spec rewrite (the #433 mechanism) is invisible. Reproduced (H6).
C (snapshot.go:137) — ownerSources omits ScalingAdapter; that owner's churn is never captured. Reproduced (H7).
D (snapshot.go:151) — captureAll lists RoleBasedGroupList only; the RBGSet root is never snapshotted. Inspection-only (capture-path gap; a runnable test needs a fake controller-runtime client the e2e package doesn't wire up).
F1 (specs.go:317) — the settle guard runs 3 of ~9 detectors; ongoing label/selector churn passes it. Reproduced (H1).
F2 (upgrade.go:275) — restartController skips waitCRDConversionCABundle; a stale conversion caBundle can surface as a phase-4 TLS error that reads like a product bug.

F4 (checkNoRestarts ignores after-only containers) is reproduced too (H2 contract — it fails on this code). The rest (F3, F5–F8, F7, F9/F10, E) are narrower coverage gaps and a stale-comment nit, called out inline.

This is a strong suite overall — the precondition/teardown argument holds, recordedRewrites is tight, and the two-sample settle window is the right shape. The blocking items are all one-call / one-loop fixes; happy to dig into any of them.

cheyang · 2026-08-30T00:16:05Z

+			checkNoPodChurn(fs, quietFirst, quietAfter)
+			checkNoRestarts(fs, quietFirst, quietAfter)
+
+			runDetectors(fs, f, before, after, preUpgradeMark, upgradeRewrites.acrossStarts(controllerStarts), mutated)


[A — blocking] The mid-rollout fixture lives in mutated, so this runDetectors(..., mutated) excludes it — and the settle comparison above excludes it too. The only check that touches those pods is countSurvivors (UID equality). So a pod that survives by UID while the upgrade bumps its restartCount, rewrites a label, or flips to Failed passes green.

Reproduced with a unit canary (H5) that calls the real detectors on synthetic snapshots: the drift is present, no finding. The same detectors without the skip do catch it, so the gap is the blanket exclusion, not a blind detector.

Fix: run the full detector set on the mid-rollout pods (or a dedicated drift detector) instead of blanket-excluding them.

Fixed in 125a226.

The half-rolled fixture is now held to the same pod-level standard as every quiet one: specs.go:386-388 runs checkNoPodChurn, checkNoRestarts and checkPodMetadataStable on the partitioned role, against the snapshot taken at the moment the partition halted the rollout. An added pod, an in-place restart and a label/annotation/readiness-gate rewrite are all reported now, not merely a count of surviving UIDs.

Two things are deliberately still out, both stated at specs.go:369-372: the object-level detectors, because setting the partition rewrote the RoleInstanceSet spec on purpose so its generation and revisions moved for a reason that has nothing to do with the upgrade; and the event search, because the deliberate half-rollout produces its own killing events.

cheyang · 2026-08-30T00:16:05Z

+	snap := RBGSnapshot{
+		Name:          rbg.Name,
+		RBGUID:        rbg.UID,
+		Generation:    rbg.Generation,


[B — blocking] Generation: rbg.Generation is captured here and only ever written back out in the dump (line ~1197) — no detector reads it. The owner-generation check in checkOwnersStable (~842) compares ownerFacts.Generation, a different field. So an RBG whose stored spec is rewritten across the upgrade (generation 1→2, the #433 spurious-rollout mechanism) is invisible.

Reproduced (H6 canary): snapshots differing only in RBGSnapshot.Generation are reported by nothing. Bites-checked — adding a Generation comparison flips it red.

Fix: assert RBGSnapshot.Generation is stable before→after.

Fixed in 125a226. checkOwnersStable asserts the root object's own generation delta against the recorded budget (snapshot.go:922), keyed by the snapshot's kind, so an RBG spec rewritten by the upgrade is reported directly rather than only through whatever it later does to a workload — which matters because what the controller hashes is the RBG spec, so a rewrite there reaches every role at once.

cheyang · 2026-08-30T00:16:06Z

+		{"RoleInstance", func() client.ObjectList { return &workloadsv1alpha2.RoleInstanceList{} }},
+		{"Deployment", func() client.ObjectList { return &appsv1.DeploymentList{} }},
+		{"StatefulSet", func() client.ObjectList { return &appsv1.StatefulSetList{} }},
+		{"LeaderWorkerSet", func() client.ObjectList { return &lwsv1.LeaderWorkerSetList{} }},


[C — blocking] ownerSources covers RIS/RI/Deployment/StatefulSet/LeaderWorkerSet but not ScalingAdapter. An RBG backed by a ScalingAdapter has its owning workload never captured, so checkOwnersStable can't see its owner UID/generation churn.

Reproduced (H7 canary); bites-checked — appending the kind flips it red.

Fix: add the ScalingAdapter list kind here.

Fixed in 125a226. RoleBasedGroupScalingAdapter is in ownerSources now (snapshot.go:161-163). It is not a workload, but it is created, relabelled and spec-patched by the same controller and it is the object an autoscaler holds on to, so its UID and generation are exactly what checkOwnersStable should be watching.

cheyang · 2026-08-30T00:16:06Z

+// g carries the assertion target. Callers inside an Eventually body must pass the
+// injected gomega.Gomega so a transient List error is retried rather than failing
+// the spec outright; callers outside one pass gomega.Default.
+func captureAll(g gomega.Gomega, f *framework.Framework) map[string]RBGSnapshot {


[D — blocking] captureAll lists RoleBasedGroupList only — the RoleBasedGroupSet root is never snapshotted, so its generation/UID churn across the upgrade is invisible. This is a capture-path gap; I verified it by inspection (a runnable capture test would need a fake controller-runtime client the e2e package doesn't currently wire up), so it's the one blocking item without a unit canary.

Fix: also capture the RBGSet root.

Fixed in 125a226 — you were right, and this was the one item of this round that was still open when I audited the branch against your review.

captureAll now lists RoleBasedGroupSetList as well and merges the roots into the same name-keyed map (snapshot.go:172-197). The roots share RBGSnapshot, which gained a Kind field, so every detector covers them for free: the ones that need pods, owners or Services find empty maps and say nothing, while identity and generation are compared exactly as for an RBG. RoleBasedGroupSet has no entry in generationBumps, so any rewrite of the object that would restamp its children is reported. Names cannot collide, since a set stamps its children out as <set>-<ordinal>.

cheyang · 2026-08-30T00:16:06Z

+			// between the two samples, so leaving it in would make this comparison
+			// report a race on every run.
+			fs := &findings{}
+			quietFirst, quietAfter := exclude(first, mutated...), exclude(after, mutated...)


[F1 — blocking] The settle guard calls only checkSameRBGSet + checkNoPodChurn + checkNoRestarts. A controller still rewriting pod labels (or a Service selector, or an owner generation) passes this, so the suite can't say "the controller hasn't converged" and falls through to the before/after comparison, blaming the upgrade. checkPodMetadataStable exists and catches it — it's just not wired in here.

Reproduced (H1 canary): the three settle detectors report nothing for a pure label drift while checkPodMetadataStable reports the same drift.

Fix: call checkPodMetadataStable (ideally the full set) in the settle comparison, same as runDetectors does.

Fixed in 125a226, and then the guard itself was reworked.

First half: the hand-picked list is gone. Both the guard and runDetectors now go through checkSnapshotDiff (snapshot.go:540), so a controller still rewriting pod labels, a Service selector or an owner generation no longer passes it, and a detector added later cannot be forgotten in one of the two places.

Second half: running the full set surfaced that the fixed window was wrong. helm upgrade --wait returns once the new controller pod is available, which is before it holds the leader-election lease and before its first reconcile of every fixture has landed, so the run reported that first reconcile — one LeaderWorkerSet patch plus the KEP-260 shared-Service narrowing — as churn caused by the upgrade. Phase 3 now calls waitQuiesced (snapshot.go:498), which samples repeatedly until two samples settleDuration apart agree, bounded at 5 minutes, and fails saying the controller is still moving. Nothing is folded and no detector is relaxed: whatever happened before it went quiet still has to be accounted for by the comparison against the pre-upgrade snapshot, where only the recorded rewrites are allowed.

On a real cluster (v0.7.0 -> v0.8.0-97fea1a2) the wait took three rounds before the samples agreed, and all 15 specs then pass.

cheyang · 2026-08-30T00:16:06Z

+	}
+}
+
+func hasDefaultRestartDelays(config map[string]any) bool {


[F8] hasDefaultRestartDelays treats an absent delay field as matching the default, so the restartPolicyConfig fold also applies to an empty config — hiding a conversion bug that wrote one behind the surviving restartPolicy string. Reproduced (H4 canary). Fix: require both delay fields present and matching, not "absent or matching".

Fixed in 125a226. hasDefaultRestartDelays now requires both keys to be present and to hold exactly the values the v1alpha2 getters default the deprecated string to; an absent key returns false (snapshot.go:829-838). The fold is only sound because the pair says the same thing, so an empty or partial config is a difference worth reporting rather than one to strip.

cheyang · 2026-08-30T00:16:06Z

+	}
+	if len(names) > 0 {
+		ginkgo.Fail(cleanupInstructions(fmt.Sprintf(
+			"%d %s CRDs are already registered (%s); the suite installs the old CRD bundle itself, so "+


[F7] requireCleanCluster checks the CRDs aren't registered, not for leftover custom resources. Narrow (needs a prior failed run whose CRDs were deleted while CRs hung on finalizers), but a leftover-CR check would close it.

I do not think this state is reachable, so I have left it alone — happy to be shown otherwise.

The scenario needs custom resources that outlive their CRD's registration. If CRs hang on finalizers, the CRD delete does not complete: it stays present in Terminating, and requireCleanCluster lists it and fails, because it reads the CRD list rather than a condition. There is no window where the CRs exist but the CRD is unregistered — deleting the CRD is what removes the resource from discovery in the first place.

What the scenario does describe accurately is a teardown that returns while the cascade is still running, and that is now waited out per F3 (install.go:299-319), so the suite no longer leaves Terminating CRDs behind for the next run to trip over.

cheyang · 2026-08-30T00:16:06Z

+			// and the failure cannot say which: the upgrade rolled instances the partition
+			// had pinned, or the partition never pinned them.
+			gomega.Expect(
+				countSurvivors(midRollPodsBefore, listPodFactsForRole(gomega.Default, f, target, midRollRole)),


[nit F9] The mid-rollout partition check (count by UID == partition) can't tell "partition removed by the upgrade" from "partition ignored". The comment already acknowledges the ambiguity; flagging in case a sharper assertion is worth it.

This is closed by the fix for A, I think. "Partition ignored" means the instances the partition pinned get replaced, and the partitioned role's pods are now compared with checkNoPodChurn, checkNoRestarts and checkPodMetadataStable against the snapshot taken at the partition (specs.go:386-388) — so that case fails on pod identity rather than having to be distinguished by a count. "Partition removed by the upgrade" fails the same way. The count is what remains ambiguous, and it is no longer the only thing being asserted.

cheyang · 2026-08-30T00:16:06Z

+// checkStillReady requires every role to be as ready as it was, and the RBG's Ready
+// condition to still hold. lastTransitionTime is deliberately not compared: the new
+// controller may legitimately rewrite conditions without changing their meaning.
+func checkStillReady(fs *findings, before, after map[string]RBGSnapshot) {


[nit F10] checkStillReady iterates beforeSnap.ReadyByRole only, so a role with no before status entry is never compared.

Fixed in 125a226. checkStillReady now also walks the after side and reports a role with a status it did not have before (snapshot.go:1179-1184).

cheyang · 2026-08-30T00:16:06Z

+  # This belongs to the release gate rather than the per-PR e2e workflow because the
+  # upgrade target is the chart's own default images. Those point at a real published
+  # release only once a release PR bumps them, and testing the upgrade with exactly the
+  # values being shipped is the point: no --set overrides, no locally built image.


[nit E] This comment still says "no --set overrides" — line 189 added --set controller.features.portAllocator.enabled=true. Stale; update or drop.

Fixed in 125a226. The comment no longer claims there are no overrides; it names the one value that is not a chart default, and says why it is set on both sides of the hop: portAllocator is how the other e2e workflows install rbgs, and setting it on the v0.7.0 install as well is what keeps the upgrade from toggling a feature (release-test.yml:404-414).

…anything Phase 3 measured one fixed window after `helm upgrade --wait` returned, but that boundary is a claim about helm: the new controller pod is available well before it holds the leader-election lease and has reconciled every fixture once. A run against v0.8.0-97fea1a2 reported the controller's very first reconcile -- one LeaderWorkerSet patch and the KEP-260 shared-Service narrowing -- as churn the upgrade had caused. It now waits for two samples taken settleDuration apart to agree, bounded, and fails saying the controller is still moving. Nothing is folded and no detector is loosened: whatever the controller did before it went quiet still has to be accounted for by the comparison against the pre-upgrade snapshot. Also snapshot the RoleBasedGroupSet roots. A child RBG can be intact while the object that owns it was recreated or had its spec rewritten, and the set is what would then restamp the children -- so the roots share RBGSnapshot, carry a Kind, and every detector covers them with no recorded generation bump of their own. The rest is documentation held to what the code establishes: the v1alpha1 Components branch is unreachable end to end rather than merely untested, the restart-policy fold applies only to roles whose rewrite stops at the stored spec, and the leader-election and observation-window limitations say what they actually are. Verified against a v0.7.0 -> v0.8.0-97fea1a2 upgrade on a real cluster: 15 of 15 specs pass, with phase 3 taking three rounds to see the controller settle.

cheyang

Approving on 125a226. Re-ran my unit harness against this head (production code untouched; every bites-check applied and reverted) and re-read the cited lines — 12 of the 16 round-2 findings are fixed, including every blocking major:

B: checkOwnersStable now compares afterSnap.Generation - beforeSnap.Generation (reproduced + reverse-bites-checked).
C: ownerSources now lists RoleBasedGroupScalingAdapter (reproduced + reverse-bites-checked).
D: captureAll now lists the RoleBasedGroupSetList roots; they share RBGSnapshot, carry Kind, and are covered by the detectors.
F1: the phase-3 settle is now waitQuiesced (two samples settleDuration apart must agree), addressing the misattribution-while-moving concern (handled differently than the checkPodMetadataStable wiring I'd suggested — fine).
F2: restartController now calls waitCRDConversionCABundle, matching waitForUpgradeReady.
F3: teardownFromRelease now loops on listOwnedCRDs until empty before returning.
F4: checkNoRestarts now reports after-only containers (contract green).
F5: a Running->Failed Phase flip is now reported.
F6: the v1alpha1 spec now takes a local baseline at annotation time, not the phase-1 before.
F8: hasDefaultRestartDelays now requires both delay fields present and matching.
F10: checkStillReady now iterates afterSnap.ReadyByRole too.
E: the stale "no --set overrides" comment is gone.

Four narrow/nit items remain, flagged inline as non-blocking follow-ups: A (mid-rollout fixture still excluded from detectors — but it's designed to churn, so narrower than B/C/D were), F7 (requireCleanCluster checks CRDs registered not leftover CRs), F9 (mid-rollout partition ambiguity, acknowledged in the comment), G (waitCRDConversionCABundle is NotBeEmpty-only; non-empty != correct). None blocks merge.

Harness, observed-vs-expected table, and bites-checks: https://github.com/cheyang/rbg/tree/verify/pr444-e2e-upgrade-suite — docs/verification/pr444-e2e-upgrade-suite/README.md. The prior REQUEST_CHANGES review was against the earlier head 3f854c1; this supersedes it.

cheyang · 2026-08-30T08:03:50Z

+			after := waitQuiesced(f, mutated...)
+
+			fs := &findings{}
+			runDetectors(fs, f, before, after, preUpgradeMark, upgradeRewrites.acrossStarts(controllerStarts), mutated)


[A — non-blocking follow-up] The mid-rollout fixture is still in mutated and excluded from runDetectors here (and from the quiescence check), so countSurvivors (UID equality) is the only assertion on those pods. A surviving partition pod whose restartCount bumped is invisible. Reproduced (H5 canary still passes). Not blocking: the fixture is designed to churn, so this is narrower than the B/C/D silent-passes were. Worth a dedicated mid-rollout drift detector later if you want it airtight.

cheyang · 2026-08-30T08:03:50Z

+	}
+	if len(names) > 0 {
+		ginkgo.Fail(cleanupInstructions(fmt.Sprintf(
+			"%d %s CRDs are already registered (%s); the suite installs the old CRD bundle itself, so "+


[F7 — non-blocking follow-up] requireCleanCluster still checks CRDs aren't registered, not for leftover custom resources. Narrow (needs a prior failed run whose CRDs were deleted while CRs hung on finalizers). A leftover-CR check would close it; not blocking.

cheyang · 2026-08-30T08:03:50Z

+				countSurvivors(midRollPodsBefore, listPodFactsForRole(gomega.Default, f, target, midRollRole)),
+			).To(
+				gomega.Equal(midRollPartition),
+				"instances that partition %d should have pinned at the old revision were rolled, either by "+


[F9 — nit] The partition check still can't distinguish "partition removed by the upgrade" from "partition ignored" — the comment already says so. Leaving as-is is reasonable; flagging only in case a sharper assertion is wanted.

cheyang · 2026-08-30T08:03:50Z

+				crd.Object, "spec", "conversion", "webhook", "clientConfig", "caBundle")
+			g.Expect(err).ToNot(gomega.HaveOccurred())
+			g.Expect(found).To(gomega.BeTrue(), "CRD %s has no conversion webhook caBundle field", crdName)
+			g.Expect(caBundle).ToNot(gomega.BeEmpty(), "CRD %s conversion caBundle is empty", crdName)


[G — non-blocking follow-up] waitCRDConversionCABundle still asserts the bundle is found + NotBeEmpty; non-empty ≠ correct, so a stale-but-populated bundle would still pass this gate. Narrow (the gate now at least waits for the post-restart re-population). A byte-equality check against the validating-webhook bundle would make it airtight; not blocking.

照微 added 2 commits August 29, 2026 22:21

e2e: test update from v0.7.0

129f28e

cheyang requested a balanced review from Copilot August 29, 2026 15:29

Copilot started reviewing on behalf of cheyang August 29, 2026 15:29

View session

run go mod tidy

06d5b4d

Copilot AI reviewed Aug 29, 2026

View reviewed changes

cheyang requested a balanced review from Copilot August 29, 2026 22:51

Copilot started reviewing on behalf of cheyang August 29, 2026 22:52

View session

This comment was marked as outdated.

Sign in to view

Copilot AI reviewed Aug 29, 2026

View reviewed changes

cheyang reviewed Aug 29, 2026

View reviewed changes

cheyang requested changes Aug 30, 2026

View reviewed changes

diw-zw requested a review from cheyang August 30, 2026 05:57

cheyang approved these changes Aug 30, 2026

View reviewed changes

cheyang merged commit 1670515 into sgl-project:main Aug 30, 2026
11 checks passed

		// - Single-node, single-replica controller: nothing about leader election
		// handover, multi-node scheduling, or rollout behavior under real load.

Conversation

diw-zw commented Aug 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Ⅰ. Motivation

Ⅱ. Modifications

Ⅲ. Does this pull request fix one issue?

Ⅳ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.

Ⅴ. Describe how to verify it

VI. Special notes for reviews

Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cheyang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

diw-zw commented Aug 29, 2026 •

edited

Loading