WIP: Remove mac from xml network def

[MalloZup]

What does this PR?

In this Pr when when we create a new network interface we don't assign a random generate MAC address anymore on the xml-definition.
Instead we delegate it to libvirt which autogenerate it randomly.

• remove of code for mac adress generation.
• adapt to wait_for_lease

addition:

• this pr additionaly fix wait_for_lease in doc and in test which was wait_for_lease = 1 but we need a boolean wait_for_lease = true

Previous behaviour:

In past we were assigning a random mac adress to the XML definition.
This doesn"t play well with IPV6 see (Fix #396)

Libvirt upstream doc

---

from https://avdv.github.io/libvirt/formatnetwork.html

---

mac
The address attribute defines a MAC (hardware) address formatted as 6 groups of 2-digit hexadecimal numbers, the groups separated by colons (eg, "52:54:00:1C:DA:2F"). This MAC address is assigned to the bridge device when it is created. Generally it is best to not specify a MAC address when creating a network - in this case, if a defined MAC address is needed for proper operation, libvirt will automatically generate a random MAC address and save it in the config. Allowing libvirt to generate the MAC address will assure that it is compatible with the idiosyncrasies of the platform where libvirt is running.

What is left todo:

• check other removal needed

• tests network setups

• finish check wait_for_lease mechanism.

libvirt will autogenerate a proper one.

---

special thx to @lgaggini

[dmacvicar]

I would expect the explanation of the PR and some context in the main description. Seeing that you add information to the PR as additional comments makes it confusing. Also the TODO list could be the last part of the main description, and less important than the main content.

[dmacvicar]

I think I understand the removal above, as it fits the description of why we remove the bridge address, but I don't understand why we remove the matching in the DHCP host list.

[MalloZup]

the mac here is only testing the unmarshalling mechanism as unit-test.
We can leave it, yop.

[MalloZup]

Currently with this patch, the wait_for_lease mechanism is not working 100%
this works:

provider "libvirt" {
  uri = "qemu:///system"
}

resource "libvirt_domain" "domain" {
  name = "domain-${count.index}"
  network_interface {
    network_name = "default"
    wait_for_lease = true
  }

  count = 1
}
      

We get the IP after waiting for lease correctly.

this won't work ( previous behaviour was working)

provider "libvirt" {
    uri = "qemu:///system"
}

resource "libvirt_network" "public4"{
  name = "public4"
  mode = "route"
  bridge = "br-public4"
  addresses = ["10.0.1.0/24"]
  dhcp {
    enabled = "false"
  }
  autostart = "true"
}

resource "libvirt_domain" "tf_test4" {
  # init
  name = "tf_test"
  metadata = "tf_test"
  vcpu = 2
  memory = 512
  running = false
  autostart = false

  network_interface {
    network_id = "${libvirt_network.public4.id}"
    addresses = ["10.0.1.3"]
    wait_for_lease = true
  }
}

we cannot get the DomainInterface infos ( i tried out with master and this work getting Iface with libvirtapi) and we loop until the 5min go over ( this is ok)

2018-09-20T12:59:42.885+0200 [DEBUG] plugin.terraform-provider-libvirt: 2018/09/20 12:59:42 [DEBUG] qemu-agent is not used
2018-09-20T12:59:42.885+0200 [DEBUG] plugin.terraform-provider-libvirt: 2018/09/20 12:59:42 [DEBUG] getting domain addresses from networks
2018-09-20T12:59:42.886+0200 [DEBUG] plugin.terraform-provider-libvirt: 2018/09/20 12:59:42 [DEBUG] Interfaces: ([]libvirt.DomainInterface) <nil>
2018-09-20T12:59:42.886+0200 [DEBUG] plugin.terraform-provider-libvirt: 2018/09/20 12:59:42 [DEBUG] no interfaces could be obtained with qemu-agent: falling back to the libvirt API
2018-09-20T12:59:42.887+0200 [DEBUG] plugin.terraform-provider-libvirt: 2018/09/20 12:59:42 [DEBUG] Interfaces info obtained with libvirt API:
2018-09-20T12:59:42.887+0200 [DEBUG] plugin.terraform-provider-libvirt: ([]libvirt.DomainInterface) {
2018-09-20T12:59:42.887+0200 [DEBUG] plugin.terraform-provider-libvirt: }
2018-09-20T12:59:42.887+0200 [DEBUG] plugin.terraform-provider-libvirt: 
2018-09-20T12:59:42.887+0200 [DEBUG] plugin.terraform-provider-libvirt: 2018/09/20 12:59:42 [DEBUG] ifaces with addresses: []
2018-09-20T12:59:42.887+0200 [DEBUG] plugin.terraform-provider-libvirt: 2018/09/20 12:59:42 [DEBUG] doesn't have IP address(es) yet...
2018-09-20T12:59:42.887+0200 [DEBUG] plugin.terraform-provider-libvirt: 2018/09/20 12:59:42 [DEBUG] IP address not found for interface  &{XMLName:{Space: Local:} Managed: TrustGuestRXFilters: MAC:0xc420215090 Source:0xc420204540 Boot:<nil> VLan:<nil> VirtualPort:<nil> IP:[] Route:[] Script:<nil> Target:<nil> Guest:<nil> Model:0xc420215000 Driver:<nil> Backend:<nil> FilterRef:<nil> Tune:<nil> Link:<nil> MTU:<nil> Bandwidth:<nil> Coalesce:<nil> ROM:<nil> Alias:<nil> Address:<nil>}  will try in a while
2018-09-20T12:59:42.887+0200 [DEBUG] plugin.terraform-provider-libvirt: 2018/09/20 12:59:42 [TRACE] Waiting 10s before next try

I think i am missing something 🤔 but i cannot figure find out.. any hints welcome 👍

[dmacvicar]

You have DHCP disabled, which means you want to get an address from the DHCP behind the bridge.

• I don't think adding addresses = ["10.0.1.3"] has any effect, as you don't control that DHCP server host table, and you can't choose what address it should give to you.
• wait_for_lease loops over the function that retrieves domain ip addresses. This is done primarily by virDomainInterfaceAddresses using the leases file, which only works if you manage the network inside libvirt and libvirt has access to the leases file.
• This may work if we enable ARP as source, as I suggested in PR Remove deprecated getDomainInterfacesFromNetworks #427
• This may work with the current qemu agent code, if the agent is running in the image (and also if we refactor to use virDomainInterfaceAddresses using AGENT as src)

[MalloZup]

i will return to work on this soon.

@dmacvicar yop this is also what i was thinking, and i think this is true.
I have some concern of some part of the code like the partialNetIface retrieval which is based on a map[mac], this i would like to double check. ( because this was working before at least in the terraform level, if then the assigned addresses = ["10.0.1.3"] was working on libvirt-networking level this i would like to retests.

I will need to test and check some small part of code let see the findings.

Thx 👍 ❤️

[MalloZup]

@dmacvicar feel free to review it. I double checked it more and i think we did it wrong even with the past.

I did a test with master and this branch. ( from the above tf. files)
for example with the master branch a virsh dumpxml domaintest give

  <interface type='network'>
      <mac address='ee:6f:4e:b9:65:47'/>
      <source network='public4'/>
      <model type='virtio'/>
      <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>
    </interface>

and with the new branch gives :

      <interface type='network'>
        <mac address='52:54:00:1b:71:d5'/>
        <source network='public4' bridge='br-public4'/>
        <target dev='vnet0'/>
        <model type='virtio'/>
        <alias name='net0'/>
        <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>
      </interface>

So we have a bridge network tag in the nomac tag is correct. I dunno why in the master we don't have it.

Anyway i think the PR add the correct behaviour, even if the wait_for_lease variable is somehow not easy to understand a first glance ( and also is not 100% right for QEMU-agent..)

So just double check it : tia 🏩

[Bischoff]

@dmacvicar :

I don't think adding addresses = ["10.0.1.3"] has any effect (without DHCP)

It has. It sets the address on the bridge.

The whole point of recent PRs was precisely to decouple these addresses and DHCP.

[MalloZup]

@Bischoff feel free to check this out, i will turn back on this once i got the qemu-agent PR merged ( should be done also soon).

#432 ( i teste dthis in every scenario, and the sumaform issues with consoles was causing my problems actually 😄 )

For this pr i think is the way we are using MAC as temporary hash to storing Ifaces need to reworked more in deep 🍶 (and it might be the time we make the things more easy, a little more ... ) 😄

[MalloZup]

Closing I will reopen properly in future. Parkinh prs aren't fun imho🦄