diff --git a/pkg/daemon/backoff.go b/pkg/daemon/backoff.go new file mode 100644 index 0000000000..a508d3952f --- /dev/null +++ b/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 :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) +} diff --git a/pkg/daemon/backoff_test.go b/pkg/daemon/backoff_test.go new file mode 100644 index 0000000000..74f90bcf48 --- /dev/null +++ b/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") +} diff --git a/pkg/daemon/daemon.go b/pkg/daemon/daemon.go index 3957146681..318329eeef 100644 --- a/pkg/daemon/daemon.go +++ b/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)