[NPM] better error handling and cache building #848

vakalapa · 2021-04-02T23:02:14Z

Today, during a resource event handling, NPM will return immediately if it encounters an error and local NPM cache will not be updated. This results in leaked ipset/iptables state in kernel which will never be cleaned up since the local NPM cache is not aware of it.

Solution:
Build cache after each successful operation. Return on first error.

This PR, incorporates the above logic, NPM now keeps updating the local cache after every successful operation. NPM also ignores the notexists errors in Delete set operations.
Logging is updated to use klog instead of using acn/log.
if the Cached Pod IP changes, now NPM will delete the existing cached pod and adds a new pod.
CleanUpDeletedPod will now only require the key of the pod which needs to be deleted.

codecov · 2021-04-02T23:10:04Z

Codecov Report

Merging #848 (ced4ab8) into master (85cd950) will decrease coverage by 0.16%.
The diff coverage is 63.90%.

@@            Coverage Diff             @@
##           master     #848      +/-   ##
==========================================
- Coverage   42.22%   42.05%   -0.17%     
==========================================
  Files         158      158              
  Lines       15025    15136     +111     
==========================================
+ Hits         6344     6366      +22     
- Misses       7904     7995      +91     
+ Partials      777      775       -2

npm/ipsm/ipsm.go

npm/nameSpaceController.go

JungukCho · 2021-04-07T19:10:55Z

npm/nameSpaceController.go

 			metrics.SendErrorLogAndMetric(util.NSID, "[UpdateNamespace] Error: failed to add namespace %s to ipset list %s with err: %v", newNsName, labelKey, err)
 			return err
 		}
+		// {IMPORTANT} Same as above order is assumed to be key and then key+val. NPM should only append to existing labels


Same. Can we resolve this dependency?

As we discussed offline, keeping this as it is for now.

JungukCho · 2021-04-07T19:27:14Z

npm/podController.go

-		log.Logf("Adding pod %s to ipset %s", newPodObj.Status.PodIP, addIPSetName)
-		if err = ipsMgr.AddToSet(addIPSetName, newPodObj.Status.PodIP, util.IpsetNetHashFlag, podKey); err != nil {
-			return fmt.Errorf("[syncAddAndUpdatePod] Error: failed to add pod to label ipset with err: %v", err)
+		// {IMPORTANT} The order of compared list will be key and then key+val. NPM should only append after both key


(Same as namespace comment) Can we resolve this issue in this PR? As I understood this issue and we discussed this before, but if possible, it is better to resolve this issue to avoid future possible issue.
Current GetLabelKVFromSet returns key and value. So better to returns (string, string) in the GetLabelKVFromSet
function and use "removedKey" and "removedValue" in this function

JungukCho · 2021-04-07T19:27:47Z

npm/podController.go

+		if err = ipsMgr.AddToSet(addIPSetName, newPodObj.Status.PodIP, util.IpsetNetHashFlag, podKey); err != nil {
+			return fmt.Errorf("[syncAddAndUpdatePod] Error: failed to add pod to label ipset with err: %v", err)
+		}
+		// {IMPORTANT} Same as above order is assumed to be key and then key+val. NPM should only append to existing labels


Can we resolve this issue in this PR?

JungukCho · 2021-04-07T19:29:53Z

npm/podController.go

+			c.npMgr.PodMap[podKey].appendLabels(map[string]string{addedLabel[0]: addedLabel[1]}, AppendToExitingLabels)
+		}
+	}
+	// This will ensure after all labels are worked on to overwrite. This way will reduce any bugs introduced above


What are possible bugs? It would be nice to update bugs case as comment.

I added a comment. Due to the ordering issues if we not delete and add a label, this appedlabels will help override it.

npm/nameSpaceController.go

npm/podController.go

JungukCho · 2021-04-12T19:22:25Z

npm/podController.go

+	// the IP addresses of the cached npmPod and newPodObj is the same
+	// If no change in labels, then GetIPSetListCompareLabels will return empty list.
+	// Otherwise it returns list of deleted PodIP from cached pod's labels and list of added PodIp from new pod's labels
+	addToIPSets, deleteFromIPSets := util.GetIPSetListCompareLabels(cachedNpmPodObj.Labels, newPodObj.Labels)


From here, do we deal with the first case? If so, can we keep using cachedNpmPodObj and update the cachedNpmPodObj if needed? Are there specific reasons to create newNpmPod() and update cache, etc?

the reason we are creating new obj is because we have fields like Phase, PodIPs, Rv etc which needs to be updated to reflect the latest newPodobj ... we can add them individually too if it makes sense

Good improvement, adding this change

npm/podController.go

JungukCho · 2021-04-12T23:42:53Z

npm/podController.go

+	portList := getContainerPortList(podObj)
+	// To remove any references to objects sent to events by sharedinformer,
+	// NPM will need to deepcopy data from the provided objects
+	copiedPortList := make([]corev1.ContainerPort, len(portList))


getContainerPortList returns new slices with copy. Why do we need the same operation again?

npm/podController.go

JungukCho · 2021-04-14T16:08:46Z

npm/podController.go

