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.

Sign up for GitHub

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

feat: Implement livenessProbe variables and readinessProbe variables in helm chart #2906

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

feat: Implement livenessProbe variables and readinessProbe variables in helm chart #2906

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
01408d7
Implement livenessProbe variables and readinessProbe variables
stefanandres Nov 7, 2024
c17f141
Remove templated tls.enabled in probe
stefanandres Dec 4, 2024
641be50
Add documentation to values
stefanandres Dec 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions charts/kargo/templates/api/deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,8 +85,11 @@ spec:
- name: h2c
containerPort: 8080
protocol: TCP

{{- if .Values.api.probes.enabled }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think with the probes fully configurable, this option becomes redundant and should be removed from the template and the values.yaml file.

It is a breaking change, but it's a low-impact breaking change:

  • Exists mainly to support local development.
  • Anyone who may have disabled the probes already (in non-local dev envs), if this breaking change were to catch them by surprise, they'd be gaining new probes rather than losing anything. Whatever disruption that might cause should be easily identified and corrected.

I will of course mention a change such as this prominently in the release notes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It occurs to me that removing this option will require some slight modifications to hack/tilt/values.dev.yaml. You'll just have to nil out the probes for a couple of components.

Copy link
Author

@stefanandres stefanandres Dec 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, I'll just remove the tls-variable for the probe then

In tilt the probes are already disabled, so we shouldn't need to override the exec there

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to mention it here: I'd personally leave the tls variable in the template. Removing it adds manual toil to deployments (like ours) because you'll be confused why the probe is failing when TLS is disabled.
in our case, we don't use TLS here because our ingress is already doing TLS

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In tilt the probes are already disabled, so we shouldn't need to override the exec there

This suggests some misunderstanding.

I think with the probes fully configurable, this option becomes redundant and should be removed from the template and the values.yaml file.

"This option," here referred to api.probes.enabled. There is no sense in having an enabled boolean when having or not having a liveness probe can simply depend on whether api.probes.livenessProbe is defined or not. Same can be said for the readiness probes.

Having said that, the following may change my line of thought...

Just to mention it here: I'd personally leave the tls variable in the template. Removing it adds manual toil to deployments (like ours) because you'll be confused why the probe is failing when TLS is disabled.

That wasn't even on my radar when I initially reviewed, but you're right. I believe this exposes the fact that it's probably not so ideal to put the definitions of the probes fully within the control of the user. It's not just about TLS. It altogether eliminates any (current or future) possibility of probes being configured conditionally based on based on other settings throughout the chart. TLS being enabled or not is just one such example.

This drives me toward thinking that we ought not let users define livenessProbe and readinessProbe in their entirety, but should, instead, surface individual options for things like initialDelaySeconds, periodSeconds, timeoutSeconds, etc., etc.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This drives me toward thinking that we ought not let users define livenessProbe and readinessProbe in their entirety, but should, instead, surface individual options for things like initialDelaySeconds, periodSeconds, timeoutSeconds, etc., etc.

I'd argue to go with my first implementation. With this, all options except the exec.command variables would be setable.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"This option," here referred to api.probes.enabled. There is no sense in having an enabled boolean when having or not having a liveness probe can simply depend on whether api.probes.livenessProbe is defined or not. Same can be said for the readiness probes.

Yep, that makes sense. But I'd leave that if we keep part of the configuration as helm templates (as suggested in my comment above)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd argue to go with my first implementation. With this, all options except the exec.command variables would be setable.

Have a look at something like #3041 where we're looking to change how one of the probes is implemented.

Changes such as this would become much more difficult if we leave things too configurable.

livenessProbe:
{{- with .Values.api.probes.livenessProbe }}
livenessProbe: {{- toYaml . | nindent 12 }}
{{- end }}
Comment on lines +90 to +92
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The following 8 lines are also part of the probe.

exec:
command:
- /usr/local/bin/grpc_health_probe
Expand All @@ -95,8 +98,9 @@ spec:
- -tls
- -tls-no-verify
{{- end }}
initialDelaySeconds: 10
readinessProbe:
{{- with .Values.api.probes.readinessProbe }}
readinessProbe: {{- toYaml . | nindent 12 }}
{{- end }}
Comment on lines +93 to +95
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The following 8 lines are also part of the probe.

exec:
command:
- /usr/local/bin/grpc_health_probe
Expand All @@ -105,7 +109,6 @@ spec:
- -tls
- -tls-no-verify
{{- end }}
initialDelaySeconds: 5
{{- end }}
{{- if or .Values.kubeconfigSecrets.kargo (and .Values.api.oidc.enabled .Values.api.oidc.dex.enabled) .Values.api.tls.enabled .Values.api.cabundle.configMapName .Values.api.cabundle.secretName }}
volumeMounts:
Expand Down
26 changes: 7 additions & 19 deletions charts/kargo/templates/dex-server/deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,25 +74,13 @@ spec:
resources:
{{- toYaml .Values.api.oidc.dex.resources | nindent 10 }}
{{- if .Values.api.oidc.dex.probes.enabled }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as this comment.

livenessProbe:
httpGet:
path: /healthz/live
port: 5558
initialDelaySeconds: 10
periodSeconds: 10
timeoutSeconds: 1
successThreshold: 1
failureThreshold: 300
readinessProbe:
httpGet:
path: /healthz/ready
port: 5558
initialDelaySeconds: 10
periodSeconds: 10
timeoutSeconds: 1
successThreshold: 1
failureThreshold: 300
{{- end }}
{{- with .Values.api.oidc.dex.probes.livenessProbe }}
livenessProbe: {{- toYaml . | nindent 12 }}
{{- end }}
{{- with .Values.api.oidc.dex.probes.readinessProbe }}
readinessProbe: {{- toYaml . | nindent 12 }}
{{- end }}
{{- end }}
volumes:
- name: config
projected:
Expand Down
24 changes: 23 additions & 1 deletion charts/kargo/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,11 @@ api:
probes:
## @param api.probes.enabled Whether liveness and readiness probes should be included in the API server deployment. It is sometimes advantageous to disable these during local development.
enabled: true
livenessProbe:
initialDelaySeconds: 10

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Killing this blank line would visually make it more obvious that readinessProbe below is a component of this block.

readinessProbe:
initialDelaySeconds: 5

tls:
## @param api.tls.enabled Whether to enable TLS directly on the API server. This is helpful if you do not intend to use an ingress controller or if you require TLS end-to-end. All other settings in this section will be ignored when this is set to `false`.
Expand Down Expand Up @@ -283,7 +288,24 @@ api:
probes:
## @param api.oidc.dex.probes.enabled Whether liveness and readiness probes should be included in the Dex server deployment. It is sometimes advantageous to disable these during local development.
enabled: true

livenessProbe:
httpGet:
path: /healthz/live
port: 5558
initialDelaySeconds: 10
periodSeconds: 10
timeoutSeconds: 1
successThreshold: 1
failureThreshold: 300
readinessProbe:
httpGet:
path: /healthz/ready
port: 5558
initialDelaySeconds: 10
periodSeconds: 10
timeoutSeconds: 1
successThreshold: 1
failureThreshold: 300
tls:
## @param api.oidc.dex.tls.selfSignedCert Whether to generate a self-signed certificate for use with Dex. If `true`, `cert-manager` CRDs **must** be present in the cluster. Kargo will create and use its own namespaced issuer. If `false`, a cert secret named `kargo-dex-server-cert` **must** be provided in the same namespace as Kargo. There is no provision for running Dex without TLS.
selfSignedCert: true
Expand Down