-
Notifications
You must be signed in to change notification settings - Fork 173
[smartagent/docker-container-stats] Use client API negotiation instead of fixed version #6941
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6941 +/- ##
=======================================
Coverage 37.93% 37.94%
=======================================
Files 367 367
Lines 25743 25743
=======================================
+ Hits 9765 9767 +2
+ Misses 15165 15163 -2
Partials 813 813 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes a compatibility issue where the smartagent/docker-container-stats monitor fails with Docker Engine v29 due to using a fixed API version (v1.24) that is below the minimum supported version (v1.44). The fix replaces the fixed version with client API version negotiation to support a wider range of Docker engine versions.
- Replaces
docker.WithVersion("v1.24")withdocker.WithAPIVersionNegotiation()in the Docker client initialization - Adds comprehensive integration test to verify compatibility with both default and custom Docker daemon configurations
- Includes shell script for upgrading Docker daemon to latest version in test environments
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
internal/signalfx-agent/pkg/monitors/docker/docker.go |
Replaces fixed API version with API version negotiation |
internal/signalfx-agent/pkg/monitors/docker/docker_test.go |
Adds integration test for minimum client version compatibility |
internal/signalfx-agent/pkg/monitors/docker/testdata/upgrade-dockerd-on-ubuntu.sh |
Provides script to upgrade Docker daemon on Ubuntu test runners |
.github/workflows/build-and-test.yml |
Adds isolated job for Docker daemon upgrade testing |
.chloggen/fix-docker-monitor-min-client-ver.yaml |
Documents the bug fix in changelog |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
internal/signalfx-agent/pkg/monitors/docker/testdata/upgrade-dockerd-on-ubuntu.sh
Show resolved
Hide resolved
internal/signalfx-agent/pkg/monitors/docker/testdata/upgrade-dockerd-on-ubuntu.sh
Outdated
Show resolved
Hide resolved
internal/signalfx-agent/pkg/monitors/docker/testdata/upgrade-dockerd-on-ubuntu.sh
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <[email protected]>
…ockerd-on-ubuntu.sh Co-authored-by: Copilot <[email protected]>
…ockerd-on-ubuntu.sh Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
internal/signalfx-agent/pkg/monitors/docker/testdata/upgrade-dockerd-on-ubuntu.sh
Show resolved
Hide resolved
Co-authored-by: Curtis Robert <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
With the release of Docker Engine v29 the fixed version of the monitor is not accepted by default, producing the error message:
This change adds a dedicated test to verify this scenario ensuring that with the fix it works OOTB for the default configuration of the Docker Engine v29 and keeps working also for the ones that adopted the temporary workaround described here.
Example of the new test failure when the fix is not in place: https://github.com/signalfx/splunk-otel-collector/actions/runs/19444329447/job/55635128801?pr=6941#step:4:168