Skip to content

Commit 5315c6c

Browse files
committed
ipam/multipool: Fix TestOrphanCIDRsAfterRestart
In TestOrphanCIDRsAfterRestart we upsert different revisions of the node to test how the node handler reacts to the presence of orphan CIDRs after an operator restart. Since the node handler uses a controller internally, we have to be a bit more careful while asserting the expected results in order to avoid races. Therefore: - copy the node before updating Spec.IPAM.Pools.Requested - rely on assert.Eventually to wait for the controller to run - assert that the controller runs as expected waiting on the CiliumNode status update using stress, the multipool package tests run successfully for over 5000 runs with the race detector enabled: ``` go test -race -c ./pkg/ipam/allocator/multipool/... stress ./multipool.test -test.shuffle=on ... 4m15s: 5326 runs so far, 0 failures, 24 active ``` Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>
1 parent 483ba24 commit 5315c6c

1 file changed

Lines changed: 43 additions & 24 deletions

File tree

pkg/ipam/allocator/multipool/node_handler_test.go

Lines changed: 43 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -221,11 +221,20 @@ func TestOrphanCIDRsAfterRestart(t *testing.T) {
221221
backend := NewPoolAllocator(hivetest.Logger(t))
222222

223223
onUpdateArgs := make(chan mockArgs)
224+
225+
onUpdateStatusArgs := make(chan mockArgs)
226+
onUpdateStatusResult := make(chan mockResult)
227+
224228
nodeUpdater := &k8sNodeMock{
225229
OnUpdate: func(oldNode, newNode *v2.CiliumNode) (*v2.CiliumNode, error) {
226230
onUpdateArgs <- mockArgs{oldNode, newNode}
227231
return nil, nil
228232
},
233+
OnUpdateStatus: func(oldNode, newNode *v2.CiliumNode) (*v2.CiliumNode, error) {
234+
onUpdateStatusArgs <- mockArgs{oldNode, newNode}
235+
r := <-onUpdateStatusResult
236+
return r.node, r.err
237+
},
229238
}
230239
nh := NewNodeHandler(hivetest.Logger(t), backend, nodeUpdater)
231240

@@ -260,59 +269,69 @@ func TestOrphanCIDRsAfterRestart(t *testing.T) {
260269
}
261270
nh.Upsert(node)
262271

272+
// CIDRs allocated from previous operator run should eventually be marked as orphans
273+
assert.EventuallyWithT(t, func(c *assert.CollectT) {
274+
backend.mutex.Lock()
275+
defer backend.mutex.Unlock()
276+
277+
assert.Equal(c, map[string]poolToCIDRs{
278+
node.Name: {
279+
"test-pool": {
280+
v4: cidrSet{
281+
netip.MustParsePrefix("10.0.0.0/24"): {},
282+
netip.MustParsePrefix("10.0.1.0/24"): {},
283+
netip.MustParsePrefix("10.0.2.0/24"): {},
284+
},
285+
v6: cidrSet{},
286+
},
287+
},
288+
}, backend.orphans)
289+
}, 5*time.Second, 50*time.Millisecond)
290+
263291
// Node should only be updated after Resync
264292
select {
265293
case <-onUpdateArgs:
266294
t.Fatal("Update should not have been called before Resync")
267295
default:
268296
}
269297

270-
// CIDRs allocated from previous operator run should be marked as orphans
271-
assert.Equal(t, map[string]poolToCIDRs{
272-
node.Name: {
273-
"test-pool": {
274-
v4: cidrSet{
275-
netip.MustParsePrefix("10.0.0.0/24"): {},
276-
netip.MustParsePrefix("10.0.1.0/24"): {},
277-
netip.MustParsePrefix("10.0.2.0/24"): {},
278-
},
279-
v6: cidrSet{},
280-
},
281-
},
282-
}, backend.orphans)
283-
284298
nh.Resync(t.Context(), time.Time{})
285299

286300
// Node should not be updated, since all allocated CIDRs are orphan
301+
nodeUpdateStatus := <-onUpdateStatusArgs
302+
assert.Equal(t, "node", nodeUpdateStatus.newNode.Name)
303+
assert.Contains(t, nodeUpdateStatus.newNode.Status.IPAM.OperatorStatus.Error, "cannot allocate from non-existing pool: test-pool")
304+
onUpdateStatusResult <- mockResult{node: nodeUpdateStatus.newNode}
305+
287306
select {
288307
case <-onUpdateArgs:
289308
t.Fatal("Update should not have been called after Resync")
290309
default:
291310
}
292311

293312
// Node should not be updated, since we cannot allocate more CIDRs from non existent pools
294-
node.Spec.IPAM.Pools.Requested[0].Needed = ipamTypes.IPAMPoolDemand{IPv4Addrs: 16}
295-
nh.Upsert(node)
313+
node2 := node.DeepCopy()
314+
node2.Spec.IPAM.Pools.Requested[0].Needed = ipamTypes.IPAMPoolDemand{IPv4Addrs: 24}
315+
nh.Upsert(node2)
316+
317+
nodeUpdate2Status := <-onUpdateStatusArgs
318+
assert.Equal(t, "node", nodeUpdate2Status.newNode.Name)
319+
assert.Contains(t, nodeUpdate2Status.newNode.Status.IPAM.OperatorStatus.Error, "cannot allocate from non-existing pool: test-pool")
320+
onUpdateStatusResult <- mockResult{node: nodeUpdate2Status.newNode}
296321

297322
select {
298323
case <-onUpdateArgs:
299324
t.Fatal("Update should not have been called after increasing requested IPs")
300325
default:
301326
}
302327

303-
// Node should not be updated, but the previous CIDRs should be unorphaned if test-pool is restored
328+
// Previous CIDRs should be unorphaned if test-pool is restored
304329
err := backend.UpsertPool("test-pool", []string{"10.0.0.0/16"}, 24, nil, 0)
305330
assert.NoError(t, err)
306331

307-
select {
308-
case <-onUpdateArgs:
309-
t.Fatal("Update should not have been called after increasing requested IPs")
310-
default:
311-
}
312-
313332
assert.Empty(t, backend.orphans)
314333
assert.Equal(t, map[string]poolToCIDRs{
315-
node.Name: {
334+
node2.Name: {
316335
"test-pool": {
317336
v4: cidrSet{
318337
netip.MustParsePrefix("10.0.0.0/24"): {},

0 commit comments

Comments
 (0)