node: co-locate the metadata and data roles in one process (#215) - #237
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 97898d8a60
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| result = meta_node::serve(meta, &observability, metrics_addr) => { | ||
| result.map_err(|error| format!("metadata role exited: {error}")) | ||
| } | ||
| result = data_node::serve(data, &observability, metrics_addr) => { |
There was a problem hiding this comment.
In node mode this passes the same NodeObservability to both roles, but each role flips observability.gate.mark_ready() independently (meta_node::serve after only the meta listeners bind, and data_node::serve after only the data listener binds). When the metadata side wins startup, the shared /readyz begins returning ready before the data native/replica listener (and, for leaders, its readiness probe) is installed, so a load balancer or harness can route traffic to a half-started co-located process instead of the promised conjunction. Give each role its own startup gate and AND them in the shared source.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. ReadinessGate gained a startup latch: require_marks(n) declares how many distinct mark_ready calls open the gate, and the co-located runner sets 2 before either role starts. The first role to finish binding now leaves /readyz at 503 with "waiting for 1 more component(s) to finish starting" instead of advertising a half-started process; post-startup the gate is an ordinary level again. Unit test a_gate_requiring_two_marks_opens_only_on_the_second pins the whole arc, and live-chaos scenario 10 now boots the co-located binary and gates on the shared /readyz before asserting both planes serve.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 5 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/vtop-node/src/main.rs">
<violation number="1" location="crates/vtop-node/src/main.rs:48">
P2: The live-chaos harness still starts separate `meta` and `data` processes, so this new command is never exercised and the advertised six-to-three process change is not covered. Wiring the launcher/config generators to invoke `vtop-node node --config` with one shared observability target and readiness marker would make the harness validate this path.</violation>
</file>
<file name="crates/vtop-node/src/colocated.rs">
<violation number="1" location="crates/vtop-node/src/colocated.rs:57">
P2: An empty per-role `observability: {}` or `listen: null` is accepted and ignored, contrary to the documented contract that any per-role observability block is rejected. Preserve field presence during deserialization and reject a supplied block, not only a non-null listen address.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| /// Run BOTH roles in one process — a metadata voter and a data-plane | ||
| /// replica sharing a runtime, one observability endpoint, and a fate | ||
| /// (#215). | ||
| Node { |
There was a problem hiding this comment.
P2: The live-chaos harness still starts separate meta and data processes, so this new command is never exercised and the advertised six-to-three process change is not covered. Wiring the launcher/config generators to invoke vtop-node node --config with one shared observability target and readiness marker would make the harness validate this path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/vtop-node/src/main.rs, line 48:
<comment>The live-chaos harness still starts separate `meta` and `data` processes, so this new command is never exercised and the advertised six-to-three process change is not covered. Wiring the launcher/config generators to invoke `vtop-node node --config` with one shared observability target and readiness marker would make the harness validate this path.</comment>
<file context>
@@ -41,6 +42,13 @@ enum Command {
+ /// Run BOTH roles in one process — a metadata voter and a data-plane
+ /// replica sharing a runtime, one observability endpoint, and a fate
+ /// (#215).
+ Node {
+ #[arg(long)]
+ config: PathBuf,
</file context>
There was a problem hiding this comment.
Fixed with a new scenario rather than by rewiring every existing one: live-chaos 10-colocated-node boots vtop-node node hosting both roles (composed by the same config emitters the split-process scenarios use, per-role observability stripped since the runner rejects it), gates on the single shared /readyz, asserts one /metrics scrape carries BOTH planes (vtop_meta_raft_state and vtop_broker_local_committed_offset), drives admin RPCs and a produce through it, then SIGKILLs the process and verifies every acknowledged record survives the shared-fate restart byte-exactly. The existing scenarios keep the split topology deliberately — chaos that kills one plane at a time needs separate processes — so the six-to-three claim is validated by 10 while 00-09 keep exercising the distributed shape.
| // Fail loudly rather than picking a winner. A config that names three | ||
| // listen addresses and gets one is a config whose author has a wrong | ||
| // model of the process. | ||
| if self.meta.observability.listen.is_some() || self.data.observability.listen.is_some() { |
There was a problem hiding this comment.
P2: An empty per-role observability: {} or listen: null is accepted and ignored, contrary to the documented contract that any per-role observability block is rejected. Preserve field presence during deserialization and reject a supplied block, not only a non-null listen address.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/vtop-node/src/colocated.rs, line 57:
<comment>An empty per-role `observability: {}` or `listen: null` is accepted and ignored, contrary to the documented contract that any per-role observability block is rejected. Preserve field presence during deserialization and reject a supplied block, not only a non-null listen address.</comment>
<file context>
@@ -0,0 +1,177 @@
+ // Fail loudly rather than picking a winner. A config that names three
+ // listen addresses and gets one is a config whose author has a wrong
+ // model of the process.
+ if self.meta.observability.listen.is_some() || self.data.observability.listen.is_some() {
+ return Err(
+ "a co-located node exposes ONE observability endpoint: set it at the top \
</file context>
There was a problem hiding this comment.
Fixed by making presence survive deserialization: the per-role observability fields are now Option<ObservabilityConfig>, and the co-located validation rejects is_some() — so observability: {} and observability: { listen: null } are refused exactly like a bound address, because an author who wrote the key at all believed the role owns an endpoint, and silently ignoring that belief hides the wrong model instead of correcting it. Test an_empty_per_role_observability_block_is_still_rejected pins it; the standalone meta/data commands default an absent block as before.
c1da48f to
9954d9e
Compare
97898d8 to
d4205c4
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d4205c43d0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| local id="$1"; shift | ||
| local cfg="$WORKDIR/colocated-$id.yaml" | ||
| emit_meta_config "$id" "$@" > /dev/null | ||
| emit_leader_config standalone > /dev/null |
There was a problem hiding this comment.
When adding a second co-located host in the chaos harness, every call still uses emit_leader_config standalone, which writes the singleton data config (data-leader, LEADER_UUID, native_addr, replica_addr 0, and data_metrics_addr 0) regardless of the metadata id passed above. That makes start_colocated_node 2 ... try to bind the same data/metrics ports, or reuse the same segment directory if run sequentially, so the helper cannot exercise the advertised three-process deployment; it needs per-id data UUIDs, dirs, and ports.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. emit_colocated_config no longer reuses the singleton standalone config: node uuid, certificate, data directory (data-colocated-$id), native port (colocated_native_addr, one per host), replica-status port, and the observability endpoint all derive from the id, and ids outside 1-3 are refused loudly. One honest scope note, in the helper where it belongs: each host still carries an independent STANDALONE range, because a replicated range under co-location needs follower-side epoch propagation — that is #239, and scenario 10 keeps validating the process shape (one scrape, one /readyz conjunction, shared fate) until it lands.
| echo "meta:" | ||
| # `peers:` with no entries is YAML null, which the typed config refuses; | ||
| # dropping the bare key gives the field its (empty) default instead. | ||
| sed -e '/^observability:/d' -e '/^peers:$/d' -e 's/^/ /' "$WORKDIR/meta-$id.yaml" |
There was a problem hiding this comment.
When this helper is called with any peer ids, this sed unconditionally deletes the peers: key but leaves the indented - { ... } entries emitted underneath it. The generated meta: section then contains orphaned list items under the previous field and vtop-node node --config fails YAML parsing instead of starting a multi-node co-located metadata cluster. Only drop the bare key when there are no peers, or keep peers: with its entries.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed — the peers: key is now dropped only when the helper is called with NO peer ids (where the bare key would deserialize as YAML null and be refused); with peers present the key and its entries pass through intact, so a multi-node co-located metadata group parses. The comment at the sed says both halves of the reason.
9954d9e to
1b63b31
Compare
d4205c4 to
343d934
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
1b63b31 to
42f00bd
Compare
Carries the first of #215's remaining gaps: "the live metadata and data processes are still separate". A live 3-node cluster meant six processes — a `meta` and a `data` invocation per machine, with separate configs, ready markers, and `/metrics` endpoints. That is not how anyone deploys this, and the gap mattered for more than tidiness: the harness never exercised the two planes sharing a process, a runtime, and a fate. `vtop-node node --config` runs both. One registry, one gate, one endpoint, so an operator scraping a host finds one target and does not have to know which roles happen to share it. A per-role `observability` block is rejected rather than silently ignored — a config naming three listen addresses and getting one belongs to an author with a wrong model of the process, and picking a winner would hide that. Shared fate is explicit: whichever role exits first ends the process, carrying its error. A metadata voter that has died inside a process still serving data is worse than a dead process, because the cluster keeps counting it toward quorum while it answers nothing. Exiting makes the failure legible to whatever supervises the node. What co-location deliberately does NOT change: the data plane still reaches metadata through the admin endpoint exactly as it would across a network, even when that endpoint is this same process. An in-memory shortcut would make the co-located path diverge from the distributed one precisely where the harness is meant to prove they agree. Mechanically this splits each role's `run` into a `serve` that takes a caller-owned observability surface, and moves the readiness probe behind a shared cell so a probe can be installed after the endpoint is already serving and so every role can hold a borrow rather than one of them owning it.
343d934 to
8003ea2
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
The live-chaos harness drives real processes over real TLS — metadata Raft, replication, fencing, promotion, failover — and it has never run in CI. No workflow referenced it. It executed only when someone ran it by hand. That gap is not theoretical. Scenario 09 sat red on main from #236, scenario 10 from #237, a data node presented a metadata node's certificate to the admin endpoint across three issues, and scenario 01 failed on a scheduling race that appeared and vanished between runs. Every one of those was found by running the suite manually this week, not by CI, and each had been on main for weeks. The harness was not linted either: the `shell` filter listed `docker/*.sh` and `docker/tests/**`, so editing `lib.sh` — which all thirteen scenarios source — triggered nothing at all. Both are now covered: * `shell` gains `scripts/live-chaos/**`, and a second shellcheck step lints the harness with `-x -P scripts/live-chaos/scenarios` so the scenarios' dynamic `source` resolves. Without `-P`, every scenario reports SC1091 plus spurious "may not be assigned" warnings for variables lib.sh does define — noise that trains people to ignore the job. The tree is clean under it. * A new `chaos` filter and `live-chaos` job run the suite. Scoped, not blanket: only the harness itself and the four crates whose binaries it executes can change a scenario's outcome, so a docs or dashboard edit does not pay for a cluster. Debug binaries, because these scenarios assert correctness rather than throughput. Unprivileged user and mount namespaces are enabled and then PROBED. Ubuntu 24.04 restricts unprivileged userns by default, and scenarios 05, 05b, and 06 need it. The probe does not skip on failure: these scenarios fail with a remediation message rather than skipping, so a runner without namespaces turns the job red honestly instead of reporting success for three scenarios that never ran — which is exactly the silent gap this job exists to close. Per-scenario logs upload on failure. A failing scenario names a log file, and without the artifact that diagnosis dies with the runner. Enabled only now because the suite had to be green first: #251, #252, and #253 fixed the three real failures. Local state is 9 of 13 passing, with the four remaining failures all requiring Linux facilities absent on macOS — `unshare` for 05, 05b, 06 and a clock shim for 07 — which is what this job exists to exercise. Scenario 01 was run five times after the race fix: 5 of 5.
The live-chaos harness drives real processes over real TLS — metadata Raft, replication, fencing, promotion, failover — and it has never run in CI. No workflow referenced it. It executed only when someone ran it by hand. That gap is not theoretical. Scenario 09 sat red on main from #236, scenario 10 from #237, a data node presented a metadata node's certificate to the admin endpoint across three issues, and scenario 01 failed on a scheduling race that appeared and vanished between runs. Every one of those was found by running the suite manually this week, not by CI, and each had been on main for weeks. The harness was not linted either: the `shell` filter listed `docker/*.sh` and `docker/tests/**`, so editing `lib.sh` — which all thirteen scenarios source — triggered nothing at all. Both are now covered: * `shell` gains `scripts/live-chaos/**`, and a second shellcheck step lints the harness with `-x -P scripts/live-chaos/scenarios` so the scenarios' dynamic `source` resolves. Without `-P`, every scenario reports SC1091 plus spurious "may not be assigned" warnings for variables lib.sh does define — noise that trains people to ignore the job. The tree is clean under it. * A new `chaos` filter and `live-chaos` job run the suite. Scoped, not blanket: only the harness itself and the four crates whose binaries it executes can change a scenario's outcome, so a docs or dashboard edit does not pay for a cluster. Debug binaries, because these scenarios assert correctness rather than throughput. Unprivileged user and mount namespaces are enabled and then PROBED. Ubuntu 24.04 restricts unprivileged userns by default, and scenarios 05, 05b, and 06 need it. The probe does not skip on failure: these scenarios fail with a remediation message rather than skipping, so a runner without namespaces turns the job red honestly instead of reporting success for three scenarios that never ran — which is exactly the silent gap this job exists to close. Per-scenario logs upload on failure. A failing scenario names a log file, and without the artifact that diagnosis dies with the runner. Enabled only now because the suite had to be green first: #251, #252, and #253 fixed the three real failures. Local state is 9 of 13 passing, with the four remaining failures all requiring Linux facilities absent on macOS — `unshare` for 05, 05b, 06 and a clock shim for 07 — which is what this job exists to exercise. Scenario 01 was run five times after the race fix: 5 of 5.
* ci: run and lint the live-chaos harness, path-gated The live-chaos harness drives real processes over real TLS — metadata Raft, replication, fencing, promotion, failover — and it has never run in CI. No workflow referenced it. It executed only when someone ran it by hand. That gap is not theoretical. Scenario 09 sat red on main from #236, scenario 10 from #237, a data node presented a metadata node's certificate to the admin endpoint across three issues, and scenario 01 failed on a scheduling race that appeared and vanished between runs. Every one of those was found by running the suite manually this week, not by CI, and each had been on main for weeks. The harness was not linted either: the `shell` filter listed `docker/*.sh` and `docker/tests/**`, so editing `lib.sh` — which all thirteen scenarios source — triggered nothing at all. Both are now covered: * `shell` gains `scripts/live-chaos/**`, and a second shellcheck step lints the harness with `-x -P scripts/live-chaos/scenarios` so the scenarios' dynamic `source` resolves. Without `-P`, every scenario reports SC1091 plus spurious "may not be assigned" warnings for variables lib.sh does define — noise that trains people to ignore the job. The tree is clean under it. * A new `chaos` filter and `live-chaos` job run the suite. Scoped, not blanket: only the harness itself and the four crates whose binaries it executes can change a scenario's outcome, so a docs or dashboard edit does not pay for a cluster. Debug binaries, because these scenarios assert correctness rather than throughput. Unprivileged user and mount namespaces are enabled and then PROBED. Ubuntu 24.04 restricts unprivileged userns by default, and scenarios 05, 05b, and 06 need it. The probe does not skip on failure: these scenarios fail with a remediation message rather than skipping, so a runner without namespaces turns the job red honestly instead of reporting success for three scenarios that never ran — which is exactly the silent gap this job exists to close. Per-scenario logs upload on failure. A failing scenario names a log file, and without the artifact that diagnosis dies with the runner. Enabled only now because the suite had to be green first: #251, #252, and #253 fixed the three real failures. Local state is 9 of 13 passing, with the four remaining failures all requiring Linux facilities absent on macOS — `unshare` for 05, 05b, 06 and a clock shim for 07 — which is what this job exists to exercise. Scenario 01 was run five times after the race fix: 5 of 5. * ci: run CI on every pull request, not only those targeting main `pull_request: branches: [main]` meant a stacked PR ran no CI whatsoever. This project stacks slices — each bases on the one below — so those PRs carried a single check, an AI reviewer's comment, and nothing else. They could be reviewed, approved, and merged with no build, no tests, and no lint. Nor did merging the parent repair it. Retargeting a PR's base fires the `edited` activity type, which is not in the default `pull_request` event set, so the retargeted PR did not reliably re-trigger CI either. The first real signal arrived on push to main — after the merge, on the branch that is supposed to stay green. The path filters already scope each run, so the added cost is only the jobs a stacked slice actually needs. This is the same shape as the gap the previous commit closes: a check that exists and is trusted, but never runs on the change it is meant to guard. * ci: make the live-chaos log artifact actually contain logs The upload step collected nothing. Scenarios delete workdirs they generated themselves, so by the time the step ran the `/tmp/vtop-chaos.*` tree was gone — and `if-no-files-found: ignore` turned that into a silent success. The first run confirmed it: the job passed and produced no artifact at all. A diagnostic that appears to exist and is absent exactly when it is needed is worse than none, because it stops anyone from looking for a real one. Supplying CHAOS_WORKDIR fixes both halves: the logs land in a known path, and because the harness only cleans up directories it generated itself, providing one suppresses the cleanup. `if-no-files-found` becomes `error`, so a future break in this wiring fails the job instead of quietly returning to collecting nothing. Upload is now `if: failure()` — the logs are for diagnosing a failed run, and uploading them on every green run is storage for nobody. Verified by running a scenario with CHAOS_WORKDIR set and confirming the log tree survives. * ci: widen the chaos filter to the real dependency closure, and gate the upload correctly Two review findings, both correct. The chaos filter named four crates and asserted that nothing else could change a scenario's outcome. That assertion was false. The two binaries this job runs pull in every workspace crate: cargo tree -p vtop-node --edges normal → broker, log, meta, observe, protocol cargo tree -p vtop-cli --no-default-features --edges normal → adapters, broker, core, log, meta, observe, protocol, state, upload The omissions were not marginal. vtop-log owns the segment format the durability and recovery scenarios assert on, and vtop-protocol owns the produce/fetch wire every scenario speaks — either could have broken the suite with this job never running, which is precisely the hole this job exists to close. The filter is now `crates/**`, derived from the closure rather than from a guess about it. That means the suite runs on most Rust changes. That is the honest cost of it being the only thing validating replication and fencing against real processes; a cheaper gate with a hole in it is what was just removed. The log upload was gated on `failure()`, which is also true when the namespace probe or the build failed. In those cases the suite never reached a scenario, so no logs exist, and `if-no-files-found: error` would stack a spurious "no files found" on top of the actual cause. It now keys on the run step's own outcome, so the empty-artifact signal only fires once a run was actually attempted — which is the only situation in which it means anything.
A StatefulSet of vtop-node node processes — the co-located shape #237 built — with a headless Service for stable peer DNS, per-plane volume claims, meta node ids derived from the pod ordinal, and a config-checksum annotation so config changes roll the set. Readiness is /readyz and the chart documents that a fenced node going unready is correct behaviour to route around, not a probe to loosen; liveness is /healthz; a startup probe covers cold recovery. Nothing identity-shaped has a default: TLS secret names, the cluster id, per-replica broker UUIDs, range identities, and the client principal are all required, refuse rendering with the exact contract in the message, and are shape-checked by values.schema.json — a chart that defaulted any of them would be shipping credentials, which is issue #81's lesson. The PodDisruptionBudget holds maxUnavailable at 1 because three voters are a quorum; pods run non-root with a read-only rootfs; the ServiceMonitor is gated behind both a value and the CRD's presence.
1 participant
First of #215's remaining gaps: "the live metadata and data processes are still separate".
A live 3-node cluster meant six processes — a
metaand adatainvocation per machine, with separate configs, ready markers, and/metricsendpoints. That is not how anyone deploys this, and the gap mattered for more than tidiness: the harness never exercised the two planes sharing a process, a runtime, and a fate.vtop-node node --configruns both.One observability surface
An operator scraping a host finds one target and does not have to know which roles happen to share it. A per-role
observabilityblock is rejected rather than silently ignored — a config naming three listen addresses and getting one belongs to an author with a wrong model of the process, and picking a winner would hide that.Shared fate, made explicit
Whichever role exits first ends the process, carrying its error. A metadata voter that has died inside a process still serving data is worse than a dead process: the cluster keeps counting it toward quorum while it answers nothing. Exiting makes the failure legible to whatever supervises the node.
What co-location deliberately does not change
The data plane still reaches metadata through the admin endpoint exactly as it would across a network, even when that endpoint is this same process. An in-memory shortcut would make the co-located path diverge from the distributed one precisely where the harness is meant to prove they agree.
Mechanics
Each role's
runsplits into aservetaking a caller-owned observability surface, and the readiness probe moves behind a shared cell — so a probe can be installed after the endpoint is already serving, and every role holds a borrow rather than one of them owning it.Refs #215.
Summary by cubic
Co-locates the metadata voter and data replica in one
vtop-nodeprocess to match real deployments and exercise shared runtime/fate (addresses #215). Adds a single observability endpoint with readiness as the conjunction of both roles, plus a chaos scenario that proves the single endpoint and shared‑fate restart.New Features
vtop-node node --configruns metadata and data in one process./metricsand/readyz; readiness opens only when both roles have started.10-colocated-nodeverifies single endpoint, readiness conjunction, admin RPCs, and post-restart data integrity.Migration
observabilityblock; per-roleobservabilityis rejected (including an empty{}).metaanddatacommands are unchanged; each still exposes its own endpoint.Written for commit 8003ea2. Summary will update on new commits.