[cmd/opampsupervisor] Make the Collector stop grace period configurable

.chloggen/opampsupervisor-configurable-stop-grace-period.yaml

@@ -0,0 +1,27 @@
+# Use this changelog template to create an entry for release notes.
+
+# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
+change_type: enhancement
+
+# The name of the component, or a single word describing the area of concern, (e.g. receiver/filelog)
+component: cmd/opampsupervisor
+
+# A brief description of the change.  Surround your text with quotes ("") if it needs to start with a backtick (`).
+note: Add an `agent::stop_grace_period` config option to control how long the Supervisor waits for the Collector to exit after a graceful shutdown signal before forcibly killing it.
+
+# Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists.
+issues: [50000]
+
+# (Optional) One or more lines of additional information to render under the primary note.
+# These lines will be padded with 2 spaces and then inserted directly into the document.
+# Use pipe (|) for multiline entries.
+subtext: Defaults to 10s when unset.
+
+# If your change doesn't affect end users or the exported elements of any package,
+# you should instead start your pull request title with [chore] or use the "Skip Changelog" label.
+# Optional: The change log or logs in which this entry should be included.
+# e.g. '[user]' or '[user, api]'
+# Include 'user' if the change is relevant to end users.
+# Include 'api' if there is a change to a library API.
+# Default: '[user]'
+change_logs: [user]

cmd/opampsupervisor/supervisor/commander/commander.go

@@ -23,8 +23,9 @@ import (
 	"github.com/open-telemetry/opentelemetry-collector-contrib/cmd/opampsupervisor/supervisor/config"
 )
 
-// defaultStopGracePeriod is how long Stop waits for the Agent process to exit
-// after the graceful shutdown signal before killing it forcibly.
+// defaultStopGracePeriod is the fallback used when agent::stop_grace_period is
+// not set: how long Stop waits for the Agent process to exit after the graceful
+// shutdown signal before killing it forcibly.
 const defaultStopGracePeriod = 10 * time.Second
 
 // AgentStartedLogMsg is logged every time an Agent process is started. Each site
