fix(inputs.docker): incorporated review comments

Author: skartikey

plugins/inputs/docker/client.go

@@ -12,10 +12,6 @@ import (
 	"github.com/docker/docker/client"
 )
 
-// IsErrConnectionFailed returns true if the error is caused by connection failure.
-// This is a passthrough to the docker client library function.
-var IsErrConnectionFailed = client.IsErrConnectionFailed
-
 var (
 	defaultHeaders = map[string]string{"User-Agent": "engine-api-cli-1.0"}
 )

plugins/inputs/docker/docker.go

@@ -21,6 +21,7 @@ import (
 	"github.com/docker/docker/api/types/filters"
 	"github.com/docker/docker/api/types/swarm"
 	"github.com/docker/docker/api/types/system"
+	"github.com/docker/docker/client"
 
 	"github.com/influxdata/telegraf"
 	"github.com/influxdata/telegraf/config"
@@ -144,26 +145,22 @@ func (d *Docker) Init() error {
 }
 
 func (d *Docker) Start(telegraf.Accumulator) error {
-	// Get client
+	// Get client - this only creates the client object, doesn't connect
 	c, err := d.getNewClient()
 	if err != nil {
-		return &internal.StartupError{
-			Err:   fmt.Errorf("failed to create Docker client: %w", err),
-			Retry: IsErrConnectionFailed(err),
-		}
+		return err
 	}
 	d.client = c
 
-	// Use Ping to check connectivity, this is a lightweight check
+	// Use Ping to check connectivity - this is a lightweight check
 	ctx, cancel := context.WithTimeout(context.Background(), time.Duration(d.Timeout))
-	_, err = d.client.Ping(ctx)
-	cancel()
-	if err != nil {
+	defer cancel()
+	if _, err := d.client.Ping(ctx); err != nil {
 		d.client.Close()
 		d.client = nil
 		return &internal.StartupError{
 			Err:   fmt.Errorf("failed to ping Docker daemon: %w", err),
-			Retry: IsErrConnectionFailed(err),
+			Retry: client.IsErrConnectionFailed(err),
 		}
 	}
 
@@ -193,7 +190,7 @@ func (d *Docker) Start(telegraf.Accumulator) error {
 		d.client = nil
 		return &internal.StartupError{
 			Err:   fmt.Errorf("failed to get Docker info: %w", err),
-			Retry: IsErrConnectionFailed(err),
+			Retry: client.IsErrConnectionFailed(err),
 		}
 	}
 
@@ -221,10 +218,6 @@ func (d *Docker) Stop() {
 }
 
 func (d *Docker) Gather(acc telegraf.Accumulator) error {
-	if d.client == nil {
-		return errors.New("docker client not initialized")
-	}
-
 	// Create label filters if not already created
 	if !d.filtersCreated {
 		err := d.createLabelFilters()

plugins/inputs/docker/docker_test.go

@@ -19,8 +19,8 @@ import (
 
 	"github.com/influxdata/telegraf"
 	"github.com/influxdata/telegraf/config"
-	"github.com/influxdata/telegraf/internal"
 	"github.com/influxdata/telegraf/internal/choice"
+	"github.com/influxdata/telegraf/models"
 	"github.com/influxdata/telegraf/testutil"
 )
 
@@ -1793,38 +1793,41 @@ func TestPodmanStatsCache(t *testing.T) {
 	require.Contains(t, d.statsCache, testID)
 }
 
-func TestStartWithUnavailableDocker(t *testing.T) {
-	// Test that Start returns a retryable StartupError when Docker is unavailable
-	var acc testutil.Accumulator
-	d := Docker{
-		Log: testutil.Logger{},
+func TestStartupErrorBehaviorError(t *testing.T) {
+	// Test that model.Start returns error when Ping fails with default "error" behavior
+	// Uses the startup-error-behavior framework (TSD-006)
+	plugin := &Docker{
+		Timeout: config.Duration(100 * time.Millisecond),
 		newClient: func(string, *tls.Config) (dockerClient, error) {
-			return nil, errors.New("cannot connect to the Docker daemon")
+			return &mockClient{
+				PingF: func() (types.Ping, error) {
+					return types.Ping{}, errors.New("connection refused")
+				},
+				CloseF: func() error {
+					return nil
+				},
+			}, nil
 		},
 		newEnvClient: func() (dockerClient, error) {
-			return nil, errors.New("cannot connect to the Docker daemon")
+			return nil, errors.New("not using env client")
 		},
 	}
-
-	require.NoError(t, d.Init())
-
-	// Start should return a StartupError when Docker is unavailable
-	err := d.Start(&acc)
-	require.Error(t, err)
-
-	var startupErr *internal.StartupError
-	require.ErrorAs(t, err, &startupErr)
-	require.Contains(t, startupErr.Error(), "failed to create Docker client")
-}
-
-func TestStartWithPingFailure(t *testing.T) {
-	// Test that Start returns a retryable StartupError when Ping fails due to connection issues
+	model := models.NewRunningInput(plugin, &models.InputConfig{
+		Name:  "docker",
+		Alias: "error-test",
+	})
+	model.StartupErrors.Set(0)
+	require.NoError(t, model.Init())
+
+	// Starting the plugin will fail with an error because Ping fails
 	var acc testutil.Accumulator
+	require.ErrorContains(t, model.Start(&acc), "failed to ping Docker daemon")
+}
 
-	// Create a mock client that succeeds on creation but fails on Ping
-	d := Docker{
-		Log:     testutil.Logger{},
-		Timeout: config.Duration(5 * time.Second),
+func TestStartupErrorBehaviorIgnore(t *testing.T) {
+	// Test that model.Start returns fatal error with "ignore" behavior when Ping fails
+	plugin := &Docker{
+		Timeout: config.Duration(100 * time.Millisecond),
 		newClient: func(string, *tls.Config) (dockerClient, error) {
 			return &mockClient{
 				PingF: func() (types.Ping, error) {
@@ -1839,26 +1842,24 @@ func TestStartWithPingFailure(t *testing.T) {
 			return nil, errors.New("not using env client")
 		},
 	}
-
-	require.NoError(t, d.Init())
-
-	// Start should return a StartupError when Ping fails
-	err := d.Start(&acc)
+	model := models.NewRunningInput(plugin, &models.InputConfig{
+		Name:                 "docker",
+		Alias:                "ignore-test",
+		StartupErrorBehavior: "ignore",
+	})
+	model.StartupErrors.Set(0)
+	require.NoError(t, model.Init())
+
+	// Starting the plugin will fail and model should convert to fatal error
+	var acc testutil.Accumulator
+	err := model.Start(&acc)
 	require.Error(t, err)
-
-	var startupErr *internal.StartupError
-	require.ErrorAs(t, err, &startupErr)
-	require.Contains(t, startupErr.Error(), "failed to ping Docker daemon")
-	// Client should be nil since we clean up on failure
-	require.Nil(t, d.client)
+	require.Contains(t, err.Error(), "failed to ping Docker daemon")
 }
 
 func TestStartSuccess(t *testing.T) {
 	// Test that Start succeeds when Docker is available
-	var acc testutil.Accumulator
-
-	d := Docker{
-		Log:     testutil.Logger{},
+	plugin := &Docker{
 		Timeout: config.Duration(5 * time.Second),
 		newClient: func(string, *tls.Config) (dockerClient, error) {
 			return &mockClient{
@@ -1883,8 +1884,13 @@ func TestStartSuccess(t *testing.T) {
 			return nil, errors.New("not using env client")
 		},
 	}
+	model := models.NewRunningInput(plugin, &models.InputConfig{
+		Name:  "docker",
+		Alias: "success-test",
+	})
+	model.StartupErrors.Set(0)
+	require.NoError(t, model.Init())
 
-	require.NoError(t, d.Init())
-	require.NoError(t, d.Start(&acc))
-	require.NotNil(t, d.client)
+	var acc testutil.Accumulator
+	require.NoError(t, model.Start(&acc))
 }