CI: parallel-safe network system test #23694
Conversation
openshift-ci
bot
added
release-note-none
approved
| @@ -295,6 +302,7 @@ load helpers.network | |||
| run_podman 1 network rm $mynetname | |||
| } | |||
|
|
|||
| # CANNOT BE PARALLELIZED due to iptables/nft commands | |||
There was a problem hiding this comment.
also podman network reload --all, it would cause flakes in other test doing networking connectivity checks
| @@ -957,14 +979,15 @@ function wait_for_restart_count() { | |||
| } | |||
|
|
|||
| # Test for https://github.com/containers/podman/issues/18615 | |||
| # CANNOT BE PARALLELIZED due to strict checking of /run/netns | |||
There was a problem hiding this comment.
I wonder how much value this check here does. I mean I want to check that we do no leak the netns files however we only do so here in a ugly way IMO. Podman uses a consistent netns file name, as such I wonder if we should just add such check in the leak_check at the end of the suite. Then it will cover all tests not just this one. There should never be reason for us to leak the file, however it would be hard to know where it was leakded in case of failures but I guess if leaks happen we can instrument teardown to ls /run/netns to notice where it starts leaking...
Of course out of scope of this PR.
There was a problem hiding this comment.
Well.... this comment does have your name on it
podman/test/system/500-networking.bats
Lines 1002 to 1003 in ae4eaaf
I'm all for adding a leak check, but what would that look like? Right now on my laptop I see six /run/netns/netns* files with no tests running. Looks like I have some orphan pasta processes
There was a problem hiding this comment.
I know this was added by me, my comment was more of if it is blocking parallel tests it may not be a good place because checking leaks in one place doesn't help much if we still leak in other places
I'm all for adding a leak check, but what would that look like? Right now on my laptop I see six /run/netns/netns* files with no tests running. Looks like I have some orphan pasta processes
pasta should not be used as root so I don't think they relate to each other. If there are leaks right now then we need to debug/fix them first of course. Anyhow as said not in this PR. I will file a new issue to consider this.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: edsantiago, Luap99 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
edsantiago
force-pushed
the
safename-500
branch
from
cb0b22d to
cdaf1a2
Compare
| ip netns add $netns | ||
| run_podman create --network=ns:/var/run/netns/$netns $IMAGE | ||
| cid=${output} | ||
| run_podman inspect --format '{{ .NetworkSettings.Networks }}' $cid | ||
| is "$output" 'map[]' "NeworkSettings should be empty" | ||
| run_podman rm $cid | ||
| echo "$_LOG_PROMPT ip netns delete $netns" | ||
| ip netns delete $netns |
There was a problem hiding this comment.
This fixes a leak. Found by a leak check I've added (experimental) to #23275.
There was a problem hiding this comment.
Not blocking but doesn't this mean this always leaks the netns when one of the checks above fails?
Should this be moved into a custom teardown function?
There was a problem hiding this comment.
Maybe, but what's different about here vs all the other places that leak netns on failure? Like this one (scroll to bottom): every time 600 shell completion fails, it leaks four netns, the UUID-named ones, even though the network create in that test uses safename. It has been bothering me for a while, but I've only spent minimal time investigating.
Anyhow, if netns cleanup is more important than other container-related cleanup, I'm happy to move that into teardown somehow.
There was a problem hiding this comment.
if the netns files created by podman are leaked then this is a podman bug, even in the error paths we should be removing them
I don't think it is super important as it would only leak on error cases and there is no problems with leaks other than a bit of kernel memory keeping it around. And a reboot fixes that so I think that is fine for machines where these tests are run.
- replace random_string with safename in container/network names - add ci:parallel tags where possible. - where not possible, add explanations - fix a userns leak Signed-off-by: Ed Santiago <[email protected]>
edsantiago
force-pushed
the
safename-500
branch
from
cdaf1a2 to
abea5ad
Compare
|
/lgtm Given this is has been up for one week without another review I am going to merge it without a second review |
stale-locking-app
bot
added
the
locked - please file new issue/PR
Signed-off-by: Ed Santiago [email protected]