feat:Added bind mount support to allow for persistent storage#65
feat:Added bind mount support to allow for persistent storage#65MS-githubaccnt wants to merge 4 commits intodevelopfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds persistent storage support for created containers by introducing an optional “volume needed” flag, provisioning a per-project loopback-backed mount under /mnt/storage, and wiring a storage-capacity pre-check into the backend create flow.
Changes:
- Frontend: add a “persistent storage” option to the create modal and include it in the create request payload.
- Backend: pass the volume flag through to
container.shand add a disk-space check via a newcanAllocateStorage()helper. - Host pipe listener + shell scripts: add a response mechanism (
RESPOND::+output_pipe) and mount/unmount/delete logic for project storage.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/frontend/src/utils/create.ts | Sends volume_needed and handles INSUFFICIENT_STORAGE responses. |
| src/frontend/src/components/modal.vue | Adds UI to choose persistent storage and passes it to create(). |
| src/cli/features/createDomain.ts | Attempts to surface insufficient storage errors in CLI flow. |
| src/backend/utils/container-storage.ts | New helper to check available disk space via host pipe response. |
| src/backend/shell_scripts/delete.sh | Unmounts and deletes project storage on domain deletion. |
| src/backend/shell_scripts/container.sh | Creates/mounts loopback image and bind-mounts into container when enabled. |
| src/backend/scripts.ts | Converts volume_needed to boolean string and passes into container.sh. |
| src/backend/main.ts | Runs storage check before provisioning when volume requested. |
| docs/admin/README.md | Documents new output_pipe requirement. |
| docker/named_pipe/listen.sh | Adds RESPOND:: handling + changes execution model for host commands. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if [ "$enable_volume" = "true" ]; then | ||
| echo "Creating persistent storage at $PROJECT_STORAGE" | ||
|
|
||
| if [ ! -f "$PROJECT_IMG" ]; then | ||
| sudo dd if=/dev/zero of=$PROJECT_IMG bs=1M count=$SIZE_MB | ||
| sudo mkfs.ext4 $PROJECT_IMG | ||
| fi | ||
| sudo mkdir -p $PROJECT_STORAGE | ||
| if ! mount | grep -q "$PROJECT_STORAGE"; then | ||
| sudo mount -o loop $PROJECT_IMG $PROJECT_STORAGE | ||
| fi | ||
| sudo chmod 777 $PROJECT_STORAGE | ||
| fi | ||
| sudo docker build -t $name . | ||
| sudo docker run --memory=$max_mem --name=$name -d -p ${available_ports[$AVAILABLE]}:$exp_port $name | ||
| if [ "$enable_volume" = "true" ]; then | ||
| sudo docker run \ | ||
| --memory=$max_mem \ | ||
| --name=$name \ | ||
| -d \ | ||
| -p ${available_ports[$AVAILABLE]}:$exp_port \ | ||
| -v $PROJECT_STORAGE:/app/data \ | ||
| -e DATA_DIR=/app/data \ |
There was a problem hiding this comment.
The volume-creation/mount logic uses mount | grep for mount detection and passes unquoted paths into dd, mkfs, mount, chmod, and docker run -v. This can mis-detect mounts (substring matches) and is vulnerable to shell globbing if $name contains ? (allowed by current backend escaping). Use a robust mount check (mountpoint -q), and quote $PROJECT_STORAGE/$PROJECT_IMG/$name in all commands and docker -v arguments.
| const memLimit = shellEscape(MEMORY_LIMIT || "512m", "MEMORY_LIMIT"); | ||
|
|
||
| volume_needed=(volume_needed=="Yes").toString(); | ||
| const volumeNeeded=shellEscape(volume_needed,"false"); |
There was a problem hiding this comment.
The label passed to shellEscape should describe the field being escaped. Using "false" here will produce confusing error messages (e.g. "Invalid characters in false"); pass a label like "volume_needed" instead.
| const volumeNeeded=shellEscape(volume_needed,"false"); | |
| const volumeNeeded=shellEscape(volume_needed,"volume_needed"); |
|
|
||
| volume_needed=(volume_needed=="Yes").toString(); | ||
| const volumeNeeded=shellEscape(volume_needed,"false"); | ||
| console.log(`volume neeeded is ${volumeNeeded}`); |
There was a problem hiding this comment.
Typo in log message: "volume neeeded" -> "volume needed".
| console.log(`volume neeeded is ${volumeNeeded}`); | |
| console.log(`volume needed is ${volumeNeeded}`); |
| cmd="${raw##RESPOND::}" | ||
| eval "$cmd" > output_pipe 2>&1 | ||
| else | ||
| eval "$raw" & |
There was a problem hiding this comment.
Backgrounding every non-RESPOND:: command (eval "$raw" &) changes host execution from sequential to concurrent. This is risky because the backend uses shared /hostpipe files (e.g. .env, Dockerfile, .dockerignore) when triggering container.sh, so concurrent runs can overwrite each other and produce corrupted builds/deployments. If concurrency isn’t explicitly handled end-to-end, keep execution sequential (no &) or introduce proper queuing/isolation (per-request working dirs/files).
| eval "$raw" & | |
| eval "$raw" |
| if (response.data.status === 'success') { | ||
| console.log(`✅ Domain '${subdomain}.${domain}' created successfully!`); | ||
| } else { | ||
| if(response.error=="INSUFFICIENT_STORAGE")console.log("INSUFFICIENT_STORAGE") |
There was a problem hiding this comment.
response.error is not an axios response field, so this insufficient-storage check will never trigger. Use response.data.error (and/or check response.data.status) when inspecting backend error codes.
| if(response.error=="INSUFFICIENT_STORAGE")console.log("INSUFFICIENT_STORAGE") | |
| if (response.data.error === "INSUFFICIENT_STORAGE") console.log("INSUFFICIENT_STORAGE") |
| export async function canAllocateStorage(requestedMb: number) { | ||
| const STORAGE_PATH = "/mnt/storage"; | ||
| const SAFETY_BUFFER_MB = 200; // keep buffer for system + docker | ||
|
|
||
| try { | ||
| const responseProcess = new Deno.Command("sh", { | ||
| args: ["-c", "cat /hostpipe/output_pipe"], | ||
| }).output(); // don't await yet, just start it | ||
| await exec(`bash -c "echo 'RESPOND::df ${STORAGE_PATH} --output=avail' > /hostpipe/pipe"`); | ||
| const response=await responseProcess; | ||
| const output = new TextDecoder().decode(response.stdout).trim().split("\n"); | ||
| const availableKb = parseInt(output[1].trim()); | ||
| if (isNaN(availableKb)) { | ||
| throw new Error(`Unexpected df output: ${output}`); | ||
| } | ||
| const availableMb = Math.floor(availableKb / 1024); | ||
| const usableMb = availableMb - SAFETY_BUFFER_MB; | ||
| const canAllocate = usableMb >= requestedMb; | ||
| console.log(`can allocate ${canAllocate} memory`); | ||
| console.log(`Available memory is ${availableMb} requested is ${requestedMb}`); | ||
| return { | ||
| can_allocate: canAllocate, | ||
| available_mb: usableMb, | ||
| requested_mb: requestedMb, | ||
| reason: canAllocate ? null : "Not enough disk space", | ||
| }; |
There was a problem hiding this comment.
This cat /hostpipe/output_pipe approach can hang indefinitely (no timeout) and is unsafe under concurrency: multiple simultaneous canAllocateStorage calls share a single FIFO, so one request can consume another request’s output (or deadlock if the writer blocks). Consider using a per-request response channel (unique FIFO/file) with correlation IDs and add a timeout/abort path so storage checks can’t stall request handling.
| export async function canAllocateStorage(requestedMb: number) { | |
| const STORAGE_PATH = "/mnt/storage"; | |
| const SAFETY_BUFFER_MB = 200; // keep buffer for system + docker | |
| try { | |
| const responseProcess = new Deno.Command("sh", { | |
| args: ["-c", "cat /hostpipe/output_pipe"], | |
| }).output(); // don't await yet, just start it | |
| await exec(`bash -c "echo 'RESPOND::df ${STORAGE_PATH} --output=avail' > /hostpipe/pipe"`); | |
| const response=await responseProcess; | |
| const output = new TextDecoder().decode(response.stdout).trim().split("\n"); | |
| const availableKb = parseInt(output[1].trim()); | |
| if (isNaN(availableKb)) { | |
| throw new Error(`Unexpected df output: ${output}`); | |
| } | |
| const availableMb = Math.floor(availableKb / 1024); | |
| const usableMb = availableMb - SAFETY_BUFFER_MB; | |
| const canAllocate = usableMb >= requestedMb; | |
| console.log(`can allocate ${canAllocate} memory`); | |
| console.log(`Available memory is ${availableMb} requested is ${requestedMb}`); | |
| return { | |
| can_allocate: canAllocate, | |
| available_mb: usableMb, | |
| requested_mb: requestedMb, | |
| reason: canAllocate ? null : "Not enough disk space", | |
| }; | |
| const STORAGE_CHECK_TIMEOUT_MS = 5000; | |
| let storagePipeLock: Promise<void> = Promise.resolve(); | |
| async function withStoragePipeLock<T>(operation: () => Promise<T>): Promise<T> { | |
| const previousLock = storagePipeLock; | |
| let releaseLock!: () => void; | |
| storagePipeLock = new Promise<void>((resolve) => { | |
| releaseLock = resolve; | |
| }); | |
| await previousLock; | |
| try { | |
| return await operation(); | |
| } finally { | |
| releaseLock(); | |
| } | |
| } | |
| export async function canAllocateStorage(requestedMb: number) { | |
| const STORAGE_PATH = "/mnt/storage"; | |
| const SAFETY_BUFFER_MB = 200; // keep buffer for system + docker | |
| try { | |
| return await withStoragePipeLock(async () => { | |
| const abortController = new AbortController(); | |
| const timeoutId = setTimeout(() => abortController.abort(), STORAGE_CHECK_TIMEOUT_MS); | |
| try { | |
| const responseProcess = new Deno.Command("sh", { | |
| args: ["-c", "cat /hostpipe/output_pipe"], | |
| signal: abortController.signal, | |
| }).output(); // don't await yet, just start it | |
| await exec(`bash -c "echo 'RESPOND::df ${STORAGE_PATH} --output=avail' > /hostpipe/pipe"`); | |
| const response = await responseProcess; | |
| const output = new TextDecoder().decode(response.stdout).trim().split("\n"); | |
| if (output.length < 2) { | |
| throw new Error(`Unexpected df output: ${output}`); | |
| } | |
| const availableKb = parseInt(output[1].trim(), 10); | |
| if (isNaN(availableKb)) { | |
| throw new Error(`Unexpected df output: ${output}`); | |
| } | |
| const availableMb = Math.floor(availableKb / 1024); | |
| const usableMb = availableMb - SAFETY_BUFFER_MB; | |
| const canAllocate = usableMb >= requestedMb; | |
| console.log(`can allocate ${canAllocate} memory`); | |
| console.log(`Available memory is ${availableMb} requested is ${requestedMb}`); | |
| return { | |
| can_allocate: canAllocate, | |
| available_mb: usableMb, | |
| requested_mb: requestedMb, | |
| reason: canAllocate ? null : "Not enough disk space", | |
| }; | |
| } finally { | |
| clearTimeout(timeoutId); | |
| } | |
| }); |
|
|
||
| if echo "$raw" | grep -q "^RESPOND::"; then | ||
| cmd="${raw##RESPOND::}" | ||
| eval "$cmd" > output_pipe 2>&1 |
There was a problem hiding this comment.
Writing command output to a single global output_pipe FIFO can block the listener (FIFO writers block until a reader is attached) and interleave/misroute responses when multiple backend requests are in-flight. If you need request/response semantics, prefer a non-blocking mechanism (e.g., write to a regular file with a unique name/correlation ID, or create a per-request FIFO) and ensure the listener can’t be stalled by a missing reader.
| eval "$cmd" > output_pipe 2>&1 | |
| response_file=$(mktemp) | |
| eval "$cmd" > "$response_file" 2>&1 | |
| ( | |
| while ! mkdir output_pipe.lock 2>/dev/null; do | |
| sleep 1 | |
| done | |
| cat "$response_file" > output_pipe | |
| rm -f "$response_file" | |
| rmdir output_pipe.lock | |
| ) & |
| <div class="volume-needed"> | ||
| <label for="volume">Do you need persistent storage (Volume)?</label><br> | ||
| <input name="radio" type="radio" value="Yes" v-model="volume_needed"> Yes | ||
| <input name="radio" type="radio" value="No" v-model="volume_needed"> No | ||
| </div> |
There was a problem hiding this comment.
The new radio inputs reuse name="radio", which groups them with the other Yes/No radios in this modal (static content, dockerfile present), so selecting one option can unexpectedly deselect others. Give each radio group a distinct name (e.g. volume_needed) and wire the <label for="..."> to an actual input id (or use <fieldset>/<legend>) to keep the form accessible and behaving correctly.
| if mount | grep -q "$PROJECT_STORAGE"; then | ||
| echo "Unmounting volume..." | ||
| sudo umount $PROJECT_STORAGE | ||
| fi | ||
|
|
||
| if [ -d "$PROJECT_STORAGE" ]; then | ||
| sudo rm -rf $PROJECT_STORAGE | ||
| fi | ||
|
|
||
| if [ -f "$PROJECT_IMG" ]; then | ||
| echo "Deleting volume image..." | ||
| sudo rm -f $PROJECT_IMG | ||
| fi |
There was a problem hiding this comment.
The new volume mount path checks/removals rely on mount | grep and use unquoted variables in umount/rm. This can produce false positives (substring matches) and can break if $name contains shell glob characters (e.g. ?, which is currently allowed by the backend’s shell escaping). Prefer mountpoint -q "$PROJECT_STORAGE" (or an exact-match grep), and quote $PROJECT_STORAGE/$PROJECT_IMG everywhere they’re used in commands.
|
@MS-githubaccnt could you please resolve the conflicts and look into the copilot suggestions? |
|
Running a bit busy will try to do this today |
415f12c to
c029dd2
Compare
|
check it out now @opbot-xd |
|
@opbot-xd I still need to test this waiting for Srishti to finish |
This PR adds bind mount support allowing persistent storage which did not previously exist.