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: test ignore MAC Address on restore feature #1187

Merged
merged 1 commit into from
Feb 8, 2023

Conversation

jelly
Copy link
Member

@jelly jelly commented Jan 18, 2023

This was broken in 4.0 and now fixed in 4.4 which is still an release candidate. Locally this has been tested with podman-4.4.0~rc1.

Closes #1179

Locally tested with https://koji.fedoraproject.org/koji/buildinfo?buildID=2111508 and Fedora-37

@jelly jelly requested a review from marusak January 18, 2023 16:07
test/check-application Outdated Show resolved Hide resolved
@marusak
Copy link
Member

marusak commented Jan 25, 2023

I would wait until we actually test this somewhere so if it does not work then we would need to adjust it. Should not take too long

@jelly
Copy link
Member Author

jelly commented Feb 6, 2023

I would wait until we actually test this somewhere so if it does not work then we would need to adjust it. Should not take too long

A new podman was released last Friday, so this should be testable in rawhide today.. maybe?!

@jelly
Copy link
Member Author

jelly commented Feb 6, 2023

rawhide has:

crun-1.8-1.fc38.x86_64
podman-4.4.0~rc2-2.fc38.x86_64

@jelly jelly requested a review from marusak February 6, 2023 14:48
Copy link
Member

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

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

Thanks!

# A new MAC address should have been generated
version = self.execute(False, "podman -v").strip().split(' ')[-1]
# Fixed in podman 4.4.0 https://github.com/containers/podman/issues/16666
if int(version.split('.')[0]) >= 4 and int(version.split('.')[1]) > 3:
Copy link
Member

Choose a reason for hiding this comment

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

This would fail for e. g. podman 5.0 or 5.2. I agree that right now a hardcoded m.image list is still too early, as 4.4 will most probably enter Fedora, Debian testing, arch etc. soon. I recommend to convert version to a tuple of ints and compare it as tuple, which works as intended. See e.g. test/verify/check-storage-resize:

if self.storaged_version < [2, 7, 6]

@jelly
Copy link
Member Author

jelly commented Feb 7, 2023

ValueError: invalid literal for int() with base 10: '0-rc2'

Arggh ofcourse!

Copy link
Member

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

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

Thanks, almost there!

Comment on lines +36 to +38
version = cls.execute(False, "podman -v").strip().split(' ')[-1]
# HACK: handle possible rc versions such as 4.4.0-rc2
return tuple(int(v.split('-')[0]) for v in version.split('.'))
Copy link
Member

Choose a reason for hiding this comment

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

If only we had a more machine readable API for determining the version. One should invent an RPC protocol over an Unix socket, and call it "data bus" or something such 🤔

(snarky comment aside -- this is ugly, but effective, thanks!)

@@ -1201,6 +1208,13 @@ class TestApplication(testlib.MachineCase):
b.click('.pf-c-modal-box button:contains(Restore)')
b.wait(lambda: self.getContainerAttr("swamped-crate", "State") in 'Running')

# A new MAC address should have been generated
# Fixed in podman 4.4.0 https://github.com/containers/podman/issues/16666
import json
Copy link
Member

Choose a reason for hiding this comment

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

Let's please keep imports at the top.

# A new MAC address should have been generated
# Fixed in podman 4.4.0 https://github.com/containers/podman/issues/16666
import json
print(json.dumps(podman_version(self)))
Copy link
Member

Choose a reason for hiding this comment

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

Debugging leftover. But to be sure that the version comparison works, you could do an else: below and check that the MAC address is equal.

Copy link
Member Author

Choose a reason for hiding this comment

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

I actually think that's a neat idea! Let's test both cases.

Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit torn between this, as it explicitly tests a buggy behaviour. But it increases my confidence in our test 😁

This was broken in 4.0 and now fixed in 4.4 which is still an release
candidate. Locally this has been tested with podman-4.4.0~rc1.

Closes cockpit-project#1179
Copy link
Member

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

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

Danjkewel!

@martinpitt martinpitt merged commit 19cb73d into cockpit-project:main Feb 8, 2023
@jelly jelly deleted the mac-address-test branch February 8, 2023 09:05
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.

Add test for container restoring with ignoring MAC address
3 participants