Reduce MachineConfigNodeFailed event spam during SNO upgrades

pkg/daemon/backoff.go

@@ -0,0 +1,82 @@
+package daemon
+
+import (
+	"errors"
+	"syscall"
+
+	utilnet "k8s.io/apimachinery/pkg/util/net"
+)
+
+// isAPIServerUnreachableError reports whether err indicates that the API server is
+// currently unreachable over the network, as opposed to a genuine, actionable
+// reconcile error.
+//
+// The canonical case is a single-node OpenShift (SNO) cluster whose only node
+// reboots during an upgrade: the kube-apiserver goes away with it, so every sync
+// attempt fails with "dial tcp <service-ip>:443: connect: connection refused" until
+// the API server comes back. Host- and network-unreachable errors show up the same
+// way while the node's networking is still coming up after the reboot. These are
+// transient connectivity failures that should be backed off rather than surfaced as
+// a node-degraded failure event on every single attempt.
+func isAPIServerUnreachableError(err error) bool {
+	if err == nil {
+		return false
+	}
+
+	// client-go surfaces these wrapped in *url.Error/*net.OpError chains; utilnet
+	// unwraps that chain and matches the underlying syscall errno.
+	if utilnet.IsConnectionRefused(err) || utilnet.IsConnectionReset(err) {
+		return true
+	}
+
+	// Host/network unreachable are the other transient forms of "the API server is
+	// not reachable right now" that occur while a rebooted node re-establishes its
+	// networking.
+	var errno syscall.Errno
+	if errors.As(err, &errno) {
+		return errno == syscall.EHOSTUNREACH || errno == syscall.ENETUNREACH
+	}
+
+	return false
+}
+
+// shouldReportUnreachable implements an exponential backoff schedule for
+// re-reporting a node's degraded state while the API server is unreachable. Given
+// the number of consecutive connection failures, it returns true only for the first
+// failure and thereafter on powers of two (1, 2, 4, 8, 16, ...).
+//
+// Combined with the sync queue's own exponential rate limiter (which spaces out the
+// retries themselves), this collapses a multi-minute outage that would otherwise
+// emit one MachineConfigNode failure event per sync attempt (dozens of identical
+// events, tripping the pathological-events monitor) down to a handful of events.
+func shouldReportUnreachable(consecutiveFailures int) bool {
+	if consecutiveFailures <= 1 {
+		return true
+	}
+	// A positive integer is a power of two iff exactly one bit is set, i.e.
+	// n & (n-1) == 0.
+	return consecutiveFailures&(consecutiveFailures-1) == 0
+}
+
+// shouldReportSyncError is the decision core of handleErr's exponential backoff for
+// API-unreachable errors on single-node (SNO) clusters. It updates the consecutive-
+// failure counter for err and returns whether the caller should (re-)report the
+// error this time.
+//
+// Non-connectivity errors reset the counter and are always reported (they represent
+// real, actionable failures). Connectivity errors increment the counter and are only
+// reported on the exponential schedule defined by shouldReportUnreachable.
+//
+// handleErr gates calls to this function behind an isSingleNodeTopology check so
+// the backoff is never applied on multi-node (HA) clusters.
+//
+// It is only called from the single sync worker goroutine, so the counter needs no
+// additional locking.
+func (dn *Daemon) shouldReportSyncError(err error) bool {
+	if !isAPIServerUnreachableError(err) {
+		dn.apiUnreachableFailures = 0
+		return true
+	}
+	dn.apiUnreachableFailures++
+	return shouldReportUnreachable(dn.apiUnreachableFailures)
+}

pkg/daemon/backoff_test.go

