Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add e2e testing of ipvlan, macvlan, and bridge plugins #167

Closed
wants to merge 6 commits into from

Conversation

dcbw
Copy link
Member

@dcbw dcbw commented Mar 30, 2016

Fixes #179

@dcbw
Copy link
Member Author

dcbw commented Mar 30, 2016

@steveej @jonboulle I intend to rebase #145 on top of this and use this framework for the testcases that were requested.

@dcbw
Copy link
Member Author

dcbw commented Mar 30, 2016

As a bonus it increases test coverage from 21% to 36% overall, and we get this for plugins:

ok github.com/appc/cni/plugins/main/ipvlan 0.014s coverage: 68.9% of statements
ok github.com/appc/cni/plugins/main/macvlan 0.013s coverage: 66.7% of statements
ok github.com/appc/cni/plugins/main/bridge 0.009s coverage: 63.5% of statements

@dcbw
Copy link
Member Author

dcbw commented Mar 30, 2016

@danwinship a review from you would be nice since you've done similar-yet-different stuff for openshift-sdn...

@danwinship
Copy link
Contributor

Seems like maybe it would be good to have a way to run the new test programs against the actual netlink implementation, so you can make sure that they actually actually work. (Maybe a second set of tests using the real ops instead of the test ops, but skipped by default?)

@dcbw dcbw force-pushed the mock-netlink-testing branch from ecd9620 to b4f221d Compare March 30, 2016 22:15
@steveej steveej added this to the v0.3.0 milestone Mar 31, 2016
@dcbw dcbw force-pushed the mock-netlink-testing branch 2 times, most recently from d3ead5f to c0d9344 Compare April 6, 2016 22:03
@dcbw
Copy link
Member Author

dcbw commented Apr 6, 2016

@steveej @jonboulle @danwinship PTAL; latest push enables both mock and live testing. I'm not 100% happy with how the live testing gets done (by running all testcases over with a new env variable) but I can fix that next week if people want.

@dcbw
Copy link
Member Author

dcbw commented Apr 6, 2016

BTW, the first commit in this PR is actually #176

@dcbw
Copy link
Member Author

dcbw commented Apr 7, 2016

travis failure seems to be a flake?

$ go get ${TOOLS_CMD}/vet

package golang.org/x/tools/cmd/vet: cannot find package "golang.org/x/tools/cmd/vet" in any of:

    /home/travis/.gimme/versions/go1.5.3.linux.amd64/src/golang.org/x/tools/cmd/vet (from $GOROOT)

    /home/travis/gopath/src/golang.org/x/tools/cmd/vet (from $GOPATH)

The command "go get ${TOOLS_CMD}/vet" failed and exited with 1 during .

Your build has been stopped.

return nil, syscall.Errno(syscall.ERANGE)
}
nsid = int(nsfd)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing default case

@rosenhouse
Copy link
Contributor

@dcbw The NewNS function has some problems still. On line 140 you're checking the value of an err variable, which makes it appear that you're trying to handle the error cases from inside the go (func() { call above. But inside that function, on line 111, you shadow the err variable. So the err values set within the func will not be visible at the outer scope.

@rosenhouse
Copy link
Contributor

I'm also unclear on why you're calling origNS.Set() 3 different times, rather than just doing a defer origNS.Set() on line 122.

@dcbw dcbw force-pushed the mock-netlink-testing branch from 5e1051e to c39e019 Compare May 12, 2016 19:43
@dcbw
Copy link
Member Author

dcbw commented May 12, 2016

@rosenhouse rebased and fixed NewNS. PTAL, thanks!

@dcbw dcbw force-pushed the mock-netlink-testing branch 2 times, most recently from ed4b93e to c12f35d Compare May 12, 2016 20:22
@dcbw
Copy link
Member Author

dcbw commented May 12, 2016

@rosenhouse as a side-effect of only calling LockOSThread() in the goroutine for netns.Do(), we now need to make sure that any checks we do in testcases are done wrapped in a Do(), otherwise I see periodic failures where the namespace gets switched back, possibly because the testcase gets switched to the main thread every so often. Just something to be aware of.

@steveej
Copy link
Member

steveej commented May 18, 2016

@dcbw @rosenhouse I suggest that we split this PR in two. The first one without change in implementation related to tests at all, just adding of tests and fixes in the network locking. The second one can then be rebased on top of the reworked first one. I would really like to get this in by the end of the week as it is the biggest blocker for v0.3 which is highly awaited at this point.

dcbw added 6 commits May 18, 2016 10:06
This also removes the thread-locking arguments from the ns package
per containernetworking#183 by doing all the namespace
changes in a separate goroutine that locks/unlocks itself, instead of
the caller having to track OS thread locking.
@dcbw dcbw force-pushed the mock-netlink-testing branch from c12f35d to ab8ecee Compare May 18, 2016 15:16
@dcbw
Copy link
Member Author

dcbw commented May 18, 2016

@steveej could you clarify which parts you'd like split out into separate PRs? I'm not sure which parts you mean by "just adding of tests and fixes in the network locking". Is there anything else I can do about the later patches in the series that add e2e testing that would make you more comfortable with them?

I'm happy to continue reworking patches, but to get things done by the end of the week the review/rework cycle needs to be a lot shorter...

@dcbw
Copy link
Member Author

dcbw commented May 18, 2016

In the interest of getting the fixes merged faster I've removed the netops stuff and pushed a reduced set of commits in #211. We'll leave this one open to discuss mocking further.

@steveej
Copy link
Member

steveej commented Aug 2, 2016

@dcbw I've lost track of this PR's changes. Have they been merged via another PR?

@dcbw
Copy link
Member Author

dcbw commented Sep 30, 2016

@steveej the only thing this PR was kept around for was the mock netlink stuff. But I guess we long ago decided not to use that, so I'll close and we can always resurrect later if we want to.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants