Merge pull request #96417 from hvenev-vmware/fix-ipam

Fix double counting of IP addresses
This commit is contained in:
Kubernetes Prow Robot 2020-11-23 07:45:33 -08:00 committed by GitHub
commit 248c116963
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 167 additions and 12 deletions

View File

@ -232,16 +232,20 @@ func (s *CidrSet) Release(cidr *net.IPNet) error {
s.Lock() s.Lock()
defer s.Unlock() defer s.Unlock()
for i := begin; i <= end; i++ { for i := begin; i <= end; i++ {
// Only change the counters if we change the bit to prevent
// double counting.
if s.used.Bit(i) != 0 {
s.used.SetBit(&s.used, i, 0) s.used.SetBit(&s.used, i, 0)
s.allocatedCIDRs-- s.allocatedCIDRs--
cidrSetReleases.WithLabelValues(s.label).Inc() cidrSetReleases.WithLabelValues(s.label).Inc()
} }
}
cidrSetUsage.WithLabelValues(s.label).Set(float64(s.allocatedCIDRs) / float64(s.maxCIDRs)) cidrSetUsage.WithLabelValues(s.label).Set(float64(s.allocatedCIDRs) / float64(s.maxCIDRs))
return nil return nil
} }
// Occupy marks the given CIDR range as used. Occupy does not check if the CIDR // Occupy marks the given CIDR range as used. Occupy succeeds even if the CIDR
// range was previously used. // range was previously used.
func (s *CidrSet) Occupy(cidr *net.IPNet) (err error) { func (s *CidrSet) Occupy(cidr *net.IPNet) (err error) {
begin, end, err := s.getBeginingAndEndIndices(cidr) begin, end, err := s.getBeginingAndEndIndices(cidr)
@ -251,10 +255,14 @@ func (s *CidrSet) Occupy(cidr *net.IPNet) (err error) {
s.Lock() s.Lock()
defer s.Unlock() defer s.Unlock()
for i := begin; i <= end; i++ { for i := begin; i <= end; i++ {
// Only change the counters if we change the bit to prevent
// double counting.
if s.used.Bit(i) == 0 {
s.used.SetBit(&s.used, i, 1) s.used.SetBit(&s.used, i, 1)
s.allocatedCIDRs++ s.allocatedCIDRs++
cidrSetAllocations.WithLabelValues(s.label).Inc() cidrSetAllocations.WithLabelValues(s.label).Inc()
} }
}
cidrSetUsage.WithLabelValues(s.label).Set(float64(s.allocatedCIDRs) / float64(s.maxCIDRs)) cidrSetUsage.WithLabelValues(s.label).Set(float64(s.allocatedCIDRs) / float64(s.maxCIDRs))
return nil return nil

View File

@ -316,6 +316,8 @@ func TestCIDRSet_AllocationOccupied(t *testing.T) {
for i := numCIDRs / 2; i < numCIDRs; i++ { for i := numCIDRs / 2; i < numCIDRs; i++ {
a.Occupy(cidrs[i]) a.Occupy(cidrs[i])
} }
// occupy the first of the last 128 again
a.Occupy(cidrs[numCIDRs/2])
// allocate the first 128 CIDRs again // allocate the first 128 CIDRs again
var rcidrs []*net.IPNet var rcidrs []*net.IPNet
@ -341,6 +343,98 @@ func TestCIDRSet_AllocationOccupied(t *testing.T) {
} }
} }
func TestDoubleOccupyRelease(t *testing.T) {
// Run a sequence of operations and check the number of occupied CIDRs
// after each one.
clusterCIDRStr := "10.42.0.0/16"
operations := []struct {
cidrStr string
operation string
numOccupied int
}{
// Occupy 1 element: +1
{
cidrStr: "10.42.5.0/24",
operation: "occupy",
numOccupied: 1,
},
// Occupy 1 more element: +1
{
cidrStr: "10.42.9.0/24",
operation: "occupy",
numOccupied: 2,
},
// Occupy 4 elements overlapping with one from the above: +3
{
cidrStr: "10.42.8.0/22",
operation: "occupy",
numOccupied: 5,
},
// Occupy an already-coccupied element: no change
{
cidrStr: "10.42.9.0/24",
operation: "occupy",
numOccupied: 5,
},
// Release an coccupied element: -1
{
cidrStr: "10.42.9.0/24",
operation: "release",
numOccupied: 4,
},
// Release an unoccupied element: no change
{
cidrStr: "10.42.9.0/24",
operation: "release",
numOccupied: 4,
},
// Release 4 elements, only one of which is occupied: -1
{
cidrStr: "10.42.4.0/22",
operation: "release",
numOccupied: 3,
},
}
// Check that there are exactly that many allocatable CIDRs after all
// operations have been executed.
numAllocatable24s := (1 << 8) - 3
_, clusterCIDR, _ := net.ParseCIDR(clusterCIDRStr)
a, err := NewCIDRSet(clusterCIDR, 24)
if err != nil {
t.Fatalf("Error allocating CIDRSet")
}
// Execute the operations
for _, op := range operations {
_, cidr, _ := net.ParseCIDR(op.cidrStr)
switch op.operation {
case "occupy":
a.Occupy(cidr)
case "release":
a.Release(cidr)
default:
t.Fatalf("test error: unknown operation %v", op.operation)
}
if a.allocatedCIDRs != op.numOccupied {
t.Fatalf("Expected %d occupied CIDRS, got %d", op.numOccupied, a.allocatedCIDRs)
}
}
// Make sure that we can allocate exactly `numAllocatable24s` elements.
for i := 0; i < numAllocatable24s; i++ {
_, err := a.AllocateNext()
if err != nil {
t.Fatalf("Expected to be able to allocate %d CIDRS, failed after %d", numAllocatable24s, i)
}
}
_, err = a.AllocateNext()
if err == nil {
t.Fatalf("Expected to be able to allocate exactly %d CIDRS, got one more", numAllocatable24s)
}
}
func TestGetBitforCIDR(t *testing.T) { func TestGetBitforCIDR(t *testing.T) {
cases := []struct { cases := []struct {
clusterCIDRStr string clusterCIDRStr string

View File

@ -486,12 +486,55 @@ func TestAllocateOrOccupyCIDRSuccess(t *testing.T) {
NodeCIDRMaskSizes: []int{24, 98, 24}, NodeCIDRMaskSizes: []int{24, 98, 24},
}, },
}, },
{
description: "no double counting",
fakeNodeHandler: &testutil.FakeNodeHandler{
Existing: []*v1.Node{
{
ObjectMeta: metav1.ObjectMeta{
Name: "node0",
},
Spec: v1.NodeSpec{
PodCIDRs: []string{"10.10.0.0/24"},
},
},
{
ObjectMeta: metav1.ObjectMeta{
Name: "node1",
},
Spec: v1.NodeSpec{
PodCIDRs: []string{"10.10.2.0/24"},
},
},
{
ObjectMeta: metav1.ObjectMeta{
Name: "node2",
},
},
},
Clientset: fake.NewSimpleClientset(),
},
allocatorParams: CIDRAllocatorParams{
ClusterCIDRs: func() []*net.IPNet {
_, clusterCIDR, _ := net.ParseCIDR("10.10.0.0/22")
return []*net.IPNet{clusterCIDR}
}(),
ServiceCIDR: nil,
SecondaryServiceCIDR: nil,
NodeCIDRMaskSizes: []int{24},
},
expectedAllocatedCIDR: map[int]string{
0: "10.10.1.0/24",
},
},
} }
// test function // test function
testFunc := func(tc testCase) { testFunc := func(tc testCase) {
fakeNodeInformer := getFakeNodeInformer(tc.fakeNodeHandler)
nodeList, _ := tc.fakeNodeHandler.List(context.TODO(), metav1.ListOptions{})
// Initialize the range allocator. // Initialize the range allocator.
allocator, err := NewCIDRRangeAllocator(tc.fakeNodeHandler, getFakeNodeInformer(tc.fakeNodeHandler), tc.allocatorParams, nil) allocator, err := NewCIDRRangeAllocator(tc.fakeNodeHandler, fakeNodeInformer, tc.allocatorParams, nodeList)
if err != nil { if err != nil {
t.Errorf("%v: failed to create CIDRRangeAllocator with error %v", tc.description, err) t.Errorf("%v: failed to create CIDRRangeAllocator with error %v", tc.description, err)
return return
@ -517,12 +560,22 @@ func TestAllocateOrOccupyCIDRSuccess(t *testing.T) {
t.Fatalf("%v: unexpected error when occupying CIDR %v: %v", tc.description, allocated, err) t.Fatalf("%v: unexpected error when occupying CIDR %v: %v", tc.description, allocated, err)
} }
} }
if err := allocator.AllocateOrOccupyCIDR(tc.fakeNodeHandler.Existing[0]); err != nil { }
updateCount := 0
for _, node := range tc.fakeNodeHandler.Existing {
if node.Spec.PodCIDRs == nil {
updateCount++
}
if err := allocator.AllocateOrOccupyCIDR(node); err != nil {
t.Errorf("%v: unexpected error in AllocateOrOccupyCIDR: %v", tc.description, err) t.Errorf("%v: unexpected error in AllocateOrOccupyCIDR: %v", tc.description, err)
} }
if err := waitForUpdatedNodeWithTimeout(tc.fakeNodeHandler, 1, wait.ForeverTestTimeout); err != nil {
t.Fatalf("%v: timeout while waiting for Node update: %v", tc.description, err)
} }
if updateCount != 1 {
t.Fatalf("test error: all tests must update exactly one node")
}
if err := waitForUpdatedNodeWithTimeout(tc.fakeNodeHandler, updateCount, wait.ForeverTestTimeout); err != nil {
t.Fatalf("%v: timeout while waiting for Node update: %v", tc.description, err)
} }
if len(tc.expectedAllocatedCIDR) == 0 { if len(tc.expectedAllocatedCIDR) == 0 {