Skip to content

fix(launcher): reuse MPC container across restarts to preserve logs - #3705

Merged
barakeinav1 merged 2 commits into
mainfrom
barak/stabilize-launcher-compose-project
Jul 1, 2026
Merged

fix(launcher): reuse MPC container across restarts to preserve logs#3705
barakeinav1 merged 2 commits into
mainfrom
barak/stabilize-launcher-compose-project

Conversation

@barakeinav1

@barakeinav1 barakeinav1 commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Closes #3695.

The launcher recreated the MPC container on every boot (docker rm -f + docker compose up -d), wiping its logs. Run Compose under a stable project (-p mpc) and drop the rm -f, so the container is reused across restarts and its logs persist — viewable via the existing dstack log interface. No name-conflict cleanup is needed: a launcher change re-keys the CVM disk, so a new launcher always boots on a fresh disk.

Scope: restart only. Logs are still reset on upgrade (new image digest → container recreated) — tracked in #3702.

Verified on a dstack TDX localnet: restart → logs preserved; upgrade → logs reset.

The launcher removed and recreated the MPC container on every boot
(docker rm -f + docker compose up -d), dropping Docker's container-ID-keyed
logs each restart. The rm -f worked around a name conflict from running as a
different compose project every boot (random temp compose path, no -p).

