feat(storage): add declarative disk layout reconciliation and kcoreOS…#14
Conversation
… branding UX Introduce end-to-end disk layout reconciliation across controller, node-agent, CLI, and docs while rebranding login/install surfaces to kcoreOS with live hardware summaries so operators get consistent day-0/day-2 workflows.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 37 minutes and 59 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR introduces disk layout management functionality across the controller and node-agent, adding a new shared type crate ( Changes
Sequence Diagram(s)sequenceDiagram
participant Operator
participant kctl as kctl CLI
participant Controller
participant Database as DB
participant Reconciler as Background<br/>Reconciler
participant NodeAgent as Node-Agent
participant lsblk as System<br/>(lsblk)
Operator->>kctl: apply -f disk-layout.yaml
kctl->>Controller: CreateDiskLayout(name, nodeId, layoutNix)
activate Controller
Controller->>Database: upsert_disk_layout(...)
Controller->>Database: reset status to pending
Controller-->>kctl: response
deactivate Controller
loop Every 15 seconds
Reconciler->>Database: list_disk_layouts_needing_reconcile()
Database-->>Reconciler: layouts with observed < generation
Reconciler->>Controller: lookup node
Reconciler->>NodeAgent: apply_disk_layout(layoutNix, apply=true, rebuild=true)
activate NodeAgent
NodeAgent->>lsblk: snapshot block devices
lsblk-->>NodeAgent: LsblkSnapshot
NodeAgent->>NodeAgent: classify_disk_layout(targets, snapshot)
alt Safe
NodeAgent->>NodeAgent: stage to /etc/kcore/disk/temp.nix
NodeAgent->>NodeAgent: disko --mode format,mount
NodeAgent->>NodeAgent: promote to /etc/kcore/disk/current.nix
NodeAgent->>NodeAgent: nixos-rebuild test+switch
NodeAgent-->>Reconciler: success, phase=applied
else Dangerous
NodeAgent-->>Reconciler: refusal, phase=refused
else Error
NodeAgent-->>Reconciler: error, phase=failed
end
deactivate NodeAgent
Reconciler->>Database: upsert_disk_layout_status(status with observed_generation)
end
Operator->>kctl: get disk-layout <name>
kctl->>Controller: GetDiskLayout(name)
Controller->>Database: get_disk_layout + get_disk_layout_status
Controller-->>kctl: DiskLayout + status (phase, refusal_reason)
kctl-->>Operator: formatted output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
docs/storage.md (3)
114-146:⚠️ Potential issue | 🟠 MajorDuplicate section headers: old "Disko Ownership Modes" not removed when the new section was added.
Lines 114 and 121 are both H2 headings about the same topic ("Disk management modes (safe installer/controller split)" and "Disko Ownership Modes (safe installer/controller split)"). The old section at line 121 reads like the pre-rename copy that was left in place when the new section was prepended; the result is two near-duplicate H2s with overlapping content. Please delete the old one so there is a single source of truth for the ownership model.
📝 Proposed cleanup
## Disk management modes (safe installer/controller split) > Naming note. The user-facing surface is **disk** (`DiskLayout`, > `kctl ... disk-layout`, `/etc/kcore/disk-management-mode`). Disko is still > the underlying tool that performs the partitioning; we just don't lead with > the brand name on operator-facing surfaces. -## Disko Ownership Modes (safe installer/controller split) - kcore uses an explicit ownership split for disk partitioning state: - `installer-only` (default): install-time layout is authoritative; controller / `kctl` day-2 disk apply is blocked. - `controller-managed`: controller / `kctl` day-2 disk apply is allowed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/storage.md` around lines 114 - 146, Remove the duplicate H2 block titled "Disko Ownership Modes (safe installer/controller split)" (the pre-rename copy) so only the new H2 "Disk management modes (safe installer/controller split)" remains; locate the repeated section content under the heading "Disko Ownership Modes (safe installer/controller split)" and delete that entire block (including its duplicated bullets and subsections like "Promote a node..." and "Apply mode safety gate") leaving the single, canonical ownership description under "Disk management modes (safe installer/controller split)".
414-421:⚠️ Potential issue | 🟡 MinorStale
apply-disko/apply_disko_layoutreferences in the file map.The file map still advertises
node apply-disko -f <file> [--apply] [--timeout-seconds N]as the current CLI surface (line 414) and sayscrates/kctl/src/commands/node.rs"Implements day-2 disko RPC client call viaapply_disko_layout" (line 421). Per the rest of this PR (and the node command renamed toApplyDiskincrates/kctl/src/main.rs) the primary command isnode apply-diskusingapply_disk_layout;apply-diskois a one-release alias only. Worth flipping these to the new names so the reference doc doesn't mint new callers of the deprecated surface.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/storage.md` around lines 414 - 421, Update the stale CLI and symbol names: replace the deprecated "apply-disko" and apply_disko_layout references with the new "apply-disk" command and apply_disk_layout symbol, and update any mention of the node command alias to the canonical ApplyDisk entry; specifically edit the docs section that lists the CLI usage and the line that says it "Implements day-2 disko RPC client call via apply_disko_layout" so they instead reference "node apply-disk -f <file> [--apply] [--timeout-seconds N]" and apply_disk_layout / ApplyDisk where applicable.
193-240:⚠️ Potential issue | 🟠 MajorSection numbering is broken after the Day-2 disk workflows rewrite.
After the new "## Day-2 disk workflows" (line 148) with sub-sections
### A)and### B), the pre-existing subsections continue with### 3) Apply with bounded timeout(193) and### 4) Reconcile mounts/services(204), and then much later### 2) Create VM with required storage metadata(228) appears. The reader sees A → B → 3 → 4 → 2, which reads like a rebase artifact. Additionally, the### 3)Apply section now overlaps with workflow B above it. Please renumber / re-nest these so the flow is monotonic (or convert the remaining numbered subsections to descriptive headings).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/storage.md` around lines 193 - 240, The subsection numbering after the "Day-2 disk workflows" block is inconsistent (you have "### A)" / "### B)" followed by "### 3) Apply with bounded timeout", "### 4) Reconcile mounts/services" and then "### 2) Create VM with required storage metadata"); update the headings so the flow is monotonic and non-overlapping by either renumbering the numeric headings to follow A/B (e.g., "### 1) …", "### 2) …", "### 3) …") or convert all numbered subsections to descriptive headings (remove numeric prefixes) to avoid rebase artifacts; specifically edit the headings titled "Day-2 disk workflows", "A)", "B)", "Apply with bounded timeout", "Reconcile mounts/services", and "Create VM with required storage metadata" so their sequence is logical and the "Apply with bounded timeout" section no longer duplicates or overlaps the content of workflow B.
🧹 Nitpick comments (7)
examples/cluster/prod.yaml (1)
5-5: Avoid hardcoding a private IP for a prod controller endpoint.Prefer a stable DNS name (for example,
controller.prod.example.com:9090) so endpoint rotation/failover doesn’t require manifest rewrites.Suggested change
spec: - controller: 192.168.40.107:9090 + controller: controller.prod.example.com:9090🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/cluster/prod.yaml` at line 5, The manifest hardcodes a private IP in the controller field; update the examples/cluster/prod.yaml so the controller entry uses a stable DNS name or resolvable service address (e.g., controller.prod.example.com:9090) or reference an environment/config variable rather than 192.168.40.107:9090; modify the controller: value in prod.yaml to a DNS-based endpoint (or templated/config reference) to enable endpoint rotation and failover without editing the manifest.crates/kcore-disko-types/Cargo.toml (1)
1-16: Naming nit: internal crate keeps-disko-while surfaces rebrand to-disk-.The package description is clear that this is an internal/implementation-side crate (the node still lowers to disko), and aligns with the explicit policy comment added in
proto/controller.protothat "the operator surface uses the plain word 'disk'." Leavingkcore-disko-typesnamed after the implementation tool is fine and actually makes the abstraction boundary explicit. Flagging only so that if disko is ever swapped out, you renameable this crate is on the radar.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/kcore-disko-types/Cargo.toml` around lines 1 - 16, The crate name "kcore-disko-types" should remain as the internal implementation identifier but document the rationale and future-rename consideration: add a short comment in Cargo.toml adjacent to package.name (name = "kcore-disko-types") stating this crate is intentionally named after the implementation ("disko") while the external operator surface uses "disk", and note that the name can be changed if disko is swapped out; this clarifying comment will make the abstraction boundary explicit for future maintainers.modules/kcore-disko.nix (1)
183-192: Reconsider the design to avoid mutable on-disk state during NixOS evaluation.The primary concern is not
types.pathitself (it doesn't automatically copy to/nix/storewithout explicit string coercion), but rather the architectural issue: using a mutable file at/etc/kcore/disk/current.nixas input during module evaluation creates non-determinism. Between twonixos-rebuildruns from the same git commit, the toplevels can differ based on what the node-agent wrote to disk in the interim.Either parse the persisted layout before evaluation (e.g., read and pass as a string/attr in a separate step), store it in an immutable location, or better yet, read the layout as a string and parse it deterministically without depending on filesystem state at eval time.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/kcore-disko.nix` around lines 183 - 192, The module currently exposes persistedLayoutPath (type = types.path, default = /etc/kcore/disk/current.nix) which lets evaluation depend on mutable on-disk state; change the design so Nix evaluation is deterministic by removing reliance on that mutable file: replace persistedLayoutPath with an option that accepts the persisted layout as an immutable string/attr (e.g., persistedLayoutContent: type = types.str or types.attrs) or accept a store-backed path produced before evaluation, and update the logic that merges `disko.devices` (and the managementMode = "controller-managed" branch) to parse/merge from the provided immutable input instead of reading /etc/kcore/disk/current.nix at eval time. Ensure functions referencing persistedLayoutPath are updated to use the new persistedLayoutContent/store-backed input.crates/node-agent/src/grpc/admin.rs (1)
1190-1208: Legacyapply_disko_layoutshim: classifier refusal reason is swallowed.The translated response drops
refusal_reasonwhen mappingApplyDiskLayoutResponseback toApplyDiskoLayoutResponse(the old proto doesn't carry the field). That's fine structurally, but it means a legacykctl node apply-diskocaller that hits a classifier refusal will seesuccess=falsewith the human-readablemessagebut no stable machine-readable code — the opposite of the refusal-reason-first UX the new flow promises. Suggest prefixingmessagewith the refusal code for legacy callers so downstream parsers can still key off it during the one-release deprecation window:let resp = apply_disk_layout_impl(translated).await?.into_inner(); + let message = if resp.refusal_reason.is_empty() { + resp.message + } else { + format!("[{}] {}", resp.refusal_reason, resp.message) + }; Ok(Response::new(proto::ApplyDiskoLayoutResponse { success: resp.success, - message: resp.message, + message, mode: resp.mode, }))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/node-agent/src/grpc/admin.rs` around lines 1190 - 1208, The legacy shim in apply_disko_layout currently drops resp.refusal_reason when mapping the new ApplyDiskLayoutResponse back to the old ApplyDiskoLayoutResponse; update apply_disko_layout (and the mapping that builds proto::ApplyDiskoLayoutResponse from resp returned by apply_disk_layout_impl) to detect a non-empty resp.refusal_reason and prefix the legacy message with that code (e.g., "<refusal_reason>: <message>") so legacy callers get a stable machine-readable token while preserving the human message. Ensure you reference resp.refusal_reason and resp.message from the response of apply_disk_layout_impl and only prefix when refusal_reason is present.crates/kctl/src/commands/disk_layout.rs (1)
52-104:diff_from_fileshould mirror thedisko.devicescheckapply_from_fileperforms.
apply_from_filefails fast with a clear local error iflayout_nixdoesn't containdisko.devices(line 55–60), butdiff_from_fileskips that check and sends the payload straight to the controller's classifier. A manifest that forgot thedisko.devicesattrset will now surface as a generic classifier refusal (or server-side invalid-argument) instead of the same actionable message theapplypath gives. Cheap to align:♻️ Align local validation between diff and apply
pub async fn diff_from_file(info: &ConnectionInfo, file: &str) -> Result<()> { let manifest = parse_manifest(file)?; let layout_nix = resolve_layout_nix(file, &manifest.spec)?; + if !layout_nix.contains("disko.devices") { + bail!( + "disk layout '{}' must define disko.devices in its layoutNix body", + manifest.metadata.name + ); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/kctl/src/commands/disk_layout.rs` around lines 52 - 104, The diff path misses the local validation for the layout_nix body that apply_from_file performs: after obtaining layout_nix in diff_from_file (using parse_manifest and resolve_layout_nix), check if layout_nix.contains("disko.devices") and bail with the same error message used in apply_from_file (referencing manifest.metadata.name) if the check fails; this keeps diff_from_file, apply_from_file, parse_manifest and resolve_layout_nix aligned and avoids sending an invalid payload to classify_disk_layout.crates/controller/src/db.rs (2)
2108-2132:observed_generationis overwritten unconditionally — consider making it monotonic.Unlike
upsert_replication_ack(Line 869) which usesMAX(replication_ack.last_event_id, excluded.last_event_id), this upsert letsobserved_generationregress. Per the reconciler's error path (crates/controller/src/disk_reconciler.rsLines 51–56 in the relevant snippet), a transient RPC failure writesobserved_generation: 0while updatingphase/message. That erases the "we know the disk is at generation N" knowledge from a prior successful apply until the next tick re-reconciles.Functionally the next tick recovers (the row re-appears in
list_disk_layouts_needing_reconcile), so this is a minor information-loss / wasted-work concern rather than a correctness bug, but tightening it here lets the reconciler freely update phase/message on failure without trampling progress.🔧 Proposed fix: take the max on conflict
"INSERT INTO disk_layout_status ( name, observed_generation, phase, refusal_reason, message, last_transition_at ) VALUES (?1, ?2, ?3, ?4, ?5, datetime('now')) ON CONFLICT(name) DO UPDATE SET - observed_generation = excluded.observed_generation, + observed_generation = MAX(disk_layout_status.observed_generation, excluded.observed_generation), phase = excluded.phase, refusal_reason = excluded.refusal_reason, message = excluded.message, last_transition_at = datetime('now')",Note: if adopted, the reconciler's error path should be reviewed — writing
observed_generation: 0becomes a no-op against a MAX clause, which is the desired behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/controller/src/db.rs` around lines 2108 - 2132, The upsert_disk_layout_status function currently overwrites disk_layout_status.observed_generation unconditionally; change the ON CONFLICT update to preserve monotonic progress by setting observed_generation = MAX(disk_layout_status.observed_generation, excluded.observed_generation) (i.e., use the MAX of the existing row and excluded value) while keeping the rest of the SET clauses the same so that phase/refusal_reason/message/last_transition_at can still update; update the SQL string in upsert_disk_layout_status to use the MAX expression referencing disk_layout_status.observed_generation and excluded.observed_generation.
2006-2041:upsert_disk_layoutuses INSERT + separate SELECT; consider using RETURNING if SQLite version permits.The current pattern requires two round-trips. While
INSERT ... ON CONFLICT ... DO UPDATE ... RETURNINGwould fold this into one statement, the bundled SQLite (3.30.1) does not support RETURNING (added in 3.35.0). If SQLite is upgraded in the future, this optimization becomes viable.Regarding generation: the gRPC caller safely prevents regression (Unchanged keeps generation, Updated calls
saturating_add(1), Created sets to 1). However, the replication caller accepts generation directly from the request body:body.get("generation").unwrap_or(1). This allows callers to pass arbitrary lower values, which would skip reconciliation sinceobserved_generation < generationwould fail. Consider guarding the UPDATE withMAX(disk_layouts.generation, excluded.generation)to make the DB layer self-consistent and prevent accidental reconciliation skips, even if the caller passes a stale generation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/controller/src/db.rs` around lines 2006 - 2041, upsert_disk_layout currently does INSERT ... ON CONFLICT ... DO UPDATE that blindly sets generation = excluded.generation, which allows callers to regress generation; modify the conflict UPDATE clause in the disk_layouts upsert to set generation = MAX(disk_layouts.generation, excluded.generation) (keep node_id/layout_nix/update_at as-is) so the DB enforces non-decreasing generation; also, once the project upgrades to SQLite >= 3.35.0 you can simplify upsert_disk_layout by using a single INSERT ... ON CONFLICT ... DO UPDATE ... RETURNING to avoid the separate SELECT.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/controller/src/disk_reconciler.rs`:
- Around line 110-124: The code currently writes row.generation into
DiskLayoutStatusRow.observed_generation for all outcomes, which causes
refused/failed applies to be considered observed and stops retries; update the
db.upsert_disk_layout_status call so that if resp.success is true set
observed_generation to row.generation, otherwise keep the existing
observed_generation (e.g. row.observed_generation) so
list_disk_layouts_needing_reconcile will continue to retry on refused/failed
applies; adjust the DiskLayoutStatusRow construction in the
db.upsert_disk_layout_status call accordingly (use resp.success to choose
between row.generation and row.observed_generation) and leave
phase/refusal_reason/message handling unchanged.
- Around line 104-108: Wrap the admin.apply_disk_layout(Request::new(req)).await
call in tokio::time::timeout using Duration::from_secs(NODE_RPC_TIMEOUT_SECS +
30) so the client future cannot stall indefinitely; call timeout(...).await, map
a timeout error into a clear Err (e.g. format!("apply_disk_layout rpc timed
out")) and preserve other RPC errors (map_err for RPC failure and .into_inner()
on success) so the variable resp is only set when the timed call completes
successfully. Ensure you reference the same expression
(admin.apply_disk_layout(Request::new(req))) and handle the Result from timeout
appropriately to avoid blocking disk layout reconciliation.
In `@crates/controller/src/grpc/controller.rs`:
- Around line 1099-1123: The current code calls db.upsert_disk_layout(...) then
db.upsert_disk_layout_status(...), which can leave the DB in a partially-applied
state if the status write fails; change this so both writes are performed
atomically: add a new transactional helper on self.db (or use an existing
transaction API) that performs upsert_disk_layout(Row) and
upsert_disk_layout_status(DiskLayoutStatusRow) inside one transaction, or catch
a failure of upsert_disk_layout_status and roll back the prior
upsert_disk_layout; update the call site to use this new helper so the
DiskLayoutRow and DiskLayoutStatusRow changes are committed or rolled back
together.
- Around line 4959-4997: GetComplianceReport's access_control list is missing
the new DiskLayout RPCs that require CN_KCTL/CN_CONTROLLER_PREFIX; update the
compliance ACL report to include entries for create_disk_layout,
get_disk_layout, list_disk_layouts, delete_disk_layout and classify_disk_layout
(each annotated with the enforced CN_KCTL and CN_CONTROLLER_PREFIX) so the
access_control output reflects the actual enforcement in those RPC handlers.
In `@crates/controller/src/replication.rs`:
- Around line 1582-1598: The disk_layout.create handler can hit a FK violation
because nodes may not be materialized yet; before calling db.upsert_disk_layout
in the "disk_layout.create" match arm, call ensure_replicated_node_exists with
the node_id extracted via required_str("nodeId", ...) (same pattern used in
vm.create and network.create) to ensure the node record exists, then proceed to
build the DiskLayoutRow and call db.upsert_disk_layout; handle and propagate
errors from ensure_replicated_node_exists similarly to the other handlers so
replication becomes order-independent.
In `@crates/kcore-disko-types/src/lib.rs`:
- Around line 23-29: The classifier currently only flags a few exact system
mounts but lets generic mounted filesystems pass as Safe; update the decision
logic that emits MOUNTED_KCORE_VOLUME / MOUNTED_SYSTEM_PARTITION / ACTIVE_LVM_PV
/ ACTIVE_ZPOOL_MEMBER so that any active mountpoint on the target device is
treated as dangerous: detect whether the target path is a mountpoint (or has any
active mount entries) and if so return the same unsafe result (e.g.,
MOUNTED_SYSTEM_PARTITION or a new ACTIVE_MOUNT constant) instead of Safe; locate
the code that checks mount/state (the classifier function that previously used
MOUNTED_SYSTEM_PARTITION and related constants) and broaden the condition to
reject generic mounts like /var, /home, /srv, /mnt (i.e., any non-root active
mount) rather than only the small exact list.
- Around line 191-199: The loops that skip only spaces and tabs around the '='
need to accept all ASCII whitespace (including newlines) so Nix formatted like
`device\n=\n"/dev/sda";` is handled; replace the byte comparisons (bytes[j] ==
b' ' || bytes[j] == b'\t') in both whitespace-skipping while loops with
bytes[j].is_ascii_whitespace() (or the equivalent ASCII-whitespace check) so the
parser will skip newlines and other ASCII whitespace when scanning around the
'='; keep the rest of the logic (incrementing j and checking for '=') unchanged
and update both occurrences that use the local variables j, bytes, and key.
- Around line 87-99: Track which requested targets actually matched the snapshot
and refuse when any requested target had no match: build target_set as before,
add a mutable matched set, and whenever is_within_targets(&dev.path,
dev.parent_path.as_deref(), &target_set, snapshot) returns true mark the
corresponding target(s) as matched (e.g., compare dev.path and any aliases
against target_set or have is_within_targets return the matched target strings);
continue to call classify_device(dev, kcore_volume_roots) as before, but after
iterating all snapshot.devices check if matched != target_set and if so return a
non-safe verdict (e.g., a Refuse/InvalidTarget verdict) instead of
Verdict::Safe; update references to target_set, is_within_targets,
classify_device, snapshot and Verdict::Safe accordingly.
In `@crates/node-agent/src/disk/lsblk.rs`:
- Around line 71-76: The parent fallback currently preserves a bare PKNAME like
"sda", causing mismatches with callers expecting "/dev/..." paths; update the
parent assignment (the parent variable computed from parent_path and
node.get("pkname")) to normalize pkname into a device path by prefixing "/dev/"
when pkname does not already start with '/' or "/dev/", e.g. map bare names to
format "/dev/{pkname}". Also add a regression test that constructs a node with
pkname = "sda" and asserts parent_path (or parent) equals Some("/dev/sda") to
prevent regressions; refer to parent_path, parent, node.get("pkname") and the
is_within_targets behavior in kcore-disko-types when adding the test.
In `@crates/node-agent/src/grpc/admin.rs`:
- Around line 1063-1097: apply_disk_layout_impl schedules run_test_then_switch
without acquiring the same self.apply_lock used by apply_nix_config, allowing
concurrent rebuilds to race and stop each other's in-flight systemctl
operations; fix by ensuring the same Arc<AsyncMutex<()>> gates both paths:
either move apply_disk_layout_impl onto AdminService so it can await
self.apply_lock before spawning the rebuild task, or add an apply_lock parameter
and clone/lock it inside apply_disk_layout_impl and inside the tokio::spawn
wrapper so the spawned task acquires the lock around run_test_then_switch (and
any systemctl/reset-failed logic) to serialize rebuilds with apply_nix_config
and other apply_disk_layout calls (references: apply_disk_layout_impl,
apply_nix_config, run_test_then_switch, self.apply_lock, AdminService).
- Around line 1003-1019: The spawn/IO error path after building the command
(Command::new("timeout") with args using run_path/temp_path) currently returns
early on cmd.output().await error and leaks the staged temp file; update the
error handling so that when map_err occurs the code first tries to remove the
temp (e.g., tokio::fs::remove_file(run_path).await or std::fs::remove_file) and
then returns Status::internal with the original error message preserved (combine
a descriptive string with the captured error), ensuring the cleanup mirrors the
non-zero-exit branch that currently unlinks temp_path.
In `@crates/node-agent/src/issue_screen.rs`:
- Around line 36-43: The TTY banner currently always renders the hardcoded
credentials (the lines that call lines.push(style(...)) with "Username: root
Password: kcore"); change it so that the credential line is only pushed when a
marker file is absent: check for the presence of a marker like
"/etc/kcore/initial-credentials-acknowledged" using std::path::Path::exists (or
fs::metadata) before calling the lines.push(style(...)) that emits the
credentials, and if the marker exists skip that push; keep the rest of the
banner (e.g., the Kernel line) unchanged and ensure the check is non-panicking
and returns false on any IO error.
- Around line 180-189: The truncate function currently returns a single "." for
any width <= 3 which underfills the column; update fn truncate(input: &str,
width: usize) to, when width <= 3, return
input.chars().take(width).collect::<String>() (or, if you prefer a sentinel,
return ".".repeat(width)) so the returned string length matches the requested
width; keep the existing behavior for width > 3 (chars[..width-3] + "...") and
keep using the truncate function name to locate the change.
In `@flake.nix`:
- Around line 434-441: In print_install_overview, the lsblk parsing splits MODEL
on spaces so the $4 == "disk" check can fail; change the lsblk invocation to a
parseable output mode (e.g., lsblk -d -o NAME,TYPE,SIZE,MODEL -P) and update the
awk (or a simple shell parse) to test TYPE="disk" from the key=value output
before printing NAME and SIZE and MODEL; this ensures MODEL spaces won’t break
the TYPE check and keeps the output formatting logic correct.
In `@Makefile`:
- Line 95: The help text in the Makefile still calls the targets "NixOS ISO"
while the produced artifact and echo message use the new kcoreOS name; update
the help strings for the Makefile targets iso and iso-remote (and any
accompanying echo/help lines near the echo "@echo "ISO built:
result-iso/iso/kcoreos-$(VERSION)-x86_64-linux.iso" ) to refer to "kcoreOS ISO"
and the new kcoreos-$(VERSION)-x86_64-linux.iso naming so the output of make
help matches the actual ISO name and the iso/iso-remote target descriptions.
In `@proto/node.proto`:
- Around line 448-452: The proto field `rebuild` in proto/node.proto claims
"true (the default)" but proto3 bool zero-value is false; change the field to
`bool skip_rebuild = 4;` and update its comment to document that skip_rebuild
defaults to false (so rebuild runs by default), then update all callers that
read/write `rebuild` (e.g., controller and kctl) to invert their logic to use
`skip_rebuild` and adjust any tests or serialization expectations accordingly;
if renaming is infeasible, alternatively update the comment on `rebuild` to
state that callers must explicitly set `rebuild: true` because there is no
proto-level true default.
In `@specs/tla/DiskLayoutReconcile.tla`:
- Around line 94-116: Liveness property Liveness_SafeEventuallyApplied is
invalid because ReconcileFailed can fire under the same guard as
ReconcileApplied and advance Observed, blocking a later apply; fix by either (A)
restricting the liveness formula to exclude the Failed outcome (e.g. change the
property to require Phase[r] ≠ "Failed" in the antecedent before <> (Phase[r] =
"Applied")), or (B) make ReconcileFailed transient like ReconcileRefused by
removing the Observed' = [Observed EXCEPT ![r] = Generation[r]] update so
failure does not advance Observed and allows retries; also ensure the SPEC
exposes the property under PROPERTIES so it is checked (references:
ReconcileFailed, ReconcileApplied, WF_Vars, Spec, and
Liveness_SafeEventuallyApplied).
---
Outside diff comments:
In `@docs/storage.md`:
- Around line 114-146: Remove the duplicate H2 block titled "Disko Ownership
Modes (safe installer/controller split)" (the pre-rename copy) so only the new
H2 "Disk management modes (safe installer/controller split)" remains; locate the
repeated section content under the heading "Disko Ownership Modes (safe
installer/controller split)" and delete that entire block (including its
duplicated bullets and subsections like "Promote a node..." and "Apply mode
safety gate") leaving the single, canonical ownership description under "Disk
management modes (safe installer/controller split)".
- Around line 414-421: Update the stale CLI and symbol names: replace the
deprecated "apply-disko" and apply_disko_layout references with the new
"apply-disk" command and apply_disk_layout symbol, and update any mention of the
node command alias to the canonical ApplyDisk entry; specifically edit the docs
section that lists the CLI usage and the line that says it "Implements day-2
disko RPC client call via apply_disko_layout" so they instead reference "node
apply-disk -f <file> [--apply] [--timeout-seconds N]" and apply_disk_layout /
ApplyDisk where applicable.
- Around line 193-240: The subsection numbering after the "Day-2 disk workflows"
block is inconsistent (you have "### A)" / "### B)" followed by "### 3) Apply
with bounded timeout", "### 4) Reconcile mounts/services" and then "### 2)
Create VM with required storage metadata"); update the headings so the flow is
monotonic and non-overlapping by either renumbering the numeric headings to
follow A/B (e.g., "### 1) …", "### 2) …", "### 3) …") or convert all numbered
subsections to descriptive headings (remove numeric prefixes) to avoid rebase
artifacts; specifically edit the headings titled "Day-2 disk workflows", "A)",
"B)", "Apply with bounded timeout", "Reconcile mounts/services", and "Create VM
with required storage metadata" so their sequence is logical and the "Apply with
bounded timeout" section no longer duplicates or overlaps the content of
workflow B.
---
Nitpick comments:
In `@crates/controller/src/db.rs`:
- Around line 2108-2132: The upsert_disk_layout_status function currently
overwrites disk_layout_status.observed_generation unconditionally; change the ON
CONFLICT update to preserve monotonic progress by setting observed_generation =
MAX(disk_layout_status.observed_generation, excluded.observed_generation) (i.e.,
use the MAX of the existing row and excluded value) while keeping the rest of
the SET clauses the same so that phase/refusal_reason/message/last_transition_at
can still update; update the SQL string in upsert_disk_layout_status to use the
MAX expression referencing disk_layout_status.observed_generation and
excluded.observed_generation.
- Around line 2006-2041: upsert_disk_layout currently does INSERT ... ON
CONFLICT ... DO UPDATE that blindly sets generation = excluded.generation, which
allows callers to regress generation; modify the conflict UPDATE clause in the
disk_layouts upsert to set generation = MAX(disk_layouts.generation,
excluded.generation) (keep node_id/layout_nix/update_at as-is) so the DB
enforces non-decreasing generation; also, once the project upgrades to SQLite >=
3.35.0 you can simplify upsert_disk_layout by using a single INSERT ... ON
CONFLICT ... DO UPDATE ... RETURNING to avoid the separate SELECT.
In `@crates/kcore-disko-types/Cargo.toml`:
- Around line 1-16: The crate name "kcore-disko-types" should remain as the
internal implementation identifier but document the rationale and future-rename
consideration: add a short comment in Cargo.toml adjacent to package.name (name
= "kcore-disko-types") stating this crate is intentionally named after the
implementation ("disko") while the external operator surface uses "disk", and
note that the name can be changed if disko is swapped out; this clarifying
comment will make the abstraction boundary explicit for future maintainers.
In `@crates/kctl/src/commands/disk_layout.rs`:
- Around line 52-104: The diff path misses the local validation for the
layout_nix body that apply_from_file performs: after obtaining layout_nix in
diff_from_file (using parse_manifest and resolve_layout_nix), check if
layout_nix.contains("disko.devices") and bail with the same error message used
in apply_from_file (referencing manifest.metadata.name) if the check fails; this
keeps diff_from_file, apply_from_file, parse_manifest and resolve_layout_nix
aligned and avoids sending an invalid payload to classify_disk_layout.
In `@crates/node-agent/src/grpc/admin.rs`:
- Around line 1190-1208: The legacy shim in apply_disko_layout currently drops
resp.refusal_reason when mapping the new ApplyDiskLayoutResponse back to the old
ApplyDiskoLayoutResponse; update apply_disko_layout (and the mapping that builds
proto::ApplyDiskoLayoutResponse from resp returned by apply_disk_layout_impl) to
detect a non-empty resp.refusal_reason and prefix the legacy message with that
code (e.g., "<refusal_reason>: <message>") so legacy callers get a stable
machine-readable token while preserving the human message. Ensure you reference
resp.refusal_reason and resp.message from the response of apply_disk_layout_impl
and only prefix when refusal_reason is present.
In `@examples/cluster/prod.yaml`:
- Line 5: The manifest hardcodes a private IP in the controller field; update
the examples/cluster/prod.yaml so the controller entry uses a stable DNS name or
resolvable service address (e.g., controller.prod.example.com:9090) or reference
an environment/config variable rather than 192.168.40.107:9090; modify the
controller: value in prod.yaml to a DNS-based endpoint (or templated/config
reference) to enable endpoint rotation and failover without editing the
manifest.
In `@modules/kcore-disko.nix`:
- Around line 183-192: The module currently exposes persistedLayoutPath (type =
types.path, default = /etc/kcore/disk/current.nix) which lets evaluation depend
on mutable on-disk state; change the design so Nix evaluation is deterministic
by removing reliance on that mutable file: replace persistedLayoutPath with an
option that accepts the persisted layout as an immutable string/attr (e.g.,
persistedLayoutContent: type = types.str or types.attrs) or accept a
store-backed path produced before evaluation, and update the logic that merges
`disko.devices` (and the managementMode = "controller-managed" branch) to
parse/merge from the provided immutable input instead of reading
/etc/kcore/disk/current.nix at eval time. Ensure functions referencing
persistedLayoutPath are updated to use the new
persistedLayoutContent/store-backed input.
🪄 Autofix (Beta)
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
Run ID: 9c7ba072-bdac-42a5-b1e2-8d34cfd11176
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.locktools/tla2tools.jaris excluded by!**/*.jar
📒 Files selected for processing (43)
.github/workflows/formal-checks.ymlCargo.tomlMakefilecrates/controller/Cargo.tomlcrates/controller/src/db.rscrates/controller/src/disk_reconciler.rscrates/controller/src/grpc/controller.rscrates/controller/src/main.rscrates/controller/src/replication.rscrates/dashboard/src/app.rscrates/dashboard/tests/pagination_test.rscrates/dashboard/tests/support/mod.rscrates/kcore-disko-types/Cargo.tomlcrates/kcore-disko-types/src/lib.rscrates/kctl/src/commands/apply.rscrates/kctl/src/commands/disk_layout.rscrates/kctl/src/commands/mod.rscrates/kctl/src/commands/node.rscrates/kctl/src/main.rscrates/kctl/src/output.rscrates/node-agent/Cargo.tomlcrates/node-agent/src/disk/classifier.rscrates/node-agent/src/disk/lsblk.rscrates/node-agent/src/disk/mod.rscrates/node-agent/src/grpc/admin.rscrates/node-agent/src/issue_screen.rscrates/node-agent/src/main.rsdocs/day-2-filesystem-operations.mddocs/kctl-commands-and-workflows.mddocs/storage.mdexamples/cluster/prod.yamlexamples/node-install/agent.yamlexamples/node-install/controller.yamlexamples/node-install/ha-controller.yamlflake.nixmodules/kcore-branding.nixmodules/kcore-disko.nixproto/controller.protoproto/node.protoscripts/check-tla.shspecs/tla/DiskLayoutReconcile.cfgspecs/tla/DiskLayoutReconcile.tlaspecs/tla/README.md
| let resp = admin | ||
| .apply_disk_layout(Request::new(req)) | ||
| .await | ||
| .map_err(|e| format!("apply_disk_layout rpc: {e}"))? | ||
| .into_inner(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm disk-layout reconciliation RPC awaits are wrapped in a client-side timeout.
rg -n -C3 'apply_disk_layout\s*\(|time::timeout\s*\(' crates/controller/src/disk_reconciler.rsRepository: rtacconi/kcore
Length of output: 281
🏁 Script executed:
#!/bin/bash
# Check if NODE_RPC_TIMEOUT_SECS is defined and find its value
rg -n 'NODE_RPC_TIMEOUT_SECS' crates/controller/src/disk_reconciler.rsRepository: rtacconi/kcore
Length of output: 152
🏁 Script executed:
#!/bin/bash
# Get broader context around lines 104-108 and 117-124 to understand the complete logic
sed -n '95,135p' crates/controller/src/disk_reconciler.rsRepository: rtacconi/kcore
Length of output: 1316
🏁 Script executed:
#!/bin/bash
# Search for other RPC calls that might need timeout protection
rg -n '\.await' crates/controller/src/disk_reconciler.rs | head -20Repository: rtacconi/kcore
Length of output: 303
🏁 Script executed:
#!/bin/bash
# Check the file for any existing timeout patterns
rg -n 'time::timeout|tokio::time::timeout' crates/controller/src/disk_reconciler.rsRepository: rtacconi/kcore
Length of output: 40
Add a client-side timeout to prevent blocking disk-layout reconciliation.
The timeout_seconds in the request payload only bounds server-side execution. If the gRPC future stalls before or during server processing, this reconciler can block indefinitely and skip subsequent disk layouts. Wrap the RPC call with tokio::time::timeout using a duration of NODE_RPC_TIMEOUT_SECS + 30 seconds to ensure the client future completes.
Proposed fix
- let resp = admin
- .apply_disk_layout(Request::new(req))
- .await
+ let resp = time::timeout(
+ Duration::from_secs(NODE_RPC_TIMEOUT_SECS as u64 + 30),
+ admin.apply_disk_layout(Request::new(req)),
+ )
+ .await
+ .map_err(|_| {
+ format!(
+ "apply_disk_layout rpc timed out after {}s",
+ NODE_RPC_TIMEOUT_SECS + 30
+ )
+ })?
.map_err(|e| format!("apply_disk_layout rpc: {e}"))?
.into_inner();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let resp = admin | |
| .apply_disk_layout(Request::new(req)) | |
| .await | |
| .map_err(|e| format!("apply_disk_layout rpc: {e}"))? | |
| .into_inner(); | |
| let resp = time::timeout( | |
| Duration::from_secs(NODE_RPC_TIMEOUT_SECS as u64 + 30), | |
| admin.apply_disk_layout(Request::new(req)), | |
| ) | |
| .await | |
| .map_err(|_| { | |
| format!( | |
| "apply_disk_layout rpc timed out after {}s", | |
| NODE_RPC_TIMEOUT_SECS + 30 | |
| ) | |
| })? | |
| .map_err(|e| format!("apply_disk_layout rpc: {e}"))? | |
| .into_inner(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/controller/src/disk_reconciler.rs` around lines 104 - 108, Wrap the
admin.apply_disk_layout(Request::new(req)).await call in tokio::time::timeout
using Duration::from_secs(NODE_RPC_TIMEOUT_SECS + 30) so the client future
cannot stall indefinitely; call timeout(...).await, map a timeout error into a
clear Err (e.g. format!("apply_disk_layout rpc timed out")) and preserve other
RPC errors (map_err for RPC failure and .into_inner() on success) so the
variable resp is only set when the timed call completes successfully. Ensure you
reference the same expression (admin.apply_disk_layout(Request::new(req))) and
handle the Result from timeout appropriately to avoid blocking disk layout
reconciliation.
| let phase = if resp.success { | ||
| "applied" | ||
| } else if !resp.refusal_reason.is_empty() { | ||
| "refused" | ||
| } else { | ||
| "failed" | ||
| }; | ||
| db.upsert_disk_layout_status(&DiskLayoutStatusRow { | ||
| name: row.name.clone(), | ||
| observed_generation: row.generation, | ||
| phase: phase.to_string(), | ||
| refusal_reason: resp.refusal_reason.clone(), | ||
| message: resp.message.clone(), | ||
| last_transition_at: String::new(), | ||
| }) |
There was a problem hiding this comment.
Do not advance observed_generation for refused or failed applies.
list_disk_layouts_needing_reconcile() only retries rows where observed_generation < generation; writing row.generation for refused/failed makes the reconciler stop retrying the exact case documented in this file and in docs/kctl-commands-and-workflows.md.
Proposed fix
let phase = if resp.success {
"applied"
} else if !resp.refusal_reason.is_empty() {
"refused"
} else {
"failed"
};
+ let observed_generation = if resp.success {
+ row.generation
+ } else {
+ row.generation.saturating_sub(1)
+ };
db.upsert_disk_layout_status(&DiskLayoutStatusRow {
name: row.name.clone(),
- observed_generation: row.generation,
+ observed_generation,
phase: phase.to_string(),
refusal_reason: resp.refusal_reason.clone(),
message: resp.message.clone(),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let phase = if resp.success { | |
| "applied" | |
| } else if !resp.refusal_reason.is_empty() { | |
| "refused" | |
| } else { | |
| "failed" | |
| }; | |
| db.upsert_disk_layout_status(&DiskLayoutStatusRow { | |
| name: row.name.clone(), | |
| observed_generation: row.generation, | |
| phase: phase.to_string(), | |
| refusal_reason: resp.refusal_reason.clone(), | |
| message: resp.message.clone(), | |
| last_transition_at: String::new(), | |
| }) | |
| let phase = if resp.success { | |
| "applied" | |
| } else if !resp.refusal_reason.is_empty() { | |
| "refused" | |
| } else { | |
| "failed" | |
| }; | |
| let observed_generation = if resp.success { | |
| row.generation | |
| } else { | |
| row.generation.saturating_sub(1) | |
| }; | |
| db.upsert_disk_layout_status(&DiskLayoutStatusRow { | |
| name: row.name.clone(), | |
| observed_generation, | |
| phase: phase.to_string(), | |
| refusal_reason: resp.refusal_reason.clone(), | |
| message: resp.message.clone(), | |
| last_transition_at: String::new(), | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/controller/src/disk_reconciler.rs` around lines 110 - 124, The code
currently writes row.generation into DiskLayoutStatusRow.observed_generation for
all outcomes, which causes refused/failed applies to be considered observed and
stops retries; update the db.upsert_disk_layout_status call so that if
resp.success is true set observed_generation to row.generation, otherwise keep
the existing observed_generation (e.g. row.observed_generation) so
list_disk_layouts_needing_reconcile will continue to retry on refused/failed
applies; adjust the DiskLayoutStatusRow construction in the
db.upsert_disk_layout_status call accordingly (use resp.success to choose
between row.generation and row.observed_generation) and leave
phase/refusal_reason/message handling unchanged.
| let row = DiskLayoutRow { | ||
| name: name.clone(), | ||
| node_id: node_id.clone(), | ||
| generation, | ||
| layout_nix: layout_nix.clone(), | ||
| created_at: String::new(), | ||
| updated_at: String::new(), | ||
| }; | ||
| let stored = self | ||
| .db | ||
| .upsert_disk_layout(&row) | ||
| .map_err(|e| Status::internal(format!("upserting disk layout: {e}")))?; | ||
|
|
||
| // Reset status to Pending on any content change so the reconciler | ||
| // picks it up and the node-agent re-observes the new generation. | ||
| self.db | ||
| .upsert_disk_layout_status(&DiskLayoutStatusRow { | ||
| name: name.clone(), | ||
| observed_generation: 0, | ||
| phase: "pending".to_string(), | ||
| refusal_reason: String::new(), | ||
| message: String::new(), | ||
| last_transition_at: String::new(), | ||
| }) | ||
| .map_err(|e| Status::internal(format!("resetting disk layout status: {e}")))?; |
There was a problem hiding this comment.
Keep the DiskLayout row and status reset atomic.
Line 1107 persists the new layout generation, then Line 1114 resets reconcile status in a separate DB write. If the status reset fails, the API returns an error after the layout has already changed, and replication is skipped, leaving callers and reconcilers with a partially-applied update. Please move the layout upsert + status reset into one DB transaction/helper, or roll back the layout write on status failure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/controller/src/grpc/controller.rs` around lines 1099 - 1123, The
current code calls db.upsert_disk_layout(...) then
db.upsert_disk_layout_status(...), which can leave the DB in a partially-applied
state if the status write fails; change this so both writes are performed
atomically: add a new transactional helper on self.db (or use an existing
transaction API) that performs upsert_disk_layout(Row) and
upsert_disk_layout_status(DiskLayoutStatusRow) inside one transaction, or catch
a failure of upsert_disk_layout_status and roll back the prior
upsert_disk_layout; update the call site to use this new helper so the
DiskLayoutRow and DiskLayoutStatusRow changes are committed or rolled back
together.
| async fn create_disk_layout( | ||
| &self, | ||
| request: Request<controller_proto::CreateDiskLayoutRequest>, | ||
| ) -> Result<Response<controller_proto::CreateDiskLayoutResponse>, Status> { | ||
| auth::require_peer(&request, &[CN_KCTL, CN_CONTROLLER_PREFIX])?; | ||
| self.create_disk_layout_impl(request.into_inner()).await | ||
| } | ||
|
|
||
| async fn get_disk_layout( | ||
| &self, | ||
| request: Request<controller_proto::GetDiskLayoutRequest>, | ||
| ) -> Result<Response<controller_proto::GetDiskLayoutResponse>, Status> { | ||
| auth::require_peer(&request, &[CN_KCTL, CN_CONTROLLER_PREFIX])?; | ||
| self.get_disk_layout_impl(request.into_inner()).await | ||
| } | ||
|
|
||
| async fn list_disk_layouts( | ||
| &self, | ||
| request: Request<controller_proto::ListDiskLayoutsRequest>, | ||
| ) -> Result<Response<controller_proto::ListDiskLayoutsResponse>, Status> { | ||
| auth::require_peer(&request, &[CN_KCTL, CN_CONTROLLER_PREFIX])?; | ||
| self.list_disk_layouts_impl(request.into_inner()).await | ||
| } | ||
|
|
||
| async fn delete_disk_layout( | ||
| &self, | ||
| request: Request<controller_proto::DeleteDiskLayoutRequest>, | ||
| ) -> Result<Response<controller_proto::DeleteDiskLayoutResponse>, Status> { | ||
| auth::require_peer(&request, &[CN_KCTL, CN_CONTROLLER_PREFIX])?; | ||
| self.delete_disk_layout_impl(request.into_inner()).await | ||
| } | ||
|
|
||
| async fn classify_disk_layout( | ||
| &self, | ||
| request: Request<controller_proto::ClassifyDiskLayoutRequest>, | ||
| ) -> Result<Response<controller_proto::ClassifyDiskLayoutResponse>, Status> { | ||
| auth::require_peer(&request, &[CN_KCTL, CN_CONTROLLER_PREFIX])?; | ||
| self.classify_disk_layout_impl(request.into_inner()).await | ||
| } |
There was a problem hiding this comment.
Add these DiskLayout RPCs to the compliance ACL report.
The new RPCs enforce CN_KCTL/CN_CONTROLLER_PREFIX, but GetComplianceReport’s access_control list does not report them, so the compliance surface is incomplete.
Suggested ACL entries
+ acl("CreateDiskLayout", CN_KCTL),
+ acl("GetDiskLayout", CN_KCTL),
+ acl("ListDiskLayouts", CN_KCTL),
+ acl("DeleteDiskLayout", CN_KCTL),
+ acl("ClassifyDiskLayout", CN_KCTL),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/controller/src/grpc/controller.rs` around lines 4959 - 4997,
GetComplianceReport's access_control list is missing the new DiskLayout RPCs
that require CN_KCTL/CN_CONTROLLER_PREFIX; update the compliance ACL report to
include entries for create_disk_layout, get_disk_layout, list_disk_layouts,
delete_disk_layout and classify_disk_layout (each annotated with the enforced
CN_KCTL and CN_CONTROLLER_PREFIX) so the access_control output reflects the
actual enforcement in those RPC handlers.
| "disk_layout.create" => { | ||
| let name = required_str(&body, "name", &head.resource_key)?; | ||
| let node_id = required_str(&body, "nodeId", &head.resource_key)?; | ||
| let layout_nix = required_str(&body, "layoutNix", &head.resource_key)?; | ||
| let generation = body.get("generation").and_then(Value::as_i64).unwrap_or(1); | ||
| let row = crate::db::DiskLayoutRow { | ||
| name: name.to_string(), | ||
| node_id: node_id.to_string(), | ||
| generation, | ||
| layout_nix: layout_nix.to_string(), | ||
| created_at: String::new(), | ||
| updated_at: String::new(), | ||
| }; | ||
| db.upsert_disk_layout(&row) | ||
| .map_err(|e| format!("upsert disk layout {name}: {e}"))?; | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
disk_layout.create materialization can fail with FK violation when the node arrives out of order.
disk_layouts.node_id is a foreign key to nodes(id) (see crates/controller/src/db.rs:386-402). If a peer replicates disk_layout.create before the corresponding node.register has been materialized locally, db.upsert_disk_layout will fail with a FK constraint error and the poller will retry indefinitely with a hard error.
Other handlers in this same function address this by calling ensure_replicated_node_exists first (see vm.create at Line 1217 and network.create at Line 1375). Please do the same here for consistency and to make cross-DC materialization order-independent.
🔧 Proposed fix
"disk_layout.create" => {
let name = required_str(&body, "name", &head.resource_key)?;
let node_id = required_str(&body, "nodeId", &head.resource_key)?;
let layout_nix = required_str(&body, "layoutNix", &head.resource_key)?;
let generation = body.get("generation").and_then(Value::as_i64).unwrap_or(1);
+ ensure_replicated_node_exists(db, node_id)?;
let row = crate::db::DiskLayoutRow {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "disk_layout.create" => { | |
| let name = required_str(&body, "name", &head.resource_key)?; | |
| let node_id = required_str(&body, "nodeId", &head.resource_key)?; | |
| let layout_nix = required_str(&body, "layoutNix", &head.resource_key)?; | |
| let generation = body.get("generation").and_then(Value::as_i64).unwrap_or(1); | |
| let row = crate::db::DiskLayoutRow { | |
| name: name.to_string(), | |
| node_id: node_id.to_string(), | |
| generation, | |
| layout_nix: layout_nix.to_string(), | |
| created_at: String::new(), | |
| updated_at: String::new(), | |
| }; | |
| db.upsert_disk_layout(&row) | |
| .map_err(|e| format!("upsert disk layout {name}: {e}"))?; | |
| Ok(()) | |
| } | |
| "disk_layout.create" => { | |
| let name = required_str(&body, "name", &head.resource_key)?; | |
| let node_id = required_str(&body, "nodeId", &head.resource_key)?; | |
| let layout_nix = required_str(&body, "layoutNix", &head.resource_key)?; | |
| let generation = body.get("generation").and_then(Value::as_i64).unwrap_or(1); | |
| ensure_replicated_node_exists(db, node_id)?; | |
| let row = crate::db::DiskLayoutRow { | |
| name: name.to_string(), | |
| node_id: node_id.to_string(), | |
| generation, | |
| layout_nix: layout_nix.to_string(), | |
| created_at: String::new(), | |
| updated_at: String::new(), | |
| }; | |
| db.upsert_disk_layout(&row) | |
| .map_err(|e| format!("upsert disk layout {name}: {e}"))?; | |
| Ok(()) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/controller/src/replication.rs` around lines 1582 - 1598, The
disk_layout.create handler can hit a FK violation because nodes may not be
materialized yet; before calling db.upsert_disk_layout in the
"disk_layout.create" match arm, call ensure_replicated_node_exists with the
node_id extracted via required_str("nodeId", ...) (same pattern used in
vm.create and network.create) to ensure the node record exists, then proceed to
build the DiskLayoutRow and call db.upsert_disk_layout; handle and propagate
errors from ensure_replicated_node_exists similarly to the other handlers so
replication becomes order-independent.
| fn truncate(input: &str, width: usize) -> String { | ||
| let chars = input.chars().collect::<Vec<_>>(); | ||
| if chars.len() <= width { | ||
| return input.to_string(); | ||
| } | ||
| if width <= 3 { | ||
| return ".".to_string(); | ||
| } | ||
| chars[..width - 3].iter().collect::<String>() + "..." | ||
| } |
There was a problem hiding this comment.
truncate returns a single . for any width <= 3, which underfills the column.
When width <= 3, truncate returns "." (one char) regardless of width. Because call sites pad via format!("{x:<12}") etc., the column still aligns, so this is not a visible bug today — but the sentinel feels wrong and will silently truncate to 1 character if any caller ever passes width ∈ {1,2,3} (e.g. for a very narrow TTY). Safer to return input.chars().take(width).collect() or a string of width . chars.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/node-agent/src/issue_screen.rs` around lines 180 - 189, The truncate
function currently returns a single "." for any width <= 3 which underfills the
column; update fn truncate(input: &str, width: usize) to, when width <= 3,
return input.chars().take(width).collect::<String>() (or, if you prefer a
sentinel, return ".".repeat(width)) so the returned string length matches the
requested width; keep the existing behavior for width > 3 (chars[..width-3] +
"...") and keep using the truncate function name to locate the change.
| print_install_overview() { | ||
| echo "kcoreOS hardware discovery" | ||
| echo "--------------------------" | ||
| echo "Network interfaces:" | ||
| ip -o -4 addr show scope global | awk '{print " - " $2 " " $4}' || true | ||
| echo "" | ||
| echo "Disks:" | ||
| lsblk -d -o NAME,SIZE,MODEL,TYPE | awk '$4 == "disk" {printf " - /dev/%s %s %s\n", $1, $2, $3}' |
There was a problem hiding this comment.
Avoid field-splitting MODEL before checking disk type.
MODEL often contains spaces, so $4 == "disk" can be false even for real disks. Put TYPE before MODEL or use a parseable output mode.
Proposed fix
- lsblk -d -o NAME,SIZE,MODEL,TYPE | awk '$4 == "disk" {printf " - /dev/%s %s %s\n", $1, $2, $3}'
+ lsblk -dn -o NAME,SIZE,TYPE,MODEL | awk '$3 == "disk" { model=$0; sub(/^[^[:space:]]+[[:space:]]+[^[:space:]]+[[:space:]]+[^[:space:]]+[[:space:]]*/, "", model); printf " - /dev/%s %s %s\n", $1, $2, model }'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@flake.nix` around lines 434 - 441, In print_install_overview, the lsblk
parsing splits MODEL on spaces so the $4 == "disk" check can fail; change the
lsblk invocation to a parseable output mode (e.g., lsblk -d -o
NAME,TYPE,SIZE,MODEL -P) and update the awk (or a simple shell parse) to test
TYPE="disk" from the key=value output before printing NAME and SIZE and MODEL;
this ensures MODEL spaces won’t break the TYPE check and keeps the output
formatting logic correct.
| @ls -lh result-iso/iso/*.iso | ||
| @echo "" | ||
| @echo "ISO built: result-iso/iso/nixos-kcore-$(VERSION)-x86_64-linux.iso" | ||
| @echo "ISO built: result-iso/iso/kcoreos-$(VERSION)-x86_64-linux.iso" |
There was a problem hiding this comment.
Update the help text to match the kcoreOS ISO rename.
Line 95 now reports a kcoreos-* ISO, but make help still describes the iso and iso-remote targets as “NixOS ISO”.
Suggested help text update
- `@echo` " iso Build NixOS ISO (Linux only)"
- `@echo` " iso-remote Build NixOS ISO on remote Linux server (from macOS)"
+ `@echo` " iso Build kcoreOS ISO (Linux only)"
+ `@echo` " iso-remote Build kcoreOS ISO on remote Linux server (from macOS)"Also applies to: 140-141
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Makefile` at line 95, The help text in the Makefile still calls the targets
"NixOS ISO" while the produced artifact and echo message use the new kcoreOS
name; update the help strings for the Makefile targets iso and iso-remote (and
any accompanying echo/help lines near the echo "@echo "ISO built:
result-iso/iso/kcoreos-$(VERSION)-x86_64-linux.iso" ) to refer to "kcoreOS ISO"
and the new kcoreos-$(VERSION)-x86_64-linux.iso naming so the output of make
help matches the actual ISO name and the iso/iso-remote target descriptions.
| // When true (the default) and the layout applies successfully, chain | ||
| // nixos-rebuild test followed by nixos-rebuild switch so the persisted | ||
| // layout is evaluated by NixOS. When false, skip the rebuild (validation | ||
| // or local-only apply flows). | ||
| bool rebuild = 4; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# See how callers populate the rebuild field.
rg -nP -C3 '\brebuild\s*[:=]' --type=rust | rg -nP -C1 'ApplyDiskLayout|apply_disk_layout'
rg -nP -C3 'ApplyDiskLayoutRequest\s*\{' --type=rustRepository: rtacconi/kcore
Length of output: 2623
🏁 Script executed:
#!/bin/bash
# Get full context of controller disk_reconciler request construction
sed -n '90,110p' crates/controller/src/disk_reconciler.rs
# Get full context of kctl command request construction
sed -n '690,710p' crates/kctl/src/commands/node.rs
# Check the proto definition for ApplyDiskLayoutRequest
rg -A15 'message ApplyDiskLayoutRequest' proto/node.protoRepository: rtacconi/kcore
Length of output: 2188
🏁 Script executed:
#!/bin/bash
# Find the function signature and CLI flag definitions for rebuild in kctl
sed -n '680,720p' crates/kctl/src/commands/node.rs | head -60
# Also search for any CLI struct/args that defines rebuild
rg -B5 -A5 'rebuild' crates/kctl/src/commands/node.rs | head -80Repository: rtacconi/kcore
Length of output: 2403
🏁 Script executed:
#!/bin/bash
# Find where apply_disk_layout is called from (CLI handler)
rg -n 'apply_disk_layout\(' crates/kctl/src/commands/node.rs | grep -v 'pub async fn'
# Find the CLI argument struct/parser for disk layout command
rg -B10 -A10 'DiskLayout\|disk.layout\|disk_layout' crates/kctl/src/commands/node.rs | grep -A15 'struct\|rebuild' | head -80Repository: rtacconi/kcore
Length of output: 125
🏁 Script executed:
#!/bin/bash
# Find the full context of the apply_disk_layout call to see what rebuild value is passed
sed -n '680,730p' crates/kctl/src/commands/node.rs
# Also look for CLI command/subcommand definitions
rg -n 'apply.*disk\|DiskLayout' crates/kctl/src/commands/node.rs | head -20Repository: rtacconi/kcore
Length of output: 1433
🏁 Script executed:
#!/bin/bash
# Search for the main command handler that dispatches to apply_disk_layout
rg -n 'Command|Subcommand|match.*cmd|fn.*disk' crates/kctl/src/commands/node.rs | head -30
# Look for CLI argument structures
rg -n 'struct.*Args|struct.*Cmd' crates/kctl/src/commands/node.rs | head -20Repository: rtacconi/kcore
Length of output: 155
🏁 Script executed:
#!/bin/bash
# Look at the beginning of the node.rs file to understand the CLI structure
head -150 crates/kctl/src/commands/node.rs
# Also search for clap derive or enum definitions
rg -n '#\[command|#\[derive|enum.*Cmd' crates/kctl/src/commands/node.rs | head -30Repository: rtacconi/kcore
Length of output: 5077
🏁 Script executed:
#!/bin/bash
# See the struct definitions that were starting at line 316
sed -n '316,400p' crates/kctl/src/commands/node.rs
# Also search for where apply_disk_layout is invoked from CLI context
rg -B30 'apply_disk_layout\(' crates/kctl/src/commands/node.rs | grep -A30 'rebuild\|struct\|command' | head -100Repository: rtacconi/kcore
Length of output: 3592
🏁 Script executed:
#!/bin/bash
# Search for the main CLI entry point or dispatcher that calls apply_disk_layout
rg -n 'apply.disk.layout\|apply-disk-layout' crates/kctl/src/ --type=rust
# Look at main.rs or cli.rs to see how commands are dispatched
find crates/kctl/src -name 'main.rs' -o -name 'cli.rs' -o -name 'lib.rs' | head -5 | xargs head -200Repository: rtacconi/kcore
Length of output: 5604
🏁 Script executed:
#!/bin/bash
# Find the NodeAction enum definition to see disk layout command parameters
rg -A100 'enum NodeAction' crates/kctl/src/lib.rs | head -150Repository: rtacconi/kcore
Length of output: 120
🏁 Script executed:
#!/bin/bash
# Find where NodeAction is defined
rg -n 'enum NodeAction' crates/kctl/src/ --type=rust
# Also check main.rs path
find crates/kctl/src -type f -name '*.rs' | grep -E '(main|cli|lib)' | head -5Repository: rtacconi/kcore
Length of output: 153
🏁 Script executed:
#!/bin/bash
# Look at the NodeAction enum definition around line 558
sed -n '558,650p' crates/kctl/src/main.rsRepository: rtacconi/kcore
Length of output: 3583
🏁 Script executed:
#!/bin/bash
# Find where the ApplyDisk command dispatches to apply_disk_layout function
rg -n 'ApplyDisk\|no_rebuild' crates/kctl/src/main.rs | head -40
# Look at the command dispatch logic
sed -n '1050,1150p' crates/kctl/src/main.rsRepository: rtacconi/kcore
Length of output: 3834
🏁 Script executed:
#!/bin/bash
# Search specifically for the ApplyDisk command handler
rg -n 'NodeAction::ApplyDisk\|ApplyDisk {' crates/kctl/src/main.rs -A5Repository: rtacconi/kcore
Length of output: 189
🏁 Script executed:
#!/bin/bash
# Search for the ApplyDisk command handler with proper regex escaping
rg -n 'NodeAction::ApplyDisk' crates/kctl/src/main.rs -A10Repository: rtacconi/kcore
Length of output: 512
rebuild default contradicts proto3 zero-value semantics.
The comment states rebuild is "true (the default)" but in proto3 the zero value for bool is false, so any client that omits the field on the wire will disable rebuild — the opposite of what the comment promises. Current callers (controller and kctl) both ensure rebuild=true explicitly or through inverted-flag logic, so they're safe, but the comment is misleading for new clients or manual message construction.
Either invert the field so its zero value matches the intended default:
- Change to
bool skip_rebuild = 4;with the default documented as false (rebuild runs by default)
Or clarify that callers must explicitly set the field:
- Update the comment to state that clients must explicitly pass
rebuild: trueand there is no proto-level default.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@proto/node.proto` around lines 448 - 452, The proto field `rebuild` in
proto/node.proto claims "true (the default)" but proto3 bool zero-value is
false; change the field to `bool skip_rebuild = 4;` and update its comment to
document that skip_rebuild defaults to false (so rebuild runs by default), then
update all callers that read/write `rebuild` (e.g., controller and kctl) to
invert their logic to use `skip_rebuild` and adjust any tests or serialization
expectations accordingly; if renaming is infeasible, alternatively update the
comment on `rebuild` to state that callers must explicitly set `rebuild: true`
because there is no proto-level true default.
| ReconcileFailed(r) == | ||
| /\ Observed[r] < Generation[r] | ||
| /\ Safe[r] | ||
| /\ Phase' = [Phase EXCEPT ![r] = "Failed"] | ||
| /\ Observed' = [Observed EXCEPT ![r] = Generation[r]] | ||
| /\ UNCHANGED <<Generation, Safe>> | ||
|
|
||
| Noop == UNCHANGED Vars | ||
|
|
||
| Next == | ||
| \/ \E r \in Resources : | ||
| \/ SubmitNewLayout(r) | ||
| \/ ClearBlockers(r) | ||
| \/ ReconcileApplied(r) | ||
| \/ ReconcileRefused(r) | ||
| \/ ReconcileFailed(r) | ||
| \/ Noop | ||
|
|
||
| FairnessApplied == \A r1 \in Resources : WF_Vars(ReconcileApplied(r1)) | ||
| FairnessRefused == \A r2 \in Resources : WF_Vars(ReconcileRefused(r2)) | ||
|
|
||
| Spec == | ||
| Init /\ [][Next]_Vars /\ FairnessApplied /\ FairnessRefused |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm liveness isn't in the cfg and show how ReconcileFailed interacts.
fd -t f 'DiskLayoutReconcile.cfg' | xargs -r catRepository: rtacconi/kcore
Length of output: 286
🏁 Script executed:
wc -l specs/tla/DiskLayoutReconcile.tlaRepository: rtacconi/kcore
Length of output: 95
🏁 Script executed:
cat -n specs/tla/DiskLayoutReconcile.tlaRepository: rtacconi/kcore
Length of output: 7384
Liveness property Liveness_SafeEventuallyApplied is invalid and unchecked.
Lines 156–158 claim that if Safe[r] and Observed[r] < Generation[r] hold, the system eventually reaches Phase[r] = "Applied". However, ReconcileFailed(r) (lines 94–99) has identical guards as ReconcileApplied(r) (lines 80–85) and can fire first, setting Phase[r] = "Failed" and advancing Observed[r] = Generation[r], which disables ReconcileApplied(r) forever.
Weak fairness WF_Vars(ReconcileApplied(r1)) does not prevent this: it only guarantees the action fires if continuously enabled, but ReconcileFailed can break that continuity. There is no fairness constraint on ReconcileFailed or SubmitNewLayout, so no forced retry either.
The property is also not validated: DiskLayoutReconcile.cfg contains only invariants, no PROPERTIES section. The spec comment at line 154–155 ("the reconciler eventually reaches Applied") is only true if Failed is excluded or modeled differently.
Fix by either:
- Constraining the liveness property to exclude the
Failedoutcome:[](Safe[r] /\ Observed[r] < Generation[r] /\ Phase[r] ≠ "Failed" => <>(Phase[r] = "Applied")) - Modeling
Failedas transient (likeRefused): don't advanceObserved[r]onReconcileFailed, matching the production reconciler atcrates/controller/src/disk_reconciler.rs, which retries on failure.
Also applies to: lines 152–158
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@specs/tla/DiskLayoutReconcile.tla` around lines 94 - 116, Liveness property
Liveness_SafeEventuallyApplied is invalid because ReconcileFailed can fire under
the same guard as ReconcileApplied and advance Observed, blocking a later apply;
fix by either (A) restricting the liveness formula to exclude the Failed outcome
(e.g. change the property to require Phase[r] ≠ "Failed" in the antecedent
before <> (Phase[r] = "Applied")), or (B) make ReconcileFailed transient like
ReconcileRefused by removing the Observed' = [Observed EXCEPT ![r] =
Generation[r]] update so failure does not advance Observed and allows retries;
also ensure the SPEC exposes the property under PROPERTIES so it is checked
(references: ReconcileFailed, ReconcileApplied, WF_Vars, Spec, and
Liveness_SafeEventuallyApplied).
Use a config-path-relative .kcore fallback for create-from-manifest defaults when ~/.kcore cannot be created so sandboxed builds and tests do not fail with permission errors.
1 participant
… branding UX
Introduce end-to-end disk layout reconciliation across controller, node-agent, CLI, and docs while rebranding login/install surfaces to kcoreOS with live hardware summaries so operators get consistent day-0/day-2 workflows.
Summary by CodeRabbit
New Features
DiskLayoutcontroller resource with declarative disk provisioning, reconciliation, and status tracking viakctl diff/apply/get/describe/delete disk-layout.Chores