@@ -0,0 +1,212 @@
+package daemon
+
+import (
+	"errors"
+	"fmt"
+	"net"
+	"net/url"
+	"os"
+	"syscall"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	corev1 "k8s.io/api/core/v1"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/client-go/util/workqueue"
+
+	configv1 "github.com/openshift/api/config/v1"
+	"github.com/openshift/machine-config-operator/pkg/daemon/constants"
+)
+
+// newConnRefusedError builds an error shaped like the ones client-go returns when
+// the API server is down: *url.Error -> *net.OpError -> *os.SyscallError -> errno.
+// The rendered message matches the real-world "dial tcp 172.30.0.1:443: connect:
+// connection refused" seen during SNO upgrades.
+func newConnRefusedError(errno syscall.Errno) error {
+	return &url.Error{
+		Op:  "Get",
+		URL: "https://172.30.0.1:443/apis/machineconfiguration.openshift.io/v1/machineconfignodes/node-0",
+		Err: &net.OpError{
+			Op:   "dial",
+			Net:  "tcp",
+			Addr: &net.TCPAddr{IP: net.IPv4(172, 30, 0, 1), Port: 443},
+			Err:  os.NewSyscallError("connect", errno),
+		},
+	}
+}
+
+func TestIsAPIServerUnreachableError(t *testing.T) {
+	tests := []struct {
+		name string
+		err  error
+		want bool
+	}{
+		{name: "nil", err: nil, want: false},
+		{name: "unrelated error", err: errors.New("something else went wrong"), want: false},
+		{name: "real API NotFound error", err: apierrors.NewNotFound(schema.GroupResource{Group: "machineconfiguration.openshift.io", Resource: "machineconfignodes"}, "node-0"), want: false},
+		{name: "unrelated errno (EPERM)", err: syscall.EPERM, want: false},
+
+		{name: "bare ECONNREFUSED", err: syscall.ECONNREFUSED, want: true},
+		{name: "wrapped ECONNREFUSED", err: fmt.Errorf("apply status failed: %w", syscall.ECONNREFUSED), want: true},
+		{name: "client-go style connection refused", err: newConnRefusedError(syscall.ECONNREFUSED), want: true},
+		{name: "connection reset by peer", err: newConnRefusedError(syscall.ECONNRESET), want: true},
+		{name: "host unreachable", err: newConnRefusedError(syscall.EHOSTUNREACH), want: true},
+		{name: "network unreachable", err: newConnRefusedError(syscall.ENETUNREACH), want: true},
+		{name: "wrapped host unreachable", err: fmt.Errorf("dialing: %w", syscall.EHOSTUNREACH), want: true},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			assert.Equal(t, tc.want, isAPIServerUnreachableError(tc.err))
+		})
+	}
+}
+
+func TestShouldReportUnreachable(t *testing.T) {
+	// True only on the first failure and on powers of two thereafter.
+	wantTrue := map[int]bool{0: true, 1: true, 2: true, 4: true, 8: true, 16: true, 32: true, 64: true}
+	for n := 0; n <= 64; n++ {
+		got := shouldReportUnreachable(n)
+		if wantTrue[n] {
+			assert.Truef(t, got, "shouldReportUnreachable(%d) should be true (power of two)", n)
+		} else {
+			assert.Falsef(t, got, "shouldReportUnreachable(%d) should be false", n)
+		}
+	}
+}
+
+func TestShouldReportSyncErrorBackoff(t *testing.T) {
+	connErr := newConnRefusedError(syscall.ECONNREFUSED)
+
+	t.Run("collapses a long outage to a handful of reports", func(t *testing.T) {
+		dn := &Daemon{}
+
+		// Simulate a ~5-10 minute API outage: 28 consecutive connection-refused sync
+		// failures (the count observed tripping the pathological-events monitor).
+		const outageFailures = 28
+		var reportedAt []int
+		for i := 1; i <= outageFailures; i++ {
+			if dn.shouldReportSyncError(connErr) {
+				reportedAt = append(reportedAt, i)
+			}
+		}
+
+		// Without backoff this would emit 28 failure events; with exponential backoff
+		// we report only on the 1st, 2nd, 4th, 8th and 16th consecutive failure.
+		assert.Equal(t, []int{1, 2, 4, 8, 16}, reportedAt)
+		assert.Lenf(t, reportedAt, 5, "expected a handful of reports, got %d for %d failures", len(reportedAt), outageFailures)
+		assert.Less(t, len(reportedAt), outageFailures/4, "backoff should drastically reduce the number of reports")
+	})
+
+	t.Run("non-connectivity error is always reported and resets the counter", func(t *testing.T) {
+		dn := &Daemon{}
+
+		// Build up some consecutive connection failures.
+		assert.True(t, dn.shouldReportSyncError(connErr))  // failure 1 -> report
+		assert.True(t, dn.shouldReportSyncError(connErr))  // failure 2 -> report
+		assert.False(t, dn.shouldReportSyncError(connErr)) // failure 3 -> suppressed
+		assert.Equal(t, 3, dn.apiUnreachableFailures)
+
+		// A genuine, actionable error must always be reported and must reset the
+		// backoff so the next outage starts fresh.
+		assert.True(t, dn.shouldReportSyncError(errors.New("reconcile failed")))
+		assert.Equal(t, 0, dn.apiUnreachableFailures)
+
+		// The connection-refused schedule restarts from the first failure.
+		assert.True(t, dn.shouldReportSyncError(connErr)) // failure 1 again -> report
+		assert.Equal(t, 1, dn.apiUnreachableFailures)
+	})
+
+	t.Run("nil error resets the counter and is treated as success", func(t *testing.T) {
+		dn := &Daemon{apiUnreachableFailures: 7}
+		// isAPIServerUnreachableError(nil) is false, so the counter resets.
+		assert.True(t, dn.shouldReportSyncError(nil))
+		assert.Equal(t, 0, dn.apiUnreachableFailures)
+	})
+}
+
+// fakeNodeWriter is a minimal NodeWriter used to drive handleErr in tests. Its
+// SetDegraded deliberately returns an error so updateErrorState short-circuits
+// before reaching cluster-dependent code (primary-pool lookup and MCN status
+// updates). The apiUnreachableFailures bookkeeping under test happens in
+// handleErr itself -- for SNO via shouldReportSyncError, and for HA via the
+// explicit reset -- both of which run before updateErrorState, so short-circuiting
+// the annotation write does not affect what these tests verify.
+type fakeNodeWriter struct{}
+
+func (f *fakeNodeWriter) Run(_ <-chan struct{})            {}
+func (f *fakeNodeWriter) SetDone(_ *stateAndConfigs) error { return nil }
+func (f *fakeNodeWriter) SetWorking() error                { return nil }
+func (f *fakeNodeWriter) SetUnreconcilable(_ error) error  { return nil }
+func (f *fakeNodeWriter) SetDegraded(_ error) error {
+	return errors.New("fake node writer: annotation writes are not wired up in this test")
+}
+func (f *fakeNodeWriter) SetAnnotations(_ map[string]string) (*corev1.Node, error) { return nil, nil }
+func (f *fakeNodeWriter) SetDesiredDrainer(_ string) error                         { return nil }
+func (f *fakeNodeWriter) Eventf(_, _, _ string, _ ...interface{})                  {}
+
+// TestHandleErrTopologyTransitionResetsBackoff is a regression test for a
+// SNO -> HA -> SNO control-plane topology transition. The node controller can
+// rewrite the controlPlaneTopology annotation during the daemon's lifetime, and
+// handleErr reads it fresh on every call via getControlPlaneTopology(). Before
+// the fix, the API-unreachable backoff counter accumulated on SNO was never
+// cleared when the cluster became HA, so a later return to SNO would resume the
+// exponential backoff from a stale count and wrongly suppress or delay
+// legitimate MachineConfigNode failure reporting. handleErr must reset the
+// counter whenever the HA branch handles a sync error.
+func TestHandleErrTopologyTransitionResetsBackoff(t *testing.T) {
+	connErr := newConnRefusedError(syscall.ECONNREFUSED)
+
+	// The daemon derives its topology from the node annotation, so mutating the
+	// annotation on this shared node object simulates the node controller flipping
+	// the cluster topology at runtime.
+	node := &corev1.Node{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:        "node-0",
+			Annotations: map[string]string{},
+		},
+	}
+	dn := &Daemon{
+		node:       node,
+		nodeWriter: &fakeNodeWriter{},
+		queue: workqueue.NewTypedRateLimitingQueueWithConfig[string](
+			workqueue.DefaultTypedControllerRateLimiter[string](),
+			workqueue.TypedRateLimitingQueueConfig[string]{Name: "backoff-topology-transition-test"}),
+	}
+	defer dn.queue.ShutDown()
+
+	setTopology := func(mode configv1.TopologyMode) {
+		node.Annotations[constants.ClusterControlPlaneTopologyAnnotationKey] = string(mode)
+	}
+
+	const key = "test/node-0"
+
+	// 1. SNO: a multi-minute API outage accumulates the backoff counter across
+	//    consecutive connection-refused sync failures.
+	setTopology(configv1.SingleReplicaTopologyMode)
+	const snoOutageFailures = 10
+	for i := 0; i < snoOutageFailures; i++ {
+		dn.handleErr(connErr, key)
+	}
+	require.Equal(t, snoOutageFailures, dn.apiUnreachableFailures,
+		"SNO outage should accumulate the API-unreachable backoff counter")
+
+	// 2. Topology flips to HA (node controller rewrote the annotation). Handling a
+	//    sync error on HA must clear the stale SNO backoff counter, because HA
+	//    reports every sync error immediately and must not carry SNO backoff state.
+	setTopology(configv1.HighlyAvailableTopologyMode)
+	dn.handleErr(connErr, key)
+	require.Equal(t, 0, dn.apiUnreachableFailures,
+		"HA branch must reset the stale SNO backoff counter (SNO -> HA transition)")
+
+	// 3. Topology flips back to SNO. Because HA cleared the counter, the backoff
+	//    schedule starts fresh from the first failure rather than resuming from the
+	//    stale pre-transition count.
+	setTopology(configv1.SingleReplicaTopologyMode)
+	dn.handleErr(connErr, key)
+	require.Equal(t, 1, dn.apiUnreachableFailures,
+		"SNO backoff must restart fresh after returning from HA, not resume the stale count")
+}

