[NPM] Ignoring hostnetwork pods from being added into Ipsets #776
Conversation
Codecov Report
@@ Coverage Diff @@
## master #776 +/- ##
==========================================
+ Coverage 41.68% 41.93% +0.24%
==========================================
Files 142 142
Lines 13778 13789 +11
==========================================
+ Hits 5744 5782 +38
+ Misses 7317 7285 -32
- Partials 717 722 +5 |
vakalapa
changed the title
|
|
||
| allNs, err := newNs(util.KubeAllNamespacesFlag) | ||
| if err != nil { | ||
| panic(err.Error) |
There was a problem hiding this comment.
shouldn't panic on tests. use t.Fatal. same for below
There was a problem hiding this comment.
Logged a work item to fix this for all NPM tests.
|
|
||
| ipsMgr := ipsm.NewIpsetManager() | ||
| if err := ipsMgr.Save(util.IpsetTestConfigFile); err != nil { | ||
| t.Errorf("TestAddHostNetworkPod failed @ ipsMgr.Save") |
There was a problem hiding this comment.
add error to test logging. gives more context as to why something failed. same for below
There was a problem hiding this comment.
Logged a work item to fix this for all NPM tests.
| }, | ||
| } | ||
|
|
||
| npMgr.Lock() |
There was a problem hiding this comment.
curious about locks being taken, since this variable is scoped to the test. what else could be modifying this?
There was a problem hiding this comment.
These are all compartmentalized tests, in the current state they are in, no two tests (i have to double check) share the npMgr. So the lock should be scop is limited to a given testcase.
There was a problem hiding this comment.
I suppose my question is, is it necessary to take locks?
matmerr
left a comment
matmerr
left a comment
There was a problem hiding this comment.
approved, the npm test suite needs some help for sure :)
* Ignoring hostnetwork pods from being added into Ipsets * generalizing the check on hostnetwork pod * Adding tests for add, update and delete hostnetwork pods
This fix involves ignoring any Add, Update or Delete Pod events for Hostnetwork Pods.