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

Fix devcontainer setup on non-rootless docker #3

Open
wants to merge 4 commits into
base: switch-to-poetry
Choose a base branch
from

Conversation

edmondchuc
Copy link

@edmondchuc edmondchuc commented Jan 11, 2023

Adding chown in the devcontainer's Dockerfile. By default, the vscode user uid is 1000.

Reference: https://code.visualstudio.com/remote/advancedcontainers/add-nonroot-user

  • Add devcontainer postCreateCommand
  • Add a new docker-compose service named run to run one-off commands with named volumes
  • Remove named volumes from docker-compose devcontainer service
  • Update docs

@edmondchuc
Copy link
Author

This should fix issues described in RDFLib#2187 (comment).

Comment on lines 50 to 63

RUN \
mkdir -p \
/srv/workspace/.venv \
/srv/workspace/.tox \
/srv/workspace/.mypy_cache \
/srv/workspace/.pytest_cache

RUN \
chown -R 1000:1000 \
/srv/workspace/.venv \
/srv/workspace/.tox \
/srv/workspace/.mypy_cache \
/srv/workspace/.pytest_cache

Choose a reason for hiding this comment

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

Under normal operation this should be mounted from outside the docker container - but I will check tonight what is happening and ensure it works as well as it can with rootful (i.e. non-rootless) docker

Copy link
Author

Choose a reason for hiding this comment

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

This may be relevant docker/compose#3270

Copy link

@aucampia aucampia Jan 11, 2023

Choose a reason for hiding this comment

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

Is it working correctly for you on the main branch? Also what docker do you use, docker desktop on windows or are you in a VM? And how do you invoke things that cause problems, do you just use it from vscode or do you use docker compose run?

Choose a reason for hiding this comment

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

@edmondchuc I think I'm just trying to be too clever with what I did here.

The reasoning for adding these volumes were so that usage of the devcontainer does not contaminate your host environment while still keeping state outside of the container. This works with rootless docker on Linux, but I'm not sure how it works on MacOS and Windows - so I think best is maybe just to not try and be too clever. if it works for you with the following lines removed then we should maybe just remove them instead:

https://github.com/RDFLib/rdflib/blob/9b778b39a7759a9c94d5437e1d5f02196bdc9c51/docker-compose.yml#L12-L15

      - dot-venv:/srv/workspace/.venv
      - dot-tox:/srv/workspace/.tox
      - dot-mypy-cache:/srv/workspace/.mypy_cache
      - dot-pytest-cache:/srv/workspace/.pytest_cache

Copy link
Author

Choose a reason for hiding this comment

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

I recall trying with Docker Desktop on Windows with WSL2, macOS and also on an Ubuntu VM. I also get the same issue when I run it in GitHub Codespaces.

I agree, I think for running a dev container using Visual Studio Code, it's not necessary to have the docker compose volumes.

Do you need the volume mounts because you're running one-off commands in the containers? E.g. docker-compose run --rm devcontainer task validate?

Maybe we can keep the current docker-compose.yml so that you can continue running one-off commands and we can have another docker-compose file for the Visual Studio Code dev container?

https://github.com/RDFLib/rdflib/blob/9b778b39a7759a9c94d5437e1d5f02196bdc9c51/.devcontainer.json#L2

Choose a reason for hiding this comment

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

Do you need the volume mounts because you're running one-off commands in the containers? E.g. docker-compose run --rm devcontainer task validate?

The default behaviour will actually work fine, the only problem is that the venv created by the devcontainer may not be to the liking of the host system, and vice versa.

If I do task configure and task validate on my host system, it will create a .venv (I run with POETRY_VIRTUALENVS_IN_PROJECT=1), and pytest, mypy and other caches in my source directory. If I then run the same commands in the devcontainer with vscode or with docker compose, the .venv and cache folders from my host system will be visible in the container.

This would be fine in some cases, but in others poetry just does not like what is happening, the venvs seem to be somewhat platform specific.

But either way, we don't need it.

Maybe we can keep the current docker-compose.yml so that you can continue running one-off commands and we can have another docker-compose file for the Visual Studio Code dev container?

We can actually just have a separate service in the same docker-compose.yml file as we can specify which service to run for devcontainer, which may be a bit more terse.

Copy link
Author

Choose a reason for hiding this comment

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

We can actually just have a separate service in the same docker-compose.yml file as we can specify which service to run for devcontainer, which may be a bit more terse.

Good idea. I've gone back to using just the one docker-compose.yml file with the devcontainer service assigned to the devcontainer and the run service dedicated to running one-off commands with named volumes in commit f3eb90f.

Choose a reason for hiding this comment

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

looks good to me 👍

Copy link

@aucampia aucampia left a comment

Choose a reason for hiding this comment

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

LGTM

@edmondchuc edmondchuc changed the title Add chown for container volumes Fix devcontainer setup on non-rootless docker Jan 12, 2023
@edmondchuc
Copy link
Author

LGTM

Thanks for the review @aucampia!

@aucampia
Copy link

I think the broken run (Validate / validate (3.7, ubuntu-latest) (pull_request) Failing after 1m) is just transient, should be fine on retry.

@jclerman
Copy link
Owner

jclerman commented Jan 13, 2023

Thanks @edmondchuc for the contribution and followup, and @aucampia for working out the details!

I just tried this on my own laptop (MacOS) and found that when I do this step:

docker-compose run --rm run task configure

poetry appears to install its dependencies into a venv, but I also get a new .venv in the root of my git clone (outside the docker container). That directory is empty, but still - can we avoid having it created? I normally let poetry create its virtual environments in its default location (on MacOS, I get /Users/jclerman/Library/Caches/pypoetry/virtualenvs/rdflib-xIc3bzCB-py3.7) and the presence of that .venv directory breaks local development - poetry sees it and thinks that the venv should be in there, and when it isn't, I get a message that

The virtual environment found in /Users/jclerman/git/rdflib-edmund/.venv seems to be broken.
Recreating virtualenv rdflib-xIc3bzCB-py3.7 in /Users/jclerman/Library/Caches/pypoetry/virtualenvs/rdflib-xIc3bzCB-py3.7

(that "Recreating" seems to be serious - I think it wipes out whatever was there and starts fresh, since I end up with nothing installed into that venv and I need to re-run "poetry install" to get things back).

It would be much less disruptive if the "configure" task didn't modify my working checked-out copy of the repo.

@aucampia
Copy link

aucampia commented Jan 13, 2023

That directory is empty, but still - can we avoid having it created? I normally let poetry create its virtual environments in its default location (on MacOS, I get /Users/jclerman/Library/Caches/pypoetry/virtualenvs/rdflib-xIc3bzCB-py3.7) and the presence of that .venv directory breaks local development - poetry sees it and thinks that the venv should be in there, and when it isn't, I get a message that

(bit of a verbose explanation and suggestion)

We are trying to compensate for docker container storage being ephemeral which means if we store state inside the container (like pytest, tox or mypy cache or venv) things will get very slow every time the container is created.

We have two options for this:

  • Create the state inside a location mounted from the host OS. We really only have the project root directory to work with here though. So this means we create .venv, .mypy_cache, .tox, .pytest_cache in your project directory. The downside to this is though, it breaks down when running with docker daemon as root on linux, because then these will be owned by root. It will also probably break development outside of the container for windows and mac users as venvs are not that portable, and I think they even have some portability issues between different Linux distros.

  • Create the state inside a volume. The benefit of this is that it does less pollution to the host environment, so no platform issues, but I guess as you have seen yourself, it is not no pollution. However, here we again have a problem which is that docker mounts volumes as root and devcontainers in VSCode does not run under root user, so these also break devcontainers.

The reason why you get this folder appearing outside is because there has to be something at the mount point, and docker just creates it if it does not exist.

For the run "service" we could mount the docker volumes somewhere else, outside of the workspace dir /srv/workspace. And then set environment variables to control where where tox, mypy, pytest and poetry will put state.

However volumes does not work well with devcontainers so there we need another solution. Possibly the best is to just instruct tox, mypy, pytest and poetry to put their state in the source directories but use different names, possibly .devcontainer-var/{pytest_cache,mypy_cache,tox,venv}. That way we at least only pollute the host in a place that does not interfere with normal dev flow.

So our options as I see them:

  1. For the devcontainer "service": configure tools to use .devcontainer-var/{pytest_cache,mypy_cache,tox,venv} (happy for other name suggestions).
  2. For the run "service", one of the following:
    1. mount volumes outside of /srv/workspace and configure tools to use those
    2. Do the same as for the devcontainer "service"

I think 2,i (mounting volumes outside /srv/workspace) is probably the right solution for the run "service".

I will look at this tonight (CET).

@aucampia aucampia mentioned this pull request Jan 16, 2023
@aucampia
Copy link

  1. For the devcontainer "service": configure tools to use .devcontainer-var/{pytest_cache,mypy_cache,tox,venv} (happy for other name suggestions).

  2. For the run "service", one of the following:

    1. mount volumes outside of /srv/workspace and configure tools to use those
    2. Do the same as for the devcontainer "service"

I think 2,i (mounting volumes outside /srv/workspace) is probably the right solution for the run "service".

Done now in:

Seems to work for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants