Add design proposal: platform migration engine - #58
myasnikovdaniil wants to merge 16 commits into
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📝 WalkthroughWalkthroughThe proposal replaces integer-based platform migrations with content-addressed IDs, ledger-based tracking, explicit execution tiers, dependency ordering, package scoping, validation rules, and a phased legacy conversion. ChangesPlatform migration design
Estimated code review effort: 2 (Simple) | ~15 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (2)
design-proposals/platform-migrations/README.md (2)
61-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd language tags to the fenced examples.
markdownlint-cli2reports MD040 for these three blocks. Addtext,sh, oryamlto each fence as appropriate.Also applies to: 102-106, 207-211
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@design-proposals/platform-migrations/README.md` around lines 61 - 70, Add language tags to all three fenced code blocks in the migration README, including the shown block and the blocks referenced at lines 102–106 and 207–211. Use the appropriate fence annotation for each block, such as text, sh, or yaml, so markdownlint MD040 passes.Source: Linters/SAST tools
120-135: 🩺 Stability & Availability | 🔵 TrivialAdd a ledger growth policy.
Retention deletes migration files but keeps every
m.*key. Package-scoped histories increase the key count. Define a size budget and a compaction or partitioning strategy before the ConfigMap becomes an availability limit.Also applies to: 244-248
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@design-proposals/platform-migrations/README.md` around lines 120 - 135, Update the Ledger design around the ConfigMap and its write mechanics to define a bounded size budget and an explicit growth-management strategy for m.* entries, such as compaction or partitioning. Ensure the policy accounts for retained package-scoped migration history and specifies how entries are handled when the budget is approached.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@design-proposals/platform-migrations/README.md`:
- Around line 59-69: Rename the documented identity scheme to a date/slug-based
identifier, removing any content-addressed implication. In the migration
identity documentation around the YYYYMMDD-slug example, define collision
handling for duplicate IDs and require verifying that an existing ID’s recorded
checksum matches its migration content; do not add content-derived ID generation
unless the scheme is intentionally changed.
- Around line 155-157: Update the migration immutability guard and ledger
checksum logic to include the transitive contents of migrations/lib/ alongside
each top-level migration script. Ensure changes to shared helpers alter the
recorded execution identity and cause mismatches to be rejected, or introduce an
equivalent versioned helper/image digest mechanism; apply the same behavior to
the related migration paths.
- Around line 110-118: Clarify the proposal’s requires semantics across
pre-apply and background execution tiers, including the referenced sections.
Either reject dependencies between tiers with explicit validation, or define a
shared barrier and deterministic topological ordering that prevents pre-apply
migrations from depending on pending background work and prevents background
jobs from starting before pre-apply dependencies are recorded.
- Around line 237-242: The migration seeding logic must populate every missing
legacy record below the cluster’s version, rather than requiring the ledger to
have no m.* keys. Update the step 3 runner behavior to iterate the legacy-map
entries below version and create only absent m.<id> records with outcome legacy,
preserving idempotency and existing records.
- Around line 174-191: Update the migration index generation around the
cozystack-migrations-index ConfigMap so its IDs are derived from the packaged
migration files rather than maintained separately. Add CI validation comparing
the chart’s packaged IDs, migration image IDs, and generated index IDs, and fail
on any mismatch to ensure every scheduled migration has a corresponding script
and background migration is not omitted.
- Around line 174-175: Update the pre-apply work-list gate described in the
migration proposal to include revoked IDs, triggering the runner when revoked
IDs are absent from the ledger so it can write the promised m.<id>: ... revoked
record. Apply the same revoked-minus-ledger handling to background IDs, using
either a revoked marker in the tier input or an equivalent gate condition.
- Around line 153-157: Update the migration proposal and hack/lint-migrations.sh
requirements to mandate an explicit fail-fast mechanism: each migration must use
set -e or the runner must invoke it with sh -e. Extend the linter/tests to
detect and enforce this requirement so a failed command cannot be followed by a
successful command and recorded as ok.
- Around line 135-136: Update the migration execution design to address crashes
between applying migration side effects and recording m.<id>: either make every
migration idempotent on retry or introduce a recoverable per-ID claim/lease that
prevents unsafe repetition. Document the selected behavior and ensure
reconciliation can safely resume pending migrations after a Job failure.
- Around line 176-193: Update the background Job specification described around
the migration index so each Job Pod explicitly uses a dedicated cozy-system
ServiceAccount rather than the namespace default. Define the ServiceAccount and
its cluster-admin binding, document their lifecycle with the migration
resources, and add a rendered identity test verifying
spec.template.spec.serviceAccountName.
- Around line 148-153: Update the migration configuration and runner logic
around the pre-apply/background tier handling and ledger outcome checks: enforce
that pre-apply migrations always use abort semantics, and make background non-ok
records consistently either retryable or terminal. Align the pending-selection
logic near the retry path and the terminal-outcome check so recorded non-ok
results follow that chosen behavior without contradicting the migration
contract.
---
Nitpick comments:
In `@design-proposals/platform-migrations/README.md`:
- Around line 61-70: Add language tags to all three fenced code blocks in the
migration README, including the shown block and the blocks referenced at lines
102–106 and 207–211. Use the appropriate fence annotation for each block, such
as text, sh, or yaml, so markdownlint MD040 passes.
- Around line 120-135: Update the Ledger design around the ConfigMap and its
write mechanics to define a bounded size budget and an explicit
growth-management strategy for m.* entries, such as compaction or partitioning.
Ensure the policy accounts for retained package-scoped migration history and
specifies how entries are handled when the budget is approached.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b7515bb1-f3f1-4578-a580-74f3336782a2
📒 Files selected for processing (1)
design-proposals/platform-migrations/README.md
| Identity is `YYYYMMDD-slug`. Tier is the **directory**, so nothing is parsed at render time and there is no generated index to keep in sync: | ||
|
|
||
| ``` | ||
| packages/core/platform/images/migrations/migrations/ | ||
| 1 .. 53 # legacy integer set — frozen, never extended | ||
| lib/ | ||
| revoked # IDs that must not run (§8) | ||
| pre-apply/ # blocking, runs in the pre-upgrade hook Job | ||
| 20260812-redis-failover-group-label | ||
| background/ # non-blocking, run by cozystack-operator | ||
| 20260814-clickhouse-keeper-pvc-labels |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Align the identity name with the actual identity scheme.
YYYYMMDD[-NN]-slug is a sortable human-assigned identifier, not a content-addressed identifier. The checksum is recorded after execution and is not part of the ID. Two branches can assign the same ID to different content, and two authors can choose the same date and slug. Either derive IDs from content, or rename this to a date/slug scheme and define collision and ID-to-content verification rules.
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 61-61: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@design-proposals/platform-migrations/README.md` around lines 59 - 69, Rename
the documented identity scheme to a date/slug-based identifier, removing any
content-addressed implication. In the migration identity documentation around
the YYYYMMDD-slug example, define collision handling for duplicate IDs and
require verifying that an existing ID’s recorded checksum matches its migration
content; do not add content-derived ID generation unless the scheme is
intentionally changed.
| `NN` covers intent and readability but enforces nothing. Where one migration genuinely depends on another — across any dates — it is declared and verified: | ||
|
|
||
| ```sh | ||
| # cozystack-migration: requires=20260801-etcd-crds | ||
| ``` | ||
|
|
||
| The runner topologically sorts on `requires` and fails loudly on a missing or cyclic dependency rather than guessing. | ||
|
|
||
| Merge order is deliberately not encoded and must not be relied on: a PR merged in June can carry a later date than one merged in July. What the scheme guarantees is that every cluster — fresh install or two years old — walks the same total order. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Define requires behavior across execution tiers.
The proposal allows dependencies across dates, but the pre-apply runner processes only pre-apply/ while the operator processes background/ later. A pre-apply migration cannot wait for a pending background dependency. A background Job can also start before its pre-apply dependency is recorded. Reject cross-tier dependencies or define a shared dependency barrier and deterministic topological ordering.
Also applies to: 167-170, 191-193
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@design-proposals/platform-migrations/README.md` around lines 110 - 118,
Clarify the proposal’s requires semantics across pre-apply and background
execution tiers, including the referenced sections. Either reject dependencies
between tiers with explicit validation, or define a shared barrier and
deterministic topological ordering that prevents pre-apply migrations from
depending on pending background work and prevents background jobs from starting
before pre-apply dependencies are recorded.
| **Write mechanics.** Ledger keys are written with `kubectl patch --type merge`: atomic, no read-modify-write race between concurrent writers. The existing `stamp_cozystack_version` in `migrations/lib/cozystack-version.sh` uses `kubectl apply` and must keep touching **only** `version`. Apply's three-way merge prunes keys that were in last-applied-configuration but absent from the new manifest; patch-written `m.*` keys were never in last-applied, so they survive — subtle enough to deserve a dedicated test. It is the same class of bug migration `42` was written to fix for the `no-delete` label. | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 6 \
'record_migration|kubectl patch|ONLY=|generateName|ownerReferences|AlreadyExists' \
packages/core/platform internal/operator hackRepository: cozystack/community
Length of output: 335
🏁 Script executed:
set -euo pipefail
printf '%s\n' '--- candidate files ---'
git ls-files | rg '(^|/)(README\.md|.*migration.*|.*platform.*|.*cozystack-version.*|.*operator.*)$' | head -200
printf '%s\n' '--- relevant symbols ---'
rg -n -C 8 'record_migration|stamp_cozystack_version|kubectl patch|kubectl apply|generateName|ownerReferences|AlreadyExists|m\.' . --glob '!vendor/**' --glob '!node_modules/**' | head -400Repository: cozystack/community
Length of output: 50377
🏁 Script executed:
set -euo pipefail
printf '%s\n' '--- proposal sections ---'
sed -n '120,205p' design-proposals/platform-migrations/README.md
printf '%s\n' '--- failure and testing sections ---'
sed -n '282,307p' design-proposals/platform-migrations/README.md
printf '%s\n' '--- implementation files referenced by the proposal ---'
for path in migrations/lib/cozystack-version.sh packages/core/platform internal/operator hack; do
if [ -e "$path" ]; then
printf 'present: %s\n' "$path"
git ls-files "$path" | head -100
else
printf 'absent: %s\n' "$path"
fi
done
printf '%s\n' '--- all tracked migration/operator paths ---'
git ls-files | rg '(^|/)(migrations|hack|internal/operator|packages/core/platform)(/|$)' | head -200Repository: cozystack/community
Length of output: 12130
🏁 Script executed:
python3 - <<'PY'
from dataclasses import dataclass
`@dataclass`
class State:
ledger: set[str]
effects: int = 0
def run_once(state: State, migration_id: str, crash_after_effect: bool = False):
if migration_id in state.ledger:
return "skip"
state.effects += 1
if crash_after_effect:
return "crash"
state.ledger.add(migration_id)
return "recorded"
state = State(set())
first = run_once(state, "m.example", crash_after_effect=True)
second = run_once(state, "m.example")
print({
"first_attempt": first,
"second_attempt": second,
"effect_count": state.effects,
"ledger": sorted(state.ledger),
"rerun_occurred": state.effects == 2,
})
assert state.effects == 2
assert "m.example" in state.ledger
PYRepository: cozystack/community
Length of output: 282
Make migration execution idempotent.
The atomic ledger patch does not make migration side effects and ledger updates transactional. If a Job exits after applying changes but before recording m.<id>, reconciliation retries the pending ID and repeats those changes. Serial background Jobs do not remove this crash window. Require idempotent migrations, or add a recoverable per-ID claim/lease.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@design-proposals/platform-migrations/README.md` around lines 135 - 136,
Update the migration execution design to address crashes between applying
migration side effects and recording m.<id>: either make every migration
idempotent on retry or introduce a recoverable per-ID claim/lease that prevents
unsafe repetition. Document the selected behavior and ensure reconciliation can
safely resume pending migrations after a Job failure.
| # cozystack-migration: tier=pre-apply # pre-apply | background | ||
| # cozystack-migration: on-error=abort # abort | warn | ||
| # cozystack-migration: requires=20260801-etcd-crds | ||
| ``` | ||
|
|
||
| `on-error=warn` turns the "best-effort by design" paragraph into something the runner enforces. The script is then written plainly fail-fast, and the runner decides what a non-zero exit means — instead of `|| true` per command, which also swallows the failures the author wanted to see. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make on-error consistent with the tier and ledger state.
A pre-apply migration with on-error=warn can fail while the chart continues, which defeats the pre-apply guarantee. Under the stated set difference, a recorded non-ok result is also no longer pending, which conflicts with the retry behavior in Line 293 and the terminal-outcome check in Line 306. Enforce pre-apply => abort, and define whether background non-ok records are retried or terminal.
Also applies to: 168-170, 287-306
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@design-proposals/platform-migrations/README.md` around lines 148 - 153,
Update the migration configuration and runner logic around the
pre-apply/background tier handling and ledger outcome checks: enforce that
pre-apply migrations always use abort semantics, and make background non-ok
records consistently either retryable or terminal. Align the pending-selection
logic near the retry path and the terminal-outcome check so recorded non-ok
results follow that chosen behavior without contradicting the migration
contract.
| `on-error=warn` turns the "best-effort by design" paragraph into something the runner enforces. The script is then written plainly fail-fast, and the runner decides what a non-zero exit means — instead of `|| true` per command, which also swallows the failures the author wanted to see. | ||
|
|
||
| **A shared library, extended.** `migrations/lib/` already holds `cozystack-version.sh` and `seaweedfs-db-adopt.sh`, so the precedent exists. It grows helpers for operations that keep being re-implemented: a `kubectl` wrapper with retry on transient apiserver errors (what the `|| true` sites are really reaching for), a list helper that does not SIGPIPE under `pipefail` (migration `44` documents that trap in a comment), and the Helm-ownership adopt / `resource-policy: keep` pattern shared by `31`, `33`, `35`, `43`, `45` and `53` — by far the most repeated operation in the tree. | ||
|
|
||
| **A linter**, `hack/lint-migrations.sh`, wired into `make unit-tests`: filename matches the ID grammar; header present, parseable, and declaring `tier` and `on-error`; one shell dialect (`#!/bin/sh`, since the image is busybox — several migrations are currently `#!/bin/bash` for no stated reason); `shellcheck` clean; `requires` targets exist; no direct writes to the ledger. That last rule is where the architectural guard currently in `hack/cozystack-version-stamp.bats` moves to. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -i 'run-migrations.sh|lint-migrations.sh' |
while IFS= read -r file; do
rg -n -C 5 'set[[:space:]]+-e|sh[[:space:]]+-e|exec[[:space:]]+.*migration' "$file"
doneRepository: cozystack/community
Length of output: 157
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- migration-related files ---'
git ls-files | rg '(^|/)(run-migrations|lint-migrations|.*migration.*|migrations/|hack/)' | head -200
printf '%s\n' '--- README references to execution and on-error ---'
rg -n -C 4 'on-error|fail-fast|run-migrations|set -e|shell|runner|execute|non-zero|exit' design-proposals/platform-migrations/README.md
printf '%s\n' '--- migration script headers and error handling ---'
files="$(git ls-files | rg '(^|/)migrations/[^/]+\.sh$' || true)"
if [ -n "$files" ]; then
while IFS= read -r file; do
printf '\n### %s\n' "$file"
sed -n '1,24p' "$file"
done <<< "$files"
fi
printf '%s\n' '--- migration invocation sites ---'
rg -n -C 5 'migrations|run-migrations|on-error|sh -|bash ' --glob '*.sh' --glob '*.yaml' --glob '*.yml' --glob 'Makefile*' --glob '*.mk' . | head -300Repository: cozystack/community
Length of output: 21382
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- proposal execution requirements ---'
sed -n '137,173p' design-proposals/platform-migrations/README.md
sed -n '282,306p' design-proposals/platform-migrations/README.md
printf '%s\n' '--- referenced implementation files present in the tree ---'
for path in \
packages/core/platform/images/migrations \
packages/core/platform/templates/migration-hook.yaml \
hack/lint-migrations.sh \
hack/cozystack-version-stamp.bats
do
if git ls-files --error-unmatch "$path" >/dev/null 2>&1; then
printf 'present: %s\n' "$path"
else
printf 'absent: %s\n' "$path"
fi
done
printf '%s\n' '--- shell behavior without and with errexit ---'
sh -c 'false; printf "continued-after-failure\n"; exit 0'
printf 'plain status: %s\n' "$?"
sh -e -c 'false; printf "continued-after-failure\n"; exit 0' || printf 'errexit status: %s\n' "$?"
printf '%s\n' '--- explicit fail-fast wording in the proposal ---'
rg -n -i -C 2 'fail[- ]fast|errexit|set[[:space:]]+-e|sh[[:space:]]+-e|shellcheck|middle of|partial|non-zero' design-proposals/platform-migrations/README.mdRepository: cozystack/community
Length of output: 10137
Require an explicit fail-fast mechanism.
The proposal says migrations are “plainly fail-fast,” but it does not require set -e or invoke scripts with sh -e. Without either mechanism, a failed command can be followed by a successful command, and the runner can record ok. Define and test this requirement.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@design-proposals/platform-migrations/README.md` around lines 153 - 157,
Update the migration proposal and hack/lint-migrations.sh requirements to
mandate an explicit fail-fast mechanism: each migration must use set -e or the
runner must invoke it with sh -e. Extend the linter/tests to detect and enforce
this requirement so a failed command cannot be followed by a successful command
and recorded as ok.
| **A shared library, extended.** `migrations/lib/` already holds `cozystack-version.sh` and `seaweedfs-db-adopt.sh`, so the precedent exists. It grows helpers for operations that keep being re-implemented: a `kubectl` wrapper with retry on transient apiserver errors (what the `|| true` sites are really reaching for), a list helper that does not SIGPIPE under `pipefail` (migration `44` documents that trap in a comment), and the Helm-ownership adopt / `resource-policy: keep` pattern shared by `31`, `33`, `35`, `43`, `45` and `53` — by far the most repeated operation in the tree. | ||
|
|
||
| **A linter**, `hack/lint-migrations.sh`, wired into `make unit-tests`: filename matches the ID grammar; header present, parseable, and declaring `tier` and `on-error`; one shell dialect (`#!/bin/sh`, since the image is busybox — several migrations are currently `#!/bin/bash` for no stated reason); `shellcheck` clean; `requires` targets exist; no direct writes to the ledger. That last rule is where the architectural guard currently in `hack/cozystack-version-stamp.bats` moves to. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Include shared helper code in the immutable execution identity.
migrations/lib/ is shared by multiple migration IDs, but the immutability guard and ledger checksum cover only the top-level migration file. Changing a helper can change migration behavior without changing the ID or recorded checksum. Version the helpers, or record an image or transitive-source digest and reject mismatches.
Also applies to: 215-215, 280-280
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@design-proposals/platform-migrations/README.md` around lines 155 - 157,
Update the migration immutability guard and ledger checksum logic to include the
transitive contents of migrations/lib/ alongside each top-level migration
script. Ensure changes to shared helpers alter the recorded execution identity
and cause mismatches to be rejected, or introduce an equivalent versioned
helper/image digest mechanism; apply the same behavior to the related migration
paths.
| **Pre-apply** stays the render-gated hook. The gate generalises from a scalar compare to a set difference: the chart already ships the scripts (there is no `.helmignore` in `packages/core/platform`), so `.Files.Glob "images/migrations/migrations/pre-apply/*"` yields the ID list at render with no content reads. The Job is only created when the difference is non-empty. | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Include revoked IDs in the work-list gate.
The gate computes IDs only from pre-apply/. If revocation moves an ID into revoked/, no Job starts and the proposal cannot write the promised m.<id>: ... revoked record. Keep a revoked marker in the tier input or trigger the runner for revoked − ledger. Apply the same rule to background IDs.
Also applies to: 203-204
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@design-proposals/platform-migrations/README.md` around lines 174 - 175,
Update the pre-apply work-list gate described in the migration proposal to
include revoked IDs, triggering the runner when revoked IDs are absent from the
ledger so it can write the promised m.<id>: ... revoked record. Apply the same
revoked-minus-ledger handling to background IDs, using either a revoked marker
in the tier input or an equivalent gate condition.
| **Pre-apply** stays the render-gated hook. The gate generalises from a scalar compare to a set difference: the chart already ships the scripts (there is no `.helmignore` in `packages/core/platform`), so `.Files.Glob "images/migrations/migrations/pre-apply/*"` yields the ID list at render with no content reads. The Job is only created when the difference is non-empty. | ||
|
|
||
| **Background** is orchestrated — not executed — by cozystack-operator, because the scripts live in the migrations image and the operator has neither them nor a Helm renderer. The chart therefore writes down the two things the operator cannot derive: which IDs are background, and which migrations image the current platform release pins. | ||
|
|
||
| ```yaml | ||
| apiVersion: v1 | ||
| kind: ConfigMap | ||
| metadata: | ||
| name: cozystack-migrations-index | ||
| namespace: cozy-system | ||
| data: | ||
| image: ghcr.io/cozystack/cozystack/platform-migrations:v1.7.0@sha256:… | ||
| background: | | ||
| 20260814-clickhouse-keeper-pvc-labels | ||
| 20260814-tenant-ancestor-labels | ||
| ``` | ||
|
|
||
| The operator reconciles `pending = background − ledger − revoked`, creates one Job per pending ID from `image` with `ONLY=<id>`, and patches the ledger on success. Because the ConfigMap is re-rendered on every platform upgrade, the image ref and the list cannot drift from the release that shipped them. The operator is already `cluster-admin`, so this needs no RBAC change. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make the packaged migration set a single verifiable contract.
The hook derives IDs from chart files, execution reads the migration image, and background execution reads the separate background list. A chart/image mismatch can create a Job without its script. An omitted background ID is never scheduled. Generate the index from the packaged files and add a CI check that compares the chart, image, and index ID sets.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@design-proposals/platform-migrations/README.md` around lines 174 - 191,
Update the migration index generation around the cozystack-migrations-index
ConfigMap so its IDs are derived from the packaged migration files rather than
maintained separately. Add CI validation comparing the chart’s packaged IDs,
migration image IDs, and generated index IDs, and fail on any mismatch to ensure
every scheduled migration has a corresponding script and background migration is
not omitted.
| **Background** is orchestrated — not executed — by cozystack-operator, because the scripts live in the migrations image and the operator has neither them nor a Helm renderer. The chart therefore writes down the two things the operator cannot derive: which IDs are background, and which migrations image the current platform release pins. | ||
|
|
||
| ```yaml | ||
| apiVersion: v1 | ||
| kind: ConfigMap | ||
| metadata: | ||
| name: cozystack-migrations-index | ||
| namespace: cozy-system | ||
| data: | ||
| image: ghcr.io/cozystack/cozystack/platform-migrations:v1.7.0@sha256:… | ||
| background: | | ||
| 20260814-clickhouse-keeper-pvc-labels | ||
| 20260814-tenant-ancestor-labels | ||
| ``` | ||
|
|
||
| The operator reconciles `pending = background − ledger − revoked`, creates one Job per pending ID from `image` with `ONLY=<id>`, and patches the ledger on success. Because the ConfigMap is re-rendered on every platform upgrade, the image ref and the list cannot drift from the release that shipped them. The operator is already `cluster-admin`, so this needs no RBAC change. | ||
|
|
||
| Background Jobs run **serially**, one at a time in ID order: it matches the current model, keeps failure attribution unambiguous, and avoids two migrations touching the same objects concurrently. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 8 \
'cozystack-migrations-index|ONLY=|serviceAccountName|ClusterRoleBinding' \
packages/core/platform internal/operatorRepository: cozystack/community
Length of output: 286
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- candidate files ---'
git ls-files | sed -n '1,160p'
printf '%s\n' '--- migration-related paths ---'
git ls-files | rg -i '(^|/)(operator|platform|migration|chart|helm|workload|rbac|serviceaccount)' | sed -n '1,240p'
printf '%s\n' '--- relevant identifiers ---'
rg -n -C 5 \
'cozystack-migrations-index|ONLY=|serviceAccountName|ClusterRoleBinding|cluster-admin|background' \
. --glob '!design-proposals/platform-migrations/README.md' \
--glob '!vendor/**' \
--glob '!node_modules/**' \
| sed -n '1,320p'Repository: cozystack/community
Length of output: 16599
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- proposal context ---'
sed -n '1,260p' design-proposals/platform-migrations/README.md
printf '%s\n' '--- repository manifests and workflow files ---'
git ls-files | rg -i '\.(yaml|yml|json|tpl|go|ts|tsx|js|jsx)$' | sed -n '1,320p'
printf '%s\n' '--- service account and RBAC declarations ---'
rg -n -C 8 \
'kind:[[:space:]]*(ServiceAccount|ClusterRole|RoleBinding|ClusterRoleBinding)|serviceAccountName:|cluster-admin|rbac.authorization.k8s.io' \
. --glob '*.{yaml,yml,json,tpl,go,ts,tsx,js,jsx}' \
| sed -n '1,320p'Repository: cozystack/community
Length of output: 24853
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path("design-proposals/platform-migrations/README.md")
text = p.read_text()
for needle in ("cluster-admin", "ServiceAccount", "serviceAccountName", "Job", "operator"):
print(f"--- {needle} ---")
for i, line in enumerate(text.splitlines(), 1):
if needle.lower() in line.lower():
print(f"{i}:{line}")
PYRepository: cozystack/community
Length of output: 10960
🌐 Web query:
Kubernetes official documentation Job Pod serviceAccountName controller ServiceAccount namespace
💡 Result:
In Kubernetes, the serviceAccountName for a Job is defined within the pod template section of the Job specification [1][2]. The Job controller itself does not directly hold or manage the serviceAccountName; rather, it uses the pod template to create new Pods [3][4]. Key details regarding the relationship between Jobs, Pods, and ServiceAccounts include: 1. Pod Template Configuration: To specify a ServiceAccount for a Job, you must set the spec.template.spec.serviceAccountName field within the Job definition [1][5]. This configuration ensures that every Pod created by the Job controller is assigned the specified ServiceAccount [1]. 2. Admission Controller Role: When a Job controller creates a new Pod based on the template, the API server's ServiceAccount admission controller processes the Pod [6]. If the serviceAccountName is not explicitly provided in the Job's pod template, the admission controller automatically assigns the default ServiceAccount for the namespace where the Pod is created [1][6]. 3. ServiceAccount Controller: A separate ServiceAccount controller manages the existence of ServiceAccounts [6]. It ensures that a ServiceAccount named default exists in every namespace [6][7]. If a Pod (created by a Job or otherwise) references a non-existent ServiceAccount, the admission controller will reject the Pod creation [6]. 4. Namespace Scope: A ServiceAccount is namespaced [8]. A Pod created by a Job will use a ServiceAccount from the same namespace as the Pod itself [1][8]. While you can use RBAC to grant a ServiceAccount permissions to access resources in other namespaces, the ServiceAccount must exist within the namespace where the Pod is running [8]. You can update the serviceAccountName of a Job's template using the kubectl set serviceaccount command, which modifies the pod template of the specified resource [9]. Note that this change affects only future Pods created by the Job controller; existing Pods managed by the Job cannot have their serviceAccountName updated [1].
Citations:
- 1: https://kubernetes.io/docs/tasks/configure-pod-container/configure-service-account/
- 2: https://kubernetes.io/docs/reference/kubernetes-api/batch/job-v1/
- 3: https://kubernetes.io/docs/concepts/workloads/controllers/job/
- 4: https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/job/job_controller.go
- 5: https://registry.terraform.io/providers/hashicorp/kubernetes/latest/docs/resources/job
- 6: https://kubernetes.io/docs/reference/access-authn-authz/service-accounts-admin/
- 7: https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/serviceaccount/serviceaccounts_controller.go
- 8: https://kubernetes.io/docs/concepts/security/service-accounts/
- 9: https://kubernetes.io/docs/reference/kubectl/generated/kubectl_set/kubectl_set_serviceaccount/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- exact design claims around identity and background Jobs ---'
sed -n '168,202p' design-proposals/platform-migrations/README.md
sed -n '268,282p' design-proposals/platform-migrations/README.md
printf '%s\n' '--- all ServiceAccount references in the proposal ---'
rg -n -C 4 'ServiceAccount|ClusterRoleBinding|serviceAccountName|operator.*identity|identity' \
design-proposals/platform-migrations/README.mdRepository: cozystack/community
Length of output: 7326
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path("design-proposals/platform-migrations/README.md")
lines = p.read_text().splitlines()
for start, end in ((176, 193), (270, 278)):
print(f"--- lines {start}-{end} ---")
for n in range(start, end + 1):
print(f"{n}:{lines[n-1]}")
PYRepository: cozystack/community
Length of output: 2777
Specify the ServiceAccount for background Jobs.
Job Pods do not inherit the operator's cluster-admin identity. Without spec.template.spec.serviceAccountName, they use the namespace default ServiceAccount and may fail with Forbidden. Define a cozy-system ServiceAccount, its binding, lifecycle, and a rendered identity test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@design-proposals/platform-migrations/README.md` around lines 176 - 193,
Update the background Job specification described around the migration index so
each Job Pod explicitly uses a dedicated cozy-system ServiceAccount rather than
the namespace default. Define the ServiceAccount and its cluster-admin binding,
document their lifecycle with the migration resources, and add a rendered
identity test verifying spec.template.spec.serviceAccountName.
| 1. Rename `migrations/1..53` to IDs derived from the release each shipped in, preserving order — `20250409-01-mariadb-operator-secrets` through `20260724-53-seaweedfs-db-adopt-repair`. The numeric suffix in the slug keeps the original sequence readable and guarantees the sort matches the old order exactly. | ||
| 2. Commit the integer → ID mapping as `migrations/legacy-map`, one `N <id>` pair per line. | ||
| 3. On first run against a cluster that has a `version` scalar but no `m.*` keys, the runner seeds the ledger from the map: every integer below `version` gets its `m.<id>` recorded with outcome `legacy` (no checksum — those files were not immutable when they ran). Idempotent, since the seeding condition is the absence of `m.*` keys. | ||
| 4. Delete the legacy pass, `targetVersion`, and `hack/check-migrations-target.sh`. | ||
|
|
||
| Only step 3 touches clusters, and it writes ledger keys rather than running anything. A cluster stamped `54` ends up with fifty-three `legacy` records and behaves identically. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Seed missing legacy records, not only an empty ledger.
A cluster can have version: "54" and one m.* key from Phase 1 or Phase 2. The current condition then skips seeding all legacy IDs. After the rename and legacy-pass removal, the 53 renamed migrations appear pending and can run again. Seed each absent m.<id> whose legacy number is below version.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@design-proposals/platform-migrations/README.md` around lines 237 - 242, The
migration seeding logic must populate every missing legacy record below the
cluster’s version, rather than requiring the ledger to have no m.* keys. Update
the step 3 runner behavior to iterate the legacy-map entries below version and
create only absent m.<id> records with outcome legacy, preserving idempotency
and existing records.
|
The identity half of this is right, and it is the hard half. Content-addressed IDs, the immutability guard, the authoring contract, the linter and mandatory tests are all worth having whatever the storage looks like, and the analysis of which migrations are The slot-contention thesis also confirmed itself while the document sat in review. All three PRs named as contending for Things in the text that need fixing regardless of substrateFresh install does not survive the new gate, and this lands in Phase 1. Today a brand-new cluster runs zero migrations, and that is deliberate two-part behaviour: Under a set-difference gate a fresh cluster has an empty ledger, so
The immutability guard is The ledger grows monotonically. §11 lets migrations be deleted at the retention floor but nothing removes their ledger keys, so the ConfigMap only ever grows. The rollback claim is not accurate. "strictly better than the scalar, where a rollback that lowered Per-instance app migrations have no expression. A different substrateWe would like to propose replacing the ConfigMap-plus-Helm-hook substrate while keeping everything above. State moves to a CRD and execution to a controller in cozystack-operator. §Alternatives rejects operator-driven execution because "it loses the runs-before-chart-apply guarantee" — true for an arbitrary operator, but cozystack-operator already owns package rollout ( The two arguments in §4 against a CRD land differently here. The Granularity is the bundle, meaning a repository or the platform chart that delivers everything else. That gives cross-package migrations an owner by construction ( Identity stays content-addressed. A per-bundle State is a watermark plus a sparse list rather than one key per migration forever: status:
baseline: 20260101-etcd-crds # everything older is applied; no entries kept
entries:
- id: 20260812-redis-failover-group-label
phase: Applied
sha256: "abcd…"
ranFromImage: "ghcr.io/…@sha256:…"
jobRef: {name: …, namespace: cozy-system}
attempts: 1
- id: 20260714-clickhouse-keeper
phase: Failed
reason: ScriptError
attempts: 3Anything not Two fields exist only because a CRD allows them and matter operationally. Fresh install becomes explicit: on first setup the controller records every shipped ID as The runner as a shipped mechanismThe piece we would like to make explicit: the controller is generic and the migration logic is not in it. Scripts ship in the bundle's own image, declared once per bundle rather than per migration — so there is no reference to keep fresh, which removes the drift That makes the runner image a thing we use ourselves and provide to anyone building a bundle on Cozystack, in the shape csi-sidecars have: a generic binary implementing the contract, and a component-supplied payload. A bundle author drops scripts in the layout, ships
What this changes in the phasingPhase 3 falls away — package-scoped hooks are unnecessary once a controller owns ordering, and its open question about per-package ledgers is answered by bundle granularity. Phases 1 and 5 are unaffected in substance: the contract, linter, tests, immutability and revocation all carry over, and the legacy set still needs its one-time seeding.
Happy to write this up as a sibling proposal if the direction is agreeable — but it is your design above the storage layer, and we would rather extend it than fork it. |
|
Thanks, taking all findings. Fresh install is the real one. I missed that Per-instance is the one that kills Phase 3, and it kills it on its own, before any substrate question. Numbers get updated to 54, 55 and 56 merged with On substrate I want Phase 1 to stay on the ConfigMap, because it is behaviour preserving and ships in the next release, and identity is what fixes contention, backports and silent skip. Substrate is orthogonal to those three, you say this yourself in the first paragraph. Sorting your arguments as I read them. Package gating I read differently. Platform chart renders The runner as a shipped mechanism I read as separate work. Generic binary plus component supplied payload for anyone building a bundle on cozystack is a product feature with its own audience, and I think it is the actual driver behind the CRD in your comment, but it is not fixing our migrations. What we have open is contention, unsafe backports, silent skips and the Write the sibling proposal, but three questions I want answered in it. How does the controller learn migrations of the incoming release before it rolls the bundle that carries them, hook gets that list free from the render. What does the CRD actually cost, the four points in §4 are still standing and "it is a price" does not size them. And which fields need a CRD rather than a ConfigMap value. Phase 4 you did not touch at all. It is the only part that reduces how many migrations get written in the first place, and it does not care which substrate wins. |
Andrei Kvapil (kvaps)
left a comment
There was a problem hiding this comment.
LGTM. Every item from the previous round is in the document, and the revision is more honest than the review that prompted it: recording that the first draft of the rollback section overstated its case, and following the per-instance argument all the way to withdrawing Phase 3 rather than patching around it. The fresh-install seeding is in Phase 1 where it belongs, revoked no longer rewrites history, the immutability guard diffs against merge-base on any release branch, and baseline closes the growth problem.
You are right about package gating, and I was wrong. I checked the code before writing this. migration-hook.yaml is pre-upgrade,pre-install on the platform chart; the same chart renders the Package CRs, and package_reconciler.go builds the HelmReleases from them. So the hook already is a barrier ahead of every package rollout, and #3406 is covered today. A per-package gate gives less blocking, not more ordering, exactly as you put it. That was the strongest argument I had for a controller and it does not survive contact with the code.
Phase 1 stays on the ConfigMap. Agreed, and for your reason rather than as a concession: identity fixes contention, backports and silent skips, and none of the three care what holds the state. Your sorting of the rest is right too — ranFromImage and compaction are values, and jobRef/attempts need an operator creating the Job, not a CRD holding the record. mode: Invariant is the only one still standing, and I did defer it.
On your first question — how a controller learns the incoming release's migrations before rolling the bundle that carries them — the direction worth working through is reading the artifact through source-controller, which fetches it before anything is rolled. That is a shape to prove, not an answer yet, and it is the question the sibling proposal has to open with rather than assume away. Questions two and three I cannot answer better than you framed them: §4's four costs still stand unsized.
The runner as a shipped mechanism goes on the roadmap as its own item. You read it correctly — it was the real driver behind the CRD in my comment, and it was pulling the storage decision along with it. It should be argued on its own audience, not smuggled in here.
Phase 4 I skipped entirely, and that was a gap in the review. It is the only part that reduces how many migrations get written at all, and it is substrate-independent. Worth a separate pass.
Andrei Kvapil (kvaps)
left a comment
There was a problem hiding this comment.
Request changes — one blocking correction, factual rather than a redesign.
My earlier approval sat on 9368f7d; the eight commits since rewrote §3 outright, so order.d/, the seal script and batch numbers are new to me. The problem statement, the bare-slug identity and the ledger owner all hold on re-read — this is about the ordering mechanism only.
Blocking: §3's release-cut slot does not exist in the shape described
§3 has it that tags.yaml commits "on that branch", giving the batch file an existing, serialised, CI-owned slot. The commit (tags.yaml:204-205) is carried by exactly one push, in Create release branch (tags.yaml:289-308), to refs/heads/release-${GITHUB_REF#refs/tags/v} — only refs/tags/v is stripped, so tag v1.7.0-rc.1 lands on branch release-1.7.0-rc.1, which the step's own comment (tags.yaml:285-288) calls "a mutable staging branch, not an immutable artifact" and force-updates. No push in that file targets main or release-X.Y; the commit reaches a line branch only through the stable promote PR (promote-rc.yaml:336-342, :1031).
So: v1.7.0-rc.1 seals 00003 = [clickhouse-keeper-pvc-labels, vm-pool-adopt] onto release-1.7.0-rc.1. etcd-crds-precreate then merges to main. v1.7.0-rc.2, cut from main, finds no 00003-* and seals its own 00003 = [clickhouse-keeper-pvc-labels, etcd-crds-precreate, vm-pool-adopt] — inserted into the middle of a batch already shipped in rc.1. A cluster that took rc.1 runs etcd-crds-precreate last; one installing rc.2 runs it in the middle. Seal rule 5 cannot catch this: the earlier batch file is absent from the branch, not different.
Slug uniqueness is checked against disk, not manifest history
§11 deletes the script at retirement and keeps its retired line forever, so the slug goes free on disk while the ledger key it once wrote stays on every cluster that ran it. A new migration taking that name is then skipped as already-applied. One extra linter input (cat order.d/*) closes it.
Two questions
Is a total order required at all? §3 proves that if it is, only a release-assigned key can carry it — not that it is; the document states almost no pair has a declared edge and names no pair whose relative order mattered. (If §3 wants a live example in place of the hypothetical July/August pair: #3315 opened 2026-07-15, before #3379 and #3406, merged 2026-08-26, after both, and shipped as 56 while those two took 54 and 55.)
Should a failed background migration really never be retried? On the air-gapped and slow-moving clusters the document counts as real when rejecting a kubeadm-style skip ban, "the record is the report" means nobody reads it, and attempts already exists in the schema.
Fix the §3 description and the uniqueness check, answer the two as you see fit, and I expect to approve.
|
Andrei Kvapil (@kvaps) confirmed, and it does not fix with a description change. The Went the other way and removed the need for a slot. Slug uniqueness closes with the same change. History of the manifest is the tag trees now, so the check runs against those and needs no extra linter input.
Took your #3315 example in place of the hypothetical pair. One thing worth knowing since it is new: batch |
A global migration counter makes concurrent changes compete for the same slot. It cannot represent divergent release histories, so a backport can cause an unrelated migration to be skipped on a later upgrade. Migration authors also lack a shared contract for execution, failure handling and tests. These gaps make correctness depend on conventions each author has to rediscover. Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> Assisted-by: LLM
Fresh installs have no historical state to repair. Treating an empty ledger as unfinished work would run every migration on a new cluster. A package-scoped ledger key cannot distinguish tenant instances. Recording one instance as complete would suppress work for the others, repeating the failure caused by assuming a single SeaweedFS instance. Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> Assisted-by: LLM
The platform hook already precedes the Package resources that trigger component rollouts. Missing execution ordering is therefore not a reason to reject an operator-driven alternative. That alternative still needs access to the incoming migrations before deploying the bundle that contains them. The cost of introducing a CRD ledger also remains unmeasured. Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> Assisted-by: LLM
Keeping integer migrations indefinitely would require a second runner path and targetVersion forever. Contributors would have to learn both identity schemes to work on migrations. Migration counts establish the scale of the problem at the time of writing. Treating them as current inventory would create ongoing maintenance unrelated to the design. Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> Assisted-by: LLM
An authoring date, filename or checksum cannot predict which release first ships a migration. Clusters that skip releases must still agree with those that upgrade through each release. Migration identity must survive cherry-picks and moves between execution tiers. Encoding order or tier in that identity would make a scheduling change alter the cluster's record of completed work. Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> Assisted-by: LLM
Understanding the design should not require reconstructing it from five separate sections. Comparisons with other tools are useful only when they explain a constraint or demonstrate a failure relevant to this design. Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> Assisted-by: LLM
Repeated work that nobody observes can leave failures running indefinitely. Operators need a durable account of the outcome and each attempt before deciding to try again. Several consumers parse ledger entries, so a positional string would invite incompatible parsers. Manual reruns require every migration to tolerate repeated execution; a separate tier for four conditional migrations would not remove that requirement. Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> Assisted-by: LLM
A blocking failure leaves no ledger entry, so the hook Job and Flux can retry it while the upgrade remains blocked. A blanket prohibition on retries would contradict that execution path. The attempt count must describe actual executions. Counting reconciles would misrepresent how often an operator has retried a recorded failure. Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> Assisted-by: LLM
Editing a JSON ledger value by hand would create a second retry interface and risk damaging the record needed to diagnose the failure. One-off Jobs already provide an execution path for a named migration. A failed record means the intended repair has not happened. Another attempt is useful only after its cause has been addressed. The attempt count must survive the rerun so repeated failures remain visible. Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> Assisted-by: LLM
Cross-package migrations need a shared record owned by the component that assembles the platform. Moving packages between repositories does not change that responsibility, so ledger ownership must not depend on an undecided repository split. Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> Assisted-by: LLM
Release cuts write their artifacts to per-version staging branches. A manifest committed there is absent from the next cut on main, allowing a later release candidate to assign a different batch to migrations already shipped. Immutable tag trees preserve the first shipment without requiring a protected-branch write at every cut. They also retain a slug's history after its script has been retired. Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> Assisted-by: LLM
No existing migration declares an ordering requirement against a sibling. The manifest's cost therefore needs justification as a precaution, while the demonstrated skip failure is already addressed by the slug-keyed ledger. Background failures have a controller that observes and reports them. A permanent failed record can leave a transient problem unresolved for months; bounded retries allow recovery without retrying forever. Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> Assisted-by: LLM
The installer publishes the platform chart inside the packages OCI artifact. Tying manifest generation to a standalone Helm packaging step would describe a build path that the chart does not use. Both the migrations image and the packages artifact need the generated order to be present in their input tree. Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> Assisted-by: LLM
The post-conversion layout made order.d/ look like committed input, contradicting the rule that rejects it from the source tree. A contributor following that layout could otherwise add files that CI refuses. Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> Assisted-by: LLM
3100992 to
f962d16
Compare
Andrei Kvapil (kvaps)
left a comment
There was a problem hiding this comment.
LGTM as of f962d16. Deriving the order from tag trees is a better fix than the one I asked for. I checked it against the release flow: vX.Y.0-rc.1 freezes release-X.Y, later rcs and patches are cut from that branch, and only CI creates tags, so the rc.1/rc.2 case is fine.
Non-blocking, for Phase 1: what about untagged builds? The generator takes batches only from tags, and §6 plus the failure cases make a slug in no batch fatal for both the runner and the render gate. But build-main.yaml on every merge, build-release.yaml on every backport and PR e2e all build slugs that no tag has yet. As written, the first migration PR fails its own e2e, and the platform chart on main stops rendering until the next cut. Tags are already fetched there (tags.yaml:140-141, build-main.yaml:56-57), so only a rule is missing. Eg untagged slugs go into a trailing provisional batch, and a tagged build never produces one because it always finds its own tag.
One more question for the same phase. Batch numbers are line-local now, and every minor upgrade crosses lines. What reads complete-through? If it is always recomputed against the incoming manifest, fine. If a stored value is ever compared with the new line's numbering, a 1.6 watermark can cover a 1.7 batch the cluster never ran. I should have asked this in the first round.
Two review questions on the ordering design, answered together. Builds no tag has sealed yet (main, backports, PR e2e) carry slugs the tag walk cannot place, and the fatal "slug in no batch" rule would fail them. The generator now puts those slugs in a trailing order.d/99999-untagged and refuses to emit it for a checkout that is itself a tag, so a broken tag walk cannot ship. Batch numbers are per line, so a stored complete-through compared with another line's numbering could cover a batch the cluster never ran. The watermark had no reader beyond compaction, and compaction removed keys of slugs that still ship. Both go: keys are never deleted, the ledger records the manifest file name in batch as information only, and the retention floor check becomes a per-slug test that every retired slug has a ledger key. Fresh-install seeding covers retired entries so that check passes on a new cluster. Also aligns the state names on skipped-fresh-install and failed. Assisted-By: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Review of the previous commit found holes in what it relied on. The legacy seeding ran only for a ledger with no m.* keys, so a cluster that had already run slug migrations was never seeded and would re-run all converted slugs. It is now per key and runs before the floor check. The floor check exempts revoked slugs, which the slug pass would have recorded only after it. legacy-map slugs are assigned to batch 00000 and left out of the tag walk and the untagged file, so no slug lands in two files. The guard on a checkout that is itself a tag could never fire, since a tag's own tree carries its slugs. It is replaced by failing on a shallow clone and on a missing previous tag. The tagged build is tags.yaml, not build-release.yaml, and stable tags are cut at the promote merge commit. The batch examples now come from one line, and the remaining state names and the shebang test wording follow the rest of the document. Assisted-By: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
|
Andrei Kvapil (@kvaps) Thanks, both are in.
While there I also fixed legacy seeding (it was skipped for clusters that already have |
This PR adds design proposal for platform migrations engine.
Current implementation uses one dense integer as both migration id and cluster state. Every new migration takes next number, so concurrent PRs fight for same slot - #3406, #3379 and #3315 all add
migrations/54right now and two of them will renumber on merge. Scalar high water mark cant describe branched history, so migrations are not backportable, #3534 is realised case of that. Pending set is also computed fromtargetVersionliving in another file than migrations, that is whyhack/check-migrations-target.shexists at all.Proposal replaces integer with
YYYYMMDD-slugid and applied set ledger in the same configmap, splits execution to blocking pre-apply tier and background tier run by cozystack-operator, and adds contract for writing migrations: declared metadata header, one shell dialect, linter and mandatory tests.Please look at open questions at the end, especially per package ledger vs single one when packages start splitting out.
Summary by CodeRabbit