Rework qemu startup/configuration#83
Conversation
| ) | ||
| parser.addoption("--qemu-image", action="store", help="Path to a QEMU image") | ||
| parser.addoption("--qemu-arch", action="store", help="Qemu architecture type") | ||
| parser.addoption("--test-binary", action="store", help="Path to the test binary to be executed on the target") |
There was a problem hiding this comment.
Can't this be fetched from some bazel manifest files? I wonder what happens if more and more command line options are added, this makes it harder and harder to quickly execute some tests.
| logger.info(f"Starting tests on host: {socket.gethostname()}") | ||
| with qemu_target(config) as qemu: | ||
| pre_tests_phase(qemu) | ||
| #pre_tests_phase(qemu) |
There was a problem hiding this comment.
Is that still needed? If not, I would avoid dead code.
There was a problem hiding this comment.
just talked to @RacheleSetto, she disabled these checks and I overlooked it.
Moved the PR in draft because we have to check this out first.
There was a problem hiding this comment.
In my opinion this needs to be reenabled as it is a runtime check to make sure the system run by QEMU is properly up and running. Disabling this might lead to changes in test behaviour.
ltekieli
left a comment
There was a problem hiding this comment.
PR Review: Rework qemu startup/configuration
What the PR does
This PR restructures the QEMU plugin's configuration schema and adds multi-architecture support (x86_64 / aarch64). It touches 9 files across config models, QEMU process management, networking, and docs.
What belongs in the core ITF QEMU plugin (good)
1. Config schema restructuring (config.py) — Appropriate for the plugin. The move from a flat JSON schema to a structured { "qemu": { ... } } format with nested host_qemu_network is a clean improvement. Pydantic validation with extra="forbid" is the right call. The computed properties ip_address and ssh_port on QemuConfigModel are a nice simplification — they eliminate the fragile networks[0].ip_address pattern that was scattered across qemu_target.py.
2. --qemu-arch CLI option (__init__.py) — Reasonable to have in the plugin. The plugin needs to know which QEMU binary to invoke. This is a fundamental launch parameter.
3. Refactored qemu_target.py — The simplification from self._config.networks[0].ip_address to self._config.ip_address is a direct win from the config restructuring. Clean and correct.
Concerns — things that should be reconsidered or split out
1. Legacy field migration in HostQemuNetwork does NOT belong in the core plugin
The subnet_address, machine_address, network_adapter legacy fields and the _migrate_legacy_fields model validator (config.py) are backward-compat shims for a specific downstream consumer's config format. This is not something the core ITF repo should own. It should live in a separate compatibility layer or a custom plugin that extends this one. Keeping it here means the core plugin is permanently coupled to a legacy format it never shipped.
Recommendation: Remove the three legacy fields and the
_migrate_legacy_fieldsvalidator. Downstream consumers should preprocess their configs or write a wrapper plugin.
2. Hardcoded aarch64 defaults leak a specific use case into the generic plugin
cpu="cortex-a57"as the new default inQemu.__init__— the old default wasCascadelake-Server-v5(x86-specific). The new default is aarch64-specific. Neither is a good generic default. The default should match--qemu-archor there should be no default (require it when arch-dependent).-machine virt-4.2is hardcoded for aarch64 — this pins a specific QEMU machine version. This is the kind of thing that should be configurable (in the JSON config) rather than baked into the plugin.- The
512Mqcow2 disk size in__disk_drive_argsis hardcoded. This should come from the config (disk_image_sizefield) or at least be a constant.
3. score_disk / disk_image and auto-creation logic is a custom workflow
__disk_drive_args() in qemu.py auto-creates a qcow2 image if it doesn't exist (qemu-img create -f qcow2 ... 512M). This is opinionated behavior that not every user will want — some may want to fail loudly if the image is missing. This feels like it belongs in a project-specific plugin or at minimum should be opt-in via config.
4. x86_64 acceleration is silently downgraded
The README now says "KVM is not used even if /dev/kvm is present" for x86_64, and the code always passes -accel tcg for x86. But enable_kvm still exists as a config field (defaulting to True). This is confusing — the config field is effectively dead. Either remove enable_kvm from the schema or actually wire it up.
5. network_adapters is now always []
In qemu_target.py, network_adapters is hardcoded to [] in the qemu_target fixture. The entire __network_devices_args() code path in qemu.py is now dead code. If bridge/tap networking via named adapters is being removed, the dead code should be cleaned up. If it's still needed, it should still be wired to config.
Test coverage gap
The test changes are minimal — only test_qemu_config_schema.py gets a one-line comment update. There are no new tests for:
- aarch64 vs x86_64 command-line generation
- The legacy field migration logic
- The
ssh_port/ip_addresscomputed properties - Disk image auto-creation
- Port-forwarding argument generation with the new format
These are all testable in isolation (unit-test the models + command builder) and should be covered before merging.
Summary
| Area | Verdict |
|---|---|
| Config schema restructure | Good for core plugin |
--qemu-arch option |
Good for core plugin |
qemu_target.py simplification |
Good for core plugin |
Legacy field migration (subnet_address, etc.) |
Move to downstream / custom plugin |
Hardcoded aarch64 defaults (cortex-a57, virt-4.2, 512M) |
Make configurable or move out |
Disk auto-creation (qemu-img create) |
Move to custom plugin or make opt-in |
enable_kvm config field |
Dead — remove or wire up |
network_adapters / __network_devices_args |
Dead code — clean up |
| Test coverage | Insufficient — needs unit tests |
The config restructuring and arch selection are the right things to land in the core plugin. But the PR also smuggles in project-specific opinions (legacy compat, hardcoded machine types, auto-creating disk images) that would be better served by a downstream plugin or additional config fields.
|
Please split this into multiple separate functionally divided PRs, I suggest:
This will allow us to review the changes better, now there is a lot of stuff added that is not used or wrong. Fix the issues mentioned by the automatic review above. Image should not be part of the qemu plugin. For that you have two options:
For this you need proper support in rules_imagefs. After this is done integration of tne rules_imagefs can be done here to test the plugin extensions. One important thing is that the qemu plugin does not really belong to ITF, long term it will be removed. ITF is just a scafolding for putting plugins together and exposing a unified interface for the tests to implement against. If you need a custom qemu handling, you can even implement it from scratch and not use the one that is currently in ITF. |
|
@ltekieli Thank you so much for the effort you put into this review, it is well written and well considered. Thanks for helping us to understand how we can better improve ITF in the future. @clanghans Since we will break this down into finer grained PRs with separated concerns, I will close this one for now, but we shall keep it as reference before we open further PRs. It may serve as a good contribution checklist for us. |


Summary
qemukey, validated strictly via Pydantic (unknown fields rejected)--qemu-archCLI option to select the target architecture (x86_64/aarch64)Configuration schema change
Old flat schema:
{ "networks": [{"name": "tap0", "ip_address": "...", "gateway": "..."}], "ssh_port": 22, "qemu_num_cores": 2, "qemu_ram_size": "1G" }New structured schema (bridge networking example):
{ "qemu": { "ram_size": 1024, "cpu_count": 2, "host_qemu_network": { "subnet": "169.254.0.0/16", "ip_address": "169.254.158.190", "mac_address": "52:54:00:12:34:56" } } }