diff --git a/CHANGELOG.md b/CHANGELOG.md index 3e186792f..785647197 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,7 @@ ## unreleased +* Use `doks.digitalocean.com/node-network-type` to distinguish public vs private-only nodes when retrying LB sync (CON-14209) (@pyadagiri-do) + ## v0.1.69 (beta) - August 25, 2026 * Retry LB sync while nodes are still cloud-provider initializing (@pyadagiri-do) diff --git a/cloud-controller-manager/do/loadbalancers.go b/cloud-controller-manager/do/loadbalancers.go index 513a5528e..7847028eb 100644 --- a/cloud-controller-manager/do/loadbalancers.go +++ b/cloud-controller-manager/do/loadbalancers.go @@ -60,6 +60,15 @@ const ( tagPrefixClusterID = "k8s" ) +const ( + // nodeNetworkTypeLabel declares droplet networking intent. DOKS sets this + // from IsolatedWorkers; self-managed clusters should set it for private-only + // workers. Missing label falls back to public. + nodeNetworkTypeLabel = "doks.digitalocean.com/node-network-type" + nodeNetworkTypePublic = "public" + nodeNetworkTypePrivate = "private" +) + var ( // ErrNetworkStackConfig indicates an error with network stack configuration // that requires user intervention (e.g., mismatched dual-stack settings) @@ -70,18 +79,11 @@ var ( errNoEligibleBackendsMsg = "REGIONAL_NETWORK EXTERNAL load balancers require at least one node with a public IPv4 address; no eligible backends found. For private-only nodes, set service.beta.kubernetes.io/do-loadbalancer-type to \"REGIONAL\" or service.beta.kubernetes.io/do-loadbalancer-network to \"INTERNAL\"." - // nodesNotYetLBReadyRetryAfter is the base retry delay while nodes are still - // cloud-provider-initializing. Kept short so scale-up races heal quickly. + // nodesNotYetLBReadyRetryAfter is the retry delay while nodes that intend a + // public network are still missing a usable ExternalIP. + // Retries continue until the address appears nodesNotYetLBReadyRetryAfter = 5 * time.Second - // nodeInitTaintedGrace bounds how long a node carrying the cloud-provider - // uninitialized taint is treated as still initializing rather than stuck. - nodeInitTaintedGrace = 10 * time.Minute - - // nodeInitAddressGrace covers the gap after the taint is removed but before - // Status.Addresses is patched; those are two separate writes. - nodeInitAddressGrace = 60 * time.Second - // Sticky sessions types. stickySessionsTypeNone = "none" stickySessionsTypeCookies = "cookies" @@ -118,8 +120,6 @@ type loadBalancers struct { lbActiveTimeout int lbActiveCheckTick int defaultLBType string - // now overrides the clock in tests; nil means time.Now. - now func() time.Time } type servicePatcher struct { @@ -153,13 +153,6 @@ func (sp *servicePatcher) Patch(ctx context.Context, err error) error { return utilerrors.NewAggregate([]error{err, perr}) } -func (l *loadBalancers) clock() time.Time { - if l.now != nil { - return l.now() - } - return time.Now() -} - // newLoadbalancers returns a cloudprovider.LoadBalancer whose concrete type is a *loadbalancer. func newLoadBalancers(resources *resources, region string, defaultLBType string) cloudprovider.LoadBalancer { return &loadBalancers{ @@ -691,62 +684,27 @@ func filterAndClassifyNodes(nodes []*v1.Node, lbType, lbNetwork string) *nodeSta return state } -// isCloudProviderUninitialized reports whether the node still carries the -// external cloud-provider uninitialized taint, meaning a missing ExternalIP is -// transient rather than a permanent private-only configuration. -func isCloudProviderUninitialized(node *v1.Node) bool { - for _, taint := range node.Spec.Taints { - if taint.Key == api.TaintExternalCloudProvider { - return true - } - } - return false +// nodeIntendsPublicNetwork reports whether the node is expected to get a public +// IPv4. Private is only trusted when explicitly labeled; a missing, empty, or +// unrecognized value falls back to public so older / unlabeled clusters keep +// retrying on scale-up races. +func nodeIntendsPublicNetwork(node *v1.Node) bool { + return !strings.EqualFold(node.Labels[nodeNetworkTypeLabel], nodeNetworkTypePrivate) } -// pendingInitNodes splits publicNetUnready nodes into those that may still be -// completing cloud-provider initialization and those past their grace period. -// -// Expired nodes are permanently ineligible: a stuck or genuinely private-only -// node must not keep the retry loop alive forever. -func pendingInitNodes(state *nodeState, now time.Time) (pending, expired []*v1.Node) { - for _, node := range state.publicNetUnreadyNodes { - age := now.Sub(node.CreationTimestamp.Time) - grace := nodeInitAddressGrace - if isCloudProviderUninitialized(node) { - grace = nodeInitTaintedGrace - } - if age < grace { +// pendingInitNodes splits nodes with no usable public address by declared +// network intent: those still expected to gain one (retry) and private-only +// nodes that never will (no retry). Both groups are already excluded from the +// backend set by filterAndClassifyNodes. +func pendingInitNodes(unready []*v1.Node) (pending, privateOnly []*v1.Node) { + for _, node := range unready { + if nodeIntendsPublicNetwork(node) { pending = append(pending, node) } else { - expired = append(expired, node) - } - } - return pending, expired -} - -// retryAfterFor slows the retry cadence as nodes stay unready, bounding the -// load balancer writes a stuck node can cause. The youngest pending node sets -// the pace so one stuck node cannot throttle a node that is merely racing. -func retryAfterFor(pending []*v1.Node, now time.Time) time.Duration { - if len(pending) == 0 { - return nodesNotYetLBReadyRetryAfter - } - - youngest := now.Sub(pending[0].CreationTimestamp.Time) - for _, node := range pending[1:] { - if age := now.Sub(node.CreationTimestamp.Time); age < youngest { - youngest = age + privateOnly = append(privateOnly, node) } } - - switch { - case youngest < 30*time.Second: - return nodesNotYetLBReadyRetryAfter - case youngest < 2*time.Minute: - return 15 * time.Second - default: - return 60 * time.Second - } + return pending, privateOnly } // classifyServiceNodes resolves LB type/network and classifies the given nodes. @@ -771,22 +729,22 @@ func (l *loadBalancers) prepareNodesForLBSync(service *v1.Service, nodes []*v1.N return nil, err } - pending, expired := pendingInitNodes(nodeState, l.clock()) - if len(expired) > 0 { - klog.Infof( - "service %s/%s: permanently omitting %d node(s) with no usable address after init grace period: %s", - service.Namespace, service.Name, len(expired), formatNodeNames(expired, 5), + pending, privateOnly := pendingInitNodes(nodeState.publicNetUnreadyNodes) + if len(privateOnly) > 0 { + klog.V(2).Infof( + "service %s/%s: permanently omitting %d private-network node(s) with no usable public address: %s", + service.Namespace, service.Name, len(privateOnly), formatNodeNames(privateOnly, 5), ) } - // Never write an empty backend set while nodes are still coming up. + // Never write an empty backend set while public-intent nodes are still coming up. if len(nodeState.lbReadyNodes) == 0 && len(pending) > 0 { msg := fmt.Sprintf( "service %s/%s: %d node(s) still initializing and not yet lb-ready (e.g. missing ExternalIP); retrying instead of updating backends with an empty node set: %s", service.Namespace, service.Name, len(pending), formatNodeNames(pending, 5), ) klog.Info(msg) - return nil, api.NewRetryError(msg, retryAfterFor(pending, l.clock())) + return nil, api.NewRetryError(msg, nodesNotYetLBReadyRetryAfter) } return pending, nil @@ -804,7 +762,7 @@ func (l *loadBalancers) retryIfNodesPendingInit(service *v1.Service, pending []* service.Namespace, service.Name, len(pending), formatNodeNames(pending, 5), ) klog.Info(msg) - return api.NewRetryError(msg, retryAfterFor(pending, l.clock())) + return api.NewRetryError(msg, nodesNotYetLBReadyRetryAfter) } // classifyNode determines node's IP stack classification. diff --git a/cloud-controller-manager/do/loadbalancers_test.go b/cloud-controller-manager/do/loadbalancers_test.go index 52e6c8fb6..769068286 100644 --- a/cloud-controller-manager/do/loadbalancers_test.go +++ b/cloud-controller-manager/do/loadbalancers_test.go @@ -266,9 +266,12 @@ func withCloudProviderUninitializedTaint(node *v1.Node) *v1.Node { return node } -// withNodeCreationTime sets CreationTimestamp for node-init grace / retry tests. -func withNodeCreationTime(node *v1.Node, t time.Time) *v1.Node { - node.CreationTimestamp = metav1.NewTime(t) +// withNodeNetworkType sets doks.digitalocean.com/node-network-type. +func withNodeNetworkType(node *v1.Node, networkType string) *v1.Node { + if node.Labels == nil { + node.Labels = map[string]string{} + } + node.Labels[nodeNetworkTypeLabel] = networkType return node } @@ -5906,8 +5909,6 @@ func Test_EnsureLoadBalancer(t *testing.T) { } func TestEnsureUpdateLoadBalancer_PendingInitNodes(t *testing.T) { - now := time.Date(2026, 8, 13, 12, 0, 0, 0, time.UTC) - regionalNetworkSvc := func() *v1.Service { return &v1.Service{ ObjectMeta: metav1.ObjectMeta{ @@ -5955,13 +5956,13 @@ func TestEnsureUpdateLoadBalancer_PendingInitNodes(t *testing.T) { wantDropletIDs []int }{ { - name: "updates ready backends then retries for initializing node", + name: "updates ready backends then retries for public-intent initializing node", nodes: []*v1.Node{ - withNodeCreationTime(newNodeWithIPs("node-ready-a", "10.0.0.1", "", true), now.Add(-time.Hour)), - withNodeCreationTime(newNodeWithIPs("node-ready-b", "10.0.0.2", "", true), now.Add(-time.Hour)), - withNodeCreationTime( + withNodeNetworkType(newNodeWithIPs("node-ready-a", "10.0.0.1", "", true), nodeNetworkTypePublic), + withNodeNetworkType(newNodeWithIPs("node-ready-b", "10.0.0.2", "", true), nodeNetworkTypePublic), + withNodeNetworkType( withCloudProviderUninitializedTaint(newNodeWithInternalIPOnly("node-new", "192.168.1.99", true)), - now.Add(-5*time.Second), + nodeNetworkTypePublic, ), }, wantRetry: true, @@ -5970,46 +5971,46 @@ func TestEnsureUpdateLoadBalancer_PendingInitNodes(t *testing.T) { wantDropletIDs: []int{100, 101}, }, { - name: "retries before update when no ready backends yet", + name: "missing network-type label falls back to public and retries", nodes: []*v1.Node{ - withNodeCreationTime( - withCloudProviderUninitializedTaint(newNodeWithInternalIPOnly("node-new", "192.168.1.99", true)), - now.Add(-5*time.Second), - ), + newNodeWithIPs("node-ready-a", "10.0.0.1", "", true), + withCloudProviderUninitializedTaint(newNodeWithInternalIPOnly("node-new", "192.168.1.99", true)), }, wantRetry: true, wantRetryAfter: nodesNotYetLBReadyRetryAfter, - wantUpdate: false, + wantUpdate: true, + wantDropletIDs: []int{100}, }, { - name: "expired initializing node is permanently omitted", + name: "retries before update when no ready backends yet", nodes: []*v1.Node{ - withNodeCreationTime(newNodeWithIPs("node-ready-a", "10.0.0.1", "", true), now.Add(-time.Hour)), - withNodeCreationTime( + withNodeNetworkType( withCloudProviderUninitializedTaint(newNodeWithInternalIPOnly("node-new", "192.168.1.99", true)), - now.Add(-(nodeInitTaintedGrace + time.Minute)), + nodeNetworkTypePublic, ), }, - wantRetry: false, - wantUpdate: true, - wantDropletIDs: []int{100}, + wantRetry: true, + wantRetryAfter: nodesNotYetLBReadyRetryAfter, + wantUpdate: false, }, { - name: "untainted young publicNetUnready node is pending via address grace", + name: "private-labeled node is permanently omitted without retry", nodes: []*v1.Node{ - withNodeCreationTime(newNodeWithIPs("node-ready-a", "10.0.0.1", "", true), now.Add(-time.Hour)), - withNodeCreationTime(newNodeWithInternalIPOnly("node-new", "192.168.1.99", true), now.Add(-10*time.Second)), + withNodeNetworkType(newNodeWithIPs("node-ready-a", "10.0.0.1", "", true), nodeNetworkTypePublic), + withNodeNetworkType(newNodeWithInternalIPOnly("node-private", "192.168.1.1", true), nodeNetworkTypePrivate), }, - wantRetry: true, - wantRetryAfter: nodesNotYetLBReadyRetryAfter, + wantRetry: false, wantUpdate: true, wantDropletIDs: []int{100}, }, { - name: "permanent private-only node does not retry", + name: "private label wins over uninitialized taint", nodes: []*v1.Node{ - withNodeCreationTime(newNodeWithIPs("node-ready-a", "10.0.0.1", "", true), now.Add(-time.Hour)), - withNodeCreationTime(newNodeWithInternalIPOnly("node-private", "192.168.1.1", true), now.Add(-(nodeInitAddressGrace + time.Minute))), + withNodeNetworkType(newNodeWithIPs("node-ready-a", "10.0.0.1", "", true), nodeNetworkTypePublic), + withNodeNetworkType( + withCloudProviderUninitializedTaint(newNodeWithInternalIPOnly("node-private", "192.168.1.99", true)), + nodeNetworkTypePrivate, + ), }, wantRetry: false, wantUpdate: true, @@ -6061,7 +6062,6 @@ func TestEnsureUpdateLoadBalancer_PendingInitNodes(t *testing.T) { lbActiveTimeout: 2, lbActiveCheckTick: 1, defaultLBType: godo.LoadBalancerTypeRegionalNetwork, - now: func() time.Time { return now }, } _, err := lb.EnsureLoadBalancer(context.TODO(), "test", svc, test.nodes) @@ -6131,7 +6131,6 @@ func TestEnsureUpdateLoadBalancer_PendingInitNodes(t *testing.T) { lbActiveTimeout: 2, lbActiveCheckTick: 1, defaultLBType: godo.LoadBalancerTypeRegionalNetwork, - now: func() time.Time { return now }, } err := lb.UpdateLoadBalancer(context.TODO(), "test", svc, test.nodes) @@ -6159,13 +6158,11 @@ func TestEnsureUpdateLoadBalancer_PendingInitNodes(t *testing.T) { } } -// Node-init retry and expiry exist only because EXTERNAL REGIONAL_NETWORK load -// balancers need a public address per backend. INTERNAL and classic REGIONAL -// load balancers accept private-only backends, so an initializing node must go +// Node-init retry exists only because EXTERNAL REGIONAL_NETWORK load balancers +// need a public address per backend. INTERNAL and classic REGIONAL load +// balancers accept private-only backends, so an initializing node must go // straight into the backend set and never hold their sync back. func TestUpdateLoadBalancer_NodeInitScopedToPublicBackends(t *testing.T) { - now := time.Date(2026, 8, 13, 12, 0, 0, 0, time.UTC) - droplets := []godo.Droplet{ {ID: 100, Name: "node-ready"}, {ID: 102, Name: "node-new"}, @@ -6173,10 +6170,10 @@ func TestUpdateLoadBalancer_NodeInitScopedToPublicBackends(t *testing.T) { nodes := func() []*v1.Node { return []*v1.Node{ - withNodeCreationTime(newNodeWithIPs("node-ready", "10.0.0.1", "", true), now.Add(-time.Hour)), - withNodeCreationTime( + withNodeNetworkType(newNodeWithIPs("node-ready", "10.0.0.1", "", true), nodeNetworkTypePublic), + withNodeNetworkType( withCloudProviderUninitializedTaint(newNodeWithInternalIPOnly("node-new", "192.168.1.99", true)), - now.Add(-5*time.Second), + nodeNetworkTypePublic, ), } } @@ -6272,7 +6269,6 @@ func TestUpdateLoadBalancer_NodeInitScopedToPublicBackends(t *testing.T) { lbActiveTimeout: 2, lbActiveCheckTick: 1, defaultLBType: godo.LoadBalancerTypeRegionalNetwork, - now: func() time.Time { return now }, } err := lb.UpdateLoadBalancer(context.TODO(), "test", svc, nodes()) @@ -6292,63 +6288,47 @@ func TestUpdateLoadBalancer_NodeInitScopedToPublicBackends(t *testing.T) { } func TestPendingInitNodes(t *testing.T) { - now := time.Date(2026, 8, 13, 12, 0, 0, 0, time.UTC) - state := &nodeState{ - publicNetUnreadyNodes: []*v1.Node{ - withNodeCreationTime( - withCloudProviderUninitializedTaint(newNodeWithInternalIPOnly("tainted-young", "10.0.0.1", true)), - now.Add(-time.Minute), - ), - withNodeCreationTime( - withCloudProviderUninitializedTaint(newNodeWithInternalIPOnly("tainted-old", "10.0.0.2", true)), - now.Add(-(nodeInitTaintedGrace + time.Minute)), - ), - withNodeCreationTime(newNodeWithInternalIPOnly("untainted-young", "10.0.0.3", true), now.Add(-10*time.Second)), - withNodeCreationTime(newNodeWithInternalIPOnly("untainted-old", "10.0.0.4", true), now.Add(-(nodeInitAddressGrace + time.Second))), - }, - } - - pending, expired := pendingInitNodes(state, now) - if got, want := nodeNames(pending), []string{"tainted-young", "untainted-young"}; !reflect.DeepEqual(got, want) { + unready := []*v1.Node{ + withNodeNetworkType(newNodeWithInternalIPOnly("public-labeled", "10.0.0.1", true), nodeNetworkTypePublic), + withNodeNetworkType(newNodeWithInternalIPOnly("private-labeled", "10.0.0.2", true), nodeNetworkTypePrivate), + newNodeWithInternalIPOnly("unlabeled", "10.0.0.3", true), + withNodeNetworkType( + withCloudProviderUninitializedTaint(newNodeWithInternalIPOnly("private-with-taint", "10.0.0.4", true)), + nodeNetworkTypePrivate, + ), + withNodeNetworkType(newNodeWithInternalIPOnly("private-mixed-case", "10.0.0.5", true), "Private"), + withNodeNetworkType(newNodeWithInternalIPOnly("unrecognized-value", "10.0.0.6", true), "bogus"), + } + + pending, privateOnly := pendingInitNodes(unready) + if got, want := nodeNames(pending), []string{"public-labeled", "unlabeled", "unrecognized-value"}; !reflect.DeepEqual(got, want) { t.Errorf("pending: got %v, want %v", got, want) } - if got, want := nodeNames(expired), []string{"tainted-old", "untainted-old"}; !reflect.DeepEqual(got, want) { - t.Errorf("expired: got %v, want %v", got, want) + want := []string{"private-labeled", "private-with-taint", "private-mixed-case"} + if got := nodeNames(privateOnly); !reflect.DeepEqual(got, want) { + t.Errorf("privateOnly: got %v, want %v", got, want) } } -func TestRetryAfterFor(t *testing.T) { - now := time.Date(2026, 8, 13, 12, 0, 0, 0, time.UTC) +func TestNodeIntendsPublicNetwork(t *testing.T) { tests := []struct { name string - ages []time.Duration - want time.Duration + node *v1.Node + want bool }{ - {name: "no pending nodes", want: nodesNotYetLBReadyRetryAfter}, - {name: "fresh", ages: []time.Duration{5 * time.Second}, want: nodesNotYetLBReadyRetryAfter}, - {name: "warming", ages: []time.Duration{time.Minute}, want: 15 * time.Second}, - {name: "slow", ages: []time.Duration{5 * time.Minute}, want: 60 * time.Second}, - { - name: "youngest node drives cadence so a stuck node cannot throttle a racing one", - ages: []time.Duration{5 * time.Minute, 3 * time.Second}, - want: nodesNotYetLBReadyRetryAfter, - }, - { - name: "backs off once every pending node is old", - ages: []time.Duration{5 * time.Minute, 3 * time.Minute}, - want: 60 * time.Second, - }, + {name: "nil labels", node: &v1.Node{}, want: true}, + {name: "missing key", node: &v1.Node{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{}}}, want: true}, + {name: "empty value", node: withNodeNetworkType(&v1.Node{}, ""), want: true}, + {name: "public", node: withNodeNetworkType(&v1.Node{}, nodeNetworkTypePublic), want: true}, + {name: "private", node: withNodeNetworkType(&v1.Node{}, nodeNetworkTypePrivate), want: false}, + {name: "private mixed case", node: withNodeNetworkType(&v1.Node{}, "PrIvAtE"), want: false}, + // Unrecognized values fail open to public so a typo keeps the retry + // path alive rather than silently dropping a node from the LB. + {name: "unrecognized value", node: withNodeNetworkType(&v1.Node{}, "bogus"), want: true}, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { - var pending []*v1.Node - for i, age := range test.ages { - name := fmt.Sprintf("node-%d", i) - pending = append(pending, withNodeCreationTime( - newNodeWithInternalIPOnly(name, "10.0.0.1", true), now.Add(-age), - )) - } - if got := retryAfterFor(pending, now); got != test.want { + if got := nodeIntendsPublicNetwork(test.node); got != test.want { t.Errorf("got %v, want %v", got, test.want) } }) @@ -7964,7 +7944,7 @@ func TestBuildLoadBalancerRequest_EventEmission(t *testing.T) { }, nodes: []*v1.Node{ newNodeWithIPs("node-ready", "10.0.0.1", "", true), - newNodeWithInternalIPOnly("node-private", "192.168.1.1", true), + withNodeNetworkType(newNodeWithInternalIPOnly("node-private", "192.168.1.1", true), nodeNetworkTypePrivate), }, expectEvent: false, expectedEventReason: "", diff --git a/docs/getting-started.md b/docs/getting-started.md index 94be4664b..b1bc290ea 100644 --- a/docs/getting-started.md +++ b/docs/getting-started.md @@ -86,6 +86,8 @@ On all-private clusters, external-facing Services must use `do-loadbalancer-type Mixed clusters: a `REGIONAL_NETWORK` + `EXTERNAL` Service registers only nodes that have a public IPv4 address as backends; private-only nodes remain schedulable but are excluded from those Services. +Label private-only workers with `doks.digitalocean.com/node-network-type=private` (DOKS sets this automatically for IsolatedWorkers). Values are `public` or `private`. If the label is missing, CCM assumes `public` and will retry LB sync while a node has no ExternalIP yet. Setting `private` is required for private-only workers so CCM does not keep retrying them forever. + ### Kubernetes nodes can be reached via IP address only When setting the droplet host name as the node name (which is the default), Kubernetes will try to reach the node using its host name. However, this won't work since host names aren't resovable on DO. For example, when you run `kubectl logs` you will get an error like so: