Skip to content

Commit 508a2bb

Browse files
neaggarwMSneaggarwMS
authored
Changes for caching pod ip (#600)
* Changes for cahing pod ip * Test fix for API changes * added test * Fixed merge conflicts * Add tests for pod cache * Add one more check to validate the cache * Incorporated the comment Co-authored-by: neaggarw <[email protected]>
1 parent 14e8a98 commit 508a2bb

File tree

7 files changed

+190
-65
lines changed

7 files changed

+190
-65
lines changed

‎go.mod‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,6 @@ require (
1212
github.com/docker/libnetwork v0.5.6
1313
github.com/golang/groupcache v0.0.0-20191027212112-611e8accdfc9 // indirect
1414
github.com/google/uuid v1.1.1
15-
github.com/googleapis/gnostic v0.3.1 // indirect
1615
github.com/gregjones/httpcache v0.0.0-20190611155906-901d90724c79 // indirect
1716
github.com/hashicorp/golang-lru v0.5.3 // indirect
1817
github.com/imdario/mergo v0.3.8 // indirect

‎npm/ipsm/ipsm.go‎

Lines changed: 40 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -28,14 +28,15 @@ type IpsetManager struct {
2828
// Ipset represents one ipset entry.
2929
type Ipset struct {
3030
name string
31-
elements []string
31+
elements map[string]string // key = ip, value: context associated to the ip like podUid
3232
referCount int
3333
}
3434

3535
// NewIpset creates a new instance for Ipset object.
3636
func NewIpset(setName string) *Ipset {
3737
return &Ipset{
38-
name: setName,
38+
name: setName,
39+
elements: make(map[string]string),
3940
}
4041
}
4142

@@ -58,13 +59,11 @@ func (ipsMgr *IpsetManager) Exists(key string, val string, kind string) bool {
5859
return false
5960
}
6061

61-
for _, elem := range m[key].elements {
62-
if elem == val {
63-
return true
64-
}
62+
if _, exists := m[key].elements[val]; !exists {
63+
return false
6564
}
6665

67-
return false
66+
return true
6867
}
6968

7069
func isNsSet(setName string) bool {
@@ -140,7 +139,7 @@ func (ipsMgr *IpsetManager) AddToList(listName string, setName string) error {
140139
return err
141140
}
142141

143-
ipsMgr.listMap[listName].elements = append(ipsMgr.listMap[listName].elements, setName)
142+
ipsMgr.listMap[listName].elements[setName] = ""
144143

145144
return nil
146145
}
@@ -152,12 +151,6 @@ func (ipsMgr *IpsetManager) DeleteFromList(listName string, setName string) erro
152151
return nil
153152
}
154153

155-
for i, val := range ipsMgr.listMap[listName].elements {
156-
if val == setName {
157-
ipsMgr.listMap[listName].elements = append(ipsMgr.listMap[listName].elements[:i], ipsMgr.listMap[listName].elements[i+1:]...)
158-
}
159-
}
160-
161154
hashedListName, hashedSetName := util.GetHashedName(listName), util.GetHashedName(setName)
162155
entry := &ipsEntry{
163156
operationFlag: util.IpsetDeletionFlag,
@@ -170,6 +163,11 @@ func (ipsMgr *IpsetManager) DeleteFromList(listName string, setName string) erro
170163
return err
171164
}
172165

166+
// Now cleanup the cache
167+
if _, exists := ipsMgr.listMap[listName].elements[setName]; exists {
168+
delete(ipsMgr.listMap[listName].elements, setName)
169+
}
170+
173171
if len(ipsMgr.listMap[listName].elements) == 0 {
174172
if err := ipsMgr.DeleteList(listName); err != nil {
175173
log.Errorf("Error: failed to delete ipset list %s.", listName)
@@ -231,8 +229,18 @@ func (ipsMgr *IpsetManager) DeleteSet(setName string) error {
231229
}
232230

233231
// AddToSet inserts an ip to an entry in setMap, and creates/updates the corresponding ipset.
234-
func (ipsMgr *IpsetManager) AddToSet(setName, ip, spec string) error {
232+
func (ipsMgr *IpsetManager) AddToSet(setName, ip, spec, podUid string) error {
235233
if ipsMgr.Exists(setName, ip, spec) {
234+
235+
// make sure we have updated the podUid in case it gets changed
236+
cachedPodUid := ipsMgr.setMap[setName].elements[ip]
237+
if cachedPodUid != podUid {
238+
log.Logf("AddToSet: PodOwner has changed for Ip: %s, setName:%s, Old podUid: %s, new PodUid: %s. Replace context with new PodOwner.",
239+
ip, setName, cachedPodUid, podUid)
240+
241+
ipsMgr.setMap[setName].elements[ip] = podUid
242+
}
243+
236244
return nil
237245
}
238246

@@ -258,24 +266,32 @@ func (ipsMgr *IpsetManager) AddToSet(setName, ip, spec string) error {
258266
return err
259267
}
260268

261-
ipsMgr.setMap[setName].elements = append(ipsMgr.setMap[setName].elements, ip)
269+
// Stores the podUid as the context for this ip.
270+
ipsMgr.setMap[setName].elements[ip] = podUid
262271

263272
return nil
264273
}
265274

266275
// DeleteFromSet removes an ip from an entry in setMap, and delete/update the corresponding ipset.
267-
func (ipsMgr *IpsetManager) DeleteFromSet(setName, ip string) error {
268-
if _, exists := ipsMgr.setMap[setName]; !exists {
276+
func (ipsMgr *IpsetManager) DeleteFromSet(setName, ip, podUid string) error {
277+
ipSet, exists := ipsMgr.setMap[setName]
278+
if !exists {
269279
log.Logf("ipset with name %s not found", setName)
270280
return nil
271281
}
272282

273-
for i, val := range ipsMgr.setMap[setName].elements {
274-
if val == ip {
275-
ipsMgr.setMap[setName].elements = append(ipsMgr.setMap[setName].elements[:i], ipsMgr.setMap[setName].elements[i+1:]...)
283+
if _, exists := ipsMgr.setMap[setName].elements[ip]; exists {
284+
// in case the IP belongs to a new Pod, then ignore this Delete call as this might be stale
285+
cachedPodUid := ipSet.elements[ip]
286+
if cachedPodUid != podUid {
287+
log.Logf("DeleteFromSet: PodOwner has changed for Ip: %s, setName:%s, Old podUid: %s, new PodUid: %s. Ignore the delete as this is stale update",
288+
ip, setName, cachedPodUid, podUid)
289+
290+
return nil
276291
}
277292
}
278293

294+
// TODO optimize to not run this command in case cache has already been updated.
279295
entry := &ipsEntry{
280296
operationFlag: util.IpsetDeletionFlag,
281297
set: util.GetHashedName(setName),
@@ -291,6 +307,9 @@ func (ipsMgr *IpsetManager) DeleteFromSet(setName, ip string) error {
291307
return err
292308
}
293309

310+
// Now cleanup the cache
311+
delete(ipsMgr.setMap[setName].elements, ip)
312+
294313
if len(ipsMgr.setMap[setName].elements) == 0 {
295314
ipsMgr.DeleteSet(setName)
296315
}

‎npm/ipsm/ipsm_test.go‎

Lines changed: 104 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -170,15 +170,55 @@ func TestAddToSet(t *testing.T) {
170170
}
171171
}()
172172

173-
if err := ipsMgr.AddToSet("test-set", "1.2.3.4", util.IpsetNetHashFlag); err != nil {
173+
if err := ipsMgr.AddToSet("test-set", "1.2.3.4", util.IpsetNetHashFlag, ""); err != nil {
174174
t.Errorf("TestAddToSet failed @ ipsMgr.AddToSet")
175175
}
176176

177-
if err := ipsMgr.AddToSet("test-set", "1.2.3.4/nomatch", util.IpsetNetHashFlag); err != nil {
177+
if err := ipsMgr.AddToSet("test-set", "1.2.3.4/nomatch", util.IpsetNetHashFlag, ""); err != nil {
178178
t.Errorf("TestAddToSet with nomatch failed @ ipsMgr.AddToSet")
179179
}
180180
}
181181

182+
func TestAddToSetWithCachePodInfo(t *testing.T) {
183+
ipsMgr := NewIpsetManager()
184+
if err := ipsMgr.Save(util.IpsetTestConfigFile); err != nil {
185+
t.Errorf("TestAddToSetWithCachePodInfo failed @ ipsMgr.Save")
186+
}
187+
188+
defer func() {
189+
if err := ipsMgr.Restore(util.IpsetTestConfigFile); err != nil {
190+
t.Errorf("TestAddToSetWithCachePodInfo failed @ ipsMgr.Restore")
191+
}
192+
}()
193+
194+
var pod1 = "pod1"
195+
var setname = "test-podcache_new"
196+
var ip = "10.0.2.7"
197+
if err := ipsMgr.AddToSet(setname, ip, util.IpsetNetHashFlag, pod1); err != nil {
198+
t.Errorf("TestAddToSetWithCachePodInfo with pod1 failed @ ipsMgr.AddToSet, setname: %s, hashedname: %s", setname, util.GetHashedName(setname))
199+
}
200+
201+
// validate if Pod1 exists
202+
cachedPodUid := ipsMgr.setMap[setname].elements[ip]
203+
if cachedPodUid != pod1 {
204+
t.Errorf("setname: %s, hashedname: %s is added with wrong podUid: %s, expected: %s", setname, util.GetHashedName(setname), cachedPodUid, pod1)
205+
}
206+
207+
// now add pod2 with the same ip. This is possible if DeletePod1 is handled after AddPod2 event callback.
208+
var pod2 = "pod2"
209+
if err := ipsMgr.AddToSet(setname, ip, util.IpsetNetHashFlag, pod2); err != nil {
210+
t.Errorf("TestAddToSetWithCachePodInfo with pod2 failed @ ipsMgr.AddToSet")
211+
}
212+
213+
cachedPodUid = ipsMgr.setMap[setname].elements[ip]
214+
if cachedPodUid != pod2 {
215+
t.Errorf("setname: %s, hashedname: %s is added with wrong podUid: %s, expected: %s", setname, util.GetHashedName(setname), cachedPodUid, pod2)
216+
}
217+
218+
// Delete from set, it will delete the set if this is the last member
219+
ipsMgr.DeleteFromSet(setname, ip, pod2)
220+
}
221+
182222
func TestDeleteFromSet(t *testing.T) {
183223
ipsMgr := NewIpsetManager()
184224
if err := ipsMgr.Save(util.IpsetTestConfigFile); err != nil {
@@ -191,15 +231,15 @@ func TestDeleteFromSet(t *testing.T) {
191231
}
192232
}()
193233

194-
if err := ipsMgr.AddToSet("test-set", "1.2.3.4", util.IpsetNetHashFlag); err != nil {
234+
if err := ipsMgr.AddToSet("test-set", "1.2.3.4", util.IpsetNetHashFlag, ""); err != nil {
195235
t.Errorf("TestDeleteFromSet failed @ ipsMgr.AddToSet")
196236
}
197237

198238
if len(ipsMgr.setMap["test-set"].elements) != 1 {
199239
t.Errorf("TestDeleteFromSet failed @ ipsMgr.AddToSet")
200240
}
201241

202-
if err := ipsMgr.DeleteFromSet("test-set", "1.2.3.4"); err != nil {
242+
if err := ipsMgr.DeleteFromSet("test-set", "1.2.3.4", ""); err != nil {
203243
t.Errorf("TestDeleteFromSet failed @ ipsMgr.DeleteFromSet")
204244
}
205245

@@ -209,6 +249,65 @@ func TestDeleteFromSet(t *testing.T) {
209249
}
210250
}
211251

252+
func TestDeleteFromSetWithPodCache(t *testing.T) {
253+
ipsMgr := NewIpsetManager()
254+
if err := ipsMgr.Save(util.IpsetTestConfigFile); err != nil {
255+
t.Errorf("TestDeleteFromSetWithPodCache failed @ ipsMgr.Save")
256+
}
257+
258+
defer func() {
259+
if err := ipsMgr.Restore(util.IpsetTestConfigFile); err != nil {
260+
t.Errorf("TestDeleteFromSetWithPodCache failed @ ipsMgr.Restore")
261+
}
262+
}()
263+
264+
var setname = "test-deleteset-withcache"
265+
var ip = "10.0.2.8"
266+
var pod1 = "pod1"
267+
if err := ipsMgr.AddToSet(setname, ip, util.IpsetNetHashFlag, pod1); err != nil {
268+
t.Errorf("TestDeleteFromSetWithPodCache failed for pod1 @ ipsMgr.AddToSet")
269+
}
270+
271+
if len(ipsMgr.setMap[setname].elements) != 1 {
272+
t.Errorf("TestDeleteFromSetWithPodCache failed @ ipsMgr.AddToSet")
273+
}
274+
275+
if err := ipsMgr.DeleteFromSet(setname, ip, pod1); err != nil {
276+
t.Errorf("TestDeleteFromSetWithPodCache for pod1 failed @ ipsMgr.DeleteFromSet")
277+
}
278+
279+
// now add the set again and then replace it with pod2
280+
var pod2 = "pod2"
281+
if err := ipsMgr.AddToSet(setname, ip, util.IpsetNetHashFlag, pod1); err != nil {
282+
t.Errorf("TestDeleteFromSetWithPodCache failed for pod1 @ ipsMgr.AddToSet")
283+
}
284+
285+
// Add Pod2 with same ip (This could happen if AddPod2 is served before DeletePod1)
286+
if err := ipsMgr.AddToSet(setname, ip, util.IpsetNetHashFlag, pod2); err != nil {
287+
t.Errorf("TestDeleteFromSetWithPodCache failed for pod2 @ ipsMgr.AddToSet")
288+
}
289+
290+
// Process DeletePod1
291+
if err := ipsMgr.DeleteFromSet(setname, ip, pod1); err != nil {
292+
t.Errorf("TestDeleteFromSetWithPodCache for pod1 failed @ ipsMgr.DeleteFromSet")
293+
}
294+
295+
// note the set will stil exist with pod ip
296+
cachedPodUid := ipsMgr.setMap[setname].elements[ip]
297+
if cachedPodUid != pod2 {
298+
t.Errorf("setname: %s, hashedname: %s is added with wrong podUid: %s, expected: %s", setname, util.GetHashedName(setname), cachedPodUid, pod2)
299+
}
300+
301+
// Now cleanup and delete pod2
302+
if err := ipsMgr.DeleteFromSet(setname, ip, pod2); err != nil {
303+
t.Errorf("TestDeleteFromSetWithPodCache for pod2 failed @ ipsMgr.DeleteFromSet")
304+
}
305+
306+
if _, exists := ipsMgr.setMap[setname]; exists {
307+
t.Errorf("TestDeleteFromSetWithPodCache failed @ ipsMgr.DeleteFromSet")
308+
}
309+
}
310+
212311
func TestClean(t *testing.T) {
213312
ipsMgr := NewIpsetManager()
214313
if err := ipsMgr.Save(util.IpsetTestConfigFile); err != nil {
@@ -242,7 +341,7 @@ func TestDestroy(t *testing.T) {
242341
}
243342
}()
244343

245-
if err := ipsMgr.AddToSet("test-set", "1.2.3.4", util.IpsetNetHashFlag); err != nil {
344+
if err := ipsMgr.AddToSet("test-set", "1.2.3.4", util.IpsetNetHashFlag, ""); err != nil {
246345
t.Errorf("TestDestroy failed @ ipsMgr.AddToSet")
247346
}
248347

‎npm/npm.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ type NetworkPolicyManager struct {
4747

4848
nodeName string
4949
nsMap map[string]*namespace
50-
podMap map[string]bool
50+
podMap map[string]string // Key: Pod uuid, Value: PodIp
5151
isAzureNpmChainCreated bool
5252
isSafeToCleanUpAzureNpmChain bool
5353

@@ -234,7 +234,7 @@ func NewNetworkPolicyManager(clientset *kubernetes.Clientset, informerFactory in
234234
npInformer: npInformer,
235235
nodeName: os.Getenv("HOSTNAME"),
236236
nsMap: make(map[string]*namespace),
237-
podMap: make(map[string]bool),
237+
podMap: make(map[string]string),
238238
isAzureNpmChainCreated: false,
239239
isSafeToCleanUpAzureNpmChain: false,
240240
clusterState: telemetry.ClusterState{

‎npm/nwpolicy.go‎

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -6,8 +6,8 @@ import (
66
"strconv"
77

88
"github.com/Azure/azure-container-networking/log"
9-
"github.com/Azure/azure-container-networking/npm/iptm"
109
"github.com/Azure/azure-container-networking/npm/ipsm"
10+
"github.com/Azure/azure-container-networking/npm/iptm"
1111
"github.com/Azure/azure-container-networking/npm/util"
1212
networkingv1 "k8s.io/api/networking/v1"
1313
)
@@ -66,12 +66,12 @@ func (npMgr *NetworkPolicyManager) AddNetworkPolicy(npObj *networkingv1.NetworkP
6666
}
6767

6868
var (
69-
hashedSelector = HashSelector(&npObj.Spec.PodSelector)
70-
addedPolicy *networkingv1.NetworkPolicy
71-
sets, namedPorts, lists []string
72-
ingressIPCidrs, egressIPCidrs [][]string
73-
iptEntries []*iptm.IptEntry
74-
ipsMgr = allNs.ipsMgr
69+
hashedSelector = HashSelector(&npObj.Spec.PodSelector)
70+
addedPolicy *networkingv1.NetworkPolicy
71+
sets, namedPorts, lists []string
72+
ingressIPCidrs, egressIPCidrs [][]string
73+
iptEntries []*iptm.IptEntry
74+
ipsMgr = allNs.ipsMgr
7575
)
7676

7777
// Remove the existing policy from processed (merged) network policy map
@@ -211,17 +211,17 @@ func createCidrsRule(ingressOrEgress, policyName, ns string, ipsetEntries [][]st
211211
log.Printf("Error creating ipset %s", ipCidrSet)
212212
}
213213
for _, ipCidrEntry := range util.DropEmptyFields(ipCidrSet) {
214-
// Ipset doesn't allow 0.0.0.0/0 to be added. A general solution is split 0.0.0.0/1 in half which convert to
214+
// Ipset doesn't allow 0.0.0.0/0 to be added. A general solution is split 0.0.0.0/1 in half which convert to
215215
// 1.0.0.0/1 and 128.0.0.0/1
216-
if (ipCidrEntry == "0.0.0.0/0") {
216+
if ipCidrEntry == "0.0.0.0/0" {
217217
splitEntry := [2]string{"1.0.0.0/1", "128.0.0.0/1"}
218218
for _, entry := range splitEntry {
219-
if err := ipsMgr.AddToSet(setName, entry, util.IpsetNetHashFlag); err != nil {
219+
if err := ipsMgr.AddToSet(setName, entry, util.IpsetNetHashFlag, ""); err != nil {
220220
log.Printf("Error adding ip cidrs %s into ipset %s", entry, ipCidrSet)
221221
}
222222
}
223223
} else {
224-
if err := ipsMgr.AddToSet(setName, ipCidrEntry, util.IpsetNetHashFlag); err != nil {
224+
if err := ipsMgr.AddToSet(setName, ipCidrEntry, util.IpsetNetHashFlag, ""); err != nil {
225225
log.Printf("Error adding ip cidrs %s into ipset %s", ipCidrEntry, ipCidrSet)
226226
}
227227
}
@@ -240,4 +240,4 @@ func removeCidrsRule(ingressOrEgress, policyName, ns string, ipsetEntries [][]st
240240
log.Printf("Error deleting ipset %s", ipCidrSet)
241241
}
242242
}
243-
}
243+
}

0 commit comments

Comments
 (0)