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 container environment variables supported by official Plex docker image #97

Open
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

runecalico
Copy link

I added support for a number of environment variables supported by the latest official plex container. I added them as optional, so if not specified would not be added or have a default, and added them explicitly. It looked like the current coding pattern was to specify the env options explicitly vs use a generic "pass any env to the container" method.

Thanks for the hard work in creating the helm chart in the first place, as well as the transcoder.

@runecalico
Copy link
Author

I have zero go coding skills, but I expect that if the primary container has the PLEX_UID and PLEX_GID environment variables set, that the transcoder pod would need to also use those UID/GIDs?

@onedr0p
Copy link
Contributor

onedr0p commented Apr 17, 2020

I wouldn't use underscores in the charts values name, use camelCase instead.

@runecalico
Copy link
Author

DONE. Is that a Style decision or is there a technical reason to use CamelCase over underscores. I don't personally mind either way, but I haven't much exposure to helm chart best practices or style guides..

@onedr0p
Copy link
Contributor

onedr0p commented Apr 17, 2020

Every chart I've seen is camelCase.

@wrender
Copy link

wrender commented Apr 20, 2020

Thanks for this helm chart. It is awesome. Maybe add these while you are at it?
NVIDIA_VISIBLE_DEVICES=all
NVIDIA_DRIVER_CAPABILITIES=compute,video,utility
CUDA_DRIVER_CAPABILITIES=compute,video,utility

@runecalico
Copy link
Author

@wrender - If this get's approved, I have another change waiting that will allow for arbitrary environment variables to be passed to the deployment, so in theory that change would allow for those variables as well as anything else you want. I don't recall seeing those as being part of the official plex docker image, and adding support for that was my focus on this PR.
Additionally I have another pending change that builds on the arbitrary env's to allow you to "set" preferences using env variables, effectively allowing you to update your preferences.xml using env values at boot.

@runecalico
Copy link
Author

runecalico commented Apr 21, 2020

@onedr0p - Is there a way to do some kind of multi-stage PR? I wanted to submit the PR's in somewhat digestible chunks, but it doesn't look like I can really do that from my fork, or perhaps it's the way I'm doing it? As I have a few additional things to add to the helm chart ..
image
I saw that munnez doesn't have the time to update code, I am hoping however that PR's can still be looked at/approved in a timely fashion.

@onedr0p
Copy link
Contributor

onedr0p commented Apr 21, 2020

I'm not a maintainer on this project. I would lump them into one PR unless you want to do individual ones.

However, there is a new chart made by billimek below. This one has removed the broken kube-plex feature and focuses more on running a single container of Plex rather than scaling it. I am a maintainer there and your PRs are welcome.

https://github.com/billimek/billimek-charts/tree/master/charts/plex

@runecalico
Copy link
Author

And your answer is basically what I was afraid of, in that maintenance of the entire repo is effectively in "mostly dead" mode, which pretty much was making me thinking about just maintaining my own helm chart ... Bleh..

Plex preferences setting using configmap, specify any ENV you want in the container, and enable it so transcode can be tmpfs (in-memory).
@wrender
Copy link

wrender commented Apr 22, 2020

That is unfortunate if the project maintainers aren't responding. I don't think you going off and doing your own thing really helps the community though...

@onedr0p
Copy link
Contributor

onedr0p commented Apr 22, 2020

@wrender there isn't much documentation on kube-plex or anyone who has stepped into do the work to support it. There also hasn't been any commits to the Go code in over 2 years. We've been trying to maintain the chart here, but it's at the mercy of the maintainers to except PRs, which often times is pretty slow at accepting. I am not complaining, I get people are busy with their lives. I am just saying maintaining the chart elsewhere is probably a wise idea, and why probably why @billimek chose to do it.

@wrender
Copy link

wrender commented Apr 22, 2020

Is it possible to reach out to the maintainer to get admin access to the project?

@onedr0p
Copy link
Contributor

onedr0p commented Apr 22, 2020

Open an issue and ask.

@wrender wrender mentioned this pull request Apr 22, 2020
@runecalico
Copy link
Author

I've started to add my work to the Billimek chart, while it does mean no transcoder fun .. at least the chart get's updated :)

@simplyzee
Copy link
Collaborator

simplyzee commented Apr 28, 2020

Hi all

