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

Max world size inconsistent between Docker image and Helm default values.yaml #143

Open
JJGadgets opened this issue Feb 22, 2023 · 8 comments

Comments

@JJGadgets
Copy link

I am switching my Minecraft from my Docker host to my Kubernetes cluster.
On the Docker side, I can't find any defaults for the envvar of the maximum world size, and when /worldborder get is run in-game, it shows what I understand to be the Minecraft default of 5999968, or roughly 60 million.
Meanwhile, the Helm Chart's default values.yaml shows that the Helm default is set to maxWorldSize: 10000.

This could cause an imported world to be significantly altered to fit the new constraints, which is undesired.
Could the default value for maxWorldSize be changed to the Minecraft default or at least be consistent with the behavior observed when the same image is deployed on Docker?

@itzg
Copy link
Owner

itzg commented Feb 22, 2023

Oh wow, I didn't notice all of those were preset to non-default values. I inherited this chart so was long before my time. ALL of those values need to be made conditional, which looks like a effort 😡.

I don't even use Helm, so I would recommend either managing the manifest files directly or using the built in kustomize basing off of https://github.com/itzg/docker-minecraft-server/tree/master/kustomize

@JJGadgets
Copy link
Author

For me personally I've set this value manually in my HelmRelease (I use FluxCD rather than helm install directly): https://github.com/JJGadgets/Biohazard/blob/main/kube/3-deploy/2-apps/minecraft/3-install.yaml#L35

Would you be open to a PR? I'll see what I can do, as far as I see there are others using this Helm Chart too so it would be beneficial.

@itzg
Copy link
Owner

itzg commented Feb 23, 2023

Definitely open to a PR. I'm pondering whether

  • an unset value would indicate the server.properties default should be left as is. Downside is that values.yaml discovery is lessened since all those lines would need to be commented out by default
  • a special value like "default" could be specified. Downside is that it will look a little odd especially for numerical values

Either way the repetition in deployment.yaml hurts my eyes. With either of the above I'd like a template-macro to be used that is given the values variable and corresponding environment variable name. The conditional can be implemented once in the macro.

Actually, I see you using a lot of extraEnv entries. I'd really prefer it all be done that way since the maintenance of value mappings here in the chart is highly redundant maintenance.

@JJGadgets
Copy link
Author

Actually, I see you using a lot of extraEnv entries. I'd really prefer it all be done that way since the maintenance of value mappings here in the chart is highly redundant maintenance.

If envvars are the way forward, and I believe so too since the minecraftServer block simply maps to envvars anyway, perhaps this Chart could take an approach similar to the "app-template" Chart by @bjw-s where the Chart is a generic template that implements values for commonly used components, accepts most images that simply need "bare essentials" such as a Service, Deployment/similar, persistence, ingress etc, and leaves configuration of the service itself to either envvars or a volume mount, which from my understanding is similar to what you're looking for.

This way, the parts of the minecraftServer block that pertains to configuration of the server's behavior itself can be removed in favor of envvars, and the rest of the components that pertain to configuring only the Kubernetes components or other components can adopt a similar style to app-template.

An example of me using app-template with envvar configuration can be found in my repo: https://github.com/JJGadgets/Biohazard/blob/main/kube/3-deploy/2-apps/whoogle/2-install.yaml (I patch the Chart specs from outside this HelmRelease cuz I'm lazy to repeat it and it's homelab :p)

@gilesknap
Copy link
Contributor

@JJGadgets I think I like your suggestion. I have quite a few charts to test your PR against if that helps.

It seems like the proposal is a breaking change for existing charts. Any ideas on how to mitigate that?

@itzg
Copy link
Owner

itzg commented Feb 23, 2023

I totally agree with you also.

It's the backward compatibility that is now the big challenge. It'll probably take a whole brand new chart to get people migrating to the cleaner baseline.

That's partly why I added first class support for kustomize since it started out clean and kustomize in general encourages clean, natural manifests

https://github.com/itzg/docker-minecraft-server/tree/master/kustomize

@billimek
Copy link
Collaborator

If you want to adopt the app-template "chart" from @bjw-s, there is nothing to do beyond deprecate this chart because it would replace this one.

You would probably want to share an example chart configuration to use with 'app-template' to deploy and run minecraft-server, but there is not migration path forward from this chart beyond not using it anymore.

Also keep in mind that you will beholden to whatever changes @bjw-s may make app-template "chart" which may or may not break how folks run the minecraft-server. It's essentially a template of a template for deploying via helm.

@itzg
Copy link
Owner

itzg commented Feb 23, 2023

Thank you for snapping me back to reality @billimek

Alternate charts are always welcome to be used. That's the beauty of open source.

I think I'll try out the macro idea to allow for null'ing out values that one would want to fallback to server.properties default.

billimek added a commit to billimek/k8s-gitops that referenced this issue Jun 18, 2023
... The minecraft default is 5999968.  See [this issue](itzg/minecraft-server-charts#143) for background.

Signed-off-by: Jeff Billimek <[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

No branches or pull requests

4 participants