discovery: enable by default on agents >= 7.78#2915
Conversation
![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: 4ceec7f0ad
ℹ️ 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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2915 +/- ##
==========================================
+ Coverage 41.38% 41.41% +0.02%
==========================================
Files 327 327
Lines 28952 28971 +19
==========================================
+ Hits 11982 11997 +15
- Misses 16109 16112 +3
- Partials 861 862 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 2 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
|
🎯 Code Coverage (details) 🔗 Commit SHA: f7e57b4 | Docs | Datadog PR Page | Give us feedback! |
Yumasi
force-pushed
the
guillaume.pagnoux/service-discovery-default-enable
branch
from
f8d22aa to
a6d4a92
Compare
Yumasi
force-pushed
the
guillaume.pagnoux/service-discovery-default-enable
branch
from
a6d4a92 to
3ff8d0b
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3ff8d0b87f
ℹ️ 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".
| if version := common.GetAgentVersionFromImage(*nodeAgent.Image); version != "" { | ||
| return version |
There was a problem hiding this comment.
When override.nodeAgent.image.name is set to a bare image name such as agent and tag is omitted, the pod override path preserves the inherited/default tag, but this branch gets latest from common.GetAgentVersionFromImage and never falls back to images.AgentLatestVersion. Since shouldEnableServiceDiscoveryByDefault treats unparseable versions as supported, clusters still running the current default Agent tag below 7.78 will unexpectedly get system-probe and DD_DISCOVERY_ENABLED just because they used the documented name-only image override; resolve the version from the actual assembled/overridden image or ignore the synthetic latest tag here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
We want service discovery defaulting to follow the effective node-agent image version, not only explicitly pinned tags. So if no version is provided, including partial image overrides, we intentionally inherit the default agent version. Unparseable provided tags are also treated as latest by design.
Added a comment + tests to ensure this is the intended behaviour.
Yumasi
force-pushed
the
guillaume.pagnoux/service-discovery-default-enable
branch
from
a45a54f to
ebe9bb9
Compare
tbavelier
left a comment
tbavelier
left a comment
There was a problem hiding this comment.
LGTM from the operator side provided the fallback to sp-lite works as expected
tbavelier
dismissed
their stale review
Removing to unblock, changes are LGTM from the operator side, but fallback is seemingly not working as expected on Agent side from system-probe to system-probe-lite
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f7e57b4281
ℹ️ 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".
| // Agent version must be >= 7.78.0 to enable service discovery by default. | ||
| if nodeAgent, ok := ddaSpec.Override[v2alpha1.NodeAgentComponentName]; ok { | ||
| if nodeAgent != nil && nodeAgent.Image != nil { | ||
| return pkgutils.IsAboveMinVersion(common.GetAgentVersionFromImage(*nodeAgent.Image), serviceDiscoveryAutoEnableMinVersion, ptr.To(true)) |
There was a problem hiding this comment.
When nodeAgent.image.name is a full image reference with a tag, the image rendering path ignores nodeAgent.image.tag, but this check calls GetAgentVersionFromImage, which gives Tag priority. For a valid spec such as name: gcr.io/datadoghq/agent:7.77.2 plus tag: 7.78.0, the pod still runs 7.77.2 while the new defaulting enables Service Discovery and DD_DISCOVERY_USE_SYSTEM_PROBE_LITE as if the Agent were supported; the inverse combination disables it on a supported rendered image. Use the same assembled/overridden image reference that will be put on the pod when deriving the version.
Useful? React with 👍 / 👎.
tbavelier
deleted the
guillaume.pagnoux/service-discovery-default-enable
branch
5 participants
What does this PR do?
This PR enables Service Discovery by default on agents >=7.78, which will use the new System-Probe-Lite binary for low-resource overhead. The version is determined based on the image tag, and an unparseable tag is considered to be the latest version.
The change makes the distinction between default enablement, and an explicit one. This allows users of an agent <7.78 to still enable discovery if they truly wants it, via system-probe (much higher overhead).
Motivation
Enable Service Discovery by default.
Additional Notes
This PR uses the fallback mechanism of System-Probe to decide to start System-Probe-Lite. This is done in order to avoid a drift between the operator & the agent in the handling of the fallback decision (i.e we would have to maintain a list of system-probe feature, as proposed #2610, which could be easily missed when adding new features. Missing it could cause wrong behaviors like not enabling system-probe when we actually should for another feature, for example).
Using system-probe to control the fallback decision, like in bare-Linux & Docker installations, avoid this sort of issues, but this means that we will see a spike in memory consumption due to system-probe's size itself (~70MiB). Once the fallback to System-Probe-Lite happens, the memory use is expected to fallback to ~3.4MiB (measured in dogfooding).
Minimum Agent Versions
Are there minimum versions of the Datadog Agent and/or Cluster Agent required?
Describe your test plan
Fallback behaviour + system-probe container creation tested with Kind and the following cases:
container status
Checklist
bug,enhancement,refactoring,documentation,tooling, and/ordependenciesqa/skip-qalabel