Thanks for opening up another PR - I have spoken to @munnerz in regards to some of the outstanding PRs that need merging. We came up with the idea of moving kube-plex potentially into its own organization / for someone to adopt. In some cases, this would mean more people can then commit/review and maintain the repository then. I'd like to think we can focus on fixing some of the problems with the transcoder pod as I feel that is the most important feature of this repository considering the capabilities it can do.

There are improvements to be made... From having an environment that allows us to test PRs and deployments all the way to testing the transcoder mechanism. I've already started looking into addressing the transcoder issues in some of my spare time but I'd like to think others are also looking into this. I'm also limited in some regards being a contributor. Feel free to reach out to me if you like on the Kubernetes Slack - https://kubernetes.slack.com/archives/D6F1JHSL9

Thanks

@@ -29,12 +35,18 @@ The following tables lists the configurable parameters of the Plex chart and the
| `ingress.tls` | Ingress TLS configuration | `[]` |
| `rbac.create` | Create RBAC roles? | `true` |
| `nodeSelector` | Node labels for pod assignment | `beta.kubernetes.io/arch: amd64` |
| `configMap.plexPreferences.enabled` | Enable init script(and load configMap) that will read all environment variables starting with PLEX_PREFERENCE_ | `false` |
| `configMap.plexPreferences.name` | Name of configMap | `41-plex-preferences` |
Copy link
Owner

Choose a reason for hiding this comment

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

Why would a user want to configure/override this, given that it's created/managed/controlled by the Helm chart anyway? From what I can tell, the user doesn't ever have a need to set/override the name of this ConfigMap (only needing to set the options within it)

| `configMap.plexPreferences.enabled` | Enable init script(and load configMap) that will read all environment variables starting with PLEX_PREFERENCE_ | `false` |
| `configMap.plexPreferences.name` | Name of configMap | `41-plex-preferences` |
| `configMap.plexPreferences.defaultMode` | Unix permissions in container (0755) | `493` |
| `configMap.plexPreferences.mountPath` | The full path to the file we will mount the configMap to | `/etc/cont-init.d/41-plex-preferences` |
Copy link
Owner

Choose a reason for hiding this comment

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

When would someone want to customise these? Would it make more sense for us to set these appropriately always when the chart is installed? I'm conscious of exposing too many options and causing confusion, when instead the Helm chart should take care of non-relevant (to the user) bits like the mountPath, file mode etc.

# Official kube-plex container environment variables
{{- if .Values.advertiseIp }}
- name: "ADVERTISE_IP"
value: "{{.Values.advertiseIp}}"
Copy link
Owner

Choose a reason for hiding this comment

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

These vars seem unrelated to the PLEX_PREFERENCES bit right?

I'm a bit conscious that PLEX_PREFERENCES doesn't really 'protect' users from misconfigurations in any way, and could potentially lead to someone doing something 'bad' (e.g. setting ALLOWED_NETWORKS here, as well as setting PLEX_PREFERENCES_ALLOWED_NETWORKS).

Would it make more sense for us to remove PLEX_PREFERENCES_ and instead just add fields here for each preference we want to allow users to modify? If we have a standard pattern for how this is done, it should be trivial for us to extend the options we support in future

{{- end }}
{{- if .Values.allowedNetworks }}
- name: "ALLOWED_NETWORKS"
value: "{{include "joinListWithComma" .Values.allowedNetworks}}"
Copy link
Owner

Choose a reason for hiding this comment

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

I think it may be simpler to just make this {{ .Values.allowedNetworks }} and require the user to specify a comma separated list, but it's not a particular big deal either way 😄 (although it makes things a little simpler and reduces the Helm templating magic, which could well come back to bite 😅)

name: 41-plex-preferences
defaultMode: 493
mountPath: /etc/cont-init.d/41-plex-preferences
subPath: 41-plex-preferences
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not quite understanding why options to change the name, mountpath, subpath or mode of this configmap are exposed here, nor why enabled is exposed either.

I'd expect the init script to not take any action if these PLEX_PREFERENCE vars are not set (i.e. the default), and I don't see anywhere/any reason why a user would want to provide their own ConfigMap to use instead of this one.

If we want to allow a user to inject arbitrary shell code that is executed before PMS starts, I think a dedicated variable that indicates what it is (i.e. preStartShellScript) would be clearer.

As an alternative way of structuring this values.yaml, what about if there was:

# An optional list of Plex Preferences.xml keys/values that will be set/updated before the Plex Media Server process starts.
plexPreferences:
  FriendlyName: plex-kube-plex-test1
  EnableIPv6: "0"
  logDebug: "0"

etc?

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.

5 participants