kvm2 driver: add dedicated network & static ip #10792
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/M
cmd/minikube/cmd/start_flags.go
Outdated
| @@ -342,6 +344,7 @@ func generateClusterConfig(cmd *cobra.Command, existing *config.ClusterConfig, k | |||
| HypervUseExternalSwitch: viper.GetBool(hypervUseExternalSwitch), | |||
| HypervExternalAdapter: viper.GetString(hypervExternalAdapter), | |||
| KVMNetwork: viper.GetString(kvmNetwork), | |||
| KVMPrivateNetwork: viper.GetString(kvmPrivateNetwork), | |||
There was a problem hiding this comment.
don't we need to add a clean up logic ? so when we delete minikube we delete the created networks too ?
There was a problem hiding this comment.
that should already be an automated part of the delete - pls have a look at the cleanup (last) section in the example given in the description above
There was a problem hiding this comment.
this is the code segment that takes care of that:
minikube/pkg/drivers/kvm/kvm.go
Lines 427 to 442 in ff0e25a
cmd/minikube/cmd/start_flags.go
Outdated
| @@ -191,7 +192,8 @@ func initDriverFlags() { | |||
| startCmd.Flags().Bool("vm", false, "Filter to use only VM Drivers") | |||
|
|
|||
| // kvm2 | |||
| startCmd.Flags().String(kvmNetwork, "default", "The KVM network name. (kvm2 driver only)") | |||
| startCmd.Flags().String(kvmNetwork, "default", "The KVM default network name. (kvm2 driver only)") | |||
| startCmd.Flags().String(kvmPrivateNetwork, "", "The KVM private network name. (kvm2 driver only) (default: 'mk-<cluster_name>')") | |||
There was a problem hiding this comment.
what is the difference of kvmNetwork and kvmPrivateNetwork
do u mind clearifying it in the help text ?
can a user specify both of them ?
how about adding some docs about it in the kvm page ?
https://minikube.sigs.k8s.io/docs/drivers/kvm2/
There was a problem hiding this comment.
kvmNetwork was there from earlier, i've just added a more descriptive message to clearly distinguish it from the new kvmPrivateNetwork flag;
kvmNetwork references the default kvm network - the one that comes with standard libvirt installation to support nat, and is always called "default"; i don't think one can change this name for libvirt, and in the code, we just check if that network exists and if is active - ie, we do not try to create 'default' network and fail if it does not already exist!
i only guess that it was maybe added with the idea if it could be customised in libvirt sometime in the future (if anyone would want that)? if it is not used - better removed
on the other hand, the new kvmPrivateNetwork allows user to specify a name for the custom dedicated kvm cluster network that we make possible with this pr
proposal for code cleanup and docs update (maybe in a separate issue, not to block this one?):
- check and remove if not used kvmNetwork flag and all references (incl the one in the cluster config) used also to pass it to the kvm driver, meant to "[re]define" 'default' kvm network
- further, see if we need/use the 'default' kvm network at all - currently, each kvm vm/cluster is created with two interfaces: the 'default' one and the other is fixed 'minikube-net' (while after this pr each kvm cluster would have its own custom private kvm network):
minikube/pkg/drivers/kvm/domain.go
Lines 69 to 78 in 63a8f28
- update the docs accordingly
i'm happy to check if we need/use the 'default' kvm network at all and do the above-proposed actions
There was a problem hiding this comment.
@prezha I suggest we use the existing
--network='': network to run minikube with. Only available with the docker/podman drivers. If left empty, minikube will create a new network.
flag except we update the Help text to say "now it is used by docker/podman and KVM"
There was a problem hiding this comment.
@medya yes, that makes sense: i'll change pr so that it uses the existing '--network' flag for private kvm network
prezha
added
co/kvm2-driver
k8s-ci-robot
added
size/XL
prezha
changed the title
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
|
/ok-to-test |
k8s-ci-robot
added
the
ok-to-test
cmd/minikube/cmd/start_flags.go
Outdated
| @@ -191,7 +192,8 @@ func initDriverFlags() { | |||
| startCmd.Flags().Bool("vm", false, "Filter to use only VM Drivers") | |||
|
|
|||
| // kvm2 | |||
| startCmd.Flags().String(kvmNetwork, "default", "The KVM network name. (kvm2 driver only)") | |||
| startCmd.Flags().String(kvmNetwork, "default", "The KVM default network name. (kvm2 driver only)") | |||
| startCmd.Flags().String(kvmPrivateNetwork, "", "The KVM private network name. (kvm2 driver only) (default: 'mk-<cluster_name>')") | |||
There was a problem hiding this comment.
@prezha I suggest we use the existing
--network='': network to run minikube with. Only available with the docker/podman drivers. If left empty, minikube will create a new network.
flag except we update the Help text to say "now it is used by docker/podman and KVM"
|
kvm2 Driver Times for Minikube (PR 10792): 64.1s 67.5s 65.2s Averages Time Per Log docker Driver Times for Minikube (PR 10792): 26.3s 27.6s 30.0s Averages Time Per Log |
|
kvm2 Driver Times for Minikube (PR 10792): 66.3s 66.1s 66.2s Averages Time Per Log docker Driver Times for Minikube (PR 10792): 31.9s 37.2s 32.5s Averages Time Per Log |
prezha
changed the title
k8s-ci-robot
added
the
do-not-merge/work-in-progress
|
kvm2 Driver Times for Minikube (PR 10792): 63.6s 64.6s 63.6s Averages Time Per Log docker Driver Times for Minikube (PR 10792): 25.5s 27.0s 26.9s Averages Time Per Log |
prezha
force-pushed
the
kvm-network
branch
3 times, most recently
from
5bac07a
to
f7f8701
Compare
|
kvm2 Driver Times for Minikube (PR 10792): 36.5s 25.5s 26.0s Averages Time Per Log |
|
kvm2 Driver Times for Minikube (PR 10792): 27.3s 25.7s 26.9s Averages Time Per Log |
|
kvm2 Driver Times for Minikube (PR 10792): 29.9s 26.5s 24.7s Averages Time Per Log |
|
kvm2 Driver Times for Minikube (PR 10792): 32.2s 25.6s 26.4s Averages Time Per Log |
prezha
force-pushed
the
kvm-network
branch
2 times, most recently
from
0e6fcbc
to
7218c9e
Compare
prezha
changed the title
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
|
kvm2 Driver Times for Minikube (PR 10792): 30.0s 31.2s 29.8s Averages Time Per Log |
|
/retest-this-please |
1 similar comment
|
/retest-this-please |
|
thank you for this PR |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: medyagh, prezha 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 |
k8s-ci-robot
added
the
approved
fixes #9809
update2: as per @medyagh suggestion below (#10792 (comment)), i've reused the existing
--networkflag for specifying the private kvm network nameupdate: i've also added at the bottom of this description a section 'local tests passed - results' with test logs and env state
this pr creates a dedicated libvirt network (and a network bridge) for each new kvm2 cluster, also assigning each machine with a static ip that is preserved across restarts
also, during cluster creation, it's possible to specify a custom name for a private network using the existing
--networkflag (defaulting to 'mk-<cluster_name>')note:
multinode test with kvm2 driver passed w/o issues
example
start
create 1st kvm machine ("kvm0")
create 2nd kvm machine ("minikube")
create 3rd kvm machine ("kvm1") specifying
--kvm-private-networkflagresult
stop 3rd kvm machine ("kvm1")
try to restart 3rd kvm machine ("kvm1") with different
--kvm-private-networkflagresult
cleanup
local tests passed - results:
TestFunctional.log
TestMultiNode.log