Skip to content

fix(cli): mount the Blazegraph journal on a named volume, and never guess its name - #2093

Closed
branarakic wants to merge 1 commit into
mainfrom
fix/blazegraph-journal-volume
Closed

fix(cli): mount the Blazegraph journal on a named volume, and never guess its name#2093
branarakic wants to merge 1 commit into
mainfrom
fix/blazegraph-journal-volume

Conversation

@branarakic

Copy link
Copy Markdown
Contributor

Summary

The daemon's Blazegraph container-recovery path can silently destroy a node's entire graph. This fixes it.

provisionBlazegraphDocker creates the container with -d --restart --name -p <image> and no -v
(blazegraph-docker.ts step 4),
so the journal lives in the container's writable layer. The image pins
com.bigdata.journal.AbstractJournal.file=/data/bigdata.jnl in WEB-INF/classes/RWStore.properties, so everything
durable is under /data.

Step 3 of the same function issues docker rm -f whenever a stopped container fails to docker start, and then
falls through to that create path:

const startResult = await docker.run(['start', containerName]);
if (startResult.exitCode !== 0) {
  log(`  docker start failed (...); recreating.`);
  await docker.run(['rm', '-f', containerName]);   // journal gone
}
// ... step 4: docker run, no -v

The node comes back serving zero triples, with no error surfaced.

Why this is not hypothetical

The V10 mainnet fleet runs with store.options.managedByDkg = true (verified on sbb and dmaast), so this code owns
those containers. The fleet's journals were migrated to a named volume out-of-band:

$ docker volume ls
DRIVER    VOLUME NAME
local     dkg-blazegraph-data

$ docker inspect dkg-blazegraph-dkg --format '{{range .Mounts}}{{.Name}}:{{.Destination}}{{end}}'
dkg-blazegraph-data:/data

So the data currently survives — but only by luck. The daemon doesn't know the volume exists and would recreate
without mounting it, orphaning 11–18 GB of graph per node behind an empty store.

Changes

1. Always mount a named volume at /data on create. docker run -v <name>:<path> creates the volume on demand
and reuses it with its contents when it already exists, so this is correct for both fresh and already-provisioned
hosts.

2. Resolve the volume from the container's own .Mounts, never from the container name. This is the important
one. The fleet's containers are named dkg-blazegraph-dkg while the volume is dkg-blazegraph-data — so a
`${containerName}-data` derivation produces dkg-blazegraph-dkg-data, a volume that does not exist and which
docker would helpfully create empty. The container is the only authoritative source for where its own journal
lives.

