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 support continuation #490

Merged
merged 80 commits into from
Dec 5, 2023
Merged

QEmu support continuation #490

merged 80 commits into from
Dec 5, 2023

Conversation

olethanh
Copy link
Collaborator

@olethanh olethanh commented Nov 29, 2023

continuation of #487

Allow launching VM Instance via QEMU instead of firecracker.

This works by adding a new Controller for Qemu alongside AlephFirecrackerProgram and AlephFirecrackerInstance and launch it if the message == Instance + hypervisor == qemu.

I'm opening this so we can get the review and discussion started but from discussion I understand it won't be for the next release since we are focusing on bugfixes for now. I can clean it up the git history afterward if needed. Please play with it plenty to find any problem I might have missed.

There is a corresponding PR in aleph-message: aleph-im/aleph-message#78

how to test

See this pretty complete readme on how to test it https://github.com/aleph-im/aleph-vm/blob/qemu_support/src/aleph/vm/controllers/qemu/QEMU.md

Necessary change in aleph-message were released in 0.4.1

Modification to the code

I had to make a few change outside the Qemu controller itself to provide compatibility between all controller:

  • New abstract class: AlephControllerInterface which define the shared interface between Firecracker and Qemu controllers for sharing and typing.
  • Add field support_snapshot on controller so the controler can declare support to the SnapShotManager without the different guessing from the method we had till now.
  • a Mixin to manage the cloud init config, I intended to have it used between all the controllers that need it but at the moment I had to tweak the cloud init configuration so it's not done yet
  • get_log_queue and unregister_queue so the operator can register to the Log queues without knowing the internal logic of the VM (which is different since Qemu don't use MicroVM

Refer to QEMU.md for a list of supported feature at the moment.
IMHO the main thing missing is automated testing.
moment I had to tweak the cloud init configuration so it's not done yet
get_log_queue and unregister_queue so the operator can register to the Log queues without knowing the internal logic of the VM (which is different since Qemu don't use MicroVM

Refer to QEMU.md for a list of supported feature at the moment.
IMHO the main thing missing is automated testing.

src/aleph/vm/models.py Outdated Show resolved Hide resolved
Copy link
Member

@MHHukiewitz MHHukiewitz left a comment

Choose a reason for hiding this comment

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

LGTM, just make sure CI/CD passes and we keep Python 3.9 compatibility

Copy link
Member

@nesitor nesitor left a comment

Choose a reason for hiding this comment

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

LGTM, maybe at the merge moment with the Systemd feature, we will need to make slight changes, but for now it is so good.

src/aleph/vm/controllers/qemu/instance.py Show resolved Hide resolved
Copy link
Member

@MHHukiewitz MHHukiewitz left a comment

Choose a reason for hiding this comment

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

LGTM

@olethanh olethanh merged commit ef8ae0b into main Dec 5, 2023
18 checks passed
hoh pushed a commit that referenced this pull request Mar 12, 2024
continuation of #487 

Allow launching VM Instance via QEMU instead of firecracker.

This works by adding a new Controller for Qemu alongside  AlephFirecrackerProgram and AlephFirecrackerInstance and launch it if the message == Instance + hypervisor == qemu.

I'm opening this so we can get the review and discussion started but from discussion I understand it won't be for the next release since we are focusing on bugfixes for now. I can clean it up the git history afterward if needed. Please play with it plenty to find any problem I might have missed.

There is a corresponding PR in aleph-message: aleph-im/aleph-message#78


## how to test
See this pretty complete readme on how to test it https://github.com/aleph-im/aleph-vm/blob/qemu_support/src/aleph/vm/controllers/qemu/QEMU.md

Necessary change in aleph-message were released in 0.4.1

## Modification to the code

I had to make a few change outside the Qemu controller itself to provide compatibility between all controller:

- New abstract class: AlephControllerInterface which define the shared interface between Firecracker and Qemu controllers for sharing and typing.
- Add field `support_snapshot` on controller so the controler can declare support to the SnapShotManager without the different guessing from the method we had till now.
- a Mixin to manage the cloud init config, I intended to have it used between all the controllers that need it  but at the moment I had to tweak the cloud init configuration so it's not done yet
- `get_log_queue` and `unregister_queue` so the operator can register to the Log queues without knowing the internal logic of the VM (which is different since Qemu don't use MicroVM


Refer to QEMU.md for a list of supported feature at the moment.
IMHO the main thing missing is automated testing.
moment I had to tweak the cloud init configuration so it's not done yet
    get_log_queue and unregister_queue so the operator can register to the Log queues without knowing the internal logic of the VM (which is different since Qemu don't use MicroVM

Refer to QEMU.md for a list of supported feature at the moment.
IMHO the main thing missing is automated testing.
@Psycojoker Psycojoker deleted the olethanh-qemu-support-cont branch July 24, 2024 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BLACK This PR has critical implications and must be reviewed by a senior engineer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants