feat(operator): initial operator implementation (no deploy)#339
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
![sentry-warden[bot]](https://avatars.githubusercontent.com/in/2755838?s=60&v=4){kind=small}
| metadata: | ||
| name: streams-k8s-operator | ||
| rules: | ||
| - apiGroups: ["streams.sentry.io"] |
There was a problem hiding this comment.
This seems specific to Sentry. Is that okay even though streams itself is open source?
|
Please provide a description for the PR with context. |
|
Also please update this file https://github.com/getsentry/streams/tree/main/sentry_streams_k8s#readme |
fpacifici
left a comment
fpacifici
left a comment
There was a problem hiding this comment.
Several high level changes:
- Let's break down this PR. I would recommend Putting together the CRD and an empty operator here. So the key of this PR is the CRD and we can iterate on that.
- Break down the macro to extract the business logic so you can use it from the operator as well. See my comments on the dependencies
- Then we can start fill in the business logic in the operator
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit cbecfc3. Configure here.
cbecfc3 to
ed2e029
Compare
Add a Kopf-based operator that reconciles StreamingConsumer CRs into Deployments, using the shared consumer_builder logic.
ed2e029 to
97b94e0
Compare
fpacifici
left a comment
fpacifici
left a comment
There was a problem hiding this comment.
One last clarification.
What is the scenario where we would prune deployments ?
| _prune_stale_deployments( | ||
| namespace=namespace, | ||
| owner_uid=uid, | ||
| service_name=consumer["service_name"], | ||
| pipeline_name=consumer["pipeline_name"], | ||
| desired_names={m["metadata"]["name"] for m in manifests if m["kind"] == "Deployment"}, | ||
| ) |
There was a problem hiding this comment.
I don't think I understand this.
Could you give me an example about when we would remove a deployment here ?
There was a problem hiding this comment.
Changing with_canary from true to false would need this.
There was a problem hiding this comment.
Also not sure if we would ever do this but in the case where we delete a CR we would need it.
fpacifici
left a comment
fpacifici
left a comment
There was a problem hiding this comment.
I see the unit test are validating rendering.
Do we have a way to test the behavior of the operator ? I think you should at least unit test the "reconcile" function against a mock of the Kubernetes api before merging.
3 participants
Phase 1 of creating a k8s operator to the streams repository to manage Kafka consumer pod lifecycles. Creates the StreamingConsumer CRD and a basic operator that reconciles resources into a Deployment and ConfigMap. Splits shared logic from that and the PipelineStep macro into consumer builder methods.