Note for #1817, which is unmerged and contains exactly that derivation
(blazegraphVolumeName(c) => \${c}-data`): it would misclassify every fleet node as needing migration and then recreate onto an empty volume. That PR should adopt the .Mounts` resolution from this one before it lands.

3. Fail closed instead of recreating over a mount we cannot reproduce. If something non-volume (e.g. an operator
bind mount) is at /data and the container won't start, throw rather than rm -f. Recovering one stopped container
by hand is strictly cheaper than silently emptying a store.

Testing

Three new tests in packages/cli/test/blazegraph-docker.test.ts:

  • fresh create mounts dkg-blazegraph-data:/data and keeps the image as the final argument
  • recreate reuses the existing volume, explicitly asserting it is not the container-name-derived form
  • bind mount at /data → throws, and asserts no rm and no run were issued

22 passed in the file; tsc --noEmit clean.

Mutation-verified: reintroducing const journalVolume = `${containerName}-data` fails two of the three new
tests. The guards have teeth rather than merely passing.

Risk

Low. Behaviour is unchanged for a running container (the reuse path is untouched). The only new failure mode is the
deliberate fail-closed throw, which replaces a silent data-availability loss with an actionable error.

Existing fleet containers already mount dkg-blazegraph-data:/data, so this change is a no-op for them until
something triggers a recreate — which is precisely the case it makes safe.

…uess its name

The daemon's container-recovery path can destroy a node's entire graph.

`provisionBlazegraphDocker` creates the Blazegraph container with
`-d --restart --name -p <image>` and no `-v`, so the journal
(`/data/bigdata.jnl`, pinned by the image in
`WEB-INF/classes/RWStore.properties`) lives in the container's writable
layer. Step 3 of the same function does `docker rm -f` whenever a stopped
container fails to `docker start`, then falls through to that create path.
The result is silent, total data unavailability for the node: it comes back
serving zero triples.

This is reachable on mainnet today. The V10 fleet runs with
`store.options.managedByDkg = true`, and its containers were migrated to a
named volume out-of-band, so the journal survives — but the daemon does not
know about it and would recreate without mounting it, orphaning 11-18 GB of
graph while starting an empty store.

Two changes:

1. Always mount a named volume at `/data` on create. `docker run -v` creates
   the volume on demand and reuses it (with its contents) when it exists, so
   this is safe for both fresh and previously-provisioned hosts.

2. Resolve the volume from the container's own `.Mounts`, never from the
   container name. The fleet is the reason: containers are named
   `dkg-blazegraph-dkg` while the journal volume is `dkg-blazegraph-data`, so
   a `${containerName}-data` derivation yields `dkg-blazegraph-dkg-data` —
   a volume that does not exist, which docker would helpfully create empty.
   The container is the only authoritative source for where its journal is.

Also fail closed rather than recreating over a mount we cannot reproduce: if
something non-volume (e.g. an operator bind mount) is mounted at `/data` and
the container will not start, throw instead of `rm -f`. Recovering one
stopped container by hand is strictly cheaper than silently emptying a store.

Tests cover the fresh-create mount, the fleet's container/volume name
mismatch, and the bind-mount refusal (asserting no `rm` and no `run` are
issued). Verified by mutation: reintroducing the `${containerName}-data`
derivation fails two of them.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
// Journal volume used by any container created below. An existing container
// is authoritative about where its own journal lives; the default name is
// only for a store that does not have one yet.
const journalVolume = inspectInfo.dataVolume ?? DEFAULT_BLAZEGRAPH_VOLUME;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Bug: Fresh containers all share the same Blazegraph journal volume

What's wrong
DEFAULT_BLAZEGRAPH_VOLUME is used whenever the target container does not already exist or lacks an inspectable named volume. Because the provisioner supports namespace-specific container names and port auto-bumping, two independently provisioned nodes on the same Docker host will mount the same named volume at /data. Blazegraph's journal is a single durable store file, so sharing it between independent containers can expose the wrong namespaces and risks concurrent journal corruption.

Example
Call provisionBlazegraphDocker({ namespace: 'alice' }), then call provisionBlazegraphDocker({ namespace: 'bob', port: 10000 }) while the first container still exists. The second inspect checks dkg-blazegraph-bob, sees no container, and runs a second Blazegraph JVM with -v dkg-blazegraph-data:/data. Both containers now share and can write the same /data/bigdata.jnl; expected behavior is isolated journals per independent container, or refusing to attach an in-use/shared journal.

Suggested direction
Separate the volume-selection path for existing-container recovery from fresh provisioning. Preserve mounted volumes discovered from .Mounts, but do not use one global writable volume for every new namespace/container; either scope the default volume to the namespace/container or make sharing an explicit, validated option.

For Agents
In packages/cli/src/daemon/blazegraph-docker.ts, keep inspectInfo.dataVolume authoritative when recreating an existing container, but make the default for a truly fresh container unique to that logical store, or explicitly detect/refuse when the default volume is already attached to another container. Add a regression test that provisions two different namespaces/container names and proves they do not receive the same writable /data volume unless the caller explicitly requests sharing.

// `dkg-blazegraph-dkg-data`, which does not exist — docker would create it
// empty and the node would come up serving zero triples while the real
// 11-18 GB journal sat orphaned.
const FLEET_VOLUME = 'dkg-blazegraph-data';

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Bug: The existing-volume regression test does not prove the inspected volume is used

What's wrong
This test is intended to verify that recreate uses the volume discovered from docker inspect, but the mocked volume name equals the default volume. That gives false confidence for the main changed behavior: a regression that stops reading the container's .Mounts would still be green.

Example
Change the production code to const journalVolume = DEFAULT_BLAZEGRAPH_VOLUME; and this test still passes, so it does not prove that an existing container's inspected Mounts[].Name is reused.

Suggested direction
Use a non-default inspected volume name, for example operator-blazegraph-journal, so the test fails if the implementation falls back to the default instead of reading .Mounts.

For Agents
In packages/cli/test/blazegraph-docker.test.ts, make the inspected volume name distinct from DEFAULT_BLAZEGRAPH_VOLUME and assert docker run -v uses that exact custom name. Preserve the separate assertion that the container-name-derived dkg-blazegraph-dkg-data is not used.

* container has one. This is the ONLY trustworthy way to learn where an
* existing container's journal lives.
*/
dataVolume?: string;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Issue: Model the journal mount as one explicit state instead of loose optional flags

What's wrong
The PR introduces a real domain concept, "what journal store should a replacement container use?", but represents it as two independent optional fields plus a later defaulting expression and a separate guard. That spreads one invariant across comments and control flow, making future edits easier to get subtly wrong and forcing readers to reconstruct which combinations are valid.

Example
The current shape allows impossible or ambiguous states such as { dataVolume: 'custom', foreignDataMount: true } or { dataVolume: undefined, foreignDataMount: undefined }, and the recreate path has to remember that one means reuse/refuse/default depending on context. A discriminated model would make those states unrepresentable.

Suggested direction
Move the mount interpretation into a small helper or discriminated union such as { kind: 'named-volume'; name } | { kind: 'foreign' } | { kind: 'default' }. Then make the recreate path switch on that value. This deletes the parallel optional fields and keeps the volume-selection policy in one place.

For Agents
In packages/cli/src/daemon/blazegraph-docker.ts, replace dataVolume?: string and foreignDataMount?: boolean with a single journal mount model returned by inspect parsing, e.g. named-volume, foreign, or default/missing. Preserve behavior: reuse the inspected named volume on recreate, use DEFAULT_BLAZEGRAPH_VOLUME for fresh creates, and refuse to recreate non-volume mounts after docker start fails. Existing volume tests should still pass, with one focused test proving the foreign state cannot fall through to docker run.

@branarakic

Copy link
Copy Markdown
Contributor Author

Closing as superseded by #2086 / #2137, which are now merged to main (f58537862 + review follow-up 271147ade).

The data-loss path this PR targets no longer exists on main:

  • dockerRunArgs always mounts a named volume at the declared dataPath (--mount type=volume,source=<container>-data,target=/data), so fresh creates are durable.
  • The stopped-container recovery path is fail-closed: the provisioner only issues docker rm -f when the journal is confirmed to live in the expected named volume (which the recreate then reattaches). Otherwise it throws with backup/migration guidance instead of recreating. The docker start fails → rm -f → volumeless recreate → empty store sequence cannot occur.

One idea from this PR is deliberately not on main: resolving the volume from the container's own .Mounts instead of deriving ${containerName}-data. Because of the fail-closed guard, the mismatch is no longer a data-loss risk — but fleet-shaped containers (e.g. dkg-blazegraph-dkg with the out-of-band dkg-blazegraph-data:/data volume) are treated as legacy: they get a spurious volume warning on every provision and require manual recovery if docker start fails, rather than auto-recreating with their real volume reattached. If that auto-recovery matters for the fleet, .Mounts-based adoption deserves a small follow-up PR against current main (this branch predates the #2086 rewrite of blazegraph-docker.ts and would conflict wholesale). Note for that follow-up: this PR's fixed shared volume name would collide on multi-container hosts, so the merged per-container naming is the right base, with .Mounts adoption layered on top for containers whose actual volume differs.

@branarakic branarakic closed this Aug 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants