Merge pull request #116174 from pacoxu/fix-TestNewNodeIpamControllerWithCIDRMasks

node ipam controller ut: run test in parallel to avoid timeout

pkg/controller/nodeipam/legacyprovider.go

@@ -20,6 +20,7 @@ limitations under the License.
 package nodeipam
 
 import (
+	"fmt"
 	"net"
 
 	coreinformers "k8s.io/client-go/informers/core/v1"
@@ -40,7 +41,7 @@ func createLegacyIPAM(
 	clusterCIDRs []*net.IPNet,
 	serviceCIDR *net.IPNet,
 	nodeCIDRMaskSizes []int,
-) *ipam.Controller {
+) (*ipam.Controller, error) {
 	cfg := &ipam.Config{
 		Resync:       ipamResyncInterval,
 		MaxBackoff:   ipamMaxBackoff,
@@ -63,12 +64,10 @@ func createLegacyIPAM(
 	}
 	ipamc, err := ipam.NewController(cfg, kubeClient, cloud, cidr, serviceCIDR, nodeCIDRMaskSizes[0])
 	if err != nil {
-		logger.Error(err, "Error creating ipam controller")
+		return nil, fmt.Errorf("error creating ipam controller: %w", err)
-		klog.FlushAndExit(klog.ExitFlushTimeout, 1)
 	}
 	if err := ipamc.Start(logger, nodeInformer); err != nil {
-		logger.Error(err, "Error trying to Init()")
+		return nil, fmt.Errorf("error trying to Init(): %w", err)
-		klog.FlushAndExit(klog.ExitFlushTimeout, 1)
 	}
-	return ipamc
+	return ipamc, nil
 }

pkg/controller/nodeipam/node_ipam_controller.go

@@ -18,6 +18,7 @@ package nodeipam
 
 import (
 	"context"
+	"fmt"
 	"net"
 	"time"
 
@@ -93,22 +94,19 @@ func NewNodeIpamController(
 
 	logger := klog.FromContext(ctx)
 	if kubeClient == nil {
-		logger.Error(nil, "kubeClient is nil when starting Controller")
+		return nil, fmt.Errorf("kubeClient is nil when starting Controller")
-		klog.FlushAndExit(klog.ExitFlushTimeout, 1)
 	}
 
 	// Cloud CIDR allocator does not rely on clusterCIDR or nodeCIDRMaskSize for allocation.
 	if allocatorType != ipam.CloudAllocatorType {
 		if len(clusterCIDRs) == 0 {
-			logger.Error(nil, "Controller: Must specify --cluster-cidr if --allocate-node-cidrs is set")
+			return nil, fmt.Errorf("Controller: Must specify --cluster-cidr if --allocate-node-cidrs is set")
-			klog.FlushAndExit(klog.ExitFlushTimeout, 1)
 		}
 
 		for idx, cidr := range clusterCIDRs {
 			mask := cidr.Mask
 			if maskSize, _ := mask.Size(); maskSize > nodeCIDRMaskSizes[idx] {
-				logger.Error(nil, "Controller: Invalid --cluster-cidr, mask size of cluster CIDR must be less than or equal to --node-cidr-mask-size configured for CIDR family")
+				return nil, fmt.Errorf("Controller: Invalid --cluster-cidr, mask size of cluster CIDR must be less than or equal to --node-cidr-mask-size configured for CIDR family")
-				klog.FlushAndExit(klog.ExitFlushTimeout, 1)
 			}
 		}
 	}
@@ -126,7 +124,11 @@ func NewNodeIpamController(
 
 	// TODO: Abstract this check into a generic controller manager should run method.
 	if ic.allocatorType == ipam.IPAMFromClusterAllocatorType || ic.allocatorType == ipam.IPAMFromCloudAllocatorType {
-		ic.legacyIPAM = createLegacyIPAM(logger, ic, nodeInformer, cloud, kubeClient, clusterCIDRs, serviceCIDR, nodeCIDRMaskSizes)
+		var err error
+		ic.legacyIPAM, err = createLegacyIPAM(logger, ic, nodeInformer, cloud, kubeClient, clusterCIDRs, serviceCIDR, nodeCIDRMaskSizes)
+		if err != nil {
+			return nil, err
+		}
 	} else {
 		var err error
 

pkg/controller/nodeipam/node_ipam_controller_test.go

@@ -21,20 +21,21 @@ package nodeipam
 
 import (
 	"context"
+	"errors"
 	"net"
-	"os"
-	"os/exec"
 	"strings"
 	"testing"
 
 	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	utilfeature "k8s.io/apiserver/pkg/util/feature"
 	"k8s.io/client-go/informers"
 	"k8s.io/client-go/kubernetes/fake"
+	featuregatetesting "k8s.io/component-base/featuregate/testing"
 	"k8s.io/klog/v2/ktesting"
-	"k8s.io/kubernetes/pkg/controller"
 	"k8s.io/kubernetes/pkg/controller/nodeipam/ipam"
 	"k8s.io/kubernetes/pkg/controller/testutil"
+	"k8s.io/kubernetes/pkg/features"
 	"k8s.io/legacy-cloud-providers/gce"
 	netutils "k8s.io/utils/net"
 )
@@ -48,7 +49,7 @@ func newTestNodeIpamController(ctx context.Context, clusterCIDR []*net.IPNet, se
 		Clientset: fake.NewSimpleClientset(),
 	}
 	fakeClient := &fake.Clientset{}
-	fakeInformerFactory := informers.NewSharedInformerFactory(fakeClient, controller.NoResyncPeriodFunc())
+	fakeInformerFactory := informers.NewSharedInformerFactory(fakeClient, 0)
 	fakeNodeInformer := fakeInformerFactory.Core().V1().Nodes()
 	fakeClusterCIDRInformer := fakeInformerFactory.Networking().V1alpha1().ClusterCIDRs()
 
@@ -75,51 +76,85 @@ func TestNewNodeIpamControllerWithCIDRMasks(t *testing.T) {
 		secondaryServiceCIDR string
 		maskSize             []int
 		allocatorType        ipam.CIDRAllocatorType
-		wantFatal            bool
+		expectedError        error
 	}{
-		{"valid_range_allocator", "10.0.0.0/21", "10.1.0.0/21", emptyServiceCIDR, []int{24}, ipam.RangeAllocatorType, false},
+		{"valid_range_allocator", "10.0.0.0/21", "10.1.0.0/21", emptyServiceCIDR, []int{24}, ipam.RangeAllocatorType, nil},
-
+		{"valid_range_allocator_dualstack", "10.0.0.0/21,2000::/48", "10.1.0.0/21", emptyServiceCIDR, []int{24, 64}, ipam.RangeAllocatorType, nil},
-		{"valid_range_allocator_dualstack", "10.0.0.0/21,2000::/10", "10.1.0.0/21", emptyServiceCIDR, []int{24, 98}, ipam.RangeAllocatorType, false},
+		{"valid_range_allocator_dualstack_dualstackservice", "10.0.0.0/21,2000::/48", "10.1.0.0/21", "3000::/112", []int{24, 64}, ipam.RangeAllocatorType, nil},
-		{"valid_range_allocator_dualstack_dualstackservice", "10.0.0.0/21,2000::/10", "10.1.0.0/21", "3000::/10", []int{24, 98}, ipam.RangeAllocatorType, false},
+		{"valid_cloud_allocator", "10.0.0.0/21", "10.1.0.0/21", emptyServiceCIDR, []int{24}, ipam.CloudAllocatorType, nil},
-
+		{"valid_ipam_from_cluster", "10.0.0.0/21", "10.1.0.0/21", emptyServiceCIDR, []int{24}, ipam.IPAMFromClusterAllocatorType, nil},
-		{"valid_multi_cidr_range_allocator", "10.0.0.0/21", "10.1.0.0/21", emptyServiceCIDR, []int{24}, ipam.MultiCIDRRangeAllocatorType, false},
+		{"valid_ipam_from_cloud", "10.0.0.0/21", "10.1.0.0/21", emptyServiceCIDR, []int{24}, ipam.IPAMFromCloudAllocatorType, nil},
-		{"valid_multi_cidr_range_allocator_dualstack", "10.0.0.0/21,2000::/10", "10.1.0.0/21", emptyServiceCIDR, []int{24, 98}, ipam.MultiCIDRRangeAllocatorType, false},
+		{"valid_skip_cluster_CIDR_validation_for_cloud_allocator", "invalid", "10.1.0.0/21", emptyServiceCIDR, []int{24}, ipam.CloudAllocatorType, nil},
-
+		{"valid_CIDR_larger_than_mask_cloud_allocator", "10.0.0.0/16", "10.1.0.0/21", emptyServiceCIDR, []int{24}, ipam.CloudAllocatorType, nil},
-		{"valid_cloud_allocator", "10.0.0.0/21", "10.1.0.0/21", emptyServiceCIDR, []int{24}, ipam.CloudAllocatorType, false},
+		{"invalid_cluster_CIDR", "", "10.1.0.0/21", emptyServiceCIDR, []int{24}, ipam.IPAMFromClusterAllocatorType, errors.New("Controller: Must specify --cluster-cidr if --allocate-node-cidrs is set")},
-		{"valid_ipam_from_cluster", "10.0.0.0/21", "10.1.0.0/21", emptyServiceCIDR, []int{24}, ipam.IPAMFromClusterAllocatorType, false},
+		{"invalid_CIDR_smaller_than_mask_other_allocators", "10.0.0.0/26", "10.1.0.0/21", emptyServiceCIDR, []int{24}, ipam.IPAMFromCloudAllocatorType, errors.New("Controller: Invalid --cluster-cidr, mask size of cluster CIDR must be less than or equal to --node-cidr-mask-size configured for CIDR family")},
-		{"valid_ipam_from_cloud", "10.0.0.0/21", "10.1.0.0/21", emptyServiceCIDR, []int{24}, ipam.IPAMFromCloudAllocatorType, false},
+		{"invalid_serviceCIDR_contains_clusterCIDR", "10.0.0.0/16", "10.0.0.0/8", emptyServiceCIDR, []int{24}, ipam.IPAMFromClusterAllocatorType, errors.New("error creating ipam controller: failed after occupy serviceCIDR: CIDR allocation failed; there are no remaining CIDRs left to allocate in the accepted range")},
-		{"valid_skip_cluster_CIDR_validation_for_cloud_allocator", "invalid", "10.1.0.0/21", emptyServiceCIDR, []int{24}, ipam.CloudAllocatorType, false},
+		{"invalid_CIDR_mask_size", "10.0.0.0/24,2000::/64", "10.1.0.0/21", emptyServiceCIDR, []int{24, 48}, ipam.IPAMFromClusterAllocatorType, errors.New("Controller: Invalid --cluster-cidr, mask size of cluster CIDR must be less than or equal to --node-cidr-mask-size configured for CIDR family")},
-		{"invalid_cluster_CIDR", "invalid", "10.1.0.0/21", emptyServiceCIDR, []int{24}, ipam.IPAMFromClusterAllocatorType, true},
-		{"valid_CIDR_smaller_than_mask_cloud_allocator", "10.0.0.0/26", "10.1.0.0/21", emptyServiceCIDR, []int{24}, ipam.CloudAllocatorType, false},
-		{"invalid_CIDR_smaller_than_mask_other_allocators", "10.0.0.0/26", "10.1.0.0/21", emptyServiceCIDR, []int{24}, ipam.IPAMFromCloudAllocatorType, true},
-		{"invalid_serviceCIDR_contains_clusterCIDR", "10.0.0.0/23", "10.0.0.0/21", emptyServiceCIDR, []int{24}, ipam.IPAMFromClusterAllocatorType, true},
-		{"invalid_CIDR_mask_size", "10.0.0.0/24,2000::/64", "10.1.0.0/21", emptyServiceCIDR, []int{24, 48}, ipam.IPAMFromClusterAllocatorType, true},
 	} {
+		test := tc
 		_, ctx := ktesting.NewTestContext(t)
-		t.Run(tc.desc, func(t *testing.T) {
+		t.Run(test.desc, func(t *testing.T) {
-			clusterCidrs, _ := netutils.ParseCIDRs(strings.Split(tc.clusterCIDR, ","))
+			t.Parallel()
-			_, serviceCIDRIpNet, _ := netutils.ParseCIDRSloppy(tc.serviceCIDR)
+			clusterCidrs, err := netutils.ParseCIDRs(strings.Split(test.clusterCIDR, ","))
-			_, secondaryServiceCIDRIpNet, _ := netutils.ParseCIDRSloppy(tc.secondaryServiceCIDR)
-
-			if os.Getenv("EXIT_ON_FATAL") == "1" {
-				// This is the subprocess which runs the actual code.
-				newTestNodeIpamController(ctx, clusterCidrs, serviceCIDRIpNet, secondaryServiceCIDRIpNet, tc.maskSize, tc.allocatorType)
-				return
-			}
-			// This is the host process that monitors the exit code of the subprocess.
-			cmd := exec.Command(os.Args[0], "-test.run=TestNewNodeIpamControllerWithCIDRMasks/"+tc.desc)
-			cmd.Env = append(os.Environ(), "EXIT_ON_FATAL=1")
-			err := cmd.Run()
-			var gotFatal bool
 			if err != nil {
-				exitErr, ok := err.(*exec.ExitError)
+				clusterCidrs = nil
-				if !ok {
-					t.Fatalf("Failed to run subprocess: %v", err)
-				}
-				gotFatal = !exitErr.Success()
 			}
-			if gotFatal != tc.wantFatal {
+			_, serviceCIDRIpNet, err := netutils.ParseCIDRSloppy(test.serviceCIDR)
-				t.Errorf("newTestNodeIpamController(%v, %v, %v, %v) : gotFatal = %t ; wantFatal = %t", clusterCidrs, serviceCIDRIpNet, tc.maskSize, tc.allocatorType, gotFatal, tc.wantFatal)
+			if err != nil {
+				serviceCIDRIpNet = nil
+			}
+			_, secondaryServiceCIDRIpNet, err := netutils.ParseCIDRSloppy(test.secondaryServiceCIDR)
+			if err != nil {
+				secondaryServiceCIDRIpNet = nil
+			}
+			_, err = newTestNodeIpamController(ctx, clusterCidrs, serviceCIDRIpNet, secondaryServiceCIDRIpNet, test.maskSize, test.allocatorType)
+			if test.expectedError == nil {
+				if err != nil {
+					t.Errorf("Test %s, unexpected error: %v", test.desc, err)
+				}
+			} else {
+				if err.Error() != test.expectedError.Error() {
+					t.Errorf("Test %s, got error: %v, expected error: %v", test.desc, err, test.expectedError)
+				}
+			}
+		})
+	}
+}
+
+// MultiCIDRRangeAllocatorType need enable feature gate
+func TestNewNodeIpamControllerWithCIDRMasks2(t *testing.T) {
+	emptyServiceCIDR := ""
+	for _, tc := range []struct {
+		desc                 string
+		clusterCIDR          string
+		serviceCIDR          string
+		secondaryServiceCIDR string
+		maskSize             []int
+		allocatorType        ipam.CIDRAllocatorType
+	}{
+		{"valid_multi_cidr_range_allocator", "10.0.0.0/21", "10.1.0.0/21", emptyServiceCIDR, []int{24}, ipam.MultiCIDRRangeAllocatorType},
+		{"valid_multi_cidr_range_allocator_dualstack", "10.0.0.0/21,2000::/48", "10.1.0.0/21", emptyServiceCIDR, []int{24, 64}, ipam.MultiCIDRRangeAllocatorType},
+	} {
+		test := tc
+		_, ctx := ktesting.NewTestContext(t)
+		t.Run(test.desc, func(t *testing.T) {
+			defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.MultiCIDRRangeAllocator, true)()
+
+			clusterCidrs, err := netutils.ParseCIDRs(strings.Split(test.clusterCIDR, ","))
+			if err != nil {
+				clusterCidrs = nil
+			}
+			_, serviceCIDRIpNet, err := netutils.ParseCIDRSloppy(test.serviceCIDR)
+			if err != nil {
+				serviceCIDRIpNet = nil
+			}
+			_, secondaryServiceCIDRIpNet, err := netutils.ParseCIDRSloppy(test.secondaryServiceCIDR)
+			if err != nil {
+				secondaryServiceCIDRIpNet = nil
+			}
+			_, err = newTestNodeIpamController(ctx, clusterCidrs, serviceCIDRIpNet, secondaryServiceCIDRIpNet, test.maskSize, test.allocatorType)
+			if err != nil {
+				t.Errorf("Test %s, got error %v", test.desc, err)
 			}
 		})
 	}

pkg/controller/nodeipam/nolegacyprovider.go

@@ -21,6 +21,7 @@ package nodeipam
 
 import (
 	"context"
+	"errors"
 	"net"
 
 	coreinformers "k8s.io/client-go/informers/core/v1"
@@ -45,8 +46,6 @@ func createLegacyIPAM(
 	clusterCIDRs []*net.IPNet,
 	serviceCIDR *net.IPNet,
 	nodeCIDRMaskSizes []int,
-) *fakeController {
+) (*fakeController, error) {
-	logger.Error(nil, "Error trying to Init(): legacy cloud provider support disabled at build time")
+	return nil, errors.New("Error trying to Init(): legacy cloud provider support disabled at build time")
-	klog.FlushAndExit(klog.ExitFlushTimeout, 1)
-	return &fakeController{}
 }