Skip to content

feat: make the probe timings configurable - #5712

Merged
Matthew Christopher (matthchr) merged 2 commits into
Azure:mainfrom
andreidorin-oprea:feat/configurable-probe-timings
Sep 21, 2026
Merged

Matthew Christopher (matthchr) merged 2 commits into
Azure:mainfrom
andreidorin-oprea:feat/configurable-probe-timings

Conversation

@andreidorin-oprea

Copy link
Copy Markdown
Contributor

What this PR does

Exposes the liveness, readiness and startup probe timings as Helm values. The defaults are exactly what the chart applies today, so this is a no-op unless you set them.

probes:
  startup:
    periodSeconds: 10
    failureThreshold: 12
    timeoutSeconds: 1
  liveness:
    periodSeconds: 10
    failureThreshold: 3
    timeoutSeconds: 1
  readiness:
    periodSeconds: 10
    failureThreshold: 3
    timeoutSeconds: 1

Paths and ports stay fixed - they have never been the thing people needed to change.

Why

The probes have been retuned in place three times, each time to suit whichever cluster the reporter had:

PR change reason
#2254 added the liveness probe -
#2844 raised initialDelaySeconds "operator pod was running into timeouts on my arm64 mac machine ... so operator gets more time to load and apply CRDs"
#4791 replaced it with a startup probe at 10s x 12 that delay now penalised fast startups: "ASO finish starting up after about 10s, then idle for an extra ~45s"

#2844 made startup too slow for small clusters, and #4791 made it too tight for large ones. We are at the far end of that range: the operator applies its CRDs and starts a controller per installed CRD before it serves /healthz, and on our production cluster - roughly 800 CRDs - that outlasts the 120s budget, so the kubelet kills it mid-apply and it never finishes starting.

Our only workaround today is a Helm post-renderer that rewrites the probe after the chart has rendered, which breaks silently whenever the chart's deployment changes shape.

Rather than move the constant a fourth time and push the problem back to the small-cluster end, this makes it a knob. Raising failureThreshold costs nothing on a fast startup, because a startup probe stops as soon as it first succeeds.

Special notes

  • v2/config/manager/manager.yaml gets the same three fields on each probe so controller:validate-helm keeps matching the chart against the kustomize output.
  • The only difference in the rendered output with default values is that the Kubernetes defaults for the liveness and readiness probes (periodSeconds: 10, failureThreshold: 3, timeoutSeconds: 1) are now written out explicitly instead of being implied. Behaviour is unchanged.
  • I deliberately left successThreshold out. Kubernetes requires it to be 1 for liveness and startup probes, so exposing it would mostly be a way to write an invalid config.
  • There is a small textual overlap with feat: make the leader election lease timings configurable #5707 in diagnosing-problems/_index.md; both add a section under Common mistakes. Whichever lands second is a trivial rebase.

How does this PR make you feel?

gif

Checklist

  • this PR contains documentation

The probes have been retuned in place three times, each time to suit one
cluster:

- Azure#2254 added the liveness probe.
- Azure#2844 raised initialDelaySeconds because the operator "was running into
  timeouts on my arm64 mac machine ... so operator gets more time to load
  and apply CRDs".
- Azure#4791 found that delay now penalised fast startups - "ASO finish
  starting up after about 10s, then idle for an extra ~45s" - and replaced
  it with a startup probe at 10s x 12.

That budget is now too tight at the other end of the range. The operator
applies its CRDs and starts a controller per installed CRD before it
serves /healthz, so on a cluster with a large number of CRDs the kubelet
kills it mid-apply and it never finishes starting.

Rather than move the constant a fourth time, this exposes the timings as
Helm values. The defaults are what the chart applies today, so the only
change to the rendered output is that the Kubernetes defaults for the
liveness and readiness probes are now written out explicitly.

The probe paths and ports stay fixed; they have never been the problem.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Note

This error may be related to your runner configuration. You can now configure runners for Copilot code review separately from Copilot cloud agent by creating a copilot-code-review.yml file with your setup steps. Read the docs for details.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, small doc nit.

Comment thread v2/charts/azure-service-operator/values.yaml Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good; will be happy to merge once the docs are tweaked.

Applies the review suggestion on the probes block: drops the sentence about
the defaults matching what the chart already applied, which belongs in the PR
description rather than the values file.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@andreidorin-oprea

Copy link
Copy Markdown
Contributor Author

Bevan Arps (@theunrepentantgeek) docs tweaked in ea7e840 - Matthew Christopher (@matthchr)'s suggestion applied verbatim, comment-only change. Ready for merge.

Merged via the queue into Azure:main with commit 9b59c9c Sep 21, 2026
9 checks passed
@github-project-automation github-project-automation Bot moved this from In Progress to Recently Completed in Azure Service Operator Roadmap Sep 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Recently Completed

Development

Successfully merging this pull request may close these issues.

4 participants