diff --git a/changelog/fragments/1783505837-give-service-runtime-components-more-time-to-check-in-before-failing-an-upgrade.yaml b/changelog/fragments/1783505837-give-service-runtime-components-more-time-to-check-in-before-failing-an-upgrade.yaml new file mode 100644 index 00000000000..356df54a8b5 --- /dev/null +++ b/changelog/fragments/1783505837-give-service-runtime-components-more-time-to-check-in-before-failing-an-upgrade.yaml @@ -0,0 +1,49 @@ +# REQUIRED +# Kind can be one of: +# - breaking-change: a change to previously-documented behavior +# - deprecation: functionality that is being removed in a later release +# - bug-fix: fixes a problem in a previous version +# - enhancement: extends functionality but does not break or fix existing behavior +# - feature: new functionality +# - known-issue: problems that we are aware of in a given version +# - security: impacts on the security of a product or a user’s deployment. +# - upgrade: important information for someone upgrading from a prior version +# - other: does not fit into any of the other categories +kind: bug-fix + +# REQUIRED for all kinds +# Change summary; a 80ish characters long description of the change. +summary: Stop upgrade rollback from being triggered by slow service component restarts + +# REQUIRED for breaking-change, deprecation, known-issue +# Long description; in case the summary is not enough to describe the change +# this field accommodate a description without length limits. +description: | + Elastic Defend restarting slowly after an upgrade could be mistaken for a + broken Agent build and trigger an unnecessary rollback. Agent now gives + service-managed components more time to check in before treating them as + failed. + +# REQUIRED for breaking-change, deprecation, known-issue +# impact: + +# REQUIRED for breaking-change, deprecation, known-issue +# action: + +# REQUIRED for all kinds +# Affected component; usually one of "elastic-agent", "fleet-server", "filebeat", "metricbeat", "auditbeat", "all", etc. +component: elastic-agent + +# AUTOMATED +# OPTIONAL to manually add other PR URLs +# PR URL: A link the PR that added the changeset. +# If not present is automatically filled by the tooling finding the PR where this changelog fragment has been added. +# NOTE: the tooling supports backports, so it's able to fill the original PR number instead of the backport PR number. +# Please provide it if you are adding a fragment for a different PR. +# pr: https://github.com/owner/repo/1234 + +# AUTOMATED +# OPTIONAL to manually add other issue URLs +# Issue URL; optional; the GitHub issue related to this changeset (either closes or is part of). +# If not present is automatically filled by the tooling with the issue linked to the PR number. +# issue: https://github.com/owner/repo/1234 diff --git a/pkg/component/runtime/service.go b/pkg/component/runtime/service.go index 729589a07ba..3d4666acad0 100644 --- a/pkg/component/runtime/service.go +++ b/pkg/component/runtime/service.go @@ -563,18 +563,45 @@ func (s *serviceRuntime) checkStatus(checkinPeriod time.Duration, lastCheckin *t } else if now.Sub(*lastCheckin) <= checkinPeriod { *missedCheckins = 0 } + maxMisses := s.maxCheckinMisses(checkinPeriod) if *missedCheckins == 0 { s.compState(client.UnitStateHealthy, *missedCheckins) - } else if *missedCheckins > 0 && *missedCheckins < maxCheckinMisses { + } else if *missedCheckins > 0 && *missedCheckins < maxMisses { s.compState(client.UnitStateDegraded, *missedCheckins) - } else if *missedCheckins >= maxCheckinMisses { + } else if *missedCheckins >= maxMisses { // something is wrong; the service should be checking in - msg := fmt.Sprintf("Failed: %s service missed %d check-ins", s.name(), maxCheckinMisses) + msg := fmt.Sprintf("Failed: %s service missed %d check-ins", s.name(), maxMisses) s.forceCompState(client.UnitStateFailed, msg) } } } +// checkinFailureTimeout returns how long this service is allowed to take to +// start up before a lack of check-ins is treated as a real failure. +func (s *serviceRuntime) checkinFailureTimeout() time.Duration { + ops := s.comp.InputSpec.Spec.Service.Operations + var longest time.Duration + for _, op := range []*component.ServiceOperationsCommandSpec{ops.Check, ops.Install, ops.Uninstall} { + if op != nil && op.Timeout > longest { + longest = op.Timeout + } + } + return longest +} + +// maxCheckinMisses returns how many consecutive check-ins this component can +// miss before it's marked FAILED. +func (s *serviceRuntime) maxCheckinMisses(checkinPeriod time.Duration) int { + timeout := s.checkinFailureTimeout() + if timeout <= 0 || checkinPeriod <= 0 { + return maxCheckinMisses + } + if misses := int(timeout / checkinPeriod); misses > maxCheckinMisses { + return misses + } + return maxCheckinMisses +} + func (s *serviceRuntime) checkinPeriod() time.Duration { checkinPeriod := s.comp.InputSpec.Spec.Service.Timeouts.Checkin if checkinPeriod == 0 { diff --git a/pkg/component/runtime/service_test.go b/pkg/component/runtime/service_test.go index 80bb5311848..f2f03c02b87 100644 --- a/pkg/component/runtime/service_test.go +++ b/pkg/component/runtime/service_test.go @@ -454,6 +454,105 @@ func TestServiceStartRetry(t *testing.T) { }, service.serviceRestartDelay+1*time.Second, 500*time.Millisecond) } +func makeServiceForCheckStatus(t *testing.T, installTimeout, uninstallTimeout, checkTimeout time.Duration) *serviceRuntime { + log, _ := loggertest.New("test") + endpoint := makeEndpointComponent(t, map[string]interface{}{}) + endpoint.InputSpec.Spec.Service = &component.ServiceSpec{ + CPort: 9999, + CSocket: ".test.sock", + Operations: component.ServiceOperationsSpec{ + Check: &component.ServiceOperationsCommandSpec{Timeout: checkTimeout}, + Install: &component.ServiceOperationsCommandSpec{Timeout: installTimeout}, + Uninstall: &component.ServiceOperationsCommandSpec{Timeout: uninstallTimeout}, + }, + } + + service, err := newServiceRuntime(endpoint, log, true) + require.NoError(t, err) + // simulate the service already being up and running + service.state.State = client.UnitStateHealthy + + // drain state updates so compState/forceCompState never block on service.ch + done := make(chan struct{}) + t.Cleanup(func() { close(done) }) + go func() { + for { + select { + case <-done: + return + case <-service.ch: + } + } + }() + + return service +} + +func TestServiceCheckinFailureTimeout(t *testing.T) { + service := makeServiceForCheckStatus(t, 600*time.Second, 600*time.Second, 60*time.Second) + require.Equal(t, 600*time.Second, service.checkinFailureTimeout()) + require.Equal(t, 20, service.maxCheckinMisses(30*time.Second)) + + // falls back to the generic count when nothing is configured + noTimeouts := makeServiceForCheckStatus(t, 0, 0, 0) + require.Equal(t, time.Duration(0), noTimeouts.checkinFailureTimeout()) + require.Equal(t, maxCheckinMisses, noTimeouts.maxCheckinMisses(30*time.Second)) +} + +func TestServiceCheckStatus(t *testing.T) { + const checkinPeriod = 30 * time.Second + + t.Run("normal check-in stays healthy", func(t *testing.T) { + service := makeServiceForCheckStatus(t, 600*time.Second, 600*time.Second, 60*time.Second) + lastCheckin := time.Now() + missedCheckins := 0 + + service.checkStatus(checkinPeriod, &lastCheckin, &missedCheckins) + + require.Equal(t, client.UnitStateHealthy, service.state.State) + require.Equal(t, 0, missedCheckins) + }) + + t.Run("slow but within configured operation timeout stays degraded", func(t *testing.T) { + service := makeServiceForCheckStatus(t, 600*time.Second, 600*time.Second, 60*time.Second) + lastCheckin := time.Now().Add(-150 * time.Second) + missedCheckins := 0 + + // past the old 90s window, but well under the 600s timeout + for i := 0; i < 5; i++ { + service.checkStatus(checkinPeriod, &lastCheckin, &missedCheckins) + } + + require.Equal(t, client.UnitStateDegraded, service.state.State) + }) + + t.Run("stuck past the configured operation timeout is marked failed", func(t *testing.T) { + service := makeServiceForCheckStatus(t, 600*time.Second, 600*time.Second, 60*time.Second) + lastCheckin := time.Now().Add(-660 * time.Second) + missedCheckins := 0 + + // past the 600s timeout + for i := 0; i < 22; i++ { + service.checkStatus(checkinPeriod, &lastCheckin, &missedCheckins) + } + + require.Equal(t, client.UnitStateFailed, service.state.State) + }) + + t.Run("component with no configured operation timeouts keeps the generic 90s failure window", func(t *testing.T) { + service := makeServiceForCheckStatus(t, 0, 0, 0) + lastCheckin := time.Now().Add(-120 * time.Second) + missedCheckins := 0 + + // past the generic 90s window + for i := 0; i < 4; i++ { + service.checkStatus(checkinPeriod, &lastCheckin, &missedCheckins) + } + + require.Equal(t, client.UnitStateFailed, service.state.State) + }) +} + func mockEndpointBinary(t *testing.T, exitCode int) string { // Build a mock Endpoint binary that can return a specific exit code. outPath := filepath.Join(t.TempDir(), "mock_endpoint")