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

test: use modern qemu numa arguments #17987

Merged
merged 1 commit into from Dec 15, 2020
Merged

test: use modern qemu numa arguments #17987

merged 1 commit into from Dec 15, 2020

Conversation

cpaelzer
Copy link
Contributor

Upgrading to qemu 5.2 breaks TEST-36-NUMAPOLICY like:
qemu-system-x86_64: total memory for NUMA nodes (0x0) should
equal RAM size (0x20000000)

Use the new (as in >=2014) form of memdev in test 36:
-object memory-backend-ram,id=mem0,size=512M -numa node,memdev=mem0,nodeid=0

Fixes #17986.

@bluca bluca added the tests label Dec 15, 2020
@bluca
Copy link
Member

bluca commented Dec 15, 2020

Which is the first qemu version that added these? Thinking of the CentOS 7 CI, if it's not available there we'll need to do some discovery

@bluca
Copy link
Member

bluca commented Dec 15, 2020

(small world btw :-D )

@bluca bluca added good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed good-to-merge/waiting-for-reporter-feedback 👍 labels Dec 15, 2020
@cpaelzer
Copy link
Contributor Author

(small world btw :-D )

Indeed :-)

@cpaelzer
Copy link
Contributor Author

Which is the first qemu version that added these? Thinking of the CentOS 7 CI, if it's not available there we'll need to do some discovery

I've outlined those details in the issue #17986 - but I can easily copy them over if it helps.

The new argument is available since qemu 2.1 (2014).
Is that enough to just change the used qemu options or are pre-2014 qemu's needed to be working with the most recent systemd (tests)?

I'm not sure about differences in the implementation detail, I only checked when that argument was added.

@bluca bluca added ci-fails/needs-rework 🔥 Please rework this, the CI noticed an issue with the PR and removed good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed good-to-merge/waiting-for-reporter-feedback 👍 labels Dec 15, 2020
@bluca
Copy link
Member

bluca commented Dec 15, 2020

So as discussed on IRC, the CentOS 7 test has QEMU emulator version 1.5.3 (qemu-kvm-1.5.3-175.el7_9.1) so we'll need to do some version parsing I'm afraid

@cpaelzer cpaelzer mentioned this pull request Dec 15, 2020
Closed
@cpaelzer
test: use modern qemu numa arguments
9bd53aa 
Upgrading to qemu 5.2 breaks TEST-36-NUMAPOLICY like:
  qemu-system-x86_64: total memory for NUMA nodes (0x0) should
  equal RAM size (0x20000000)

Use the new (as in >=2014) form of memdev in test 36:
 -object memory-backend-ram,id=mem0,size=512M -numa node,memdev=mem0,nodeid=0

Since some target systems are as old as qemu 1.5.3 (CentOS7) but the new
kind to specify was added in qemu 2.1 this needs to add version parsing and
add the argument only when qemu is >=5.2.

Fixes systemd#17986.

Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
@bluca bluca added good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed and removed ci-fails/needs-rework 🔥 Please rework this, the CI noticed an issue with the PR labels Dec 15, 2020
bluca
bluca approved these changes Dec 15, 2020
View reviewed changes
Copy link
Member

@bluca bluca 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! it's unfortunate having to do such checks, but not much we can do when interfaces change and we need backward compat.

@mrc0mmand
Copy link
Member

mrc0mmand commented Dec 15, 2020 •
edited

Thanks a lot for this patch! I noticed this issue just this morning when our Arch Linux CI attempted to upgrade itself and fetched the latest QEMU, so this fix is right on time ;)

@yuwata yuwata merged commit 43b4947 into systemd:master Dec 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bluca bluca bluca approved these changes

Assignees
No one assigned
Labels
good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed tests
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

TEST-36-NUMAPOLICY incompatible with qemu >=5.2
4 participants
@cpaelzer @bluca @mrc0mmand @yuwata