diff --git a/dev/freesurfer-volume-versioning.md b/dev/freesurfer-volume-versioning.md new file mode 100644 index 00000000..54305a19 --- /dev/null +++ b/dev/freesurfer-volume-versioning.md @@ -0,0 +1,52 @@ +# FreeSurfer volume versioning + +## Problem + +The FreeSurfer install (`/usr/local/freesurfer/`) is shared into the simnibs +container through a Docker named volume. Docker only seeds a volume from the +image **once**, when the volume is first created empty. The volume had a fixed +name (`ti-toolbox_freesurfer_data`), so once a user created it, it was never +refreshed — even after pulling a newer freesurfer image. Users who installed +before the image gained the MATLAB runtime (MCR) kept an MCR-less volume, which +broke `segmentThalamicNuclei.sh` / `segmentHA_T1.sh` (GitHub issue #123). + +## Fix + +1. **Version the volume name by the freesurfer image tag** + (`ti-toolbox_freesurfer_data_v7.4.1`). Bumping the image bumps the name, so a + new empty volume is created and re-seeded from the new image. +2. **Make the volume compose-managed (not `external`)** so `docker compose up` + creates and seeds it automatically — no launcher pre-step, and a raw + `docker compose up` works too. +3. **Launchers derive the name** from the compose image tag, pass it as + `FREESURFER_VOLUME`, and prune stale older-version volumes (best-effort). + +`docker compose down` keeps the volume; only `down -v` removes it. + +## Files changed + +| File | Change | +|------|--------| +| `docker-compose.yml` | `freesurfer_data` volume: drop `external`, `name: ${FREESURFER_VOLUME:-ti-toolbox_freesurfer_data_v7.4.1}` | +| `package/docker/docker-compose.yml` | same | +| `dev/loader/docker-compose.dev.yml` | same | +| `loader.py` | `get_freesurfer_volume_name()` + `prune_old_freesurfer_volumes()`; set `FREESURFER_VOLUME`; removed manual volume create | +| `dev/loader/loader_dev.py` | same | +| `package/src/backend/docker-manager.js` | `computeFreesurferVolume()` + `pruneOldVolumes()`; inject `FREESURFER_VOLUME`; removed `ensureVolume()` | + +## When bumping the FreeSurfer image + +Update the freesurfer `image:` tag **and** the matching `${FREESURFER_VOLUME:-...}` +default in all three compose files. The launchers read the tag automatically; the +default only matters for a bare `docker compose up`. + +## Existing users (one-time) + +Their old `ti-toolbox_freesurfer_data` volume is replaced automatically on next +launch (new versioned volume is seeded fresh; the launcher prunes the old one). +A manual reset also works: + +```bash +docker rm -f simnibs_container freesurfer_container +docker volume rm ti-toolbox_freesurfer_data +``` diff --git a/dev/loader/docker-compose.dev.yml b/dev/loader/docker-compose.dev.yml index b55ff030..49817dae 100644 --- a/dev/loader/docker-compose.dev.yml +++ b/dev/loader/docker-compose.dev.yml @@ -4,7 +4,7 @@ services: platform: linux/amd64 container_name: freesurfer_container volumes: - - ti-toolbox_freesurfer_data:/usr/local/freesurfer/ + - freesurfer_data:/usr/local/freesurfer/ networks: - ti_network restart: unless-stopped @@ -14,7 +14,7 @@ services: image: idossha/simnibs:v2.3.2 container_name: simnibs_container volumes: - - ti-toolbox_freesurfer_data:/usr/local/freesurfer/ + - freesurfer_data:/usr/local/freesurfer/ - /tmp/.X11-unix:/tmp/.X11-unix - ${LOCAL_PROJECT_DIR}:/mnt/${PROJECT_DIR_NAME} - ${TIT_USER_CONFIG}:/root/.config/ti-toolbox # User-level config (telemetry, prefs) @@ -55,6 +55,7 @@ networks: driver: bridge volumes: - ti-toolbox_freesurfer_data: - external: true - name: ti-toolbox_freesurfer_data + freesurfer_data: + # Versioned name (must match the freesurfer image tag) so a FreeSurfer bump + # re-seeds a fresh volume. Launcher overrides FREESURFER_VOLUME. + name: ${FREESURFER_VOLUME:-ti-toolbox_freesurfer_data_v7.4.1} diff --git a/dev/loader/loader_dev.py b/dev/loader/loader_dev.py index 9f279dd4..f4bcff15 100644 --- a/dev/loader/loader_dev.py +++ b/dev/loader/loader_dev.py @@ -16,6 +16,7 @@ SCRIPT_DIR = Path(__file__).resolve().parent DEFAULT_PATHS_FILE = SCRIPT_DIR / ".default_paths.dev" DOCKER_COMPOSE_FILE = SCRIPT_DIR / "docker-compose.dev.yml" +FREESURFER_VOLUME_PREFIX = "ti-toolbox_freesurfer_data" TOOLBOX_ROOT = (SCRIPT_DIR / ".." / "..").resolve() STATUS_RELATIVE_PATH = Path("code/ti-toolbox/config/project_status.json") SYSTEM_INFO_RELATIVE_DIR = Path("derivatives/ti-toolbox/.ti-toolbox-info") @@ -166,6 +167,43 @@ def initialize_project_structure(project_dir: Path) -> None: initializer.setup_example_data(str(TOOLBOX_ROOT), project_dir) +def get_freesurfer_volume_name() -> Optional[str]: + """Versioned FreeSurfer volume name from the compose image tag. + + Returns ``None`` if the image tag cannot be parsed. Callers must then leave + ``FREESURFER_VOLUME`` unset (so compose's versioned default seeds the correct + volume) and skip pruning, rather than falling back to the bare prefix. + """ + for line in DOCKER_COMPOSE_FILE.read_text().splitlines(): + match = re.match(r"^\s*image:\s*\S*ti-toolbox_freesurfer:(\S+)\s*$", line) + if match: + return f"{FREESURFER_VOLUME_PREFIX}_{match.group(1)}" + return None + + +def prune_old_freesurfer_volumes(current_name: Optional[str]) -> None: + """Remove stale older-version FreeSurfer volumes (best-effort). + + A ``None`` ``current_name`` means the active version is unknown (tag parse + failed); pruning is skipped so a good versioned volume is never destroyed. + """ + if not current_name: + return + try: + names = capture(["docker", "volume", "ls", "--format", "{{.Name}}"]).split() + except Exception: + return + for name in names: + is_versioned = name.startswith(f"{FREESURFER_VOLUME_PREFIX}_") + is_legacy = name == FREESURFER_VOLUME_PREFIX + if (is_versioned or is_legacy) and name != current_name: + subprocess.run( + ["docker", "volume", "rm", name], + stdout=subprocess.DEVNULL, + stderr=subprocess.DEVNULL, + ) + + def get_compose_images() -> list[str]: images: list[str] = [] for line in DOCKER_COMPOSE_FILE.read_text().splitlines(): @@ -309,6 +347,23 @@ def main() -> None: env["TIT_HOST_OS"] = platform.system().lower() # darwin, linux, windows env["TIT_HOST_OS_VERSION"] = platform.release() env["TIT_HOST_ARCH"] = platform.machine() # x86_64, arm64 + + freesurfer_volume = get_freesurfer_volume_name() + if freesurfer_volume: + env["FREESURFER_VOLUME"] = freesurfer_volume + + # Bring any containers from a previous (older-image) run down first so they + # release the old FreeSurfer volume; otherwise the prune below fails with + # "volume is in use" and the stale volume lingers. + subprocess.run( + ["docker", "compose", "-f", str(DOCKER_COMPOSE_FILE), "down"], + env=env, + stdout=subprocess.DEVNULL, + stderr=subprocess.DEVNULL, + check=False, + ) + prune_old_freesurfer_volumes(freesurfer_volume) + ensure_images_pulled(env) run_docker_compose(env, dev_codebase_dir) diff --git a/docker-compose.yml b/docker-compose.yml index 1467908e..97d921ea 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -4,7 +4,7 @@ services: platform: linux/amd64 container_name: freesurfer_container volumes: - - ti-toolbox_freesurfer_data:/usr/local/freesurfer/ + - freesurfer_data:/usr/local/freesurfer/ networks: - ti_network restart: unless-stopped @@ -14,7 +14,7 @@ services: image: idossha/simnibs:v2.3.2 container_name: simnibs_container volumes: - - ti-toolbox_freesurfer_data:/usr/local/freesurfer/ + - freesurfer_data:/usr/local/freesurfer/ - /tmp/.X11-unix:/tmp/.X11-unix:rw # X11 socket mount for GUI - ${LOCAL_PROJECT_DIR}:/mnt/${PROJECT_DIR_NAME} - ${TIT_USER_CONFIG}:/root/.config/ti-toolbox # User-level config (telemetry, prefs) @@ -51,6 +51,7 @@ networks: driver: bridge volumes: - ti-toolbox_freesurfer_data: - external: true - name: ti-toolbox_freesurfer_data + freesurfer_data: + # Versioned name (must match the freesurfer image tag) so a FreeSurfer bump + # re-seeds a fresh volume. Launchers override FREESURFER_VOLUME. + name: ${FREESURFER_VOLUME:-ti-toolbox_freesurfer_data_v7.4.1} diff --git a/loader.py b/loader.py index adadccf8..b32bcfd8 100644 --- a/loader.py +++ b/loader.py @@ -16,6 +16,7 @@ SCRIPT_DIR = Path(__file__).resolve().parent DEFAULT_PATHS_FILE = SCRIPT_DIR / ".default_paths.user" DOCKER_COMPOSE_FILE = SCRIPT_DIR / "docker-compose.yml" +FREESURFER_VOLUME_PREFIX = "ti-toolbox_freesurfer_data" X11_MARKER_NAME = ".ti_toolbox_x11_initialized" X11_MARKER_DIR = Path("code/ti-toolbox/config") @@ -218,14 +219,41 @@ def maybe_init_macos_x11( print("XQuartz: ensure 'Allow connections from network clients' is enabled.") -def ensure_docker_volume(volume_name: str) -> None: - result = subprocess.run( - ["docker", "volume", "inspect", volume_name], - stdout=subprocess.DEVNULL, - stderr=subprocess.DEVNULL, - ) - if result.returncode != 0: - run(["docker", "volume", "create", volume_name], check=False) +def get_freesurfer_volume_name() -> Optional[str]: + """Versioned FreeSurfer volume name from the compose image tag. + + Returns ``None`` if the image tag cannot be parsed. Callers must then leave + ``FREESURFER_VOLUME`` unset (so compose's versioned default seeds the correct + volume) and skip pruning, rather than falling back to the bare prefix. + """ + for line in DOCKER_COMPOSE_FILE.read_text().splitlines(): + match = re.match(r"^\s*image:\s*\S*ti-toolbox_freesurfer:(\S+)\s*$", line) + if match: + return f"{FREESURFER_VOLUME_PREFIX}_{match.group(1)}" + return None + + +def prune_old_freesurfer_volumes(current_name: Optional[str]) -> None: + """Remove stale older-version FreeSurfer volumes (best-effort). + + A ``None`` ``current_name`` means the active version is unknown (tag parse + failed); pruning is skipped so a good versioned volume is never destroyed. + """ + if not current_name: + return + try: + names = capture(["docker", "volume", "ls", "--format", "{{.Name}}"]).split() + except Exception: + return + for name in names: + is_versioned = name.startswith(f"{FREESURFER_VOLUME_PREFIX}_") + is_legacy = name == FREESURFER_VOLUME_PREFIX + if (is_versioned or is_legacy) and name != current_name: + subprocess.run( + ["docker", "volume", "rm", name], + stdout=subprocess.DEVNULL, + stderr=subprocess.DEVNULL, + ) def get_compose_images() -> list[str]: @@ -300,9 +328,24 @@ def run_project_init_in_container(container_name: str, project_dir_name: str) -> def run_docker_compose(project_dir: Path, project_dir_name: str) -> None: - ensure_docker_volume("ti-toolbox_freesurfer_data") + freesurfer_volume = get_freesurfer_volume_name() env = os.environ.copy() + if freesurfer_volume: + env["FREESURFER_VOLUME"] = freesurfer_volume + + # Bring any containers from a previous (older-image) run down first so they + # release the old FreeSurfer volume; otherwise the prune below fails with + # "volume is in use" and the stale volume lingers. + subprocess.run( + ["docker", "compose", "-f", str(DOCKER_COMPOSE_FILE), "down"], + env=env, + stdout=subprocess.DEVNULL, + stderr=subprocess.DEVNULL, + check=False, + ) + prune_old_freesurfer_volumes(freesurfer_volume) + env["LOCAL_PROJECT_DIR"] = str(project_dir) env["PROJECT_DIR_NAME"] = project_dir_name env["TZ"] = get_host_timezone() diff --git a/package/docker/docker-compose.yml b/package/docker/docker-compose.yml index db454bfd..6a3665db 100644 --- a/package/docker/docker-compose.yml +++ b/package/docker/docker-compose.yml @@ -6,7 +6,7 @@ services: platform: linux/amd64 container_name: freesurfer_container volumes: - - ti-toolbox_freesurfer_data:/usr/local/freesurfer/ + - freesurfer_data:/usr/local/freesurfer/ networks: - ti_network restart: unless-stopped @@ -16,7 +16,7 @@ services: image: idossha/simnibs:v2.3.2 container_name: simnibs_container volumes: - - ti-toolbox_freesurfer_data:/usr/local/freesurfer/ + - freesurfer_data:/usr/local/freesurfer/ - /tmp/.X11-unix:/tmp/.X11-unix:rw # X11 socket mount for Linux/macOS - ${LOCAL_PROJECT_DIR}:/mnt/${PROJECT_DIR_NAME} - ${TIT_USER_CONFIG}:/root/.config/ti-toolbox # User-level config (telemetry, prefs) @@ -54,7 +54,8 @@ networks: driver: bridge volumes: - ti-toolbox_freesurfer_data: - external: true - name: ti-toolbox_freesurfer_data + freesurfer_data: + # Versioned name (must match the freesurfer image tag) so a FreeSurfer bump + # re-seeds a fresh volume. Launchers override FREESURFER_VOLUME. + name: ${FREESURFER_VOLUME:-ti-toolbox_freesurfer_data_v7.4.1} diff --git a/package/src/backend/docker-manager.js b/package/src/backend/docker-manager.js index dd1cd582..013d0ee0 100644 --- a/package/src/backend/docker-manager.js +++ b/package/src/backend/docker-manager.js @@ -8,7 +8,7 @@ const { getExeca } = require('./execa'); const { ensurePathEnv } = require('./env'); const MANAGED_CONTAINERS = ['simnibs_container', 'freesurfer_container']; -const FREESURFER_VOLUME = 'ti-toolbox_freesurfer_data'; +const FREESURFER_VOLUME_PREFIX = 'ti-toolbox_freesurfer_data'; function toExecEnv(env = {}) { return Object.entries(env) @@ -25,6 +25,7 @@ class DockerManager extends EventEmitter { super(); this.toolboxRoot = toolboxRoot; this.originalComposeFile = path.join(toolboxRoot, 'docker-compose.yml'); + this.freesurferVolume = this.computeFreesurferVolume(); this.composeFile = this.getAccessibleComposeFile(); this.docker = this.createDockerClient(); this.globalExecEnv = ensurePathEnv(process.env); @@ -51,6 +52,27 @@ class DockerManager extends EventEmitter { } } + computeFreesurferVolume() { + // Versioned FreeSurfer volume name from the compose image tag. + // Returns null if the tag can't be parsed; callers then leave + // FREESURFER_VOLUME unset (so compose's versioned default seeds the correct + // volume) and skip pruning, rather than falling back to the bare prefix and + // destroying the good versioned volume. + // The ^\s* + /m anchor mirrors the Python launchers: a commented-out + // "# image:" line is not matched, and only the first real image line wins. + try { + const content = fs.readFileSync(this.originalComposeFile, 'utf8'); + const match = content.match(/^\s*image:\s*\S*ti-toolbox_freesurfer:(\S+)/m); + if (match) { + return `${FREESURFER_VOLUME_PREFIX}_${match[1].trim()}`; + } + logger.warn('Could not parse FreeSurfer image tag; leaving FREESURFER_VOLUME unset and skipping volume prune.'); + } catch (error) { + logger.warn(`Could not parse FreeSurfer image tag for volume name: ${error.message}`); + } + return null; + } + createDockerClient() { if (process.env.DOCKER_HOST) { return new Docker(this.parseDockerHost(process.env.DOCKER_HOST)); @@ -181,21 +203,49 @@ class DockerManager extends EventEmitter { ); } - async ensureVolume() { - this.emitProgress('volume', 'Ensuring Freesurfer data volume exists…'); + // Build a compose env that only sets FREESURFER_VOLUME when the tag was + // parsed. When it is null we leave the var unset so compose's versioned + // ${FREESURFER_VOLUME:-ti-toolbox_freesurfer_data_v7.4.1} default applies. + composeEnv(env) { + if (this.freesurferVolume) { + return ensurePathEnv({ ...env, FREESURFER_VOLUME: this.freesurferVolume }); + } + return ensurePathEnv(env); + } + + async pruneOldVolumes() { + // Remove stale older-version FreeSurfer volumes (best-effort). + // Skip entirely when the active version is unknown (tag parse failed); + // otherwise we would delete every versioned volume, including the good one. + if (!this.freesurferVolume) { + logger.warn('FreeSurfer volume prune skipped: active version unknown.'); + return; + } try { - await this.docker.createVolume({ Name: FREESURFER_VOLUME }); - } catch (error) { - if (error.statusCode !== 409) { - throw error; + const { Volumes } = await this.docker.listVolumes(); + const stale = (Volumes || []) + .map(volume => volume.Name) + .filter(name => + name !== this.freesurferVolume && + (name === FREESURFER_VOLUME_PREFIX || name.startsWith(`${FREESURFER_VOLUME_PREFIX}_`)) + ); + for (const name of stale) { + try { + await this.docker.getVolume(name).remove(); + this.emitProgress('volume', `Removed stale FreeSurfer volume ${name}`); + } catch (error) { + logger.warn(`Could not remove old volume ${name}: ${error.message}`); + } } + } catch (error) { + logger.warn(`FreeSurfer volume prune skipped: ${error.message}`); } } async composeUp(env) { this.emitProgress('compose-up', 'Starting Docker Compose services…'); const execa = await getExeca(); - const execEnv = ensurePathEnv(env); + const execEnv = this.composeEnv(env); const dockerPath = this.findDockerCommand(); const subprocess = execa( @@ -242,7 +292,7 @@ class DockerManager extends EventEmitter { } const execa = await getExeca(); - const execEnv = ensurePathEnv(env); + const execEnv = this.composeEnv(env); const dockerPath = this.findDockerCommand(); await execa(dockerPath, ['compose', '-f', this.composeFile, 'down', '--remove-orphans'], { env: execEnv, @@ -283,7 +333,7 @@ class DockerManager extends EventEmitter { this.emitProgress('cleanup', 'Cleaning up stale containers…'); await this.cleanupExistingContainers(); - await this.ensureVolume(); + await this.pruneOldVolumes(); await this.composeUp(env); await this.waitForContainer('simnibs_container'); }