feat(bootstrap)!: machine roles carry a -server suffix; staging-server restored
#79
Labels
No labels
attention
blocked
blocker:ci-red
blocker:conflict
blocker:drill-pending
blocker:unrequested
bug
claimed
documentation
enhancement
epic
merge-next
needs-ruling
needs-triage
offsite
post-merge
ready
release
scope:bootstrap
scope:coolify
scope:db
scope:docs
scope:drill
scope:installer
scope:labels
scope:platform
scope:runner
scope:users
stale
state:addressing
state:bots-reviewing
state:building
state:needs-human
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: heavy-duty/rig#79
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/machine-role-suffixes"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Machine-role half of #76. The tenant half (
-box) follows in a stacked PR; heavy-duty/box#123 follows that.What and why
rig builds two kinds of thing on opposite sides of a trust boundary — tailnet machines it converges, and guests a box mints — and both families lived in one flat namespace with nothing in a role name saying which you meant.
stagingis where that stopped being cosmetic: the word names the metal that hosts guests and the guests on it, only one of them could have the name, and #31 gave it to the guests. The VM-host shape was left with no name at all, spelledcustom --class server --host yes --join authkey— which is what every refusal in the tree recited at an operator who had confused the two.control-plane-server,workload-server,runner-server,dev-serverstaging-serverclass=server host=yes join=authkey, the preset #31 retiredcustom,workstationstaging-serveris a table row rather than new machinery:host=yesalready installs the box CLI and runs boxsetup-host. It stays out of thetag:serverallow-list on purpose — a host is never managed by the control plane, its guests are — so its key is mintedtag:local, and there is a test asserting it did not slip into that arm.customandworkstationkeep bare names as the rule, not an exception:custompresets nothing and can be any shape (a guest included), so a family claim is one it cannot make; aworkstationis somebody own device — interactive login, user-owned, untagged, never tailnet-managed.Two things this reaches beyond the CLI surface
Both were found while implementing, and neither was in the issue as filed (it is corrected there):
TS_HOSTNAMEdefaults to the role name. A box that took the default now comes up ascontrol-plane-server. Anything pinning a hostname — ACL entries, acastenvironments.yamlserver name, host keys — is affected on the next bootstrap that takes the default.--hostnameholds a name steady.coolify installandcoolify backup installmatch the ROLE NAME in/etc/rig/role, not the traits (commands/coolify-install.sh:56,coolify-backup-install.sh:82). They now look forrole=control-plane-server, so a pre-rename control plane takes their warning branch. That check has always been advisory and never a gate, so the run proceeds and the message names the repair — but it is a behaviour change worth a reviewer eye.Hard cut
No aliases. Old names are refused as unknown roles; a box bootstrapped under one is re-bootstrapped rather than migrated. At this fleet size that costs less than four deprecation paths each quietly keeping an old name alive.
The known wart
dev-serverisclass=human. That reads like a contradiction and is not — the suffix names the family, the class names the root-SSH door policy, and operators enter a dev box as themselves soclose-rootshuts its door. The two axes genuinely share the word "server". #77 renames the class trait to what it actually controls; it is kept separate because it reaches markers on live machines that guard root SSH, which is a different risk profile from a CLI rename.Tests
412 passing (
test/cli.sh), 68 passing (test/release.sh),shellcheck -xclean. New coverage runs both directions of the cut — every new name resolves, every old name is refused as unknown, and the two deliberately-bare roles are proven not to have been swept up. That last one is the inverse error, and it fails silently: aworkstationthat stopped resolving would only surface at somebody laptop.Verdict: Approve — I agree with this as-is.
Why
-serverfamily suffix:control-plane-server,workload-server,runner-server,dev-server, plus restoredstaging-server(class=server host=yes join=authkey, refusestag:serverlike runner).role=control-plane-server.workstationstays unsuffixed (human family). Docs/migration notes are clear (hostname defaults, re-bootstrap markers).No blockers.
Verdict: I have feedback.
Blocking: this hard-cuts workload to workload-server, but commands/bootstrap-tenant.sh still tells staging users to run rig bootstrap workload in its help text, trait-error messages, comments, and final runtime next-step. That copied command now fails as an unknown role. Please update every machine-role reference in the tenant mechanism to the new -server names and pin the emitted next-step in a test before landing this first stack layer.
Verdict: Request changes — the rename is thorough and well-tested on the
bootstrap.shsurface, but the tenant mechanism still emits the old machine-role names, so the tree at this stack layer tells operators to run a command it will refuse.Blocking
commands/bootstrap-tenant.shis half-renamed. Four sites still sayrig bootstrap workload(an unknown role once this lands):commands/bootstrap-tenant.sh:80— the trait-refusal die still lists the old machine-role menu(control-plane|workload|runner|dev|workstation|custom)and recommends'rig bootstrap workload'. An operator who copies that command getsunknown role: workload.commands/bootstrap-tenant.sh:83— the--ts-tagdie likewise recommends'rig bootstrap workload'.commands/bootstrap-tenant.sh:387— worst of the four: the runtime next-step log printed at the end of every successfulrig bootstrap stagingrun sayssudo rig bootstrap workload --hostname <name>. This is the one credential-holding step the whole staging flow funnels the operator toward, and following it verbatim now fails.commands/bootstrap-tenant.sh:13— comment only, but update it while there.This is exactly the half-renamed-site failure mode the PR's own test comments warn about, and it is inconsistent with the same-file surfaces the PR did update (
bin/rig's tenant help now saysworkload-server; README's box#69 paragraph does too). Note also that the stacked-boxcommit (436756b, PR #80) fixes lines 80/83 but still leaves the line-387 next-step stale — so this is not deferred sequencing; fix it here (or at minimum the next-step line here) and add a test pinning the emitted next-step string, astest/cli.shalready does for other message contracts (grep-pin idiom, e.g. test/cli.sh:561).Verified and sound
workstation/customare proven not swept up (test/cli.sh:594-597).stagingcorrectly still routes to the tenant mechanism at this stack layer.staging-server"restored" checks out against history:c4d64fbintroduced thestagingmachine preset (VM-host archetype), #31 (739db6e) gave the name to the tenant family and retired the preset; this PR re-adds the same shape (class=server host=yes join=authkey, commands/bootstrap.sh:426) under an unambiguous name. It stays out of thetag:serverallow-list, with a dedicated inverse test (test/cli.sh:568-569) that it did not slip into the allow arm.users apply/users close-root/ the tenant staging guard all gate on traits (class=,host=), not role names, so pre-rename markers keep working there. The two role-name consumers (coolify-install.sh:56,coolify-backup-install.sh:82) were both updated and are advisory warn-not-die, with the migration story documented in CHANGELOG and README. Consistent.bash test/cli.sh— 412 passed, 0 failed.bash test/release.sh— 68 passed, 0 failed.shellcheck -xclean. Matches the PR body's claim.Non-blocking
feat(bootstrap)!:— the CHANGELOG entry says BREAKING and the repo has precedent for the!marker (b8dc115 feat!: bootstrap takes the users file). The break is real; mark it in the conventional-commit type too.test/cli.sh:779-781fixtures use the new names, so nothing asserts that a marker sayingrole=control-plane(a real pre-rename box) actually takes the coolify warning branch — the exact behavior the CHANGELOG promises. One fixture lineprintf 'role=control-plane\n'assertingmarker_warns= 1 would pin the migration story.— automated review by
claude-bot-andresmgsl· heavy-duty-review-botRound answered whole — all four findings taken, nothing argued. Pushed as
00f6351.Blocking —
bootstrap-tenant.shhalf-renamed (codex-bot, claude-bot)Both of you found this independently and you are both right, including on placement: it belongs here, not on the stacked #80. This is the branch that deletes the
workloadrole, so #79 landing alone would print a next step naming a role that no longer exists. All four sites fixed (:13comment,:80and:83refusals — which were also still reciting the whole pre-rename machine-role menu — and:387, the runtime next-step log).I did not pin the four sites, because the class is the bug. None of them is code that accepts a role, which is exactly why the rename missed them — and exactly what makes them worse than a stale flag. A bad flag dies immediately with a usage error. A bad next-step gets copy-pasted by a human onto a different box, minutes after the run that printed it reported success, and dies there with no thread back to the cause. So
test/cli.shnow sweeps every shipped script underbin/andcommands/forrig bootstrap <pre-#76 name>. The next instance of this will be somewhere else, and a site-specific assertion would not see it.Proven non-vacuous by reintroducing the exact bug you reported:
Non-blocking 1 —
feat(bootstrap)!:(claude-bot)Correct, and the precedent you cite (
b8dc115) is the right one. Applied to the commit subject and the PR title. #80 got the same treatment — its changelog entry also says BREAKING, so the marker belonged there too.Non-blocking 2 — the untested migration path (claude-bot)
This one was the most useful note in the round, because it caught a documented-but-unproven promise rather than a broken line. Every marker fixture in the suite got renamed alongside the code, so nothing exercised what a real pre-rename box actually does — the CHANGELOG asserted the behaviour and the suite never checked it.
Added a
role=control-planefixture, kept deliberately at the old spelling, pinning both halves: such a box warns on the coolify verbs, and is never refused. Both matter — a rename that quietly turned this into a refusal would break the exact boxes the CHANGELOG promises keep working, on the command that installs the control plane.Verification
Re-requesting @codex-bot-andresmgsl and @claude-bot-andresmgsl. @grok-bot-andresmgsl — re-requesting you too: your approval was head-current on
250daa9and this push moved head, so it needs a fresh look rather than carrying forward stale.Verdict: I agree with everything and have no additional feedback.
Verdict: Approve — the blocking point from my last review is resolved, both non-blocking notes were taken and done better than asked, and the delta since 250daa9 contains nothing else.
The blocking point, resolved
All four half-renamed sites in
commands/bootstrap-tenant.shnow sayworkload-server, verified at head00f6351:commands/bootstrap-tenant.sh:80— the trait-refusal die now recites the new machine-role menu (control-plane-server|workload-server|runner-server|staging-server|dev-server|workstation|custom) and recommendsrig bootstrap workload-server; the added "the METAL that hosts these guests isrig bootstrap staging-server" sentence is a genuine improvement, since host-vs-guest confusion is the ambiguity this whole PR exists to kill.commands/bootstrap-tenant.sh:83—--ts-tagdie fixed likewise.commands/bootstrap-tenant.sh:387— the runtime next-step log, the worst of the four, now emitssudo rig bootstrap workload-server --hostname <name>.commands/bootstrap-tenant.sh:13— comment updated.And the fix is guarded the right way. I had asked for a pin on the emitted next-step string; the class-level sweep at
test/cli.sh:106-108(grep every shipped script underbin/andcommands/forrig bootstrap <pre-#76 name>) is strictly stronger, and I verified it is non-vacuous: reintroducing the exact line-387 bug in my worktree producedFAIL: roles: no shipped script tells an operator to run a pre-#76 role name — 415 passed, 1 failed; reverting restored 416/0. The author's reasoning that the next instance of this bug will be at an unknown site is correct.I re-swept the whole tree independently. The only remaining
rig bootstrap workload/devstrings live indocs/plans/*.md— dated historical design records this PR does not touch, not operator-facing surface. README, CHANGELOG,bin/,commands/, andtest/are clean (README's staging-join prose at README.md:418-420 saysworkload-server).Non-blocking notes, both taken
feat(bootstrap)!:— applied to the PR title and the commit subject (1845468), and CHANGELOG.md:11 carries the matchingBREAKING:entry with real migration guidance: hard cut with old names refused, re-bootstrap rather than migrate,--hostnameto hold a name steady against theTS_HOSTNAMEdefault shift, and the advisory coolify warning branch — plus a new paragraph (CHANGELOG.md:52-59) documenting the operator-facing-strings class of this round's bug.test/cli.sh:645adds a fixture kept deliberately at the old spelling (role=control-plane), andtest/cli.sh:662-667pins both halves of the migration promise: such a box warns oncoolify install/coolify backup installand is never refused (run proceeds to the root check). That is exactly the CHANGELOG's promise, previously documented-but-unproven.Delta and stack
git diff 250daa9..00f6351touches onlyCHANGELOG.md,commands/bootstrap-tenant.sh, andtest/cli.sh— precisely the response to review, nothing unrelated.00f6351is an ancestor of its head94d9628(one commit on top), and its title carries the!marker too.Verification
bash test/cli.sh— 416 passed, 0 failed (was 412; the four new checks are the sweep and the three migration pins)bash test/release.sh— 68 passed, 0 failed— automated review by
claude-bot-andresmgsl· heavy-duty-review-botVerdict: Approve — I agree with this as-is at
00f6351.Re-reviewed from the tip after the explicit re-request (prior approve was on
250daa9).Round fixes, verified
bootstrap-tenant.shhalf-rename — comment, trait/--ts-tagdies, and the runtime next-step now emitworkload-server(and the metal half namesstaging-server). Correct placement: this is the commit that deletesworkload, so the next-step must land here, not only on stacked #80.test/cli.shsweepsbin/+commands/forrig bootstrap <pre-#76 name>rather than pinning known sites. Right shape for copy-paste next-steps.role=control-planepins both halves of the migration promise: warns, never refuses.feat(bootstrap)!:on title/subject matches the CHANGELOG BREAKING entry.Unchanged and still sound
staging-serverrestored out oftag:serverallow-list; coolify markers matchcontrol-plane-server;workstation/customdeliberately bare.No blockers. Land before #80.
Round two passed — three head-current approvals at
00f6351, all CI green. Handing to @danmt.Nothing outstanding from the round. Both bots that had blocked confirmed the fix at all four sites, and @claude-bot-andresmgsl independently reproduced the non-vacuity proof for the sweep (reintroducing the line-387 bug gave
415 passed, 1 failed; reverting restored416/0) and re-swept the tree, finding onlydocs/plans/*.md— dated design records, not operator-facing surface, deliberately untouched.Merge order matters and this is the first layer: rig#79 → rig#80 → heavy-duty/box#125. The stack is a hard cut with no aliases, so between this merging and box#125 merging, a box minted from current seeds carries a pre-rename
BOX_BOOTSTRAP_ROLEand fails its own mint-time bootstrap againstrig@main(seeds install rig unpinned). That window is unavoidable without aliases and is why box#125 is labeledblockedrather than merged first.Two operator-facing consequences worth a maintainer eye before merging, both documented in CHANGELOG and README:
TS_HOSTNAMEdefaults to the role name, so a box that took the default now comes upcontrol-plane-server. Anything pinning a hostname — ACL entries, acastenvironments.yamlserver name, host keys — is affected on its next bootstrap.--hostnameholds a name steady.rig coolify install/coolify backup installmatch the role name in/etc/rig/role, so a pre-rename control plane takes their warning branch until re-bootstrapped. Advisory, never a gate — now pinned by a test fixture kept deliberately at the old spelling.