linuxserver / docker-plex-meta-manager

GNU General Public License v3.0
29 stars 4 forks source link

Issues with paths #5

Closed JohnFawkes closed 2 years ago

JohnFawkes commented 2 years ago

Hello, I'm not sure if you guys are aware or not but the way you've set up pmm has caused some issues with PMM devs/users. Here's a write up on the official wiki of the issues. https://metamanager.wiki/en/latest/home/guides/alternative-docker.html we get users all time that use your container and end up not getting it to work correctly because they follow our wiki and it fails. So people end up giving up or they have to switch to the official docker container. If you guys could address these issues in some way, it would be greatly appreciated. Thank you and have a good day

github-actions[bot] commented 2 years ago

Thanks for opening your first issue here! Be sure to follow the bug or feature issue templates!

thespad commented 2 years ago
  1. The container defaults to using config.yml in the /config path, so this is only an issue if users want to use custom yml names/paths, at which point we expect them to be capable of getting the path correct (and if they blindly copy our example compose it has the default path prefilled so will still work as expected). However, I'll see if we can also have it handle relative paths without breaking anything.

  2. If there's demand from users, we'll provide develop/nightly branches, but we always start with just stable branches where possible as the vast majority of users pull latest (over 99% in most cases).

  3. Supporting runtime arguments with s6 services is non-trivial, but I'll look at providing a wrapper for it when I can.

  4. This was resolved some time ago; single runs will only perform a single run and then terminate the container.

  5. This is just a matter of following the same process that applies to all of our containers for file ownership; it's necessary for us running services as non-root users and not something we can change. If you don't provide a UID/GID to run as, it will default to 911 as this is commonly unused on a default Linux install and so won't cause issues with existing host permissions. In our experience, unless your app is running as root, not chowning /config on init causes far more problems because users typically do not set up folders and permissions correctly before running containers.

JohnFawkes commented 2 years ago
  1. For the official container we use config and not /config, so can be confusing when following the wiki
  2. This I completely understand why it's only latest. However right now they are in a phase where nightly is about to become latest so most users right now are using nightly and develop branch has fixes for latest but hasn't made it to latest yet.
  3. This would be awesome if you could.
  4. Great! Much appreciated.
  5. I'm not sure what perms the container is running. I would imagine it's using root though.

Thanks again for taking the time to consider this and making changes. I'm not a dev or anything of pmm. Just a user who loves pmm and making my own overlays and I also provide support officially in the pmm discord and I use alot of lsio containers so just trying to bring the 2 worlds closer together lol. Thanks again and have a great day

chazlarson commented 2 years ago

Issue 1 also affects things like the built-in fonts and means that users will see problems with the supplied examples and a lot [maybe all?] of the built-in default metadata files, since many use relative paths to things like fonts and overlay images. A new PMM user, who wants to use the default metadata files to create collections and apply overlays rather than writing any YAML, will be unable to do so if they happen to pick the lsio image. This just came up today where such a user's overlays failed because a font wasn't available. They changed nothing but the image they were using and it worked. Maybe that specific issue is due to the fonts not even being in the lsio image; I haven't checked that.

Issue 3 leads to the same sort of thing. The wiki has a lot of docker run examples, none of which work with lsio.

thespad commented 2 years ago

@JohnFawkes @chazlarson please give this a go lspipepr/plex-meta-manager:v1.17.3-pkg-f2c0cdbd-pr-6

It should allow any command line options to be passed and have them honoured. I've done basic testing, but I don't have the setup to extensively test everything.

Relative paths should also work, but again I've only been able to do limited testing, so I may have missed specific cases.

chazlarson commented 2 years ago
➜ docker pull lspipepr/plex-meta-manager:v1.17.3-pkg-f2c0cdbd-pr-6 && docker run --user 1000:1000 --rm -it -v /opt/Plex-Meta-Manager/config:/config lspipepr/plex-meta-manager:v1.17.3-pkg-f2c0cdbd-pr-6 -c /config/config.yml -rl 'Movies - 4K' -ca --run
v1.17.3-pkg-f2c0cdbd-pr-6: Pulling from lspipepr/plex-meta-manager
Digest: sha256:d036b1d42850b57836675f84cb3cb1b4feb6f1469f42c1e98697daf14105f841
Status: Image is up to date for lspipepr/plex-meta-manager:v1.17.3-pkg-f2c0cdbd-pr-6
docker.io/lspipepr/plex-meta-manager:v1.17.3-pkg-f2c0cdbd-pr-6
s6-overlay-suexec: fatal: can only run as pid 1

Same command with stock image:

➜ docker pull meisnate12/plex-meta-manager:nightly && docker run --user 1000:1000 --rm -it -v /opt/Plex-Meta-Manager/config:/config meisnate12/plex-meta-manager:nightly -c /config/config.yml -rl 'Movies - 4K' -ca --run
nightly: Pulling from meisnate12/plex-meta-manager
Digest: sha256:b71b98a627a49fa2cb130b0beced315d7ee183f760057b0337dfd60a9e5f1c02
Status: Image is up to date for meisnate12/plex-meta-manager:nightly
docker.io/meisnate12/plex-meta-manager:nightly
|====================================================================================================|
|                                                                                                    |
|        ____  _             __  __      _          __  __                                           |
|       |  _ \| | _____  __ |  \/  | ___| |_ __ _  |  \/  | __ _ _ __   __ _  __ _  ___ _ __         |
|       | |_) | |/ _ \ \/ / | |\/| |/ _ \ __/ _` | | |\/| |/ _` | '_ \ / _` |/ _` |/ _ \ '__|        |
...
thespad commented 2 years ago

You've got init: true set in your docker daemon.json or are otherwise overriding the default init so s6 can't launch.

Oh, also don't use --user 1000:1000 as it may break some elements of the s6 init (it can run with --user, subject to some limitations, but we don't test with it). Use -e PUID=1000 -e PGID=1000, which will have the same effect.

JohnFawkes commented 2 years ago
docker run -e PUID=1000 -e PGID=1000 --rm -it -v "/home/johnfawkes/docker/plexmetamanager/data/config:/config:rw" lspipepr/plex-meta-manager:v1.17.3-pkg-f2c0cdbd-pr-6 -rl "TV Shows" -op
[custom-init] No custom services found, skipping...
[migrations] started
[migrations] no migrations found

-------------------------------------
          _         ()
         | |  ___   _    __
         | | / __| | |  /  \
         | | \__ \ | | | () |
         |_| |___/ |_|  \__/

Brought to you by linuxserver.io
-------------------------------------

To support the app dev(s) visit:
Plex-Meta-Manager: https://github.com/sponsors/meisnate12

To support LSIO projects visit:
https://www.linuxserver.io/donate/
-------------------------------------
GID/UID
-------------------------------------

User uid:    1000
User gid:    1000
-------------------------------------

[custom-init] No custom files found, skipping...
[ls.io-init] done.
usage: plex_meta_manager.py [-h] [-c CONFIG] [-t TIMES] [-ti TIMEOUT] [-re RESUME] [-r] [-is] [-ig] [-rt] [-co] [-po] [-op] [-ov] [-lf] [-rc COLLECTIONS] [-rl LIBRARIES] [-rm METADATA] [-ca] [-dc] [-nc] [-nm] [-ro] [-d DIVIDER] [-w WIDTH]
plex_meta_manager.py: error: unrecognized arguments: Shows
thespad commented 2 years ago

Ugh, word splitting problems. OK, I'll have a look at that.

chazlarson commented 2 years ago

You've got init: true set in your docker daemon.json or are otherwise overriding the default init so s6 can't launch.

I'm pretty sure I use other s6 images on this machine, but will have a look. That's not something I've set deliberately.

thespad commented 2 years ago

That PR image has now been updated and should properly handle quoted strings with spaces if you repull it.

JohnFawkes commented 2 years ago

so its hard for me to test at them moment because the recommended branch is the nightly right now as its about to become the latest very soon and theres stuff in the config.yml that just wont work on latest. with that said it appears to be working right now but i did notice something that may or may not be a bug. if i run docker run -e PUID=1000 -e PGID=1000 --rm -it -v "/home/johnfawkes/docker/plexmetamanager/data/config:/config:rw" lspipepr/plex-meta-manager:v1.17.3-pkg-f2c0cdbd-pr-6 --config "/config/config.yml" -rl "TV Shows" then the container never stops. it just keeps running over and over and over, if I run docker run -e PUID=1000 -e PGID=1000 --rm -it -v "/home/johnfawkes/docker/plexmetamanager/data/config:/config:rw" lspipepr/plex-meta-manager:v1.17.3-pkg-f2c0cdbd-pr-6 --config "/config/config.yml" -r then the container just loops over all libraries available and exits accordingly at the end. againi though, I cant fully test as my config.yml is geared towards nightly. Ill write up a config.yml for latest here in a little bit and try again. Thank you again for working on this

JohnFawkes commented 2 years ago

but it did handle quoted strings with spaces btw

thespad commented 2 years ago

so its hard for me to test at them moment because the recommended branch is the nightly right now as its about to become the latest very soon and theres stuff in the config.yml that just wont work on latest. with that said it appears to be working right now but i did notice something that may or may not be a bug. if i run docker run -e PUID=1000 -e PGID=1000 --rm -it -v "/home/johnfawkes/docker/plexmetamanager/data/config:/config:rw" lspipepr/plex-meta-manager:v1.17.3-pkg-f2c0cdbd-pr-6 --config "/config/config.yml" -rl "TV Shows" then the container never stops. it just keeps running over and over and over, if I run docker run -e PUID=1000 -e PGID=1000 --rm -it -v "/home/johnfawkes/docker/plexmetamanager/data/config:/config:rw" lspipepr/plex-meta-manager:v1.17.3-pkg-f2c0cdbd-pr-6 --config "/config/config.yml" -r then the container just loops over all libraries available and exits accordingly at the end. againi though, I cant fully test as my config.yml is geared towards nightly. Ill write up a config.yml for latest here in a little bit and try again. Thank you again for working on this

Yeah that'll be "as intended", in that I didn't realise -rl etc. implied -r so it invokes the scheduled run. Do you have the full list of args that do a one-time run without needing -r?

JohnFawkes commented 2 years ago

-rm -rl -rc -rt -re

thespad commented 2 years ago

-rm -rl -rc -rt -re

Do they also work with --time or do they always imply --run?

JohnFawkes commented 2 years ago

always imply --run

JohnFawkes commented 2 years ago

sorry. if you saw my last massage, he isnt changing anything. i deleted the message. they all imply --run

thespad commented 2 years ago

New build should be done.

JohnFawkes commented 2 years ago

Awesome! its now ending instead of looping. Have to take my daughter to dance practice but when i get back ill write up a config for the latest branch of pmm and do a full run. thanks again

chazlarson commented 2 years ago

I've gotten past that s6 thing and it seems to be running fine; however, I cannot cancel a run without killing the docker container from outside. With the official image a control-c will cancel the run. Is there a way to accomplish the same thing in this image without opening a second SSH session and doing a docker stop?

thespad commented 2 years ago

I've gotten past that s6 thing and it seems to be running fine; however, I cannot cancel a run without killing the docker container from outside. With the official image a control-c will cancel the run. Is there a way to accomplish the same thing in this image without opening a second SSH session and doing a docker stop?

Short version is with s6 you can run with -i or -t but not both if you want Ctrl+C to get trapped and stop the container. It's a limitation of how the process supervision handles inputs.

chazlarson commented 2 years ago

Thanks, but neither of those options seem to work. Same behavior with just -i or just -t. I hold down control-c and it just keeps on going.

thespad commented 2 years ago

It can take maybe ~5 seconds to complete the shutdown, but it should work:

image

chazlarson commented 2 years ago

I'm trying to shut it down while it's actively running using the --run flag.

docker pull lspipepr/plex-meta-manager:v1.17.3-pkg-f2c0cdbd-pr-6 && docker run -e PUID=1000 -e PGID=1000 --rm -t -v /opt/Plex-Meta-Manager/config:/config lspipepr/plex-meta-manager:v1.17.3-pkg-f2c0cdbd-pr-6 -c /config/config.yml -rl 'Movies - 4K' -ca --run

Script starts, and as soon as the PMM banner appears I start tapping control-c.

^C keeps showing up in the log output, and it just keeps going. I've let it run for 3-5 minutes and hit control-C many many times. Only way I've found to stop it is to docker stop from another terminal session.

With the official image it stops with the first or second control-C.

thespad commented 2 years ago

I'll have to set up a live environment and see if I can replicate it during a full run.

thespad commented 2 years ago

I can't replicate it at the moment but I've asked some other people to try in case it's something environmental.

If everything else looks good I'll merge the PR and circle back to the Ctrl+C thing if there's actually anything that can be done other than a readme note about it.

JohnFawkes commented 2 years ago

sorry, been crazy busy with work and everything else. I also cant ctrl-c in this image either. I dont think its a huge deal though but something that if it can be fixed might be good to have. Other than that i think it can be merged and pushed.

thespad commented 2 years ago

Merged. if I manage to replicate the Ctrl+C issue in a way that allows me to find a sensible workaround I'll do another PR for it.