twuni
feat: add logic to support custom liveness and readiness probes
Closes #54
Retains current liveness and readiness probe logic while adding the ability to override the defaults.
During local manual verification after cloning this branch locally, I expected to be able to install the chart with --set readinessProbe.httpGet.path=/metrics to patch just the path of the readiness probe without impacting the rest of its configuration. Instead, I got an error for missing required field "port". As an end-user, I don't know what value I need to provide for that, because port 5000 is hard-coded into the chart as the port on which the container is listening. Further, in the default scenario, the chart decides when to set the scheme based on the presence of a TLS secret.
Upon closer inspection, then, and for the above reasons, I'm not sure that a full-on livenessProbe and readinessProbe override is an appropriate solution. It simply requires too much intermingled knowledge of anxilliary parts of the chart.
Based on the ask in #54, a more targeted solution may be more appropriate and less cumbersome.
To help in narrowing down on a solution:
- Is it the livenessProbe or the readinessProbe (or both) where the failure count is too low? Maybe only one or the other needs to be adjusted. If they're essentially the same (which in the chart currently, they're identical), then it may be a signal for the chart to make them configurable as a set.
- At what point are the failures happening? If it's as your registry is being launched, then the default
initialDelaySecondsmay be too low for you (and likely others).
Here are the timing and threshold related properties on liveness and readiness probes, for reference:
- [ ]
failureThreshold - [ ]
initialDelaySeconds - [ ]
periodSeconds - [ ]
successThreshold - [ ]
terminationGracePeriodSeconds - [ ]
timeoutSeconds
My soft suggestion here would be to, instead of allowing the whole livenessProbe and readinessProbe blocks to be overridden, to support overriding each of these individual properties above, for each.
A simpler implementation might be to merge any configured livenessProbe into the default one, so the livenessProbe value becomes a patch rather than a replacement. It's much more powerful than it needs to be, though, to remedy the false-negatives reported in #54, and opens up a higher probability of misconfigured or dysfunctional probes.
@AbrohamLincoln Hey, sorry for what may have come across as shutting down your good work here. I'd really like to address the core issue! Any thoughts or suggestions on how to move forward?
Sorry, I've been offline since making that last commit. I made these changes to work around an edge case I encountered involving PKI authentication. I ended up completely overriding the probes in my scenario. The default settings in the chart seem to be fine for most use cases. We can add some additional settings if you'd like, but I always appreciate having a mechanism to allow for advanced configurations.
On Mon, Aug 15, 2022, 5:44 PM Devin Canterberry @.***> wrote:
@AbrohamLincoln https://github.com/AbrohamLincoln Hey, sorry for what may have come across as shutting down your good work here. I'd really like to address the core issue! Any thoughts or suggestions on how to move forward?
— Reply to this email directly, view it on GitHub https://github.com/twuni/docker-registry.helm/pull/57#issuecomment-1215888003, or unsubscribe https://github.com/notifications/unsubscribe-auth/APMRDFHAA4N7QJYLAN4F6P3VZK25JANCNFSM5P4N33SA . You are receiving this because you were mentioned.Message ID: @.***>