Repository navigation
Add Observability Pipelines Worker CRD and named BYOC pipelines - #3531
Conversation
shuheiktgw
changed the title
This comment has been minimized.
This comment has been minimized.
58461bb to
ecd7770
Compare
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ecd7770fe7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Identity configures the Kubernetes identity used by the BYOC workloads. | ||
| // Identity configures an existing ServiceAccount used by the BYOC workloads, excluding the pipelines. | ||
| // When ServiceAccountName is omitted, the controller creates and owns a ServiceAccount named after the cluster. | ||
| // +optional |
There was a problem hiding this comment.
I've slightly changed the interface so users can choose whether they want the operator to manage the SA or manage it themselves.
| // +kubebuilder:validation:MinItems=1 | ||
| // +listType=map | ||
| // +listMapKey=name | ||
| Pipelines []DatadogBYOCClusterPipelineSpec `json:"pipelines,omitempty"` |
There was a problem hiding this comment.
It turns out a single BYOC cluster can have multiple OP workers, so I've extended the interface to support that.
1dce5f8 to
cf5eb82
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cf5eb8275a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // +kubebuilder:validation:MinItems=1 | ||
| // +listType=map | ||
| // +listMapKey=name | ||
| Pipelines []DatadogBYOCClusterPipelineSpec `json:"pipelines,omitempty"` |
There was a problem hiding this comment.
Since each entry will materialize a DatadogObservabilityPipelinesWorker, could we document the intended ownership contract for BYOC-created Workers?
My expectation would be:
- the BYOC cluster owns and reconciles the complete spec of generated Workers;
- direct edits to those Workers are unsupported and reverted;
- standalone Workers without a BYOC owner remain user-managed;
- a same-name Worker not owned by this cluster is reported as a conflict rather than adopted or overwritten.
I don’t think this necessarily requires an API change for the first alpha, but making the rule explicit before implementing reconciliation would prevent the parent and child from becoming competing sources of truth.
There was a problem hiding this comment.
Documented the ownership contract on the Pipelines field in 7118354.
| // Name identifies the pipeline within the cluster and is used in the worker resource name. | ||
| // Changing Name creates a new worker and deletes the previous one. | ||
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:MaxLength=63 |
There was a problem hiding this comment.
nit - if this name is going to be used in other k8s resource names probably could be lower maxLength.
| // Identity configures an existing ServiceAccount used by the worker. | ||
| // When ServiceAccountName is omitted, the controller creates and owns a dedicated worker ServiceAccount. | ||
| // +optional | ||
| Identity *DatadogBYOCClusterIdentitySpec `json:"identity,omitempty"` |
There was a problem hiding this comment.
nit - not sure if it's intentional but types declared in two CRD files reference each other. autoscaling solves this by using common package, probably moving some of the types form cluster to pipelines is enough in this case.
There was a problem hiding this comment.
Let me handle that as a follow-up! 🙏
3 participants
What does this PR do?
Adds the namespaced
DatadogObservabilityPipelinesWorkerv1alpha1 CRD.The diff is large, but most of it is autogenerated. The changes worth reviewing are:
api/datadoghq/v1alpha1/datadogbyoccluster_types.goapi/datadoghq/v1alpha1/datadogobservabilitypipelinesworker_types.goMotivation
Defines the Worker API alongside the named BYOC pipelines that will create OP Workers. This completes the API refinements following #3501 and establishes the API needed by the upcoming BYOC and Worker implementation PRs.
Checklist
enhancementqa/skip-qalabel