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

stefanandres · 2024-11-07T09:24:27Z

It's sometimes needed to configure certain timeouts. This PR makes all the settings available as helm variables.

netlify · 2024-11-07T09:24:42Z

✅ Deploy Preview for docs-kargo-io ready!

Name	Link
🔨 Latest commit	`641be50`
🔍 Latest deploy log	https://app.netlify.com/sites/docs-kargo-io/deploys/675051de7a97b9000825ea4b
😎 Deploy Preview	https://deploy-preview-2906.docs.kargo.io
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

It's sometimes needed to configure certain timeouts. This PR makes all the settings available as helm variables. Signed-off-by: Stefan Andres <[email protected]>

krancour · 2024-11-07T14:50:13Z

I'm supportive of this change, but there are a few issues that need to be resolved:

This only adds the options to configure liveness and readiness probes to the API server and the Dex server. For consistency (and because it's useful), the same options should also be added to the controller, management controller, and webhook server.
Inline documentation/metadata is required for the new fields in values.yaml. This isn't because I assume users require much instruction on liveness or readiness probes; rather it is because we generate the chart's README.md from metadata in the values.yaml file. Without proper metadata having been applied to these new fields, even the existence of these options will not be surfaced in the README.md.
A few other issues, that I will call out inline in the diffs.

krancour · 2024-11-07T14:53:47Z

charts/kargo/templates/api/deployment.yaml

+          {{- with .Values.api.probes.livenessProbe }}
+          livenessProbe: {{- toYaml . | nindent 12 }}
+          {{- end }}


The following 8 lines are also part of the probe.

krancour · 2024-11-07T14:54:17Z

charts/kargo/templates/api/deployment.yaml

+          {{- with .Values.api.probes.readinessProbe }}
+          readinessProbe: {{- toYaml . | nindent 12 }}
+          {{- end }}


The following 8 lines are also part of the probe.

stefanandres · 2024-11-07T14:58:35Z

Thanks for the review, I'll see that I do the changes.

I would have changed other deployments if they'd already used probes, but can add placeholder for them as well.

krancour · 2024-11-07T14:59:27Z

charts/kargo/templates/api/deployment.yaml

@@ -85,8 +85,11 @@ spec:
            - name: h2c
              containerPort: 8080
              protocol: TCP
+
 {{- if .Values.api.probes.enabled }}


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.

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.

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

https://github.com/akuity/kargo/blob/main/hack/tilt/values.dev.yaml#L8

https://github.com/akuity/kargo/blob/main/hack/tilt/values.dev.yaml#L22

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

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.

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.

"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)

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.

krancour · 2024-11-07T14:59:57Z

charts/kargo/templates/dex-server/deployment.yaml

@@ -74,25 +74,13 @@ spec:
        resources:
          {{- toYaml .Values.api.oidc.dex.resources | nindent 10 }}
        {{- if .Values.api.oidc.dex.probes.enabled }}


Same as this comment.

krancour · 2024-11-07T15:01:05Z

charts/kargo/values.yaml

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


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

krancour · 2024-11-07T15:05:37Z

rather it is because we generate the chart's README.md from metadata in the values.yaml file

Running make codegen-docs will bring the chart's README.md up to date after any modifications to values.yaml.

stefanandres · 2024-12-04T10:24:56Z

rather it is because we generate the chart's README.md from metadata in the values.yaml file

Running make codegen-docs will bring the chart's README.md up to date after any modifications to values.yaml.

This fails for me, how do I fix that?

❯ make codegen-docs
npm install -g @bitnami/readme-generator-for-helm
(node:9158) ExperimentalWarning: CommonJS module /opt/homebrew/lib/node_modules/npm/node_modules/debug/src/node.js is loading ES Module /opt/homebrew/lib/node_modules/npm/node_modules/supports-color/index.js using require().
Support for loading ES Module in require() is an experimental feature and might change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
npm warn deprecated [email protected]: This module is not supported, and leaks memory. Do not use it. Check out lru-cache if you want a good and tested way to coalesce async requests by a key value, which is much more comprehensive and powerful.
npm warn deprecated [email protected]: Glob versions prior to v9 are no longer supported

added 19 packages in 3s

2 packages are looking for funding
  run `npm fund` for details
npm notice
npm notice New patch version of npm available! 10.9.0 -> 10.9.1
npm notice Changelog: https://github.com/npm/cli/releases/tag/v10.9.1
npm notice To update run: npm install -g [email protected]
npm notice
bash hack/helm-docs/helm-docs.sh
Skipping check for kubeconfigSecrets
INFO: Checking missing metadata...


######
The following errors must be fixed before proceeding
######


ERROR: Missing metadata for key: api.probes.livenessProbe.initialDelaySeconds
ERROR: Missing metadata for key: api.probes.livenessProbe.exec.command
ERROR: Missing metadata for key: api.probes.readinessProbe.initialDelaySeconds
ERROR: Missing metadata for key: api.probes.readinessProbe.exec.command
ERROR: Missing metadata for key: api.oidc.dex.probes.livenessProbe.httpGet.path
ERROR: Missing metadata for key: api.oidc.dex.probes.livenessProbe.httpGet.port
ERROR: Missing metadata for key: api.oidc.dex.probes.livenessProbe.initialDelaySeconds
ERROR: Missing metadata for key: api.oidc.dex.probes.livenessProbe.periodSeconds
ERROR: Missing metadata for key: api.oidc.dex.probes.livenessProbe.timeoutSeconds
ERROR: Missing metadata for key: api.oidc.dex.probes.livenessProbe.successThreshold
ERROR: Missing metadata for key: api.oidc.dex.probes.livenessProbe.failureThreshold
ERROR: Missing metadata for key: api.oidc.dex.probes.readinessProbe.httpGet.path
ERROR: Missing metadata for key: api.oidc.dex.probes.readinessProbe.httpGet.port
ERROR: Missing metadata for key: api.oidc.dex.probes.readinessProbe.initialDelaySeconds
ERROR: Missing metadata for key: api.oidc.dex.probes.readinessProbe.periodSeconds
ERROR: Missing metadata for key: api.oidc.dex.probes.readinessProbe.timeoutSeconds
ERROR: Missing metadata for key: api.oidc.dex.probes.readinessProbe.successThreshold
ERROR: Missing metadata for key: api.oidc.dex.probes.readinessProbe.failureThreshold
/opt/homebrew/lib/node_modules/@bitnami/readme-generator-for-helm/lib/checker.js:93
    throw new Error('ERROR: Wrong metadata!');
    ^

Error: ERROR: Wrong metadata!
    at checkKeys (/opt/homebrew/lib/node_modules/@bitnami/readme-generator-for-helm/lib/checker.js:93:11)
    at getParsedMetadata (/opt/homebrew/lib/node_modules/@bitnami/readme-generator-for-helm/index.js:26:3)
    at runReadmeGenerator (/opt/homebrew/lib/node_modules/@bitnami/readme-generator-for-helm/index.js:52:28)
    at Object.<anonymous> (/opt/homebrew/lib/node_modules/@bitnami/readme-generator-for-helm/bin/index.js:22:1)
    at Module._compile (node:internal/modules/cjs/loader:1546:14)
    at Object..js (node:internal/modules/cjs/loader:1698:10)
    at Module.load (node:internal/modules/cjs/loader:1303:32)
    at Function._load (node:internal/modules/cjs/loader:1117:12)
    at TracingChannel.traceSync (node:diagnostics_channel:322:14)
    at wrapModuleLoad (node:internal/modules/cjs/loader:218:24)

Node.js v23.3.0
make: *** [codegen-docs] Error 1

Edit: ah, that probably means if wants to have a description for every defined variable.
Edit2: fixed

Signed-off-by: Stefan Andres <[email protected]>

stefanandres force-pushed the probe-variable branch 4 times, most recently from 959aa1b to c468b6f Compare November 7, 2024 09:51

stefanandres changed the title ~~Implement livenessProbe variables and readinessProbe variables~~ feat: Implement livenessProbe variables and readinessProbe variables Nov 7, 2024

stefanandres changed the title ~~feat: Implement livenessProbe variables and readinessProbe variables~~ feat: Implement livenessProbe variables and readinessProbe variables in helm chart Nov 7, 2024

stefanandres marked this pull request as ready for review November 7, 2024 09:58

stefanandres requested a review from a team as a code owner November 7, 2024 09:58

Implement livenessProbe variables and readinessProbe variables

01408d7

It's sometimes needed to configure certain timeouts. This PR makes all the settings available as helm variables. Signed-off-by: Stefan Andres <[email protected]>

stefanandres force-pushed the probe-variable branch from c468b6f to 01408d7 Compare November 7, 2024 10:13

krancour reviewed Nov 7, 2024

View reviewed changes

krancour assigned stefanandres and krancour Nov 7, 2024

krancour added the area/charts label Nov 7, 2024

krancour added this to the v1.1.0 milestone Nov 7, 2024

krancour added kind/enhancement priority/normal labels Nov 7, 2024

krancour removed this from the v1.1.0 milestone Nov 15, 2024

stefanandres requested a review from krancour December 4, 2024 10:23

stefanandres added 2 commits December 4, 2024 13:57

Remove templated tls.enabled in probe

c17f141

Signed-off-by: Stefan Andres <[email protected]>

Add documentation to values

641be50

Signed-off-by: Stefan Andres <[email protected]>

stefanandres force-pushed the probe-variable branch from 63890e6 to 641be50 Compare December 4, 2024 12:58

krancour added this to the v1.2.0 milestone Dec 9, 2024

hiddeco mentioned this pull request Dec 9, 2024

chore(chart): start using Kubernetes' builtin gRPC probe #3041

Merged

krancour removed this from the v1.2.0 milestone Dec 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

stefanandres commented Nov 7, 2024

netlify bot commented Nov 7, 2024 •

edited

Loading

krancour commented Nov 7, 2024

krancour Nov 7, 2024

krancour Nov 7, 2024

stefanandres commented Nov 7, 2024

krancour Nov 7, 2024

krancour Nov 7, 2024

stefanandres Dec 4, 2024 •

edited

Loading

stefanandres Dec 4, 2024

krancour Dec 4, 2024

stefanandres Dec 4, 2024

stefanandres Dec 4, 2024

krancour Dec 9, 2024

krancour Nov 7, 2024

krancour Nov 7, 2024

krancour commented Nov 7, 2024

stefanandres commented Dec 4, 2024 •

edited

Loading

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

Are you sure you want to change the base?

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

Conversation

stefanandres commented Nov 7, 2024

netlify bot commented Nov 7, 2024 • edited Loading

✅ Deploy Preview for docs-kargo-io ready!

krancour commented Nov 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanandres commented Nov 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanandres Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krancour commented Nov 7, 2024

stefanandres commented Dec 4, 2024 • edited Loading

netlify bot commented Nov 7, 2024 •

edited

Loading

stefanandres Dec 4, 2024 •

edited

Loading

stefanandres commented Dec 4, 2024 •

edited

Loading