Fix issue 2513

pkg/drivers/kvm/domain.go

@@ -62,7 +62,7 @@ const domainTmpl = `
     </interface>
     <interface type='network'>
       <source network='{{.PrivateNetwork}}'/>
-      <mac address='{{.MAC}}'/>
+      <mac address='{{.PrivateMAC}}'/>
       <model type='virtio'/>
     </interface>
     <serial type='pty'>
@@ -98,7 +98,7 @@ $ sudo usermod -a -G libvirt $(whoami)
 # NOTE: For older Debian/Ubuntu versions change the group to [libvirtd]
 $ newgrp libvirt
 
-Visit https://github.com/kubernetes/minikube/blob/master/docs/drivers.md#kvm-driver for more information.
+Visit https://github.com/kubernetes/minikube/blob/master/docs/drivers.md#kvm2-driver for more information.
 `
 
 func randomMAC() (net.HardwareAddr, error) {
@@ -152,9 +152,27 @@ func closeDomain(dom *libvirt.Domain, conn *libvirt.Connect) error {
 }
 
 func (d *Driver) createDomain() (*libvirt.Domain, error) {
+	// create random MAC addresses first for our NICs
+	if d.MAC == "" {
+		mac, err := randomMAC()
+		if err != nil {
+			return nil, errors.Wrap(err, "generating mac address")
+		}
+		d.MAC = mac.String()
+	}
+
+	if d.PrivateMAC == "" {
+		mac, err := randomMAC()
+		if err != nil {
+			return nil, errors.Wrap(err, "generating mac address")
+		}
+		d.PrivateMAC = mac.String()
+	}
+
+	// create the XML for the domain using our domainTmpl template
 	tmpl := template.Must(template.New("domain").Parse(domainTmpl))
-	var domainXml bytes.Buffer
-	if err := tmpl.Execute(&domainXml, d); err != nil {
+	var domainXML bytes.Buffer
+	if err := tmpl.Execute(&domainXML, d); err != nil {
 		return nil, errors.Wrap(err, "executing domain xml")
 	}
 
@@ -164,9 +182,10 @@ func (d *Driver) createDomain() (*libvirt.Domain, error) {
 	}
 	defer conn.Close()
 
-	dom, err := conn.DomainDefineXML(domainXml.String())
+	// define the domain in libvirt using the generated XML
+	dom, err := conn.DomainDefineXML(domainXML.String())
 	if err != nil {
-		return nil, errors.Wrapf(err, "Error defining domain xml: %s", domainXml.String())
+		return nil, errors.Wrapf(err, "Error defining domain xml: %s", domainXML.String())
 	}
 
 	return dom, nil

pkg/drivers/kvm/kvm.go

@@ -66,6 +66,10 @@ type Driver struct {
 	// If empty, a random MAC will be generated.
 	MAC string
 
+	// The randomly generated MAC Address for the NIC attached to the private network
+	// If empty, a random MAC will be generated.
+	PrivateMAC string
+
 	// Whether to passthrough GPU devices from the host to the VM.
 	GPU bool
 
@@ -211,6 +215,21 @@ func (d *Driver) Restart() error {
 }
 
 func (d *Driver) Start() error {
+	// if somebody/something deleted the network in the meantime,
+	// we might need to recreate it. It's (nearly) a noop if the network exists.
+	log.Info("Creating network...")
+	err := d.createNetwork()
+	if err != nil {
+		return errors.Wrap(err, "creating network")
+	}
+
+	// this call ensures that all networks are active
+	log.Info("Ensuring networks are active...")
+	err = d.ensureNetwork()
+	if err != nil {
+		return errors.Wrap(err, "ensuring active networks")
+	}
+
 	log.Info("Getting domain xml...")
 	dom, conn, err := d.getDomain()
 	if err != nil {
@@ -348,18 +367,15 @@ func (d *Driver) Remove() error {
 	}
 	defer conn.Close()
 
-	//Tear down network and disk if they exist
-	log.Debug("Checking if the network needs to be deleted")
-	network, err := conn.LookupNetworkByName(d.PrivateNetwork)
-	if err != nil {
-		log.Warn("Network %s does not exist, nothing to clean up...", d.PrivateNetwork)
-	}
-	if network != nil {
-		log.Infof("Network %s exists, removing...", d.PrivateNetwork)
-		network.Destroy()
-		network.Undefine()
+	// Tear down network if it exists and is not in use by another minikube instance
+	log.Debug("Trying to delete the networks (if possible)")
+	if err := d.deleteNetwork(); err != nil {
+		log.Warnf("Deleting of networks failed: %s", err.Error())
+	} else {
+		log.Info("Successfully deleted networks")
 	}
 
+	// Tear down the domain now
 	log.Debug("Checking if the domain needs to be deleted")
 	dom, err := conn.LookupDomainByName(d.MachineName)
 	if err != nil {

pkg/drivers/kvm/network.go

@@ -19,6 +19,7 @@ package kvm
 import (
 	"bytes"
 	"encoding/json"
+	"encoding/xml"
 	"fmt"
 	"io/ioutil"
 	"strings"
@@ -43,11 +44,15 @@ const networkTmpl = `
 </network>
 `
 
+// setupNetwork ensures that the network with `name` is started (active)
+// and has the autostart feature set.
 func setupNetwork(conn *libvirt.Connect, name string) error {
-	n, err := conn.LookupNetworkByName(defaultNetworkName)
+	n, err := conn.LookupNetworkByName(name)
 	if err != nil {
 		return errors.Wrapf(err, "checking network %s", name)
 	}
+
+	// always ensure autostart is set on the network
 	autostart, err := n.GetAutostart()
 	if err != nil {
 		return errors.Wrapf(err, "checking network %s autostart", name)
@@ -58,6 +63,7 @@ func setupNetwork(conn *libvirt.Connect, name string) error {
 		}
 	}
 
+	// always ensure the network is started (active)
 	active, err := n.IsActive()
 	if err != nil {
 		return errors.Wrapf(err, "checking network status for %s", name)
@@ -67,52 +73,175 @@ func setupNetwork(conn *libvirt.Connect, name string) error {
 			return errors.Wrapf(err, "starting network %s", name)
 		}
 	}
+
 	return nil
+}
+
+// ensureNetwork is called on start of the VM
+func (d *Driver) ensureNetwork() error {
+	conn, err := getConnection()
+	if err != nil {
+		return errors.Wrap(err, "getting libvirt connection")
+	}
+	defer conn.Close()
+
+	// network: default
+
+	// Start the default network
+	// It is assumed that the libvirt/kvm installation has already created this network
+	log.Infof("Ensuring network %s is active", defaultNetworkName)
+	if err := setupNetwork(conn, defaultNetworkName); err != nil {
+		return err
+	}
+
+	// network: private
+
+	// Start the private network
+	log.Infof("Ensuring network %s is active", d.PrivateNetwork)
+	if err := setupNetwork(conn, d.PrivateNetwork); err != nil {
+		return err
+	}
 
+	return nil
 }
 
+// createNetwork is called during creation of the VM only (and not on start)
 func (d *Driver) createNetwork() error {
-	if d.MAC == "" {
-		mac, err := randomMAC()
+	conn, err := getConnection()
+	if err != nil {
+		return errors.Wrap(err, "getting libvirt connection")
+	}
+	defer conn.Close()
+
+	// network: default
+	// It is assumed that the libvirt/kvm installation has already created this network
+
+	// network: private
+
+	// Only create the private network if it does not already exist
+	if _, err := conn.LookupNetworkByName(d.PrivateNetwork); err != nil {
+		// create the XML for the private network from our networkTmpl
+		tmpl := template.Must(template.New("network").Parse(networkTmpl))
+		var networkXML bytes.Buffer
+		if err := tmpl.Execute(&networkXML, d); err != nil {
+			return errors.Wrap(err, "executing network template")
+		}
+
+		// define the network using our template
+		network, err := conn.NetworkDefineXML(networkXML.String())
 		if err != nil {
-			return errors.Wrap(err, "generating mac address")
+			return errors.Wrapf(err, "defining network from xml: %s", networkXML.String())
+		}
+
+		// and finally create it
+		if err := network.Create(); err != nil {
+			return errors.Wrapf(err, "creating network %s", d.PrivateNetwork)
 		}
-		d.MAC = mac.String()
 	}
+
+	return nil
+}
+
+func (d *Driver) deleteNetwork() error {
+	type source struct {
+		//XMLName xml.Name `xml:"source"`
+		Network string `xml:"network,attr"`
+	}
+	type iface struct {
+		//XMLName xml.Name `xml:"interface"`
+		Source source `xml:"source"`
+	}
+	type result struct {
+		//XMLName xml.Name `xml:"domain"`
+		Name       string  `xml:"name"`
+		Interfaces []iface `xml:"devices>interface"`
+	}
+
 	conn, err := getConnection()
 	if err != nil {
 		return errors.Wrap(err, "getting libvirt connection")
 	}
 	defer conn.Close()
 
-	tmpl := template.Must(template.New("network").Parse(networkTmpl))
-	var networkXML bytes.Buffer
-	if err := tmpl.Execute(&networkXML, d); err != nil {
-		return errors.Wrap(err, "executing network template")
-	}
+	// network: default
+	// It is assumed that the OS manages this network
 
-	// Start the default network
-	log.Infof("Setting up network %s", defaultNetworkName)
-	if err := setupNetwork(conn, defaultNetworkName); err != nil {
-		return err
+	// network: private
+	log.Debugf("Checking if network %s exists...", d.PrivateNetwork)
+	network, err := conn.LookupNetworkByName(d.PrivateNetwork)
+	if err != nil {
+		// TODO: decide if we really wanna throw an error?
+		return errors.Wrap(err, "network %s does not exist")
 	}
+	log.Debugf("Network %s exists", d.PrivateNetwork)
 
-	//Check if network already exists
-	if _, err := conn.LookupNetworkByName(d.PrivateNetwork); err == nil {
-		return nil
+	// iterate over every (also turned off) domains, and check if it
+	// is using the private network. Do *not* delete the network if
+	// that is the case
+	log.Debug("Trying to list all domains...")
+	doms, err := conn.ListAllDomains(0)
+	if err != nil {
+		return errors.Wrap(err, "list all domains")
 	}
+	log.Debugf("Listed all domains: total of %d domains", len(doms))
 
-	network, err := conn.NetworkDefineXML(networkXML.String())
-	if err != nil {
-		return errors.Wrapf(err, "defining network from xml: %s", networkXML.String())
+	// fail if there are 0 domains
+	if len(doms) == 0 {
+		log.Warn("list of domains is 0 length")
 	}
-	if err := network.Create(); err != nil {
-		return errors.Wrapf(err, "creating network %s", d.PrivateNetwork)
+
+	for _, dom := range doms {
+		// get the name of the domain we iterate over
+		log.Debug("Trying to get name of domain...")
+		name, err := dom.GetName()
+		if err != nil {
+			return errors.Wrap(err, "failed to get name of a domain")
+		}
+		log.Debugf("Got domain name: %s", name)
+
+		// skip the domain if it is our own machine
+		if name == d.MachineName {
+			log.Debug("Skipping domain as it is us...")
+			continue
+		}
+
+		// unfortunately, there is no better way to retrieve a list of all defined interfaces
+		// in domains than getting it from the defined XML of all domains
+		// NOTE: conn.ListAllInterfaces does not help in this case
+		log.Debugf("Getting XML for domain %s...", name)
+		xmlString, err := dom.GetXMLDesc(libvirt.DOMAIN_XML_INACTIVE)
+		if err != nil {
+			return errors.Wrapf(err, "failed to get XML of domain '%s'", name)
+		}
+		log.Debugf("Got XML for domain %s", name)
+
+		v := result{}
+		err = xml.Unmarshal([]byte(xmlString), &v)
+		if err != nil {
+			return errors.Wrapf(err, "failed to unmarshal XML of domain '%s", name)
+		}
+		log.Debugf("Unmarshaled XML for domain %s: %#v", name, v)
+
+		// iterate over the found interfaces
+		for _, i := range v.Interfaces {
+			if i.Source.Network == d.PrivateNetwork {
+				log.Debugf("domain %s DOES use network %s, aborting...", name, d.PrivateNetwork)
+				return fmt.Errorf("network still in use at least by domain '%s',", name)
+			}
+			log.Debugf("domain %s does not use network %s", name, d.PrivateNetwork)
+		}
 	}
 
-	log.Infof("Setting up network %s", d.PrivateNetwork)
-	if err := setupNetwork(conn, d.PrivateNetwork); err != nil {
-		return err
+	// when we reach this point, it means it is safe to delete the network
+	log.Debugf("Trying to destroy network %s...", d.PrivateNetwork)
+	err = network.Destroy()
+	if err != nil {
+		return errors.Wrap(err, "network destroy")
+	}
+	log.Debugf("Trying to undefine network %s...", d.PrivateNetwork)
+	err = network.Undefine()
+	if err != nil {
+		return errors.Wrap(err, "network undefine")
 	}
 
 	return nil
@@ -136,6 +265,7 @@ func (d *Driver) lookupIP() (string, error) {
 		return d.lookupIPFromLeasesFile()
 	}
 
+	// TODO: for everything > 1002006, there is direct support in the libvirt-go for handling this
 	return d.lookupIPFromStatusFile(conn)
 }
 
@@ -167,7 +297,7 @@ func (d *Driver) lookupIPFromStatusFile(conn *libvirt.Connect) (string, error) {
 
 	ipAddress := ""
 	for _, status := range statusEntries {
-		if status.MacAddress == d.MAC {
+		if status.MacAddress == d.PrivateMAC {
 			ipAddress = status.IPAddress
 		}
 	}
@@ -192,7 +322,7 @@ func (d *Driver) lookupIPFromLeasesFile() (string, error) {
 		if len(entry) != 5 {
 			return "", fmt.Errorf("Malformed leases entry: %s", entry)
 		}
-		if entry[1] == d.MAC {
+		if entry[1] == d.PrivateMAC {
 			ipAddress = entry[2]
 		}
 	}