diff --git a/pkg/agent/controller/networkpolicy/priority.go b/pkg/agent/controller/networkpolicy/priority.go index e90184e25b1..78ddde09563 100644 --- a/pkg/agent/controller/networkpolicy/priority.go +++ b/pkg/agent/controller/networkpolicy/priority.go @@ -190,7 +190,7 @@ Loop: // reassignPriorities re-arranges current Priority mappings to make place for the inserting Priority. It sifts // existing priorties up or down based on cost (how many priorities it needs to move). siftPrioritiesDown is used // as a tie-breaker. An error should only be returned if all the available ofPriorities in the table are occupied. -func (pa *priorityAssigner) reassignPriorities(insertionPoint uint16, p types.Priority) (*uint16, map[uint16]uint16, error) { +func (pa *priorityAssigner) reassignPriorities(insertionPoint uint16, p types.Priority) (*uint16, map[uint16]uint16, func(), error) { nextVacant, lastVacant := pa.getNextVacantOFPriority(insertionPoint), pa.getLastVacantOFPriority(insertionPoint) switch { case (insertionPoint == PriorityBottomCNP || lastVacant == nil) && nextVacant != nil: @@ -206,16 +206,16 @@ func (pa *priorityAssigner) reassignPriorities(insertionPoint uint16, p types.Pr return pa.siftPrioritiesDown(insertionPoint-uint16(1), *lastVacant, p) } default: - return nil, map[uint16]uint16{}, fmt.Errorf("no available Openflow priority left to insert priority %v", p) + return nil, map[uint16]uint16{}, nil, fmt.Errorf("no available Openflow priority left to insert priority %v", p) } } // siftPrioritiesUp moves all consecutive occupied ofPriorities and corresponding Priorities up by one ofPriority, // starting from the insertionPoint. It also assigns the freed ofPriority to the input Priority. -func (pa *priorityAssigner) siftPrioritiesUp(insertionPoint, nextVacant uint16, p types.Priority) (*uint16, map[uint16]uint16, error) { +func (pa *priorityAssigner) siftPrioritiesUp(insertionPoint, nextVacant uint16, p types.Priority) (*uint16, map[uint16]uint16, func(), error) { priorityReassignments := map[uint16]uint16{} if insertionPoint >= nextVacant { - return nil, priorityReassignments, fmt.Errorf("failed to determine the range for sifting priorities up") + return nil, priorityReassignments, nil, fmt.Errorf("failed to determine the range for sifting priorities up") } for i := nextVacant; i > insertionPoint; i-- { p, _ := pa.ofPriorityMap[i-1] @@ -224,15 +224,28 @@ func (pa *priorityAssigner) siftPrioritiesUp(insertionPoint, nextVacant uint16, klog.V(4).Infof("Original priority %v now needs to be re-assigned %v", i-1, i) } pa.updatePriorityAssignment(insertionPoint, p) - return &insertionPoint, priorityReassignments, nil + revertFunc := func() { + delete(pa.priorityMap, p) + for original := insertionPoint; original < nextVacant; original++ { + updated := original + 1 + p := pa.ofPriorityMap[updated] + // nextVacant was allocated because of the reassignment and needs to be released when reverting. + if updated == nextVacant { + pa.Release(updated) + } + pa.ofPriorityMap[original] = p + pa.priorityMap[p] = original + } + } + return &insertionPoint, priorityReassignments, revertFunc, nil } // siftPrioritiesDown moves all consecutive occupied ofPriorities and corresponding Priorities down by one ofPriority, // starting from the insertionPoint. It also assigns the freed ofPriority to the input Priority. -func (pa *priorityAssigner) siftPrioritiesDown(insertionPoint, lastVacant uint16, p types.Priority) (*uint16, map[uint16]uint16, error) { +func (pa *priorityAssigner) siftPrioritiesDown(insertionPoint, lastVacant uint16, p types.Priority) (*uint16, map[uint16]uint16, func(), error) { priorityReassignments := map[uint16]uint16{} if insertionPoint <= lastVacant { - return nil, priorityReassignments, fmt.Errorf("failed to determine the range for sifting priorities down") + return nil, priorityReassignments, nil, fmt.Errorf("failed to determine the range for sifting priorities down") } for i := lastVacant; i < insertionPoint; i++ { p, _ := pa.ofPriorityMap[i+1] @@ -241,28 +254,41 @@ func (pa *priorityAssigner) siftPrioritiesDown(insertionPoint, lastVacant uint16 klog.V(4).Infof("Original priority %v now needs to be re-assigned %v", i+1, i) } pa.updatePriorityAssignment(insertionPoint, p) - return &insertionPoint, priorityReassignments, nil + revertFunc := func() { + delete(pa.priorityMap, p) + for original := insertionPoint; original > lastVacant; original-- { + updated := original - 1 + p := pa.ofPriorityMap[updated] + // lastVacant was allocated because of the reassignment and needs to be released when reverting. + if updated == lastVacant { + pa.Release(updated) + } + pa.ofPriorityMap[original] = p + pa.priorityMap[p] = original + } + } + return &insertionPoint, priorityReassignments, revertFunc, nil } // GetOFPriority retrieves the OFPriority for the input Priority to be installed, // and returns installed priorities that need to be re-assigned if necessary. -func (pa *priorityAssigner) GetOFPriority(p types.Priority) (*uint16, map[uint16]uint16, error) { +func (pa *priorityAssigner) GetOFPriority(p types.Priority) (*uint16, map[uint16]uint16, func(), error) { if ofPriority, exists := pa.priorityMap[p]; exists { - return &ofPriority, nil, nil + return &ofPriority, nil, nil, nil } insertionPoint, occupied := pa.getInsertionPoint(p) if insertionPoint == PriorityBottomCNP || insertionPoint > PriorityTopCNP || occupied { return pa.reassignPriorities(insertionPoint, p) } pa.updatePriorityAssignment(insertionPoint, p) - return &insertionPoint, nil, nil + return &insertionPoint, nil, nil, nil } // RegisterPriorities registers a list of Priorities to be created with the priorityAssigner. // It is used to populate the priorityMap in case of batch rule adds. func (pa *priorityAssigner) RegisterPriorities(priorities []types.Priority) error { for _, p := range priorities { - if _, _, err := pa.GetOFPriority(p); err != nil { + if _, _, _, err := pa.GetOFPriority(p); err != nil { return err } } diff --git a/pkg/agent/controller/networkpolicy/priority_test.go b/pkg/agent/controller/networkpolicy/priority_test.go index 3424679a5bf..ab3affb2420 100644 --- a/pkg/agent/controller/networkpolicy/priority_test.go +++ b/pkg/agent/controller/networkpolicy/priority_test.go @@ -23,18 +23,19 @@ import ( ) var ( - p111 = types.Priority{TierPriority: 1, PolicyPriority: 1, RulePriority: 0} - p1121 = types.Priority{TierPriority: 1, PolicyPriority: 1.2, RulePriority: 0} - p1122 = types.Priority{TierPriority: 1, PolicyPriority: 1.2, RulePriority: 1} - p1131 = types.Priority{TierPriority: 1, PolicyPriority: 1.3, RulePriority: 0} - p1132 = types.Priority{TierPriority: 1, PolicyPriority: 1.3, RulePriority: 1} - p1141 = types.Priority{TierPriority: 1, PolicyPriority: 1.4, RulePriority: 0} - p1142 = types.Priority{TierPriority: 1, PolicyPriority: 1.4, RulePriority: 1} - p1161 = types.Priority{TierPriority: 1, PolicyPriority: 1.6, RulePriority: 0} - p191 = types.Priority{TierPriority: 1, PolicyPriority: 9, RulePriority: 0} - p192 = types.Priority{TierPriority: 1, PolicyPriority: 9, RulePriority: 1} - p193 = types.Priority{TierPriority: 1, PolicyPriority: 9, RulePriority: 2} - p194 = types.Priority{TierPriority: 1, PolicyPriority: 9, RulePriority: 3} + p110 = types.Priority{TierPriority: 1, PolicyPriority: 1, RulePriority: 0} + p1120 = types.Priority{TierPriority: 1, PolicyPriority: 1.2, RulePriority: 0} + p1121 = types.Priority{TierPriority: 1, PolicyPriority: 1.2, RulePriority: 1} + p1130 = types.Priority{TierPriority: 1, PolicyPriority: 1.3, RulePriority: 0} + p1131 = types.Priority{TierPriority: 1, PolicyPriority: 1.3, RulePriority: 1} + p1140 = types.Priority{TierPriority: 1, PolicyPriority: 1.4, RulePriority: 0} + p1141 = types.Priority{TierPriority: 1, PolicyPriority: 1.4, RulePriority: 1} + p1160 = types.Priority{TierPriority: 1, PolicyPriority: 1.6, RulePriority: 0} + p1161 = types.Priority{TierPriority: 1, PolicyPriority: 1.6, RulePriority: 1} + p190 = types.Priority{TierPriority: 1, PolicyPriority: 9, RulePriority: 0} + p191 = types.Priority{TierPriority: 1, PolicyPriority: 9, RulePriority: 1} + p192 = types.Priority{TierPriority: 1, PolicyPriority: 9, RulePriority: 2} + p193 = types.Priority{TierPriority: 1, PolicyPriority: 9, RulePriority: 3} ) func TestUpdatePriorityAssignment(t *testing.T) { @@ -48,18 +49,18 @@ func TestUpdatePriorityAssignment(t *testing.T) { }{ { "in-order", - []types.Priority{p111, p1121, p1122}, + []types.Priority{p110, p1120, p1121}, []uint16{10000, 9999, 9998}, - map[types.Priority]uint16{p111: 10000, p1121: 9999, p1122: 9998}, - map[uint16]types.Priority{10000: p111, 9999: p1121, 9998: p1122}, + map[types.Priority]uint16{p110: 10000, p1120: 9999, p1121: 9998}, + map[uint16]types.Priority{10000: p110, 9999: p1120, 9998: p1121}, []uint16{9998, 9999, 10000}, }, { "reverse-order", - []types.Priority{p1122, p1121, p111}, + []types.Priority{p1121, p1120, p110}, []uint16{9998, 9999, 10000}, - map[types.Priority]uint16{p111: 10000, p1121: 9999, p1122: 9998}, - map[uint16]types.Priority{10000: p111, 9999: p1121, 9998: p1122}, + map[types.Priority]uint16{p110: 10000, p1120: 9999, p1121: 9998}, + map[uint16]types.Priority{10000: p110, 9999: p1120, 9998: p1121}, []uint16{9998, 9999, 10000}, }, } @@ -69,9 +70,9 @@ func TestUpdatePriorityAssignment(t *testing.T) { for i := 0; i < len(tt.argsPriorities); i++ { pa.updatePriorityAssignment(tt.argsOFPriorities[i], tt.argsPriorities[i]) } - assert.Equalf(t, tt.expectedPriorityMap, pa.priorityMap, "Got priorityMap %v, expected %v", pa.priorityMap, tt.expectedPriorityMap) - assert.Equalf(t, tt.expectedOFMap, pa.ofPriorityMap, "Got ofPriorityMap %v, expected %v", pa.ofPriorityMap, tt.expectedOFMap) - assert.Equalf(t, tt.expectedSorted, pa.sortedOFPriorities, "Got sortedOFPriorities %v, expected %v", pa.sortedOFPriorities, tt.expectedSorted) + assert.Equalf(t, tt.expectedPriorityMap, pa.priorityMap, "Got unexpected priorityMap") + assert.Equalf(t, tt.expectedOFMap, pa.ofPriorityMap, "Got unexpected ofPriorityMap") + assert.Equalf(t, tt.expectedSorted, pa.sortedOFPriorities, "Got unexpected sortedOFPriorities") }) } } @@ -90,70 +91,70 @@ func TestGetInsertionPoint(t *testing.T) { "spot-on", []types.Priority{}, []uint16{}, - p111, + p110, 10000, 10000, false, }, { "stepped-on-toes-lower", - []types.Priority{p111}, + []types.Priority{p110}, []uint16{10000}, - p1121, + p1120, 10000, 9999, false, }, { "stepped-on-toes-higher", - []types.Priority{p1121}, + []types.Priority{p1120}, []uint16{10000}, - p111, + p110, 10000, 10001, false, }, { "search-up", - []types.Priority{p1121, p1122, p1131, p1132}, + []types.Priority{p1120, p1121, p1130, p1131}, []uint16{10000, 9999, 9998, 9997}, - p111, + p110, 9998, 10001, false, }, { "search-down", - []types.Priority{p1121, p1122, p1131}, + []types.Priority{p1120, p1121, p1130}, []uint16{10000, 9999, 9998}, - p1132, + p1131, 10000, 9997, false, }, { "find-insertion-up", - []types.Priority{p111, p1121, p1131, p1132}, + []types.Priority{p110, p1120, p1130, p1131}, []uint16{10000, 9999, 9998, 9997}, - p1122, + p1121, 9997, 9999, true, }, { "find-insertion-down", - []types.Priority{p111, p1121, p1131, p1132}, + []types.Priority{p110, p1120, p1130, p1131}, []uint16{10000, 9999, 9998, 9997}, - p1122, + p1121, 10000, 9999, true, }, { "upper-bound", - []types.Priority{p1121, p1122, p1131}, + []types.Priority{p1120, p1121, p1130}, []uint16{PriorityTopCNP, PriorityTopCNP - 1, PriorityTopCNP - 2}, - p111, + p110, PriorityTopCNP - 2, PriorityTopCNP + 1, false, @@ -168,8 +169,8 @@ func TestGetInsertionPoint(t *testing.T) { pa.updatePriorityAssignment(tt.argsOFPriorities[i], tt.argsPriorities[i]) } got, occupied := pa.getInsertionPoint(tt.insertingPriority) - assert.Equalf(t, tt.expectInsertionPoint, got, "Got insertion point %v, expected %v", got, tt.expectInsertionPoint) - assert.Equalf(t, tt.expectOccupied, occupied, "Got insertion point occupied %v, expected %v", got, tt.expectOccupied) + assert.Equalf(t, tt.expectInsertionPoint, got, "Got unexpected insertion point") + assert.Equalf(t, tt.expectOccupied, occupied, "Insertion point occupied status in unexpected") }) } } @@ -187,9 +188,9 @@ func TestReassignPriorities(t *testing.T) { }{ { "sift-down-at-upper-bound", - []types.Priority{p192, p194}, - []uint16{PriorityTopCNP, PriorityTopCNP - 1}, []types.Priority{p191, p193}, + []uint16{PriorityTopCNP, PriorityTopCNP - 1}, + []types.Priority{p190, p192}, []uint16{PriorityTopCNP + 1, PriorityTopCNP - 1}, []uint16{PriorityTopCNP, PriorityTopCNP - 2}, []map[uint16]uint16{ @@ -204,9 +205,9 @@ func TestReassignPriorities(t *testing.T) { }, { "sift-up-at-lower-bound", - []types.Priority{p1131, p1121}, + []types.Priority{p1130, p1120}, []uint16{PriorityBottomCNP, PriorityBottomCNP + 1}, - []types.Priority{p1122, p1132}, + []types.Priority{p1121, p1131}, []uint16{PriorityBottomCNP + 1, PriorityBottomCNP}, []uint16{PriorityBottomCNP + 1, PriorityBottomCNP}, []map[uint16]uint16{ @@ -222,9 +223,9 @@ func TestReassignPriorities(t *testing.T) { }, { "sift-based-on-cost", - []types.Priority{p111, p1122, p1132}, + []types.Priority{p110, p1121, p1131}, []uint16{10000, 9999, 9998}, - []types.Priority{p1131, p1121}, + []types.Priority{p1130, p1120}, []uint16{9999, 10000}, []uint16{9998, 10000}, []map[uint16]uint16{ @@ -239,12 +240,10 @@ func TestReassignPriorities(t *testing.T) { pa.updatePriorityAssignment(tt.argsOFPriorities[i], tt.argsPriorities[i]) } for i := 0; i < len(tt.insertingPriorities); i++ { - got, updates, err := pa.reassignPriorities(tt.insertionPoints[i], tt.insertingPriorities[i]) + got, updates, _, err := pa.reassignPriorities(tt.insertionPoints[i], tt.insertingPriorities[i]) assert.Equalf(t, err, nil, "Error occurred in reassigning priorities") - assert.Equalf(t, tt.expectedAssigned[i], *got, "Got %v for priority %v, expected %v", - got, tt.insertingPriorities[i], tt.expectedAssigned[i]) - assert.Equalf(t, tt.expectedUpdates[i], updates, "Got updates %v for priority %v, expected %v", - updates, tt.insertingPriorities[i], tt.expectedUpdates[i]) + assert.Equalf(t, tt.expectedAssigned[i], *got, "Got unexpected assigned priority") + assert.Equalf(t, tt.expectedUpdates[i], updates, "Got unexpected priority updates") } }) } @@ -253,25 +252,88 @@ func TestReassignPriorities(t *testing.T) { func TestRegisterPrioritiesAndRelease(t *testing.T) { pa := newPriorityAssigner(InitialOFPrioritySingleTierPerTable) err := pa.RegisterPriorities([]types.Priority{ - p111, p1121, p1122, p1141, p1142, p1131, p1161, + p110, p1120, p1121, p1140, p1141, p1130, p1160, }) assert.Equalf(t, err, nil, "Error occurred in registering priorities") expectedOFMap := map[uint16]types.Priority{ - 64360: p111, 64359: p1121, 64358: p1122, 64357: p1131, 64356: p1141, 64355: p1142, 64354: p1161, + 64360: p110, 64359: p1120, 64358: p1121, 64357: p1130, 64356: p1140, 64355: p1141, 64354: p1160, } - assert.Equalf(t, expectedOFMap, pa.ofPriorityMap, "Got ofPriorityMap %v, expected %v", pa.ofPriorityMap, expectedOFMap) + assert.Equalf(t, expectedOFMap, pa.ofPriorityMap, "Got unexpected ofPriorityMap") pa.Release(64359) pa.Release(64356) pa.Release(64354) expectedOFMap = map[uint16]types.Priority{ - 64360: p111, 64358: p1122, 64357: p1131, 64355: p1142, + 64360: p110, 64358: p1121, 64357: p1130, 64355: p1141, } expectedPriorityMap := map[types.Priority]uint16{ - p111: 64360, p1122: 64358, p1131: 64357, p1142: 64355, + p110: 64360, p1121: 64358, p1130: 64357, p1141: 64355, } expectedSorted := []uint16{64355, 64357, 64358, 64360} - assert.Equalf(t, expectedOFMap, pa.ofPriorityMap, "Got ofPriorityMap %v, expected %v", pa.ofPriorityMap, expectedOFMap) - assert.Equalf(t, expectedPriorityMap, pa.priorityMap, "Got priorityMap %v, expected %v", pa.priorityMap, expectedPriorityMap) - assert.Equalf(t, expectedSorted, pa.sortedOFPriorities, "Got sortedOFPriorities %v, expected %v", pa.sortedOFPriorities, expectedSorted) + assert.Equalf(t, expectedOFMap, pa.ofPriorityMap, "Got unexpected priorityMap") + assert.Equalf(t, expectedPriorityMap, pa.priorityMap, "Got unexpected ofPriorityMap") + assert.Equalf(t, expectedSorted, pa.sortedOFPriorities, "Got unexpected sortedOFPriorities") +} + +func TestRevertUpdates(t *testing.T) { + tests := []struct { + name string + insertionPoint uint16 + extraPriority types.Priority + originalPriorityMap map[types.Priority]uint16 + originalOFMap map[uint16]types.Priority + originalSorted []uint16 + }{ + { + "single-update-up", + 9999, + p1121, + map[types.Priority]uint16{p1120: 9999, p1130: 9998}, + map[uint16]types.Priority{9999: p1120, 9998: p1130}, + []uint16{9998, 9999}, + }, + { + "multiple-updates-up", + 9997, + p1131, + map[types.Priority]uint16{ + p1120: 9999, p1121: 9998, p1130: 9997, p1140: 9996, p1141: 9995, p1160: 9994, p1161: 9993}, + map[uint16]types.Priority{ + 9999: p1120, 9998: p1121, 9997: p1130, 9996: p1140, 9995: p1141, 9994: p1160, 9993: p1161}, + []uint16{9993, 9994, 9995, 9996, 9997, 9998, 9999}, + }, + { + "single-update-down", + 9999, + p1121, + map[types.Priority]uint16{p1120: 10000, p1130: 9999}, + map[uint16]types.Priority{10000: p1120, 9999: p1130}, + []uint16{9999, 10000}, + }, + { + "multiple-updates-down", + 9998, + p1131, + map[types.Priority]uint16{ + p1120: 10000, p1121: 9999, p1130: 9998, p1140: 9997, p1141: 9996, p1160: 9995}, + map[uint16]types.Priority{ + 10000: p1120, 9999: p1121, 9998: p1130, 9997: p1140, 9996: p1141, 9995: p1160}, + []uint16{9995, 9996, 9997, 9998, 9999, 10000}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + pa := newPriorityAssigner(func(p types.Priority) uint16 { + return tt.insertionPoint + }) + for ofPriority, p := range tt.originalOFMap { + pa.updatePriorityAssignment(ofPriority, p) + } + _, _, revertFunc, _ := pa.GetOFPriority(tt.extraPriority) + revertFunc() + assert.Equalf(t, tt.originalPriorityMap, pa.priorityMap, "Got unexpected priorityMap") + assert.Equalf(t, tt.originalOFMap, pa.ofPriorityMap, "Got unexpected ofPriorityMap") + assert.Equalf(t, tt.originalSorted, pa.sortedOFPriorities, "Got unexpected sortedOFPriorities") + }) + } } diff --git a/pkg/agent/controller/networkpolicy/reconciler.go b/pkg/agent/controller/networkpolicy/reconciler.go index e5e059ce616..0ba91fd5250 100644 --- a/pkg/agent/controller/networkpolicy/reconciler.go +++ b/pkg/agent/controller/networkpolicy/reconciler.go @@ -259,7 +259,7 @@ func (r *reconciler) getOFPriority(rule *CompletedRule, table binding.TableIDTyp PolicyPriority: *rule.PolicyPriority, RulePriority: rule.Priority, } - ofPriority, priorityUpdates, err := pa.assigner.GetOFPriority(p) + ofPriority, priorityUpdates, revertFunc, err := pa.assigner.GetOFPriority(p) if err != nil { return nil, err } @@ -267,8 +267,7 @@ func (r *reconciler) getOFPriority(rule *CompletedRule, table binding.TableIDTyp if len(priorityUpdates) > 0 { err := r.ofClient.ReassignFlowPriorities(priorityUpdates, table) if err != nil { - // TODO: revert the priorityUpdates in priorityMap if err occurred here. - pa.assigner.Release(*ofPriority) + revertFunc() return nil, err } }