Fix ASTS ControllerRevision adoption idempotency#132
Open
fgksgf wants to merge 2 commits into
Open
Conversation
|
|
There was a problem hiding this comment.
Pull request overview
This PR improves AdvancedStatefulSet (ASTS) migration robustness by making ControllerRevision adoption and migration-marker processing idempotent, preventing repeated adoption attempts and cleaning up upgrade markers once revisions have been reconciled.
Changes:
- Deduplicate ControllerRevisions returned from selector-based and upgrade-marker-based listing.
- Make adoption idempotent by skipping revisions already owned by the same ASTS UID and rejecting conflicting ownership.
- Ensure revision labels are synced via deep-copy and remove the migration marker; add regression tests for mixed owned/orphan scenarios.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/controller/statefulset/stateful_set.go | Deep-copies revisions before label mutation; removes upgrade marker; adjusts label-sync/adoption flow. |
| pkg/controller/statefulset/stateful_set_test.go | Adds controller-level regression tests covering marker cleanup and orphan adoption behavior. |
| pkg/controller/statefulset/stateful_set_control.go | Dedupes listed revisions and makes adoption idempotent for already-owned revisions while rejecting other owners. |
| pkg/controller/statefulset/stateful_set_control_test.go | Adds unit tests for revision dedupe and adoption ownership semantics; adds helpers for upgrade-marker scenarios. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Make ControllerRevision adoption idempotent during native StatefulSet to AdvancedStatefulSet migration so partial-adoption retries no longer fail with "attempt to adopt revision owned by ...". - ListRevisions returns each revision once and keeps revisions that are orphaned, owned by this set, or still owned by the builtin StatefulSet being migrated (marker + apps/v1 StatefulSet owner); foreign revisions are filtered out. - AdoptOrphanRevisions skips revisions already owned by this set, adopts only orphans, and still rejects revisions owned by another controller. - createControllerRevision only reuses a data-equal revision when it is usable by this set, avoiding duplicate revisions during migration. - truncateHistory keeps builtin migration revisions until GC orphans them; getStatefulSetRevisions does not bump the Revision of a builtin-owned migration revision on rollback. - syncLabels deep-copies the revision, restores selector labels, and removes the migration marker only for orphaned/owned revisions. Add regression coverage for mixed owned/orphan revisions, conflicting ownership, builtin-owner migration revisions, and rollback handling.
fgksgf
force-pushed
the
codex/asts-adoption-idempotent
branch
from
05fecf3 to
148d79e
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Why
During ASTS migration, a reconcile can partially adopt ControllerRevisions and then retry while some revisions are already owned by the same AdvancedStatefulSet and others still carry
apps.pingcap.com/upgrade-to-asts. The previous adoption path tried to adopt every listed revision again, so it failed withattempt to adopt revision owned by ...and could leave the StatefulSet status/scale path stuck.How
ListRevisionsstill lists revisions by selector and by upgrade marker, but now returns each revision once.AdoptOrphanRevisionsskips revisions already owned by the same StatefulSet UID, adopts only orphan revisions, and still rejects revisions owned by another controller.syncLabelsdeep-copies the revision, restores selector labels from the pod template, and removes the migration marker.adoptOrphanRevisionssyncs labels whenever marker cleanup is needed, even if there are no remaining orphan revisions.Testing
git diff --check origin/master...HEADGOOS=linux GOARCH=amd64 go test -c ./pkg/controller/statefulset -o /tmp/advanced-statefulset-statefulset.testgo test ./pkg/controller/statefulsetGINKGO_NO_COLOR=y KUBE_VERSION=v1.28 KUBE_WORKERS=1 ./hack/e2e.sh05fecf337327:16 Passed | 0 Failed | 0 Pending | 5 Skippedin15m35.765805677s.Risks and Reviewer Focus
v0.7.0tag for a patch-release style fix, while the PR targetsmaster.git merge-treedid not show conflict markers when merging into currentmaster.