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.

Sign up for GitHub

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

Jump to bottom

Upgrade tests: reenable, but revamped #21535

Merged
merged 1 commit into from
Feb 8, 2024
Merged

Upgrade tests: reenable, but revamped #21535

merged 1 commit into from
Feb 8, 2024

Conversation

edsantiago
Copy link
Member

@edsantiago edsantiago commented Feb 6, 2024 •
edited
Loading

No longer bother testing any 2.x or 3.x. Only 4.1 and above.

Remove all CNI-related code. CNI is gone.

Add DatabaseBackend tests, confirming that we can handle
both boltdb and sqlite.

Require BATS >= 1.8.0, and use "run -0" to do exit-status checks.

Update docs.

[ marked WIP because this is going to fail. I need help figuring out what to do.]

Signed-off-by: Ed Santiago [email protected]

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. do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None approved Indicates a PR has been approved by an approver from all required OWNERS files. release-note-none and removed do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None labels Feb 6, 2024
edsantiago
edsantiago commented Feb 6, 2024
View reviewed changes
test/upgrade/test-upgrade.bats Outdated
Comment on lines 277 to 284
run_podman network connect $MYTESTNETWORK myrunningcontainer
# FIXME: this fails with "netavark: iptables: No * by that name"
# ...and if we comment it out, curl fails (28, timeout), presumably
# because of the above network connect
run_podman network disconnect podman myrunningcontainer
run curl --max-time 3 -s 127.0.0.1:$HOST_PORT/index.txt
run -0 curl --max-time 3 -s 127.0.0.1:$HOST_PORT/index.txt
is "$output" "$RANDOM_STRING_1" "curl on container with second network connected"
Copy link
Member Author

Choose a reason for hiding this comment

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

help

Copy link
Member

Choose a reason for hiding this comment

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

CI seems to pass on that, did you mix CNI and netavark on your system again?
Or maybe mixing between iptables-legacy and iptables-nft?

Copy link
Member Author

Choose a reason for hiding this comment

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

CNI was my first thought, of course. I'm 99% sure that's not it because there's no podmanX interface. It could be iptables? These are the packages on my laptop:

$ rpm -qa|sed -ne 's/^iptables-//' -e 's/-1.8.9-5.*$//p'|sort|fmt
compat legacy legacy-libs libs nft utils

So, there's compat and legacy and nft. Presumably because of my long upgrade path. Is that not a supported configuration, and what should I do to fix it?

Copy link
Member

Choose a reason for hiding this comment

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

Having both installed is fine, usually you have to switch via alternatives between them. Both should work with netavark.
You can just run iptables -V to check which one is active. What I was hinting at is that we first run in the old podman container with a potential different iptables, so if container has iptables-legacy while the host uses iptables-nft then an error with no rule/chain could make sense because they use different kernel backends and cannot see the rules from each other.

My understanding is that they should never be mixed on a running system as different rules can cause weird conflicts, so if one switches the iptables backend it is best to reboot first.

Copy link
Member Author

Choose a reason for hiding this comment

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

iptables -V says nf_tables. That might explain the failure on my system, but it also raises questions about what exactly we're testing in the upgrade test. I have not yet had enough coffee to suss that out.

Copy link
Member

Choose a reason for hiding this comment

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

yes the image uses legacy so this cannot work correctly in this combination

$ podman run --rm quay.io/podman/stable:v4.1.0 iptables -V
iptables v1.8.7 (legacy)

And it looks like the image has only legacy installed so the only way to run these tests successfully is to use iptables-legacy on the host or patch up the podman image to include and use iptables-nft but that seems harder.

test/upgrade/test-upgrade.bats Outdated
Comment on lines 307 to 292
# FIXME: fails, exit code 28 (timeout)
run -0 curl --max-time 3 -s 127.0.0.1:$HOST_PORT/index.txt
Copy link
Member Author

Choose a reason for hiding this comment

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

help

test/upgrade/test-upgrade.bats Outdated
Comment on lines 340 to 342
# FIXME: fails: pod X cgroup is not set: internal libpod error
# run_podman run --pod=mypod --ipc=host --rm $IMAGE echo it works
# is "$output" ".*it works.*" "podman run --pod"
Copy link
Member Author

Choose a reason for hiding this comment

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

help

@edsantiago edsantiago force-pushed the upgrade_tests branch 3 times, most recently from 73d29ae to 6d28957 Compare February 7, 2024 16:09
@packit-as-a-service Packit-as-a-Service
Copy link

Cockpit tests failed for commit 6d28957bf0a19b2614359b9d49d40e3e434f1440. @martinpitt, @jelly, @mvollmer please check.

@packit-as-a-service Packit-as-a-Service
Copy link

Cockpit tests failed for commit 73d29ae12413bf8c8930465408c23a1634e74fdc. @martinpitt, @jelly, @mvollmer please check.

@edsantiago
Copy link
Member Author

All right team, how's this? Imperfect, but it would at least get us back into upgrade testing.

@edsantiago edsantiago changed the title WIP: Upgrade tests: reenable, but revamped Upgrade tests: reenable, but revamped Feb 7, 2024
@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 Feb 7, 2024
Luap99
Luap99 reviewed Feb 8, 2024
View reviewed changes
test/upgrade/test-upgrade.bats Outdated Show resolved Hide resolved
test/upgrade/test-upgrade.bats Outdated Show resolved Hide resolved
test/upgrade/test-upgrade.bats Outdated Show resolved Hide resolved
test/upgrade/test-upgrade.bats Outdated Show resolved Hide resolved
test/upgrade/test-upgrade.bats Outdated Show resolved Hide resolved
@Luap99 Luap99 mentioned this pull request Feb 8, 2024
Merged
@edsantiago
Upgrade tests: reenable, but revamped
e20b70c 
No longer bother testing any 2.x or 3.x. Only 4.1 and above.

Remove all CNI-related code. CNI is gone.

Add DatabaseBackend tests, confirming that we can handle
both boltdb and sqlite.

Require BATS >= 1.8.0, and use "run -0" to do exit-status checks.

Update docs.

Signed-off-by: Ed Santiago <[email protected]>
Luap99
Luap99 approved these changes Feb 8, 2024
View reviewed changes
Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

LGTM

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Feb 8, 2024

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@edsantiago edsantiago mentioned this pull request Feb 8, 2024
Closed
@Luap99
Copy link
Member

Luap99 commented Feb 8, 2024

@containers/podman-maintainers PTAL

@edsantiago edsantiago mentioned this pull request Feb 8, 2024
Closed
@mheon
Copy link
Member

mheon commented Feb 8, 2024

Sure
/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 8, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 4570ccb into containers:main Feb 8, 2024
82 of 84 checks passed
@edsantiago edsantiago deleted the upgrade_tests branch February 8, 2024 21:57
@stale-locking-app stale-locking-app bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label May 9, 2024
@stale-locking-app stale-locking-app bot locked as resolved and limited conversation to collaborators May 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Luap99 Luap99 Luap99 approved these changes

Assignees

@mheon mheon

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@edsantiago @Luap99 @mheon