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

docs: Extend non-root guide and create dedicated compose file for it #13257

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

R-Rudolf
Copy link
Contributor

@R-Rudolf R-Rudolf commented Oct 7, 2024

Based on discussion 13124, creating a dedicated compose file for easier non-root setup guide and refining the relevant FAQ item.

Copy link
Contributor

github-actions bot commented Oct 7, 2024

Label error. Requires exactly 1 of: changelog:.*. Found:

@github-actions github-actions bot added the documentation Improvements or additions to documentation label Oct 7, 2024
@bo0tzz
Copy link
Member

bo0tzz commented Oct 7, 2024

Does this need a whole separate compose file?

@mmomjian
Copy link
Contributor

mmomjian commented Oct 7, 2024

I don’t think this should be added. I rather we document (as in the FAQ) the needed changes for users who know what to do. Non root requires more technical knowledge.

Also, have you tested this PR? Pretty sure ML won’t work. I’m not too sure about the 999:999 for Postgres either - what OS was this determined on?

@R-Rudolf
Copy link
Contributor Author

R-Rudolf commented Oct 7, 2024

Hi,
Yes, a separate compose file would need to be kept in sync with the others, so I understand that it should be avoided.
I will check if the FAQ could be extended reasonably to help.

I use Debian bookworm, I tested only the onboarding and a single photo upload. I waited for some time for the machine learning to kick in for the facial recognition or such and saw some logs about it without error.
How can I easiest determine if the machine learning is OK? I guess, I can build some curl commands or check the locustfile.
After startup, it created a new matplotlib directory within the mounted /.config dir, but logs some warnings next to it as well:

WARNING Matplotlib created a temporary cache directory at
/tmp/matplotlib-vomdv14l because the default path
(/.cache/matplotlib) is not a writable directory;
it is highly recommended to set the MPLCONFIGDIR
environment variable to a writable directory, in
particular to speed up the import of Matplotlib and
to better support multiprocessing.

The directory is empty, I am not sure how to troubleshoot, or what can be wrong, I checked with exec into the container and the mounted dir is writeable.

The postgres was tested. When I did not set userid 999 it failed even with read-write permissions, as it tried to change ownership of the directory and failed (I guess this line fails, but not sure on the lineage of the image). Even if the init process has root/given_userid, it changes the userid of the child processes, as I can tell from the logs and behavior. The 999 userid was empirically checked exec-ing into the running container, but I think this dockerfile template is what this image is based on as well, or based on which it was created.

@mertalev
Copy link
Contributor

mertalev commented Oct 8, 2024

Machine learning actually doesn't use matplotlib at all - it's just a transitive dependency for a module to provide functionality we don't use. Setting MPLCONFIGDIR to somewhere in the cache folder should simplify things a bit.

@CiNcH83
Copy link

CiNcH83 commented Oct 10, 2024

How about if we put USER_ID and GROUP_ID into example.env file and set it to 0 (root) by default? In the common docker-compose.yml, we add the user tag to all services but postgres which runs as user anyway.

Does it make sense to turn immich-machine-learning:/cache (model-cache) into a bind mount in the common docker-compose.yml and add immich-machine-learning:/.config and redis:/data as bind mounts as well? What would be the disadvantages of also doing it that way in the root case? The required changes to go non-root would be minimal then. Also the FAQ would be easier, only explaining how all bind mount paths need to be accessible by the given user.

@R-Rudolf
Copy link
Contributor Author

The userid variable introduction would only cause some clutter in the .env file, but I see value in. However, the bind mount would give unnecessary extra requirement to the normal deployment, I would avoid that.

@CiNcH83
Copy link

CiNcH83 commented Oct 11, 2024

However, the bind mount would give unnecessary extra requirement to the normal deployment, I would avoid that.

It would also be possible to use named volumes and chown them to the given user [LINK].

@CiNcH83
Copy link

CiNcH83 commented Oct 23, 2024

I am still trying to figure out a solution where we do not complicate the standard case (root) but at the same time make the delta to go non-root as small as possible.

One question on the way there is why we need a volume for immich-machine-learning:/.config and redis:/data in the root case but not in the non-root case!?

[EDIT]
OK. So I guess the reason is that the service running as non-root user can't access the directories /.config and /data otherwise. This does not have anything to do with persistency at all...

@CiNcH83
Copy link

CiNcH83 commented Oct 23, 2024

Something like the following could be a solution for named volumes:

  immich-setup-named-volumes:
    image: ghcr.io/immich-app/immich-server:${IMMICH_VERSION:-release}
    user: root
    group_add: $IMMICH_GID
    volumes:
      - model-cache:/tmp/cache
      - model-config:/tmp/.config
      - redis-data:/tmp/data
    command: chown -R $IMMICH_UID:$IMMICH_GID /tmp/cache /tmp/config /tmp/data
    
volumes:
  model-cache:
  model-config:
  redis-data:

The creation of the directories and chown could also be done in the Dockerfile to take the burden away from the docker-compose.yml [LINK]. It should be enough to only pass UID and GID via user tag then.

Just some thoughts... I will probably start with the bind mount solution and merge changes as needed by future versions.

@mmomjian
Copy link
Contributor

Correct, it’s non persistent data so volume isn’t needed except for permission issue.

We’ve discussed a “permission setup” container before and we will not be adding that. We’re only going to add details on which volumes need to be mounted.

@CiNcH83
Copy link

CiNcH83 commented Oct 23, 2024

I think it may also be possible to chown the required directories to USER in Dockerfile without requiring a volume [LINK].

@mmomjian
Copy link
Contributor

You can only run chown with root privileges, which defeats the point of running the container as non root.

@eiqnepm
Copy link

eiqnepm commented Oct 30, 2024

I just made a pull request #13834 not noticing this one.

I used tmpfs so I didn't need to use a dedicated chown container or manually create the directories on the host, although it does make the model-cache folder ephemeral.

@CiNcH83
Copy link

CiNcH83 commented Oct 30, 2024

Thanks for the tmpfs idea! I actually like it as it works for root and non-root alike. tmpfs uses RAM though. I wonder whether this can cause trouble...

The .cache is not needed though. It is a mistake in the FAQ. It is only cache and .config.

@mmomjian
Copy link
Contributor

/.cache is not a mistake, or it was not when I tested about 2 months ago. The ML container fails to start without it being writable.

@CiNcH83
Copy link

CiNcH83 commented Oct 30, 2024

So we indeed need cache and .cache for ML? I did not mount .cache and at least face recognition seems to work...

@mmomjian
Copy link
Contributor

Not all of the facial recognition features require the ML container, so that doesn’t prove much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants