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

Add arm64 support and Update Plex Download URLs to latest API #48

Merged
merged 6 commits into from
Apr 7, 2020

Conversation

maxlabuche
Copy link
Contributor

@maxlabuche maxlabuche commented Feb 1, 2020

(EDIT 1: Making my PR explanations more understandable)

Hi Plex Community,

I have a suggestion for the official Plex Docker Image, which is to make it available for different platforms other than Ubuntu/amd64.

Modification suggested by my branch:

  • ADD: Add environment variables (ARGs in Dockerfile) to be able to create Docker Images for almost any OS and architecture supported by Plex Media Server by only changing these environment variables ;
  • ADD: Add a Dockerfile for ubuntu/arm64 platforms ;
  • EDIT: Update S6-Overlay to a more recent version, which now supports new architectures like ARM ;
  • EDIT: Update package URLs to the latest Plex Download API

PLEX_BUILD and PLEX_DISTRO environment variables correspond to the "build" and "distro" variables listed in the API 5 JSON, provided by Plex, referencing all available packages :
https://plex.tv/pms/downloads/5.json

Testing environment:

I have tested this with a Raspberry Pi 4 with this configuration:

Element Version
Motherboard Raspberry Pi 4 Model B (4GB RAM)
Host OS Ubuntu Server 19.10 (eoan) [arch=arm64]
Docker Docker CE for Ubuntu 19.04 (disco) [arch=arm64]
Docker container: Plex Build Ubuntu 16.04+ / linux-aarch64 / debian
Docker container: Plex pkg plexmediaserver_1.18.5.2309-f5213a238_arm64.deb

Everything works fine, even updates.
Please consider merging my branch to this repo, I am available for any feedback!

Greetings

Copy link
Contributor

@gbooker gbooker left a comment

Choose a reason for hiding this comment

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

Some comments to facilitate discussion.

Dockerfile Outdated Show resolved Hide resolved
root/installBinary.sh Outdated Show resolved Hide resolved
root/plex-common.sh Outdated Show resolved Hide resolved
Dockerfile.arm64 Show resolved Hide resolved
@maxlabuche
Copy link
Contributor Author

Some comments to facilitate discussion.

Thank you very much @gbooker for your feedback, I will work around all of the discussed points, then submit the following modifications during the next days! 👍

Comment on lines +3 to +22
CONT_CONF_FILE="/version.txt"

function addVarToConf {
local variable="$1"
local value="$2"
if [ ! -z "${variable}" ]; then
echo ${variable}=${value} >> ${CONT_CONF_FILE}
fi
}

function readVarFromConf {
local variable="$1"
declare -n value=$2
if [ ! -z "${variable}" ]; then
value="$(grep -w ${variable} ${CONT_CONF_FILE} | cut -d'=' -f2 | tail -n 1)"
else
value=NULL
fi
}

Copy link
Contributor Author

@maxlabuche maxlabuche Feb 11, 2020

Choose a reason for hiding this comment

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

@gbooker I think there are better ways to write different variables to a "conf" file, but I wanted to keep things simple given that these variables are written once in this file (while building the image), then they are read only.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since the values are written once and in only one place, the function to add them isn't really needed (doesn't hurt though). I was expecting just a

echo "version=${TAG}" > /version.txt
echo "plex_build=${PLEX_BUILD}" >> /version.txt
echo "plex_distro=${PLEX_DISTRO}" >> /version.txt

but the above should work too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes exactly I was thinking to write the same lines at the beginning like you are suggesting. At least if at some point we need to have such as a function, we just have to modify the behaviour of this one

Dockerfile.arm64 Show resolved Hide resolved
@maxlabuche
Copy link
Contributor Author

maxlabuche commented Feb 11, 2020

@gbooker I have made the changes we discussed last week to the corresponding files. I am waiting for your feedback, do not hesitate to suggest any enhancement you're thinking they are relevant :)

Btw, I have tested this in the same environment from scratch, it's working as expected

local variable="$1"
declare -n value=$2
if [ ! -z "${variable}" ]; then
value="$(grep -w ${variable} ${CONT_CONF_FILE} | cut -d'=' -f2 | tail -n 1)"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Explanations for reading value :

Expecting file should be like:

version=beta
plex-distro=debian
...

Value is : look for variable name (grep), then take the part after the '=' sign (cut), then take the last value if multiple (tail) (that should not happen but in case).

@gbooker
Copy link
Contributor

gbooker commented Feb 11, 2020

Overall this is looking good. I need to have a discussion with our build-team to see if we can make our CI build the ARM64 images as well.

@maxlabuche
Copy link
Contributor Author

maxlabuche commented Feb 11, 2020

Ok nice, I am looking forward to see what's happening next, please let me know :)
Thank you for your review and your time!

@half2me
Copy link

half2me commented Mar 3, 2020

@maxlabuche @gbooker I would really be interested in this, as I want to have a setup with the raspberry pi 4. Let me know if there is anything I can help with.

@maxlabuche
Copy link
Contributor Author

@maxlabuche @gbooker I would really be interested in this, as I want to have a setup with the raspberry pi 4. Let me know if there is anything I can help with.

Hi @half2me ,

I am glad to see other people interested in supporting ARM architectures :)
Indeed, I have worked on this and submitted it to plexinc, but as you might see it's still in review process by the CI team (integration team).

I have made some tests at home and it's working fine for me (with RPi 4 / 4GB, better with at least 2GB of RAM on arm64), so if you are interested in testing it, you can pull my image on my Docker registry before the integration in the official repository.

(Instructions are exactly the same as the official one, and update checking is automatic each time you start the container)

https://hub.docker.com/r/maxlbch/pms-docker-arm64

Enjoy!

@half2me
Copy link

half2me commented Mar 5, 2020

@maxlabuche I have the rpi4 4gb as well, so I will try your image. Thanks :)

@maxlabuche
Copy link
Contributor Author

You're welcome :)

@half2me
Copy link

half2me commented Mar 6, 2020

You're welcome :)

@maxlabuche any chance you could build your image against amd64 arch and upload? (You can have multiple archs under the same tag) I'm trying to make my own helm chart for plex media server, and I would like to use your modifications on arm64 and amd64 as well.

@maxlabuche
Copy link
Contributor Author

@half2me Yes that was my base idea to have multiple arch under the same tag, I just forgot to do that, I am working on it I'll let you know when it's pushed to the registry :)

@half2me
Copy link

half2me commented Mar 9, 2020

@maxlabuche a bit off topic, but if its of any help, I use github actions to build my docker images automatically on each push using docker buildx. You can build for any arch you want. Here is an example: https://github.com/half2me/charts/blob/master/.github/workflows/dockerimage.yml

@maxlabuche
Copy link
Contributor Author

@half2me Oh that's this kind of stuff I am looking for, indeed I was using buildx to build my images but for some reasons it failed for arm64, that's why I made it directly from the Raspberry, I will look around, thanks!

@maxlabuche
Copy link
Contributor Author

I remember, here the thing is that the Dockerfile is not the same for amd64 and arm64, that's what I should consider

@half2me
Copy link

half2me commented Mar 9, 2020

I remember, here the thing is that the Dockerfile is not the same for amd64 and arm64, that's what I should consider

Do you think it would be worth making it the same, and branching via an ARG? If most of it is similar...

@maxlabuche
Copy link
Contributor Author

maxlabuche commented Mar 9, 2020

We need to look around before, for instance here is the muti-arch image :
https://hub.docker.com/r/maxlbch/pms-docker
Hope that it correspond in what you're looking for :)

I will be working on your suggestion to be even more futureproof.

@half2me
Copy link

half2me commented Mar 10, 2020

We need to look around before, for instance here is the muti-arch image :
hub.docker.com/r/maxlbch/pms-docker
Hope that it correspond in what you're looking for :)

I will be working on your suggestion to be even more futureproof.

@maxlabuche Exactly what I was looking for, thank you!
Let me know if you need any help, (dm me on twitter or keybase)

@ipeacocks
Copy link

Why this can't be merged?

Dockerfile Outdated
@@ -15,11 +18,12 @@ RUN \
xmlstarlet \
uuid-runtime \
unrar \

Choose a reason for hiding this comment

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

Do you really need unrar and all of these programs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @ipeacocks, actually I am not an expert of the Plex Core and features, so I can't answer you about that.. Maybe @gbooker can bring you more details about that.

I just added udev because can be useful to access external storage like in my Raspberry project

Choose a reason for hiding this comment

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

A bit offtop. Why do you use 64 bit OS for Raspberry Pi. Raspberry advices to use 32-bits Raspbian even on 64-bits Pi 4.

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 use Pi 4 (RAM 4GB) with lots of containers on it (maria-db, postgre-db, web servers, python server, plex...) All of these is consuming lots of RAM and even more when several people are connected simultaneously. I am also using Ubuntu Server for ARM (no GUI), not Raspbian.

I haven't made deep benchmarks, but I can tell there is a significant gain in global performance and responsiveness using 64-bit with such a config. 32-bit architectures have per-process limits when it comes to use large amount of RAM.

Hope that's answering your question 🙂

Copy link

Choose a reason for hiding this comment

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

@ipeacocks just like @maxlabuche I also 64bit. I believe in the future, this will be the only option, its just a matter of time. I'd like to make the switch as early as possible. Plus the performance gains :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you really need unrar and all of these programs?

unrar is needed for some of the third-party plugins (subtitles IIRC) but maybe it's not needed anymore. The rest of the above are definitively needed.

I just added udev because can be useful to access external storage like in my Raspberry project

udev doesn't work as you would expect in containers. I'm skeptical of this and missed it in the first pass through.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK @gbooker for udev, we can skip it if not needed, I have also figured out other solutions. So what is missing to merge this PR? Do you have any concerns?

Copy link
Contributor

Choose a reason for hiding this comment

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

In my experimentation, udev didn't work inside the containers. We used to have it there but its lack of functionality prompted us to remove it.

We are still looking at getting our CI to build ARM images but that may take longer than expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @gbooker , do not hesitate to add new comments.

@maxlabuche
Copy link
Contributor Author

Why this can't be merged?

I submitted my work, seems like CI team is still reviewing / testing it.. Do you have any news @gbooker ? Thanks, hope you guys are doing great. Stay safe

@dekimsey
Copy link

dekimsey commented Apr 3, 2020

It might be worth renaming the Dockerfile to .aarch64 to follow the architecture being used. The file is currently arm64 but the code refers to aarch64.

@maxlabuche
Copy link
Contributor Author

@dekimsey You're right, but on Docker Hub 64-Bit ARM architectures are under the arm64 tag and not aarch64, that's why I'm using this naming method

Copy link
Contributor

@gbooker gbooker left a comment

Choose a reason for hiding this comment

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

Getting our CI to build ARM64 images is going to take longer so I'm going to target merging this without that. I think that udev shouldn't be in the image since it doesn't actually work in containers but otherwise I'm fine with the changes in this PR.

Dockerfile Outdated Show resolved Hide resolved
Dockerfile.arm64 Outdated Show resolved Hide resolved
@half2me
Copy link

half2me commented Apr 6, 2020

@gbooker not sure what you use in your CI to build arm images, but I've had success using docker buildx to cross-compile for arm. If there is anything I can help with, let me know.

@hexpunk
Copy link
Contributor

hexpunk commented Apr 6, 2020

@half2me Speaking of buildx, I'm trying to build using this setup (with my armhf addition) and buildx to make all the versions. But I'm not seeing how to tell buildx that it's one build but different Dockerfiles based on the architecture. Basically, it's trying to use Dockerfile for all the different architectures, which technically succeeds, but then all the binaries are wrong.

Any ideas on how to do what I'm describing so that pms-docker:latest would work for all supported architectures? Or should we maybe consider unifying the Dockerfiles into a single one that builds correctly no matter what architecture you're on?

@hexpunk
Copy link
Contributor

hexpunk commented Apr 7, 2020

FWIW, I rewrote my version to support x86, x86_64, aarch64, and armhf using a single Dockerfile so it's far more buildx friendly.

@half2me
Copy link

half2me commented Apr 7, 2020

@jayandcatchfire personally I like having a single Dockerfile whenever possible, but if there are too many differences between archs, you are better off with multiple Dockerfiles. You can simply tell docker buildx to use a specific Dockerfile for the build. If you give them all the same tag, they should be consolidated.

@maxlabuche
Copy link
Contributor Author

Getting our CI to build ARM64 images is going to take longer so I'm going to target merging this without that. I think that udev shouldn't be in the image since it doesn't actually work in containers but otherwise I'm fine with the changes in this PR.

@gbooker OK, that's done, I just removed udev as it seems unnecessary. I think we are good, you can merge if you do not have more requests.

@jayandcatchfire I like your work, for now I just wanted to add minor updates to this repo, that's why I didn't merge your PR to my branch. Also well suggested by @half2me , working on a multi-arch Dockerfile is definitely a good idea. Currently with the worldwide situation I can not put a lot of time on this project but I can help you if you need support implementing that (with buildx, scripting, etc.)

Thank you all for your very interesting reviews! Do not hesitate to @ me in your comments if you need further help. Take care.

@maxlabuche maxlabuche requested a review from gbooker April 7, 2020 13:02
@hexpunk
Copy link
Contributor

hexpunk commented Apr 7, 2020

@half2me Please tell me more about that. When I ran buildx on separate Dockerfiles serially, it replaced the latest tag on Docker Hub for me rather than adding to it.

@gbooker gbooker merged commit 4d069dc into plexinc:master Apr 7, 2020
@maxlabuche maxlabuche deleted the arm64-support branch April 7, 2020 16:34
Cornelicorn added a commit to Cornelicorn/pms-docker that referenced this pull request May 17, 2020
* Add arm64 support and Update Plex Download URLs to latest API (plexinc#48)

Add arm64 support and Update Plex Download URLs to latest API

* Create Dockerfile.armv7

Co-authored-by: Maxime Marmont <[email protected]>
Co-authored-by: Jay Sherby <[email protected]>
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.

6 participants