Give service-runtime components more time to check in before failing an upgrade

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

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 {

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")