Skip to content

Commit 7b04a36

Browse files
santhoshmprabhusanthoshmprabhu
committed
refactor: break down IPAM reconciliation to address Evan's comment
1 parent e3a7876 commit 7b04a36

File tree

5 files changed

+75
-40
lines changed

5 files changed

+75
-40
lines changed

‎cns/nodesubnet/initialization.go‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,13 +6,12 @@ import (
66
"github.com/Azure/azure-container-networking/cns"
77
"github.com/Azure/azure-container-networking/cns/logger"
88
cnstypes "github.com/Azure/azure-container-networking/cns/types"
9-
"github.com/Azure/azure-container-networking/crd/nodenetworkconfig/api/v1alpha"
109
"github.com/pkg/errors"
1110
"golang.org/x/exp/maps"
1211
)
1312

1413
type ipamReconciler interface {
15-
ReconcileIPAMState(ncRequests []*cns.CreateNetworkContainerRequest, podInfoByIP map[string]cns.PodInfo, nnc *v1alpha.NodeNetworkConfig) cnstypes.ResponseCode
14+
ReconcileIPAMStateForNodeSubnet(ncRequests []*cns.CreateNetworkContainerRequest, podInfoByIP map[string]cns.PodInfo) cnstypes.ResponseCode
1615
}
1716

