Skip to content

feat: Add startup probe to operator deployment - #654

Open
xeniape wants to merge 2 commits into
mainfrom
feat/add-startup-probe
Open

xeniape wants to merge 2 commits into
mainfrom
feat/add-startup-probe

Conversation

@xeniape

@xeniape xeniape commented Oct 1, 2026

Copy link
Copy Markdown
Member

Part of stackabletech/issues#828. Adds the startup probe checking the /ready endpoint for CRD install status. The /ready endpoint is already served by the operators, only one would still need a merge before rolling this change out:

@xeniape xeniape self-assigned this Oct 1, 2026
@xeniape xeniape moved this to Development: Waiting for Review in Stackable Engineering Oct 1, 2026
Techassi
Techassi previously approved these changes Oct 1, 2026
@Techassi

Techassi commented Oct 1, 2026

Copy link
Copy Markdown
Member

Ah one thing I'm super sure about: Are we 100% sure that we can unconditionally include the readiness probe? Because if I remember correctly, it is tied to the webhook, which in turn is tied to the CRD maintenance toggle.

This will be improved by stackabletech/issues#839, but that is not implemented yet.

scheme: HTTPS
periodSeconds: 2
failureThreshold: 30
timeoutSeconds: 3

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.

It felt weird to have timeout > period, so I asked AI:

timeoutSeconds (3) is larger than periodSeconds (2). The kubelet doesn't run probes in parallel, so nothing breaks, but probes that time out end up running back-to-back every ~3 s. Your startup budget then varies between 60 s (fast failures like connection refused) and about 90 s (timeouts), which makes the config harder to reason about. Keep timeout ≤ period. For example, periodSeconds: 3, timeoutSeconds: 3, failureThreshold: 20 gives a clean 60 s.

And I fully agree with it

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm fine with increasing periodSeconds to 3. The only advantage of that configuration, I guess, was that it would become ready faster if CRDs were there while also giving some more time to the kubelet in case of timeouts/something hangs.

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.

Works for me. Having both at 2s also works for me, I let you decide

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Comment thread template/deploy/helm/[[operator]]/templates/deployment.yaml.j2
@sbernauer

sbernauer commented Oct 5, 2026 •

Copy link
Copy Markdown
Member

Please note that whatever review feedback get's picked we need to roll out to PRs that don't use templating (such as stackabletech/secret-operator#759 or stackabletech/listener-operator#434)

@xeniape

xeniape commented Oct 5, 2026

Copy link
Copy Markdown
Member Author

Ah one thing I'm super sure about: Are we 100% sure that we can unconditionally include the readiness probe? Because if I remember correctly, it is tied to the webhook, which in turn is tied to the CRD maintenance toggle.

This will be improved by stackabletech/issues#839, but that is not implemented yet.

The webhook is currently always started regardless of the CRD maintenance toggle, example: https://github.com/stackabletech/airflow-operator/blob/16e3ef3df9257e6630e53d5ea5e1c7f0c7e55eeb/rust/operator-binary/src/main.rs#L129-L139 and https://github.com/stackabletech/airflow-operator/blob/16e3ef3df9257e6630e53d5ea5e1c7f0c7e55eeb/rust/operator-binary/src/main.rs#L238. The only thing the toggle is used for is deciding to skip cert rotation and CRD patching.

This is also a valid use case. If a user disabled the CRD maintenance, because they manage the CRDs themselves, it still makes sense to have the operator only be ready once the CRDs are there, otherwise it can't work properly anyway.

@sbernauer

Copy link
Copy Markdown
Member

it still makes sense to have the operator only be ready once the CRDs are there, otherwise it can't work properly anyway.

+1, I also explicitly tested that as part of my code review

@xeniape
xeniape requested a review from sbernauer October 5, 2026 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Development: In Review

Development

Successfully merging this pull request may close these issues.

3 participants