perf: [NPM] make apply ipsets faster (#1307)

* wip

* fix member update

* fix hashed name bug

* fix hashed name bug and bug for original members for delete cache

* fix delete bug

* print cache in logs again

* some UTs

* dirty cache UTs

* fix windows build problem

* dirty cache with members to add/delete & keep old apply with save file code

* fix windows

* fix windows 2

* fix windows 3

* UTs and dirty cache in same struct shared between each OS

* fix windows build

* change some error situations to valid situations

* log clean up and addressing comments

npm/pkg/dataplane/ipsets/dirtycache.go

@@ -0,0 +1,252 @@
+package ipsets
+
+import (
+	"fmt"
+	"strings"
+
+	"github.com/Azure/azure-container-networking/npm/metrics"
+	"github.com/Azure/azure-container-networking/npm/util"
+	"k8s.io/klog"
+)
+
+/*
+	dirtyCacheInterface will maintain the dirty cache.
+	It may maintain membersToAdd and membersToDelete.
+	Members are either IPs, CIDRs, IP-Port pairs, or prefixed set names if the parent is a list.
+
+	Assumptions:
+	- if the set becomes dirty via update or destroy, then the set WAS in the kernel before
+	- if the set becomes dirty via create, then the set was NOT in the kernel before
+
+	Usage:
+	- create, addMember, deleteMember, and destroy are idempotent
+	- create should not be called if the set becomes dirty via add/delete or the set is removed from the deleteCache via add/update
+	- deleteMember should not be called if the set is in the deleteCache
+	- deleteMember is safe to call on members in the kernel and members added via addMember
+	- deleteMember is also safe to call on members not in the kernel if the set isn't in the kernel yet (became dirty via create)
+
+	Examples of Expected Behavior:
+	- if a set is created and then destroyed, that set will not be in the dirty cache anymore
+	- if a set is updated and then destroyed, that set will be in the delete cache
+	- if the only operations on a set are adding and removing the same member, the set may still be in the dirty cache, but the member will be untracked
+*/
+type dirtyCacheInterface interface {
+	// reset empties dirty cache
+	reset()
+	// resetAddOrUpdateCache empties the dirty cache of sets to be created or updated
+	resetAddOrUpdateCache()
+	// create will mark the new set to be created.
+	create(set *IPSet)
+	// addMember will mark the set to be updated and track the member to be added (if implemented).
+	addMember(set *IPSet, member string)
+	// deleteMember will mark the set to be updated and track the member to be deleted (if implemented).
+	deleteMember(set *IPSet, member string)
+	// delete will mark the set to be deleted in the cache
+	destroy(set *IPSet)
+	// setsToAddOrUpdate returns the set names to be added or updated
+	setsToAddOrUpdate() map[string]struct{}
+	// setsToDelete returns the set names to be deleted
+	setsToDelete() map[string]struct{}
+	// numSetsToAddOrUpdate returns the number of sets to be added or updated
+	numSetsToAddOrUpdate() int
+	// numSetsToDelete returns the number of sets to be deleted
+	numSetsToDelete() int
+	// isSetToAddOrUpdate returns true if the set is dirty and should be added or updated
+	isSetToAddOrUpdate(setName string) bool
+	// isSetToDelete returns true if the set is dirty and should be deleted
+	isSetToDelete(setName string) bool
+	// printAddOrUpdateCache returns a string representation of the add/update cache
+	printAddOrUpdateCache() string
+	// printDeleteCache returns a string representation of the delete cache
+	printDeleteCache() string
+	// memberDiff returns the member diff for the set.
+	// Will create a new memberDiff if the setName isn't in the dirty cache.
+	memberDiff(setName string) *memberDiff
+}
+
+type dirtyCache struct {
+	// all maps have keys of set names and values of members to add/delete
+	toCreateCache  map[string]*memberDiff
+	toUpdateCache  map[string]*memberDiff
+	toDestroyCache map[string]*memberDiff
+}
+
+func newDirtyCache() *dirtyCache {
+	dc := &dirtyCache{}
+	dc.reset()
+	return dc
+}
+
+func (dc *dirtyCache) reset() {
+	dc.toCreateCache = make(map[string]*memberDiff)
+	dc.toUpdateCache = make(map[string]*memberDiff)
+	dc.toDestroyCache = make(map[string]*memberDiff)
+}
+
+func (dc *dirtyCache) resetAddOrUpdateCache() {
+	dc.toCreateCache = make(map[string]*memberDiff)
+	dc.toUpdateCache = make(map[string]*memberDiff)
+}
+
+func (dc *dirtyCache) create(set *IPSet) {
+	if _, ok := dc.toCreateCache[set.Name]; ok {
+		return
+	}
+	// error checking
+	if _, ok := dc.toUpdateCache[set.Name]; ok {
+		msg := fmt.Sprintf("create should not be called for set %s since it's in the toUpdateCache", set.Name)
+		klog.Warning(msg)
+		metrics.SendErrorLogAndMetric(util.IpsmID, msg)
+		return
+	}
+
+	diff, ok := dc.toDestroyCache[set.Name]
+	if ok {
+		// transfer from toDestroyCache to toUpdateCache and maintain member diff
+		dc.toUpdateCache[set.Name] = diff
+		delete(dc.toDestroyCache, set.Name)
+	} else {
+		// put in the toCreateCache
+		dc.toCreateCache[set.Name] = diffOnCreate(set)
+	}
+}
+
+// could optimize Linux to remove from toUpdateCache if there were no member diff afterwards,
+// but leaving as is prevents difference between OS caches
+func (dc *dirtyCache) addMember(set *IPSet, member string) {
+	diff, ok := dc.toCreateCache[set.Name]
+	if !ok {
+		diff, ok = dc.toUpdateCache[set.Name]
+		if !ok {
+			diff, ok = dc.toDestroyCache[set.Name]
+			if !ok {
+				diff = newMemberDiff()
+			}
+		}
+		dc.toUpdateCache[set.Name] = diff
+	}
+	delete(dc.toDestroyCache, set.Name)
+	diff.addMember(member)
+}
+
+// could optimize Linux to remove from toUpdateCache if there were no member diff afterwards,
+// but leaving as is prevents difference between OS caches
+func (dc *dirtyCache) deleteMember(set *IPSet, member string) {
+	// error checking #1
+	if dc.isSetToDelete(set.Name) {
+		msg := fmt.Sprintf("attempting to delete member %s for set %s in the toDestroyCache", member, set.Name)
+		klog.Warning(msg)
+		metrics.SendErrorLogAndMetric(util.IpsmID, msg)
+		return
+	}
+	if diff, ok := dc.toCreateCache[set.Name]; ok {
+		// don't mark a member to be deleted if it never existed in the kernel
+		diff.removeMemberFromDiffToAdd(member)
+	} else {
+		diff, ok := dc.toUpdateCache[set.Name]
+		if !ok {
+			diff = newMemberDiff()
+		}
+		dc.toUpdateCache[set.Name] = diff
+		diff.deleteMember(member)
+	}
+}
+
+func (dc *dirtyCache) destroy(set *IPSet) {
+	if dc.isSetToDelete(set.Name) {
+		return
+	}
+
+	if _, ok := dc.toCreateCache[set.Name]; !ok {
+		// mark all current members as membersToDelete to accommodate force delete
+		diff, ok := dc.toUpdateCache[set.Name]
+		if !ok {
+			diff = newMemberDiff()
+		}
+		if set.Kind == HashSet {
+			for ip := range set.IPPodKey {
+				diff.deleteMember(ip)
+			}
+		} else {
+			for _, memberSet := range set.MemberIPSets {
+				diff.deleteMember(memberSet.HashedName)
+			}
+		}
+		// must call this after deleteMember for correct member diff
+		diff.resetMembersToAdd()
+
+		// put the set/diff in the toDestroyCache
+		dc.toDestroyCache[set.Name] = diff
+	}
+	// remove set from toCreateCache or toUpdateCache if necessary
+	// if the set/diff was in the toCreateCache before, we'll forget about it
+	delete(dc.toCreateCache, set.Name)
+	delete(dc.toUpdateCache, set.Name)
+}
+
+func (dc *dirtyCache) setsToAddOrUpdate() map[string]struct{} {
+	sets := make(map[string]struct{}, len(dc.toCreateCache)+len(dc.toUpdateCache))
+	for set := range dc.toCreateCache {
+		sets[set] = struct{}{}
+	}
+	for set := range dc.toUpdateCache {
+		sets[set] = struct{}{}
+	}
+	return sets
+}
+
+func (dc *dirtyCache) setsToDelete() map[string]struct{} {
+	sets := make(map[string]struct{}, len(dc.toDestroyCache))
+	for setName := range dc.toDestroyCache {
+		sets[setName] = struct{}{}
+	}
+	return sets
+}
+
+func (dc *dirtyCache) numSetsToAddOrUpdate() int {
+	return len(dc.toCreateCache) + len(dc.toUpdateCache)
+}
+
+func (dc *dirtyCache) numSetsToDelete() int {
+	return len(dc.toDestroyCache)
+}
+
+func (dc *dirtyCache) isSetToAddOrUpdate(setName string) bool {
+	_, ok1 := dc.toCreateCache[setName]
+	_, ok2 := dc.toUpdateCache[setName]
+	return ok1 || ok2
+}
+
+func (dc *dirtyCache) isSetToDelete(setName string) bool {
+	_, ok := dc.toDestroyCache[setName]
+	return ok
+}
+
+func (dc *dirtyCache) printAddOrUpdateCache() string {
+	toCreate := make([]string, 0, len(dc.toCreateCache))
+	for setName, diff := range dc.toCreateCache {
+		toCreate = append(toCreate, fmt.Sprintf("%s: %+v", setName, diff))
+	}
+	toUpdate := make([]string, 0, len(dc.toUpdateCache))
+	for setName, diff := range dc.toUpdateCache {
+		toUpdate = append(toUpdate, fmt.Sprintf("%s: %+v", setName, diff))
+	}
+	return fmt.Sprintf("to create: [%+v], to update: [%+v]", strings.Join(toCreate, ","), strings.Join(toUpdate, ","))
+}
+
+func (dc *dirtyCache) printDeleteCache() string {
+	return fmt.Sprintf("%+v", dc.toDestroyCache)
+}
+
+func (dc *dirtyCache) memberDiff(setName string) *memberDiff {
+	if diff, ok := dc.toCreateCache[setName]; ok {
+		return diff
+	}
+	if diff, ok := dc.toUpdateCache[setName]; ok {
+		return diff
+	}
+	if diff, ok := dc.toDestroyCache[setName]; ok {
+		return diff
+	}
+	return newMemberDiff()
+}

npm/pkg/dataplane/ipsets/dirtycache_linux.go

@@ -0,0 +1,59 @@
+package ipsets
+
+type memberDiff struct {
+	membersToAdd    map[string]struct{}
+	membersToDelete map[string]struct{}
+}
+
+func newMemberDiff() *memberDiff {
+	return &memberDiff{
+		membersToAdd:    make(map[string]struct{}),
+		membersToDelete: make(map[string]struct{}),
+	}
+}
+
+func diffOnCreate(set *IPSet) *memberDiff {
+	// mark all current members as membersToAdd
+	var members map[string]struct{}
+	if set.Kind == HashSet {
+		members = make(map[string]struct{}, len(set.IPPodKey))
+		for ip := range set.IPPodKey {
+			members[ip] = struct{}{}
+		}
+	} else {
+		members = make(map[string]struct{}, len(set.MemberIPSets))
+		for _, memberSet := range set.MemberIPSets {
+			members[memberSet.HashedName] = struct{}{}
+		}
+	}
+	return &memberDiff{
+		membersToAdd:    members,
+		membersToDelete: make(map[string]struct{}),
+	}
+}
+
+func (diff *memberDiff) addMember(member string) {
+	_, ok := diff.membersToDelete[member]
+	if ok {
+		delete(diff.membersToDelete, member)
+	} else {
+		diff.membersToAdd[member] = struct{}{}
+	}
+}
+
+func (diff *memberDiff) deleteMember(member string) {
+	_, ok := diff.membersToAdd[member]
+	if ok {
+		delete(diff.membersToAdd, member)
+	} else {
+		diff.membersToDelete[member] = struct{}{}
+	}
+}
+
+func (diff *memberDiff) removeMemberFromDiffToAdd(member string) {
+	delete(diff.membersToAdd, member)
+}
+
+func (diff *memberDiff) resetMembersToAdd() {
+	diff.membersToAdd = make(map[string]struct{})
+}

npm/pkg/dataplane/ipsets/dirtycache_linux_test.go

@@ -0,0 +1,28 @@
+package ipsets
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func assertDiff(t *testing.T, expected testDiff, actual *memberDiff) {
+	if len(expected.toAdd) == 0 {
+		require.Equal(t, 0, len(actual.membersToAdd), "expected 0 members to add")
+	} else {
+		require.Equal(t, stringSliceToSet(expected.toAdd), actual.membersToAdd, "unexpected members to add for set")
+	}
+	if len(expected.toDelete) == 0 {
+		require.Equal(t, 0, len(actual.membersToDelete), "expected 0 members to delete")
+	} else {
+		require.Equal(t, stringSliceToSet(expected.toDelete), actual.membersToDelete, "unexpected members to delete for set")
+	}
+}
+
+func stringSliceToSet(s []string) map[string]struct{} {
+	m := make(map[string]struct{}, len(s))
+	for _, v := range s {
+		m[v] = struct{}{}
+	}
+	return m
+}

npm/pkg/dataplane/ipsets/dirtycache_test.go

@@ -0,0 +1,426 @@
+package ipsets
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+// members are only important for Linux
+type dirtyCacheResults struct {
+	toCreate  map[string]testDiff
+	toUpdate  map[string]testDiff
+	toDestroy map[string]testDiff
+}
+
+type testDiff struct {
+	toAdd    []string
+	toDelete []string
+}
+
+const (
+	ip1    = "1.1.1.1"
+	ip2    = "2.2.2.2"
+	podKey = "pod1"
+)
+
+func TestDirtyCacheReset(t *testing.T) {
+	set1 := NewIPSet(NewIPSetMetadata("set1", Namespace))
+	set2 := NewIPSet(NewIPSetMetadata("set2", Namespace))
+	set3 := NewIPSet(NewIPSetMetadata("set3", Namespace))
+	set4 := NewIPSet(NewIPSetMetadata("set4", Namespace))
+	dc := newDirtyCache()
+	dc.create(set1)
+	dc.addMember(set2, ip1)
+	dc.deleteMember(set3, ip2)
+	dc.destroy(set4)
+	dc.reset()
+	assertDirtyCache(t, dc, &dirtyCacheResults{})
+}
+
+func TestDirtyCacheResetAddOrUpdate(t *testing.T) {
+	set1 := NewIPSet(NewIPSetMetadata("set1", Namespace))
+	set2 := NewIPSet(NewIPSetMetadata("set2", Namespace))
+	set3 := NewIPSet(NewIPSetMetadata("set3", Namespace))
+	set4 := NewIPSet(NewIPSetMetadata("set4", Namespace))
+	dc := newDirtyCache()
+	dc.create(set1)
+	dc.addMember(set2, ip1)
+	dc.deleteMember(set3, ip2)
+	// destroy will maintain this member to delete
+	set4.IPPodKey[ip1] = podKey
+	dc.destroy(set4)
+	dc.resetAddOrUpdateCache()
+	assertDirtyCache(t, dc, &dirtyCacheResults{
+		toDestroy: map[string]testDiff{
+			set4.Name: {
+				toDelete: []string{ip1},
+			},
+		},
+	})
+}
+
+func TestDirtyCacheCreate(t *testing.T) {
+	set1 := NewIPSet(NewIPSetMetadata("set1", Namespace))
+	dc := newDirtyCache()
+	dc.create(set1)
+	assertDirtyCache(t, dc, &dirtyCacheResults{
+		toCreate: map[string]testDiff{
+			set1.Name: {},
+		},
+	})
+}
+
+func TestDirtyCacheCreateWithMembers(t *testing.T) {
+	// hash set
+	dc := newDirtyCache()
+	set1 := NewIPSet(NewIPSetMetadata("set2", Namespace))
+	set1.IPPodKey[ip1] = podKey
+	dc.create(set1)
+	assertDirtyCache(t, dc, &dirtyCacheResults{
+		toCreate: map[string]testDiff{
+			set1.Name: {
+				toAdd: []string{ip1},
+			},
+		},
+	})
+
+	// list
+	dc.reset()
+	list := NewIPSet(NewIPSetMetadata("list", KeyValueLabelOfNamespace))
+	list.MemberIPSets[set1.Name] = set1
+	dc.create(list)
+	assertDirtyCache(t, dc, &dirtyCacheResults{
+		toCreate: map[string]testDiff{
+			list.Name: {
+				toAdd: []string{set1.HashedName},
+			},
+		},
+	})
+}
+
+func TestDirtyCacheCreateAfterAddOrDelete(t *testing.T) {
+	set1 := NewIPSet(NewIPSetMetadata("set1", Namespace))
+	dc := newDirtyCache()
+	dc.addMember(set1, ip1)
+	// already updated: this would create a warning log
+	dc.create(set1)
+	assertDirtyCache(t, dc, &dirtyCacheResults{
+		toUpdate: map[string]testDiff{
+			set1.Name: {
+				toAdd: []string{ip1},
+			},
+		},
+	})
+
+	dc.reset()
+	dc.deleteMember(set1, ip1)
+	// already updated: this would create a warning log
+	dc.create(set1)
+	assertDirtyCache(t, dc, &dirtyCacheResults{
+		toUpdate: map[string]testDiff{
+			set1.Name: {
+				toDelete: []string{ip1},
+			},
+		},
+	})
+}
+
+func TestDirtyCacheCreateIdempotence(t *testing.T) {
+	set1 := NewIPSet(NewIPSetMetadata("set1", Namespace))
+	dc := newDirtyCache()
+	dc.create(set1)
+	// already created: no warning log
+	dc.create(set1)
+	assertDirtyCache(t, dc, &dirtyCacheResults{
+		toCreate: map[string]testDiff{
+			set1.Name: {},
+		},
+	})
+}
+
+func TestDirtyCacheCreateAfterDestroy(t *testing.T) {
+	set1 := NewIPSet(NewIPSetMetadata("set1", Namespace))
+	dc := newDirtyCache()
+	set1.IPPodKey[ip1] = podKey
+	dc.destroy(set1)
+	// maintain members to delete in Linux
+	dc.create(set1)
+	assertDirtyCache(t, dc, &dirtyCacheResults{
+		toUpdate: map[string]testDiff{
+			set1.Name: {
+				toDelete: []string{ip1},
+			},
+		},
+	})
+}
+
+func TestDirtyCacheDestroy(t *testing.T) {
+	set1 := NewIPSet(NewIPSetMetadata("set1", Namespace))
+	dc := newDirtyCache()
+	dc.destroy(set1)
+	assertDirtyCache(t, dc, &dirtyCacheResults{
+		toDestroy: map[string]testDiff{
+			set1.Name: {},
+		},
+	})
+}
+
+func TestDirtyCacheDestroyWithMembers(t *testing.T) {
+	// hash set
+	dc := newDirtyCache()
+	set1 := NewIPSet(NewIPSetMetadata("set2", Namespace))
+	set1.IPPodKey[ip1] = podKey
+	dc.destroy(set1)
+	assertDirtyCache(t, dc, &dirtyCacheResults{
+		toDestroy: map[string]testDiff{
+			set1.Name: {
+				toDelete: []string{ip1},
+			},
+		},
+	})
+
+	// list
+	dc.reset()
+	list := NewIPSet(NewIPSetMetadata("list", KeyValueLabelOfNamespace))
+	list.MemberIPSets[set1.Name] = set1
+	dc.destroy(list)
+	assertDirtyCache(t, dc, &dirtyCacheResults{
+		toDestroy: map[string]testDiff{
+			list.Name: {
+				toDelete: []string{set1.HashedName},
+			},
+		},
+	})
+}
+
+func TestDirtyCacheDestroyIdempotence(t *testing.T) {
+	set1 := NewIPSet(NewIPSetMetadata("set1", Namespace))
+	dc := newDirtyCache()
+	dc.destroy(set1)
+	// already destroyed: this would create an error log
+	dc.destroy(set1)
+	assertDirtyCache(t, dc, &dirtyCacheResults{
+		toDestroy: map[string]testDiff{
+			set1.Name: {},
+		},
+	})
+}
+
+func TestDirtyCacheDestroyAfterCreate(t *testing.T) {
+	set1 := NewIPSet(NewIPSetMetadata("set1", Namespace))
+	dc := newDirtyCache()
+	dc.create(set1)
+	dc.addMember(set1, ip1)
+	dc.deleteMember(set1, ip2)
+	dc.destroy(set1)
+	// no set/diff to cache since the set was never in the kernel
+	assertDirtyCache(t, dc, &dirtyCacheResults{})
+}
+
+func TestDirtyCacheDestroyAfterAdd(t *testing.T) {
+	set1 := NewIPSet(NewIPSetMetadata("set1", Namespace))
+	dc := newDirtyCache()
+	dc.addMember(set1, ip1)
+	dc.destroy(set1)
+	// assumes the set was in the kernel before
+	assertDirtyCache(t, dc, &dirtyCacheResults{
+		toDestroy: map[string]testDiff{
+			set1.Name: {},
+		},
+	})
+}
+
+func TestDirtyCacheDestroyAfterDelete(t *testing.T) {
+	set1 := NewIPSet(NewIPSetMetadata("set1", Namespace))
+	dc := newDirtyCache()
+	dc.deleteMember(set1, ip1)
+	dc.destroy(set1)
+	// assumes the set was in the kernel before
+	assertDirtyCache(t, dc, &dirtyCacheResults{
+		toDestroy: map[string]testDiff{
+			set1.Name: {
+				toDelete: []string{ip1},
+			},
+		},
+	})
+}
+
+func TestDirtyCacheAdd(t *testing.T) {
+	set1 := NewIPSet(NewIPSetMetadata("set1", Namespace))
+	dc := newDirtyCache()
+	dc.addMember(set1, ip1)
+	assertDirtyCache(t, dc, &dirtyCacheResults{
+		toUpdate: map[string]testDiff{
+			set1.Name: {
+				toAdd: []string{ip1},
+			},
+		},
+	})
+}
+
+func TestDirtyCacheAddIdempotence(t *testing.T) {
+	set1 := NewIPSet(NewIPSetMetadata("set1", Namespace))
+	dc := newDirtyCache()
+	dc.addMember(set1, ip1)
+	// no warning log
+	dc.addMember(set1, ip1)
+	assertDirtyCache(t, dc, &dirtyCacheResults{
+		toUpdate: map[string]testDiff{
+			set1.Name: {
+				toAdd: []string{ip1},
+			},
+		},
+	})
+}
+
+func TestDirtyCacheAddAfterCreate(t *testing.T) {
+	set1 := NewIPSet(NewIPSetMetadata("set1", Namespace))
+	dc := newDirtyCache()
+	dc.create(set1)
+	dc.addMember(set1, ip1)
+	assertDirtyCache(t, dc, &dirtyCacheResults{
+		toCreate: map[string]testDiff{
+			set1.Name: {
+				toAdd: []string{ip1},
+			},
+		},
+	})
+}
+
+func TestDirtyCacheAddAfterDelete(t *testing.T) {
+	set1 := NewIPSet(NewIPSetMetadata("set1", Namespace))
+	dc := newDirtyCache()
+	dc.deleteMember(set1, ip1)
+	dc.addMember(set1, ip1)
+	// in update cache despite no-diff
+	assertDirtyCache(t, dc, &dirtyCacheResults{
+		toUpdate: map[string]testDiff{
+			set1.Name: {},
+		},
+	})
+}
+
+func TestDirtyCacheAddAfterDestroy(t *testing.T) {
+	set1 := NewIPSet(NewIPSetMetadata("set1", Namespace))
+	dc := newDirtyCache()
+	dc.deleteMember(set1, ip1)
+	dc.destroy(set1)
+	dc.addMember(set1, ip2)
+	assertDirtyCache(t, dc, &dirtyCacheResults{
+		toUpdate: map[string]testDiff{
+			set1.Name: {
+				toAdd:    []string{ip2},
+				toDelete: []string{ip1},
+			},
+		},
+	})
+}
+
+func TestDirtyCacheDelete(t *testing.T) {
+	set1 := NewIPSet(NewIPSetMetadata("set1", Namespace))
+	dc := newDirtyCache()
+	dc.deleteMember(set1, ip1)
+	assertDirtyCache(t, dc, &dirtyCacheResults{
+		toUpdate: map[string]testDiff{
+			set1.Name: {
+				toDelete: []string{ip1},
+			},
+		},
+	})
+}
+
+func TestDirtyCacheDeleteIdempotence(t *testing.T) {
+	set1 := NewIPSet(NewIPSetMetadata("set1", Namespace))
+	dc := newDirtyCache()
+	dc.deleteMember(set1, ip1)
+	// no error log
+	dc.deleteMember(set1, ip1)
+	assertDirtyCache(t, dc, &dirtyCacheResults{
+		toUpdate: map[string]testDiff{
+			set1.Name: {
+				toDelete: []string{ip1},
+			},
+		},
+	})
+}
+
+func TestDirtyCacheDeleteAfterCreate(t *testing.T) {
+	set1 := NewIPSet(NewIPSetMetadata("set1", Namespace))
+	dc := newDirtyCache()
+	dc.create(set1)
+	dc.deleteMember(set1, ip1)
+	assertDirtyCache(t, dc, &dirtyCacheResults{
+		toCreate: map[string]testDiff{
+			set1.Name: {},
+		},
+	})
+}
+
+func TestDirtyCacheDeleteAfterAdd(t *testing.T) {
+	set1 := NewIPSet(NewIPSetMetadata("set1", Namespace))
+	dc := newDirtyCache()
+	dc.addMember(set1, ip1)
+	dc.deleteMember(set1, ip1)
+	// in update cache despite no-diff
+	assertDirtyCache(t, dc, &dirtyCacheResults{
+		toUpdate: map[string]testDiff{
+			set1.Name: {},
+		},
+	})
+}
+
+func TestDirtyCacheDeleteAfterDestroy(t *testing.T) {
+	set1 := NewIPSet(NewIPSetMetadata("set1", Namespace))
+	dc := newDirtyCache()
+	dc.deleteMember(set1, ip1)
+	dc.destroy(set1)
+	// do nothing and create a warning log
+	dc.deleteMember(set1, ip2)
+	assertDirtyCache(t, dc, &dirtyCacheResults{
+		toDestroy: map[string]testDiff{
+			set1.Name: {
+				toDelete: []string{ip1},
+			},
+		},
+	})
+}
+
+func TestDirtyCacheNumSetsToAddOrUpdate(t *testing.T) {
+	dc := newDirtyCache()
+	dc.toCreateCache["a"] = &memberDiff{}
+	dc.toCreateCache["b"] = &memberDiff{}
+	dc.toUpdateCache["c"] = &memberDiff{}
+	require.Equal(t, 3, dc.numSetsToAddOrUpdate())
+}
+
+func assertDirtyCache(t *testing.T, dc *dirtyCache, expected *dirtyCacheResults) {
+	require.Equal(t, len(expected.toCreate), len(dc.toCreateCache), "unexpected number of sets to create")
+	require.Equal(t, len(expected.toUpdate), len(dc.toUpdateCache), "unexpected number of sets to update")
+	require.Equal(t, len(expected.toDestroy), dc.numSetsToDelete(), "unexpected number of sets to delete")
+	for setName, diff := range expected.toCreate {
+		actualDiff, ok := dc.toCreateCache[setName]
+		require.True(t, ok, "set %s not found in toCreateCache", setName)
+		require.NotNil(t, actualDiff, "member diff should not be nil for set %s", setName)
+		require.True(t, dc.isSetToAddOrUpdate(setName), "set %s should be added/updated", setName)
+		require.False(t, dc.isSetToDelete(setName), "set %s should not be deleted", setName)
+		// implemented in OS-specific test file
+		assertDiff(t, diff, dc.memberDiff(setName))
+	}
+	for setName, diff := range expected.toUpdate {
+		actualDiff, ok := dc.toUpdateCache[setName]
+		require.True(t, ok, "set %s not found in toUpdateCache", setName)
+		require.NotNil(t, actualDiff, "member diff should not be nil for set %s", setName)
+		require.True(t, dc.isSetToAddOrUpdate(setName), "set %s should be added/updated", setName)
+		require.False(t, dc.isSetToDelete(setName), "set %s should not be deleted", setName)
+		// implemented in OS-specific test file
+		assertDiff(t, diff, dc.memberDiff(setName))
+	}
+	for setName, diff := range expected.toDestroy {
+		require.NotNil(t, dc.toDestroyCache[setName], "member diff should not be nil for set %s", setName)
+		require.True(t, dc.isSetToDelete(setName), "set %s should be deleted", setName)
+		require.False(t, dc.isSetToAddOrUpdate(setName), "set %s should not be added/updated", setName)
+		// implemented in OS-specific test file
+		assertDiff(t, diff, dc.memberDiff(setName))
+	}
+}

npm/pkg/dataplane/ipsets/dirtycache_windows.go

@@ -0,0 +1,27 @@
+package ipsets
+
+type memberDiff struct{}
+
+func newMemberDiff() *memberDiff {
+	return &memberDiff{}
+}
+
+func diffOnCreate(set *IPSet) *memberDiff {
+	return newMemberDiff()
+}
+
+func (diff *memberDiff) addMember(member string) {
+	// no-op
+}
+
+func (diff *memberDiff) deleteMember(member string) {
+	// no-op
+}
+
+func (diff *memberDiff) removeMemberFromDiffToAdd(member string) {
+	// no-op
+}
+
+func (diff *memberDiff) resetMembersToAdd() {
+	// no-op
+}

npm/pkg/dataplane/ipsets/dirtycache_windows_test.go

@@ -0,0 +1,7 @@
+package ipsets
+
+import "testing"
+
+func assertDiff(_ *testing.T, _ testDiff, _ *memberDiff) {
+	// no-op
+}

npm/pkg/dataplane/ipsets/ipset.go

@@ -71,7 +71,11 @@ func (setMetadata *IPSetMetadata) GetPrefixName() string {
 }
 
 func (setMetadata *IPSetMetadata) GetSetKind() SetKind {
-	switch setMetadata.Type {
+	return setMetadata.Type.getSetKind()
+}
+
+func (setType SetType) getSetKind() SetKind {
+	switch setType {
 	case CIDRBlocks:
 		return HashSet
 	case Namespace:

npm/pkg/dataplane/ipsets/ipsetmanager.go

@@ -36,11 +36,7 @@ const (
 type IPSetManager struct {
 	iMgrCfg    *IPSetManagerCfg
 	setMap     map[string]*IPSet
-	// Map with Key as IPSet name to to emulate set
-	// and value as struct{} for minimal memory consumption.
-	toAddOrUpdateCache map[string]struct{}
-	// IPSets referred to in this cache may be in the setMap, but must be deleted from the kernel
-	toDeleteCache map[string]struct{}
+	dirtyCache dirtyCacheInterface
 	ioShim     *common.IOShim
 	sync.Mutex
 }
@@ -54,8 +50,7 @@ func NewIPSetManager(iMgrCfg *IPSetManagerCfg, ioShim *common.IOShim) *IPSetMana
 	return &IPSetManager{
 		iMgrCfg:    iMgrCfg,
 		setMap:     make(map[string]*IPSet),
-		toAddOrUpdateCache: make(map[string]struct{}),
-		toDeleteCache:      make(map[string]struct{}),
+		dirtyCache: newDirtyCache(),
 		ioShim:     ioShim,
 	}
 }
@@ -79,7 +74,7 @@ func (iMgr *IPSetManager) Reconcile() {
 	}
 	numRemovedSets := originalNumSets - len(iMgr.setMap)
 	if numRemovedSets > 0 {
-		klog.Infof("[IPSetManager] removed %d empty/unreferenced ipsets, updating toDeleteCache to: %+v", numRemovedSets, iMgr.toDeleteCache)
+		klog.Infof("[IPSetManager] removed %d empty/unreferenced ipsets, updating toDeleteCache to: %+v", numRemovedSets, iMgr.dirtyCache.printDeleteCache())
 	}
 }
 
@@ -117,7 +112,7 @@ func (iMgr *IPSetManager) createAndGetIPSet(setMetadata *IPSetMetadata) *IPSet {
 	iMgr.setMap[prefixedName] = set
 	metrics.IncNumIPSets()
 	if iMgr.iMgrCfg.IPSetMode == ApplyAllIPSets {
-		iMgr.modifyCacheForKernelCreation(prefixedName)
+		iMgr.modifyCacheForKernelCreation(set)
 	}
 	return set
 }
@@ -160,7 +155,7 @@ func (iMgr *IPSetManager) AddReference(setMetadata *IPSetMetadata, referenceName
 	if !wasInKernel {
 		// the set should be in the kernel, so add it to the kernel if it wasn't beforehand
 		// this branch can only be taken for ApplyOnNeed mode
-		iMgr.modifyCacheForKernelCreation(set.Name)
+		iMgr.modifyCacheForKernelCreation(set)
 
 		// for ApplyAllIPSets mode, the set either:
 		// a) existed already and doesn't need to be added to toAddOrUpdateCache
@@ -195,7 +190,7 @@ func (iMgr *IPSetManager) DeleteReference(setName, referenceName string, referen
 	if wasInKernel && !iMgr.shouldBeInKernel(set) {
 		// remove from kernel if it was in the kernel before and shouldn't be now
 		// this branch can only be taken for ApplyOnNeed mode
-		iMgr.modifyCacheForKernelRemoval(set.Name)
+		iMgr.modifyCacheForKernelRemoval(set)
 
 		// for ApplyAllIPSets mode, we don't want to make the set dirty
 
@@ -235,14 +230,12 @@ func (iMgr *IPSetManager) AddToSets(addToSets []*IPSetMetadata, ip, podKey strin
 
 		// 2. add ip to the set, and update the pod key
 		_, ok := set.IPPodKey[ip]
-		set.IPPodKey[ip] = podKey
-		if ok {
-			continue
-		}
-
-		iMgr.modifyCacheForKernelMemberUpdate(set)
+		if !ok {
+			iMgr.modifyCacheForKernelMemberAdd(set, ip)
 			metrics.AddEntryToIPSet(prefixedName)
 		}
+		set.IPPodKey[ip] = podKey
+	}
 	return nil
 }
 
@@ -288,8 +281,8 @@ func (iMgr *IPSetManager) RemoveFromSets(removeFromSets []*IPSetMetadata, ip, po
 		}
 
 		// update the IP ownership with podkey
+		iMgr.modifyCacheForKernelMemberDelete(set, ip)
 		delete(set.IPPodKey, ip)
-		iMgr.modifyCacheForKernelMemberUpdate(set)
 		metrics.RemoveEntryFromIPSet(prefixedName)
 	}
 	return nil
@@ -327,7 +320,6 @@ func (iMgr *IPSetManager) AddToLists(listMetadatas, setMetadatas []*IPSetMetadat
 			return npmerrors.Errorf(npmerrors.AppendIPSet, false, msg)
 		}
 
-		modified := false
 		// 3. add all members to the list
 		for _, memberMetadata := range setMetadatas {
 			memberName := memberMetadata.GetPrefixName()
@@ -341,6 +333,7 @@ func (iMgr *IPSetManager) AddToLists(listMetadatas, setMetadatas []*IPSetMetadat
 			}
 			member := iMgr.setMap[memberName]
 
+			iMgr.modifyCacheForKernelMemberAdd(list, member.HashedName)
 			list.MemberIPSets[memberName] = member
 			member.incIPSetReferCount()
 			metrics.AddEntryToIPSet(list.Name)
@@ -348,10 +341,6 @@ func (iMgr *IPSetManager) AddToLists(listMetadatas, setMetadatas []*IPSetMetadat
 			if listIsInKernel {
 				iMgr.incKernelReferCountAndModifyCache(member)
 			}
-			modified = true
-		}
-		if modified {
-			iMgr.modifyCacheForKernelMemberUpdate(list)
 		}
 	}
 	return nil
@@ -377,7 +366,6 @@ func (iMgr *IPSetManager) RemoveFromList(listMetadata *IPSetMetadata, setMetadat
 		return npmerrors.Errorf(npmerrors.DeleteIPSet, false, msg)
 	}
 
-	modified := false
 	for _, setMetadata := range setMetadatas {
 		memberName := setMetadata.GetPrefixName()
 		if memberName == "" {
@@ -392,9 +380,6 @@ func (iMgr *IPSetManager) RemoveFromList(listMetadata *IPSetMetadata, setMetadat
 		// Nested IPSets are only supported for windows
 		// Check if we want to actually use that support
 		if member.Kind != HashSet {
-			if modified {
-				iMgr.modifyCacheForKernelMemberUpdate(list)
-			}
 			msg := fmt.Sprintf("ipset %s is not a hash set and nested list sets are not supported", memberName)
 			metrics.SendErrorLogAndMetric(util.IpsmID, "error: failed to remove from list: %s", msg)
 			return npmerrors.Errorf(npmerrors.DeleteIPSet, false, msg)
@@ -405,6 +390,7 @@ func (iMgr *IPSetManager) RemoveFromList(listMetadata *IPSetMetadata, setMetadat
 			continue
 		}
 
+		iMgr.modifyCacheForKernelMemberDelete(list, member.HashedName)
 		delete(list.MemberIPSets, memberName)
 		member.decIPSetReferCount()
 		metrics.RemoveEntryFromIPSet(list.Name)
@@ -412,10 +398,6 @@ func (iMgr *IPSetManager) RemoveFromList(listMetadata *IPSetMetadata, setMetadat
 		if listIsInKernel {
 			iMgr.decKernelReferCountAndModifyCache(member)
 		}
-		modified = true
-	}
-	if modified {
-		iMgr.modifyCacheForKernelMemberUpdate(list)
 	}
 	return nil
 }
@@ -424,12 +406,15 @@ func (iMgr *IPSetManager) ApplyIPSets() error {
 	iMgr.Lock()
 	defer iMgr.Unlock()
 
-	if len(iMgr.toAddOrUpdateCache) == 0 && len(iMgr.toDeleteCache) == 0 {
+	if iMgr.dirtyCache.numSetsToAddOrUpdate() == 0 && iMgr.dirtyCache.numSetsToDelete() == 0 {
 		klog.Info("[IPSetManager] No IPSets to apply")
 		return nil
 	}
 
-	klog.Infof("[IPSetManager] toAddUpdateCache: %+v \ntoDeleteCache: %+v", iMgr.toAddOrUpdateCache, iMgr.toDeleteCache)
+	klog.Infof(
+		"[IPSetManager] dirty caches. toAddUpdateCache: %s, toDeleteCache: %s",
+		iMgr.dirtyCache.printAddOrUpdateCache(), iMgr.dirtyCache.printDeleteCache(),
+	)
 	iMgr.sanitizeDirtyCache()
 
 	// Call the appropriate apply ipsets
@@ -476,18 +461,17 @@ func (iMgr *IPSetManager) modifyCacheForCacheDeletion(set *IPSet, deleteOption u
 	}
 
 	delete(iMgr.setMap, set.Name)
-	metrics.DecNumIPSets()
+	metrics.DeleteIPSet(set.Name)
 	if iMgr.iMgrCfg.IPSetMode == ApplyAllIPSets {
 		// NOTE: in ApplyAllIPSets mode, if this ipset has never been created in the kernel,
 		// it would be added to the deleteCache, and then the OS would fail to delete it
-		iMgr.modifyCacheForKernelRemoval(set.Name)
+		iMgr.modifyCacheForKernelRemoval(set)
 	}
 	// if mode is ApplyOnNeed, the set will not be in the kernel (or will be in the delete cache already) since there are no references
 }
 
-func (iMgr *IPSetManager) modifyCacheForKernelCreation(setName string) {
-	iMgr.toAddOrUpdateCache[setName] = struct{}{}
-	delete(iMgr.toDeleteCache, setName)
+func (iMgr *IPSetManager) modifyCacheForKernelCreation(set *IPSet) {
+	iMgr.dirtyCache.create(set)
 	/*
 		TODO kernel-based prometheus metrics
 
@@ -501,7 +485,7 @@ func (iMgr *IPSetManager) incKernelReferCountAndModifyCache(member *IPSet) {
 	wasInKernel := iMgr.shouldBeInKernel(member)
 	member.incKernelReferCount()
 	if !wasInKernel {
-		iMgr.modifyCacheForKernelCreation(member.Name)
+		iMgr.modifyCacheForKernelCreation(member)
 	}
 }
 
@@ -509,9 +493,8 @@ func (iMgr *IPSetManager) shouldBeInKernel(set *IPSet) bool {
 	return set.shouldBeInKernel() || iMgr.iMgrCfg.IPSetMode == ApplyAllIPSets
 }
 
-func (iMgr *IPSetManager) modifyCacheForKernelRemoval(setName string) {
-	iMgr.toDeleteCache[setName] = struct{}{}
-	delete(iMgr.toAddOrUpdateCache, setName)
+func (iMgr *IPSetManager) modifyCacheForKernelRemoval(set *IPSet) {
+	iMgr.dirtyCache.destroy(set)
 	/*
 		TODO kernel-based prometheus metrics
 
@@ -524,13 +507,19 @@ func (iMgr *IPSetManager) modifyCacheForKernelRemoval(setName string) {
 func (iMgr *IPSetManager) decKernelReferCountAndModifyCache(member *IPSet) {
 	member.decKernelReferCount()
 	if !iMgr.shouldBeInKernel(member) {
-		iMgr.modifyCacheForKernelRemoval(member.Name)
+		iMgr.modifyCacheForKernelRemoval(member)
 	}
 }
 
-func (iMgr *IPSetManager) modifyCacheForKernelMemberUpdate(set *IPSet) {
+func (iMgr *IPSetManager) modifyCacheForKernelMemberAdd(set *IPSet, member string) {
 	if iMgr.shouldBeInKernel(set) {
-		iMgr.toAddOrUpdateCache[set.Name] = struct{}{}
+		iMgr.dirtyCache.addMember(set, member)
+	}
+}
+
+func (iMgr *IPSetManager) modifyCacheForKernelMemberDelete(set *IPSet, member string) {
+	if iMgr.shouldBeInKernel(set) {
+		iMgr.dirtyCache.deleteMember(set, member)
 	}
 }
 
@@ -538,9 +527,8 @@ func (iMgr *IPSetManager) modifyCacheForKernelMemberUpdate(set *IPSet) {
 // if so will not delete it
 func (iMgr *IPSetManager) sanitizeDirtyCache() {
 	anyProblems := false
-	for setName := range iMgr.toDeleteCache {
-		_, ok := iMgr.toAddOrUpdateCache[setName]
-		if ok {
+	for setName := range iMgr.dirtyCache.setsToDelete() {
+		if iMgr.dirtyCache.isSetToAddOrUpdate(setName) {
 			klog.Errorf("[IPSetManager] Unexpected state in dirty cache %s set is part of both update and delete caches", setName)
 			anyProblems = true
 		}
@@ -551,8 +539,7 @@ func (iMgr *IPSetManager) sanitizeDirtyCache() {
 }
 
 func (iMgr *IPSetManager) clearDirtyCache() {
-	iMgr.toAddOrUpdateCache = make(map[string]struct{})
-	iMgr.toDeleteCache = make(map[string]struct{})
+	iMgr.dirtyCache.reset()
 }
 
 // validateIPSetMemberIP helps valid if a member added to an HashSet has valid IP or CIDR

npm/pkg/dataplane/ipsets/ipsetmanager_linux.go

@@ -244,18 +244,59 @@ example where every set in add/update cache should have ip 1.2.3.4 and 2.3.4.5:
 		-X set-to-delete2
 		-X set-to-delete3
 		-X set-to-delete1
-
 */
-func (iMgr *IPSetManager) applyIPSets() error {
+// unused currently, but may be used later for reconciling with the kernel
+func (iMgr *IPSetManager) applyIPSetsWithSaveFile() error {
 	var saveFile []byte
 	var saveError error
-	if len(iMgr.toAddOrUpdateCache) > 0 {
+	if iMgr.dirtyCache.numSetsToAddOrUpdate() > 0 {
 		saveFile, saveError = iMgr.ipsetSave()
 		if saveError != nil {
 			return npmerrors.SimpleErrorWrapper("ipset save failed when applying ipsets", saveError)
 		}
 	}
-	creator := iMgr.fileCreatorForApply(maxTryCount, saveFile)
+	creator := iMgr.fileCreatorForApplyWithSaveFile(maxTryCount, saveFile)
+	restoreError := creator.RunCommandWithFile(ipsetCommand, ipsetRestoreFlag)
+	if restoreError != nil {
+		return npmerrors.SimpleErrorWrapper("ipset restore failed when applying ipsets with save file", restoreError)
+	}
+	return nil
+}
+
+/*
+See error handling in applyIPSetsWithSaveFile().
+
+overall format for ipset restore file:
+	[creates]  (random order)
+	[deletes and adds] (sets in random order, where each set has deletes first (random order), then adds (random order))
+	[flushes]  (random order)
+	[destroys] (random order)
+
+example where:
+- set1 and set2 will delete 1.2.3.4 and 2.3.4.5 and add 7.7.7.7 and 8.8.8.8
+- set3 will be created with 1.0.0.1
+- set4 and set5 will be destroyed
+
+	restore file: [flag meanings: -F (flush), -X (destroy), -N (create), -D (delete), -A (add)]
+		-N set2
+		-N set3
+		-N set1
+		-D set2 2.3.4.5
+		-D set2 1.2.3.4
+		-A set2 8.8.8.8
+		-A set2 7.7.7.7
+		-A set3 1.0.0.1
+		-D set1 1.2.3.4
+		-D set1 2.3.4.5
+		-A set1 7.7.7.7
+		-A set1 8.8.8.8
+		-F set5
+		-F set4
+		-X set5
+		-X set4
+*/
+func (iMgr *IPSetManager) applyIPSets() error {
+	creator := iMgr.fileCreatorForApply(maxTryCount)
 	restoreError := creator.RunCommandWithFile(ipsetCommand, ipsetRestoreFlag)
 	if restoreError != nil {
 		return npmerrors.SimpleErrorWrapper("ipset restore failed when applying ipsets", restoreError)
@@ -276,11 +317,13 @@ func (iMgr *IPSetManager) ipsetSave() ([]byte, error) {
 	return saveFile, nil
 }
 
-func (iMgr *IPSetManager) fileCreatorForApply(maxTryCount int, saveFile []byte) *ioutil.FileCreator {
+// NOTE: duplicate code in the first step of this function and fileCreatorForApply
+func (iMgr *IPSetManager) fileCreatorForApplyWithSaveFile(maxTryCount int, saveFile []byte) *ioutil.FileCreator {
 	creator := ioutil.NewFileCreator(iMgr.ioShim, maxTryCount, ipsetRestoreLineFailurePattern) // TODO make the line failure pattern into a definition constant eventually
 
 	// 1. create all sets first so we don't try to add a member set to a list if it hasn't been created yet
-	for prefixedName := range iMgr.toAddOrUpdateCache {
+	setsToAddOrUpdate := iMgr.dirtyCache.setsToAddOrUpdate()
+	for prefixedName := range setsToAddOrUpdate {
 		set := iMgr.setMap[prefixedName]
 		iMgr.createSetForApply(creator, set)
 		// NOTE: currently no logic to handle this scenario:
@@ -288,10 +331,10 @@ func (iMgr *IPSetManager) fileCreatorForApply(maxTryCount int, saveFile []byte)
 	}
 
 	// 2. for dirty sets already in the kernel, update members (add members not in the kernel, and delete undesired members in the kernel)
-	iMgr.updateDirtyKernelSets(saveFile, creator)
+	iMgr.updateDirtyKernelSets(setsToAddOrUpdate, saveFile, creator)
 
 	// 3. for the remaining dirty sets, add their members to the kernel
-	for prefixedName := range iMgr.toAddOrUpdateCache {
+	for prefixedName := range setsToAddOrUpdate {
 		set := iMgr.setMap[prefixedName]
 		sectionID := sectionID(addOrUpdateSectionPrefix, prefixedName)
 		if set.Kind == HashSet {
@@ -307,6 +350,50 @@ func (iMgr *IPSetManager) fileCreatorForApply(maxTryCount int, saveFile []byte)
 
 	/*
 		4. flush and destroy sets in the original delete cache
+		We must perform this step after member deletions because of the following scenario:
+		Suppose we want to destroy set A, which is referenced by list L. For set A to be in the toDeleteCache,
+		we must have deleted the reference in list L, so list L is in the toAddOrUpdateCache. In step 2, we will delete this reference,
+		but until then, set A is in use by a kernel component and can't be destroyed.
+	*/
+	// flush all sets first in case a set we're destroying is referenced by a list we're destroying
+	setsToDelete := iMgr.dirtyCache.setsToDelete()
+	for prefixedName := range setsToDelete {
+		iMgr.flushSetForApply(creator, prefixedName)
+	}
+	for prefixedName := range setsToDelete {
+		iMgr.destroySetForApply(creator, prefixedName)
+	}
+	return creator
+}
+
+// NOTE: duplicate code in the first step in this function and fileCreatorForApplyWithSaveFile
+func (iMgr *IPSetManager) fileCreatorForApply(maxTryCount int) *ioutil.FileCreator {
+	creator := ioutil.NewFileCreator(iMgr.ioShim, maxTryCount, ipsetRestoreLineFailurePattern) // TODO make the line failure pattern into a definition constant eventually
+
+	// 1. create all sets first so we don't try to add a member set to a list if it hasn't been created yet
+	setsToAddOrUpdate := iMgr.dirtyCache.setsToAddOrUpdate()
+	for prefixedName := range setsToAddOrUpdate {
+		set := iMgr.setMap[prefixedName]
+		iMgr.createSetForApply(creator, set)
+		// NOTE: currently no logic to handle this scenario:
+		// if a set in the toAddOrUpdateCache is in the kernel with the wrong type, then we'll try to create it, which will fail in the first restore call, but then be skipped in a retry
+	}
+
+	// 2. delete/add members from dirty sets to add or update
+	for prefixedName := range setsToAddOrUpdate {
+		sectionID := sectionID(addOrUpdateSectionPrefix, prefixedName)
+		set := iMgr.setMap[prefixedName]
+		diff := iMgr.dirtyCache.memberDiff(prefixedName)
+		for member := range diff.membersToDelete {
+			iMgr.deleteMemberForApply(creator, set, sectionID, member)
+		}
+		for member := range diff.membersToAdd {
+			iMgr.addMemberForApply(creator, set, sectionID, member)
+		}
+	}
+
+	/*
+		3. flush and destroy sets in the original delete cache
 
 		We must perform this step after member deletions because of the following scenario:
 		Suppose we want to destroy set A, which is referenced by list L. For set A to be in the toDeleteCache,
@@ -314,24 +401,25 @@ func (iMgr *IPSetManager) fileCreatorForApply(maxTryCount int, saveFile []byte)
 		but until then, set A is in use by a kernel component and can't be destroyed.
 	*/
 	// flush all sets first in case a set we're destroying is referenced by a list we're destroying
-	for prefixedName := range iMgr.toDeleteCache {
+	setsToDelete := iMgr.dirtyCache.setsToDelete()
+	for prefixedName := range setsToDelete {
 		iMgr.flushSetForApply(creator, prefixedName)
 	}
-	for prefixedName := range iMgr.toDeleteCache {
+	for prefixedName := range setsToDelete {
 		iMgr.destroySetForApply(creator, prefixedName)
 	}
 	return creator
 }
 
 // updates the creator (adds/deletes members) for dirty sets already in the kernel
-// updates the toAddOrUpdateCache: after calling this function, the cache will only consist of sets to create
+// updates the setsToAddOrUpdate: after calling this function, the map will only consist of sets to create
 // error handling principal:
 // - if contract with ipset save (or grep) is breaking, salvage what we can, take a snapshot (TODO), and log the failure
 // - have a background process for sending/removing snapshots intermittently
-func (iMgr *IPSetManager) updateDirtyKernelSets(saveFile []byte, creator *ioutil.FileCreator) {
+func (iMgr *IPSetManager) updateDirtyKernelSets(setsToAddOrUpdate map[string]struct{}, saveFile []byte, creator *ioutil.FileCreator) {
 	// map hashed names to prefixed names
 	toAddOrUpdateHashedNames := make(map[string]string)
-	for prefixedName := range iMgr.toAddOrUpdateCache {
+	for prefixedName := range setsToAddOrUpdate {
 		hashedName := iMgr.setMap[prefixedName].HashedName
 		toAddOrUpdateHashedNames[hashedName] = prefixedName
 	}
@@ -363,7 +451,7 @@ func (iMgr *IPSetManager) updateDirtyKernelSets(saveFile []byte, creator *ioutil
 		// 3. update the set from the kernel
 		set := iMgr.setMap[prefixedName]
 		// remove from the dirty cache so we don't add it later
-		delete(iMgr.toAddOrUpdateCache, prefixedName)
+		delete(setsToAddOrUpdate, prefixedName)
 		// mark the set as in the kernel
 		delete(toAddOrUpdateHashedNames, hashedName)
 

npm/pkg/dataplane/ipsets/ipsetmanager_linux_test.go

@@ -12,6 +12,7 @@ import (
 	"github.com/Azure/azure-container-networking/npm/metrics/promutil"
 	dptestutils "github.com/Azure/azure-container-networking/npm/pkg/dataplane/testutils"
 	"github.com/Azure/azure-container-networking/npm/util"
+	"github.com/Azure/azure-container-networking/npm/util/ioutil"
 	testutils "github.com/Azure/azure-container-networking/test/utils"
 	"github.com/stretchr/testify/require"
 )
@@ -86,16 +87,6 @@ func TestApplyIPSets(t *testing.T) {
 			expectedExecCount: 1,
 			wantErr:           false,
 		},
-		{
-			name: "set is in both delete and add/update cache",
-			args: args{
-				toAddUpdateSets: []*IPSetMetadata{namespaceSet},
-				toDeleteSets:    []*IPSetMetadata{namespaceSet},
-				commandError:    false,
-			},
-			expectedExecCount: 1,
-			wantErr:           false,
-		},
 		{
 			name: "apply error",
 			args: args{
@@ -127,7 +118,7 @@ func TestApplyIPSets(t *testing.T) {
 			iMgr := NewIPSetManager(applyAlwaysCfg, ioShim)
 			iMgr.CreateIPSets(tt.args.toAddUpdateSets)
 			for _, set := range tt.args.toDeleteSets {
-				iMgr.toDeleteCache[set.GetPrefixName()] = struct{}{}
+				iMgr.dirtyCache.destroy(NewIPSet(set))
 			}
 			err := iMgr.ApplyIPSets()
 
@@ -457,17 +448,27 @@ func TestResetIPSetsOnFailure(t *testing.T) {
 	})
 }
 
+// for applyIPSetsWithSave()
 func TestApplyIPSetsSuccessWithoutSave(t *testing.T) {
-	// no sets to add/update, so don't call ipset save
-	calls := []testutils.TestCmd{{Cmd: ipsetRestoreStringSlice}}
+	calls := []testutils.TestCmd{
+		{Cmd: ipsetSaveStringSlice, PipedToCommand: true},
+		{Cmd: []string{"grep", "azure-npm-"}},
+		{Cmd: ipsetRestoreStringSlice},
+		{Cmd: ipsetRestoreStringSlice},
+	}
 	ioshim := common.NewMockIOShim(calls)
 	defer ioshim.VerifyCalls(t, calls)
 	iMgr := NewIPSetManager(applyAlwaysCfg, ioshim)
 
 	// delete a set so the file isn't empty (otherwise the creator won't even call the exec command)
 	iMgr.CreateIPSets([]*IPSetMetadata{TestNSSet.Metadata}) // create so we can delete
+	err := iMgr.applyIPSetsWithSaveFile()
+	require.NoError(t, err)
+	iMgr.clearDirtyCache()
+
+	// no sets to add/update, so don't call ipset save
 	iMgr.DeleteIPSet(TestNSSet.PrefixName, util.SoftDelete)
-	err := iMgr.applyIPSets()
+	err = iMgr.applyIPSetsWithSaveFile()
 	require.NoError(t, err)
 }
 
@@ -483,7 +484,7 @@ func TestApplyIPSetsSuccessWithSave(t *testing.T) {
 
 	// create a set so we run ipset save
 	iMgr.CreateIPSets([]*IPSetMetadata{TestNSSet.Metadata})
-	err := iMgr.applyIPSets()
+	err := iMgr.applyIPSetsWithSaveFile()
 	require.NoError(t, err)
 }
 
@@ -498,14 +499,12 @@ func TestApplyIPSetsFailureOnSave(t *testing.T) {
 
 	// create a set so we run ipset save
 	iMgr.CreateIPSets([]*IPSetMetadata{TestNSSet.Metadata})
-	err := iMgr.applyIPSets()
+	err := iMgr.applyIPSetsWithSaveFile()
 	require.Error(t, err)
 }
 
 func TestApplyIPSetsFailureOnRestore(t *testing.T) {
 	calls := []testutils.TestCmd{
-		{Cmd: ipsetSaveStringSlice, PipedToCommand: true},
-		{Cmd: []string{"grep", "azure-npm-"}},
 		// fail 3 times because this is our max try count
 		{Cmd: ipsetRestoreStringSlice, ExitCode: 1},
 		{Cmd: ipsetRestoreStringSlice, ExitCode: 1},
@@ -514,28 +513,56 @@ func TestApplyIPSetsFailureOnRestore(t *testing.T) {
 	ioshim := common.NewMockIOShim(calls)
 	defer ioshim.VerifyCalls(t, calls)
 	iMgr := NewIPSetManager(applyAlwaysCfg, ioshim)
-
 	// create a set so we run ipset save
 	iMgr.CreateIPSets([]*IPSetMetadata{TestNSSet.Metadata})
 	err := iMgr.applyIPSets()
 	require.Error(t, err)
+
+	// same test with save file
+	calls = []testutils.TestCmd{
+		{Cmd: ipsetSaveStringSlice, PipedToCommand: true},
+		{Cmd: []string{"grep", "azure-npm-"}},
+		// fail 3 times because this is our max try count
+		{Cmd: ipsetRestoreStringSlice, ExitCode: 1},
+		{Cmd: ipsetRestoreStringSlice, ExitCode: 1},
+		{Cmd: ipsetRestoreStringSlice, ExitCode: 1},
+	}
+	ioshim = common.NewMockIOShim(calls)
+	defer ioshim.VerifyCalls(t, calls)
+	iMgr = NewIPSetManager(applyAlwaysCfg, ioshim)
+	// create a set so we run ipset save
+	iMgr.CreateIPSets([]*IPSetMetadata{TestNSSet.Metadata})
+	err = iMgr.applyIPSetsWithSaveFile()
+	require.Error(t, err)
 }
 
 func TestApplyIPSetsRecoveryForFailureOnRestore(t *testing.T) {
 	calls := []testutils.TestCmd{
-		{Cmd: ipsetSaveStringSlice, PipedToCommand: true},
-		{Cmd: []string{"grep", "azure-npm-"}},
 		{Cmd: ipsetRestoreStringSlice, ExitCode: 1},
 		{Cmd: ipsetRestoreStringSlice},
 	}
 	ioshim := common.NewMockIOShim(calls)
 	defer ioshim.VerifyCalls(t, calls)
 	iMgr := NewIPSetManager(applyAlwaysCfg, ioshim)
-
 	// create a set so we run ipset save
 	iMgr.CreateIPSets([]*IPSetMetadata{TestNSSet.Metadata})
 	err := iMgr.applyIPSets()
 	require.NoError(t, err)
+
+	// same test with save file
+	calls = []testutils.TestCmd{
+		{Cmd: ipsetSaveStringSlice, PipedToCommand: true},
+		{Cmd: []string{"grep", "azure-npm-"}},
+		{Cmd: ipsetRestoreStringSlice, ExitCode: 1},
+		{Cmd: ipsetRestoreStringSlice},
+	}
+	ioshim = common.NewMockIOShim(calls)
+	defer ioshim.VerifyCalls(t, calls)
+	iMgr = NewIPSetManager(applyAlwaysCfg, ioshim)
+	// create a set so we run ipset save
+	iMgr.CreateIPSets([]*IPSetMetadata{TestNSSet.Metadata})
+	err = iMgr.applyIPSetsWithSaveFile()
+	require.NoError(t, err)
 }
 
 func TestIPSetSave(t *testing.T) {
@@ -567,7 +594,16 @@ func TestIPSetSaveNoMatch(t *testing.T) {
 }
 
 func TestCreateForAllSetTypes(t *testing.T) {
-	// without save file
+	tests := []struct {
+		name         string
+		withSaveFile bool
+	}{
+		{name: "with save file", withSaveFile: true},
+		{name: "no save file", withSaveFile: false},
+	}
+	for _, tt := range tests {
+		tt := tt
+		t.Run(tt.name, func(t *testing.T) {
 			calls := []testutils.TestCmd{fakeRestoreSuccessCommand}
 			ioshim := common.NewMockIOShim(calls)
 			defer ioshim.VerifyCalls(t, calls)
@@ -583,7 +619,12 @@ func TestCreateForAllSetTypes(t *testing.T) {
 			require.NoError(t, iMgr.AddToLists([]*IPSetMetadata{TestKVNSList.Metadata}, []*IPSetMetadata{TestKVPodSet.Metadata}))
 			iMgr.CreateIPSets([]*IPSetMetadata{TestNestedLabelList.Metadata})
 
-	creator := iMgr.fileCreatorForApply(len(calls), nil)
+			var creator *ioutil.FileCreator
+			if tt.withSaveFile {
+				creator = iMgr.fileCreatorForApplyWithSaveFile(len(calls), nil)
+			} else {
+				creator = iMgr.fileCreatorForApply(len(calls))
+			}
 			actualLines := testAndSortRestoreFileString(t, creator.ToString())
 
 			expectedLines := []string{
@@ -609,15 +650,33 @@ func TestCreateForAllSetTypes(t *testing.T) {
 			wasFileAltered, err := creator.RunCommandOnceWithFile("ipset", "restore")
 			require.NoError(t, err, "ipset restore should be successful")
 			require.False(t, wasFileAltered, "file should not be altered")
+		})
+	}
 }
 
 func TestDestroy(t *testing.T) {
-	// without save file
-	calls := []testutils.TestCmd{fakeRestoreSuccessCommand}
+	tests := []struct {
+		name         string
+		withSaveFile bool
+	}{
+		{name: "with save file", withSaveFile: true},
+		{name: "no save file", withSaveFile: false},
+	}
+	for _, tt := range tests {
+		tt := tt
+		t.Run(tt.name, func(t *testing.T) {
+			calls := []testutils.TestCmd{
+				fakeRestoreSuccessCommand,
+			}
 			ioshim := common.NewMockIOShim(calls)
 			defer ioshim.VerifyCalls(t, calls)
 			iMgr := NewIPSetManager(applyAlwaysCfg, ioshim)
 
+			iMgr.CreateIPSets([]*IPSetMetadata{TestCIDRSet.Metadata})         // create so we can delete
+			iMgr.CreateIPSets([]*IPSetMetadata{TestNestedLabelList.Metadata}) // create so we can delete
+			// clear dirty cache, otherwise a set deletion will be a no-op
+			iMgr.clearDirtyCache()
+
 			// remove some members and destroy some sets
 			require.NoError(t, iMgr.AddToSets([]*IPSetMetadata{TestNSSet.Metadata}, "10.0.0.0", "a"))
 			require.NoError(t, iMgr.AddToSets([]*IPSetMetadata{TestNSSet.Metadata}, "10.0.0.1", "b"))
@@ -625,12 +684,15 @@ func TestDestroy(t *testing.T) {
 			iMgr.CreateIPSets([]*IPSetMetadata{TestKeyPodSet.Metadata})
 			require.NoError(t, iMgr.AddToLists([]*IPSetMetadata{TestKeyNSList.Metadata}, []*IPSetMetadata{TestNSSet.Metadata, TestKeyPodSet.Metadata}))
 			require.NoError(t, iMgr.RemoveFromList(TestKeyNSList.Metadata, []*IPSetMetadata{TestKeyPodSet.Metadata}))
-	iMgr.CreateIPSets([]*IPSetMetadata{TestCIDRSet.Metadata}) // create so we can delete
 			iMgr.DeleteIPSet(TestCIDRSet.PrefixName, util.SoftDelete)
-	iMgr.CreateIPSets([]*IPSetMetadata{TestNestedLabelList.Metadata}) // create so we can delete
 			iMgr.DeleteIPSet(TestNestedLabelList.PrefixName, util.SoftDelete)
 
-	creator := iMgr.fileCreatorForApply(len(calls), nil)
+			var creator *ioutil.FileCreator
+			if tt.withSaveFile {
+				creator = iMgr.fileCreatorForApplyWithSaveFile(1, nil)
+			} else {
+				creator = iMgr.fileCreatorForApply(1)
+			}
 			actualLines := testAndSortRestoreFileString(t, creator.ToString())
 
 			expectedLines := []string{
@@ -647,6 +709,54 @@ func TestDestroy(t *testing.T) {
 			}
 			sortedExpectedLines := testAndSortRestoreFileLines(t, expectedLines)
 
+			dptestutils.AssertEqualLines(t, sortedExpectedLines, actualLines)
+			wasFileAltered, err := creator.RunCommandOnceWithFile("ipset", "restore")
+			require.NoError(t, err, "ipset restore should be successful")
+			require.False(t, wasFileAltered, "file should not be altered")
+		})
+	}
+}
+
+// no save file involved
+func TestDeleteMembers(t *testing.T) {
+	calls := []testutils.TestCmd{
+		fakeRestoreSuccessCommand,
+	}
+	ioshim := common.NewMockIOShim(calls)
+	defer ioshim.VerifyCalls(t, calls)
+	iMgr := NewIPSetManager(applyAlwaysCfg, ioshim)
+	require.NoError(t, iMgr.AddToSets([]*IPSetMetadata{TestNSSet.Metadata}, "1.1.1.1", "a"))
+	require.NoError(t, iMgr.AddToSets([]*IPSetMetadata{TestNSSet.Metadata}, "2.2.2.2", "b"))
+	require.NoError(t, iMgr.AddToSets([]*IPSetMetadata{TestNSSet.Metadata}, "3.3.3.3", "c"))
+	// create to destroy later
+	iMgr.CreateIPSets([]*IPSetMetadata{TestCIDRSet.Metadata})
+	// clear dirty cache, otherwise a set deletion will be a no-op
+	iMgr.clearDirtyCache()
+
+	// will remove this member
+	require.NoError(t, iMgr.RemoveFromSets([]*IPSetMetadata{TestNSSet.Metadata}, "1.1.1.1", "a"))
+	// will add this member
+	require.NoError(t, iMgr.AddToSets([]*IPSetMetadata{TestNSSet.Metadata}, "5.5.5.5", "e"))
+	// won't add/remove this member since the next two calls cancel each other out
+	require.NoError(t, iMgr.AddToSets([]*IPSetMetadata{TestNSSet.Metadata}, "4.4.4.4", "d"))
+	require.NoError(t, iMgr.RemoveFromSets([]*IPSetMetadata{TestNSSet.Metadata}, "4.4.4.4", "d"))
+	// won't add/remove this member since the next two calls cancel each other out
+	require.NoError(t, iMgr.RemoveFromSets([]*IPSetMetadata{TestNSSet.Metadata}, "2.2.2.2", "b"))
+	require.NoError(t, iMgr.AddToSets([]*IPSetMetadata{TestNSSet.Metadata}, "2.2.2.2", "b"))
+	// destroy extra set
+	iMgr.DeleteIPSet(TestCIDRSet.PrefixName, util.SoftDelete)
+
+	expectedLines := []string{
+		fmt.Sprintf("-N %s --exist nethash", TestNSSet.HashedName),
+		fmt.Sprintf("-D %s 1.1.1.1", TestNSSet.HashedName),
+		fmt.Sprintf("-A %s 5.5.5.5", TestNSSet.HashedName),
+		fmt.Sprintf("-F %s", TestCIDRSet.HashedName),
+		fmt.Sprintf("-X %s", TestCIDRSet.HashedName),
+		"",
+	}
+	sortedExpectedLines := testAndSortRestoreFileLines(t, expectedLines)
+	creator := iMgr.fileCreatorForApply(len(calls))
+	actualLines := testAndSortRestoreFileString(t, creator.ToString())
 	dptestutils.AssertEqualLines(t, sortedExpectedLines, actualLines)
 	wasFileAltered, err := creator.RunCommandOnceWithFile("ipset", "restore")
 	require.NoError(t, err, "ipset restore should be successful")
@@ -684,7 +794,7 @@ func TestUpdateWithIdenticalSaveFile(t *testing.T) {
 	require.NoError(t, iMgr.AddToLists([]*IPSetMetadata{TestKVNSList.Metadata}, []*IPSetMetadata{TestKVPodSet.Metadata}))
 	iMgr.CreateIPSets([]*IPSetMetadata{TestNestedLabelList.Metadata})
 
-	creator := iMgr.fileCreatorForApply(len(calls), saveFileBytes)
+	creator := iMgr.fileCreatorForApplyWithSaveFile(len(calls), saveFileBytes)
 	actualLines := testAndSortRestoreFileString(t, creator.ToString())
 
 	expectedLines := []string{
@@ -733,6 +843,10 @@ func TestUpdateWithRealisticSaveFile(t *testing.T) {
 	saveFileString := strings.Join(saveFileLines, "\n")
 	saveFileBytes := []byte(saveFileString)
 
+	iMgr.CreateIPSets([]*IPSetMetadata{TestNestedLabelList.Metadata}) // create so we can delete
+	// clear dirty cache, otherwise a set deletion will be a no-op
+	iMgr.clearDirtyCache()
+
 	require.NoError(t, iMgr.AddToSets([]*IPSetMetadata{TestNSSet.Metadata}, "10.0.0.0", "a"))
 	require.NoError(t, iMgr.AddToSets([]*IPSetMetadata{TestNSSet.Metadata}, "10.0.0.1", "b"))
 	require.NoError(t, iMgr.AddToSets([]*IPSetMetadata{TestNSSet.Metadata}, "10.0.0.2", "c"))
@@ -743,10 +857,9 @@ func TestUpdateWithRealisticSaveFile(t *testing.T) {
 	require.NoError(t, iMgr.AddToLists([]*IPSetMetadata{TestKeyNSList.Metadata}, []*IPSetMetadata{TestNSSet.Metadata, TestKeyPodSet.Metadata}))
 	iMgr.CreateIPSets([]*IPSetMetadata{TestKVNSList.Metadata})
 	require.NoError(t, iMgr.AddToSets([]*IPSetMetadata{TestCIDRSet.Metadata}, "1.2.3.4", "z")) // set not in save file
-	iMgr.CreateIPSets([]*IPSetMetadata{TestNestedLabelList.Metadata})                          // create so we can delete
 	iMgr.DeleteIPSet(TestNestedLabelList.PrefixName, util.SoftDelete)
 
-	creator := iMgr.fileCreatorForApply(len(calls), saveFileBytes)
+	creator := iMgr.fileCreatorForApplyWithSaveFile(len(calls), saveFileBytes)
 	actualLines := testAndSortRestoreFileString(t, creator.ToString()) // adding NSSet and KeyPodSet (should be keeping NSSet and deleting NamedportSet)
 
 	expectedLines := []string{
@@ -1007,7 +1120,7 @@ func TestUpdateWithBadSaveFile(t *testing.T) {
 
 			iMgr.CreateIPSets(tt.args.dirtySet)
 
-			creator := iMgr.fileCreatorForApply(len(calls), saveFileBytes)
+			creator := iMgr.fileCreatorForApplyWithSaveFile(len(calls), saveFileBytes)
 			actualLines := testAndSortRestoreFileString(t, creator.ToString())
 			sortedExpectedLines := testAndSortRestoreFileLines(t, tt.expectedLines)
 
@@ -1020,6 +1133,16 @@ func TestUpdateWithBadSaveFile(t *testing.T) {
 }
 
 func TestFailureOnCreateForNewSet(t *testing.T) {
+	tests := []struct {
+		name         string
+		withSaveFile bool
+	}{
+		{name: "with save file", withSaveFile: true},
+		{name: "no save file", withSaveFile: false},
+	}
+	for _, tt := range tests {
+		tt := tt
+		t.Run(tt.name, func(t *testing.T) {
 			// with respect to the error line, be weary that sets in the save file are processed first and in order, and other sets are processed in random order
 			// test logic:
 			// - delete a set
@@ -1046,7 +1169,12 @@ func TestFailureOnCreateForNewSet(t *testing.T) {
 			iMgr.DeleteIPSet(TestKeyNSList.PrefixName, util.SoftDelete)
 
 			// get original creator and run it the first time
-	creator := iMgr.fileCreatorForApply(len(calls), nil)
+			var creator *ioutil.FileCreator
+			if tt.withSaveFile {
+				creator = iMgr.fileCreatorForApplyWithSaveFile(len(calls), nil)
+			} else {
+				creator = iMgr.fileCreatorForApply(len(calls))
+			}
 			originalLines := strings.Split(creator.ToString(), "\n")
 			wasFileAltered, err := creator.RunCommandOnceWithFile("ipset", "restore")
 			require.Error(t, err, "ipset restore should fail")
@@ -1066,6 +1194,8 @@ func TestFailureOnCreateForNewSet(t *testing.T) {
 			wasFileAltered, err = creator.RunCommandOnceWithFile("ipset", "restore")
 			require.NoError(t, err)
 			require.False(t, wasFileAltered, "file should not be altered")
+		})
+	}
 }
 
 func TestFailureOnCreateForSetInKernel(t *testing.T) {
@@ -1103,7 +1233,7 @@ func TestFailureOnCreateForSetInKernel(t *testing.T) {
 	iMgr.DeleteIPSet(TestKeyNSList.PrefixName, util.SoftDelete)
 
 	// get original creator and run it the first time
-	creator := iMgr.fileCreatorForApply(len(calls), saveFileBytes)
+	creator := iMgr.fileCreatorForApplyWithSaveFile(len(calls), saveFileBytes)
 	originalLines := strings.Split(creator.ToString(), "\n")
 	wasFileAltered, err := creator.RunCommandOnceWithFile("ipset", "restore")
 	require.Error(t, err, "ipset restore should fail")
@@ -1163,7 +1293,7 @@ func TestFailureOnAddToListInKernel(t *testing.T) {
 	iMgr.CreateIPSets([]*IPSetMetadata{TestCIDRSet.Metadata})                                                                     // create so we can delete
 	iMgr.DeleteIPSet(TestCIDRSet.PrefixName, util.SoftDelete)
 
-	creator := iMgr.fileCreatorForApply(len(calls), saveFileBytes)
+	creator := iMgr.fileCreatorForApplyWithSaveFile(len(calls), saveFileBytes)
 	originalLines := strings.Split(creator.ToString(), "\n")
 	wasFileAltered, err := creator.RunCommandOnceWithFile("ipset", "restore")
 	require.Error(t, err, "ipset restore should fail")
@@ -1215,7 +1345,7 @@ func TestFailureOnAddToNewList(t *testing.T) {
 	iMgr.CreateIPSets([]*IPSetMetadata{TestCIDRSet.Metadata})                                                                 // create so we can delete
 	iMgr.DeleteIPSet(TestCIDRSet.PrefixName, util.SoftDelete)
 
-	creator := iMgr.fileCreatorForApply(len(calls), saveFileBytes)
+	creator := iMgr.fileCreatorForApplyWithSaveFile(len(calls), saveFileBytes)
 	originalLines := strings.Split(creator.ToString(), "\n")
 	wasFileAltered, err := creator.RunCommandOnceWithFile("ipset", "restore")
 	require.Error(t, err, "ipset restore should fail")
@@ -1264,14 +1394,17 @@ func TestFailureOnFlush(t *testing.T) {
 	saveFileString := strings.Join(saveFileLines, "\n")
 	saveFileBytes := []byte(saveFileString)
 
+	iMgr.CreateIPSets([]*IPSetMetadata{TestCIDRSet.Metadata})  // create so we can delete
+	iMgr.CreateIPSets([]*IPSetMetadata{TestKVPodSet.Metadata}) // create so we can delete
+	// clear dirty cache, otherwise a set deletion will be a no-op
+	iMgr.clearDirtyCache()
+
 	require.NoError(t, iMgr.AddToSets([]*IPSetMetadata{TestNSSet.Metadata}, "10.0.0.0", "a"))     // in kernel already
 	require.NoError(t, iMgr.AddToSets([]*IPSetMetadata{TestKeyPodSet.Metadata}, "10.0.0.0", "a")) // not in kernel yet
-	iMgr.CreateIPSets([]*IPSetMetadata{TestKVPodSet.Metadata})                                    // create so we can delete
 	iMgr.DeleteIPSet(TestKVPodSet.PrefixName, util.SoftDelete)
-	iMgr.CreateIPSets([]*IPSetMetadata{TestCIDRSet.Metadata}) // create so we can delete
 	iMgr.DeleteIPSet(TestCIDRSet.PrefixName, util.SoftDelete)
 
-	creator := iMgr.fileCreatorForApply(len(calls), saveFileBytes)
+	creator := iMgr.fileCreatorForApplyWithSaveFile(len(calls), saveFileBytes)
 	originalLines := strings.Split(creator.ToString(), "\n")
 	wasFileAltered, err := creator.RunCommandOnceWithFile("ipset", "restore")
 	require.Error(t, err, "ipset restore should fail")
@@ -1317,14 +1450,17 @@ func TestFailureOnDestroy(t *testing.T) {
 	saveFileString := strings.Join(saveFileLines, "\n")
 	saveFileBytes := []byte(saveFileString)
 
+	iMgr.CreateIPSets([]*IPSetMetadata{TestCIDRSet.Metadata})  // create so we can delete
+	iMgr.CreateIPSets([]*IPSetMetadata{TestKVPodSet.Metadata}) // create so we can delete
+	// clear dirty cache, otherwise a set deletion will be a no-op
+	iMgr.clearDirtyCache()
+
 	require.NoError(t, iMgr.AddToSets([]*IPSetMetadata{TestNSSet.Metadata}, "10.0.0.0", "a"))     // in kernel already
 	require.NoError(t, iMgr.AddToSets([]*IPSetMetadata{TestKeyPodSet.Metadata}, "10.0.0.0", "a")) // not in kernel yet
-	iMgr.CreateIPSets([]*IPSetMetadata{TestKVPodSet.Metadata})                                    // create so we can delete
 	iMgr.DeleteIPSet(TestKVPodSet.PrefixName, util.SoftDelete)
-	iMgr.CreateIPSets([]*IPSetMetadata{TestCIDRSet.Metadata}) // create so we can delete
 	iMgr.DeleteIPSet(TestCIDRSet.PrefixName, util.SoftDelete)
 
-	creator := iMgr.fileCreatorForApply(len(calls), saveFileBytes)
+	creator := iMgr.fileCreatorForApplyWithSaveFile(len(calls), saveFileBytes)
 	originalLines := strings.Split(creator.ToString(), "\n")
 	wasFileAltered, err := creator.RunCommandOnceWithFile("ipset", "restore")
 	require.Error(t, err, "ipset restore should fail")
@@ -1343,6 +1479,16 @@ func TestFailureOnDestroy(t *testing.T) {
 }
 
 func TestFailureOnLastLine(t *testing.T) {
+	tests := []struct {
+		name         string
+		withSaveFile bool
+	}{
+		{name: "with save file", withSaveFile: true},
+		{name: "no save file", withSaveFile: false},
+	}
+	for _, tt := range tests {
+		tt := tt
+		t.Run(tt.name, func(t *testing.T) {
 			// make sure that the file recovers and returns no error when there are no more lines on the second run
 			// test logic:
 			// - delete a set
@@ -1359,9 +1505,17 @@ func TestFailureOnLastLine(t *testing.T) {
 			iMgr := NewIPSetManager(applyAlwaysCfg, ioshim)
 
 			iMgr.CreateIPSets([]*IPSetMetadata{TestCIDRSet.Metadata}) // create so we can delete
+			// clear dirty cache, otherwise a set deletion will be a no-op
+			iMgr.clearDirtyCache()
+
 			iMgr.DeleteIPSet(TestCIDRSet.PrefixName, util.SoftDelete)
 
-	creator := iMgr.fileCreatorForApply(2, nil)
+			var creator *ioutil.FileCreator
+			if tt.withSaveFile {
+				creator = iMgr.fileCreatorForApplyWithSaveFile(2, nil)
+			} else {
+				creator = iMgr.fileCreatorForApply(2)
+			}
 			wasFileAltered, err := creator.RunCommandOnceWithFile("ipset", "restore")
 			require.Error(t, err, "ipset restore should fail")
 			require.True(t, wasFileAltered, "file should be altered")
@@ -1372,6 +1526,8 @@ func TestFailureOnLastLine(t *testing.T) {
 			wasFileAltered, err = creator.RunCommandOnceWithFile("ipset", "restore")
 			require.NoError(t, err)
 			require.False(t, wasFileAltered, "file should not be altered")
+		})
+	}
 }
 
 func testAndSortRestoreFileString(t *testing.T, multilineString string) []string {

npm/pkg/dataplane/ipsets/ipsetmanager_test.go

@@ -404,6 +404,53 @@ func TestDeleteIPSet(t *testing.T) {
 	}
 }
 
+func TestDeleteAfterCreate(t *testing.T) {
+	metrics.ReinitializeAll()
+	calls := []testutils.TestCmd{}
+	ioShim := common.NewMockIOShim(calls)
+	defer ioShim.VerifyCalls(t, calls)
+	iMgr := NewIPSetManager(applyOnNeedCfg, ioShim)
+
+	setMetadata := NewIPSetMetadata(testSetName, Namespace)
+	iMgr.CreateIPSets([]*IPSetMetadata{setMetadata})
+	iMgr.DeleteIPSet(setMetadata.GetPrefixName(), util.SoftDelete)
+	assertExpectedInfo(t, iMgr, &expectedInfo{})
+}
+
+func TestCreateAfterHardDelete(t *testing.T) {
+	metrics.ReinitializeAll()
+	calls := []testutils.TestCmd{}
+	ioShim := common.NewMockIOShim(calls)
+	defer ioShim.VerifyCalls(t, calls)
+	iMgr := NewIPSetManager(applyAlwaysCfg, ioShim)
+
+	setMetadata := NewIPSetMetadata(testSetName, Namespace)
+	require.NoError(t, iMgr.AddToSets([]*IPSetMetadata{setMetadata}, "1.2.3.4", "pod-a"))
+	// clear dirty cache, otherwise a set deletion will be a no-op
+	iMgr.clearDirtyCache()
+
+	numIPSetsInCache, _ := metrics.GetNumIPSets()
+	fmt.Println(numIPSetsInCache)
+
+	iMgr.DeleteIPSet(setMetadata.GetPrefixName(), util.ForceDelete)
+	numIPSetsInCache, _ = metrics.GetNumIPSets()
+	fmt.Println(numIPSetsInCache)
+
+	assertExpectedInfo(t, iMgr, &expectedInfo{
+		toDeleteCache: []string{setMetadata.GetPrefixName()},
+	})
+
+	iMgr.CreateIPSets([]*IPSetMetadata{setMetadata})
+	assertExpectedInfo(t, iMgr, &expectedInfo{
+		mainCache: []setMembers{
+			{
+				metadata: setMetadata,
+			},
+		},
+		toAddUpdateCache: []*IPSetMetadata{setMetadata},
+	})
+}
+
 func TestDeleteIPSetNotAllowed(t *testing.T) {
 	// try to delete a list with a member and a set referenced in kernel (by a list)
 	// must use applyAlwaysCfg for set to be in kernel
@@ -1392,26 +1439,23 @@ func assertExpectedInfo(t *testing.T, iMgr *IPSetManager, info *expectedInfo) {
 	}
 
 	// 1.2. make sure the toAddOrUpdateCache is equal
-	require.Equal(t, len(info.toAddUpdateCache), len(iMgr.toAddOrUpdateCache), "toAddUpdateCache size mismatch")
+	require.Equal(t, len(info.toAddUpdateCache), iMgr.dirtyCache.numSetsToAddOrUpdate(), "toAddUpdateCache size mismatch")
 	for _, setMetadata := range info.toAddUpdateCache {
 		setName := setMetadata.GetPrefixName()
-		_, ok := iMgr.toAddOrUpdateCache[setName]
-		require.True(t, ok, "set %s not in the toAddUpdateCache")
+		require.True(t, iMgr.dirtyCache.isSetToAddOrUpdate(setName), "set %s not in the toAddUpdateCache")
 		require.True(t, iMgr.exists(setName), "set %s not in the main cache but is in the toAddUpdateCache", setName)
 	}
 
 	// 1.3. make sure the toDeleteCache is equal
-	require.Equal(t, len(info.toDeleteCache), len(iMgr.toDeleteCache), "toDeleteCache size mismatch")
+	require.Equal(t, len(info.toDeleteCache), iMgr.dirtyCache.numSetsToDelete(), "toDeleteCache size mismatch")
 	for _, setName := range info.toDeleteCache {
-		_, ok := iMgr.toDeleteCache[setName]
-		require.True(t, ok, "set %s not found in toDeleteCache", setName)
+		require.True(t, iMgr.dirtyCache.isSetToDelete(setName), "set %s not found in toDeleteCache", setName)
 	}
 
 	// 1.4. assert kernel status of sets in the toAddOrUpdateCache
 	for _, setMetadata := range info.setsForKernel {
 		// check semantics
-		_, ok := iMgr.toAddOrUpdateCache[setMetadata.GetPrefixName()]
-		require.True(t, ok, "setsForKernel should be a subset of toAddUpdateCache")
+		require.True(t, iMgr.dirtyCache.isSetToAddOrUpdate(setMetadata.GetPrefixName()), "setsForKernel should be a subset of toAddUpdateCache")
 
 		setName := setMetadata.GetPrefixName()
 		require.True(t, iMgr.exists(setName), "kernel set %s not found", setName)
@@ -1430,7 +1474,7 @@ func assertExpectedInfo(t *testing.T, iMgr *IPSetManager, info *expectedInfo) {
 	// TODO update get function when we have prometheus metric for in kernel
 	numIPSetsInCache, err := metrics.GetNumIPSets()
 	promutil.NotifyIfErrors(t, err)
-	require.Equal(t, len(iMgr.setMap), numIPSetsInCache, "numIPSetsInCache mismatch")
+	require.Equal(t, len(iMgr.setMap), numIPSetsInCache, "num ipsets mismatch")
 
 	// the setMap is equal
 	expectedNumEntriesInCache := 0
@@ -1452,7 +1496,7 @@ func assertExpectedInfo(t *testing.T, iMgr *IPSetManager, info *expectedInfo) {
 	// TODO update get func when we have prometheus metric for in kernel
 	numEntriesInCache, err := metrics.GetNumIPSetEntries()
 	promutil.NotifyIfErrors(t, err)
-	require.Equal(t, expectedNumEntriesInCache, numEntriesInCache)
+	require.Equal(t, expectedNumEntriesInCache, numEntriesInCache, "incorrect num ipset entries")
 	for _, set := range iMgr.setMap {
 		expectedNumEntries := 0
 		// TODO replace bool with iMgr.shouldBeInKernel(set) when we have prometheus metric for in kernel

npm/pkg/dataplane/ipsets/ipsetmanager_windows.go

@@ -124,7 +124,7 @@ func (iMgr *IPSetManager) applyIPSets() error {
 		}
 	}
 
-	iMgr.toAddOrUpdateCache = make(map[string]struct{})
+	iMgr.dirtyCache.resetAddOrUpdateCache()
 
 	if len(setPolicyBuilder.toDeleteSets) > 0 {
 		err = iMgr.modifySetPolicies(network, hcn.RequestTypeRemove, setPolicyBuilder.toDeleteSets)
@@ -157,7 +157,7 @@ func (iMgr *IPSetManager) calculateNewSetPolicies(networkPolicies []hcn.NetworkP
 	}
 	existingSets, toDeleteSets := iMgr.segregateSetPolicies(networkPolicies, donotResetIPSets)
 	// some of this below logic can be abstracted a step above
-	toAddUpdateSetNames := iMgr.toAddOrUpdateCache
+	toAddUpdateSetNames := iMgr.dirtyCache.setsToAddOrUpdate()
 	setPolicyBuilder.toDeleteSets = toDeleteSets
 
 	// for faster look up changing a slice to map
@@ -276,7 +276,7 @@ func (iMgr *IPSetManager) segregateSetPolicies(networkPolicies []hcn.NetworkPoli
 		if !strings.HasPrefix(set.Id, util.AzureNpmPrefix) {
 			continue
 		}
-		_, ok := iMgr.toDeleteCache[set.Name]
+		ok := iMgr.dirtyCache.isSetToDelete(set.Name)
 		if !ok && !reset {
 			// if the set is not in delete cache, go ahead and add it to update cache
 			toUpdateSets = append(toUpdateSets, set.Name)

npm/pkg/dataplane/ipsets/testutils_linux.go

@@ -14,13 +14,7 @@ var (
 )
 
 func GetApplyIPSetsTestCalls(toAddOrUpdateIPSets, toDeleteIPSets []*IPSetMetadata) []testutils.TestCmd {
-	if len(toAddOrUpdateIPSets) > 0 {
-		return []testutils.TestCmd{
-			{Cmd: ipsetSaveStringSlice, PipedToCommand: true},
-			{Cmd: []string{"grep", "azure-npm-"}, ExitCode: 1}, // grep didn't find anything
-			{Cmd: ipsetRestoreStringSlice},
-		}
-	} else if len(toDeleteIPSets) == 0 {
+	if len(toAddOrUpdateIPSets) == 0 && len(toDeleteIPSets) == 0 {
 		return []testutils.TestCmd{}
 	}
 	return []testutils.TestCmd{fakeRestoreSuccessCommand}