Skip to content

Fix the issue where run.sh deletes the container upon exit after creating it from an image each time. #17

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
LittleRookie1115 wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fix the issue where run.sh deletes the container upon exit after creating it from an image each time. #17

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8825331
Fix the issue where run.sh deletes the container upon exit after crea…
LittleRookie1115 Jan 8, 2026
51d351f
Merge remote-tracking branch 'upstream/main'
LittleRookie1115 Jan 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 21 additions & 1 deletion docker/run.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,27 @@ echo "[*] XAUTHORITY=$XAUTHORITY"
echo "[*] XDG_RUNTIME_DIR=$XDG_RUNTIME_DIR"
xhost +local:docker 2>/dev/null || xhost + 2>/dev/null || echo "[*] Warning: Could not set xhost permissions"

docker run -it --rm \
# Check if container already exists
if docker ps -a --format '{{.Names}}' | grep -q "^${CONTAINER_NAME}$"; then
echo "[*] Container ${CONTAINER_NAME} already exists"

# Check if container is running
if docker ps --format '{{.Names}}' | grep -q "^${CONTAINER_NAME}$"; then
echo "[*] Container is already running, attaching..."
docker exec -it $CONTAINER_NAME bash /docker-entrypoint.sh
else
echo "[*] Container exists but is stopped, starting and attaching..."
docker start $CONTAINER_NAME
sleep 2
docker exec -it $CONTAINER_NAME bash /docker-entrypoint.sh
Comment on lines +111 to +116
Copy link

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

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

Running the docker-entrypoint.sh script multiple times via docker exec when attaching to an existing container will cause unintended side effects. The entrypoint script performs actions like apt install, modifying .bashrc, and setting up directories. Running it repeatedly will append duplicate 'source /opt/ros/humble/setup.bash' lines to .bashrc and re-run apt install on every attachment. Consider either: 1) using 'docker exec -it CONTAINER_NAME bash' without running the entrypoint for re-attachments, or 2) making the entrypoint script idempotent with guards to prevent duplicate operations.

Suggested change
docker exec -it $CONTAINER_NAME bash /docker-entrypoint.sh
else
echo "[*] Container exists but is stopped, starting and attaching..."
docker start $CONTAINER_NAME
sleep 2
docker exec -it $CONTAINER_NAME bash /docker-entrypoint.sh
docker exec -it $CONTAINER_NAME bash
else
echo "[*] Container exists but is stopped, starting and attaching..."
docker start $CONTAINER_NAME
sleep 2
docker exec -it $CONTAINER_NAME bash

Copilot uses AI. Check for mistakes.
Comment on lines +114 to +116
Copy link

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

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

Using a fixed 2-second sleep is fragile. On slower systems, the container might not be ready after 2 seconds, causing the subsequent docker exec to fail. On faster systems, this introduces unnecessary delay. Consider removing the sleep and relying on docker exec to fail and retry if needed, or implement a polling loop that waits for the container to reach a running state.

Suggested change
docker start $CONTAINER_NAME
sleep 2
docker exec -it $CONTAINER_NAME bash /docker-entrypoint.sh
docker start "$CONTAINER_NAME"
echo "[*] Waiting for container to be running..."
for i in {1..10}; do
if docker inspect -f '{{.State.Running}}' "$CONTAINER_NAME" 2>/dev/null | grep -q "true"; then
break
fi
sleep 1
done
docker exec -it "$CONTAINER_NAME" bash /docker-entrypoint.sh

Copilot uses AI. Check for mistakes.
fi
exit 0
fi
Comment on lines +105 to +119
Copy link

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

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

When reusing an existing container with docker exec, the script bypasses the volume mounts, environment variables, GPU arguments, and network settings that are only applied during container creation. This means if any of these configurations change (e.g., GPU_ARGS, NETWORK_ARGS, or volume paths), the existing container will continue using outdated settings. Consider adding a comment warning users that they need to use the -d flag to delete and recreate the container if they modify any docker run arguments, or implement a mechanism to detect configuration changes.

Copilot uses AI. Check for mistakes.

# If container doesn't exist, create new one
echo "[*] Creating new container..."

docker run -it \
--name $CONTAINER_NAME \
--hostname docker-ub \
$NETWORK_ARGS \
Expand Down
Loading