feat: add logic to support custom liveness and readiness probes

[AbrohamLincoln]

Closes #54

Retains current liveness and readiness probe logic while adding the ability to override the defaults.

[vladciobancai]

Are there any plans for this PR to be merged ?

[canterberry]

Spurious whitespace changes here. Please remove.

[AbrohamLincoln]

Removes spaces. Yamllint has a rule that requires two spaces between content and comments by default.

[canterberry]

Ahh, interesting. I've never seen that rule before. Thanks for the context! I didn't know yamllint existed, and I'm glad it does. CI for this repo is currently doing helm lint --strict, which doesn't enforce nearly as much as one might naively expect it to. We should add yamllint to this repo's CI and get a PR together to fix any current violations so we have a basis for code style consistency.

[canterberry]

Thanks for contributing! Just made a review pass and have quite minor blocking bits of feedback (just about adding a prefix to the new variable definitions and omitting extraneous whitespace changes). With that, I have no concerns with 👍 and having this merged.

[canterberry]

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 initialDelaySeconds may 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.

[canterberry]

@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?

[AbrohamLincoln]

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 <#57 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/APMRDFHAA4N7QJYLAN4F6P3VZK25JANCNFSM5P4N33SA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[edwargix]

nit: add \n