From ce491bf9542757a35cf629c10a61370684ae41b7 Mon Sep 17 00:00:00 2001 From: pyadagiri Date: Fri, 4 Sep 2026 11:01:36 +0530 Subject: [PATCH 1/2] Use node-network-type label for LB retry eligibility --- CHANGELOG.md | 2 + cloud-controller-manager/do/loadbalancers.go | 106 ++++--------- .../do/loadbalancers_test.go | 141 +++++++----------- docs/getting-started.md | 2 + 4 files changed, 95 insertions(+), 156 deletions(-) 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..0882f3559 100644 --- a/cloud-controller-manager/do/loadbalancers.go +++ b/cloud-controller-manager/do/loadbalancers.go @@ -70,17 +70,17 @@ 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 + // 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" // Sticky sessions types. stickySessionsTypeNone = "none" @@ -118,8 +118,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 +151,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 +682,31 @@ 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 - } +// nodeIntendsPublicNetwork reports whether the node is expected to get a public +// IPv4. Private is only trusted when explicitly labeled; missing label falls +// back to public so older / unlabeled clusters keep retrying on scale-up races. +func nodeIntendsPublicNetwork(node *v1.Node) bool { + if node.Labels == nil { + return true } - return false + v, ok := node.Labels[nodeNetworkTypeLabel] + if !ok || v == "" { + return true + } + return v != 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) { +// pendingInitNodes splits publicNetUnready nodes into those still expected to +// gain a public address (retry) and permanently private-only nodes (omit). +func pendingInitNodes(state *nodeState) (pending, permanent []*v1.Node) { for _, node := range state.publicNetUnreadyNodes { - age := now.Sub(node.CreationTimestamp.Time) - grace := nodeInitAddressGrace - if isCloudProviderUninitialized(node) { - grace = nodeInitTaintedGrace - } - if age < grace { + if nodeIntendsPublicNetwork(node) { pending = append(pending, node) } else { - expired = append(expired, node) + permanent = append(permanent, 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 - } - } - - switch { - case youngest < 30*time.Second: - return nodesNotYetLBReadyRetryAfter - case youngest < 2*time.Minute: - return 15 * time.Second - default: - return 60 * time.Second - } + return pending, permanent } // classifyServiceNodes resolves LB type/network and classifies the given nodes. @@ -771,22 +731,22 @@ func (l *loadBalancers) prepareNodesForLBSync(service *v1.Service, nodes []*v1.N return nil, err } - pending, expired := pendingInitNodes(nodeState, l.clock()) - if len(expired) > 0 { + pending, permanent := pendingInitNodes(nodeState) + if len(permanent) > 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), + "service %s/%s: permanently omitting %d private-network node(s) with no usable public address: %s", + service.Namespace, service.Name, len(permanent), formatNodeNames(permanent, 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 +764,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..7ce3ad1a4 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,42 @@ 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)), + 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, ), - 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) { + pending, permanent := pendingInitNodes(state) + if got, want := nodeNames(pending), []string{"public-labeled", "unlabeled"}; !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) + if got, want := nodeNames(permanent), []string{"private-labeled", "private-with-taint"}; !reflect.DeepEqual(got, want) { + t.Errorf("permanent: 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}, } 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 +7939,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: From b38e9f8fd1fd85c23c95a0687fa4dbb003147c3e Mon Sep 17 00:00:00 2001 From: pyadagiri Date: Fri, 4 Sep 2026 11:42:48 +0530 Subject: [PATCH 2/2] Simplify node-network-type label check and clarify naming --- cloud-controller-manager/do/loadbalancers.go | 52 +++++++++---------- .../do/loadbalancers_test.go | 37 +++++++------ 2 files changed, 46 insertions(+), 43 deletions(-) diff --git a/cloud-controller-manager/do/loadbalancers.go b/cloud-controller-manager/do/loadbalancers.go index 0882f3559..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) @@ -75,13 +84,6 @@ var ( // Retries continue until the address appears nodesNotYetLBReadyRetryAfter = 5 * time.Second - // 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" - // Sticky sessions types. stickySessionsTypeNone = "none" stickySessionsTypeCookies = "cookies" @@ -683,30 +685,26 @@ func filterAndClassifyNodes(nodes []*v1.Node, lbType, lbNetwork string) *nodeSta } // nodeIntendsPublicNetwork reports whether the node is expected to get a public -// IPv4. Private is only trusted when explicitly labeled; missing label falls -// back to public so older / unlabeled clusters keep retrying on scale-up races. +// 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 { - if node.Labels == nil { - return true - } - v, ok := node.Labels[nodeNetworkTypeLabel] - if !ok || v == "" { - return true - } - return v != nodeNetworkTypePrivate + return !strings.EqualFold(node.Labels[nodeNetworkTypeLabel], nodeNetworkTypePrivate) } -// pendingInitNodes splits publicNetUnready nodes into those still expected to -// gain a public address (retry) and permanently private-only nodes (omit). -func pendingInitNodes(state *nodeState) (pending, permanent []*v1.Node) { - for _, node := range state.publicNetUnreadyNodes { +// 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 { - permanent = append(permanent, node) + privateOnly = append(privateOnly, node) } } - return pending, permanent + return pending, privateOnly } // classifyServiceNodes resolves LB type/network and classifies the given nodes. @@ -731,11 +729,11 @@ func (l *loadBalancers) prepareNodesForLBSync(service *v1.Service, nodes []*v1.N return nil, err } - pending, permanent := pendingInitNodes(nodeState) - if len(permanent) > 0 { - klog.Infof( + 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(permanent), formatNodeNames(permanent, 5), + service.Namespace, service.Name, len(privateOnly), formatNodeNames(privateOnly, 5), ) } diff --git a/cloud-controller-manager/do/loadbalancers_test.go b/cloud-controller-manager/do/loadbalancers_test.go index 7ce3ad1a4..769068286 100644 --- a/cloud-controller-manager/do/loadbalancers_test.go +++ b/cloud-controller-manager/do/loadbalancers_test.go @@ -6288,24 +6288,25 @@ func TestUpdateLoadBalancer_NodeInitScopedToPublicBackends(t *testing.T) { } func TestPendingInitNodes(t *testing.T) { - state := &nodeState{ - publicNetUnreadyNodes: []*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, - ), - }, - } - - pending, permanent := pendingInitNodes(state) - if got, want := nodeNames(pending), []string{"public-labeled", "unlabeled"}; !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(permanent), []string{"private-labeled", "private-with-taint"}; !reflect.DeepEqual(got, want) { - t.Errorf("permanent: 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) } } @@ -6320,6 +6321,10 @@ func TestNodeIntendsPublicNetwork(t *testing.T) { {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) {