[NPM] fix management of DeletedFinalStateUnknown object on deletion event in each controller #856
Conversation
… each controller. Add Unit test for them. Correct log messages.
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
| checkNetPolTestResult("TestDeleteNetworkPolicyWithTombstone", f, testCases) | ||
| } | ||
|
|
||
| func TestDeleteNetworkPolicyWithTombstoneAfterAddingNetworkPolicy(t *testing.T) { |
There was a problem hiding this comment.
I feel like there is no major difference in functionality between TestDeleteNetworkPolicyWithTombstone and TestDeleteNetworkPolicyWithTombstoneAfterAddingNetworkPolicy as AddingNS does not care about tombstone and in deleting the way we contruct tombstone is same. So we can reduce the complexity these test files by having only one tescase and not send delefinalstate in deleteNetPol(t *testing.T, ...) func bu
There was a problem hiding this comment.
They are different.
First test case only check "delete" event function in each controller.
Second test case is to try to mimic the realistic situation which walks through "delete" event -> workqueue -> reconcile. Thus, second test case first adds resources and then we emulate the realistic story (i.e., missing event and relist happens). Then, it proceeds all required steps like cleaning up the applied states from "delete" event.
If we want to keep one test case, I would like to keep second one which has more comprehensive test.
I am not sure this increase complexity. It has one extra boolean variable for only delete event in UT and later we can do more sophisticated tests. But, if there is better way to deal with this, let me know.
| } | ||
|
|
||
| func deleteNamespace(t *testing.T, f *nameSpaceFixture, nsObj *corev1.Namespace) { | ||
| func deleteNamespace(t *testing.T, f *nameSpaceFixture, nsObj *corev1.Namespace, isDeletedFinalStateUnknownObject IsDeletedFinalStateUnknownObject) { |
There was a problem hiding this comment.
Potential function name ambiguity, can clarify between this helper deleteNamespace and f.nsController.deleteNamespace()
There was a problem hiding this comment.
Good point. I think last time Vamsi also mentioned this. We can discuss and refactor this point when we add more UTs with ipset and iptables.
3 participants
Reason for Change:
Current code does not enqueue a key of DeletedFinalStateUnknown object on deletion event, which may cause leak of applied states (e.g., ipset, iptables) and keep an item in a local cache (e.g., podMap, nsMap, nsMap).
This PR is to properly clean up applied states (e.g., ipset, iptables, and local cache (e.g., podMap, nsMap, nsMap) in case of DeletedFinalStateUnknown object on deletion event in each controller.
Issue Fixed:
Requirements:
Notes: