-
Notifications
You must be signed in to change notification settings - Fork 10
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.
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
base: main
Are you sure you want to change the base?
Conversation
…ting it from an image each time.
There was a problem hiding this comment.
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 modifies the run.sh script to enable persistent Docker containers for iterative development by removing the --rm flag and adding logic to check for and reuse existing containers.
Changes:
- Removed
--rmflag from docker run command to allow container persistence after exit - Added container existence checking logic to reuse running or stopped containers before creating new ones
- Implemented different attachment logic for running vs stopped containers
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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 | ||
| fi | ||
| exit 0 | ||
| fi |
Copilot
AI
Jan 12, 2026
There was a problem hiding this comment.
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.
| 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 |
Copilot
AI
Jan 12, 2026
There was a problem hiding this comment.
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.
| 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 |
| docker start $CONTAINER_NAME | ||
| sleep 2 | ||
| docker exec -it $CONTAINER_NAME bash /docker-entrypoint.sh |
Copilot
AI
Jan 12, 2026
There was a problem hiding this comment.
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.
| 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 |
Description
This PR modifies
run.shto support iterative development in a local container by making two key changes:--rmflag (line 104) from thedocker runcommand, allowing the container to persist after exit for inspection and reuse.These changes are especially helpful for developers who are new to the project, as they enable a stable, long-term development environment within a single container, eliminating the need to rebuild from scratch for every test.