Skip to content

Drop provider arg from Feature interface methods#3007

Open
levan-m wants to merge 2 commits into
mainfrom
levan-m/drop-feature-provider
Open

Drop provider arg from Feature interface methods#3007
levan-m wants to merge 2 commits into
mainfrom
levan-m/drop-feature-provider

Conversation

@levan-m
Copy link
Copy Markdown
Collaborator

@levan-m levan-m commented May 12, 2026

CONTP-1577

What does this PR do?

Drop provider from feature interface to switch to DDA/I metadata based lookup.

This "removes" COS support from two features which is effectively non-functional since not used in DDAI code path.

Motivation

What inspired you to submit this pull request?

Additional Notes

Anything else we should know when reviewing?

Minimum Agent Versions

Are there minimum versions of the Datadog Agent and/or Cluster Agent required?

  • Agent: vX.Y.Z
  • Cluster Agent: vX.Y.Z

Describe your test plan

Write there any instructions and details you may have to test your PR.

Checklist

  • PR has at least one valid label: bug, enhancement, refactoring, documentation, tooling, and/or dependencies
  • PR has a milestone or the qa/skip-qa label
  • All commits are signed (see: signing commits)

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4a7e6de580

ℹ️ 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".

// Configure is used to configure the feature from a v2alpha1.DatadogAgent instance.
func (f *controlPlaneMonitoringFeature) Configure(dda metav1.Object, ddaSpec *v2alpha1.DatadogAgentSpec, _ *v2alpha1.RemoteConfigConfiguration) (reqComp feature.RequiredComponents) {
f.owner = dda
f.provider = dda.GetAnnotations()[providercaps.ProviderAnnotationKey]
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Restore provider fallback when annotation is absent

After dropping the provider parameter, this feature now derives f.provider only from dda annotations. In this commit, there is no corresponding write path for providercaps.ProviderAnnotationKey, so f.provider is empty in normal reconciles and the OpenShift/EKS branches in ManageDependencies/ManageClusterAgent are skipped. That regresses control-plane monitoring setup on those providers unless users manually inject this internal annotation.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's intended change, annotation will be user provided or detected and set by controller.

Comment on lines 84 to +87
// src volume mount
_, providerValue := kubernetes.GetProviderLabelKeyValue(provider)
if providerValue != kubernetes.GKECosType {
srcVol, srcVolMount := volume.GetVolumes(common.SrcVolumeName, common.SrcVolumePath, common.SrcVolumePath, true)
managers.VolumeMount().AddVolumeMountToContainer(&srcVolMount, apicommon.SystemProbeContainerName)
managers.Volume().AddVolume(&srcVol)
}
srcVol, srcVolMount := volume.GetVolumes(common.SrcVolumeName, common.SrcVolumePath, common.SrcVolumePath, true)
managers.VolumeMount().AddVolumeMountToContainer(&srcVolMount, apicommon.SystemProbeContainerName)
managers.Volume().AddVolume(&srcVol)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve GKE-COS guard before adding src hostPath

This change unconditionally adds the /usr/src hostPath for OOM Kill, removing the previous gke-cos exclusion. That is a behavioral regression for GKE COS nodes, which previously got a different pod template specifically avoiding this mount. If provider handling is being migrated elsewhere, this feature still needs an equivalent guard to avoid changing system-probe volume behavior on COS clusters.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was dead code after DDAI switch. Will have a separate PR for adding this back.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this mount is only used for runtime compilation, which is rarely used these days. It is strange that it is only on the OOM/TCPQ checks. It could be used by many eBPF-based products in system-probe.

I think it might be safer to just remove the mount in this PR, and then add it back once you have the provider value to check against again.

@datadog-prod-us1-5

This comment has been minimized.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented May 12, 2026

Codecov Report

❌ Patch coverage is 42.80936% with 171 lines in your changes missing coverage. Please review.
✅ Project coverage is 40.76%. Comparing base (097216f) to head (ad3bec8).

Files with missing lines Patch % Lines
.../controller/datadogagent/feature/test/testsuite.go 0.00% 7 Missing ⚠️
...dogagent/feature/controlplanemonitoring/feature.go 45.45% 4 Missing and 2 partials ⚠️
...l/controller/datadogagent/feature/dummy/feature.go 0.00% 6 Missing ⚠️
...ller/datadogagent/feature/enabledefault/feature.go 0.00% 6 Missing ⚠️
...ntroller/datadogagent/feature/dataplane/feature.go 28.57% 5 Missing ⚠️
...ler/datadogagent/feature/flightrecorder/feature.go 28.57% 5 Missing ⚠️
...ller/datadogagent/feature/logcollection/feature.go 44.44% 5 Missing ⚠️
...al/controller/datadogagent/feature/sbom/feature.go 28.57% 5 Missing ⚠️
...atadogagent/feature/admissioncontroller/feature.go 33.33% 4 Missing ⚠️
.../controller/datadogagent/feature/appsec/feature.go 33.33% 4 Missing ⚠️
... and 37 more
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3007      +/-   ##
==========================================
- Coverage   40.77%   40.76%   -0.02%     
==========================================
  Files         332      332              
  Lines       28204    28199       -5     
==========================================
- Hits        11500    11495       -5     
  Misses      15929    15929              
  Partials      775      775              
Flag Coverage Δ
unittests 40.76% <42.80%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...r/datadogagent/feature/otelagentgateway/feature.go 90.97% <100.00%> (ø)
internal/controller/datadogagent/feature/types.go 22.10% <ø> (ø)
...ogagentinternal/controller_reconcile_v2_helpers.go 38.51% <100.00%> (ø)
pkg/kubernetes/provider.go 86.36% <ø> (ø)
...al/controller/datadogagent/component_reconciler.go 6.06% <0.00%> (ø)
...ller/datadogagent/feature/clusterchecks/feature.go 66.45% <88.88%> (ø)
...atadogagent/feature/kubernetesstatecore/feature.go 79.25% <83.33%> (ø)
...oller/datadogagentinternal/component_reconciler.go 0.00% <0.00%> (ø)
...nal/controller/datadogagent/feature/apm/feature.go 70.77% <77.77%> (ø)
...tadogagent/feature/orchestratorexplorer/feature.go 71.94% <66.66%> (ø)
... and 41 more

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 097216f...ad3bec8. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants