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

Two minor testsuite fixes #918

Merged
merged 2 commits into from
Jul 21, 2023

Conversation

SirPhuttel
Copy link
Contributor

@SirPhuttel SirPhuttel commented Jun 27, 2023

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.

@SirPhuttel SirPhuttel changed the title meta: firewall: Fix firewalld test with non-abstract sockets Two minor testsuite fixes Jun 27, 2023
Copy link
Contributor

@s1061123 s1061123 left a 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

@squeed
Copy link
Member

squeed commented Jul 3, 2023

@SirPhuttel this is failing lint, can you fix that?

@SirPhuttel
Copy link
Contributor Author

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?
Anyway, I figured I could run 'gofmt' myself and it suggested to use a tab instead of whitespace for the continued line in firewall_firewalld_test.go. Let's see if CI is happy now. :)

@SirPhuttel
Copy link
Contributor Author

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?

@squeed
Copy link
Member

squeed commented Jul 5, 2023

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.

@SirPhuttel
Copy link
Contributor Author

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 iptables -N foo && iptables -X foo it should be possible to iptables-restore --noflush <<< "*filter\n:foo - [0:0]\n-X foo\nCOMMIT" i.e., be less racey and save an extra execve() doing so.

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>
@squeed
Copy link
Member

squeed commented Jul 21, 2023

I didn't have a close look at go-iptables, but I think it would benefit from more "transactional behaviour"

Indeed it should. Honestly, it needs a rewrite (or just to be killed); it's a random survivor from the old CoreOS days.

@SirPhuttel
Copy link
Contributor Author

I didn't have a close look at go-iptables, but I think it would benefit from more "transactional behaviour"

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.

@squeed squeed merged commit fb8ca5d into containernetworking:main Jul 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants