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

test/e2e: use fresh network config dir for each test #17975

Closed
wants to merge 3 commits into from

Conversation

Luap99
Copy link
Member

@Luap99 Luap99 commented Mar 29, 2023

The e2e test are isolated and have their own --root/--runroot arguments. However networks were always shared, this causes problem with tests that do a prune or reset because they can effect other parallel running tests.

Over the time I fixed some of of these cases to use their own config dir but #17946 suggests that this is not enough. Instead of trying to find and fix these tests just go with the big hammer and make every test use a new clean network config directory.

This will also make the use of defer podmanTest.removeNetwork(...) unnecessary. This is required at the moment for every test which creates a network. However to keep the diff small and to see if it is even working I will do it later in a follow up commit.

Fixes #17946

Does this PR introduce a user-facing change?

None

@openshift-ci openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none labels Mar 29, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Mar 29, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 29, 2023
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

LGTM

@Luap99
Copy link
Member Author

Luap99 commented Mar 29, 2023

Note while this works it still needs some cleanup follow-up changes thus I marked it as draft.

@Luap99 Luap99 force-pushed the e2e-networks-dir branch 2 times, most recently from 654077e to d4a2e8e Compare March 29, 2023 17:04
@Luap99 Luap99 marked this pull request as ready for review March 29, 2023 17:04
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 29, 2023
Luap99 added 3 commits March 30, 2023 14:40
The e2e test are isolated and have their own --root/--runroot arguments.
However networks were always shared, this causes problem with tests that
do a prune or reset because they can effect other parallel running
tests.

Over the time I fixed some of of these cases to use their own config dir
but containers#17946 suggests that this is not enough. Instead of trying to find
and fix these tests just go with the big hammer and make every test use
a new clean network config directory.

This will also make the use of `defer podmanTest.removeNetwork(...)`
unnecessary. This is required at the moment for every test which creates
a network. However to keep the diff small and to see if it is even
working I will do it later in a follow up commit.

Fixes containers#17946

Signed-off-by: Paul Holzinger <[email protected]>
Now that we ran each test with their own custom config there is no need
to have the special cases anymore.

Signed-off-by: Paul Holzinger <[email protected]>
Now that each test has its own custom network config dir we now longer
have to explicitly cleanup the network because it is just a file which
will be deleted in the test cleanup anyway.

Signed-off-by: Paul Holzinger <[email protected]>
@Luap99 Luap99 force-pushed the e2e-networks-dir branch from d4a2e8e to 4f281ec Compare March 30, 2023 12:40
@Luap99
Copy link
Member Author

Luap99 commented Mar 30, 2023

Ok this is not going to work, it will introduce much more flakes than it fixes.
The problem is we only have one host netns which is shared between tests. If the network config dir is not shared we cannot make conflict checks for interface names and ip address. This results in different test trying to use the same interface and/or ip address which will cause runtime failures in CNI and netavark.

Thus I am closing this PR as it is incorrect, the correct fix is to manually check for all tests which do a prune or reset and make those are using a custom directory, see the change from the second commit here.

@Luap99 Luap99 closed this Mar 30, 2023
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 4, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note-none
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ci: unable to find network with name or ID podman-default-kube-network
2 participants