Pass an explicit `-p mpc` compose project so Compose reuses the existing
container when the config is unchanged (preserving its logs) and recreates it
only when the rendered config changes (e.g. image digest). No name-conflict
cleanup is needed: a launcher change re-keys the CVM disk (the key is derived
from the launcher's compose measurement), so a new launcher always boots on a
fresh disk with no prior mpc-node container.

Also note in the TDX guide that logs persist across a restart but are cleared
on an upgrade.
@barakeinav1
barakeinav1 force-pushed the barak/stabilize-launcher-compose-project branch from 9fa37d8 to 3e679db Compare July 1, 2026 08:39
@barakeinav1
barakeinav1 marked this pull request as ready for review July 1, 2026 08:40
Copilot AI review requested due to automatic review settings July 1, 2026 08:40
@claude

claude Bot commented Jul 1, 2026

Copy link
Copy Markdown

Pull request overview

Fixes a log-loss issue in the TEE launcher: on every boot the launcher was force-removing the mpc-node container and then invoking docker compose up -d without a project name, which caused Compose to always recreate the container and wipe its logs. This PR removes the docker rm -f step and pins a stable Compose project (-p mpc), so Compose reuses the container across restarts (same rendered config → no recreation) while still recreating on upgrade (image digest changes → config changes). Docs are updated to describe the new log-retention semantics.

Changes:

  • crates/tee-launcher/src/compose.rs: drop pre-launch docker rm -f, pass -p mpc to docker compose up.
  • docs/running-an-mpc-node-in-tdx-external-guide.md: note that restarts preserve logs while upgrades clear them.

Reviewed changes

Per-file summary
File Description
crates/tee-launcher/src/compose.rs Removed docker rm -f mpc-node and added -p mpc project flag to docker compose up -d.
docs/running-an-mpc-node-in-tdx-external-guide.md Added blockquote clarifying restart-vs-upgrade log retention behavior.

Findings

Non-blocking (nits, follow-ups, suggestions):

  • crates/tee-launcher/src/compose.rs:66 — The Compose project name "mpc" is a magic literal, whereas the closely related MPC_CONTAINER_NAME lives in crates/tee-launcher/src/constants.rs:1. Consider promoting it (e.g. MPC_COMPOSE_PROJECT_NAME: &str = "mpc") to keep all Docker-facing identifiers in one place — this also makes it easy to reference from a future test.
  • crates/tee-launcher/src/compose.rs:65-71 — The existing tests only exercise render_compose_file; there is no assertion that launch_mpc_container invokes docker compose with -p mpc. Since the whole fix hinges on that flag being present and stable, a small regression test (e.g. extract the argv-building into a pure helper and assert it contains ["-p", "mpc"]) would guard against future refactors silently reintroducing the log-loss behavior.
  • crates/tee-launcher/src/compose.rs:62-64 — The comment is helpful. Minor: it could also note the operational assumption stated in the PR description ("a launcher upgrade re-keys the CVM disk, so there is no legacy container to conflict on first boot") — that is the reason the docker rm -f safety net can be removed, and it's non-obvious from the code alone. Optional; the PR body captures it already.

✅ Approved

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the TEE launcher’s Docker Compose invocation to reuse the existing MPC container across restarts (so Docker’s container-ID-keyed logs persist), and documents the resulting log-retention behavior for operators using dstack/TDX.

Changes:

  • Run docker compose up under a stable project name (-p mpc) and stop force-removing the MPC container on each boot.
  • Document that logs persist across restarts but reset on upgrades (container recreation due to new image digest).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
docs/running-an-mpc-node-in-tdx-external-guide.md Adds a note clarifying restart vs upgrade log-retention behavior in the dstack log UI section.
crates/tee-launcher/src/compose.rs Switches Compose to a stable project (-p mpc) and removes unconditional container deletion to preserve logs across restarts.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread crates/tee-launcher/src/compose.rs Outdated
Comment on lines 62 to 67
// Use a stable compose project (`-p`) so the container is reused across
// restarts rather than recreated, preserving its logs. Compose
// recreates it only when the rendered config (e.g. image digest) changes.
let run_output = Command::new("docker")
.args(["compose", "-f", &compose_path, "up", "-d"])
.args(["compose", "-p", "mpc", "-f", &compose_path, "up", "-d"])
.output()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks — this is the right concern on a general Docker host, but it doesn't apply to our deployment, so the one-time cleanup is intentionally left out:

  • TEE (production): the CVM disk is encrypted with a key derived from the launcher's docker-compose measurement (see docs/securing-mpc-with-tee-design-doc.md, Launcher Pattern, step 3). A launcher change ⇒ different measurement ⇒ different key ⇒ the old disk can't be decrypted, so a new launcher always boots on a fresh disk with no pre-existing mpc-node container to collide with.
  • The only situation this would guard is an in-place launcher upgrade that reuses the disk (e.g. NonTee on a persistent host), which isn't a supported flow for us.
  • Node-image upgrades keep the same launcher (same compose project label), so there's no conflict on that path either.

@barakeinav1

Copy link
Copy Markdown
Contributor Author

Thanks for the review. On the non-blocking nits — leaving the code as-is, with rationale:

  • (a) Compose project constant: "mpc" is kept inline on purpose. Without a migration-cleanup step it would be a single-use constant, and the literal sits alongside the other inline docker args ("compose", "-f", …); the comment above the call explains the -p rationale.
  • (b) -p mpc regression test: fair that the fix hinges on the flag, but the only clean way to assert it is re-introducing an arg-builder indirection we deliberately inlined. For a single, commented flag in a one-line .args([...]), a dedicated test felt like over-engineering, and a dropped flag would be caught in review. Noted as the thing to revisit if this area grows.
  • (c) disk-rekey comment: intentionally omitted — that assumption holds only in TEE (not NonTee), so stating it in-code would be misleading; the PR description carries the reasoning.

Comment thread crates/tee-launcher/src/compose.rs Outdated
// recreates it only when the rendered config (e.g. image digest) changes.
let run_output = Command::new("docker")
.args(["compose", "-f", &compose_path, "up", "-d"])
.args(["compose", "-p", "mpc", "-f", &compose_path, "up", "-d"])

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: you can instead having project name in yaml itself: name: mpc

https://docs.docker.com/reference/compose-file/version-and-name/#name-top-level-element

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good call — done in b23ac3e: moved the project name into the compose file as a top-level name: mpc-node and dropped the -p flag, so it's declarative and now covered by the render tests. Thanks!

anodar
anodar previously approved these changes Jul 1, 2026
gilcu3
gilcu3 previously approved these changes Jul 1, 2026

@gilcu3 gilcu3 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you!

Comment thread crates/tee-launcher/src/compose.rs Outdated
Set the project name declaratively as a top-level `name: mpc-node` in the
rendered compose file instead of passing `-p mpc` on the command line (per
review). The project identity now travels with the compose config and is
covered by render tests. Behaviour is unchanged: a stable project name keeps
the container reused across restarts, preserving its logs.
@barakeinav1
barakeinav1 dismissed stale reviews from gilcu3 and anodar via b23ac3e July 1, 2026 11:48

@anodar anodar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you!

@barakeinav1
barakeinav1 added this pull request to the merge queue Jul 1, 2026
Merged via the queue into main with commit fdeefd5 Jul 1, 2026
15 checks passed
@barakeinav1
barakeinav1 deleted the barak/stabilize-launcher-compose-project branch July 1, 2026 12:44
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.

add support for keeping MPC container logs (in dstack) after a restart

4 participants