This repository was archived by the owner on Jan 23, 2026. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 21
Add disk resize support for QEMU driver #793
Merged
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well, this one at least have "some" constrains :) we're probably need to allow less than
(root.parent).freefor 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to squash manually via rebase then force-push, or there's any facilities available for it in the repo?
I thought
Allow squash mergingis enabled in this repo so you can choice it from your side while you merge?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
force push...
yeah we just use merge commits too so i don't want to make it inconsistent
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?