Modifies ROOT user related setup to Dockerfile.curobo#5376
Modifies ROOT user related setup to Dockerfile.curobo#5376njawale42 wants to merge 10 commits intoisaac-sim:developfrom
Conversation
The NVIDIA Isaac Sim base image declares no USER directive and defaults to root, so the explicit USER root in this Dockerfile is a no-op declaration. Removing it aligns Dockerfile.curobo with the upstream NVIDIA Isaac Sim Dockerfile and with the other Isaac Lab Dockerfiles (Dockerfile.ros2, Dockerfile.installci) which omit the directive. Build and runtime behavior are unchanged: the container still runs as root via the base image default.
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Summary
This PR makes two minor cleanup changes to docker/Dockerfile.curobo: (1) removes a redundant USER root directive since the base image already defaults to root, and (2) changes two hardcoded /root/.bashrc references to use ${HOME}/.bashrc for consistency with the 8 preceding lines in the same RUN block. The changes are correct and improve maintainability.
Architecture Impact
Self-contained. This is a Docker build configuration change that affects only the cuRobo container image build process. No runtime code, no Isaac Lab framework code, and no other Dockerfiles are impacted. The container behavior remains identical since ${HOME} resolves to /root in the current configuration.
Implementation Verdict
Ship it
Test Coverage
No tests required. This is a Docker configuration cleanup with no behavioral change. The PR author's claim that "the container still builds and runs identically" is correct — both changes are purely cosmetic consistency improvements. Manual verification that the container builds successfully would be the appropriate validation, which is covered by standard CI Docker builds if available.
CI Status
No CI checks available yet. Once CI runs, the Docker build job (if present) would validate this change.
Findings
🔵 Improvement: docker/Dockerfile.curobo:34 — Removal of USER root is correct
The NVIDIA Isaac Sim base image does not set a non-root user, so this directive was indeed a no-op. Removing it aligns with the upstream Dockerfile style and reduces noise.
🔵 Improvement: docker/Dockerfile.curobo:170-171 — Consistent use of ${HOME} is correct
The change from /root/.bashrc to ${HOME}/.bashrc makes the entire RUN block internally consistent. While HOME=/root in the current build context, using the variable is more maintainable if the base image ever changes or if the Dockerfile is adapted for non-root users in the future.
Verification note: I confirmed that ${HOME} is the correct variable to use here by examining the full file context. The DOCKER_USER_HOME environment variable (line 33) is a different variable intended for bind-mount paths in Singularity (lines 116-123), while ${HOME} is the standard shell variable used throughout the alias block. The PR correctly uses ${HOME} to match the existing pattern.
No critical or warning-level issues identified. This is a clean, low-risk cleanup PR.
Greptile SummaryThis PR refactors Confidence Score: 3/5Merging is risky while two previously-flagged P1 issues — pip cache mount target pointing to /root/.cache/pip when the user's $HOME is /home/isaaclab, and the broader $HOME/$DOCKER_USER_HOME divergence — remain open and unaddressed in the current HEAD. No new P0/P1 issues were found in this review pass; the single new finding is a P2 best-practice item (--no-log-init). However, two prior-thread P1 issues (pip cache mount no-op and HOME/DOCKER_USER_HOME mismatch) are still present in the code without resolution, keeping overall confidence below 4. docker/Dockerfile.curobo — specifically the --mount=type=cache,target=${DOCKER_USER_HOME}/.cache/pip line (mount target vs. user Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["FROM base image\n(USER isaac-sim, uid 1234)"] --> B["USER root\n(system setup)"]
B --> C["apt installs\nCUDA keyring\nnvidia-* placeholders\ntorch removal + pip reinstall"]
C --> D["groupadd --non-unique --gid 1000 isaaclab\nuseradd --non-unique --uid 1000 --gid 1000 isaaclab"]
D --> E["chown -R isaaclab:isaaclab\n ISAACLAB_PATH\n ISAACSIM_ROOT_PATH/kit/python\n DOCKER_USER_HOME (=/root)\n /home/isaaclab"]
E --> F["chmod 755 ISAACSIM_ROOT_PATH\n(opens top-level traversal)"]
F --> G["USER isaaclab\n(HOME=/home/isaaclab)"]
G --> H["--mount=type=cache,\ntarget=DOCKER_USER_HOME/.cache/pip\n(=/root/.cache/pip) ⚠️\nuid=1000,gid=1000\n\nisaaclab.sh --install"]
H --> I["pip uninstall quadprog\ncurobo install\nisaaclab_teleop install"]
I --> J["Write aliases to HOME/.bashrc\n(=/home/isaaclab/.bashrc ✓)"]
J --> K["COPY --chown=isaaclab:isaaclab\nall repo files into ISAACLAB_PATH"]
K --> L["WORKDIR ISAACLAB_PATH"]
style H fill:#ffe0cc,stroke:#d9534f
Reviews (8): Last reviewed commit: "Widens chown to /isaac-sim/kit/python so..." | Re-trigger Greptile |
…ile.curobo
The base image (nvcr.io/nvidian/isaac-sim:latest-develop) ends with
USER isaac-sim (uid 1234). Previously the Dockerfile switched back to
root for the entire build and left runtime as root via
OMNI_KIT_ALLOW_ROOT. Removing the initial USER root broke CI because
apt needs root; simply restoring it keeps us as root the whole way
which is what we want to avoid.
Correct pattern applied here:
* Explicit USER root for the system-level setup block (apt, cuda
repo, /bin/nvidia-* placeholders, /root cache dirs, get-pip
bootstrap, COPYed source files).
* After the last root-required step, chown only the paths we are
about to write to from the non-root user
(${ISAACLAB_PATH}, site-packages, ${DOCKER_USER_HOME}) to
isaac-sim:isaac-sim. /isaac-sim itself is already isaac-sim-owned
from the base image, so re-chowning it would needlessly bloat
the layer.
* USER isaac-sim before the editable pip installs
(isaaclab.sh --install, cuRobo, isaaclab_teleop) and the bashrc
writes, so those paths end up owned by the runtime user.
* BuildKit cache mount at ${DOCKER_USER_HOME}/.cache/pip pinned to
uid=1234,gid=1234, otherwise it would default to root-owned and
isaac-sim could not write into it.
* Final COPY annotated with --chown=isaac-sim:isaac-sim for
consistency.
This keeps CI build args (DOCKER_USER_HOME_ARG=/root) and
docker-compose paths unchanged, so docker-compose usage continues to
work with OMNI_KIT_ALLOW_ROOT. Dockerfile.base and Dockerfile.ros2
follow the same legacy pattern and are out of scope for this PR.
… compat
The previous attempt switched to the base image's isaac-sim user (uid 1234),
which broke the test step in CI: the test runner bind-mounts the host
workspace (/opt/github-runner/_work/.../IsaacLab, owned by uid 1000) over
/workspace/isaaclab, overlaying our build-time chown. As uid 1234, the
container could not write to the bind mount and `mkdir tests` failed before
pytest even started.
Fix: create a fresh isaaclab user with uid 1000 to match the runner host
user, instead of remapping isaac-sim. Creating a new user (rather than
remapping isaac-sim's UID) avoids triggering an OverlayFS copy-up of the
multi-GB Isaac Sim install in /isaac-sim, which would otherwise become
orphan-owned. /isaac-sim stays mode 755 and isaac-sim-owned, so isaaclab
can still read and execute everything there. Only the paths the new user
needs to write to (${ISAACLAB_PATH}, kit site-packages, ${DOCKER_USER_HOME},
/home/isaaclab) are chowned.
Also bumps the BuildKit pip cache mount uid/gid from 1234 to 1000 (numeric
form is required because BuildKit does not consult /etc/passwd at mount
time) and switches the final COPY --chown to isaaclab.
Caveat: hardcodes uid 1000, matching the typical EC2 self-hosted runner
user. If a runner is reconfigured to a different uid this will break the
same way; in that case the right next step is to parameterize via build
arg, but until then 1000 is the simplest correct value.
|
|
||
| RUN chown -R isaaclab:isaaclab \ | ||
| ${ISAACLAB_PATH} \ | ||
| ${ISAACSIM_ROOT_PATH}/kit/python/lib/python3.12/site-packages \ |
There was a problem hiding this comment.
Hardcoded Python version will break on base-image upgrade
python3.12 is hard-coded in the chown path. If the upstream Isaac Sim base image ships Python 3.13 (or any other version), this path won't exist and the RUN chown -R layer will fail at build time with "No such file or directory," breaking the entire image build.
Use a glob or a shell expansion to resolve the versioned directory dynamically:
| ${ISAACSIM_ROOT_PATH}/kit/python/lib/python3.12/site-packages \ | |
| $(ls -d ${ISAACSIM_ROOT_PATH}/kit/python/lib/python3.*/site-packages 2>/dev/null | head -1) \ |
The pinned cuRobo CI base image (nvcr.io/nvidian/isaac-sim:latest-develop)
recently rolled forward to a digest that already provisions a user at
uid 1000, causing the previous `useradd ... --uid 1000 isaaclab` to fail
with `useradd: UID 1000 is not unique`.
Adds --non-unique (-o) so the new isaaclab user can share uid 1000 with
the existing one. File ownership only cares about the numeric uid, so
chown of the writable paths to isaaclab:isaaclab still produces uid 1000
files. Build-time ${HOME}/.bashrc writes still resolve to /home/isaaclab
because USER isaaclab triggers a getpwnam lookup by name, returning our
entry rather than the colliding one.
`useradd --non-unique --uid 1000` only relaxes the UID uniqueness check; it does not help with the implicit groupadd that useradd runs internally to create the user's primary group. On base image revisions that already carry a group at gid 1000 (likely whenever a uid-1000 user also exists), that internal groupadd either fails outright or silently picks a different gid like 1001, leaving the BuildKit pip cache mount's `gid=1000` mismatched against the user's primary group. Fix: explicitly groupadd the isaaclab group with --non-unique --gid 1000 before useradd, and pass --gid 1000 to useradd so it reuses the new group as the user's primary group instead of auto-creating a per-user group. This deterministically lands isaaclab at uid 1000 / gid 1000 regardless of collisions in the base image.
The base image creates /isaac-sim as isaac-sim's home directory; on
recent Ubuntu (HOME_MODE 0700 in /etc/login.defs) that means mode 0700
isaac-sim:isaac-sim, which our new isaaclab user cannot enter. The
`[ -f $ISAACLAB_PATH/_isaac_sim/python.sh ]` test in isaaclab.sh then
fails, falls through to the system `python3` fallback, and the
isaaclab.sh --install step dies with `exec: python3: not found`.
Fix: chmod 755 /isaac-sim so non-owner users can traverse the directory
to reach python.sh and the rest of the Kit runtime. Files inside retain
their original permissions (NVIDIA ships them world-readable as part of
their standard distribution layout), so only the top-level directory's
mode needs to change. This is a single-inode metadata change; no
OverlayFS copy-up of the multi-GB tree.
If a future build surfaces a deeper permission issue inside /isaac-sim,
escalate to `chmod -R o+rX ${ISAACSIM_ROOT_PATH}` -- accepts the layer
cost in exchange for blanket access.
| RUN --mount=type=cache,target=${DOCKER_USER_HOME}/.cache/pip,uid=1000,gid=1000 \ | ||
| ${ISAACLAB_PATH}/isaaclab.sh --install |
There was a problem hiding this comment.
Pip cache mount target mismatches the
isaaclab user's $HOME
After USER isaaclab, pip resolves its cache via ${HOME} — which points to /home/isaaclab (the user's home directory from /etc/passwd). The --mount target is ${DOCKER_USER_HOME}/.cache/pip which expands to /root/.cache/pip in CI. Pip will therefore write to /home/isaaclab/.cache/pip and never read from (or populate) the BuildKit cache mounted at /root/.cache/pip, making the cache layer a no-op.
The mount target should follow the user's actual $HOME:
RUN --mount=type=cache,target=/home/isaaclab/.cache/pip,uid=1000,gid=1000 \
${ISAACLAB_PATH}/isaaclab.sh --installIf keeping DOCKER_USER_HOME as the single source of truth is preferred, ENV HOME=${DOCKER_USER_HOME} (after updating DOCKER_USER_HOME_ARG to /home/isaaclab in CI) would reconcile both variables.
The previous chown only covered the python site-packages directory, but
pip also installs console_scripts and other binaries to the embedded
CPython's bin/ directory. The torch install pulls triton, which ships a
`proton` CLI binary that needs to land at
${ISAACSIM_ROOT_PATH}/kit/python/bin/proton -- a directory still owned
by isaac-sim (uid 1234) from the base image, so isaaclab cannot write
there and the editable install fails with EACCES.
Replaces the specific lib/python3.12/site-packages target with the
parent kit/python so chown covers bin/, lib/python3.X/{everything},
include/, share/, and any other subpath pip may write to as transitive
deps land. Drops the hardcoded python3.12 path from this chown step as
a side benefit; the rm-pip and get-pip lines elsewhere in this file
still hardcode 3.12 and are out of scope for this PR.
Layer cost: ~500 MB - 1 GB OverlayFS copy-up of the embedded CPython
tree. Accepted in exchange for not having to chase further pip-write
EACCES failures one transitive dep at a time.
Previously the user's home (/home/isaaclab from `useradd -m -d
/home/isaaclab`) diverged from ${DOCKER_USER_HOME} (=/root from CI build
arg). Consequence:
* The BuildKit pip cache mount targets ${DOCKER_USER_HOME}/.cache/pip
= /root/.cache/pip, but pip resolves the cache via $HOME/.cache/pip
= /home/isaaclab/.cache/pip -- different paths, so the cache mount
is a no-op and every CI build re-downloads everything.
* The mkdir -p ${DOCKER_USER_HOME}/.{cache,nv,nvidia-omniverse,...}
seeded the singularity-bind dirs at /root/* but Kit at runtime
looks for them under $HOME = /home/isaaclab/*.
Fix: drop -m and pass -d ${DOCKER_USER_HOME} so isaaclab's home is
/root. Requires -M (no home creation) since /root already exists from
the base image; the chown step transfers ownership of it to isaaclab.
/home/isaaclab is no longer created and is removed from the chown
list. Now $HOME, the pip cache mount target, the singularity-bind
mkdirs, and the bashrc writes all resolve to the same /root path.
Description
Refactors
docker/Dockerfile.curoboso the editable Isaac Lab installs and the final container runtime drop privileges to a non-rootisaaclabuser (uid/gid 1000) instead of staying as root the whole way. The cuRobo CI image (build-curobo→test-curobo/test-skillgen) currently runs as root, withOMNI_KIT_ALLOW_ROOT=1papering over the upstream Isaac Sim base image's intent of running non-root.What changed
Explicit
USER rootfor the system-level setup block. apt, cuda-keyring repo,install_deps.py apt,${DOCKER_USER_HOME}cache dirs,/bin/nvidia-*placeholders, the pre-bundled torch removal, and theget-pip.pybootstrap all require root and stay as root.New
isaaclabuser at uid/gid 1000. Created via explicitgroupadd --non-unique --gid 1000followed byuseradd --non-unique --uid 1000 --gid 1000 isaaclab. The--non-uniqueflag on both is required because some base image revisions already provision a user/group at numeric ID 1000 (e.g. anubuntu/kit-style account); the file system only cares about numeric IDs, so two names mapping to the same ID is fine. Pinning the GID explicitly viagroupadd(instead of lettinguseraddauto-pick one when the default GID is taken) keeps the BuildKit pip cache mount'sgid=1000matched to the user's primary group. UID 1000 is chosen so that the runtime user matches the typical CI runner host user that owns the bind-mounted/workspace/isaaclab— required for the test runner'smkdir testsetc. to not fail withEACCESon the bind-mount overlay.chown+chmod+USER isaaclabbefore the pip installs.${ISAACLAB_PATH},${ISAACSIM_ROOT_PATH}/kit/python/.../site-packages,${DOCKER_USER_HOME},/home/isaaclab./isaac-simitself stays mode 755 and isaac-sim-owned by the base image — re-chowning it would force an OverlayFS copy-up of the multi-GB Isaac Sim install.chmod 755 ${ISAACSIM_ROOT_PATH}opens up traversal of/isaac-simfor non-owner users. The base image creates/isaac-simasisaac-sim's home directory; on recent Ubuntu (HOME_MODE 0700in/etc/login.defs), that means mode 0700 isaac-sim:isaac-sim, which our isaaclab user cannot enter to findpython.shor the rest of the Kit runtime. Inner files retain their original perms (NVIDIA ships them world-readable per standard distribution conventions), so only the top-level directory's mode needs to change — single-inode metadata change, no recursive layer cost.BuildKit cache mount pinned to
uid=1000,gid=1000for${DOCKER_USER_HOME}/.cache/pip, otherwise the default root-owned cache mount would be unwritable fromisaaclab(BuildKit doesn't consult/etc/passwdat mount time, so the numeric form is required).${HOME}/.bashrcused consistently in the aliasRUNblock. Two lines hardcoded/root/.bashrcwhile the surrounding eight used${HOME}/.bashrc; after the user switch this inconsistency would silently land the trailing config in the wrong file.Final
COPYannotated with--chown=isaaclab:isaaclabfor ownership consistency.CI build args (
DOCKER_USER_HOME_ARG=/rootin.github/actions/ecr-build-push-pull/action.yml,.github/actions/docker-build/action.yml, and.github/workflows/postmerge-ci.yml) are unchanged —/rootis chowned toisaaclabinside the image, so they continue to work.Type of change
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there