Conversation
|
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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Works for me. Having both at 2s also works for me, I let you decide
|
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) |
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. |
+1, I also explicitly tested that as part of my code review |
Part of stackabletech/issues#828. Adds the startup probe checking the
/readyendpoint for CRD install status. The/readyendpoint is already served by the operators, only one would still need a merge before rolling this change out:/readyendpoint to operator deployment commons-operator#461