🤖 IsaacLab Review Bot — PR #6082

[Docker] Make non-root image volume mount points writable by the isaaclab user

Summary

Well-structured fix for a real regression: fresh Docker named volumes were inheriting root:root ownership at mount points that either didn't exist or were root-owned in the image, breaking training for the non-root isaaclab user. The approach (pre-create + chown before USER switch) is the correct Docker idiom, and the static test suite adds valuable regression protection.

Findings

🔵 Suggestion — Layer consolidation opportunity
The new RUN mkdir -p ... && chown -R ... adds an image layer. Since it immediately follows the COPY --chown directive, and the earlier RUN chown -R isaaclab:isaaclab ${ISAACLAB_PATH} ${DOCKER_USER_HOME} is a separate layer, there's no opportunity to merge without disrupting the logical ordering. The current approach is correct — just noting that the layer is small (metadata-only directories) so build cache impact is negligible.

🔵 Suggestion — chown -R scope for kit/logs
The chown -R targets ${ISAACSIM_ROOT_PATH}/kit/logs (the parent) rather than the specific kit/logs/Kit/Isaac-Sim mount point. This is intentional and correct — it covers both the created path and any intermediate directories — but it does chown content that Isaac Sim may have placed under kit/logs/ at build time. Given that the Isaac Sim install is root-owned and this only affects the logs subtree, the risk is minimal. No action needed.

🔵 Suggestion — Dockerfile.curobo duplication (resolved)
The 20-line block is identical between Dockerfile.base and Dockerfile.curobo. Both Dockerfiles now use the same dynamic approach via volume_mounts.py, so the duplication concern is reduced to a small, identical invocation block rather than a hardcoded path list.

🟡 Warning — Test relies on YAML indentation parsing (resolved)
_compose_volume_targets() parses docker-compose.yaml with regex line-by-line rather than using a YAML parser. Fixed in baa7bb8 — now uses yaml.safe_load via the dedicated volume_mounts.py module.

✅ Architecture: Correct use of mkdir -p + chown -R before USER switch. All non-home volume targets from docker-compose.yaml are covered dynamically.

✅ Completeness: Cross-checked against docker-compose.yaml — all type: volume targets that live outside ${DOCKER_USER_HOME} are addressed.

✅ Test coverage: The parser module is tested directly; Dockerfile wiring is validated via string assertions; PyYAML absence is handled gracefully.

CI Status

• ✅ pre-commit, Build Wheel, Detect Changes — passed
• ⏳ Build Base Docker Image, Build cuRobo Docker Image — pending (expected for Dockerfile changes)

Verdict

Looks good. Clean, well-motivated fix with thorough regression tests. No blocking concerns.

Update (baa7bb8): The author addressed the previous review's 🟡 warning about fragile regex-based YAML parsing by introducing a proper parser module (docker/utils/volume_mounts.py) that uses yaml.safe_load. This is a significant improvement:

Changes in this push:

1. New docker/utils/volume_mounts.py — A standalone module that parses docker-compose.yaml via PyYAML, extracts all type: volume targets from the x-default-isaac-lab-volumes extension field, and resolves ${VAR} references from the environment. Includes a main() entrypoint with validation (fails on empty results or unresolved variables).

2. Dockerfiles refactored — Both Dockerfile.base and Dockerfile.curobo now dynamically derive mount points by running the parser at build time:

   RUN set -o pipefail \
    && export DOCKER_ISAACSIM_ROOT_PATH="${ISAACSIM_ROOT_PATH}" ... \
    && dirs="$(... volume_mounts.py | grep '^/')" \
    && test -n "${dirs}" \
    && mkdir -p ${dirs} \
    && chown -R isaaclab:isaaclab ${dirs}

   The set -o pipefail + test -n guards ensure the build aborts if parsing fails — no silent degradation.

3. Test suite simplified — Removed ~100 lines of regex-based parsing/env-resolution logic. Tests now validate the parser module directly (test_compose_volume_targets_parse, test_resolved_targets_are_absolute_paths) and verify each Dockerfile wires in the parser with the expected guards.

Assessment:

✅ Addresses 🟡 warning — YAML parsing is now done properly via PyYAML rather than regex.
✅ Single source of truth — docker-compose.yaml is parsed once by one module; no duplicated path lists.
✅ Defensive build — pipefail + grep '^/' + test -n is a solid guard chain.
✅ Test coverage — Both the parser and the Dockerfile wiring are tested; PyYAML absence gracefully skips parser unit tests via pytest.importorskip.

🔵 Minor note — The grep '^/' filter in the Dockerfile silently drops any target that doesn't resolve to an absolute path. Combined with volume_mounts.py's own unresolved-variable check on stderr, this is safe — but a build log won't show which paths were dropped if stderr isn't visible. Very minor; no action needed.

Verdict: Still looks good. This push meaningfully improves the architecture by eliminating path duplication and the fragile regex parsing. No new concerns. Recommend merge after Docker image build CI passes.

_{🤖 automated review by isaaclab-review-bot | commits: 87bdb54 → baa7bb8}

Update (b17f0e3): Minor refinement to the build-time guard logic:

Changes in this push:

1. Removed test -n "${dirs}" guard from both Dockerfile.base and Dockerfile.curobo — This is now redundant because volume_mounts.py itself exits non-zero on an empty/unresolved list, and grep exits non-zero if no lines match. Under set -o pipefail, either failure already aborts the RUN step.

2. Updated Dockerfile comments — Clarified that the safety net is set -o pipefail + the non-zero exits from volume_mounts.py and grep, rather than the explicit test -n guard.

3. Test assertion updated — test_dockerfile_prepares_volume_mounts_from_compose now asserts set -o pipefail presence instead of test -n "${dirs}", matching the simplified Dockerfile logic.

4. Minor test hardening — Added assert spec is not None and spec.loader is not None in _load_module helper to satisfy type checkers and make failures more explicit.

Assessment:

✅ Correct simplification — The test -n was belt-and-suspenders; removing it reduces noise without sacrificing safety since both upstream commands already fail on empty output under pipefail.
✅ Tests updated consistently — Assertions match the new guard mechanism.
✅ No new concerns.

Verdict: Still LGTM. Clean follow-up that removes redundancy. Ready for merge once Docker CI passes.

_{🤖 automated review by isaaclab-review-bot | commits: 87bdb54 → b17f0e3}