pabloromeo / clusterplex

ClusterPlex is an extended version of Plex, which supports distributed Workers across a cluster to handle transcoding requests.
MIT License
452 stars 35 forks source link

Improvements for the helm chart #235

Closed brandan-schmitz closed 1 year ago

brandan-schmitz commented 1 year ago

I have a few improvements to the helm chart that should help prevent users from installing clusterplex in an unsupported configuration. The following changes were made:

Support for enabling/disabling the local relay

Adds two new variables to the pms.config section:

If localRelayEnabled is set to true then the pms container will contain the LOCAL_RELAY_ENABLED=1, the LOCAL_RELAY_PORT and the PMS_SERVICE variables.

However, if localRelayEnabled is set to false the pms container will contain the LOCAL_RELAY_ENABLED=0 and the PMS_IP variables. If set to false, the user must also provide a valid IP address in the pmsIP field. There is a verification on this, if the user leaves it blank or it does not pass a regex verification for a IP format then it displays a error to the user and the helm chart does not get installed.

image


Uses correct TRANSCODE_OPERATING_MODE if workers are disabled

The chart now overrides the transcodeOperatingMode option if a user disables the workers by setting workers.enabled to false. It will ensure that the TRANSCODE_OPERATING_MODE variable is set to local if no workers are present.


Prevent 0 replicas of the worker when it is enabled

This builds off the above change to prevent invalid worker configuration. It now ensures that the worker.config.replicas option cannot be set to 0 since this would effectively disable the workers without letting the above change take effect to ensure the proper TRANSCODE_OPERATING_MODE is set.

image


Increase default delay for worker probes

Sometimes the liveness and readiness probe for the worker was causing it to restart unexpectedly when first installing the chart if the process of downloading codecs took longer than 60 seconds. I increased the time before k8s starts checking this probe to be 120 seconds to alleviate this.


Additional Changes

Aside from changes to the helm chart logic, I made the following changes as well:


It seems GitHub is wanting to include the previous commits from my previous PR as well as it seems the history did not quite line up. However only the new changes I made in the Updates for the helm chart commit are actually showing so this should be safe to merge.

brandan-schmitz commented 1 year ago

It might be a good idea to actually have a separate tag for the helm chart as well. That would allow for releasing a helm chart update without releasing a clusterplex update, yet still preventing any push to the master branch from causing a overwrite in the last released version of the chart and only have it be updated when desired.

Using a tag like chart-v1.1.0 would be ideal, then in the .github/workflows/chart.yaml file under the tags you can add a line for matching that as well: - 'chart-v*.*.*' so that it will run anytime a chart or clusterplex release is created.

pabloromeo commented 1 year ago

Awesome! I'll take a look. Yes, I agree that a separate tag for chart changes would be great. We should probably also exclude changes to the Chart from triggering a build of the applications.

brandan-schmitz commented 1 year ago

Excluding the chart changes from triggering a build of the applications is a good idea. I added a commit that adds the chart tag to the chart pipeline as well as adds a paths-ignore section to the Docker Builds and DockerMods Builds actions.

That will make it so that if the only file changes are within the charts folder it will not trigger those two actions. However if there are also changes outside that folder then it will be triggered. Allows them to still function if you make a change to the chart and to one of the apps.