-
Notifications
You must be signed in to change notification settings - Fork 790
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
Two minor testsuite fixes #918
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, thanks!
/lgtm
@SirPhuttel this is failing lint, can you fix that? |
31eae0c
to
7db9b98
Compare
Hmm. I wasn't able to find a failure cause in the CI logs. Maybe something to improve? It seems the system creates a tarball with logs, but it's not made available? |
That failing test is odd. I can't reproduce it locally, maybe a one-off? The error seems unrelated to the testsuite fixes at least. Also, it's odd how iptables chain "removal" happening in portmap's teardown() implicitly creates a chain. IIUC, the code does 'iptables -N foo && iptables -X foo' ten times in parallel? In the same netns? |
That test failure is due to a go-iptables bug I need to get around to fixing. It doesn't detect all the right error codes, I think. |
I didn't have a close look at go-iptables, but I think it would benefit from more "transactional behaviour", e.g. instead of |
On a recent Fedora Rawhide, dbus-daemon-1.14.8-1 prints a string prefixed by 'unix:path' instead of the expected 'unix:abstract', thereby failing the test. Allowing this alternate prefix fixes the test, so for communication with the daemon it is not relevant. Signed-off-by: Phil Sutter <psutter@redhat.com>
The script is set to exit on error, so mkdir failing because /tmp/cni-rootless already exists aborts the test run. Call 'mkdir -p' to avoid the spurious error. Signed-off-by: Phil Sutter <psutter@redhat.com>
7db9b98
to
8e69e38
Compare
Indeed it should. Honestly, it needs a rewrite (or just to be killed); it's a random survivor from the old CoreOS days. |
Yes, just adding a "batch mode" means introducing a new API anyway. Luckily go-nft has this already, so if users migrate the fork'n'exec overhead will go away. |
Patch 1 fixes for unexpected output from dbus-daemon on Fedora Rawhide, patch 2 fixes a spurious error when calling test_linux.sh a second time.