Skip to content

Commit cf3a3ed

Browse files
pjanottiCopilotcrobert-1
authored
[smartagent/docker-container-stats] Use client API negotiation instead of fixed version (#6941)
* Add test before the fix * Use sudo when setting the dockerd config file * Use system V CLI commands * run a container so there are metrics to be collected * Keep container running * Fix run/rm docker container location * do not error on dockerd restart * change function to get service state * Remove unused import * Missed update * Fix mv command * Upgrade to latest dockerd * Re-org CI workflow * Remove duplicated code * Negotiate client API version * Re-add container to ensure metrics are generated * Remove test w/ 1.44 * Expect test failure for default * Reinstate the fix * Add changelog * Update internal/signalfx-agent/pkg/monitors/docker/docker_test.go Co-authored-by: Copilot <[email protected]> * Update internal/signalfx-agent/pkg/monitors/docker/testdata/upgrade-dockerd-on-ubuntu.sh Co-authored-by: Copilot <[email protected]> * Update internal/signalfx-agent/pkg/monitors/docker/testdata/upgrade-dockerd-on-ubuntu.sh Co-authored-by: Copilot <[email protected]> * Update internal/signalfx-agent/pkg/monitors/docker/docker_test.go Co-authored-by: Copilot <[email protected]> * Run go test with `-v` Co-authored-by: Curtis Robert <[email protected]> * Code review feedback * Copilot suggestion --------- Co-authored-by: Copilot <[email protected]> Co-authored-by: Curtis Robert <[email protected]>
1 parent b7774f0 commit cf3a3ed

File tree

5 files changed

+297
-3
lines changed

5 files changed

+297
-3
lines changed
Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
2+
change_type: bug_fix
3+
4+
# The name of the component, or a single word describing the area of concern, (e.g. crosslink)
5+
component: smartagent/docker-container-stats
6+
7+
8+
# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
9+
note: Use client API version negotiation instead of fixed version
10+
11+
# One or more tracking issues related to the change
12+
issues: [6941]
13+
14+
# (Optional) One or more lines of additional information to render under the primary note.
15+
# These lines will be padded with 2 spaces and then inserted directly into the document.
16+
# Use pipe (|) for multiline entries.
17+
subtext: |
18+
The `smartagent/docker-container-stats` monitor was using a fixed API version of `v1.24`,
19+
which is not compatible with version `v29` of the Docker engine. This change updates the
20+
monitor to use API version negotiation, allowing it to work with a wider range of Docker
21+
engine versions.

.github/workflows/build-and-test.yml

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -128,3 +128,15 @@ jobs:
128128
uses: ./.github/workflows/reusable-compile.yml
129129
with:
130130
SYS_BINARY: ${{ matrix.SYS_BINARIES }}
131+
132+
latest-dockerd-test:
133+
# Run this as a separate test to avoid affecting other tests with the dockerd upgrade.
134+
runs-on: ubuntu-24.04
135+
steps:
136+
- uses: actions/checkout@v5
137+
- uses: ./.github/actions/setup-environment
138+
139+
- name: dockerd test
140+
working-directory: ./internal/signalfx-agent/pkg/monitors/docker
141+
run: |
142+
go test -v -tags=dockerd -timeout 3m ./...

internal/signalfx-agent/pkg/monitors/docker/docker.go

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -25,8 +25,6 @@ import (
2525
"github.com/signalfx/signalfx-agent/pkg/utils/timeutil"
2626
)
2727

28-
const dockerAPIVersion = "v1.24"
29-
3028
func init() {
3129
monitors.Register(&monitorMetadata, func() interface{} { return &Monitor{} }, &Config{})
3230
}
@@ -97,7 +95,7 @@ func (m *Monitor) Configure(conf *Config) error {
9795
defaultHeaders := map[string]string{"User-Agent": "signalfx-agent"}
9896

9997
var err error
100-
m.client, err = docker.NewClientWithOpts(docker.WithHost(conf.DockerURL), docker.WithVersion(dockerAPIVersion), docker.WithHTTPHeaders(defaultHeaders))
98+
m.client, err = docker.NewClientWithOpts(docker.WithHost(conf.DockerURL), docker.WithAPIVersionNegotiation(), docker.WithHTTPHeaders(defaultHeaders))
10199
if err != nil {
102100
return fmt.Errorf("could not create docker client: %w", err)
103101
}
Lines changed: 237 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,237 @@
1+
//go:build dockerd
2+
3+
package docker
4+
5+
import (
6+
"encoding/json"
7+
"os"
8+
"os/exec"
9+
"path/filepath"
10+
"runtime"
11+
"strings"
12+
"sync"
13+
"testing"
14+
"time"
15+
16+
"github.com/signalfx/defaults"
17+
"github.com/signalfx/golib/v3/datapoint" //nolint:staticcheck // SA1019: deprecated package still in use
18+
"github.com/signalfx/golib/v3/event" //nolint:staticcheck // SA1019: deprecated package still in use
19+
"github.com/signalfx/golib/v3/trace" //nolint:staticcheck // SA1019: deprecated package still in use
20+
"github.com/stretchr/testify/require"
21+
"go.opentelemetry.io/collector/pdata/pmetric"
22+
23+
"github.com/signalfx/signalfx-agent/pkg/core/config"
24+
"github.com/signalfx/signalfx-agent/pkg/core/dpfilters"
25+
"github.com/signalfx/signalfx-agent/pkg/monitors/types"
26+
)
27+
28+
const (
29+
useDockerEngineDefault = "default"
30+
)
31+
32+
func TestMinimumRequiredClientVersion(t *testing.T) {
33+
// Skip this test if not running on Linux GitHub runner
34+
if runtime.GOOS != "linux" {
35+
t.Skip("Skipping test on non-Linux OS")
36+
}
37+
if os.Getenv("GITHUB_ACTIONS") != "true" {
38+
t.Skip("Skipping test outside of GitHub Actions")
39+
}
40+
41+
// Execute the dockerd upgrade script and fail the test if it fails.
42+
// This test is not rolling back the dockerd upgrade, so it can affect all subsequent tests.
43+
scriptPath := filepath.Join("testdata", "upgrade-dockerd-on-ubuntu.sh")
44+
scriptCmd := exec.Command("bash", scriptPath)
45+
scriptOut, err := scriptCmd.CombinedOutput()
46+
t.Logf("upgrade-dockerd-on-ubuntu.sh output:\n%s\n", string(scriptOut))
47+
require.NoError(t, err, "upgrade-dockerd-on-ubuntu.sh failed with exit code %d", scriptCmd.ProcessState.ExitCode())
48+
49+
tt := []struct {
50+
minimumRequiredClientVersion string
51+
}{
52+
{
53+
minimumRequiredClientVersion: useDockerEngineDefault,
54+
},
55+
{
56+
minimumRequiredClientVersion: "1.24",
57+
},
58+
}
59+
60+
for _, tc := range tt {
61+
t.Run(tc.minimumRequiredClientVersion, func(t *testing.T) {
62+
if tc.minimumRequiredClientVersion != useDockerEngineDefault {
63+
updateGHLinuxRunnerDockerDaemonMinClientVersion(t, tc.minimumRequiredClientVersion)
64+
}
65+
66+
cleanupContainer := runDockerContainerToGenerateMetrics(t)
67+
// This needs to be in a defer so the container is removed before the docker daemon settings are reset.
68+
defer cleanupContainer()
69+
70+
output := &fakeOutput{}
71+
monitor := &Monitor{
72+
Output: output,
73+
}
74+
config := &Config{
75+
MonitorConfig: config.MonitorConfig{
76+
IntervalSeconds: 1,
77+
},
78+
}
79+
defaults.Set(config)
80+
81+
err := monitor.Configure(config)
82+
require.NoError(t, err, "Expected no error during monitor configuration")
83+
defer monitor.Shutdown()
84+
85+
require.Eventually(t, func() bool {
86+
return output.HasDatapoints()
87+
}, 10*time.Second, 100*time.Millisecond, "Expected datapoints to be collected")
88+
})
89+
}
90+
}
91+
92+
func updateGHLinuxRunnerDockerDaemonMinClientVersion(t *testing.T, minimumRequiredClientVersion string) {
93+
// Fail if there is already a daemon.json file
94+
if _, err := os.Stat("/etc/docker/daemon.json"); err == nil {
95+
t.Fatal("daemon.json already exists, cannot update minimum required client version")
96+
}
97+
98+
daemonConfig := map[string]string{
99+
"min-api-version": minimumRequiredClientVersion,
100+
}
101+
102+
configJSON, err := json.MarshalIndent(daemonConfig, "", " ")
103+
require.NoError(t, err, "Failed to marshal daemon config")
104+
t.Logf("Docker daemon config JSON:\n%s", string(configJSON))
105+
106+
// Create a temporary daemon.json file with the new configuration then
107+
// move it using sudo to the correct location.
108+
tempFileName := filepath.Join(t.TempDir(), "daemon.json")
109+
err = os.WriteFile(tempFileName, configJSON, 0o644)
110+
require.NoError(t, err, "Failed to write daemon.json")
111+
112+
cmd := exec.Command("sudo", "mv", tempFileName, "/etc/docker/")
113+
err = cmd.Run()
114+
require.NoError(t, err, "Failed to move daemon.json")
115+
116+
cmd = exec.Command("sudo", "service", "docker", "restart")
117+
// Ignore error since the docker daemon might automatically restart after
118+
// adding the config file
119+
err = cmd.Run()
120+
if err != nil {
121+
t.Logf("Docker daemon restart error: %s", err)
122+
}
123+
124+
t.Cleanup(func() {
125+
cmd := exec.Command("sudo", "rm", "/etc/docker/daemon.json")
126+
err := cmd.Run()
127+
require.NoError(t, err, "Failed to remove daemon.json")
128+
129+
cmd = exec.Command("sudo", "service", "docker", "restart")
130+
// Ignore error since the docker daemon might automatically restart after
131+
// removing the config file
132+
err = cmd.Run()
133+
if err != nil {
134+
t.Logf("Docker daemon restart error: %s", err)
135+
}
136+
137+
requireDockerDaemonRunning(t)
138+
})
139+
140+
requireDockerDaemonRunning(t)
141+
}
142+
143+
func runDockerContainerToGenerateMetrics(t *testing.T) func() {
144+
cmd := exec.Command("docker", "run", "-d", "--name", "docker-client-test", "alpine", "sleep", "180")
145+
err := cmd.Run()
146+
require.NoError(t, err, "Failed to run docker container")
147+
return func() {
148+
cmd := exec.Command("docker", "rm", "-f", "docker-client-test")
149+
err := cmd.Run()
150+
require.NoError(t, err, "Failed to remove docker container")
151+
}
152+
}
153+
154+
func requireDockerDaemonRunning(t *testing.T) {
155+
require.Eventually(t, func() bool {
156+
isRunning, err := isServiceRunning(t, "docker")
157+
require.NoError(t, err, "Failed to get docker service status")
158+
return isRunning
159+
}, 30*time.Second, 1*time.Second)
160+
}
161+
162+
func isServiceRunning(t *testing.T, serviceName string) (bool, error) {
163+
cmd := exec.Command("service", serviceName, "status")
164+
output, err := cmd.CombinedOutput()
165+
if err != nil {
166+
// The 'service status' command often returns a non-zero exit code if the service is not running
167+
// or if there's an error. We still need to parse the output to determine the status.
168+
t.Logf("Error getting %q service status: %v", serviceName, err)
169+
}
170+
171+
outputStr := string(output)
172+
173+
if strings.Contains(outputStr, "active (running)") {
174+
return true, nil
175+
}
176+
177+
return false, nil
178+
}
179+
180+
type fakeOutput struct {
181+
datapoints []*datapoint.Datapoint
182+
mu sync.Mutex
183+
}
184+
185+
var _ types.FilteringOutput = (*fakeOutput)(nil)
186+
187+
func (fo *fakeOutput) AddDatapointExclusionFilter(_ dpfilters.DatapointFilter) {
188+
panic("unimplemented")
189+
}
190+
191+
func (fo *fakeOutput) AddExtraDimension(_ string, _ string) {
192+
panic("unimplemented")
193+
}
194+
195+
func (fo *fakeOutput) Copy() types.Output {
196+
panic("unimplemented")
197+
}
198+
199+
func (fo *fakeOutput) EnabledMetrics() []string {
200+
return []string{}
201+
}
202+
203+
func (fo *fakeOutput) HasAnyExtraMetrics() bool {
204+
panic("unimplemented")
205+
}
206+
207+
func (fo *fakeOutput) HasEnabledMetricInGroup(_ string) bool {
208+
panic("unimplemented")
209+
}
210+
211+
func (fo *fakeOutput) SendDimensionUpdate(_ *types.Dimension) {
212+
panic("unimplemented")
213+
}
214+
215+
func (fo *fakeOutput) SendEvent(_ *event.Event) {
216+
panic("unimplemented")
217+
}
218+
219+
func (fo *fakeOutput) SendMetrics(_ ...pmetric.Metric) {
220+
panic("unimplemented")
221+
}
222+
223+
func (fo *fakeOutput) SendSpans(_ ...*trace.Span) {
224+
panic("unimplemented")
225+
}
226+
227+
func (fo *fakeOutput) SendDatapoints(dps ...*datapoint.Datapoint) {
228+
fo.mu.Lock()
229+
defer fo.mu.Unlock()
230+
fo.datapoints = append(fo.datapoints, dps...)
231+
}
232+
233+
func (fo *fakeOutput) HasDatapoints() bool {
234+
fo.mu.Lock()
235+
defer fo.mu.Unlock()
236+
return len(fo.datapoints) > 0
237+
}
Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
#!/bin/bash
2+
3+
# This script is used to upgrade the Docker daemon on Ubuntu to the latest version.
4+
# As recommended by https://docs.docker.com/engine/install/ubuntu/#install-using-the-repository
5+
6+
set -e
7+
8+
# Add Docker's official GPG key:
9+
sudo apt update
10+
sudo apt install -y ca-certificates curl
11+
sudo install -m 0755 -d /etc/apt/keyrings
12+
sudo curl -fsSL https://download.docker.com/linux/ubuntu/gpg -o /etc/apt/keyrings/docker.asc
13+
sudo chmod a+r /etc/apt/keyrings/docker.asc
14+
15+
# Add the repository to Apt sources:
16+
sudo tee /etc/apt/sources.list.d/docker.sources <<EOF
17+
Types: deb
18+
URIs: https://download.docker.com/linux/ubuntu
19+
Suites: $(. /etc/os-release && echo "${UBUNTU_CODENAME:-$VERSION_CODENAME}")
20+
Components: stable
21+
Signed-By: /etc/apt/keyrings/docker.asc
22+
EOF
23+
24+
sudo apt update
25+
sudo apt-get install -y docker-ce --only-upgrade
26+
sudo dockerd --version

0 commit comments

Comments
 (0)