diff --git a/.chloggen/opampsupervisor-configurable-stop-grace-period.yaml b/.chloggen/opampsupervisor-configurable-stop-grace-period.yaml new file mode 100644 index 0000000000000..e60105a192b98 --- /dev/null +++ b/.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] diff --git a/cmd/opampsupervisor/supervisor/commander/commander.go b/cmd/opampsupervisor/supervisor/commander/commander.go index 42396051ae186..2cffb09b6b475 100644 --- a/cmd/opampsupervisor/supervisor/commander/commander.go +++ b/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,6 +58,11 @@ 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, @@ -64,7 +70,7 @@ func NewCommander(logger *zap.Logger, logFilePath string, cfg config.Agent, args 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), diff --git a/cmd/opampsupervisor/supervisor/commander/commander_test.go b/cmd/opampsupervisor/supervisor/commander/commander_test.go index 286146ef9562f..c44a280e6147d 100644 --- a/cmd/opampsupervisor/supervisor/commander/commander_test.go +++ b/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) +} diff --git a/cmd/opampsupervisor/supervisor/config/config.go b/cmd/opampsupervisor/supervisor/config/config.go index 73be512179c13..148cc103caa4c 100644 --- a/cmd/opampsupervisor/supervisor/config/config.go +++ b/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, diff --git a/cmd/opampsupervisor/supervisor/config/config_test.go b/cmd/opampsupervisor/supervisor/config/config_test.go index 549da895b1bc7..2cd91d6d99a2e 100644 --- a/cmd/opampsupervisor/supervisor/config/config_test.go +++ b/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,