bitnami / kube-libsonnet

Bitnami's jsonnet library for building Kubernetes manifests
https://bitnami.com
Apache License 2.0
174 stars 50 forks source link

Removes replicas assert #19

Closed jasongwartz closed 5 years ago

jasongwartz commented 5 years ago

First of all, thanks for the great library! We are using it in dozens of applications at Ecosia.org, and have built lots of whole application presets on top.

In some cases, we manually scale down deployments to zero replicas. I think it's totally valid to have the assert on the replicas field, but zero is also a valid value.

Also, we use the HPA to autoscale, and including the replicas field when using an HPA can lead to weird behaviour (see https://github.com/kubernetes/kubernetes/issues/25238). I added here a hide_replicas hidden field to the deploy spec, which will remote the replicas key from the spec (open to suggestions on a better way to do that - but I wanted to make it backwards-compatible for anyone who relies on the default being replicas: 1).

anguslees commented 5 years ago

Thanks for the suggestion!

Adding a new option just to say that it was ok we disabled the other option we added seems like a step backwards :P

I counter propose the simple/obvious step of just removing the assert replicas >= 1.

In particular, this allows setting replicas: 0 and even replicas:: null (or replicas:: error "unknown replicas!") to remove replicas from the payload, while preserving the current default of replicas: 1

The downside of removing the assert is that any other jsonnet code that tries to derive something from the replicas value (like kube.HorizontalPodAutoscaler.spec.minReplicas) might produce a confusing/erroneous result and it will be harder to debug because there wasn't an easy assert replicas >=1 to draw your attention. I think this is a completely fine tradeoff, and something we have in many places elsewhere in jsonnet / kube.libsonnet.

Thoughts?

jasongwartz commented 5 years ago

agreed that it's a bit of a hack like this - if you're happy to remove the assert, then that works for our use-case perfectly :smile:

I'll update this PR shortly