[TON-742] Support managed Agent installation and removal - #3268
[TON-742] Support managed Agent installation and removal#3268fanny-jiang wants to merge 20 commits into
Conversation
|
🎯 Code Coverage (details) 🔗 Commit SHA: fafb31f | Docs | Datadog PR Page | Give us feedback! |
fanny-jiang
changed the title
fanny-jiang
changed the title
fanny-jiang
changed the title
fanny-jiang
changed the title
|
@codex review |
![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: d7f0b3296d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b92399a8e7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
gpalmz
left a comment
gpalmz
left a comment
There was a problem hiding this comment.
Really solid, thoroughly tested PR. Approving. Left a few comments - a couple are MED questions worth a look (the credential-guard casing bypass, the promote-reported-as-failed coupling, and the partial-stuck edge), the rest are minor.
| if _, ok := global["credentials"]; ok { | ||
| return fmt.Errorf("config must not contain spec.global.credentials") | ||
| } | ||
| for _, field := range []string{"clusterAgentToken", "clusterAgentTokenSecret"} { |
There was a problem hiding this comment.
encoding/json matches struct fields case-insensitively, so a config with capitalized keys (e.g. {"Spec":{"Global":{"ClusterAgentToken":"..."}}}) slips past this case-sensitive map scan and still decodes into the typed struct. buildFleetDatadogAgentSpec overwrites Credentials but not ClusterAgentToken/ClusterAgentTokenSecret, so those would survive into the created DDA. Since it's already decoded into a typed struct, could we reject on the decoded fields instead (non-nil Global.Credentials/ClusterAgentToken/ClusterAgentTokenSecret)? That closes the casing gap and drops the duplicate parse.
| func (d *Daemon) finishPendingOperation(ctx context.Context, task pendingOperation, resultErr error) { | ||
| if resultErr == nil && task.intent == pendingIntentPromote { | ||
| if err := d.persistManagedAgentInstallationStableConfig(ctx, task.nsn, task.experimentID, task.resultVersion); err != nil { | ||
| resultErr = fmt.Errorf("persist promoted managed Agent installation config: %w", err) |
There was a problem hiding this comment.
If the reconciler has already promoted the experiment but persistManagedAgentInstallationStableConfig fails hard (not a conflict), this flips resultErr and reports the promote as failed to RC, even though the cluster is in the promoted phase - so Fleet may re-drive a successful operation. Is the re-drive idempotent here, or would it be safer to report the promote DONE and retry the config-persist separately (best-effort/async) rather than gating the task result on it? Same coupling on the daemon_operations.go fast path.
| return fmt.Errorf("hash live DatadogAgent spec: %w", err) | ||
| } | ||
| if dda.Annotations[fleetConfigHashAnnotation] != liveHash { | ||
| return &stateDoesntMatchError{msg: fmt.Sprintf("DatadogAgent %s/%s spec changed after Fleet config %q was accepted", dda.Namespace, dda.Name, configID)} |
There was a problem hiding this comment.
Small durability edge: if the process dies after Create but before recordFleetDatadogAgentSpecHash commits, the DDA is left partial with fleetConfigHashAnnotation = hash(preSpec) while the live spec is the post-admission spec. On retry, this annotation != liveHash check fails terminally ("spec changed after Fleet config was accepted") and the install never self-heals. This only bites if a DDA .spec is mutated on admission/defaulting - is that possible here? If so, could the replay path re-record the accepted hash (like the create path does) when the spec still passes DeepDerivative, rather than hard-failing?
| } | ||
| if fleetOwned != nil && unmanaged != nil { | ||
| return &stateDoesntMatchError{msg: fmt.Sprintf( | ||
| "Fleet-managed DatadogAgent %s/%s coexists with unmanaged DatadogAgent %s/%s", |
There was a problem hiding this comment.
ddas.Items is pre-filtered to the canonical target above, so the multiple-owned / active-experiment / owned+unmanaged-coexistence branches here can't fire (activeExperiment is also unused). Not a deletion risk, but it means the coexistence guard doesn't actually guard. Classify before the filter, or drop the dead branches?
| if d.managedAgentInstallationUpdates == nil { | ||
| return | ||
| } | ||
| time.AfterFunc(delay, d.requestManagedAgentInstallationRetry) |
There was a problem hiding this comment.
time.AfterFunc here isn't tracked or stopped on ctx.Done(), so after shutdown the timer still fires into requestManagedAgentInstallationRetry (whose consumer has exited). Harmless per-timer, but they can accumulate across leader churn - worth gating the callback on a context check or stopping the timers on Stop.
There was a problem hiding this comment.
The callback only performs a non-blocking send, and all delays are at most 8 seconds; manager shutdown terminates the process. I'd prefer to keep this scoped and defer timer tracking.
fanny-jiang
changed the title
| } | ||
|
|
||
| func (d *Daemon) installManagedAgentInstallationIntentForwarder(ctx context.Context) error { | ||
| informer, err := d.cache.GetInformer(ctx, &corev1.ConfigMap{}) |
There was a problem hiding this comment.
Managed install can be enabled while the cache watches a different namespace. The feature hard-codes managed resources to datadog-agent in pkg/fleet/managed_agent_installation.go:52-56, but cache namespaces still come from DD_AGENT_WATCH_NAMESPACE/WATCH_NAMESPACE in pkg/config/config.go:245-249. The intent/credential informers in pkg/fleet/managed_agent_installation_intent.go:87-119 will miss updates if that namespace is not watched
3 participants
What does this PR do?
Adds opt-in support for managed Datadog Agent installation and removal. The Operator reads installation requests from a ConfigMap, creates or removes the DatadogAgent and Windows AgentProfile resources, and reports progress and completion through Remote Config.
The new flow coordinates with existing Fleet configuration updates so the two operations do not overwrite each other's state.
Motivation
The EKS instrumenter needs a reliable way to install the Agent and confirm that uninstall has completed before removing the Datadog Operator add-on.
Additional Notes
The feature is disabled by default. EKS is the first supported provider, while the managed-installation flow remains provider-neutral and extensible. Existing DatadogAgent and DatadogAgentInternal reconciliation behavior is unchanged.
Minimum Agent Versions
Describe your test plan
CGO_ENABLED=0 make ci-testCGO_ENABLED=0 make lintChecklist
bug,enhancement,refactoring,documentation,tooling, and/ordependenciesqa/skip-qalabel