Skip to content
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

Update docker instructions to nvidia-docker2 #282

Merged
merged 3 commits into from
Oct 17, 2019

Conversation

felixvd
Copy link
Contributor

@felixvd felixvd commented Apr 3, 2019

Description

Fixes moveit/moveit#1368.

Our nvidia-docker instructions were outdated. This is just a rough draft that copies the instructions from the ROS.org wiki.

The previous version was more elegant, but I haven't found a way to do this without building a local image to set the environment variables. I think making the Dockerfile and run-gui-moveit-docker downloadable like gui-docker would be good, but I didn't want to open two PRs at once.

@davetcoleman is listed as the author of the file so he will have to comment.

This script is more self-contained than gui-docker, but I think I prefer it without the additional arguments. Fewer things to remember when spinning up a test environment, and users who need to change something are also comfortable making changes to the script.

Checklist

  • Tested modified webpage locally using the build_locally.sh script
  • While waiting for someone to review your request, please consider reviewing another open pull request to support the maintainers

@gavanderhoorn
Copy link
Contributor

Perhaps we should consider osrf/rocker?

@felixvd
Copy link
Contributor Author

felixvd commented Apr 4, 2019

Since we can already reduce the instructions to one copy-pastable block that doesn't install anything, I think that's the better option in this case. More self-contained, too.

Looks convenient though.

Copy link
Member

@davetcoleman davetcoleman left a comment

Choose a reason for hiding this comment

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

approval to @ruffsl

install/docker/index.markdown Outdated Show resolved Hide resolved
ENV NVIDIA_VISIBLE_DEVICES \
${NVIDIA_VISIBLE_DEVICES:-all}
ENV NVIDIA_DRIVER_CAPABILITIES \
${NVIDIA_DRIVER_CAPABILITIES:+$NVIDIA_DRIVER_CAPABILITIES,}graphics
Copy link
Member

Choose a reason for hiding this comment

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

seems we should just include this in the moveit repo under .docker folder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. Either that, or we use rocker.

@ruffsl
Copy link

ruffsl commented Apr 12, 2019

Perhaps we should consider osrf/rocker?

I think off loading the cognitive load and setup to osrf/rocker would be advantageous here, as the user could use and learn/transfer rocker for other ROS related GUI projects as well. cc @tfoote .

Since we can already reduce the instructions to one copy-pastable block that doesn't install anything, I think that's the better option in this case. More self-contained, too.

Rather than remembering to use a one-off script with a patched Dockerfile for each Docker image you intend on testing, folks could use a consistent interface to run containtered GUIs using rocker. We're already redirect the reader to apt install nvidia-docker2; installing osrf/rocker too for simplifying the runtime steps doesn't seem much of a stretch:

sudo apt-get install python3-rocker

If users don't want to remember how to patch there Dockerfiles, or want run the GUI with audio or as the local user privileges, or connect to ROS nodes accessible from the host's network, these are simple runtime flags for osrf/rocker the translate to non-trivial docker file/run configurations:

rocker --nvidia --user --x11 --network host --pulse osrf/ros:crystal-desktop rviz2

v.s. what this expands into that the use would otherwise have to do manually:

# Preamble from extension [nvidia]
FROM nvidia/opengl:1.0-glvnd-devel-ubuntu18.04 as glvnd