@@ -57,14 +58,19 @@ type Commander struct {
 }
 
 func NewCommander(logger *zap.Logger, logFilePath string, cfg config.Agent, args ...string) (*Commander, error) {
+	stopGracePeriod := cfg.StopGracePeriod
+	if stopGracePeriod <= 0 {
+		// Fall back to the default when unset, e.g. a config built without defaults.
+		stopGracePeriod = defaultStopGracePeriod
+	}
 	return &Commander{
 		logger:          logger,
 		logFilePath:     logFilePath,
 		cfg:             cfg,
 		args:            args,
 		outputDoneCh:    make(chan struct{}),
 		running:         &atomic.Int64{},
-		stopGracePeriod: defaultStopGracePeriod,
+		stopGracePeriod: stopGracePeriod,
 		// Buffer channels so we can send messages without blocking on listeners.
 		doneCh: make(chan struct{}, 1),
 		exitCh: make(chan struct{}, 1),

cmd/opampsupervisor/supervisor/commander/commander_test.go

@@ -229,3 +229,28 @@ func TestStopKillsUnresponsiveProcess(t *testing.T) {
 	require.NoError(t, cmdr.Stop(t.Context()))
 	require.False(t, cmdr.IsRunning())
 }
+
+// NewCommander uses the configured stop grace period, falling back to the
+// default when it is unset.
+func TestNewCommanderUsesConfiguredStopGracePeriod(t *testing.T) {
+	cmdr, err := NewCommander(
+		zap.NewNop(),
+		filepath.Join(t.TempDir(), "agent.log"),
+		config.Agent{
+			Executable:      os.Args[0],
+			StopGracePeriod: 3 * time.Second,
+		},
+	)
+	require.NoError(t, err)
+	require.Equal(t, 3*time.Second, cmdr.stopGracePeriod)
+
+	cmdrDefault, err := NewCommander(
+		zap.NewNop(),
+		filepath.Join(t.TempDir(), "agent.log"),
+		config.Agent{
+			Executable: os.Args[0],
+		},
+	)
+	require.NoError(t, err)
+	require.Equal(t, defaultStopGracePeriod, cmdrDefault.stopGracePeriod)
+}

cmd/opampsupervisor/supervisor/config/config.go

@@ -223,6 +223,7 @@ type Agent struct {
 	Description                 AgentDescription  `mapstructure:"description"`
 	ConfigApplyTimeout          time.Duration     `mapstructure:"config_apply_timeout"`
 	BootstrapTimeout            time.Duration     `mapstructure:"bootstrap_timeout"`
+	StopGracePeriod             time.Duration     `mapstructure:"stop_grace_period"`
 	OpAMPServerPort             int               `mapstructure:"opamp_server_port"`
 	PassthroughLogs             bool              `mapstructure:"passthrough_logs"`
 	CollectorCrashLogSnippetKiB int               `mapstructure:"collector_crash_log_snippet_kib"`
@@ -279,6 +280,10 @@ func (a Agent) Validate() error {
 		return errors.New("agent::config_apply_timeout must be valid duration")
 	}
 
+	if a.StopGracePeriod < 0 {
+		return errors.New("agent::stop_grace_period must not be negative")
+	}
+
 	for _, file := range a.ConfigFiles {
 		if !strings.HasPrefix(file, "$") {
 			continue
@@ -479,6 +484,7 @@ func DefaultSupervisor() Supervisor {
 			OrphanDetectionInterval:     5 * time.Second,
 			ConfigApplyTimeout:          5 * time.Second,
 			BootstrapTimeout:            3 * time.Second,
+			StopGracePeriod:             10 * time.Second,
 			PassthroughLogs:             false,
 			CollectorCrashLogSnippetKiB: 0,
 			ValidateConfig:              false,

cmd/opampsupervisor/supervisor/config/config_test.go

@@ -472,6 +472,33 @@ func TestValidate(t *testing.T) {
 			},
 			expectedErrorFunc: simpleError("agent::config_apply_timeout must be valid duration"),
 		},
+		{
+			name: "Negative stop grace period",
+			config: Supervisor{
+				Server: OpAMPServer{
+					Endpoint: "wss://localhost:9090/opamp",
+					Headers: http.Header{
+						"Header1": []string{"HeaderValue"},
+					},
+					TLS: tlsConfig,
+				},
+				Agent: Agent{
+					Executable:              "${file_path}",
+					OrphanDetectionInterval: 5 * time.Second,
+					OpAMPServerPort:         8080,
+					ConfigApplyTimeout:      2 * time.Second,
+					BootstrapTimeout:        5 * time.Second,
+					StopGracePeriod:         -1 * time.Second,
+				},
+				Capabilities: Capabilities{
+					AcceptsRemoteConfig: true,
+				},
+				Storage: Storage{
+					Directory: "/etc/opamp-supervisor/storage",
+				},
+			},
+			expectedErrorFunc: simpleError("agent::stop_grace_period must not be negative"),
+		},
 		{
 			name: "HUP config reload not supported on Windows",
 			config: Supervisor{
@@ -1058,6 +1085,7 @@ agent:
 						OrphanDetectionInterval:     DefaultSupervisor().Agent.OrphanDetectionInterval,
 						ConfigApplyTimeout:          DefaultSupervisor().Agent.ConfigApplyTimeout,
 						BootstrapTimeout:            DefaultSupervisor().Agent.BootstrapTimeout,
+						StopGracePeriod:             DefaultSupervisor().Agent.StopGracePeriod,
 						CollectorCrashLogSnippetKiB: DefaultSupervisor().Agent.CollectorCrashLogSnippetKiB,
 						ValidateConfig:              DefaultSupervisor().Agent.ValidateConfig,
 						Package:                     DefaultSupervisor().Agent.Package,
@@ -1103,6 +1131,7 @@ agent:
   orphan_detection_interval: 10s
   config_apply_timeout: 8s
   bootstrap_timeout: 8s
+  stop_grace_period: 20s
   opamp_server_port: 8090
   passthrough_logs: true
   automatic_config_rollback: true
@@ -1155,6 +1184,7 @@ telemetry:
 						OrphanDetectionInterval:     10 * time.Second,
 						ConfigApplyTimeout:          8 * time.Second,
 						BootstrapTimeout:            8 * time.Second,
+						StopGracePeriod:             20 * time.Second,
 						OpAMPServerPort:             8090,
 						PassthroughLogs:             true,
 						CollectorCrashLogSnippetKiB: 100,
@@ -1201,6 +1231,7 @@ agent:
 						OrphanDetectionInterval:     DefaultSupervisor().Agent.OrphanDetectionInterval,
 						ConfigApplyTimeout:          DefaultSupervisor().Agent.ConfigApplyTimeout,
 						BootstrapTimeout:            DefaultSupervisor().Agent.BootstrapTimeout,
+						StopGracePeriod:             DefaultSupervisor().Agent.StopGracePeriod,
 						CollectorCrashLogSnippetKiB: DefaultSupervisor().Agent.CollectorCrashLogSnippetKiB,
 						ValidateConfig:              DefaultSupervisor().Agent.ValidateConfig,
 						Package:                     DefaultSupervisor().Agent.Package,