-
Notifications
You must be signed in to change notification settings - Fork 5k
Add ability to create extra disks on qemu2 vms #15887
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
Merged
spowelljr
merged 1 commit into
kubernetes:master
from
BlaineEXE:qemu2-add-extra-disk-capability
Jun 9, 2023
Merged
Changes from all commits
Commits
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
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
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
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
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
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
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.
"reduce ID collision" does not sound very promissing. Can we eliminate collisons or
avoid specificing the index, letting qemu handle this?
I don't remember that I had to specify index for drives when using multiple disks
but I usually use libvirt so maybe libvirt handles this for me.
If the intent is to be able to locate the drive later inside the guest,
it is better to specify the drive serial, which will be available in the
guest via the udev links (e.g. /dev/disk/by-id/virtio-{serial}).
It will also be better to use
-device
and-blockdev
instead of-drive
,which I think is also required to set the serial (serial is set on
the device, not on the drive). I could not find any docs about converting
old style
-drive
options to-device
and-blockdev
. Proably the bestway to do this right is to check how libvirt does this.
Anyway I think this can be improved 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.
I tried to use
bus=2,unit=<id>
parameters to use a different bus entirely, but those also collided with other devices like the CDROM drive in my local testing. This seemed like a simple (if fairly blunt) way of preventing that collision for other users and avoiding corner cases as best as possible if the cloud init drive or other options change in the future.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 also think this is not the most robust way of solving this issue, my main concern is if a minikube cluster is created and user deletes the minikube config folder without properly deleting minikube...then in the next minikube run this would colide again?
could we ensure that minikube delete --all deletes the abandoned disks ? simmilar to the orpahned disks in docker driver we have a clean up mechanim for them
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.
Could you try creating two clusters with extra disk and one without extra disk and see if there is a collision with the extra disks? And after deleting ensure that there are no disks left over
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 still confused about how the config folder being deleted could result in problems. I'll go through the behavior I am seeing from minikube, and you can let me know if I'm missing what "config" folder you are talking about.
I don't have anything in my config folder other than an empty
config.json
:I create minikube clusters from CLI only; example:
I have 3 minikube environments using
-p
. The first 2 have 3 extra disks each, and the last has no extra disks.The
~/.minikube/machines
dir has separate disks for each machine profile. TheAs an example, the machine config for profile
minikube2
, located in the minikube2 subdir, looks like this:And the qemu processes that are running are using the correct disks for all 3 vms
If I delete the first minikube cluster, all disks are removed:
I can still ssh to minikube2, lsblk shows vd[b-d] are the extra disks, and partprobe reads the disk successfully.
minikube delete --all
deletes the remaining VMsminikube delete --all --purge
deletes the whole~/.minikube
dir.Does this sufficiently show that disks are handled correctly in the case of multiple differently-configured clusters?