CRDs: wait for CRDs instead of exiting - #169
Conversation
lukebond
left a comment
There was a problem hiding this comment.
thanks for this! you've got the logic right but for a couple of rusty things.
however something else i would push back on a little... we're doing this PR because we want it to more gracefully handle the fact that the CRDs aren't there yet, it needn't go into crashloop backoff and have people waiting ages for it. but nor do we want it silently unready, forever, if it's a scenario that gets an infinite retry but doesn't surface it loudly somewhere. there is the log message but i think we can do better.
one solution is to add a new ready endpoint that reflects that all controllers have passed wait_for_crds and then moving the health endpoint to a liveness probe, but that's got too much blast radius for an open source project and for a relatively trivial fixing PR that you're trying to do. and it requires a chart change. but i do want to avoid the silent unreadiness reporting as ready somehow, in a proportional way for this PR.
i would offer two suggestions:
- a metric that we can alert on to detect a controller that's waiting forever for CRDs that aren't coming
- emit a k8s event, attached tot he opererator's pod or deployment, e.g. with message
reason: WaitingForCRD, so the API can aggregate repeated occurrences of it. it also needs RBAC 🫠 - i'm sure you know all this though!
we could do both, but i think we should at least do the k8s event. up to you if you want to do both in this PR or file the metric as a follow-up.
25daf20 to
931c18c
Compare
Adjusted the controller startup logic to use a discovery client to wait for CRDs instead of just existing. The new behavior is more consistent with the other Kubernetes Operators. Additionally, health checks and events are emitted to indicate controllers are not ready. Resolves #166
931c18c to
63c7a06
Compare
|
@lukebond fixed up, let me know what you think. |
lukebond
left a comment
There was a problem hiding this comment.
this is great. you went and did the whole health & readiness thing! that's great :)
however it might be a surprise to some users, especially those who get a chart bump but not a container image bump, it would wait forever for the non-existent ready endpoint to pass. this is not a problem, just a problem if it's a surprise.
so i suggest we do a major version bump of the helm chart and the operator version itself. not everyone reads release notes but a major version bump gives users the best chance of doing so.
so please go ahead and do that, it's good education in the process anyway (none of which will be surprising to you i'm sure). you've already got the release note ready to go. i think it'll be a good time with the recent CRD packaging changes.
Adjusted the controller startup logic to use a discovery client to wait for CRDs instead of just existing. The new behavior is more consistent with the other Kubernetes Operators.
Resolves #166