From 6d3a5d10f00488ff2ca3ad8d06e734bdffbf7077 Mon Sep 17 00:00:00 2001 From: Hunter Gregory Date: Mon, 15 Nov 2021 08:12:44 -0800 Subject: [PATCH 1/6] fix: for prometheus ResetIPSetEntries & feat: reset ipsets in NPM v2 --- npm/metrics/ipsets.go | 3 + npm/metrics/ipsets_test.go | 29 +- .../dataplane/ipsets/ipsetmanager_linux.go | 150 +++++++- .../ipsets/ipsetmanager_linux_test.go | 350 ++++++++++++++++-- 4 files changed, 464 insertions(+), 68 deletions(-) diff --git a/npm/metrics/ipsets.go b/npm/metrics/ipsets.go index 503388f110..01f685bd4c 100644 --- a/npm/metrics/ipsets.go +++ b/npm/metrics/ipsets.go @@ -81,6 +81,9 @@ func DeleteIPSet(setName string) { // It doesn't ever update the number of IPSets. func ResetIPSetEntries() { numIPSetEntries.Set(0) + for setName := range ipsetInventoryMap { + removeFromIPSetInventory(setName) + } ipsetInventoryMap = make(map[string]int) } diff --git a/npm/metrics/ipsets_test.go b/npm/metrics/ipsets_test.go index 6300ee545d..9c02efd6ca 100644 --- a/npm/metrics/ipsets_test.go +++ b/npm/metrics/ipsets_test.go @@ -94,11 +94,19 @@ func assertNumEntriesAndCounts(t *testing.T, sets ...*testSet) { expectedTotal := 0 for _, set := range sets { val, exists := ipsetInventoryMap[set.name] - if !exists { + if exists && set.entryCount == 0 { + require.FailNowf(t, "", "expected set %s to not exist since we expect an entry count of 0", set.name) + } + if !exists && set.entryCount > 0 { require.FailNowf(t, "", "expected set %s to exist in map for ipset entries", set.name) } if set.entryCount != val { - require.FailNowf(t, "", "set %s has incorrect number of entries. Expected %d, got %d", set.name, set.entryCount, val) + require.FailNowf(t, "", "set %s has incorrect number of entries in the map. Expected %d, got %d", set.name, set.entryCount, val) + } + prometheusVal, err := GetNumEntriesForIPSet(set.name) + require.NoError(t, err, "unexpectedly got an error when getting prometheus num entries for set %s", set.name) + if set.entryCount != prometheusVal { + require.FailNow(t, "", "set %s has incorrect number of entries in the Prometheus metric. Expected %d, got %d", set.name, set.entryCount, prometheusVal) } expectedTotal += val } @@ -110,15 +118,6 @@ func assertNumEntriesAndCounts(t *testing.T, sets ...*testSet) { } } -func assertNotInMap(t *testing.T, setNames ...string) { - for _, setName := range setNames { - _, exists := ipsetInventoryMap[setName] - if exists { - require.FailNowf(t, "", "expected set %s to not exist in map for ipset entries", setName) - } - } -} - func assertMapIsGood(t *testing.T) { assertEqualMapAndMetricElements(t) assertNoZeroEntriesInMap(t) @@ -160,8 +159,7 @@ func TestRemoveAllEntriesFromIPSet(t *testing.T) { AddEntryToIPSet(testName1) AddEntryToIPSet(testName2) RemoveAllEntriesFromIPSet(testName1) - assertNotInMap(t, testName1) - assertNumEntriesAndCounts(t, &testSet{testName2, 1}) + assertNumEntriesAndCounts(t, &testSet{testName1, 0}, &testSet{testName2, 1}) assertMapIsGood(t) } @@ -175,8 +173,7 @@ func TestDeleteIPSet(t *testing.T) { AddEntryToIPSet(testName2) DeleteIPSet(testName1) assertNumSets(t, 1) - assertNotInMap(t, testName1) - assertNumEntriesAndCounts(t, &testSet{testName2, 1}) + assertNumEntriesAndCounts(t, &testSet{testName1, 0}, &testSet{testName2, 1}) assertMapIsGood(t) } @@ -193,6 +190,6 @@ func TestResetIPSetEntries(t *testing.T) { AddEntryToIPSet(testName1) AddEntryToIPSet(testName2) ResetIPSetEntries() - assertNotInMap(t, testName1, testName2) + assertNumEntriesAndCounts(t, &testSet{testName1, 0}, &testSet{testName2, 0}) assertMapIsGood(t) } diff --git a/npm/pkg/dataplane/ipsets/ipsetmanager_linux.go b/npm/pkg/dataplane/ipsets/ipsetmanager_linux.go index c145d9dea5..991bda5a29 100644 --- a/npm/pkg/dataplane/ipsets/ipsetmanager_linux.go +++ b/npm/pkg/dataplane/ipsets/ipsetmanager_linux.go @@ -4,6 +4,7 @@ import ( "fmt" "regexp" + "github.com/Azure/azure-container-networking/npm/metrics" "github.com/Azure/azure-container-networking/npm/pkg/dataplane/ioutil" "github.com/Azure/azure-container-networking/npm/pkg/dataplane/parse" "github.com/Azure/azure-container-networking/npm/util" @@ -15,6 +16,8 @@ const ( azureNPMPrefix = "azure-npm-" ipsetCommand = "ipset" + ipsetListFlag = "list" + ipsetNameFlag = "--name" ipsetSaveFlag = "save" ipsetRestoreFlag = "restore" ipsetCreateFlag = "-N" @@ -57,14 +60,135 @@ var ( nameForAddRegex = regexp.MustCompile(fmt.Sprintf("%s (%s) ", ipsetAddString, hashedNamePattern)) ) +/* + based on ipset list output with azure-npm- prefix, create an ipset restore file where we flush all sets first, then destroy all sets + + overall error handling: + - if flush fails because the set doesn't exist (should never happen because we're listing sets right before), then ignore it and the destroy + - if flush fails otherwise, then add to destroyFailureCount and continue (aborting the destroy too) + - if destroy fails because the set doesn't exist (should never happen since the flush operation would have worked), then ignore it + - if destroy fails for another reason, then ignore it and add to destroyFailureCount and mark for reconcile (TODO) + + example: + grep output: + azure-npm-123456 + azure-npm-987654 + azure-npm-777777 + + example restore file [flag meanings: -F (flush), -X (destroy)]: + -F azure-npm-123456 + -F azure-npm-987654 + -F azure-npm-777777 + -X azure-npm-123456 + -X azure-npm-987654 + -X azure-npm-777777 + + prometheus metrics: + After this function, NumIPSets should be 0 or the number of NPM IPSets that existed and failed to be destroyed. + When NPM restarts, Prometheus metrics will initialize at 0, but NPM IPSets may exist. + We will reset ipset entry metrics if the restore succeeds whether or not some flushes/destroys failed (NOTE: this is different behavior than v1). + If a flush fails, we could update the num entries for that set, but that would be a lot of overhead. +*/ func (iMgr *IPSetManager) resetIPSets() error { - // called on failure or when NPM is created - // so no ipset cache. need to use ipset list like in ipsm.go + listCommand := iMgr.ioShim.Exec.Command(ipsetCommand, ipsetListFlag, ipsetNameFlag) + grepCommand := iMgr.ioShim.Exec.Command(ioutil.Grep, azureNPMPrefix) + searchResults, gotMatches, grepError := ioutil.PipeCommandToGrep(listCommand, grepCommand) + if grepError != nil { + return npmerrors.SimpleErrorWrapper("failed to run ipset list for resetting IPSets", grepError) + } + if !gotMatches { + metrics.ResetNumIPSets() + metrics.ResetIPSetEntries() + return nil + } + creator, originalNumSets, destroyFailureCount := iMgr.fileCreatorForReset(searchResults) + restoreError := creator.RunCommandWithFile(ipsetCommand, ipsetRestoreFlag) + if restoreError != nil { + metrics.SetNumIPSets(originalNumSets) + // NOTE: the num entries for sets may be incorrect if the restore fails + return npmerrors.SimpleErrorWrapper("failed to run ipset restore for resetting IPSets", restoreError) + } + if metrics.NumIPSetsIsPositive() { + metrics.SetNumIPSets(*destroyFailureCount) + } else { + metrics.ResetNumIPSets() + } + metrics.ResetIPSetEntries() // NOTE: the num entries for sets that fail to flush may be incorrect after this + return nil +} - // create restore file that flushes all sets, then deletes all sets - // technically don't need to flush a hashset +// this needs to be a separate function because we need to check creator contents in UTs +func (iMgr *IPSetManager) fileCreatorForReset(ipsetListOutput []byte) (creator *ioutil.FileCreator, numSets int, destroyFailureCount *int) { + zero := 0 + destroyFailureCount = &zero + creator = ioutil.NewFileCreator(iMgr.ioShim, maxTryCount, ipsetRestoreLineFailurePattern) + names := make([]string, 0) + readIndex := 0 + var line []byte + // flush all the sets and create a list of the sets so we can destroy them + for readIndex < len(ipsetListOutput) { + line, readIndex = parse.Line(readIndex, ipsetListOutput) + hashedSetName := string(line) + names = append(names, hashedSetName) + // error handlers specific to resetting ipsets + errorHandlers := []*ioutil.LineErrorHandler{ + { + Definition: setDoesntExistDefinition, + Method: ioutil.ContinueAndAbortSection, + Callback: func() { + klog.Infof("[RESET-IPSETS] skipping flush and upcoming destroy for set %s since the set doesn't exist", hashedSetName) + }, + }, + { + Definition: ioutil.AlwaysMatchDefinition, + Method: ioutil.ContinueAndAbortSection, + Callback: func() { + klog.Errorf("[RESET-IPSETS] marking flush and upcoming destroy for set %s as a failure due to unknown error", hashedSetName) + *destroyFailureCount++ + // TODO mark as a failure + }, + }, + } + sectionID := sectionID(destroySectionPrefix, hashedSetName) + creator.AddLine(sectionID, errorHandlers, ipsetFlushFlag, hashedSetName) // flush set + } - return nil + // destroy all the sets + for _, hashedSetName := range names { + hashedSetName := hashedSetName // to appease go lint + errorHandlers := []*ioutil.LineErrorHandler{ + // error handlers specific to resetting ipsets + { + Definition: setInUseByKernelDefinition, + Method: ioutil.Continue, + Callback: func() { + klog.Errorf("[RESET-IPSETS] marking destroy for set %s as a failure since the set is in use by a kernel component", hashedSetName) + *destroyFailureCount++ + // TODO mark the set as a failure and reconcile what iptables rule or ipset is referring to it + }, + }, + { + Definition: setDoesntExistDefinition, + Method: ioutil.Continue, + Callback: func() { + klog.Infof("[RESET-IPSETS] skipping destroy for set %s since the set does not exist", hashedSetName) + }, + }, + { + Definition: ioutil.AlwaysMatchDefinition, + Method: ioutil.Continue, + Callback: func() { + klog.Errorf("[RESET-IPSETS] marking destroy for set %s as a failure due to unknown error", hashedSetName) + *destroyFailureCount++ + // TODO mark the set as a failure and reconcile what iptables rule or ipset is referring to it + }, + }, + } + sectionID := sectionID(destroySectionPrefix, hashedSetName) + creator.AddLine(sectionID, errorHandlers, ipsetDestroyFlag, hashedSetName) // destroy set + } + numSets = len(names) + return creator, numSets, destroyFailureCount } /* @@ -134,7 +258,6 @@ example where every set in add/update cache should have ip 1.2.3.4 and 2.3.4.5: -A new-set-3 2.3.4.5 */ - func (iMgr *IPSetManager) applyIPSets() error { var saveFile []byte var saveError error @@ -144,7 +267,7 @@ func (iMgr *IPSetManager) applyIPSets() error { return npmerrors.SimpleErrorWrapper("ipset save failed when applying ipsets", saveError) } } - creator := iMgr.fileCreator(maxTryCount, saveFile) + creator := iMgr.fileCreatorForApply(maxTryCount, saveFile) restoreError := creator.RunCommandWithFile(ipsetCommand, ipsetRestoreFlag) if restoreError != nil { return npmerrors.SimpleErrorWrapper("ipset restore failed when applying ipsets", restoreError) @@ -165,7 +288,7 @@ func (iMgr *IPSetManager) ipsetSave() ([]byte, error) { return searchResults, nil } -func (iMgr *IPSetManager) fileCreator(maxTryCount int, saveFile []byte) *ioutil.FileCreator { +func (iMgr *IPSetManager) fileCreatorForApply(maxTryCount int, saveFile []byte) *ioutil.FileCreator { creator := ioutil.NewFileCreator(iMgr.ioShim, maxTryCount, ipsetRestoreLineFailurePattern) // TODO make the line failure pattern into a definition constant eventually // flush all sets first so we don't try to delete an ipset referenced by a list we're deleting too @@ -350,15 +473,14 @@ func (iMgr *IPSetManager) flushSetInFile(creator *ioutil.FileCreator, prefixedNa Definition: setDoesntExistDefinition, Method: ioutil.ContinueAndAbortSection, Callback: func() { - // no action needed - klog.Infof("skipping flush and upcoming delete for set %s since the set doesn't exist", prefixedName) + klog.Infof("skipping flush and upcoming destroy for set %s since the set doesn't exist", prefixedName) }, }, { Definition: ioutil.AlwaysMatchDefinition, Method: ioutil.ContinueAndAbortSection, Callback: func() { - klog.Errorf("skipping flush and upcoming delete for set %s due to unknown error", prefixedName) + klog.Errorf("skipping flush and upcoming destroy for set %s due to unknown error", prefixedName) // TODO mark as a failure // would this ever happen? }, @@ -366,7 +488,7 @@ func (iMgr *IPSetManager) flushSetInFile(creator *ioutil.FileCreator, prefixedNa } sectionID := sectionID(destroySectionPrefix, prefixedName) hashedName := util.GetHashedName(prefixedName) - creator.AddLine(sectionID, errorHandlers, ipsetFlushFlag, hashedName) + creator.AddLine(sectionID, errorHandlers, ipsetFlushFlag, hashedName) // flush set } func (iMgr *IPSetManager) destroySetInFile(creator *ioutil.FileCreator, prefixedName string) { @@ -438,7 +560,7 @@ func (iMgr *IPSetManager) deleteMemberInFile(creator *ioutil.FileCreator, set *I }, }, } - creator.AddLine(sectionID, errorHandlers, ipsetDeleteFlag, set.HashedName, member) + creator.AddLine(sectionID, errorHandlers, ipsetDeleteFlag, set.HashedName, member) // delete member } func (iMgr *IPSetManager) addMemberInFile(creator *ioutil.FileCreator, set *IPSet, sectionID, member string) { @@ -472,7 +594,7 @@ func (iMgr *IPSetManager) addMemberInFile(creator *ioutil.FileCreator, set *IPSe }, } } - creator.AddLine(sectionID, errorHandlers, ipsetAddFlag, set.HashedName, member) + creator.AddLine(sectionID, errorHandlers, ipsetAddFlag, set.HashedName, member) // add member } func sectionID(prefix, prefixedName string) string { diff --git a/npm/pkg/dataplane/ipsets/ipsetmanager_linux_test.go b/npm/pkg/dataplane/ipsets/ipsetmanager_linux_test.go index 8125ed4cb1..8fe803e55e 100644 --- a/npm/pkg/dataplane/ipsets/ipsetmanager_linux_test.go +++ b/npm/pkg/dataplane/ipsets/ipsetmanager_linux_test.go @@ -8,29 +8,295 @@ import ( "testing" "github.com/Azure/azure-container-networking/common" + "github.com/Azure/azure-container-networking/npm/metrics" + "github.com/Azure/azure-container-networking/npm/metrics/promutil" dptestutils "github.com/Azure/azure-container-networking/npm/pkg/dataplane/testutils" testutils "github.com/Azure/azure-container-networking/test/utils" "github.com/stretchr/testify/require" ) -const saveResult = "create test-list1 list:set size 8\nadd test-list1 test-list2" +const ( + saveResult = "create test-list1 list:set size 8\nadd test-list1 test-list2" -var iMgrApplyAllCfg = &IPSetManagerCfg{ - IPSetMode: ApplyAllIPSets, - NetworkName: "", -} + resetIPSetsListOutputString = `azure-npm-123456 +azure-npm-987654 +azure-npm-777777` + resetIPSetsNumGreppedSets = 3 +) + +var ( + iMgrApplyAllCfg = &IPSetManagerCfg{ + IPSetMode: ApplyAllIPSets, + NetworkName: "", + } + + resetIPSetsListOutput = []byte(resetIPSetsListOutputString) +) // TODO test that a reconcile list is updated for all the TestFailure UTs // TODO same exact TestFailure UTs for unknown errors -func TestDestroyNPMIPSets(t *testing.T) { - // TODO - calls := []testutils.TestCmd{} +func TestDestroyNPMIPSetsCreatorSuccess(t *testing.T) { + calls := []testutils.TestCmd{fakeRestoreSuccessCommand} ioshim := common.NewMockIOShim(calls) defer ioshim.VerifyCalls(t, calls) iMgr := NewIPSetManager(iMgrApplyAllCfg, ioshim) + creator, numSets, destroyFailureCount := iMgr.fileCreatorForReset(resetIPSetsListOutput) + actualLines := strings.Split(creator.ToString(), "\n") + expectedLines := []string{ + "-F azure-npm-123456", + "-F azure-npm-987654", + "-F azure-npm-777777", + "-X azure-npm-123456", + "-X azure-npm-987654", + "-X azure-npm-777777", + "", + } + dptestutils.AssertEqualLines(t, expectedLines, actualLines) + require.Equal(t, resetIPSetsNumGreppedSets, numSets, "got unexpected num sets") + wasModified, err := creator.RunCommandOnceWithFile("ipset", "restore") + require.False(t, wasModified) + require.NoError(t, err) + require.Equal(t, 0, *destroyFailureCount, "got unexpected failure count") +} - require.NoError(t, iMgr.resetIPSets()) +func TestDestroyNPMIPSetsCreatorErrorHandling(t *testing.T) { + tests := []struct { + name string + call testutils.TestCmd + expectedLines []string + expectedFailureCount int + }{ + { + name: "set doesn't exist on flush", + call: testutils.TestCmd{ + Cmd: ipsetRestoreStringSlice, + Stdout: "Error in line 2: The set with the given name does not exist", + ExitCode: 1, + }, + expectedLines: []string{ + "-F azure-npm-777777", + "-X azure-npm-123456", + "-X azure-npm-777777", + "", + }, + expectedFailureCount: 0, + }, + { + name: "some other error on flush", + call: testutils.TestCmd{ + Cmd: ipsetRestoreStringSlice, + Stdout: "Error in line 2: for some other error", + ExitCode: 1, + }, + expectedLines: []string{ + "-F azure-npm-777777", + "-X azure-npm-123456", + "-X azure-npm-777777", + "", + }, + expectedFailureCount: 1, + }, + { + name: "set doesn't exist on destroy", + call: testutils.TestCmd{ + Cmd: ipsetRestoreStringSlice, + Stdout: "Error in line 5: The set with the given name does not exist", + ExitCode: 1, + }, + expectedLines: []string{ + "-X azure-npm-777777", + "", + }, + expectedFailureCount: 0, + }, + { + name: "some other error on destroy", + call: testutils.TestCmd{ + Cmd: ipsetRestoreStringSlice, + Stdout: "Error in line 5: some other error", + ExitCode: 1, + }, + expectedLines: []string{ + "-X azure-npm-777777", + "", + }, + expectedFailureCount: 1, + }, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + calls := []testutils.TestCmd{tt.call} + ioshim := common.NewMockIOShim(calls) + defer ioshim.VerifyCalls(t, calls) + iMgr := NewIPSetManager(iMgrApplyAllCfg, ioshim) + creator, numSets, destroyFailureCount := iMgr.fileCreatorForReset(resetIPSetsListOutput) + require.Equal(t, resetIPSetsNumGreppedSets, numSets, "got unexpected num sets") + wasModified, err := creator.RunCommandOnceWithFile("ipset", "restore") + require.True(t, wasModified) + require.Error(t, err) + actualLines := strings.Split(creator.ToString(), "\n") + dptestutils.AssertEqualLines(t, tt.expectedLines, actualLines) + require.Equal(t, tt.expectedFailureCount, *destroyFailureCount, "got unexpected failure count") + }) + break + } +} + +func TestDestroyNPMIPSets(t *testing.T) { + numSetsToStart := 2 + numEntriesToStart := 5 + + tests := []struct { + name string + calls []testutils.TestCmd + wantErr bool + expectedNumSets int + expectedNumEntries int + }{ + { + name: "success with no results from grep", + calls: []testutils.TestCmd{ + {Cmd: []string{"ipset", "list", "--name"}, PipedToCommand: true}, + {Cmd: []string{"grep", "azure-npm-"}, ExitCode: 1}, + }, + wantErr: false, + expectedNumSets: 0, + expectedNumEntries: 0, + }, + { + name: "successfully delete sets", + calls: []testutils.TestCmd{ + {Cmd: []string{"ipset", "list", "--name"}, PipedToCommand: true}, + {Cmd: []string{"grep", "azure-npm-"}, Stdout: resetIPSetsListOutputString}, + fakeRestoreSuccessCommand, + }, + wantErr: false, + expectedNumSets: 0, + expectedNumEntries: 0, + }, + { + name: "grep error", + calls: []testutils.TestCmd{ + {Cmd: []string{"ipset", "list", "--name"}, HasStartError: true, PipedToCommand: true, ExitCode: 1}, + {Cmd: []string{"grep", "azure-npm-"}}, + }, + wantErr: true, + expectedNumSets: numSetsToStart, + expectedNumEntries: numEntriesToStart, + }, + { + name: "restore error from max tries", + calls: []testutils.TestCmd{ + {Cmd: []string{"ipset", "list", "--name"}, PipedToCommand: true}, + {Cmd: []string{"grep", "azure-npm-"}, Stdout: resetIPSetsListOutputString}, + {Cmd: ipsetRestoreStringSlice, ExitCode: 1}, + {Cmd: ipsetRestoreStringSlice, ExitCode: 1}, + {Cmd: ipsetRestoreStringSlice, ExitCode: 1}, + }, + wantErr: true, + expectedNumSets: resetIPSetsNumGreppedSets, + expectedNumEntries: numEntriesToStart, + }, + { + name: "successfully restore, but fail to flush/destroy 1 set since the set doesn't exist when flushing", + calls: []testutils.TestCmd{ + {Cmd: []string{"ipset", "list", "--name"}, PipedToCommand: true}, + {Cmd: []string{"grep", "azure-npm-"}, Stdout: resetIPSetsListOutputString}, + { + Cmd: ipsetRestoreStringSlice, + Stdout: "Error in line 2: The set with the given name does not exist", + ExitCode: 1, + }, + fakeRestoreSuccessCommand, + }, + wantErr: false, + expectedNumSets: 0, + expectedNumEntries: 0, + }, + { + name: "successfully restore, but fail to flush/destroy 1 set due to other flush error", + calls: []testutils.TestCmd{ + {Cmd: []string{"ipset", "list", "--name"}, PipedToCommand: true}, + {Cmd: []string{"grep", "azure-npm-"}, Stdout: resetIPSetsListOutputString}, + { + Cmd: ipsetRestoreStringSlice, + Stdout: "Error in line 2: for some other error", + ExitCode: 1, + }, + fakeRestoreSuccessCommand, + }, + wantErr: false, + expectedNumSets: 1, + expectedNumEntries: 0, + }, + { + name: "successfully restore, but fail to destroy 1 set since the set doesn't exist when destroying", + calls: []testutils.TestCmd{ + {Cmd: []string{"ipset", "list", "--name"}, PipedToCommand: true}, + {Cmd: []string{"grep", "azure-npm-"}, Stdout: resetIPSetsListOutputString}, + { + Cmd: ipsetRestoreStringSlice, + Stdout: "Error in line 5: The set with the given name does not exist", + ExitCode: 1, + }, + fakeRestoreSuccessCommand, + }, + wantErr: false, + expectedNumSets: 0, + expectedNumEntries: 0, + }, + { + name: "successfully restore, but fail to destroy 1 set due to other destroy error", + calls: []testutils.TestCmd{ + {Cmd: []string{"ipset", "list", "--name"}, PipedToCommand: true}, + {Cmd: []string{"grep", "azure-npm-"}, Stdout: resetIPSetsListOutputString}, + { + Cmd: ipsetRestoreStringSlice, + Stdout: "Error in line 5: for some other error", + ExitCode: 1, + }, + fakeRestoreSuccessCommand, + }, + wantErr: false, + expectedNumSets: 1, + expectedNumEntries: 0, + }, + } + + testSet := "set1" + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + ioshim := common.NewMockIOShim(tt.calls) + defer ioshim.VerifyCalls(t, tt.calls) + iMgr := NewIPSetManager(iMgrApplyAllCfg, ioshim) + metrics.SetNumIPSets(numSetsToStart) + metrics.ResetIPSetEntries() + for i := 0; i < numEntriesToStart; i++ { + metrics.AddEntryToIPSet(testSet) + } + + err := iMgr.resetIPSets() + if tt.wantErr { + require.Error(t, err) + } else { + require.NoError(t, err) + } + numSets, err := metrics.GetNumIPSets() + promutil.NotifyIfErrors(t, err) + require.Equal(t, tt.expectedNumSets, numSets, "got unexpected prometheus metric for num ipsets") + + numEntries, err := metrics.GetNumIPSetEntries() + promutil.NotifyIfErrors(t, err) + require.Equal(t, tt.expectedNumEntries, numEntries, "got unexpected prometheus metric for num ipset entries") + + numEntriesForSet, err := metrics.GetNumEntriesForIPSet(testSet) + promutil.NotifyIfErrors(t, err) + require.Equal(t, tt.expectedNumEntries, numEntriesForSet, "got unexpected prometheus metric for num entries for the test set") + }) + } } func TestApplyIPSetsSuccessWithoutSave(t *testing.T) { @@ -48,7 +314,6 @@ func TestApplyIPSetsSuccessWithoutSave(t *testing.T) { } func TestApplyIPSetsSuccessWithSave(t *testing.T) { - // no sets to add/update, so don't call ipset save calls := []testutils.TestCmd{ {Cmd: ipsetSaveStringSlice, PipedToCommand: true}, {Cmd: []string{"grep", "azure-npm-"}}, @@ -160,7 +425,7 @@ func TestCreateForAllSetTypes(t *testing.T) { require.NoError(t, iMgr.AddToLists([]*IPSetMetadata{TestKVNSList.Metadata}, []*IPSetMetadata{TestKVPodSet.Metadata})) iMgr.CreateIPSets([]*IPSetMetadata{TestNestedLabelList.Metadata}) - creator := iMgr.fileCreator(len(calls), nil) + creator := iMgr.fileCreatorForApply(len(calls), nil) actualLines := testAndSortRestoreFileString(t, creator.ToString()) lines := []string{ @@ -207,7 +472,7 @@ func TestDestroy(t *testing.T) { iMgr.CreateIPSets([]*IPSetMetadata{TestNestedLabelList.Metadata}) // create so we can delete iMgr.DeleteIPSet(TestNestedLabelList.PrefixName) - creator := iMgr.fileCreator(len(calls), nil) + creator := iMgr.fileCreatorForApply(len(calls), nil) actualLines := testAndSortRestoreFileString(t, creator.ToString()) lines := []string{ @@ -261,7 +526,7 @@ func TestUpdateWithIdenticalSaveFile(t *testing.T) { require.NoError(t, iMgr.AddToLists([]*IPSetMetadata{TestKVNSList.Metadata}, []*IPSetMetadata{TestKVPodSet.Metadata})) iMgr.CreateIPSets([]*IPSetMetadata{TestNestedLabelList.Metadata}) - creator := iMgr.fileCreator(len(calls), saveFileBytes) + creator := iMgr.fileCreatorForApply(len(calls), saveFileBytes) actualLines := testAndSortRestoreFileString(t, creator.ToString()) lines := []string{ @@ -323,7 +588,7 @@ func TestUpdateWithRealisticSaveFile(t *testing.T) { iMgr.CreateIPSets([]*IPSetMetadata{TestNestedLabelList.Metadata}) // create so we can delete iMgr.DeleteIPSet(TestNestedLabelList.PrefixName) - creator := iMgr.fileCreator(len(calls), saveFileBytes) + creator := iMgr.fileCreatorForApply(len(calls), saveFileBytes) actualLines := testAndSortRestoreFileString(t, creator.ToString()) // adding NSSet and KeyPodSet (should be keeping NSSet and deleting NamedportSet) lines := []string{ @@ -423,7 +688,7 @@ func TestUpdateWithBadSaveFile(t *testing.T) { TestKeyNSList.Metadata, TestNamedportSet.Metadata, cidrSet2.Metadata, nsSet2.Metadata, }) - creator := iMgr.fileCreator(len(calls), saveFileBytes) + creator := iMgr.fileCreatorForApply(len(calls), saveFileBytes) actualLines := testAndSortRestoreFileString(t, creator.ToString()) expectedLines := []string{ @@ -475,7 +740,7 @@ func TestFailureOnCreateForNewSet(t *testing.T) { iMgr.DeleteIPSet(TestKeyNSList.PrefixName) // get original creator and run it the first time - creator := iMgr.fileCreator(len(calls), nil) + creator := iMgr.fileCreatorForApply(len(calls), nil) originalLines := strings.Split(creator.ToString(), "\n") wasFileAltered, err := creator.RunCommandOnceWithFile("ipset", "restore") require.Error(t, err, "ipset restore should fail") @@ -532,7 +797,7 @@ func TestFailureOnCreateForSetInKernel(t *testing.T) { iMgr.DeleteIPSet(TestKeyNSList.PrefixName) // get original creator and run it the first time - creator := iMgr.fileCreator(len(calls), saveFileBytes) + creator := iMgr.fileCreatorForApply(len(calls), saveFileBytes) originalLines := strings.Split(creator.ToString(), "\n") wasFileAltered, err := creator.RunCommandOnceWithFile("ipset", "restore") require.Error(t, err, "ipset restore should fail") @@ -563,12 +828,12 @@ func TestFailureOnAddToListInKernel(t *testing.T) { // - update three lists already in the set, each with a delete and add line. the second list to appear will have the failed add // - create a set and add a member to it errorLineNum := 10 - setAlreadyExistsCommand := testutils.TestCmd{ + memberDoesNotExistCommand := testutils.TestCmd{ Cmd: ipsetRestoreStringSlice, Stdout: fmt.Sprintf("Error in line %d: Set to be added/deleted/tested as element does not exist", errorLineNum), // this error might happen if the cache is out of date with the kernel ExitCode: 1, } - calls := []testutils.TestCmd{setAlreadyExistsCommand, fakeRestoreSuccessCommand} + calls := []testutils.TestCmd{memberDoesNotExistCommand, fakeRestoreSuccessCommand} ioshim := common.NewMockIOShim(calls) defer ioshim.VerifyCalls(t, calls) iMgr := NewIPSetManager(iMgrApplyAllCfg, ioshim) @@ -592,7 +857,7 @@ func TestFailureOnAddToListInKernel(t *testing.T) { iMgr.CreateIPSets([]*IPSetMetadata{TestCIDRSet.Metadata}) // create so we can delete iMgr.DeleteIPSet(TestCIDRSet.PrefixName) - creator := iMgr.fileCreator(len(calls), saveFileBytes) + creator := iMgr.fileCreatorForApply(len(calls), saveFileBytes) originalLines := strings.Split(creator.ToString(), "\n") wasFileAltered, err := creator.RunCommandOnceWithFile("ipset", "restore") require.Error(t, err, "ipset restore should fail") @@ -620,12 +885,12 @@ func TestFailureOnAddToNewList(t *testing.T) { // - update a set already in the kernel with a delete and add line // - create three lists in the set, each with an add line. the second list to appear will have the failed add errorLineNum := 10 - setAlreadyExistsCommand := testutils.TestCmd{ + memberDoesNotExistCommand := testutils.TestCmd{ Cmd: ipsetRestoreStringSlice, Stdout: fmt.Sprintf("Error in line %d: Set to be added/deleted/tested as element does not exist", errorLineNum), // this error might happen if the cache is out of date with the kernel ExitCode: 1, } - calls := []testutils.TestCmd{setAlreadyExistsCommand, fakeRestoreSuccessCommand} + calls := []testutils.TestCmd{memberDoesNotExistCommand, fakeRestoreSuccessCommand} ioshim := common.NewMockIOShim(calls) defer ioshim.VerifyCalls(t, calls) iMgr := NewIPSetManager(iMgrApplyAllCfg, ioshim) @@ -644,7 +909,7 @@ func TestFailureOnAddToNewList(t *testing.T) { iMgr.CreateIPSets([]*IPSetMetadata{TestCIDRSet.Metadata}) // create so we can delete iMgr.DeleteIPSet(TestCIDRSet.PrefixName) - creator := iMgr.fileCreator(len(calls), saveFileBytes) + creator := iMgr.fileCreatorForApply(len(calls), saveFileBytes) originalLines := strings.Split(creator.ToString(), "\n") wasFileAltered, err := creator.RunCommandOnceWithFile("ipset", "restore") require.Error(t, err, "ipset restore should fail") @@ -675,12 +940,12 @@ func TestFailureOnFlush(t *testing.T) { // - update a set by deleting a member // - create a set with a member errorLineNum := 1 - setAlreadyExistsCommand := testutils.TestCmd{ + setDoesNotExistCommand := testutils.TestCmd{ Cmd: ipsetRestoreStringSlice, Stdout: fmt.Sprintf("Error in line %d: The set with the given name does not exist", errorLineNum), // this error might happen if the cache is out of date with the kernel ExitCode: 1, } - calls := []testutils.TestCmd{setAlreadyExistsCommand, fakeRestoreSuccessCommand} + calls := []testutils.TestCmd{setDoesNotExistCommand, fakeRestoreSuccessCommand} ioshim := common.NewMockIOShim(calls) defer ioshim.VerifyCalls(t, calls) iMgr := NewIPSetManager(iMgrApplyAllCfg, ioshim) @@ -700,7 +965,7 @@ func TestFailureOnFlush(t *testing.T) { iMgr.CreateIPSets([]*IPSetMetadata{TestCIDRSet.Metadata}) // create so we can delete iMgr.DeleteIPSet(TestCIDRSet.PrefixName) - creator := iMgr.fileCreator(len(calls), saveFileBytes) + creator := iMgr.fileCreatorForApply(len(calls), saveFileBytes) originalLines := strings.Split(creator.ToString(), "\n") wasFileAltered, err := creator.RunCommandOnceWithFile("ipset", "restore") require.Error(t, err, "ipset restore should fail") @@ -728,12 +993,12 @@ func TestFailureOnDestroy(t *testing.T) { // - update a set by deleting a member // - create a set with a member errorLineNum := 3 - setAlreadyExistsCommand := testutils.TestCmd{ + inUseByKernelCommand := testutils.TestCmd{ Cmd: ipsetRestoreStringSlice, Stdout: fmt.Sprintf("Error in line %d: Set cannot be destroyed: it is in use by a kernel component", errorLineNum), ExitCode: 1, } - calls := []testutils.TestCmd{setAlreadyExistsCommand, fakeRestoreSuccessCommand} + calls := []testutils.TestCmd{inUseByKernelCommand, fakeRestoreSuccessCommand} ioshim := common.NewMockIOShim(calls) defer ioshim.VerifyCalls(t, calls) iMgr := NewIPSetManager(iMgrApplyAllCfg, ioshim) @@ -753,7 +1018,7 @@ func TestFailureOnDestroy(t *testing.T) { iMgr.CreateIPSets([]*IPSetMetadata{TestCIDRSet.Metadata}) // create so we can delete iMgr.DeleteIPSet(TestCIDRSet.PrefixName) - creator := iMgr.fileCreator(len(calls), saveFileBytes) + creator := iMgr.fileCreatorForApply(len(calls), saveFileBytes) originalLines := strings.Split(creator.ToString(), "\n") wasFileAltered, err := creator.RunCommandOnceWithFile("ipset", "restore") require.Error(t, err, "ipset restore should fail") @@ -790,7 +1055,7 @@ func TestFailureOnLastLine(t *testing.T) { iMgr.CreateIPSets([]*IPSetMetadata{TestCIDRSet.Metadata}) // create so we can delete iMgr.DeleteIPSet(TestCIDRSet.PrefixName) - creator := iMgr.fileCreator(2, nil) + creator := iMgr.fileCreatorForApply(2, nil) wasFileAltered, err := creator.RunCommandOnceWithFile("ipset", "restore") require.Error(t, err, "ipset restore should fail") require.True(t, wasFileAltered, "file should be altered") @@ -803,20 +1068,20 @@ func TestFailureOnLastLine(t *testing.T) { require.False(t, wasFileAltered, "file should not be altered") } -// make sure file goes in order of flushes, destroys, creates, then adds/deletes, -// then sort those sections and return the lines in an array func testAndSortRestoreFileString(t *testing.T, multilineString string) []string { return testAndSortRestoreFileLines(t, strings.Split(multilineString, "\n")) } +// make sure file goes in order of flushes, destroys, creates, then adds/deletes, +// then sort those sections and return the lines in an array func testAndSortRestoreFileLines(t *testing.T, lines []string) []string { require.True(t, lines[len(lines)-1] == "", "restore file must end with blank line") lines = lines[:len(lines)-1] // remove the blank line flushIndices := [2]int{0, len(lines)} - destroyIndices := flushIndices - createIndices := flushIndices - addDeleteIndices := flushIndices + destroyIndices := [2]int{-1, len(lines)} // -1 means the file ended with the previous operation + createIndices := [2]int{-1, len(lines)} + addDeleteIndices := [2]int{-1, len(lines)} k := 0 for k < len(lines) { operation := lines[k][0:2] @@ -853,9 +1118,18 @@ func testAndSortRestoreFileLines(t *testing.T, lines []string) []string { k++ } flushLines := lines[flushIndices[0]:flushIndices[1]] - destroyLines := lines[destroyIndices[0]:destroyIndices[1]] - createLines := lines[createIndices[0]:createIndices[1]] - addDeleteLines := lines[addDeleteIndices[0]:addDeleteIndices[1]] + var destroyLines []string + var createLines []string + var addDeleteLines []string + if destroyIndices[0] != -1 { + destroyLines = lines[destroyIndices[0]:destroyIndices[1]] + } + if createIndices[0] != -1 { + createLines = lines[createIndices[0]:createIndices[1]] + } + if addDeleteIndices[0] != -1 { + addDeleteLines = lines[addDeleteIndices[0]:addDeleteIndices[1]] + } sort.Strings(flushLines) sort.Strings(destroyLines) sort.Strings(createLines) From dd5fedc995ad79638eb52b34052f75281d2139b3 Mon Sep 17 00:00:00 2001 From: Hunter Gregory Date: Tue, 16 Nov 2021 12:30:16 -0800 Subject: [PATCH 2/6] add note about difference in prometheus metrics in v1 vs v2, and strengthen a UT --- npm/ipsm/ipsm.go | 3 +++ npm/ipsm/ipsm_test.go | 8 ++++++++ 2 files changed, 11 insertions(+) diff --git a/npm/ipsm/ipsm.go b/npm/ipsm/ipsm.go index 6b73e65eb7..112054037d 100644 --- a/npm/ipsm/ipsm.go +++ b/npm/ipsm/ipsm.go @@ -644,6 +644,9 @@ func (ipsMgr *IpsetManager) DestroyNpmIpsets() error { } else { metrics.ResetNumIPSets() } + // NOTE: in v2, we reset ipset entries, but in v1 we only remove entries for ipsets we delete. + // So v2 may underestimate the number of entries if there are destroy failures, + // but v1 may miss removing entries if some sets are in the prometheus metric but not in the kernel. return nil } diff --git a/npm/ipsm/ipsm_test.go b/npm/ipsm/ipsm_test.go index 0788f458af..2b3f1e9e2c 100644 --- a/npm/ipsm/ipsm_test.go +++ b/npm/ipsm/ipsm_test.go @@ -528,6 +528,7 @@ func TestDestroyNpmIpsets(t *testing.T) { calls := []testutils.TestCmd{ {Cmd: []string{"ipset", "-N", "-exist", util.GetHashedName(testSet1Name), "nethash"}}, {Cmd: []string{"ipset", "-N", "-exist", util.GetHashedName(testSet2Name), "nethash"}}, + {Cmd: []string{"ipset", "-A", "-exist", util.GetHashedName(testSet1Name), "1.2.3.4"}}, {Cmd: []string{"ipset", "list"}, Stdout: ipsetListStdout}, {Cmd: []string{"ipset", "-F", "-exist", testSet1Name}}, {Cmd: []string{"ipset", "-F", "-exist", testSet2Name}}, @@ -555,6 +556,13 @@ func TestDestroyNpmIpsets(t *testing.T) { t.Errorf(err.Error()) } + // expect prometheus to add this entry, but remove it when destroying npm sets + err = ipsMgr.AddToSet(testSet1Name, "1.2.3.4", util.IpsetNetHashFlag, "") + if err != nil { + t.Errorf("TestDestroyNpmIpsets failed @ ipsMgr.addToSet") + t.Errorf(err.Error()) + } + err = ipsMgr.DestroyNpmIpsets() if err != nil { t.Errorf("TestDestroyNpmIpsets failed @ ipsMgr.DestroyNpmIpsets") From eae57225dd8cd5df00e1ef2b4de90b9d890f3057 Mon Sep 17 00:00:00 2001 From: Hunter Gregory Date: Tue, 16 Nov 2021 12:37:08 -0800 Subject: [PATCH 3/6] add comment to delegate prometheus metrics from generic ipsetmanager to OS-specific ones --- npm/pkg/dataplane/ipsets/ipsetmanager.go | 1 + 1 file changed, 1 insertion(+) diff --git a/npm/pkg/dataplane/ipsets/ipsetmanager.go b/npm/pkg/dataplane/ipsets/ipsetmanager.go index 5f477a5b82..f642343be7 100644 --- a/npm/pkg/dataplane/ipsets/ipsetmanager.go +++ b/npm/pkg/dataplane/ipsets/ipsetmanager.go @@ -37,6 +37,7 @@ type IPSetManagerCfg struct { NetworkName string } +// TODO delegate prometheus metrics logic to OS-specific ones? func NewIPSetManager(iMgrCfg *IPSetManagerCfg, ioShim *common.IOShim) *IPSetManager { return &IPSetManager{ iMgrCfg: iMgrCfg, From 85161e003f83e79c360e308a668cf192b6cd46b0 Mon Sep 17 00:00:00 2001 From: Hunter Gregory Date: Tue, 16 Nov 2021 14:05:46 -0800 Subject: [PATCH 4/6] fix UT for dataplane_test.go and fix lint --- npm/pkg/dataplane/ipsets/ipsetmanager.go | 1 + npm/pkg/dataplane/ipsets/testutils_linux.go | 5 ++++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/npm/pkg/dataplane/ipsets/ipsetmanager.go b/npm/pkg/dataplane/ipsets/ipsetmanager.go index f642343be7..13b6e1870d 100644 --- a/npm/pkg/dataplane/ipsets/ipsetmanager.go +++ b/npm/pkg/dataplane/ipsets/ipsetmanager.go @@ -38,6 +38,7 @@ type IPSetManagerCfg struct { } // TODO delegate prometheus metrics logic to OS-specific ones? + func NewIPSetManager(iMgrCfg *IPSetManagerCfg, ioShim *common.IOShim) *IPSetManager { return &IPSetManager{ iMgrCfg: iMgrCfg, diff --git a/npm/pkg/dataplane/ipsets/testutils_linux.go b/npm/pkg/dataplane/ipsets/testutils_linux.go index 4181f75a17..0b063fb215 100644 --- a/npm/pkg/dataplane/ipsets/testutils_linux.go +++ b/npm/pkg/dataplane/ipsets/testutils_linux.go @@ -25,5 +25,8 @@ func GetApplyIPSetsTestCalls(toAddOrUpdateIPSets, toDeleteIPSets []*IPSetMetadat } func GetResetTestCalls() []testutils.TestCmd { - return []testutils.TestCmd{} + return []testutils.TestCmd{ + {Cmd: []string{"ipset", "list", "--name"}, PipedToCommand: true}, + {Cmd: []string{"grep", "azure-npm-"}, ExitCode: 1}, // grep didn't find anything + } } From 3b1ed9836d55fb6a3ab8c20d4ff11e29a284ac3f Mon Sep 17 00:00:00 2001 From: Hunter Gregory Date: Tue, 16 Nov 2021 15:07:01 -0800 Subject: [PATCH 5/6] rename variables based on suggestions --- .../dataplane/ipsets/ipsetmanager_linux.go | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/npm/pkg/dataplane/ipsets/ipsetmanager_linux.go b/npm/pkg/dataplane/ipsets/ipsetmanager_linux.go index 991bda5a29..d87339ec2b 100644 --- a/npm/pkg/dataplane/ipsets/ipsetmanager_linux.go +++ b/npm/pkg/dataplane/ipsets/ipsetmanager_linux.go @@ -92,19 +92,19 @@ var ( func (iMgr *IPSetManager) resetIPSets() error { listCommand := iMgr.ioShim.Exec.Command(ipsetCommand, ipsetListFlag, ipsetNameFlag) grepCommand := iMgr.ioShim.Exec.Command(ioutil.Grep, azureNPMPrefix) - searchResults, gotMatches, grepError := ioutil.PipeCommandToGrep(listCommand, grepCommand) - if grepError != nil { - return npmerrors.SimpleErrorWrapper("failed to run ipset list for resetting IPSets", grepError) + azureIPSets, haveAzureIPSets, commandError := ioutil.PipeCommandToGrep(listCommand, grepCommand) + if commandError != nil { + return npmerrors.SimpleErrorWrapper("failed to run ipset list for resetting IPSets", commandError) } - if !gotMatches { + if !haveAzureIPSets { metrics.ResetNumIPSets() metrics.ResetIPSetEntries() return nil } - creator, originalNumSets, destroyFailureCount := iMgr.fileCreatorForReset(searchResults) + creator, originalNumAzureSets, destroyFailureCount := iMgr.fileCreatorForReset(azureIPSets) restoreError := creator.RunCommandWithFile(ipsetCommand, ipsetRestoreFlag) if restoreError != nil { - metrics.SetNumIPSets(originalNumSets) + metrics.SetNumIPSets(originalNumAzureSets) // NOTE: the num entries for sets may be incorrect if the restore fails return npmerrors.SimpleErrorWrapper("failed to run ipset restore for resetting IPSets", restoreError) } @@ -118,7 +118,7 @@ func (iMgr *IPSetManager) resetIPSets() error { } // this needs to be a separate function because we need to check creator contents in UTs -func (iMgr *IPSetManager) fileCreatorForReset(ipsetListOutput []byte) (creator *ioutil.FileCreator, numSets int, destroyFailureCount *int) { +func (iMgr *IPSetManager) fileCreatorForReset(ipsetListOutput []byte) (creator *ioutil.FileCreator, originalNumAzureSets int, destroyFailureCount *int) { zero := 0 destroyFailureCount = &zero creator = ioutil.NewFileCreator(iMgr.ioShim, maxTryCount, ipsetRestoreLineFailurePattern) @@ -187,8 +187,8 @@ func (iMgr *IPSetManager) fileCreatorForReset(ipsetListOutput []byte) (creator * sectionID := sectionID(destroySectionPrefix, hashedSetName) creator.AddLine(sectionID, errorHandlers, ipsetDestroyFlag, hashedSetName) // destroy set } - numSets = len(names) - return creator, numSets, destroyFailureCount + originalNumAzureSets = len(names) + return creator, originalNumAzureSets, destroyFailureCount } /* @@ -278,14 +278,14 @@ func (iMgr *IPSetManager) applyIPSets() error { func (iMgr *IPSetManager) ipsetSave() ([]byte, error) { command := iMgr.ioShim.Exec.Command(ipsetCommand, ipsetSaveFlag) grepCommand := iMgr.ioShim.Exec.Command(ioutil.Grep, azureNPMPrefix) - searchResults, gotMatches, err := ioutil.PipeCommandToGrep(command, grepCommand) + saveFile, haveAzureSets, err := ioutil.PipeCommandToGrep(command, grepCommand) if err != nil { return nil, npmerrors.SimpleErrorWrapper("failed to run ipset save", err) } - if !gotMatches { + if !haveAzureSets { return nil, nil } - return searchResults, nil + return saveFile, nil } func (iMgr *IPSetManager) fileCreatorForApply(maxTryCount int, saveFile []byte) *ioutil.FileCreator { From 949ff04a4f596deec709ea04d190156aecf98abc Mon Sep 17 00:00:00 2001 From: Hunter Gregory Date: Wed, 17 Nov 2021 14:54:50 -0800 Subject: [PATCH 6/6] switch to unnamed return values (will throw a go lint error) --- npm/pkg/dataplane/ipsets/ipsetmanager_linux.go | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/npm/pkg/dataplane/ipsets/ipsetmanager_linux.go b/npm/pkg/dataplane/ipsets/ipsetmanager_linux.go index d87339ec2b..93a3b2c861 100644 --- a/npm/pkg/dataplane/ipsets/ipsetmanager_linux.go +++ b/npm/pkg/dataplane/ipsets/ipsetmanager_linux.go @@ -118,10 +118,9 @@ func (iMgr *IPSetManager) resetIPSets() error { } // this needs to be a separate function because we need to check creator contents in UTs -func (iMgr *IPSetManager) fileCreatorForReset(ipsetListOutput []byte) (creator *ioutil.FileCreator, originalNumAzureSets int, destroyFailureCount *int) { - zero := 0 - destroyFailureCount = &zero - creator = ioutil.NewFileCreator(iMgr.ioShim, maxTryCount, ipsetRestoreLineFailurePattern) +func (iMgr *IPSetManager) fileCreatorForReset(ipsetListOutput []byte) (*ioutil.FileCreator, int, *int) { + destroyFailureCount := 0 + creator := ioutil.NewFileCreator(iMgr.ioShim, maxTryCount, ipsetRestoreLineFailurePattern) names := make([]string, 0) readIndex := 0 var line []byte @@ -144,7 +143,7 @@ func (iMgr *IPSetManager) fileCreatorForReset(ipsetListOutput []byte) (creator * Method: ioutil.ContinueAndAbortSection, Callback: func() { klog.Errorf("[RESET-IPSETS] marking flush and upcoming destroy for set %s as a failure due to unknown error", hashedSetName) - *destroyFailureCount++ + destroyFailureCount++ // TODO mark as a failure }, }, @@ -163,7 +162,7 @@ func (iMgr *IPSetManager) fileCreatorForReset(ipsetListOutput []byte) (creator * Method: ioutil.Continue, Callback: func() { klog.Errorf("[RESET-IPSETS] marking destroy for set %s as a failure since the set is in use by a kernel component", hashedSetName) - *destroyFailureCount++ + destroyFailureCount++ // TODO mark the set as a failure and reconcile what iptables rule or ipset is referring to it }, }, @@ -179,7 +178,7 @@ func (iMgr *IPSetManager) fileCreatorForReset(ipsetListOutput []byte) (creator * Method: ioutil.Continue, Callback: func() { klog.Errorf("[RESET-IPSETS] marking destroy for set %s as a failure due to unknown error", hashedSetName) - *destroyFailureCount++ + destroyFailureCount++ // TODO mark the set as a failure and reconcile what iptables rule or ipset is referring to it }, }, @@ -187,8 +186,8 @@ func (iMgr *IPSetManager) fileCreatorForReset(ipsetListOutput []byte) (creator * sectionID := sectionID(destroySectionPrefix, hashedSetName) creator.AddLine(sectionID, errorHandlers, ipsetDestroyFlag, hashedSetName) // destroy set } - originalNumAzureSets = len(names) - return creator, originalNumAzureSets, destroyFailureCount + originalNumAzureSets := len(names) + return creator, originalNumAzureSets, &destroyFailureCount } /*