Add disk resize support for QEMU driver#793
Conversation
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughAdds disk and memory sizing controls: new QemuClient CLI and setters; a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as Click CLI
participant Client as QemuClient
participant Driver as Qemu Driver
participant Host as Host FS / qemu-img
User->>CLI: run "resize disk <size>" or "resize memory <size>"
CLI->>Client: set_disk_size(size) / set_memory_size(size)
Client->>Driver: store target size
CLI-->>User: echo confirmation
Note over Driver,Host: On next boot/init when disk_size is set
Driver->>Host: qemu-img info <image>
Host-->>Driver: virtual_size
alt target > virtual_size
Driver->>Host: qemu-img resize <image> <disk_size>
Host-->>Driver: success / error
alt error
Driver->>Driver: raise RuntimeError
end
else target <= virtual_size
Driver->>Driver: skip resize (shrinking blocked or unnecessary)
end
Driver->>Driver: launch QEMU with configured memory (if set)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/client.py (1)
45-70: CLI structure looks good!The nested command group organization is clean and follows Click best practices. Minor consideration: the echo message "Disk will be resized to {size}" could be slightly confusing for relative sizes like "+5G" (which means "grow by 5G" rather than "set to 5G"), but the help text clarifies the accepted formats.
Optional: Slightly clearer message for relative sizes
@resize.command(name="disk") @click.argument("size") def resize_disk(size): """Resize the root disk (e.g., 20G, +5G). Run before power on.""" self.set_disk_size(size) - click.echo(f"Disk will be resized to {size} on next power on") + click.echo(f"Disk size will be set to {size} on next power on")Or alternatively, keep as-is since
qemu-img resizeuses similar terminology.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/client.pypackages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.py
🧰 Additional context used
📓 Path-based instructions (4)
packages/jumpstarter-driver-*/jumpstarter_driver_*/driver.py
📄 CodeRabbit inference engine (.cursor/rules/creating-new-drivers.mdc)
Driver class names should be in CamelCase and be descriptive with appropriate suffixes based on functionality: Power drivers should end with
*Power, Network drivers with*Network, Flasher drivers with*Flasher, Console drivers with*Console, Server drivers with*Server
Files:
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.py
packages/jumpstarter-driver-**/jumpstarter_driver_**/*.py
📄 CodeRabbit inference engine (.cursor/rules/creating-new-drivers.mdc)
Driver implementations should follow existing code style validated with
make lint(fix withmake lint-fix), perform static type checking withmake ty-pkg-${package_name}, add comprehensive tests, and verify all tests pass withmake test-pkg-${package_name}ormake test
Files:
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.pypackages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/client.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/project-structure.mdc)
Ruff should be used for code formatting and linting, excluding
jumpstarter-protocolpackage
Files:
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.pypackages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/client.py
packages/jumpstarter-driver-*/jumpstarter_driver_*/client.py
📄 CodeRabbit inference engine (.cursor/rules/creating-new-drivers.mdc)
Driver client CLIs should either implement known classes that provide a CLI interface, or implement their own CLI interface. Use
CompositeClientfromjumpstarter_driver_composite.clientfor composite drivers with child drivers.
Files:
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/client.py
🧠 Learnings (2)
📚 Learning: 2025-11-27T09:58:41.875Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/client.py : Driver client CLIs should either implement known classes that provide a CLI interface, or implement their own CLI interface. Use `CompositeClient` from `jumpstarter_driver_composite.client` for composite drivers with child drivers.
Applied to files:
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/client.py
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/ : Driver packages must implement a `client.py` file containing the client implementation
Applied to files:
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/client.py
🧬 Code graph analysis (1)
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.py (1)
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/client.py (2)
set_disk_size(23-25)set_memory_size(27-29)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: build
- GitHub Check: e2e
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.11)
🔇 Additional comments (5)
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.py (3)
156-167: LGTM!The disk resize logic is well-integrated into the boot flow. The resize correctly occurs after flashing (when
root.exists()) and before VM startup. Error handling appropriately captures stderr for diagnostics.
263-263: LGTM!The optional
disk_sizefield with sensible defaults enables opt-in disk resizing. The inline comment documenting the format options is helpful.
383-393: LGTM!The setter methods follow the established pattern of other exported methods in this class. Using
@validate_callensures type safety, and runtime validation by QEMU tools is an acceptable approach.packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/client.py (2)
3-7: LGTM!Imports follow the established pattern for driver CLIs using
driver_click_groupas per project conventions.
23-29: LGTM!The client-side setter methods correctly proxy to the driver via RPC, following the established pattern used by property accessors above.
|
cc @mangelajo @bennyz @bkhizgiy for review. thanks! |
|
do we want to consider the resources available on the host? |
|
overall looking good. Adding to @bennyz question, if we try to run the resize cmd and there are not enough resources, will we get some error back? |
|
yep unit tests are a good idea.. I was waiting to see what you think about the approach before I implement those... for the resources, I have this check that is forwarding stderr from qemu-img command. sufficient? |
|
it is thin provisioned so you can request more than what host has, it will fail only when the space is actually used |
|
ah good point.. I need to check that 👍 |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.py:
- Line 289: The comment for disk_size currently claims support for relative
sizes like "+5G" but QemuPower.on() validates/parses only absolute sizes; either
remove the "+5G" example from the disk_size comment to reflect current behavior,
or update QemuPower.on() to detect and preserve a leading '+' or '-' prefix
(e.g., check disk_size[0] in ("+","-"), strip/validate the rest as the absolute
magnitude, then reattach the sign when invoking qemu-img resize) so relative
resizes are passed through unchanged; update the disk_size comment to match the
implemented behavior and adjust any validation logic in QemuPower.on()
accordingly.
- Around line 415-419: The set_memory_size method lacks input validation for the
size string; update set_memory_size to validate the incoming size (reuse the
same logic used by set_disk_size — e.g., call the shared validate_size helper or
the same regex/parse routine) and raise a clear ValueError on invalid formats
before assigning self.mem, so invalid memory specs are caught early; keep the
@validate_call(validate_return=True) decorator and only set self.mem after
successful validation.
- Around line 409-413: The set_disk_size method accepts arbitrary strings but
lacks format validation; update set_disk_size to validate the incoming size
string (e.g., numeric + optional unit like K/M/G/T with or without "B", or
whatever project standard is used) before assigning to self.disk_size and raise
a clear ValueError for invalid formats so callers fail fast; implement the check
inside set_disk_size (or extend the validate_call rule) and keep the assignment
to self.disk_size only after the format check.
- Around line 186-188: The current logic around self.parent.disk_size blindly
appends "iB" and can create invalid strings like "20GBiB"; to fix, normalize the
input before calling TypeAdapter(ByteSize).validate_python by detecting common
suffix patterns (case-insensitive) and only appending the proper unit when
missing: if the string already contains "B" or "iB" (e.g., "GB", "GiB", "MiB")
leave it as-is, if it ends with a single letter unit like "K/M/G/T" append "iB",
and if it has no suffix append a default unit (or reject/raise a clear
validation error); update the code that constructs requested to use this
normalized size string instead of naively doing s + "iB".
🧹 Nitpick comments (1)
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver_test.py (1)
147-164: Consider expanding test coverage.The test validates the happy path effectively, but additional coverage would strengthen the test suite:
- Relative size support: The documentation mentions "+5G" format, but no test covers relative sizes.
- Log verification: The resize operation logs the action (line 204 in driver.py), but this isn't verified.
- Edge case: Test when
requested == current_virtual_size(no resize needed).📝 Additional test cases
@pytest.mark.anyio async def test_resize_no_op_when_equal(): """Resize should be skipped if requested size equals current size.""" driver, current = _setup_resize_test("10G", 10) # requested: 10G, current: 10G with patch("jumpstarter_driver_qemu.driver.run_process", side_effect=_mock_qemu_img_info(current)) as mock_run: with patch("jumpstarter_driver_qemu.driver.Popen"): try: await driver.children["power"].on() except Exception: pass # Verify resize was NOT called resize_calls = [c for c in mock_run.call_args_list if "resize" in c[0][0]] assert not resize_calls, "qemu-img resize should not be called when sizes are equal" @pytest.mark.anyio async def test_resize_with_relative_size(): """Test resize with relative size format.""" driver, current = _setup_resize_test("+5G", 10) # relative: +5G, current: 10G # Implementation would need to handle relative sizes differently # This test would currently fail and indicates a gap in the implementation
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.pypackages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver_test.py
🧰 Additional context used
📓 Path-based instructions (3)
packages/jumpstarter-driver-*/jumpstarter_driver_*/driver.py
📄 CodeRabbit inference engine (.cursor/rules/creating-new-drivers.mdc)
Driver class names should be in CamelCase and be descriptive with appropriate suffixes based on functionality: Power drivers should end with
*Power, Network drivers with*Network, Flasher drivers with*Flasher, Console drivers with*Console, Server drivers with*Server
Files:
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.py
packages/jumpstarter-driver-**/jumpstarter_driver_**/*.py
📄 CodeRabbit inference engine (.cursor/rules/creating-new-drivers.mdc)
Driver implementations should follow existing code style validated with
make lint(fix withmake lint-fix), perform static type checking withmake ty-pkg-${package_name}, add comprehensive tests, and verify all tests pass withmake test-pkg-${package_name}ormake test
Files:
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.pypackages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver_test.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/project-structure.mdc)
Ruff should be used for code formatting and linting, excluding
jumpstarter-protocolpackage
Files:
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.pypackages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver_test.py
🧠 Learnings (2)
📚 Learning: 2025-11-27T09:58:41.875Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-**/jumpstarter_driver_**/*.py : Driver implementations should follow existing code style validated with `make lint` (fix with `make lint-fix`), perform static type checking with `make ty-pkg-${package_name}`, add comprehensive tests, and verify all tests pass with `make test-pkg-${package_name}` or `make test`
Applied to files:
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver_test.py
📚 Learning: 2025-11-27T09:58:41.875Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: After driver creation, implement driver logic in `driver.py`, add tests in `driver_test.py`, update README.md with specific documentation, and test the driver
Applied to files:
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver_test.py
🧬 Code graph analysis (2)
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.py (1)
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/client.py (2)
set_disk_size(23-25)set_memory_size(27-29)
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver_test.py (1)
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.py (1)
on(73-250)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: e2e
🔇 Additional comments (7)
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.py (3)
7-7: LGTM!The import additions are appropriate for the disk resize functionality—
shutilfor disk space checks andByteSize/TypeAdapterfor size parsing and formatting.Also applies to: 24-24
173-173: LGTM!The virtual size calculation correctly falls back to file size when
qemu-img infodoesn't providevirtual-sizeor fails entirely. This ensures the resize logic has a valid baseline to work with.Also applies to: 182-183
203-211: The code is correct as written. According toqemu-imgdocumentation, raw byte integers (e.g.,104857600) are the documented standard format for the SIZE argument and are shown as the primary example in official documentation. The current implementation properly passesstr(requested)as a raw byte count, which is fully supported. Additionally, the code already logs a human-readable version viaByteSize(requested).human_readable()on line 204, so readability in logs is not a concern.Likely an incorrect or invalid review comment.
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver_test.py (4)
1-1: LGTM!The import additions and
anyio_backendfixture are appropriate. Forcing the asyncio backend ensures consistent test behavior across environments.Also applies to: 7-7, 18-21
104-121: LGTM!The helper functions are well-designed.
_setup_resize_testcreates the test environment efficiently using sparse files, and_mock_qemu_img_infoprovides clean mocking for theqemu-img infocommand.
124-131: LGTM!This test correctly validates that the resize logic blocks disk shrinking operations, which aligns with the safety constraints mentioned in the PR objectives.
134-144: LGTM!This test validates the host disk space check. Note that this check may be overly conservative for thin-provisioned qcow2 images (as discussed in the PR comments), but the test correctly validates the current implementation.
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.py
Outdated
Show resolved
Hide resolved
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.py (1)
409-415: Narrow the exception handling to catch specific validation errors.The validation logic is correct, but catching
Exceptionis too broad and could hide unexpected errors. Pydantic'sTypeAdapter.validate_pythonraisesValidationErrorfor validation failures.♻️ Narrow exception handling
+from pydantic import ValidationError + def _validate_size(self, size: str) -> None: """Validate size string (e.g., '20G', '512M').""" try: s = size TypeAdapter(ByteSize).validate_python(s + "iB" if s[-1] in "kmgtKMGT" else s) - except Exception: + except (ValidationError, ValueError): raise ValueError(f"Invalid size: '{size}'. Use e.g. '20G', '512M', '2T'") from None
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/client.pypackages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.py
🧰 Additional context used
📓 Path-based instructions (4)
packages/jumpstarter-driver-*/jumpstarter_driver_*/driver.py
📄 CodeRabbit inference engine (.cursor/rules/creating-new-drivers.mdc)
Driver class names should be in CamelCase and be descriptive with appropriate suffixes based on functionality: Power drivers should end with
*Power, Network drivers with*Network, Flasher drivers with*Flasher, Console drivers with*Console, Server drivers with*Server
Files:
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.py
packages/jumpstarter-driver-**/jumpstarter_driver_**/*.py
📄 CodeRabbit inference engine (.cursor/rules/creating-new-drivers.mdc)
Driver implementations should follow existing code style validated with
make lint(fix withmake lint-fix), perform static type checking withmake ty-pkg-${package_name}, add comprehensive tests, and verify all tests pass withmake test-pkg-${package_name}ormake test
Files:
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.pypackages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/client.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/project-structure.mdc)
Ruff should be used for code formatting and linting, excluding
jumpstarter-protocolpackage
Files:
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.pypackages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/client.py
packages/jumpstarter-driver-*/jumpstarter_driver_*/client.py
📄 CodeRabbit inference engine (.cursor/rules/creating-new-drivers.mdc)
Driver client CLIs should either implement known classes that provide a CLI interface, or implement their own CLI interface. Use
CompositeClientfromjumpstarter_driver_composite.clientfor composite drivers with child drivers.
Files:
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/client.py
🧠 Learnings (5)
📓 Common learnings
Learnt from: evakhoni
Repo: jumpstarter-dev/jumpstarter PR: 793
File: packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.py:196-201
Timestamp: 2026-01-07T14:45:57.173Z
Learning: In the QEMU driver (jumpstarter_driver_qemu), the disk space check before resize is intentionally conservative, checking available space even for thin-provisioned qcow2 images. This is a design decision to prevent potential future issues when the thin-provisioned image is actually filled.
Learnt from: evakhoni
Repo: jumpstarter-dev/jumpstarter PR: 793
File: packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.py:289-289
Timestamp: 2026-01-07T14:47:34.243Z
Learning: In the QEMU driver (jumpstarter_driver_qemu), the disk_size configuration only supports absolute size values (e.g., "20G"), not relative sizes (e.g., "+5G"). This is an intentional design decision to keep validation logic straightforward.
📚 Learning: 2026-01-07T14:45:57.173Z
Learnt from: evakhoni
Repo: jumpstarter-dev/jumpstarter PR: 793
File: packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.py:196-201
Timestamp: 2026-01-07T14:45:57.173Z
Learning: In the QEMU driver (jumpstarter_driver_qemu), keep the disk space check before resize conservative by validating available space even for thin-provisioned qcow2 images. This helps prevent future issues when the thin image is actually filled. Apply this guideline to all Python files within the QEMU driver package to ensure consistent behavior across related resize operations.
Applied to files:
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.pypackages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/client.py
📚 Learning: 2026-01-07T14:47:34.243Z
Learnt from: evakhoni
Repo: jumpstarter-dev/jumpstarter PR: 793
File: packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.py:289-289
Timestamp: 2026-01-07T14:47:34.243Z
Learning: In the QEMU driver (jumpstarter_driver_qemu), the disk_size configuration only supports absolute size values (e.g., "20G"), not relative sizes (e.g., "+5G"). This is an intentional design decision to keep validation logic straightforward.
Applied to files:
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.pypackages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/client.py
📚 Learning: 2025-11-27T09:58:41.875Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/client.py : Driver client CLIs should either implement known classes that provide a CLI interface, or implement their own CLI interface. Use `CompositeClient` from `jumpstarter_driver_composite.client` for composite drivers with child drivers.
Applied to files:
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/client.py
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/ : Driver packages must implement a `client.py` file containing the client implementation
Applied to files:
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/client.py
🧬 Code graph analysis (2)
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.py (1)
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/client.py (2)
set_disk_size(23-25)set_memory_size(27-29)
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/client.py (5)
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.py (3)
client(302-303)set_disk_size(419-422)set_memory_size(426-429)packages/jumpstarter-driver-composite/jumpstarter_driver_composite/client.py (1)
CompositeClient(28-55)packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/fabric.py (1)
FabricAdapter(16-37)packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/novnc.py (1)
NovncAdapter(13-54)packages/jumpstarter/jumpstarter/client/decorators.py (1)
driver_click_group(13-55)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: build
- GitHub Check: e2e
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.11)
🔇 Additional comments (9)
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/client.py (4)
3-3: LGTM!The imports are appropriate for the CLI functionality being added.
Also applies to: 7-7
23-25: LGTM!The method correctly delegates to the driver's
set_disk_sizeimplementation.
27-29: LGTM!The method correctly delegates to the driver's
set_memory_sizeimplementation.
45-70: CLI implementation follows framework patterns correctly.The CLI structure follows the
driver_click_grouppattern demonstrated inCompositeClientand provides clear user feedback. The commands delegate to the validation-enabled setters, which will raise appropriate errors for invalid inputs.packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.py (5)
7-7: LGTM!The additional imports (
shutilandByteSize) are appropriate for the disk resize functionality.Also applies to: 24-24
173-173: LGTM!The extraction of
current_virtual_sizecorrectly handles both qemu-img info output and raw format fallback.Also applies to: 182-182
289-289: LGTM!The
disk_sizefield is appropriately typed and documented. Based on learnings, only absolute size values are supported.
184-211: Resize logic correctly handles disk expansion with appropriate safeguards.The implementation properly:
- Parses disk size using ByteSize validation
- Prevents shrinking operations
- Checks available space conservatively (intentional design for thin-provisioned images)
- Passes byte count to qemu-img resize, which accepts raw byte values per QEMU documentation
- Handles errors with clear messages and stderr capture
The test suite confirms the byte format is correct and the overall logic is sound.
417-422: Validation at setter time is correctly implemented and provides proper feedback.Both
set_disk_sizeandset_memory_sizenow validate immediately via_validate_size, ensuring invalid formats like "+5G" (relative sizing) are rejected at call time. The stored formats ("20G", "2G") are compatible with their respective usage contexts:
disk_sizeis correctly converted to bytes forqemu-img resizememis correctly passed as-is to QEMU's-mflag, which accepts binary format strings
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver_test.py:
- Around line 147-164: The test test_resize_succeeds uses a broad except
Exception: pass around the awaited call to driver.children["power"].on(), which
can mask real failures, and then uses a fragile nested-index assertion on
mock_run.call_args_list; fix by mocking Popen to raise the specific expected
exception (or catch that specific exception type) so only the anticipated
failure is suppressed, and replace the brittle index-based check on resize_calls
with a robust inspection of mock_run.call_args_list (e.g., iterate calls,
join/inspect the command args to find the "resize" invocation and assert the
final size argument equals str(20 * 1024**3)) while still asserting that a
resize call exists.
🧹 Nitpick comments (1)
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver_test.py (1)
139-139: Consider a more readable mock object pattern.The
type()approach works but is unconventional. Consider usingtypes.SimpleNamespaceorunittest.mock.Mockfor better readability.♻️ Alternative approaches
Option 1: Using
types.SimpleNamespace:+from types import SimpleNamespace + - mock_usage = type("U", (), {"free": 5 * 1024**3})() + mock_usage = SimpleNamespace(free=5 * 1024**3)Option 2: Using
unittest.mock.Mock:+from unittest.mock import Mock + - mock_usage = type("U", (), {"free": 5 * 1024**3})() + mock_usage = Mock(free=5 * 1024**3)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver_test.py
🧰 Additional context used
📓 Path-based instructions (2)
packages/jumpstarter-driver-**/jumpstarter_driver_**/*.py
📄 CodeRabbit inference engine (.cursor/rules/creating-new-drivers.mdc)
Driver implementations should follow existing code style validated with
make lint(fix withmake lint-fix), perform static type checking withmake ty-pkg-${package_name}, add comprehensive tests, and verify all tests pass withmake test-pkg-${package_name}ormake test
Files:
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver_test.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/project-structure.mdc)
Ruff should be used for code formatting and linting, excluding
jumpstarter-protocolpackage
Files:
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver_test.py
🧠 Learnings (4)
📓 Common learnings
Learnt from: evakhoni
Repo: jumpstarter-dev/jumpstarter PR: 793
File: packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.py:196-201
Timestamp: 2026-01-07T14:45:57.173Z
Learning: In the QEMU driver (jumpstarter_driver_qemu), the disk space check before resize is intentionally conservative, checking available space even for thin-provisioned qcow2 images. This is a design decision to prevent potential future issues when the thin-provisioned image is actually filled.
Learnt from: evakhoni
Repo: jumpstarter-dev/jumpstarter PR: 793
File: packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.py:289-289
Timestamp: 2026-01-07T14:47:34.243Z
Learning: In the QEMU driver (jumpstarter_driver_qemu), the disk_size configuration only supports absolute size values (e.g., "20G"), not relative sizes (e.g., "+5G"). This is an intentional design decision to keep validation logic straightforward.
📚 Learning: 2026-01-07T14:45:57.173Z
Learnt from: evakhoni
Repo: jumpstarter-dev/jumpstarter PR: 793
File: packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.py:196-201
Timestamp: 2026-01-07T14:45:57.173Z
Learning: In the QEMU driver (jumpstarter_driver_qemu), keep the disk space check before resize conservative by validating available space even for thin-provisioned qcow2 images. This helps prevent future issues when the thin image is actually filled. Apply this guideline to all Python files within the QEMU driver package to ensure consistent behavior across related resize operations.
Applied to files:
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver_test.py
📚 Learning: 2025-11-27T09:58:41.875Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-**/jumpstarter_driver_**/*.py : Driver implementations should follow existing code style validated with `make lint` (fix with `make lint-fix`), perform static type checking with `make ty-pkg-${package_name}`, add comprehensive tests, and verify all tests pass with `make test-pkg-${package_name}` or `make test`
Applied to files:
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver_test.py
📚 Learning: 2026-01-07T14:47:34.243Z
Learnt from: evakhoni
Repo: jumpstarter-dev/jumpstarter PR: 793
File: packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.py:289-289
Timestamp: 2026-01-07T14:47:34.243Z
Learning: In the QEMU driver (jumpstarter_driver_qemu), the disk_size configuration only supports absolute size values (e.g., "20G"), not relative sizes (e.g., "+5G"). This is an intentional design decision to keep validation logic straightforward.
Applied to files:
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver_test.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: e2e
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.12)
🔇 Additional comments (3)
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver_test.py (3)
18-22: LGTM: Consistent async backend configuration.The
anyio_backendfixture ensures all async tests use the same asyncio backend, which improves test reliability and consistency.
113-121: LGTM: Well-structured mock helper.The mock function properly simulates
qemu-img infooutput with appropriate async behavior and return values.
167-192: LGTM: Comprehensive validation test coverage.The validation tests effectively cover both valid and invalid inputs for
set_disk_sizeandset_memory_sizemethods. The test structure is clear, follows pytest conventions, and verifies the expected behavior.
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver_test.py
Outdated
Show resolved
Hide resolved
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver_test.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.py (1)
409-415: LGTM! Validation helper is robust.The
_validate_sizehelper properly validates size strings and provides clear error messages. The method catches bothValidationErrorandIndexErrorto handle edge cases like empty strings.Minor note: The size parsing logic
s + "iB" if s[-1] in "kmgtKMGT" else sappears in both_validate_size(line 413) andQemuPower.on()(line 188). This could optionally be extracted to a tiny helper method to reduce duplication, but given it's just one line in related contexts, the current approach is acceptable.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.pypackages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver_test.py
🧰 Additional context used
📓 Path-based instructions (3)
packages/jumpstarter-driver-*/jumpstarter_driver_*/driver.py
📄 CodeRabbit inference engine (.cursor/rules/creating-new-drivers.mdc)
Driver class names should be in CamelCase and be descriptive with appropriate suffixes based on functionality: Power drivers should end with
*Power, Network drivers with*Network, Flasher drivers with*Flasher, Console drivers with*Console, Server drivers with*Server
Files:
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.py
packages/jumpstarter-driver-**/jumpstarter_driver_**/*.py
📄 CodeRabbit inference engine (.cursor/rules/creating-new-drivers.mdc)
Driver implementations should follow existing code style validated with
make lint(fix withmake lint-fix), perform static type checking withmake ty-pkg-${package_name}, add comprehensive tests, and verify all tests pass withmake test-pkg-${package_name}ormake test
Files:
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.pypackages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver_test.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/project-structure.mdc)
Ruff should be used for code formatting and linting, excluding
jumpstarter-protocolpackage
Files:
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.pypackages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver_test.py
🧠 Learnings (6)
📓 Common learnings
Learnt from: evakhoni
Repo: jumpstarter-dev/jumpstarter PR: 793
File: packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.py:196-201
Timestamp: 2026-01-07T14:45:57.173Z
Learning: In the QEMU driver (jumpstarter_driver_qemu), the disk space check before resize is intentionally conservative, checking available space even for thin-provisioned qcow2 images. This is a design decision to prevent potential future issues when the thin-provisioned image is actually filled.
Learnt from: evakhoni
Repo: jumpstarter-dev/jumpstarter PR: 793
File: packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.py:289-289
Timestamp: 2026-01-07T14:47:34.243Z
Learning: In the QEMU driver (jumpstarter_driver_qemu), the disk_size configuration only supports absolute size values (e.g., "20G"), not relative sizes (e.g., "+5G"). This is an intentional design decision to keep validation logic straightforward.
📚 Learning: 2026-01-07T14:45:57.173Z
Learnt from: evakhoni
Repo: jumpstarter-dev/jumpstarter PR: 793
File: packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.py:196-201
Timestamp: 2026-01-07T14:45:57.173Z
Learning: In the QEMU driver (jumpstarter_driver_qemu), keep the disk space check before resize conservative by validating available space even for thin-provisioned qcow2 images. This helps prevent future issues when the thin image is actually filled. Apply this guideline to all Python files within the QEMU driver package to ensure consistent behavior across related resize operations.
Applied to files:
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.pypackages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver_test.py
📚 Learning: 2026-01-07T14:47:34.243Z
Learnt from: evakhoni
Repo: jumpstarter-dev/jumpstarter PR: 793
File: packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.py:289-289
Timestamp: 2026-01-07T14:47:34.243Z
Learning: In the QEMU driver (jumpstarter_driver_qemu), the disk_size configuration only supports absolute size values (e.g., "20G"), not relative sizes (e.g., "+5G"). This is an intentional design decision to keep validation logic straightforward.
Applied to files:
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.pypackages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver_test.py
📚 Learning: 2025-11-27T09:58:41.875Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-**/jumpstarter_driver_**/*.py : Driver implementations should follow existing code style validated with `make lint` (fix with `make lint-fix`), perform static type checking with `make ty-pkg-${package_name}`, add comprehensive tests, and verify all tests pass with `make test-pkg-${package_name}` or `make test`
Applied to files:
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver_test.py
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/ : Driver packages must implement a `driver.py` file containing the driver implementation
Applied to files:
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver_test.py
📚 Learning: 2025-11-27T09:58:41.875Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: After driver creation, implement driver logic in `driver.py`, add tests in `driver_test.py`, update README.md with specific documentation, and test the driver
Applied to files:
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver_test.py
🧬 Code graph analysis (2)
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.py (1)
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/client.py (2)
set_disk_size(23-25)set_memory_size(27-29)
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver_test.py (1)
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.py (3)
on(73-250)set_disk_size(419-422)set_memory_size(426-429)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: build
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: e2e
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.12)
🔇 Additional comments (12)
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.py (5)
7-7: LGTM! Imports are properly used.The
shutilimport is needed for disk space checking (line 196), andValidationErroris needed for size validation (line 414).Also applies to: 24-24
173-173: LGTM! Robust virtual size detection.The code properly tracks the current virtual size with appropriate fallbacks when
qemu-img infofails or doesn't provide the information.Also applies to: 182-182
184-211: LGTM! Resize logic is comprehensive and handles edge cases.The implementation correctly:
- Prevents shrinking operations (lines 190-194)
- Validates available disk space before resizing (lines 196-201)
- Only resizes when necessary (line 203)
- Reports qemu-img errors clearly (line 211)
Based on learnings, the conservative space check for thin-provisioned images is intentional.
289-289: LGTM! Field correctly documented.The
disk_sizefield comment correctly shows absolute size format only, which aligns with the design decision.Based on learnings, only absolute sizes are supported.
417-429: LGTM! Setters provide early validation.Both
set_disk_sizeandset_memory_sizeproperly validate input before assignment, providing immediate feedback to users. This addresses previous feedback about validating at command send time rather than waiting until boot.The validation ensures invalid formats like
"invalid"are caught early with clear error messages.packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver_test.py (7)
1-1: LGTM! Test infrastructure properly set up.The import additions support the new mocking and async test functionality, and the
anyio_backendfixture ensures consistent async behavior across tests.Also applies to: 7-7, 18-21
104-120: LGTM! Fixture provides proper cleanup.The
resize_testfixture properly creates test drivers and ensures cleanup of temporary directories. The explicit cleanup in lines 119-120 is a defensive best practice, even though garbage collection would typically handle it.
123-131: LGTM! Mock helper is well-structured.The
_mock_qemu_img_infohelper properly simulatesqemu-img infooutput with the necessary fields for testing resize logic.
134-141: LGTM! Shrink prevention properly tested.The test correctly verifies that attempting to shrink a disk raises a
RuntimeErrorwith an appropriate message.
144-154: LGTM! Insufficient space check properly tested.The test correctly verifies that attempting to resize beyond available disk space raises a
RuntimeErrorwith a clear message about insufficient space.
157-174: LGTM! Resize success path properly tested.The test verifies that the resize flow executes correctly and calls
qemu-img resizewith the expected size argument. The improved exception handling (lines 166-168) and assertion robustness (lines 171-174) address previous review feedback well.
177-202: LGTM! Setter validation comprehensively tested.The four tests properly verify that both
set_disk_sizeandset_memory_sizeaccept valid size strings and reject invalid ones with clear error messages. This ensures the early validation logic works as intended.
bkhizgiy
left a comment
bkhizgiy
left a comment
There was a problem hiding this comment.
looking good, added a nitpick command but approved by me :)
| f"requested {self.parent.disk_size}" | ||
| ) | ||
|
|
||
| available = shutil.disk_usage(root.parent).free |
There was a problem hiding this comment.
this could exhaust all the resources on the host, but i think the issue here goes deeper, since if we consider multi tenancy a single client can overcommit and make the host unusable for others. perhaps it is not for this PR, but maybe we need to set hard limits, perhaps as some percentage of what is available
There was a problem hiding this comment.
ah that's pretty easy to implement, we just need a consensus on the maximum percentage, or the host reserved amount, or both. I don't mind including it in this PR as well if we all agree on the amounts.
speaking about potential issues, it goes deeper than that, because iiuc our free space calculation will not take into account the virtual size of other thin-provisioned guests on that host, and assuming they're all stored in the same partition, each of them theoretically allowed to grow not taking into account the growth of the others.. however, in order to address that properly, we'll probably have to either iterate over all other images stored in the host at runtime, or alternatively maintain a data structure storing this, and update it at each resize, both of which seems to me like an overkill for such a niche feature. maybe I'm shooting myself in the foot writing this one 😆 but it had to be said.
lastly, ideally we should not be implementation-specific on the user-facing side, but rather let the user make a lease request with his needs, and figure out on our side what we can do in order to satisfy it, if the resources and user permissions allow it (something similar to https://github.com/jumpstarter-dev/jumpstarter/issues/724)
that being said, it's a slippery slope and a deep rabbit hole, not sure if we have the resources to dive into that one 😉
anyway, let me know what you think about implementing a percentage or something, in this (or a follow up) PR.
There was a problem hiding this comment.
well, some is left for the lab admin to decide the configuration of the host, and how many exporters they set up on each host, and how much each gets, and whether they set a separate partition for jumpstarter storage
another problem however is that it would be easy to violate the quota constraints without trying too hard when the image is a compressed qcow2 with a large virtual size
There was a problem hiding this comment.
well, this one at least have "some" constrains :) we're probably need to allow less than (root.parent).free for a safe margin, but iiuc without this PR just flashing a qemu image have no limits at all, so this one's a step in a good direction in a sense 😉
so what do you suggest?
There was a problem hiding this comment.
i'm not saying it's bad, i'm just thinking out loud :) let's merge for now and circle back
so please squash the commits and i'll merge
There was a problem hiding this comment.
need to squash manually via rebase then force-push, or there's any facilities available for it in the repo?
I thought Allow squash merging is enabled in this repo so you can choice it from your side while you merge?
There was a problem hiding this comment.
force push...
yeah we just use merge commits too so i don't want to make it inconsistent
There was a problem hiding this comment.
ok done 👌 squashed into 3 separated by functionality. the first adds the resize, second one is the j command and third have the tests. fine like that or have to be one?
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.py (1)
407-426: Well-designed size parsing and setter methods.The implementation is clean and correct:
_parse_size(lines 407-412): Clever logic that appends "iB" to bare unit letters (e.g., "20G" → "20GiB") to force binary interpretation, while passing through explicit formats ("20GiB", "20GB") forByteSizeto parse. Error handling properly catches invalid inputs with helpful error messages.
set_disk_sizeandset_memory_size(lines 414-426): Both follow a consistent pattern of validating input before storing. The validation ensures the size string is parseable without converting it, allowing QEMU to interpret the format directly in the command line.The
@validate_calldecorators provide additional safety for RPC calls, and the docstrings clearly explain each method's purpose.Optional: Add inline comment explaining the "iB" append logic
Consider adding a brief comment at line 410 to explain why "iB" is appended:
def _parse_size(self, size: str) -> int: """Parse size string (e.g., '20G') to bytes.""" try: + # Append 'iB' to bare unit letters (G→GiB) to force binary interpretation return int(TypeAdapter(ByteSize).validate_python(size + "iB" if size[-1] in "kmgtKMGT" else size)) except (ValidationError, IndexError): raise ValueError(f"Invalid size: '{size}'. Use e.g. '20G', '512M', '2T'") from NoneThis is optional as the logic is relatively straightforward.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/client.pypackages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.pypackages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver_test.py
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver_test.py
- packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/client.py
🧰 Additional context used
📓 Path-based instructions (3)
packages/jumpstarter-driver-*/jumpstarter_driver_*/driver.py
📄 CodeRabbit inference engine (.cursor/rules/creating-new-drivers.mdc)
Driver class names should be in CamelCase and be descriptive with appropriate suffixes based on functionality: Power drivers should end with
*Power, Network drivers with*Network, Flasher drivers with*Flasher, Console drivers with*Console, Server drivers with*Server
Files:
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.py
packages/jumpstarter-driver-**/jumpstarter_driver_**/*.py
📄 CodeRabbit inference engine (.cursor/rules/creating-new-drivers.mdc)
Driver implementations should follow existing code style validated with
make lint(fix withmake lint-fix), perform static type checking withmake ty-pkg-${package_name}, add comprehensive tests, and verify all tests pass withmake test-pkg-${package_name}ormake test
Files:
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/project-structure.mdc)
Ruff should be used for code formatting and linting, excluding
jumpstarter-protocolpackage
Files:
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.py
🧠 Learnings (3)
📓 Common learnings
Learnt from: evakhoni
Repo: jumpstarter-dev/jumpstarter PR: 793
File: packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.py:196-201
Timestamp: 2026-01-07T14:45:57.173Z
Learning: In the QEMU driver (jumpstarter_driver_qemu), the disk space check before resize is intentionally conservative, checking available space even for thin-provisioned qcow2 images. This is a design decision to prevent potential future issues when the thin-provisioned image is actually filled.
Learnt from: evakhoni
Repo: jumpstarter-dev/jumpstarter PR: 793
File: packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.py:289-289
Timestamp: 2026-01-07T14:47:34.243Z
Learning: In the QEMU driver (jumpstarter_driver_qemu), the disk_size configuration only supports absolute size values (e.g., "20G"), not relative sizes (e.g., "+5G"). This is an intentional design decision to keep validation logic straightforward.
📚 Learning: 2026-01-07T14:45:57.173Z
Learnt from: evakhoni
Repo: jumpstarter-dev/jumpstarter PR: 793
File: packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.py:196-201
Timestamp: 2026-01-07T14:45:57.173Z
Learning: In the QEMU driver (jumpstarter_driver_qemu), keep the disk space check before resize conservative by validating available space even for thin-provisioned qcow2 images. This helps prevent future issues when the thin image is actually filled. Apply this guideline to all Python files within the QEMU driver package to ensure consistent behavior across related resize operations.
Applied to files:
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.py
📚 Learning: 2026-01-07T14:47:34.243Z
Learnt from: evakhoni
Repo: jumpstarter-dev/jumpstarter PR: 793
File: packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.py:289-289
Timestamp: 2026-01-07T14:47:34.243Z
Learning: In the QEMU driver (jumpstarter_driver_qemu), the disk_size configuration only supports absolute size values (e.g., "20G"), not relative sizes (e.g., "+5G"). This is an intentional design decision to keep validation logic straightforward.
Applied to files:
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.py
🧬 Code graph analysis (1)
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.py (1)
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/client.py (2)
set_disk_size(23-25)set_memory_size(27-29)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: build
- GitHub Check: e2e
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.13)
🔇 Additional comments (3)
packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.py (3)
7-7: LGTM - Import additions support disk resize functionality.The new imports are appropriately used for disk space checking (
shutil.disk_usage) and size validation (TypeAdapter,ByteSizefrom pydantic).Also applies to: 24-24
287-287: LGTM - Clear field definition with appropriate typing.The
disk_sizefield is well-documented and correctly typed as optional. The inline comment clearly explains its purpose.Based on learnings: This field only supports absolute size values (e.g., "20G"), not relative sizes (e.g., "+5G"), which is an intentional design decision for straightforward validation.
173-173: Well-implemented disk resize flow with comprehensive validation.The implementation correctly handles disk resizing before VM boot:
- Obtains current virtual disk size from image metadata with fallback to file size
- Prevents shrinking operations with a clear error message
- Performs conservative disk space validation—intentionally checks full requested size even for thin-provisioned images per design decision
- Only performs resize when expansion is needed, avoiding unnecessary operations
- Properly forwards
qemu-imgerrors to the userThe byte count format passed to
qemu-img resizeis correct; the test suite validates this format withassert resize_cmd[-1] == str(20 * 1024**3).
3 participants
closing https://github.com/jumpstarter-dev/jumpstarter/issues/792
Adds the ability to resize QEMU disk images before boot. This enables users to expand disk images to a larger size without manual intervention.
Changes
Driver (
driver.py):disk_size: str | Noneconfig field toQemuclassQemuPower.on()usingqemu-img resizebefore VM bootset_disk_size()exported method for runtime overrideClient (
client.py):QemuClient.cli()with newresizecommand groupj qemu resize disk <size>command for runtime disk size configurationUsage
Via exporter config:
Via CLI (overrides config):
Override priority
CLI command > config file
Notes
Bonus
Also added j qemu resize memory command for overriding memory allocation at runtime, following the same pattern.
Summary by CodeRabbit
New Features
Bug Fixes / Validation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.