Drop provider arg from Feature interface methods#3007
Conversation
levan-m
removed
team/agent-apm
team/remote-config
team/agent-security
team/container-experiences
team/kubernetes-experiences
team/profiling-full-host
team/container-autoscaling
team/asm-go
team/container-integrations
team/universal-service-monitoring
team/opentelemetry-agent
team/agent-data-plane
labels
![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: 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] |
There was a problem hiding this comment.
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 👍 / 👎.
There was a problem hiding this comment.
that's intended change, annotation will be user provided or detected and set by controller.
| // 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) |
There was a problem hiding this comment.
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 👍 / 👎.
There was a problem hiding this comment.
this was dead code after DDAI switch. Will have a separate PR for adding this back.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@brycekahle the check conditionally disables mounts on GKE COS nodes only, so those mounts are added on all non-COS nodes so I'm not sure removing those would be safe. Dead code was referring to the fact that we don't run provider detection since last several version so providerValue is never set to COS.
There was a problem hiding this comment.
The problem is adding the mount for COS nodes causes it to not start, so this would be a breaking change.
🛑 Gate Violations
|
8 participants
CONTP-1577
What does this PR do?
Drop
providerfrom 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?
Describe your test plan
Write there any instructions and details you may have to test your PR.
Checklist
bug,enhancement,refactoring,documentation,tooling, and/ordependenciesqa/skip-qalabel