[CONTP-1503] Add standalone DatadogCSIDriver controller#2856
[CONTP-1503] Add standalone DatadogCSIDriver controller#2856gh-worker-dd-mergequeue-cf854d[bot] merged 14 commits intomainfrom
Conversation
Define the DatadogCSIDriver, DatadogCSIDriverSpec, DatadogCSIDriverOverride, and DatadogCSIDriverStatus types in api/datadoghq/v1alpha1. The CRD enables declarative management of the Datadog CSI Driver via the operator, replacing the standalone Helm chart deployment. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Controller-runtime wiring for the DatadogCSIDriver reconciler. Watches the primary CR with GenerationChangedPredicate, owned DaemonSets for all changes (including status), and CSIDriver objects via label-based enqueue for drift detection on the cluster-scoped resource. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Implements the reconciliation logic for the DatadogCSIDriver controller: - Deferred SSA status patch with ObservedGeneration tracking - CSIDriver object management with Get+DeepEqual+Update for full drift reversion - DaemonSet management with the same pattern, including label enforcement - Override system with merge-by-name semantics (env vars, volumes, mounts) - Image resolution via pkg/images (supports tag-only overrides) - Finalizer-based cleanup of the cluster-scoped CSIDriver on deletion - Comprehensive unit tests covering creation, updates, drift, deletion, overrides Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Register the controller in setup.go with a feature flag and add the -datadogCSIDriverEnabled flag (default: false) to cmd/main.go. Also registers the storagev1 scheme required for CSIDriver object management. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
| - bases/v1/datadoghq.com_datadogdashboards.yaml | ||
| - bases/v1/datadoghq.com_datadoggenericresources.yaml | ||
| - bases/v1/datadoghq.com_datadogagentinternals.yaml | ||
| - bases/v1/datadoghq.com_datadogcsidrivers.yaml |
There was a problem hiding this comment.
only change is adding csidriver CRD, otherwise, it's simply yaml formatting
![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: fae3f3211d
ℹ️ 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".
Codecov Report❌ Patch coverage is ❌ Your patch status has failed because the patch coverage (65.71%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #2856 +/- ##
==========================================
+ Coverage 39.57% 40.05% +0.48%
==========================================
Files 315 319 +4
Lines 27508 28031 +523
==========================================
+ Hits 10885 11229 +344
- Misses 15826 15979 +153
- Partials 797 823 +26
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
| type DatadogCSIDriverSpec struct { | ||
| // CSIDriverImage is the image configuration for the main CSI node driver container. | ||
| // Default image: gcr.io/datadoghq/csi-driver:1.2.1 | ||
| // +optional | ||
| CSIDriverImage *v2alpha1.AgentImageConfig `json:"csiDriverImage,omitempty"` | ||
|
|
||
| // RegistrarImage is the image configuration for the CSI node driver registrar sidecar. | ||
| // Default image: registry.k8s.io/sig-storage/csi-node-driver-registrar:v2.0.1 | ||
| // +optional | ||
| RegistrarImage *v2alpha1.AgentImageConfig `json:"registrarImage,omitempty"` | ||
|
|
||
| // APMSocketPath is the host path to the APM socket. | ||
| // Default: /var/run/datadog/apm.socket | ||
| // +optional | ||
| APMSocketPath *string `json:"apmSocketPath,omitempty"` | ||
|
|
||
| // DSDSocketPath is the host path to the DogStatsD socket. | ||
| // Default: /var/run/datadog/dsd.socket | ||
| // +optional | ||
| DSDSocketPath *string `json:"dsdSocketPath,omitempty"` | ||
|
|
||
| // Override allows customization of the CSI driver DaemonSet pod template. | ||
| // +optional | ||
| Override *DatadogCSIDriverOverride `json:"override,omitempty"` | ||
| } |
There was a problem hiding this comment.
There was a problem hiding this comment.
Nope, by "design", I did not include it in the CRD to have it default true, and users have the possibility of disabling it with an override should they desire: do you think it should be present in the CRD, do we expect users (tho we mostly expect people to rely on automatic creation from DDA ?) to play with it a lot ? Do we expect more fields in the APM section in the future / does it belong within a struct with other options we expect to introduce ?
Here's how it can be disabled at the moment (override takes precedence on any default/already set variable):
override:
containers:
csi-node-driver:
env:
- name: DD_APM_ENABLED
value: "false"
| } | ||
| } | ||
|
|
||
| func applyContainerOverrides(container *corev1.Container, override *v2alpha1.DatadogAgentGenericContainer) { |
There was a problem hiding this comment.
QQ Not specifically related to CSI:
The container overrides applied here seem to be applicable to any container override in any other controller.
Is there a reason why this utility function lives here and not in some shared package to avoid reimplementing the same logic for all other controllers ?
There was a problem hiding this comment.
Yes and no: it's applicable to a controller using v2alpha1.DatadogAgentGenericContainer type with a fixed strategy: override always wins. In DatadogAgent controller, this is way more complex, as there's a callback to a strategy to have a merge be dependent on said strategy, and the signatures are different
I tried to extract outside of DatadogAgent to use in both paths but it simply adds indirection making it less readable. They could definitely be extracted if we start using more controllers with the same simple logic that is present in CSIDriver, re-using the same type. So letting as is, with comments: 9633847
adel121
left a comment
adel121
left a comment
There was a problem hiding this comment.
Left some comments / questions.
Also a general question, I don't see where the CSI Driver Custom Resource is being created by the Datadog Agent controller in case CSI feature is enabled. Or is this deferred to a follow-up PR?
Follow-up PR #2857 as shared offline, to discuss further |
khewonc
left a comment
khewonc
left a comment
There was a problem hiding this comment.
A few small optional nits, but otherwise lgtm
gh-worker-dd-mergequeue-cf854d
bot
merged commit 61e7c1b
into
main
gh-worker-dd-devflow-36fce6
bot
added
mergequeue-status: done
and removed
mergequeue-status: in_progress
labels
gh-worker-dd-mergequeue-cf854d
bot
deleted the
tbavelier/csi-driver-standalone-v2
branch
tbavelier
changed the title
4 participants
Summary
DatadogCSIDriverCRD (v1alpha1) for declarative management of the Datadog CSI Driver via the operator, replacing the standalone Helm chart deploymentCSIDriverobject (cluster-scoped, label-based ownership) and aDaemonSet(owner-ref based), with full drift reversion via Get+DeepEqual+Update-datadogCSIDriverEnabledflag (default:false)Commit walkthrough
DatadogCSIDriver,DatadogCSIDriverSpec,DatadogCSIDriverOverride,DatadogCSIDriverStatustypes + CRD YAMLGenerationChangedPredicateon the CR,Owns(DaemonSet), andWatches(CSIDriver)via label-based enqueuecmd/main.goandsetup.goTest plan
go test ./internal/controller/datadogcsidriver/...)-datadogCSIDriverEnabled=trueand verify CSIDriver + DaemonSet creationkubectl edita DaemonSet label/spec field → next reconcile reverts itspec.override🤖 Generated with Claude Code