1817
func ReconcileInitialCNSState(_ context.Context, ipamReconciler ipamReconciler, podInfoByIPProvider cns.PodInfoByIPProvider) (int, error) {
@@ -27,7 +26,7 @@ func ReconcileInitialCNSState(_ context.Context, ipamReconciler ipamReconciler,
2726
// Create a network container request that holds all the IPs from PodInfoByIP
2827
secondaryIPs := maps.Keys(podInfoByIP)
2928
ncRequest := CreateNodeSubnetNCRequest(secondaryIPs)
30-
responseCode := ipamReconciler.ReconcileIPAMState([]*cns.CreateNetworkContainerRequest{ncRequest}, podInfoByIP, nil)
29+
responseCode := ipamReconciler.ReconcileIPAMStateForNodeSubnet([]*cns.CreateNetworkContainerRequest{ncRequest}, podInfoByIP)
3130

3231
if responseCode != cnstypes.Success {
3332
return 0, errors.Errorf("failed to reconcile initial CNS state: %d", responseCode)

‎cns/nodesubnet/initialization_test.go‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,6 @@ import (
99
"github.com/Azure/azure-container-networking/cns/nodesubnet"
1010
"github.com/Azure/azure-container-networking/cns/restserver"
1111
"github.com/Azure/azure-container-networking/cns/types"
12-
"github.com/Azure/azure-container-networking/crd/nodenetworkconfig/api/v1alpha"
1312
"github.com/Azure/azure-container-networking/store"
1413
)
1514

@@ -55,7 +54,7 @@ func getMockStore() store.KeyValueStore {
5554

5655
type MockIpamStateReconciler struct{}
5756

58-
func (m *MockIpamStateReconciler) ReconcileIPAMState(ncRequests []*cns.CreateNetworkContainerRequest, podInfoByIP map[string]cns.PodInfo, _ *v1alpha.NodeNetworkConfig) types.ResponseCode {
57+
func (m *MockIpamStateReconciler) ReconcileIPAMStateForNodeSubnet(ncRequests []*cns.CreateNetworkContainerRequest, podInfoByIP map[string]cns.PodInfo) types.ResponseCode {
5958
if len(ncRequests) == 1 && len(ncRequests[0].SecondaryIPConfigs) == len(podInfoByIP) {
6059
return types.Success
6160
}

‎cns/restserver/internalapi.go‎

Lines changed: 60 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -275,24 +275,7 @@ func (service *HTTPRestService) syncHostNCVersion(ctx context.Context, channelMo
275275
return len(programmedNCs), nil
276276
}
277277

278-
func (service *HTTPRestService) ReconcileIPAMState(ncReqs []*cns.CreateNetworkContainerRequest, podInfoByIP map[string]cns.PodInfo, nnc *v1alpha.NodeNetworkConfig) types.ResponseCode {
279-
logger.Printf("Reconciling CNS IPAM state with nc requests: [%+v], PodInfo [%+v], NNC: [%+v]", ncReqs, podInfoByIP, nnc)
280-
// if no nc reqs, there is no CRD state yet
281-
if len(ncReqs) == 0 {
282-
logger.Printf("CNS starting with no NC state, podInfoMap count %d", len(podInfoByIP))
283-
return types.Success
284-
}
285-
286-
// first step in reconciliation is to create all the NCs in CNS, no IP assignment yet.
287-
for _, ncReq := range ncReqs {
288-
returnCode := service.CreateOrUpdateNetworkContainerInternal(ncReq)
289-
if returnCode != types.Success {
290-
return returnCode
291-
}
292-
}
293-
294-
logger.Debugf("ncReqs created successfully, now save IPs")
295-
278+
func (service *HTTPRestService) ReconcileIPAssignment(podInfoByIP map[string]cns.PodInfo, ncReqs []*cns.CreateNetworkContainerRequest) types.ResponseCode {
296279
// index all the secondary IP configs for all the nc reqs, for easier lookup later on.
297280
allSecIPsIdx := make(map[string]*cns.CreateNetworkContainerRequest)
298281
for i := range ncReqs {
@@ -323,6 +306,7 @@ func (service *HTTPRestService) ReconcileIPAMState(ncReqs []*cns.CreateNetworkCo
323306
// }
324307
//
325308
// such that we can iterate over pod interfaces, and assign all IPs for it at once.
309+
326310
podKeyToPodIPs, err := newPodKeyToPodIPsMap(podInfoByIP)
327311
if err != nil {
328312
logger.Errorf("could not transform pods indexed by IP address to pod IPs indexed by interface: %v", err)
@@ -380,14 +364,67 @@ func (service *HTTPRestService) ReconcileIPAMState(ncReqs []*cns.CreateNetworkCo
380364
}
381365
}
382366

383-
if nnc != nil {
384-
if err := service.MarkExistingIPsAsPendingRelease(nnc.Spec.IPsNotInUse); err != nil {
385-
logger.Errorf("[Azure CNS] Error. Failed to mark IPs as pending %v", nnc.Spec.IPsNotInUse)
386-
return types.UnexpectedError
367+
return types.Success
368+
}
369+
370+
func (service *HTTPRestService) CreateNCs(ncReqs []*cns.CreateNetworkContainerRequest) types.ResponseCode {
371+
for _, ncReq := range ncReqs {
372+
returnCode := service.CreateOrUpdateNetworkContainerInternal(ncReq)
373+
if returnCode != types.Success {
374+
return returnCode
387375
}
388376
}
389377

390-
return 0
378+
return types.Success
379+
}
380+
381+
func (service *HTTPRestService) ReconcileIPAMStateForSwift(ncReqs []*cns.CreateNetworkContainerRequest, podInfoByIP map[string]cns.PodInfo, nnc *v1alpha.NodeNetworkConfig) types.ResponseCode {
382+
logger.Printf("Reconciling CNS IPAM state with nc requests: [%+v], PodInfo [%+v], NNC: [%+v]", ncReqs, podInfoByIP, nnc)
383+
// if no nc reqs, there is no CRD state yet
384+
if len(ncReqs) == 0 {
385+
logger.Printf("CNS starting with no NC state, podInfoMap count %d", len(podInfoByIP))
386+
return types.Success
387+
}
388+
389+
// first step in reconciliation is to create all the NCs in CNS, no IP assignment yet.
390+
if returnCode := service.CreateNCs(ncReqs); returnCode != types.Success {
391+
return returnCode
392+
}
393+
394+
logger.Debugf("ncReqs created successfully, now save IPs")
395+
// now reconcile IPAM state.
396+
if returnCode := service.ReconcileIPAssignment(podInfoByIP, ncReqs); returnCode != types.Success {
397+
return returnCode
398+
}
399+
400+
if err := service.MarkExistingIPsAsPendingRelease(nnc.Spec.IPsNotInUse); err != nil {
401+
logger.Errorf("[Azure CNS] Error. Failed to mark IPs as pending %v", nnc.Spec.IPsNotInUse)
402+
return types.UnexpectedError
403+
}
404+
405+
return types.Success
406+
}
407+
408+
func (service *HTTPRestService) ReconcileIPAMStateForNodeSubnet(ncReqs []*cns.CreateNetworkContainerRequest, podInfoByIP map[string]cns.PodInfo) types.ResponseCode {
409+
logger.Printf("Reconciling CNS IPAM state with nc requests: [%+v], PodInfo [%+v]", ncReqs, podInfoByIP)
410+
// if no nc reqs, there is no CRD state yet
411+
if len(ncReqs) != 1 {
412+
logger.Errorf("Nodesubnet should always have 1 NC to hold secondary IPs")
413+
return types.NetworkContainerNotSpecified
414+
}
415+
416+
// first step in reconciliation is to create all the NCs in CNS, no IP assignment yet.
417+
if returnCode := service.CreateNCs(ncReqs); returnCode != types.Success {
418+
return returnCode
419+
}
420+
421+
logger.Debugf("ncReqs created successfully, now save IPs")
422+
// now reconcile IPAM state.
423+
if returnCode := service.ReconcileIPAssignment(podInfoByIP, ncReqs); returnCode != types.Success {
424+
return returnCode
425+
}
426+
427+
return types.Success
391428
}
392429

393430
var (

‎cns/restserver/internalapi_test.go‎

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,7 @@ func TestReconcileNCStatePrimaryIPChangeShouldFail(t *testing.T) {
9393
}
9494

9595
// now try to reconcile the state where the NC primary IP has changed
96-
resp := svc.ReconcileIPAMState(ncReqs, map[string]cns.PodInfo{}, &v1alpha.NodeNetworkConfig{})
96+
resp := svc.ReconcileIPAMStateForSwift(ncReqs, map[string]cns.PodInfo{}, &v1alpha.NodeNetworkConfig{})
9797

9898
assert.Equal(t, types.PrimaryCANotSame, resp)
9999
}
@@ -140,7 +140,7 @@ func TestReconcileNCStateGatewayChange(t *testing.T) {
140140
}
141141

142142
// now try to reconcile the state where the NC gateway has changed
143-
resp := svc.ReconcileIPAMState(ncReqs, map[string]cns.PodInfo{}, &v1alpha.NodeNetworkConfig{})
143+
resp := svc.ReconcileIPAMStateForSwift(ncReqs, map[string]cns.PodInfo{}, &v1alpha.NodeNetworkConfig{})
144144

145145
assert.Equal(t, types.Success, resp)
146146
// assert the new state reflects the gateway update
@@ -337,7 +337,7 @@ func TestReconcileNCWithEmptyState(t *testing.T) {
337337

338338
expectedNcCount := len(svc.state.ContainerStatus)
339339
expectedAssignedPods := make(map[string]cns.PodInfo)
340-
returnCode := svc.ReconcileIPAMState(nil, expectedAssignedPods, &v1alpha.NodeNetworkConfig{
340+
returnCode := svc.ReconcileIPAMStateForSwift(nil, expectedAssignedPods, &v1alpha.NodeNetworkConfig{
341341
Status: v1alpha.NodeNetworkConfigStatus{
342342
Scaler: v1alpha.Scaler{
343343
BatchSize: batchSize,
@@ -387,7 +387,7 @@ func TestReconcileNCWithEmptyStateAndPendingRelease(t *testing.T) {
387387
return pendingIPs
388388
}()
389389
req := generateNetworkContainerRequest(secondaryIPConfigs, "reconcileNc1", "-1")
390-
returnCode := svc.ReconcileIPAMState([]*cns.CreateNetworkContainerRequest{req}, expectedAssignedPods, &v1alpha.NodeNetworkConfig{
390+
returnCode := svc.ReconcileIPAMStateForSwift([]*cns.CreateNetworkContainerRequest{req}, expectedAssignedPods, &v1alpha.NodeNetworkConfig{
391391
Spec: v1alpha.NodeNetworkConfigSpec{
392392
IPsNotInUse: pending,
393393
},
@@ -434,7 +434,7 @@ func TestReconcileNCWithExistingStateAndPendingRelease(t *testing.T) {
434434
req := generateNetworkContainerRequest(secondaryIPConfigs, "reconcileNc1", "-1")
435435

436436
expectedNcCount := len(svc.state.ContainerStatus)
437-
returnCode := svc.ReconcileIPAMState([]*cns.CreateNetworkContainerRequest{req}, expectedAssignedPods, &v1alpha.NodeNetworkConfig{
437+
returnCode := svc.ReconcileIPAMStateForSwift([]*cns.CreateNetworkContainerRequest{req}, expectedAssignedPods, &v1alpha.NodeNetworkConfig{
438438
Spec: v1alpha.NodeNetworkConfigSpec{
439439
IPsNotInUse: maps.Keys(pendingIPIDs),
440440
},
@@ -471,7 +471,7 @@ func TestReconcileNCWithExistingState(t *testing.T) {
471471
}
472472

473473
expectedNcCount := len(svc.state.ContainerStatus)
474-
returnCode := svc.ReconcileIPAMState([]*cns.CreateNetworkContainerRequest{req}, expectedAssignedPods, &v1alpha.NodeNetworkConfig{
474+
returnCode := svc.ReconcileIPAMStateForSwift([]*cns.CreateNetworkContainerRequest{req}, expectedAssignedPods, &v1alpha.NodeNetworkConfig{
475475
Status: v1alpha.NodeNetworkConfigStatus{
476476
Scaler: v1alpha.Scaler{
477477
BatchSize: batchSize,
@@ -522,7 +522,7 @@ func TestReconcileCNSIPAMWithDualStackPods(t *testing.T) {
522522

523523
ncReqs := []*cns.CreateNetworkContainerRequest{ipv4NC, ipv6NC}
524524

525-
returnCode := svc.ReconcileIPAMState(ncReqs, podByIP, &v1alpha.NodeNetworkConfig{
525+
returnCode := svc.ReconcileIPAMStateForSwift(ncReqs, podByIP, &v1alpha.NodeNetworkConfig{
526526
Status: v1alpha.NodeNetworkConfigStatus{
527527
Scaler: v1alpha.Scaler{
528528
BatchSize: batchSize,
@@ -570,7 +570,7 @@ func TestReconcileCNSIPAMWithMultipleIPsPerFamilyPerPod(t *testing.T) {
570570

571571
ncReqs := []*cns.CreateNetworkContainerRequest{ipv4NC, ipv6NC}
572572

573-
returnCode := svc.ReconcileIPAMState(ncReqs, podByIP, &v1alpha.NodeNetworkConfig{
573+
returnCode := svc.ReconcileIPAMStateForSwift(ncReqs, podByIP, &v1alpha.NodeNetworkConfig{
574574
Status: v1alpha.NodeNetworkConfigStatus{
575575
Scaler: v1alpha.Scaler{
576576
BatchSize: batchSize,
@@ -697,7 +697,7 @@ func TestReconcileNCWithExistingStateFromInterfaceID(t *testing.T) {
697697
}
698698

699699
expectedNcCount := len(svc.state.ContainerStatus)
700-
returnCode := svc.ReconcileIPAMState([]*cns.CreateNetworkContainerRequest{req}, expectedAssignedPods, &v1alpha.NodeNetworkConfig{
700+
returnCode := svc.ReconcileIPAMStateForSwift([]*cns.CreateNetworkContainerRequest{req}, expectedAssignedPods, &v1alpha.NodeNetworkConfig{
701701
Status: v1alpha.NodeNetworkConfigStatus{
702702
Scaler: v1alpha.Scaler{
703703
BatchSize: batchSize,
@@ -742,7 +742,7 @@ func TestReconcileCNSIPAMWithKubePodInfoProvider(t *testing.T) {
742742
expectedAssignedPods["192.168.0.1"] = cns.NewPodInfo("", "", "systempod", "kube-system")
743743

744744
expectedNcCount := len(svc.state.ContainerStatus)
745-
returnCode := svc.ReconcileIPAMState([]*cns.CreateNetworkContainerRequest{req}, expectedAssignedPods, &v1alpha.NodeNetworkConfig{
745+
returnCode := svc.ReconcileIPAMStateForSwift([]*cns.CreateNetworkContainerRequest{req}, expectedAssignedPods, &v1alpha.NodeNetworkConfig{
746746
Status: v1alpha.NodeNetworkConfigStatus{
747747
Scaler: v1alpha.Scaler{
748748
BatchSize: batchSize,

‎cns/service/main.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1124,7 +1124,7 @@ type nodeNetworkConfigGetter interface {
11241124
}
11251125

11261126
type ipamStateReconciler interface {
1127-
ReconcileIPAMState(ncRequests []*cns.CreateNetworkContainerRequest, podInfoByIP map[string]cns.PodInfo, nnc *v1alpha.NodeNetworkConfig) cnstypes.ResponseCode
1127+
ReconcileIPAMStateForSwift(ncRequests []*cns.CreateNetworkContainerRequest, podInfoByIP map[string]cns.PodInfo, nnc *v1alpha.NodeNetworkConfig) cnstypes.ResponseCode
11281128
}
11291129

11301130
// TODO(rbtr) where should this live??
@@ -1182,7 +1182,7 @@ func reconcileInitialCNSState(ctx context.Context, cli nodeNetworkConfigGetter,
11821182
}
11831183

11841184
// Call cnsclient init cns passing those two things.
1185-
if err := restserver.ResponseCodeToError(ipamReconciler.ReconcileIPAMState(ncReqs, podInfoByIP, nnc)); err != nil {
1185+
if err := restserver.ResponseCodeToError(ipamReconciler.ReconcileIPAMStateForSwift(ncReqs, podInfoByIP, nnc)); err != nil {
11861186
return errors.Wrap(err, "failed to reconcile CNS IPAM state")
11871187
}
11881188

0 commit comments

Comments
 (0)