diff --git a/pkg/defaults/defaults.go b/pkg/defaults/defaults.go index 4ece394a92892..fa0fb98723a1e 100644 --- a/pkg/defaults/defaults.go +++ b/pkg/defaults/defaults.go @@ -10,7 +10,10 @@ import ( // Hive options const ( // HiveStartTimeout is the default value for option.HiveStartTimeout - HiveStartTimeout = 5 * time.Minute + // NOTE: temporarily very large for control-plane-outage testing so the + // agent does not abort start hooks (e.g. the API server cold-start wait) + // too soon. Revert to 5 * time.Minute before merging upstream. + HiveStartTimeout = 168 * time.Hour // HiveStopTimeout is the default value for option.HiveStopTimeout HiveStopTimeout = time.Minute diff --git a/pkg/k8s/client/cell.go b/pkg/k8s/client/cell.go index d1868a96470fe..aca9981c86c2c 100644 --- a/pkg/k8s/client/cell.go +++ b/pkg/k8s/client/cell.go @@ -310,8 +310,18 @@ func (c *compositeClientset) startHeartbeat() { } func (c *compositeClientset) waitForConn(ctx context.Context) error { + // How long to keep retrying the initial connection before giving up is + // configurable so the agent does not terminate too soon during a + // control-plane / API server outage. Fall back to the built-in default + // when unset. The effective wait is still bounded by ctx (the hive + // start-hook timeout). + connRetryTimeout := connTimeout + if c.config.K8sClientConnectionRetryTimeout > 0 { + connRetryTimeout = c.config.K8sClientConnectionRetryTimeout + } + stop := make(chan struct{}) - timeout := time.NewTimer(connTimeout) + timeout := time.NewTimer(connRetryTimeout) defer timeout.Stop() var err error wait.Until(func() { diff --git a/pkg/k8s/client/config.go b/pkg/k8s/client/config.go index ee2c4bb990a28..6231630ea0edc 100644 --- a/pkg/k8s/client/config.go +++ b/pkg/k8s/client/config.go @@ -40,6 +40,14 @@ type SharedConfig struct { // K8sHeartbeatTimeout configures the timeout for apiserver heartbeat K8sHeartbeatTimeout time.Duration + // K8sClientConnectionRetryTimeout configures how long the agent keeps + // retrying to establish its initial connection to the API server at + // startup (cold start) before giving up and exiting. Increase to avoid + // terminating too soon during a control-plane / API server outage. The + // effective wait is min(this, --hive-start-timeout), so raise + // --hive-start-timeout as well for values above its default. + K8sClientConnectionRetryTimeout time.Duration + // EnableAPIDiscovery enables Kubernetes API discovery EnableK8sAPIDiscovery bool } @@ -69,7 +77,13 @@ var defaultSharedConfig = SharedConfig{ K8sClientConnectionTimeout: 30 * time.Second, K8sClientConnectionKeepAlive: 30 * time.Second, K8sHeartbeatTimeout: 30 * time.Second, - EnableK8sAPIDiscovery: defaults.K8sEnableAPIDiscovery, + // NOTE: temporarily very large for control-plane-outage testing so the + // agent does not terminate at cold start when the API server is + // unreachable. Revert to 1 minute (the previous hardcoded connTimeout) + // before merging upstream. The default HiveStartTimeout is raised to + // match so this is not capped at start-hook timeout. + K8sClientConnectionRetryTimeout: 168 * time.Hour, + EnableK8sAPIDiscovery: defaults.K8sEnableAPIDiscovery, } func (def SharedConfig) Flags(flags *pflag.FlagSet) { @@ -79,6 +93,7 @@ func (def SharedConfig) Flags(flags *pflag.FlagSet) { flags.Duration(option.K8sClientConnectionTimeout, def.K8sClientConnectionTimeout, "Configures the timeout of K8s client connections. K8s client is disabled if the value is set to 0") flags.Duration(option.K8sClientConnectionKeepAlive, def.K8sClientConnectionKeepAlive, "Configures the keep alive duration of K8s client connections. K8 client is disabled if the value is set to 0") flags.Duration(option.K8sHeartbeatTimeout, def.K8sHeartbeatTimeout, "Configures the timeout for api-server heartbeat, set to 0 to disable") + flags.Duration("k8s-client-connection-retry-timeout", def.K8sClientConnectionRetryTimeout, "Maximum time the agent keeps retrying its initial API server connection at startup before exiting. Increase to avoid terminating too soon during a control-plane / API server outage. Effective wait is min(this, --hive-start-timeout). A non-positive value falls back to the built-in default") flags.Bool(option.K8sEnableAPIDiscovery, def.EnableK8sAPIDiscovery, "Enable discovery of Kubernetes API groups and resources with the discovery API") } diff --git a/pkg/nodediscovery/cell.go b/pkg/nodediscovery/cell.go index edce21a9d16ab..f071145860b2a 100644 --- a/pkg/nodediscovery/cell.go +++ b/pkg/nodediscovery/cell.go @@ -9,6 +9,7 @@ import ( "github.com/cilium/cilium/pkg/defaults" "github.com/cilium/cilium/pkg/endpoint/regeneration" + "github.com/cilium/cilium/pkg/time" ) // The node discovery cell provides the local node configuration and node discovery @@ -39,6 +40,12 @@ var defaultConfig = config{ IPAMMaxAllocate: 0, IPAMStaticIPTags: map[string]string{}, + // NOTE: temporarily very large for control-plane-outage testing so the + // agent keeps retrying instead of exiting. Revert to maxRetryCount (10) + // before merging upstream. + CiliumNodeUpdateMaxRetries: 1000000, + CiliumNodeUpdateRetryBackoff: backoffDuration, + ENIFirstInterfaceIndex: defaults.ENIFirstInterfaceIndex, ENISubnetIDs: []string{}, ENISubnetTags: map[string]string{}, @@ -63,6 +70,15 @@ type config struct { IPAMMaxAllocate int IPAMStaticIPTags map[string]string + // CiliumNodeUpdateMaxRetries is the number of attempts the agent makes to + // create or update its own CiliumNode resource against the API server + // before giving up and exiting. A higher value lets the agent tolerate a + // longer control-plane / API server outage without crash-looping. + CiliumNodeUpdateMaxRetries int + // CiliumNodeUpdateRetryBackoff is the backoff between CiliumNode create or + // update retries against the API server. + CiliumNodeUpdateRetryBackoff time.Duration + ENIFirstInterfaceIndex int ENISubnetIDs []string ENISubnetTags map[string]string @@ -87,6 +103,9 @@ func (c config) Flags(flags *pflag.FlagSet) { flags.Int("ipam-max-allocate", c.IPAMMaxAllocate, "Maximum number of IPs that can be allocated at the node level") flags.StringToString("ipam-static-ip-tags", c.IPAMStaticIPTags, "List of tags to determine the pool of IPs from which to attribute a static IP to the node at the node level, this currently works with AWS and Azure") + flags.Int("cilium-node-update-max-retries", c.CiliumNodeUpdateMaxRetries, "Number of attempts to create or update the CiliumNode resource against the API server before the agent gives up and exits. Increase to tolerate longer control-plane / API server outages without crash-looping") + flags.Duration("cilium-node-update-retry-backoff", c.CiliumNodeUpdateRetryBackoff, "Backoff duration between CiliumNode create/update retries against the API server") + flags.Int("eni-first-interface-index", c.ENIFirstInterfaceIndex, "Index of the first ENI to use for IP allocation at the node level") flags.StringToString("eni-exclude-interface-tags", c.ENIExcludeInterfaceTags, "List of tags to use when excluding ENIs for Cilium IP allocation at the node level") flags.StringSlice("eni-subnet-ids", c.ENISubnetIDs, "List of subnet ids to use when evaluating what AWS subnets to use for ENI and IP allocation at the node level") diff --git a/pkg/nodediscovery/nodediscovery.go b/pkg/nodediscovery/nodediscovery.go index bf8243836ae2a..127a5af25222a 100644 --- a/pkg/nodediscovery/nodediscovery.go +++ b/pkg/nodediscovery/nodediscovery.go @@ -227,10 +227,22 @@ func (n *NodeDiscovery) updateCiliumNodeResource(ctx context.Context, ln *node.L logfields.Node, nodeTypes.GetName(), ) + // Retry count and backoff are configurable so the agent can tolerate a + // longer control-plane / API server outage before giving up and exiting. + // Fall back to the package defaults if unset (e.g. zero-valued config). + maxRetries := n.config.CiliumNodeUpdateMaxRetries + if maxRetries <= 0 { + maxRetries = maxRetryCount + } + retryBackoff := n.config.CiliumNodeUpdateRetryBackoff + if retryBackoff <= 0 { + retryBackoff = backoffDuration + } + performGet := true var nodeResource *ciliumv2.CiliumNode var lastErr error - for retryCount := range maxRetryCount { + for retryCount := range maxRetries { performUpdate := true if performGet { var err error @@ -271,7 +283,7 @@ func (n *NodeDiscovery) updateCiliumNodeResource(ctx context.Context, ln *node.L lastErr = err n.logger.Info("Unable to update CiliumNode resource, will retry", logfields.Error, err) // Backoff before retrying - time.Sleep(backoffDuration) + time.Sleep(retryBackoff) continue } else { return @@ -281,7 +293,7 @@ func (n *NodeDiscovery) updateCiliumNodeResource(ctx context.Context, ln *node.L lastErr = err n.logger.Info("Unable to create CiliumNode resource, will retry", logfields.Error, err) // Backoff before retrying - time.Sleep(backoffDuration) + time.Sleep(retryBackoff) continue } else { n.logger.Info("Successfully created CiliumNode resource") @@ -289,7 +301,7 @@ func (n *NodeDiscovery) updateCiliumNodeResource(ctx context.Context, ln *node.L } } } - logging.Fatal(n.logger, "Could not create or update CiliumNode resource", logfields.Error, lastErr, logfields.Retries, maxRetryCount) + logging.Fatal(n.logger, "Could not create or update CiliumNode resource", logfields.Error, lastErr, logfields.Retries, maxRetries) } func (n *NodeDiscovery) mutateNodeResource(ctx context.Context, nodeResource *ciliumv2.CiliumNode, ln *node.LocalNode) error { diff --git a/pkg/nodediscovery/nodediscovery_test.go b/pkg/nodediscovery/nodediscovery_test.go index 5176477d82584..a35ff07a951e3 100644 --- a/pkg/nodediscovery/nodediscovery_test.go +++ b/pkg/nodediscovery/nodediscovery_test.go @@ -21,6 +21,7 @@ import ( "github.com/cilium/cilium/pkg/node" nodeTypes "github.com/cilium/cilium/pkg/node/types" "github.com/cilium/cilium/pkg/option" + "github.com/cilium/cilium/pkg/time" cnitypes "github.com/cilium/cilium/plugins/cilium-cni/types" ) @@ -97,3 +98,66 @@ func TestUpdateCiliumNodeResourceTransientErrorCausesFatal(t *testing.T) { require.Equal(t, maxRetryCount, updateCalls, "transient errors should be retried %d times, not fataled immediately", maxRetryCount) } + +// TestUpdateCiliumNodeResourceConfigurableRetries verifies that the number of +// CiliumNode update attempts before the agent gives up and exits is driven by +// the configurable CiliumNodeUpdateMaxRetries, so operators can tune how long +// the agent tolerates an API server outage instead of being fixed at the +// compiled-in default. +func TestUpdateCiliumNodeResourceConfigurableRetries(t *testing.T) { + option.Config.AutoCreateCiliumNodeResource = true + option.Config.IPAM = "" + t.Cleanup(func() { + option.Config.AutoCreateCiliumNodeResource = false + }) + + const nodeName = "test-node" + nodeTypes.SetName(nodeName) + + // Cover values both below and above the default (maxRetryCount == 10) to + // show the retry budget is honored in either direction. + for _, maxRetries := range []int{1, 3, 15} { + t.Run(fmt.Sprintf("maxRetries=%d", maxRetries), func(t *testing.T) { + logging.RegisterExitHandler(func() { panic("fatal called") }) + t.Cleanup(func() { logging.RegisterExitHandler(func() {}) }) + + fakeClient, _ := clienttestutils.NewFakeClientset(hivetest.Logger(t)) + + existingNode := &ciliumv2.CiliumNode{ + ObjectMeta: metav1.ObjectMeta{Name: nodeName}, + } + require.NoError(t, fakeClient.CiliumFakeClientset.Tracker().Add(existingNode)) + + updateCalls := 0 + fakeClient.CiliumFakeClientset.PrependReactor("update", "ciliumnodes", + func(_ k8stesting.Action) (bool, runtime.Object, error) { + updateCalls++ + return true, nil, fmt.Errorf("connection reset by peer") + }, + ) + + nd := &NodeDiscovery{ + logger: hivetest.Logger(t), + clientset: fakeClient, + k8sGetters: &mockK8sGetters{ciliumNode: existingNode}, + cniConfigManager: &mockCNIConfigManager{}, + config: config{ + CiliumNodeUpdateMaxRetries: maxRetries, + CiliumNodeUpdateRetryBackoff: time.Millisecond, + }, + } + + ln := &node.LocalNode{ + Node: nodeTypes.Node{Name: nodeName}, + Local: &node.LocalNodeInfo{}, + } + + require.Panics(t, func() { + nd.updateCiliumNodeResource(context.Background(), ln) + }) + + require.Equal(t, maxRetries, updateCalls, + "update should be retried exactly CiliumNodeUpdateMaxRetries (%d) times before exiting", maxRetries) + }) + } +}