[NPM] Use utilexec for IPSet calls and fakeexec in podcontroller tests #861
Conversation
matmerr
force-pushed
the
npmexec
branch
5 times, most recently
from
4d25267 to
ed574fa
Compare
matmerr
force-pushed
the
npmexec
branch
2 times, most recently
from
057f457 to
ed72856
Compare
matmerr
changed the title
| output, err := cmd.CombinedOutput() | ||
| if err != nil { | ||
| metrics.SendErrorLogAndMetric(util.IpsmID, "Error: failed to save ipset to file with err %v, stderr: %v", err, string(output)) | ||
| return err |
There was a problem hiding this comment.
Is it better to return error with the same format you did in "Run" function to provide more info (e.g., stderr)?
Same for Restore and DestroyNpmIpsets functions.
| LabelsMap: make(map[string]string), | ||
| SetMap: make(map[string]string), | ||
| IpsMgr: ipsm.NewIpsetManager(), | ||
| IpsMgr: ipsm.NewIpsetManager(exec), |
There was a problem hiding this comment.
As far as I understood our code, there is no reason to tie IpsMgr and IptMgr in Namespace struct. In my opinion, it is better to locate them to other places (e.g., IpsMgr in npMgr, IptMgr in networkpolicyController). So, it will be easier to use our main code and UTs. I felt like there are multiple places to pass exec, etc now. However, I think that is beyond of this PR. We will fix it in other PRs. Any thoughts?
JungukCho
left a comment
JungukCho
left a comment
There was a problem hiding this comment.
lgtm and look forward to subsequent PR.
JungukCho
left a comment
JungukCho
left a comment
There was a problem hiding this comment.
lgtm and look forward to subsequent PR again.
3 participants
Reason for Change:
Our current test framework for NPM is heavily dependent on making actual syscalls to ipset/iptables. By interfacing out the calls to ipset, we can have actual UT coverage, without having to run tests as root, but we can also verify call ordering and have better visibility to the syscalls we're making.
Issue Fixed:
Requirements:
Notes: