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

QEMU balloon script abstraction improvement #248

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

slakkala
Copy link
Contributor

@slakkala slakkala commented Jun 6, 2024

Make the QEMU balloon script abstract away the difference between hypervisor balloon commands. The QMP balloon command expects the wanted memory size of the guest, so calculate the correct value from the balloon size given as argument.

Also some changes to reported balloon size; the changed signal is not emitted, if balloon size is not changed; and may report an old value.

The balloon command of qemu expects the total guest size. Alter the qemu
balloon script to calculate the correct value from passed balloon size to
make functionality closer to that with other hypervisors.

Signed-off-by: Santtu Lakkala <[email protected]>
Use query-balloon command instead of relying on the changed signal for
slightly improved reliability.

The 2 second delay between commands is arbitrarily chosen.

Signed-off-by: Santtu Lakkala <[email protected]>
Copy link
Owner

@astro astro left a comment

Choose a reason for hiding this comment

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

Very cool that you're taking care of consistency.

I think we should add documentation regarding what the $SIZE argument actually means. A comment in the generated shell script would be enough.

${writeQmp { execute = "balloon"; arguments.value = 987; }}
) | sed -e s/987/$VALUE/ | \
(${writeQmp { execute = "balloon"; arguments.value = 987; }}) | sed -e s/987/$VALUE/
${pkgs.coreutils}/bin/sleep 2
Copy link
Owner

Choose a reason for hiding this comment

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

The other VMMs also exhibit delayed effects. I think we should add the sleep to the other runners, too.

Also, the hardcoded 2 might be too low for loaded systems. Do you mind creating an option for it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cloud-hypervisor didn't seem to require a delay, added delay to crosvm and made the delay configurable (still defaults to 2)

@slakkala
Copy link
Contributor Author

Added a description of SIZE. Also added balloon size query to cloud-hv's balloon script.

Copy link
Owner

@astro astro left a comment

Choose a reason for hiding this comment

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

I am happy with this. Thanks!

One last question before merging: do you really think it's intuitive to pass the amount of Megabytes to remove from the memory size? Personally, I would expect the number to be the size of the balloon memory available to the VM. Then again I don't use ballooning myself, so your opinion is all that matters to me.

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.

None yet

2 participants