Skip to content

Commit

Permalink
Merge pull request kubernetes#116174 from pacoxu/fix-TestNewNodeIpamC…
Browse files Browse the repository at this point in the history 
…ontrollerWithCIDRMasks

node ipam controller ut: run test in parallel to avoid timeout
  • Loading branch information
@k8s-ci-robot
k8s-ci-robot authored Mar 8, 2023
2 parents e390791 + 0e6636e commit f99c351
Show file tree
Hide file tree
Showing 4 changed files with 93 additions and 58 deletions.
11 changes: 5 additions & 6 deletions pkg/controller/nodeipam/legacyprovider.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ limitations under the License.
package nodeipam

import (
"fmt"
"net"

coreinformers "k8s.io/client-go/informers/core/v1"
Expand All @@ -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,
Expand All @@ -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")
klog.FlushAndExit(klog.ExitFlushTimeout, 1)
return nil, fmt.Errorf("error creating ipam controller: %w", err)
}
if err := ipamc.Start(logger, nodeInformer); err != nil {
logger.Error(err, "Error trying to Init()")
klog.FlushAndExit(klog.ExitFlushTimeout, 1)
return nil, fmt.Errorf("error trying to Init(): %w", err)
}
return ipamc
return ipamc, nil
}
16 changes: 9 additions & 7 deletions pkg/controller/nodeipam/node_ipam_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package nodeipam

import (
"context"
"fmt"
"net"
"time"

Expand Down Expand Up @@ -93,22 +94,19 @@ func NewNodeIpamController(

logger := klog.FromContext(ctx)
if kubeClient == nil {
logger.Error(nil, "kubeClient is nil when starting Controller")
klog.FlushAndExit(klog.ExitFlushTimeout, 1)
return nil, fmt.Errorf("kubeClient is nil when starting Controller")
}

// 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")
klog.FlushAndExit(klog.ExitFlushTimeout, 1)
return nil, fmt.Errorf("Controller: Must specify --cluster-cidr if --allocate-node-cidrs is set")
}

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")
klog.FlushAndExit(klog.ExitFlushTimeout, 1)
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")
}
}
}
Expand All @@ -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

Expand Down
117 changes: 76 additions & 41 deletions pkg/controller/nodeipam/node_ipam_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand All @@ -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()

Expand All @@ -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_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::/10", "10.1.0.0/21", "3000::/10", []int{24, 98}, ipam.RangeAllocatorType, false},

{"valid_multi_cidr_range_allocator", "10.0.0.0/21", "10.1.0.0/21", emptyServiceCIDR, []int{24}, ipam.MultiCIDRRangeAllocatorType, false},
{"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_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_dualstackservice", "10.0.0.0/21,2000::/48", "10.1.0.0/21", "3000::/112", []int{24, 64}, ipam.RangeAllocatorType, nil},
{"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_ipam_from_cloud", "10.0.0.0/21", "10.1.0.0/21", emptyServiceCIDR, []int{24}, ipam.IPAMFromCloudAllocatorType, nil},
{"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},
{"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")},
{"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")},
{"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")},
{"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")},
} {
test := tc
_, ctx := ktesting.NewTestContext(t)
t.Run(test.desc, func(t *testing.T) {
t.Parallel()
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 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)
}
}
})
}
}

{"valid_cloud_allocator", "10.0.0.0/21", "10.1.0.0/21", emptyServiceCIDR, []int{24}, ipam.CloudAllocatorType, false},
{"valid_ipam_from_cluster", "10.0.0.0/21", "10.1.0.0/21", emptyServiceCIDR, []int{24}, ipam.IPAMFromClusterAllocatorType, false},
{"valid_ipam_from_cloud", "10.0.0.0/21", "10.1.0.0/21", emptyServiceCIDR, []int{24}, ipam.IPAMFromCloudAllocatorType, false},
{"valid_skip_cluster_CIDR_validation_for_cloud_allocator", "invalid", "10.1.0.0/21", emptyServiceCIDR, []int{24}, ipam.CloudAllocatorType, false},
{"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},
// 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(tc.desc, func(t *testing.T) {
clusterCidrs, _ := netutils.ParseCIDRs(strings.Split(tc.clusterCIDR, ","))
_, serviceCIDRIpNet, _ := netutils.ParseCIDRSloppy(tc.serviceCIDR)
_, secondaryServiceCIDRIpNet, _ := netutils.ParseCIDRSloppy(tc.secondaryServiceCIDR)
t.Run(test.desc, func(t *testing.T) {
defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.MultiCIDRRangeAllocator, true)()

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
clusterCidrs, err := netutils.ParseCIDRs(strings.Split(test.clusterCIDR, ","))
if err != nil {
clusterCidrs = nil
}
// 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
_, serviceCIDRIpNet, err := netutils.ParseCIDRSloppy(test.serviceCIDR)
if err != nil {
exitErr, ok := err.(*exec.ExitError)
if !ok {
t.Fatalf("Failed to run subprocess: %v", err)
}
gotFatal = !exitErr.Success()
serviceCIDRIpNet = nil
}
if gotFatal != tc.wantFatal {
t.Errorf("newTestNodeIpamController(%v, %v, %v, %v) : gotFatal = %t ; wantFatal = %t", clusterCidrs, serviceCIDRIpNet, tc.maskSize, tc.allocatorType, gotFatal, tc.wantFatal)
_, 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)
}
})
}
Expand Down
7 changes: 3 additions & 4 deletions pkg/controller/nodeipam/nolegacyprovider.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ package nodeipam

import (
"context"
"errors"
"net"

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

0 comments on commit f99c351

Please sign in to comment.