Added liveness and readiness probe to pms container

[koiralasandesh]

Please let me know if you would like me to include any default probes.

[cilindrox]

Thanks @koiralasandesh - let's include some default probes as comments underneath their values.yaml definition - that way, users can uncomment and expect a working probe by default.

[koiralasandesh]

Thanks @koiralasandesh - let's include some default probes as comments underneath their values.yaml definition - that way, users can uncomment and expect a working probe by default.

@cilindrox can you please provide me some guidance on what you think would be good defaults for these probes?

I am personally using this in my values file, would you like me to add this as default or update anything?

livenessProbe: httpGet: path: /identity port: 32400 initialDelaySeconds: 60 periodSeconds: 60 timeoutSeconds: 1 failureThreshold: 3 readinessProbe: null

[cilindrox]

Thanks @koiralasandesh - let's include some default probes as comments underneath their values.yaml definition - that way, users can uncomment and expect a working probe by default.

@cilindrox can you please provide me some guidance on what you think would be good defaults for these probes?

I am personally using this in my values file, would you like me to add this as default or update anything?

livenessProbe: httpGet: path: /identity port: 32400 initialDelaySeconds: 60 periodSeconds: 60 timeoutSeconds: 1 failureThreshold: 3 readinessProbe: null

sounds good - either /identity or /web work here - maybe we can do /web for readiness?

[gbooker]

I'm not certain of the definitions of liveness and readiness but /identity and /web are not going to behave that differently. Both are going to be fairly lightweight but /web will yield a redirect which may not be desirable.

Note: when PMS is starting up, running database migrations, and essentially not quite ready to respond to HTTP requests, these endpoint will all respond but with a 503. While there are some other cases where a 503 is returned on endpoints, this is the only case it is returned on these two. This would be better to make sure it doesn't result in a container being restarted while yielding a 503 because if it's a particularly long db migration, the restart will just result in it being run again from the beginning and will indefinitely until the migration is allowed enough time to complete.

[cilindrox]

@gbooker the restart scenario would be covered by the readiness checks, then the control is handed to the liveness probes. Good point about the redirect too, guess /identity should be used for both in that case.

[MarshallAsch]

looks good to me, same comment as Gaston about the commented defaults.

Also re-run the command helm-docs to update the readme with the new values, (probably after you add the comment)

[koiralasandesh]

Hello @cilindrox I have added commented liveness and readiness checks as per our discussion.

However, I am not much familiar with helm-docs. I installed helm-docs as per @MarshallAsch's instructions and tried to generate the docs. But, the generated one was quite different from the one in the repo. It only has the values table and they are missing the comment descriptions.

Can you point me to an instruction or Readme that might help me understand how I can update the docs?

[MarshallAsch]

I'll take care of the docs then, I'll push a commit tomorrow probably. (I had a work in progress to switch the doc tool I might have mixed up if I had merged that one or not)

[cilindrox]

LGTM 👍 - thanks @koiralasandesh

[jpflouret]

It appears to me that this is incorrectly nested within resources due to the line above conatining with .Values.pms.resources.