-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Add ability to create extra disks on hyperkit vms #11483
Add ability to create extra disks on hyperkit vms #11483
Conversation
Welcome @BlaineEXE! |
Hi @BlaineEXE. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Can one of the admins verify this patch? |
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.
@BlaineEXE thank you for contributing this interesting PR. do u mind adding in the PR description the After this PR in action ?
I'm happy to add more details or move them as is most helpful. This is my first PR to the minikube repo, and I'm not quite sure what you mean exactly, though. I don't see anything like that in the contributor guide to help me get there. |
thank you for reading the contributor guide and welcome to contributing to minikube. I meant like if you run the PR with this new feature locally and paste the output of how u would use this new feature in the PR description, so it be easier for me to review it |
Done. Let me know if more details would be helpful. |
@medyagh are you able to approve running workflows? I think to fix the issue I mentioned I need to make a change to the |
dc23417
to
5a30c47
Compare
cmd/minikube/cmd/start_flags.go
Outdated
@@ -164,6 +166,8 @@ func initMinikubeFlags() { | |||
startCmd.Flags().StringP(network, "", "", "network to run minikube with. Now it is used by docker/podman and KVM drivers. If left empty, minikube will create a new network.") | |||
startCmd.Flags().StringVarP(&outputFormat, "output", "o", "text", "Format to print stdout in. Options include: [text,json]") | |||
startCmd.Flags().StringP(trace, "", "", "Send trace events. Options include: [gcp]") | |||
startCmd.Flags().Int(extraDisks, 0, "Number of extra disks created and attached to the minikube VM.") |
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.
add (only used by HyperKit driver)
cmd/minikube/cmd/start_flags.go
Outdated
@@ -164,6 +166,8 @@ func initMinikubeFlags() { | |||
startCmd.Flags().StringP(network, "", "", "network to run minikube with. Now it is used by docker/podman and KVM drivers. If left empty, minikube will create a new network.") | |||
startCmd.Flags().StringVarP(&outputFormat, "output", "o", "text", "Format to print stdout in. Options include: [text,json]") | |||
startCmd.Flags().StringP(trace, "", "", "Send trace events. Options include: [gcp]") | |||
startCmd.Flags().Int(extraDisks, 0, "Number of extra disks created and attached to the minikube VM.") | |||
startCmd.Flags().String(extraDiskSize, defaultDiskSize, "Disk size allocated for extra disks attached to the minikube VM (format: <number>[<unit>], where unit = b, k, m or g).") |
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.
add only used by hyperkit driver
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 opted to message this as currently only implemented for hyperkit driver
since I think that makes it more clear that it can be implemented for other drivers but isn't currently. Let me know if your wording is still preferred.
cmd/minikube/cmd/start_flags.go
Outdated
@@ -525,9 +531,15 @@ func updateExistingConfigFromFlags(cmd *cobra.Command, existing *config.ClusterC | |||
// validate the memory size in case user changed their system memory limits (example change docker desktop or upgraded memory.) | |||
validateRequestedMemorySize(cc.Memory, cc.Driver) | |||
|
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.
add a warning if user added this flag to non-hyperkit drivers, that
out.WarningT("Specifying extra disk is currently only supported for hyperkit driver, if you can contribute to add this feature please create a 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.
I added this check for flags on both create and update (see checkExtraDiskOptions
). If I were a user, I think I would expect using these flags for unsupported drivers would cause a failure since the option isn't supported. Would it be better for me to return an error instead of just logging a warning?
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.
since we are not passing the option to the driver if it doesnt suppor it, it wont do anything, I would be okay with ExitWith Usage or just warning. your call !
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.
[Edited] It seems like using a warning is the trend elsewhere, so I will stick with that.
b9d94de
to
410b3da
Compare
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.
also consider adding to the Hyperkit website documentation a section called
"driver specific features"
- add extra disk...
example ...
cmd/minikube/cmd/start_flags.go
Outdated
@@ -118,6 +118,8 @@ const ( | |||
defaultSSHUser = "root" | |||
defaultSSHPort = 22 | |||
listenAddress = "listen-address" | |||
extraDisks = "extra-disks" | |||
extraDiskSize = "extra-disk-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.
for the sake of not having too many flags, what do u think/feel about re-suing the existing the flag we have disk-size
$ minikube start --help | grep disk
--disk-size='20000mb': Disk size allocated to the minikube VM (format: <number>[<unit>], where unit = b, k, m or g).
so if user passes this flag we will use disk-size value as the extra disk size and if they pass --disk-size it will be used for both main disk and extra disk, is that something acceptable ?
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 like that idea, and I thought about doing that. It is more flexible to have separate options, but it does add complexity for the user. I keep waffling on this. For all of the use-cases I have in mind, I don't think there is a very strong reason to have the extra parameter for development just to save a few 10s of gigabytes on a hard disk. I think I will take your suggestion to simply re-use the disk-size
parameter for extra disks as well, and if that ends up being insufficient, it can always be changed in the future.
a39290d
to
456c49b
Compare
@BlaineEXE no worries we can build the ISO on the PR |
/ok-to-build-iso |
ok-to-build-iso |
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 don't see code to clean up the disks, is this handled automatically when removing the VM? Just want to make sure we don't have disk files lingering around when the env is destroyed.
RE: @leseb
I don't think it's necessary. When creating a VM, minikube creates a directory for each machine in the config directory (e.g., |
/ok-to-test |
kvm2 driver with docker runtime
Times for minikube start: 53.2s 49.3s 47.4s 52.9s 50.3s Times for minikube (PR 11483) ingress: 34.8s 34.9s 34.5s 34.2s 42.3s docker driver with docker runtime
Times for minikube ingress: 31.5s 32.7s 33.0s 28.5s 31.5s Times for minikube start: 23.9s 24.6s 22.8s 23.2s 22.7s docker driver with containerd runtime
Times for minikube start: 32.8s 43.3s 43.4s 47.2s 43.9s |
@BlaineEXE sorry for long wait on this PR, do u mind pulling upstream ? |
f5f504f
to
e51052e
Compare
No problem @medyagh. Thanks for looking. I just updated on the upstream master. |
kvm2 driver with docker runtime
Times for minikube start: 47.5s 46.6s 47.2s 46.3s 45.3s Times for minikube ingress: 39.2s 42.7s 34.2s 33.8s 42.7s docker driver with docker runtime
Times for minikube start: 23.7s 21.2s 22.2s 21.9s 21.1s Times for minikube ingress: 36.0s 33.5s 38.0s 38.0s 37.0s docker driver with containerd runtime
Times for minikube start: 32.4s 42.8s 42.9s 43.9s 47.1s |
These are the flake rates of all failed tests on Docker_Linux.
|
@medyagh It's a little unclear to me if the Are there more things I should be doing from here to get this merged? |
/retest-this-please |
kvm2 driver with docker runtime
Times for minikube ingress: 31.3s 32.3s 31.3s 32.8s 31.9s Times for minikube start: 53.8s 51.1s 51.2s 52.1s 51.6s docker driver with docker runtime
Times for minikube (PR 11483) start: 23.0s 21.9s 21.9s 21.7s 23.2s Times for minikube ingress: 27.0s 34.5s 34.5s 36.0s 28.0s docker driver with containerd runtime
Times for minikube start: 32.1s 45.0s 44.3s 43.1s 43.7s |
Add the ability to create and attach extra disks to hyperkit vms. Signed-off-by: Blaine Gardner <blaine.gardner@redhat.com>
e51052e
to
8f05ee0
Compare
ok-to-build-iso |
@BlaineEXE, sorry for the delay here. we'll rebuild the new ISO and run the tests one more time here. if there's a failure outside macOS, it's highly unlikely to be related to this PR. |
kvm2 driver with docker runtime
Times for minikube start: 48.5s 47.7s 47.1s 46.4s 45.9s Times for minikube ingress: 32.3s 32.2s 33.8s 31.7s 32.2s docker driver with docker runtime
Times for minikube ingress: 35.0s 35.5s 36.5s 36.5s 36.5s Times for minikube start: 23.7s 22.1s 21.3s 21.5s 24.0s docker driver with containerd runtime
Times for minikube start: 31.9s 37.0s 43.7s 43.8s 37.4s |
Hi @BlaineEXE, we have updated your PR with the reference to newly built ISO. Pull the changes locally if you want to test with them or update your PR further. |
These are the flake rates of all failed tests.
Too many tests failed - See test logs for more details. To see the flake rates of all tests by environment, click here. |
kvm2 driver with docker runtime
Times for minikube start: 53.1s 47.9s 46.6s 47.5s 51.4s Times for minikube ingress: 32.8s 31.8s 34.8s 38.8s 31.8s docker driver with docker runtime
Times for minikube start: 21.6s 22.2s 23.1s 22.0s 21.7s Times for minikube (PR 11483) ingress: 35.0s 28.5s 26.5s 38.5s 27.5s docker driver with containerd runtime
Times for minikube start: 42.4s 43.4s 36.7s 44.2s 42.8s |
Thanks @sharifelgamal. Would you be able to help me find logs from the script |
I don't personally know where those are collected, if I had to guess, I'd say there are not. Either way, if the logs aren't exported, they're eventually lost. I'm not sure I see where the problem is, all the hyperkit tests passed. I'm comfortable merging this if you are. |
These are the flake rates of all failed tests.
Too many tests failed - See test logs for more details. To see the flake rates of all tests by environment, click here. |
@sharifelgamal I think I feel good if this is merged. The flakes here don't seem to be far off what I see in other PRs, and I have tested this pretty thoroughly on my side with starting, adding, stopping, and re-starting the cluster with the extra disks. |
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.
Sounds good. Thanks for your contribution and your patience!
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: BlaineEXE, sharifelgamal The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Add the ability to create and attach extra disks to hyperkit vms.
For example, if I wish to add 3 extra disk to my minikube VM, each 10GB in size, the command with the 2 new options would look like below.
> minikube start --driver=hyperkit --extra-disks=3 --extra-disk-size=10gb
In the minikube VM, I would expect to see the extra disks (in addition to
vda
andvda1
normally present) available in/dev
as shown below.Signed-off-by: Blaine Gardner blaine.gardner@redhat.com
Fixes #3883
This will be useful for me for developing on Rook, and I think it maybe useful for others doing other types of storage development on Kubernetes with minikube.