-		// Delete the pod from its namespace's ipset.
-		if err = ipsMgr.DeleteFromSet(cachedNpmPodObj.Namespace, cachedNpmPodObj.PodIP, podKey); err != nil {
-			return fmt.Errorf("[syncAddAndUpdatePod] Error: failed to delete pod from namespace ipset with err: %v", err)
+		metrics.SendErrorLogAndMetric(util.PodID, "[syncAddAndUpdatePod] Info: Unexpected state. Pod (Namespace:%s, Name:%s, newUid:%s) , has cachedPodIp:%s which is different from PodIp:%s",


I guess we do not need this message since it can happen.

this line was very useful in Genesys debugging, we would want to see why the delete and add happened for any new folks.

JungukCho · 2021-04-14T16:15:52Z

npm/podController.go

 	podObj, ok := obj.(*corev1.Pod)
 	if !ok {
 		metrics.SendErrorLogAndMetric(util.NpmID, "ADD Pod: Received unexpected object type: %v", obj)
 		return key, needSync


About metrics.SEndErrorLogAndMetric().
I think we do not need send this information since it can be ignorable. If we need to keep it, may need to change "util.NpmID" -> util.PodID". Same opinion for metrics.SEndErrorLogAndMetric() in this function.

It would be nice to check this in the rest of code (e.g., deletePod(), it uses "util.NpmID" and is better to use ""util.PodID" ).

good catch changed this in 3 locations.

JungukCho

Thank you for applying comments! Looks good to me.

[NPM] better error handling and cache building

dab1f5e

vakalapa requested review from JungukCho and matmerr April 2, 2021 23:02

vakalapa added 4 commits April 2, 2021 16:11

using const for label operations

c96f44e

tests correction to discard non-exists delete op

4934655

cleaning up some stale code

ea6a163

fixing some port issues

0761667

JungukCho reviewed Apr 7, 2021

View reviewed changes

npm/ipsm/ipsm.go Show resolved Hide resolved

JungukCho suggested changes Apr 7, 2021

View reviewed changes

vakalapa added 2 commits April 8, 2021 14:40

Addressing some comments

7df3b63

Addressing some comments

0bf4e2b

JungukCho reviewed Apr 9, 2021

View reviewed changes

npm/podController.go Outdated Show resolved Hide resolved

Addressing some comments

f72d3f4

JungukCho reviewed Apr 9, 2021

View reviewed changes

npm/podController.go Outdated Show resolved Hide resolved

vakalapa added 3 commits April 9, 2021 15:38

fixing a missed ns- prefix while deleting from ns ipset

3f64a16

fixing a missed ns- prefix while deleting from ns ipset

0b5f9c6

resducing complexity when pod ip changes

2c74675

JungukCho reviewed Apr 12, 2021

View reviewed changes

npm/podController.go Outdated Show resolved Hide resolved

JungukCho reviewed Apr 12, 2021

View reviewed changes

npm/podController.go Outdated Show resolved Hide resolved

JungukCho reviewed Apr 12, 2021

View reviewed changes

npm/podController.go Outdated Show resolved Hide resolved

JungukCho reviewed Apr 12, 2021

View reviewed changes

npm/podController.go Outdated Show resolved Hide resolved

vakalapa added 3 commits April 12, 2021 15:32

Adding some test validations

08887c8

Adding some test validations

277bb7a

Adding some test validations

ce8c41f

JungukCho reviewed Apr 12, 2021

View reviewed changes

npm/podController.go Outdated Show resolved Hide resolved

JungukCho reviewed Apr 12, 2021

View reviewed changes

npm/podController.go Show resolved Hide resolved

JungukCho reviewed Apr 12, 2021

View reviewed changes

removing more unsed fields

3ec4e3d

vakalapa added 3 commits April 13, 2021 10:00

removing redundant checks

0d1611b

Adding improvement of podinformer

0aa9a6a

removing the deepcopy logic

7a2ac7a

JungukCho reviewed Apr 14, 2021

View reviewed changes

npm/podController.go Outdated Show resolved Hide resolved

JungukCho reviewed Apr 14, 2021

View reviewed changes

npm/podController.go Outdated Show resolved Hide resolved

JungukCho reviewed Apr 14, 2021

View reviewed changes

npm/podController.go Outdated Show resolved Hide resolved

JungukCho reviewed Apr 14, 2021

View reviewed changes

vakalapa added 2 commits April 14, 2021 10:02

streamlining some redudant log messages

c319438

moving a comment

ced4ab8

JungukCho approved these changes Apr 14, 2021

View reviewed changes

vakalapa added the npm Related to NPM. label Apr 14, 2021

vakalapa merged commit 53e45c8 into master Apr 14, 2021

vakalapa deleted the vakr/npmerrorcontroller branch April 14, 2021 19:20

[NPM] better error handling and cache building #848

[NPM] better error handling and cache building #848

Uh oh!

Conversation

vakalapa commented Apr 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Apr 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vakalapa Apr 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JungukCho Apr 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JungukCho left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vakalapa commented Apr 2, 2021 •

edited

Loading

codecov bot commented Apr 2, 2021 •

edited

Loading

vakalapa Apr 14, 2021 •

edited

Loading

JungukCho Apr 14, 2021 •

edited

Loading