From 6c50b16f98a5e0d6fdc1f5dc5ddedc8a59b947ca Mon Sep 17 00:00:00 2001 From: Antonio Ojea Date: Mon, 5 Aug 2024 09:44:46 +0000 Subject: [PATCH 1/3] don't fail controller during start if ClusterCIDR not available for Nodes Change-Id: I94c72bd257c75c764aaaeb48c1e2b99844724bb4 --- pkg/controller/ipam/multi_cidr_range_allocator.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/controller/ipam/multi_cidr_range_allocator.go b/pkg/controller/ipam/multi_cidr_range_allocator.go index fbbfb26..dfa2d85 100644 --- a/pkg/controller/ipam/multi_cidr_range_allocator.go +++ b/pkg/controller/ipam/multi_cidr_range_allocator.go @@ -26,7 +26,7 @@ import ( "sync" "time" - "sigs.k8s.io/node-ipam-controller/pkg/apis/clustercidr/v1" + v1 "sigs.k8s.io/node-ipam-controller/pkg/apis/clustercidr/v1" clustercidrclient "sigs.k8s.io/node-ipam-controller/pkg/client/clientset/versioned/typed/clustercidr/v1" clustercidrinformers "sigs.k8s.io/node-ipam-controller/pkg/client/informers/externalversions/clustercidr/v1" clustercidrlisters "sigs.k8s.io/node-ipam-controller/pkg/client/listers/clustercidr/v1" @@ -274,9 +274,9 @@ func NewMultiCIDRRangeAllocator( // This will happen if: // 1. We find garbage in the podCIDRs field. Retrying is useless. // 2. CIDR out of range: This means ClusterCIDR is not yet created - // This error will keep crashing controller-manager until the - // appropriate ClusterCIDR has been created - return nil, err + // or the node is not managed by this IPAM controller + // This error will be information only, see https://github.com/kubernetes-sigs/node-ipam-controller/issues/27 + logger.Info("Node CIDR has no associated ClusterCIDR, skipping", "node", klog.KObj(&node), "error", err) } } } From 4e69501e4e693a24078054a7650fe54e050ea891 Mon Sep 17 00:00:00 2001 From: Antonio Ojea Date: Thu, 1 Aug 2024 14:07:50 +0000 Subject: [PATCH 2/3] occupyCIDRs: differentiate between no matching and no available cidrs When trying to allocate a new PodCIDR for a Node it can happen that there is no ClusterCIDR available for the node, so we error and inform the user of the problem so it can create the ClusterCIDR later or decide it leave it as is (migration from legacy node IPAM use case) The other situation that can happen is that all the existing ClusterCIDRs have exhausted the available CIDRs. Change-Id: I34fac8f00243a8e206a7ce49189d76d0f9f0f630 --- pkg/controller/ipam/multi_cidr_range_allocator.go | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/pkg/controller/ipam/multi_cidr_range_allocator.go b/pkg/controller/ipam/multi_cidr_range_allocator.go index dfa2d85..c843caa 100644 --- a/pkg/controller/ipam/multi_cidr_range_allocator.go +++ b/pkg/controller/ipam/multi_cidr_range_allocator.go @@ -530,8 +530,20 @@ func (r *multiCIDRRangeAllocator) occupyCIDRs(logger klog.Logger, node *corev1.N return err } + // There can be clusters with nodes that were handled by a different IPAM controller, in order to allow + // migrations to the new IPAM controller users can create a ClusterCIDR matching their values, but they may + // not want to do that because they just want to get rid of that specific range. In the other hand, users + // may not be aware of this problem and they may want to add a new CIDR, so letting the reconcile loop + // fail and retry a couple of time can provide information to these users. + // https://github.com/kubernetes-sigs/node-ipam-controller/issues/27 + if len(clusterCIDRList) == 0 { + return fmt.Errorf("could not occupy cidrs: %v, No matching ClusterCIDRs found", node.Spec.PodCIDRs) + } + + attempts := 0 for _, clusterCIDR := range clusterCIDRList { occupiedCount := 0 + attempts++ for _, cidr := range node.Spec.PodCIDRs { _, podCIDR, err := netutil.ParseCIDRSloppy(cidr) @@ -556,7 +568,7 @@ func (r *multiCIDRRangeAllocator) occupyCIDRs(logger klog.Logger, node *corev1.N } } - return fmt.Errorf("could not occupy cidrs: %v, No matching ClusterCIDRs found", node.Spec.PodCIDRs) + return fmt.Errorf("could not occupy cidrs: %v after %d attempts", node.Spec.PodCIDRs, attempts) }(node) return err From dff174a5efb310777a71495b68fa65cd767f2112 Mon Sep 17 00:00:00 2001 From: Max Neverov Date: Thu, 1 Aug 2024 19:29:48 +0200 Subject: [PATCH 3/3] Fix tests. --- .../ipam/multi_cidr_range_allocator_test.go | 349 +++++++++--------- 1 file changed, 173 insertions(+), 176 deletions(-) diff --git a/pkg/controller/ipam/multi_cidr_range_allocator_test.go b/pkg/controller/ipam/multi_cidr_range_allocator_test.go index b8941be..e4fc210 100644 --- a/pkg/controller/ipam/multi_cidr_range_allocator_test.go +++ b/pkg/controller/ipam/multi_cidr_range_allocator_test.go @@ -299,181 +299,6 @@ func TestMultiCIDROccupyPreExistingCIDR(t *testing.T) { expectedAllocatedCIDR: nil, ctrlCreateFail: false, }, - // failure cases. - { - description: "fail, single stack incorrect node allocation", - fakeNodeHandler: &test.FakeNodeHandler{ - Existing: []*corev1.Node{ - { - ObjectMeta: metav1.ObjectMeta{ - Name: "node0", - Labels: map[string]string{ - "testLabel-0": "node0", - }, - }, - Spec: corev1.NodeSpec{ - PodCIDRs: []string{"172.10.0.1/24"}, - }, - }, - }, - Clientset: fake.NewSimpleClientset(), - }, - allocatorParams: CIDRAllocatorParams{ - ServiceCIDR: nil, - SecondaryServiceCIDR: nil, - }, - testCIDRMap: getTestCidrMap( - map[string][]*testClusterCIDR{ - getTestNodeSelector([]testNodeSelectorRequirement{ - { - key: "testLabel-0", - operator: corev1.NodeSelectorOpIn, - values: []string{"node0"}, - }, - }): { - { - name: "single-stack-cidr-allocate-fail", - perNodeHostBits: 8, - ipv4CIDR: "10.10.0.0/16", - }, - }, - }), - allocatedCIDRs: nil, - expectedAllocatedCIDR: nil, - ctrlCreateFail: true, - }, - { - description: "fail, dualstack node allocating from non existing cidr", - - fakeNodeHandler: &test.FakeNodeHandler{ - Existing: []*corev1.Node{ - { - ObjectMeta: metav1.ObjectMeta{ - Name: "node0", - Labels: map[string]string{ - "testLabel-0": "node0", - }, - }, - Spec: corev1.NodeSpec{ - PodCIDRs: []string{"10.10.0.1/24", "a00::/86"}, - }, - }, - }, - Clientset: fake.NewSimpleClientset(), - }, - allocatorParams: CIDRAllocatorParams{ - ServiceCIDR: nil, - SecondaryServiceCIDR: nil, - }, - testCIDRMap: getTestCidrMap( - map[string][]*testClusterCIDR{ - getTestNodeSelector([]testNodeSelectorRequirement{ - { - key: "testLabel-0", - operator: corev1.NodeSelectorOpIn, - values: []string{"node0"}, - }, - }): { - { - name: "dual-stack-cidr-allocate-fail", - perNodeHostBits: 8, - ipv4CIDR: "10.10.0.0/16", - ipv6CIDR: "ace:cab:deca::/112", - }, - }, - }), - allocatedCIDRs: nil, - expectedAllocatedCIDR: nil, - ctrlCreateFail: true, - }, - { - description: "fail, dualstack node allocating bad v4", - - fakeNodeHandler: &test.FakeNodeHandler{ - Existing: []*corev1.Node{ - { - ObjectMeta: metav1.ObjectMeta{ - Name: "node0", - Labels: map[string]string{ - "testLabel-0": "node0", - }, - }, - Spec: corev1.NodeSpec{ - PodCIDRs: []string{"172.10.0.1/24", "ace:cab:deca::1/120"}, - }, - }, - }, - Clientset: fake.NewSimpleClientset(), - }, - allocatorParams: CIDRAllocatorParams{ - ServiceCIDR: nil, - SecondaryServiceCIDR: nil, - }, - testCIDRMap: getTestCidrMap( - map[string][]*testClusterCIDR{ - getTestNodeSelector([]testNodeSelectorRequirement{ - { - key: "testLabel-0", - operator: corev1.NodeSelectorOpIn, - values: []string{"node0"}, - }, - }): { - { - name: "dual-stack-cidr-bad-v4", - perNodeHostBits: 8, - ipv4CIDR: "10.10.0.0/16", - ipv6CIDR: "ace:cab:deca::/112", - }, - }, - }), - allocatedCIDRs: nil, - expectedAllocatedCIDR: nil, - ctrlCreateFail: true, - }, - { - description: "fail, dualstack node allocating bad v6", - - fakeNodeHandler: &test.FakeNodeHandler{ - Existing: []*corev1.Node{ - { - ObjectMeta: metav1.ObjectMeta{ - Name: "node0", - Labels: map[string]string{ - "testLabel-0": "node0", - }, - }, - Spec: corev1.NodeSpec{ - PodCIDRs: []string{"10.10.0.1/24", "cdd::/86"}, - }, - }, - }, - Clientset: fake.NewSimpleClientset(), - }, - allocatorParams: CIDRAllocatorParams{ - ServiceCIDR: nil, - SecondaryServiceCIDR: nil, - }, - testCIDRMap: getTestCidrMap( - map[string][]*testClusterCIDR{ - getTestNodeSelector([]testNodeSelectorRequirement{ - { - key: "testLabel-0", - operator: corev1.NodeSelectorOpIn, - values: []string{"node0"}, - }, - }): { - { - name: "dual-stack-cidr-bad-v6", - perNodeHostBits: 8, - ipv4CIDR: "10.10.0.0/16", - ipv6CIDR: "ace:cab:deca::/112", - }, - }, - }), - allocatedCIDRs: nil, - expectedAllocatedCIDR: nil, - ctrlCreateFail: true, - }, } // test function @@ -1322,6 +1147,176 @@ func TestMultiCIDRAllocateOrOccupyCIDRFailure(t *testing.T) { 0: {"127.123.234.0/30", "127.123.234.4/30", "127.123.234.8/30", "127.123.234.12/30"}, }, }, + { + description: "fail, single stack incorrect node allocation", + fakeNodeHandler: &test.FakeNodeHandler{ + Existing: []*corev1.Node{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "node0", + Labels: map[string]string{ + "testLabel-0": "node0", + }, + }, + Spec: corev1.NodeSpec{ + PodCIDRs: []string{"172.10.0.1/24"}, + }, + }, + }, + Clientset: fake.NewSimpleClientset(), + }, + allocatorParams: CIDRAllocatorParams{ + ServiceCIDR: nil, + SecondaryServiceCIDR: nil, + }, + testCIDRMap: getTestCidrMap( + map[string][]*testClusterCIDR{ + getTestNodeSelector([]testNodeSelectorRequirement{ + { + key: "testLabel-0", + operator: corev1.NodeSelectorOpIn, + values: []string{"node0"}, + }, + }): { + { + name: "single-stack-cidr-allocate-fail", + perNodeHostBits: 8, + ipv4CIDR: "10.10.0.0/16", + }, + }, + }), + allocatedCIDRs: nil, + expectedAllocatedCIDR: nil, + }, + { + description: "fail, dualstack node allocating from non existing cidr", + + fakeNodeHandler: &test.FakeNodeHandler{ + Existing: []*corev1.Node{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "node0", + Labels: map[string]string{ + "testLabel-0": "node0", + }, + }, + Spec: corev1.NodeSpec{ + PodCIDRs: []string{"10.10.0.1/24", "a00::/86"}, + }, + }, + }, + Clientset: fake.NewSimpleClientset(), + }, + allocatorParams: CIDRAllocatorParams{ + ServiceCIDR: nil, + SecondaryServiceCIDR: nil, + }, + testCIDRMap: getTestCidrMap( + map[string][]*testClusterCIDR{ + getTestNodeSelector([]testNodeSelectorRequirement{ + { + key: "testLabel-0", + operator: corev1.NodeSelectorOpIn, + values: []string{"node0"}, + }, + }): { + { + name: "dual-stack-cidr-allocate-fail", + perNodeHostBits: 8, + ipv4CIDR: "10.10.0.0/16", + ipv6CIDR: "ace:cab:deca::/112", + }, + }, + }), + allocatedCIDRs: nil, + expectedAllocatedCIDR: nil, + }, + { + description: "fail, dualstack node allocating bad v4", + + fakeNodeHandler: &test.FakeNodeHandler{ + Existing: []*corev1.Node{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "node0", + Labels: map[string]string{ + "testLabel-0": "node0", + }, + }, + Spec: corev1.NodeSpec{ + PodCIDRs: []string{"172.10.0.1/24", "ace:cab:deca::1/120"}, + }, + }, + }, + Clientset: fake.NewSimpleClientset(), + }, + allocatorParams: CIDRAllocatorParams{ + ServiceCIDR: nil, + SecondaryServiceCIDR: nil, + }, + testCIDRMap: getTestCidrMap( + map[string][]*testClusterCIDR{ + getTestNodeSelector([]testNodeSelectorRequirement{ + { + key: "testLabel-0", + operator: corev1.NodeSelectorOpIn, + values: []string{"node0"}, + }, + }): { + { + name: "dual-stack-cidr-bad-v4", + perNodeHostBits: 8, + ipv4CIDR: "10.10.0.0/16", + ipv6CIDR: "ace:cab:deca::/112", + }, + }, + }), + allocatedCIDRs: nil, + expectedAllocatedCIDR: nil, + }, + { + description: "fail, dualstack node allocating bad v6", + + fakeNodeHandler: &test.FakeNodeHandler{ + Existing: []*corev1.Node{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "node0", + Labels: map[string]string{ + "testLabel-0": "node0", + }, + }, + Spec: corev1.NodeSpec{ + PodCIDRs: []string{"10.10.0.1/24", "cdd::/86"}, + }, + }, + }, + Clientset: fake.NewSimpleClientset(), + }, + allocatorParams: CIDRAllocatorParams{ + ServiceCIDR: nil, + SecondaryServiceCIDR: nil, + }, + testCIDRMap: getTestCidrMap( + map[string][]*testClusterCIDR{ + getTestNodeSelector([]testNodeSelectorRequirement{ + { + key: "testLabel-0", + operator: corev1.NodeSelectorOpIn, + values: []string{"node0"}, + }, + }): { + { + name: "dual-stack-cidr-bad-v6", + perNodeHostBits: 8, + ipv4CIDR: "10.10.0.0/16", + ipv6CIDR: "ace:cab:deca::/112", + }, + }, + }), + allocatedCIDRs: nil, + expectedAllocatedCIDR: nil, + }, } logger, ctx := ktesting.NewTestContext(t) @@ -1397,7 +1392,9 @@ func TestMultiCIDRAllocateOrOccupyCIDRFailure(t *testing.T) { } } for _, tc := range testCaseMultiCIDRs { - testFunc(tc) + t.Run(tc.description, func(t *testing.T) { + testFunc(tc) + }) } }