pkg/daemon/daemon.go

@@ -146,6 +146,16 @@ type Daemon struct {
 	// rebootQueued is true when the node is waiting for graceful shutdown
 	rebootQueued bool
 
+	// apiUnreachableFailures counts consecutive sync failures caused by the API
+	// server being unreachable on single-node (SNO) clusters (for example while
+	// the only node reboots during an upgrade). It drives an exponential backoff
+	// that avoids re-reporting the node's degraded state -- and emitting a
+	// MachineConfigNode failure event -- on every sync attempt during an outage.
+	// On multi-node (HA) clusters the backoff is not applied. It is only
+	// accessed from the single sync worker goroutine, so it needs no additional
+	// locking.
+	apiUnreachableFailures int
+
 	currentConfigPath string
 	currentImagePath  string
 
@@ -621,6 +631,7 @@ func (dn *Daemon) processNextWorkItem() bool {
 func (dn *Daemon) handleErr(err error, key string) {
 	if err == nil {
 		dn.queue.Forget(key)
+		dn.apiUnreachableFailures = 0
 		return
 	}
 
@@ -630,8 +641,35 @@ func (dn *Daemon) handleErr(err error, key string) {
 		klog.Fatalf("Error handling node sync: %v", err)
 	}
 
-	if err := dn.updateErrorState(err); err != nil {
-		klog.Errorf("Could not update annotation: %v", err)
+	// On single-node (SNO) clusters whose only node reboots during an upgrade,
+	// the API server goes down and every sync attempt fails with "connection
+	// refused" until it comes back. Marking the node Degraded on each of those
+	// attempts emits a MachineConfigNode failure event every time and, over a
+	// multi-minute outage, trips the pathological-events monitor. On SNO we
+	// apply an exponential backoff so we only re-report on the 1st, 2nd, 4th,
+	// 8th, ... consecutive connection failure. On multi-node (HA) clusters
+	// API-unreachable errors are uncommon and actionable, so we always report
+	// them immediately.
+	if isSingleNodeTopology(dn.getControlPlaneTopology()) {
+		if dn.shouldReportSyncError(err) {
+			if err := dn.updateErrorState(err); err != nil {
+				klog.Errorf("Could not update annotation: %v", err)
+			}
+		} else {
+			klog.V(2).Infof("API server appears unreachable (%d consecutive failures); backing off before re-reporting node degraded state", dn.apiUnreachableFailures)
+		}
+	} else {
+		// On multi-node (HA) clusters we report every sync error immediately, so
+		// clear any API-unreachable backoff state that may have been accumulated
+		// while this node was single-node. The node controller can update the
+		// control-plane topology annotation during the daemon's lifetime; without
+		// this reset a SNO -> HA -> SNO transition could resume the exponential
+		// backoff from a stale failure count and wrongly suppress or delay
+		// legitimate degraded-state reports.
+		dn.apiUnreachableFailures = 0
+		if err := dn.updateErrorState(err); err != nil {
+			klog.Errorf("Could not update annotation: %v", err)
+		}
 	}
 	// This is at V(2) since the updateErrorState() call above ends up logging too
 	klog.V(2).Infof("Error syncing node %v (retries %d): %v", key, dn.queue.NumRequeues(key), err)