FROM osrf/ros:crystal-desktop
# Snippet from extension [nvidia]
RUN apt-get update && apt-get install -y --no-install-recommends \
    libglvnd0 \
    libgl1 \
    libglx0 \
    libegl1 \
    libgles2 \
    && rm -rf /var/lib/apt/lists/*
COPY --from=glvnd /usr/share/glvnd/egl_vendor.d/10_nvidia.json /usr/share/glvnd/egl_vendor.d/10_nvidia.json

ENV NVIDIA_VISIBLE_DEVICES ${NVIDIA_VISIBLE_DEVICES:-all}
ENV NVIDIA_DRIVER_CAPABILITIES ${NVIDIA_DRIVER_CAPABILITIES:+$NVIDIA_DRIVER_CAPABILITIES,}graphics

# Snippet from extension [pulse]
RUN mkdir -p /etc/pulse
RUN echo '\n\
# Connect to the hosts server using the mounted UNIX socket\n\
default-server = unix:/run/user/1000/pulse/native\n\
\n\
# Prevent a server running in the container\n\
autospawn = no\n\
daemon-binary = /bin/true\n\
\n\
# Prevent the use of shared memory\n\
enable-shm = false\n\
\n'\
> /etc/pulse/client.conf

# Snippet from extension [user]
# make sure sudo is installed to be able to give user sudo access in docker
RUN apt-get update \
 && apt-get install -y \
    sudo \
 && apt-get clean

RUN groupadd -g "1000" "ubuntu" \
 && useradd --uid "1000" -s "/bin/bash" -c "ubuntu,,," -g "1000" -d "/home/ubuntu" "ubuntu" \
 && echo "ubuntu:ubuntu" | chpasswd \
 && adduser ubuntu sudo \
 && echo "ubuntu ALL=NOPASSWD: ALL" >> /etc/sudoers.d/ubuntu
# Commands below run as the developer user
USER ubuntu
docker build --tag rocker_osrf/ros:crystal-desktop_nvidia_pulse_x11_user .

docker run -it --rm \
  --network host \
  --runtime=nvidia \
  --security-opt seccomp=unconfined \
  -v /run/user/1000/pulse:/run/user/1000/pulse \
  --device /dev/snd \
  -e PULSE_SERVER=unix:/run/user/1000/pulse/native \
  -v /run/user/1000/pulse/native:/run/user/1000/pulse/native \
  --group-add 29 \
  -e DISPLAY \
  -e TERM \
  -e QT_X11_NO_MITSHM=1 \
  -e XAUTHORITY=/tmp/.docker.xauth \
  -v /tmp/.docker.xauth:/tmp/.docker.xauth \
  -v /tmp/.X11-unix:/tmp/.X11-unix \
  -v /etc/localtime:/etc/localtime:ro \
  rocker_osrf/ros:crystal-desktop_nvidia_pulse_x11_user rviz2

@felixvd
Copy link
Contributor Author

felixvd commented May 1, 2019

As nice as I find it that the image auto-updates with rocker, I think we should stick to the tutorial, because:

  • Regular docker arguments can't be passed through (e.g. for mounting directories, as I usually do to link my workspace)
  • The rocker --user flag doesn't work, because the moveit docker images have their WORKDIR set to /root/ws_moveit/

I guess you can add extensions, but it's not clear to me how, and I don't feel like adding more indirection to a functioning workflow.

@davetcoleman
Copy link
Member

Is this ready for re-review? Where @ruffsl 's concerns addressed?

Also needs a rebase.

@ruffsl
Copy link

ruffsl commented Jun 7, 2019

Regular docker arguments can't be passed through (e.g. for mounting directories, as I usually do to link my workspace)

You can use --user argument to transparently mount your host's user home into the container. Useful if you want to keep the same vim config and .ros files to be persistent across container lifecycles.

The rocker --user flag doesn't work, because the moveit docker images have their WORKDIR set to /root/ws_moveit/

That was poor president set on my part that I've since then tried to rectify by encouraging the use of /opt for workspace install to avoid /root or /tmp conflicts.

moveit/moveit2#46 (comment)

I still think using the tool developed by OSRF specific for this use case, rocker, would help simplify this greatly for beginning contributors not versered in Docker, Nvidia drivers, and X11 unix socket permissions, but I don't want to block anything.

@felixvd
Copy link
Contributor Author

felixvd commented Jun 8, 2019

That was poor president set on my part that I've since then tried to rectify by encouraging the use of /opt for workspace install to avoid /root or /tmp conflicts.

Well, I haven't seen a norm for docker entrypoints or installations, so it seems to me that making rocker more flexible would be easier than expecting users to conform. I opened an issue explaining what I think would make it a good replacement for the current instructions.

Until then I'd use the version I just pushed. I added the scripts here, as suggested by @davetcoleman, which makes the instructions quite short.

I also updated to the new container names (master-source, master-ci), made master-source the default, and made the container persistent, so that users can open multiple terminals in the same container conveniently.

@ruffsl
Copy link

ruffsl commented Jun 9, 2019

Well, I haven't seen a norm for docker entrypoints or installations,

This is just the norm we choose for the official ros images at osrf/docker_images.
If you'd like take a look, we keep our devel image as up to date reference for compiling ros2 from source:
https://github.com/osrf/docker_images/blob/408de55ce174438fb7848e1d2125d9d34248a65b/ros2/source/devel/Dockerfile#L62-L63

so it seems to me that making rocker more flexible would be easier than expecting users to conform.

The issue with choosing /root is not related to rocker or docker, but about the inherent permission restrictions of linux file systems. If you mount /root from inside the container to anywhere on host you'll have to consequently elevate the user privilege to recursively change file permissions or your IDE process to even read workspace from the host.

@felixvd
Copy link
Contributor Author

felixvd commented Jun 10, 2019

The issue with choosing /root is not related to rocker or docker, but about the inherent permission restrictions of linux file systems. If you mount /root from inside the container to anywhere on host you'll have to consequently elevate the user privilege to recursively change file permissions or your IDE process to even read workspace from the host.

I'm not sure why that would be, that hasn't been my experience. Files created by root in the container are owned by root (until you chown them to the host user), but files that existed on the host file system are unchanged and can be read by the host IDE without problems.

we keep our devel image as up to date reference for compiling ros2 from source

That's a good reference, but if I understand correctly we will have to change the MoveIt Dockerfiles to link directories with rocker. Since this set of PRs works without more changes and makes a later change towards rocker easier too, shall we go ahead and merge?

@gavanderhoorn
Copy link
Contributor

@felixvd @ruffsl: I see my suggestion to perhaps consider using rocker lead to this PR getting a bit sidetracked.

I believe I'd agree with @ruffsl that using rocker would be much nicer, but that might be too big a chance for this PR.

Perhaps a good compromise could be if @felixvd creates an issue to look into upgrading the tutorials to use rocker and then we'd accept this PR as-is -- of course after it's been validated by someone to (still) work.

Copy link
Contributor

@BryceStevenWilley BryceStevenWilley left a comment

Choose a reason for hiding this comment

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

Took a stab at getting this to work, but got stuck.

install/docker/index.markdown Outdated Show resolved Hide resolved

You can test that the GUI works by running rviz:

roscore > /dev/null & rosrun rviz rviz
Copy link
Contributor

@BryceStevenWilley BryceStevenWilley Jun 20, 2019

Choose a reason for hiding this comment

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

I could get the docker image running, but unfortunately rviz didn't run. Instead on this command nothing happens for a while (~30 seconds) and upon ^C, I get:

QStandardPaths: XDG_RUNTIME_DIR not set, defaulting to '/tmp/runtime-root'
No protocol specified
qt.qpa.screen: QXcbConnection: Could not connect to display :1
Could not connect to any X display.

Successive invocations don't hang and directly give the error.
Don't have much experience with Docker, is there something obvious I could have missed?

Copy link
Contributor

@rhaschke rhaschke left a comment

Choose a reason for hiding this comment

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

Thanks for taking the initiative. I would prefer to keep the old gui-docker script.


Note that for recent Linux distros, the installation is basically just a single wget command. You may also want [add your user to the docker group](https://docs.docker.com/engine/installation/linux/ubuntulinux/#/create-a-docker-group) to avoid having to use sudo permissions when you use the docker command.

### Ubuntu

If you are running a recent version of Ubuntu (e.g. 14.04, 16.04) it can be as simple as:
If you are running a recent version of Ubuntu (e.g. 16.04, 18.04) it can be as simple as:

sudo apt-get install curl
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we refer to the distro-provided docker.io package here?


wget https://raw.githubusercontent.com/ros-planning/moveit/master/.docker/gui-docker -O gui-docker && chmod +x gui-docker
./gui-docker -it --rm moveit/moveit:melodic-release /bin/bash
docker run -it moveit/moveit:master-source bash
Copy link
Contributor

Choose a reason for hiding this comment

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

Add option --rm to really cleanup the non-persistent container?

Suggested change
docker run -it moveit/moveit:master-source bash
docker run -it --rm moveit/moveit:master-source bash


**Command-Line Only**
Then, use the commands below to:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can keep the old, simpler instructions, if we stick with gui-docker from moveit/moveit#1532

@felixvd
Copy link
Contributor Author

felixvd commented Jul 5, 2019

I will update this to correspond to moveit/moveit#1532 soon.

@rhaschke
Copy link
Contributor

rhaschke commented Sep 9, 2019

Friendly ping @felixvd.

@felixvd felixvd force-pushed the update-nvidia-docker-instructions branch from 2f7f10b to 240c72c Compare October 9, 2019 12:23
@felixvd
Copy link
Contributor Author

felixvd commented Oct 9, 2019

Sorry for the long wait <3

@felixvd felixvd force-pushed the update-nvidia-docker-instructions branch from 0c486ab to accf6d0 Compare October 10, 2019 10:13
Copy link
Contributor

@rhaschke rhaschke left a comment

Choose a reason for hiding this comment

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

I committed some cleanups. See details for my motivation.

@@ -31,23 +31,35 @@ And you will likely need to log out and back into your user account for the chan

## Running MoveIt Containers

To use Rviz (and other GUIs), you need to first set up hardware acceleration for Docker as described [here](http://wiki.ros.org/docker/Tutorials/Hardware%20Acceleration#nvidia-docker2) and install nvidia-docker2 according to [these instructions](https://github.com/nvidia/nvidia-docker/wiki/Installation-(version-2.0)).
To use Rviz (and other GUIs), you probably want to set up hardware acceleration for Docker as described [here](http://wiki.ros.org/docker/Tutorials/Hardware%20Acceleration).
Copy link
Contributor

Choose a reason for hiding this comment

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

nvidia-docker is not a prerequisite for gui-docker.


Files created inside the Docker container are owned by root. Use `chown . -R 1000:1000` inside the container to assign files to the host user if you run into access issues.
Files created inside the Docker container are usually owned by root. Check and fix permissions, if you run into issues.
Copy link
Contributor

Choose a reason for hiding this comment

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

1000 is the default user id of the first user created in Debian-like distros. However, in many situations, this ID might be plain wrong.

@felixvd
Copy link
Contributor Author

felixvd commented Oct 10, 2019

Thanks for the additions and corrections. It reads quite well now, I think.

This commit just fixes a minor issue (the default is melodic-source) and small typo, and completes the hand-holding example :)
I also propose to keep moveit in the container name for clarity.

@rhaschke rhaschke merged commit 1380dad into moveit:gh-pages Oct 17, 2019
davetcoleman pushed a commit to davetcoleman/moveit.ros.org that referenced this pull request Aug 19, 2020
@felixvd felixvd deleted the update-nvidia-docker-instructions branch January 29, 2021 03:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

nvidia-docker file needs to be updated.
6 participants