add actuator binary tests and decouples libvirt utils from machine api #38

enxebre · 2018-10-05T14:17:02Z

Refactor cloud/libvir/actuator/machines/utils and decouples it from the cluster api, so in a follow up we should move utils up to pkg/
Add automation on tests/actuator for running tests for the actuator in isolation (no cluster api)
Increase the DSL based on https://github.com/dmacvicar/terraform-provider-libvirt into the utils library so it can be also used by the tests to generate assumptions via libvirt library, e.g CreateNetwork or support to create ignition volumes
This is to start to give some love to this actuator. This tests can be now run on CI on a packet machine.

enxebre · 2018-10-05T14:17:21Z

root@libvirt-randomid:~/go/src/github.com/openshift/cluster-api-provider-libvirt# ~/go/bin/actuator -m examples/
2018-10-05 14:12:24.178892 I | [INFO] Created libvirt client
2018-10-05 14:12:24.178960 I | [DEBUG] Create a libvirt volume with name baseVolume for pool default from the base volume
2018-10-05 14:12:24.180098 I | Image /root/coreos_production_qemu_image.img image is: 919536640 bytes
2018-10-05 14:12:30.709141 I | 919536640 bytes uploaded
2018-10-05 14:12:38.411041 I | [INFO] Volume ID: /var/lib/libvirt/images/baseVolume
2018-10-05 14:12:38.411102 I | [DEBUG] creating ignition file
2018-10-05 14:12:38.411167 I | [INFO] ignition: {Name:worker.ign PoolName:default Content:{"ignition":{"version":"2.2.0"},"networkd":{},"passwd":{"users":[{"name":"core","passwordHash": "$6$Jez3bVF7jG$ncmvBeJiYbzFKZSQKzTwg9gJ2qoF4N.JYyt8iv4qCThCdJmOtxnZz3l1W3btoh9.bXE8DcdXr6iXuV7ES4kww0"}]},"storage":{},"systemd":{}}}
2018-10-05 14:12:38.412010 I | Creating Ignition temporary file
2018-10-05 14:12:38.415568 I | 228 bytes uploaded
2018-10-05 14:12:39.019100 I | [INFO] Ignition ID: /var/lib/libvirt/images/worker.ign
2018-10-05 14:12:39.019184 I | [INFO] Ignition ID: /var/lib/libvirt/images/worker.ign
2018-10-05 14:12:39.019508 I | [INFO] Creating libvirt network at qemu+tcp://127.0.0.1/system
2018-10-05 14:12:39.019732 I | [DEBUG] Creating libvirt network at qemu+tcp://127.0.0.1/system:   <network>
      <name>actuatorTestNetwork</name>
      <forward mode="nat"></forward>
      <bridge name="tt0" stp="on"></bridge>
      <domain name="tt.test"></domain>
      <ip address="192.168.124.1" family="ipv4" prefix="24">
          <dhcp>
              <range start="192.168.124.2" end="192.168.124.254"></range>
          </dhcp>
      </ip>
  </network>
2018-10-05 14:12:51.793153 I | [INFO] Created network actuatorTestNetwork [65f4c460-ab4d-4eab-8bed-18643e7be7f0]
ERROR: logging before flag.Parse: I1005 14:12:51.794909   18420 actuator.go:47] Creating machine "worker-example" for cluster "tb-asg-35".
2018-10-05 14:12:51.796631 I | [INFO] Created libvirt client
2018-10-05 14:12:51.796669 I | [DEBUG] Create a libvirt volume with name worker-example for pool default from the base volume /var/lib/libvirt/images/baseVolume
2018-10-05 14:12:53.680457 I | [INFO] Volume ID: /var/lib/libvirt/images/worker-example
2018-10-05 14:12:53.680509 I | [DEBUG] Create resource libvirt_domain
2018-10-05 14:12:53.701527 I | [TRACE] Capabilities of host
 {XMLName:{Space: Local:capabilities} Host:{UUID:656450a9-a84d-4147-a84e-e36931dd75bd CPU:0xc4204d0140 PowerManagement:0xc42019ac40 IOMMU:<nil> MigrationFeatures:0xc420780570 NUMA:0xc4204454a8 Cache:<nil> SecModel:[{Name:none DOI:0 Labels:[]} {Name:dac DOI:0 Labels:[{Type:kvm Value:+64055:+115} {Type:qemu Value:+64055:+115}]}]} Guests:[{OSType:hvm Arch:{Name:i686 WordSize:32 Emulator:/usr/bin/qemu-system-i386 Loader: Machines:[{Name:pc-i440fx-xenial MaxCPUs:255 Canonical:} {Name:ubuntu MaxCPUs:255 Canonical:pc-i440fx-xenial} {Name:pc-i440fx-2.4 MaxCPUs:255 Canonical:} {Name:pc-1.3 MaxCPUs:255 Canonical:} {Name:pc-0.12 MaxCPUs:255 Canonical:} {Name:pc-q35-1.6 MaxCPUs:255 Canonical:} {Name:pc-q35-1.5 MaxCPUs:255 Canonical:} {Name:xenpv MaxCPUs:1 Canonical:} {Name:pc-i440fx-1.6 MaxCPUs:255 Canonical:} {Name:pc-q35-2.2 MaxCPUs:255 Canonical:} {Name:pc-i440fx-vivid MaxCPUs:255 Canonical:} {Name:pc-0.11 MaxCPUs:255 Canonical:} {Name:pc-i440fx-2.3 MaxCPUs:255 Canonical:} {Name:pc-0.10 MaxCPUs:255 Canonical:} {Name:pc-1.2 MaxCPUs:255 Canonical:} {Name:pc-i440fx-2.2 MaxCPUs:255 Canonical:} {Name:isapc MaxCPUs:1 Canonical:} {Name:pc-i440fx-1.7 MaxCPUs:255 Canonical:} {Name:pc-q35-xenial MaxCPUs:255 Canonical:} {Name:pc-q35-1.4 MaxCPUs:255 Canonical:} {Name:pc-i440fx-trusty MaxCPUs:255 Canonical:} {Name:pc-i440fx-wily MaxCPUs:255 Canonical:} {Name:xenfv MaxCPUs:128 Canonical:} {Name:pc-q35-2.5 MaxCPUs:255 Canonical:} {Name:q35 MaxCPUs:255 Canonical:pc-q35-2.5} {Name:pc-0.15 MaxCPUs:255 Canonical:} {Name:pc-i440fx-utopic MaxCPUs:255 Canonical:} {Name:pc-i440fx-1.5 MaxCPUs:255 Canonical:} {Name:pc-0.14 MaxCPUs:255 Canonical:} {Name:pc-q35-2.0 MaxCPUs:255 Canonical:} {Name:pc-i440fx-1.4 MaxCPUs:255 Canonical:} {Name:pc-q35-2.1 MaxCPUs:255 Canonical:} {Name:pc-i440fx-2.5 MaxCPUs:255 Canonical:} {Name:pc MaxCPUs:255 Canonical:pc-i440fx-2.5} {Name:pc-1.1 MaxCPUs:255 Canonical:} {Name:pc-q35-1.7 MaxCPUs:255 Canonical:} {Name:pc-i440fx-2.1 MaxCPUs:255 Canonical:} {Name:pc-1.0 MaxCPUs:255 Canonical:} {Name:pc-i440fx-2.0 MaxCPUs:255 Canonical:} {Name:pc-q35-2.4 MaxCPUs:255 Canonical:} {Name:pc-q35-2.3 MaxCPUs:255 Canonical:} {Name:pc-0.13 MaxCPUs:255 Canonical:}] Domains:[{Type:qemu Emulator: Machines:[]} {Type:kvm Emulator:/usr/bin/kvm-spice Machines:[{Name:pc-i440fx-xenial MaxCPUs:255 Canonical:} {Name:ubuntu MaxCPUs:255 Canonical:pc-i440fx-xenial} {Name:pc-i440fx-2.4 MaxCPUs:255 Canonical:} {Name:pc-1.3 MaxCPUs:255 Canonical:} {Name:pc-0.12 MaxCPUs:255 Canonical:} {Name:pc-q35-1.6 MaxCPUs:255 Canonical:} {Name:pc-q35-1.5 MaxCPUs:255 Canonical:} {Name:pc-i440fx-1.6 MaxCPUs:255 Canonical:} {Name:pc-q35-2.2 MaxCPUs:255 Canonical:} {Name:pc-i440fx-1.7 MaxCPUs:255 Canonical:} {Name:pc-i440fx-vivid MaxCPUs:255 Canonical:} {Name:pc-0.11 MaxCPUs:255 Canonical:} {Name:xenpv MaxCPUs:1 Canonical:} {Name:pc-q35-2.1 MaxCPUs:255 Canonical:} {Name:pc-q35-xenial MaxCPUs:255 Canonical:} {Name:pc-0.10 MaxCPUs:255 Canonical:} {Name:pc-1.2 MaxCPUs:255 Canonical:} {Name:pc-i440fx-2.2 MaxCPUs:255 Canonical:} {Name:isapc MaxCPUs:1 Canonical:} {Name:pc-i440fx-2.3 MaxCPUs:255 Canonical:} {Name:pc-i440fx-trusty MaxCPUs:255 Canonical:} {Name:pc-q35-1.4 MaxCPUs:255 Canonical:} {Name:pc-i440fx-wily MaxCPUs:255 Canonical:} {Name:xenfv MaxCPUs:128 Canonical:} {Name:pc-q35-2.5 MaxCPUs:255 Canonical:} {Name:q35 MaxCPUs:255 Canonical:pc-q35-2.5} {Name:pc-0.15 MaxCPUs:255 Canonical:} {Name:pc-i440fx-utopic MaxCPUs:255 Canonical:} {Name:pc-i440fx-1.5 MaxCPUs:255 Canonical:} {Name:pc-q35-2.0 MaxCPUs:255 Canonical:} {Name:pc-i440fx-1.4 MaxCPUs:255 Canonical:} {Name:pc-i440fx-2.5 MaxCPUs:255 Canonical:} {Name:pc MaxCPUs:255 Canonical:pc-i440fx-2.5} {Name:pc-0.14 MaxCPUs:255 Canonical:} {Name:pc-1.1 MaxCPUs:255 Canonical:} {Name:pc-i440fx-2.1 MaxCPUs:255 Canonical:} {Name:pc-q35-1.7 MaxCPUs:255 Canonical:} {Name:pc-1.0 MaxCPUs:255 Canonical:} {Name:pc-i440fx-2.0 MaxCPUs:255 Canonical:} {Name:pc-q35-2.4 MaxCPUs:255 Canonical:} {Name:pc-q35-2.3 MaxCPUs:255 Canonical:} {Name:pc-0.13 MaxCPUs:255 Canonical:}]}]} Features:0xc42015ad40} {OSType:hvm Arch:{Name:x86_64 WordSize:64 Emulator:/usr/bin/qemu-system-x86_64 Loader: Machines:[{Name:pc-i440fx-xenial MaxCPUs:255 Canonical:} {Name:ubuntu MaxCPUs:255 Canonical:pc-i440fx-xenial} {Name:pc-i440fx-2.4 MaxCPUs:255 Canonical:} {Name:pc-1.3 MaxCPUs:255 Canonical:} {Name:pc-0.12 MaxCPUs:255 Canonical:} {Name:pc-q35-1.6 MaxCPUs:255 Canonical:} {Name:pc-q35-1.5 MaxCPUs:255 Canonical:} {Name:pc-i440fx-1.6 MaxCPUs:255 Canonical:} {Name:pc-q35-2.2 MaxCPUs:255 Canonical:} {Name:pc-i440fx-1.7 MaxCPUs:255 Canonical:} {Name:pc-i440fx-vivid MaxCPUs:255 Canonical:} {Name:pc-0.11 MaxCPUs:255 Canonical:} {Name:xenpv MaxCPUs:1 Canonical:} {Name:pc-q35-2.1 MaxCPUs:255 Canonical:} {Name:pc-q35-xenial MaxCPUs:255 Canonical:} {Name:pc-0.10 MaxCPUs:255 Canonical:} {Name:pc-1.2 MaxCPUs:255 Canonical:} {Name:pc-i440fx-2.2 MaxCPUs:255 Canonical:} {Name:isapc MaxCPUs:1 Canonical:} {Name:pc-i440fx-2.3 MaxCPUs:255 Canonical:} {Name:pc-i440fx-trusty MaxCPUs:255 Canonical:} {Name:pc-q35-1.4 MaxCPUs:255 Canonical:} {Name:pc-i440fx-wily MaxCPUs:255 Canonical:} {Name:xenfv MaxCPUs:128 Canonical:} {Name:pc-q35-2.5 MaxCPUs:255 Canonical:} {Name:q35 MaxCPUs:255 Canonical:pc-q35-2.5} {Name:pc-0.15 MaxCPUs:255 Canonical:} {Name:pc-i440fx-utopic MaxCPUs:255 Canonical:} {Name:pc-i440fx-1.5 MaxCPUs:255 Canonical:} {Name:pc-q35-2.0 MaxCPUs:255 Canonical:} {Name:pc-i440fx-1.4 MaxCPUs:255 Canonical:} {Name:pc-i440fx-2.5 MaxCPUs:255 Canonical:} {Name:pc MaxCPUs:255 Canonical:pc-i440fx-2.5} {Name:pc-0.14 MaxCPUs:255 Canonical:} {Name:pc-1.1 MaxCPUs:255 Canonical:} {Name:pc-i440fx-2.1 MaxCPUs:255 Canonical:} {Name:pc-q35-1.7 MaxCPUs:255 Canonical:} {Name:pc-1.0 MaxCPUs:255 Canonical:} {Name:pc-i440fx-2.0 MaxCPUs:255 Canonical:} {Name:pc-q35-2.4 MaxCPUs:255 Canonical:} {Name:pc-q35-2.3 MaxCPUs:255 Canonical:} {Name:pc-0.13 MaxCPUs:255 Canonical:}] Domains:[{Type:qemu Emulator: Machines:[]} {Type:kvm Emulator:/usr/bin/kvm-spice Machines:[{Name:pc-i440fx-xenial MaxCPUs:255 Canonical:} {Name:ubuntu MaxCPUs:255 Canonical:pc-i440fx-xenial} {Name:pc-i440fx-2.4 MaxCPUs:255 Canonical:} {Name:pc-1.3 MaxCPUs:255 Canonical:} {Name:pc-0.12 MaxCPUs:255 Canonical:} {Name:pc-q35-1.6 MaxCPUs:255 Canonical:} {Name:pc-q35-1.5 MaxCPUs:255 Canonical:} {Name:pc-i440fx-1.6 MaxCPUs:255 Canonical:} {Name:pc-q35-2.2 MaxCPUs:255 Canonical:} {Name:pc-i440fx-1.7 MaxCPUs:255 Canonical:} {Name:pc-i440fx-vivid MaxCPUs:255 Canonical:} {Name:pc-0.11 MaxCPUs:255 Canonical:} {Name:xenpv MaxCPUs:1 Canonical:} {Name:pc-q35-2.1 MaxCPUs:255 Canonical:} {Name:pc-q35-xenial MaxCPUs:255 Canonical:} {Name:pc-0.10 MaxCPUs:255 Canonical:} {Name:pc-1.2 MaxCPUs:255 Canonical:} {Name:pc-i440fx-2.2 MaxCPUs:255 Canonical:} {Name:isapc MaxCPUs:1 Canonical:} {Name:pc-i440fx-2.3 MaxCPUs:255 Canonical:} {Name:pc-i440fx-trusty MaxCPUs:255 Canonical:} {Name:pc-q35-1.4 MaxCPUs:255 Canonical:} {Name:pc-i440fx-wily MaxCPUs:255 Canonical:} {Name:xenfv MaxCPUs:128 Canonical:} {Name:pc-q35-2.5 MaxCPUs:255 Canonical:} {Name:q35 MaxCPUs:255 Canonical:pc-q35-2.5} {Name:pc-0.15 MaxCPUs:255 Canonical:} {Name:pc-i440fx-utopic MaxCPUs:255 Canonical:} {Name:pc-i440fx-1.5 MaxCPUs:255 Canonical:} {Name:pc-q35-2.0 MaxCPUs:255 Canonical:} {Name:pc-i440fx-1.4 MaxCPUs:255 Canonical:} {Name:pc-i440fx-2.5 MaxCPUs:255 Canonical:} {Name:pc MaxCPUs:255 Canonical:pc-i440fx-2.5} {Name:pc-0.14 MaxCPUs:255 Canonical:} {Name:pc-1.1 MaxCPUs:255 Canonical:} {Name:pc-i440fx-2.1 MaxCPUs:255 Canonical:} {Name:pc-q35-1.7 MaxCPUs:255 Canonical:} {Name:pc-1.0 MaxCPUs:255 Canonical:} {Name:pc-i440fx-2.0 MaxCPUs:255 Canonical:} {Name:pc-q35-2.4 MaxCPUs:255 Canonical:} {Name:pc-q35-2.3 MaxCPUs:255 Canonical:} {Name:pc-0.13 MaxCPUs:255 Canonical:}]}]} Features:0xc420381940}]}
2018-10-05 14:12:53.701661 I | [TRACE] Checking for x86_64/hvm against i686/hvm
2018-10-05 14:12:53.701683 I | [TRACE] Checking for x86_64/hvm against x86_64/hvm
2018-10-05 14:12:53.701704 I | [DEBUG] Found 42 machines in guest for x86_64/hvm
2018-10-05 14:12:53.701723 I | [INFO] getCanonicalMachineName
2018-10-05 14:12:53.701744 I | [TRACE] Checking for x86_64/hvm against i686/hvm
2018-10-05 14:12:53.701771 I | [TRACE] Checking for x86_64/hvm against x86_64/hvm
2018-10-05 14:12:53.701792 I | [DEBUG] Found 42 machines in guest for x86_64/hvm
2018-10-05 14:12:53.701814 I | [INFO] setCoreOSIgnition
2018-10-05 14:12:53.701837 I | [INFO] setDisks
2018-10-05 14:12:53.701867 I | [INFO] LookupStorageVolByKey
2018-10-05 14:12:53.702317 I | [INFO] diskVolume
2018-10-05 14:12:53.702687 I | [INFO] DomainDiskSource
2018-10-05 14:12:53.702718 I | [INFO] setNetworkInterfaces
2018-10-05 14:12:53.703770 I | Networkaddress 192.168.124.0/24
2018-10-05 14:12:53.703825 I | [INFO] Adding IP/MAC/host=192.168.124.51/52:fd:fc:07:21:82/worker-example to actuatorTestNetwork
2018-10-05 14:12:53.703915 I | Updating host with XML:
  <host mac="52:fd:fc:07:21:82" name="worker-example" ip="192.168.124.51"></host>
2018-10-05 14:13:12.852384 I | Adding host with XML:
  <host mac="52:fd:fc:07:21:82" name="worker-example" ip="192.168.124.51"></host>
2018-10-05 14:13:31.993780 I | [INFO] Creating libvirt domain at qemu+tcp://127.0.0.1/system
2018-10-05 14:13:31.995232 I | [DEBUG] Creating libvirt domain with XML:
  <domain type="kvm">
      <name>worker-example</name>
      <memory unit="MiB">4086</memory>
      <vcpu>2</vcpu>
      <os>
          <type arch="x86_64" machine="pc-i440fx-xenial">hvm</type>
      </os>
      <features>
          <pae></pae>
          <acpi></acpi>
          <apic></apic>
      </features>
      <cpu></cpu>
      <devices>
          <emulator>/usr/bin/qemu-system-x86_64</emulator>
          <disk type="file" device="disk">
              <driver name="qemu" type="qcow2"></driver>
              <source file="/var/lib/libvirt/images/worker-example"></source>
              <target dev="vda" bus="virtio"></target>
          </disk>
          <interface type="network">
              <mac address="52:fd:fc:07:21:82"></mac>
              <source network="actuatorTestNetwork"></source>
              <model type="virtio"></model>
          </interface>
          <console type="pty">
              <target type="virtio" port="0"></target>
          </console>
          <channel>
              <target type="virtio" name="org.qemu.guest_agent.0"></target>
          </channel>
          <graphics type="spice" autoport="yes"></graphics>
          <rng model="virtio">
              <backend model="random"></backend>
          </rng>
      </devices>
      <commandline xmlns="http://libvirt.org/schemas/domain/qemu/1.0">
          <arg value="-fw_cfg"></arg>
          <arg value="name=opt/com.coreos/config,file=/var/lib/libvirt/images/worker.ign"></arg>
      </commandline>
  </domain>
2018-10-05 14:13:32.951414 I | [INFO] Domain ID: 21defc7a-eb17-485f-be20-3ab868e40691
INFO[0068] machine creation was successful!
ERROR: logging before flag.Parse: I1005 14:13:32.951546   18420 actuator.go:71] Checking if machine worker-example for cluster tb-asg-35 exists.
2018-10-05 14:13:32.953014 I | [INFO] Created libvirt client
2018-10-05 14:13:32.953056 I | [DEBUG] Check if a domain exists
INFO[0068] verified the actuator reporrs the machine worker-example exists
2018-10-05 14:13:32.953614 I | [DEBUG] Check if a domain exists
INFO[0068] verified that domain worker-example exists
INFO[0093] verified domain worker-example is reachable for ip 192.168.124.51
ERROR: logging before flag.Parse: I1005 14:13:57.967301   18420 actuator.go:59] Deleting machine "worker-example" for cluster "tb-asg-35".
2018-10-05 14:13:57.969728 I | [INFO] Created libvirt client
2018-10-05 14:13:57.969787 I | [DEBUG] Delete a domain
2018-10-05 14:13:57.969826 I | [DEBUG] Deleting domain worker-example
INFO[0114] machine deletion was successful!
2018-10-05 14:14:28.322059 I | deleted network actuatorTestNetwork

enxebre · 2018-10-05T14:21:15Z

cmd/libvirt-actuator/utils/utils.go

@@ -0,0 +1,48 @@
+package utils


this was just moved into a package so it can be consumed by the tests as well

enxebre · 2018-10-05T14:22:21Z

ping @vikaschoudhary16 @bison @ingvagabund

frobware

Looks generally OK but given the size somebody else should also take a look.

frobware · 2018-10-05T15:13:31Z

cloud/libvirt/actuators/machine/actuator.go

+}
+
+// CreateVolumeAndMachine creates a volume and domain which consumes the former one
+func createVolumeAndDomain(machine *clusterv1.Machine, offset int) error {


nit: this function does too much. Why not two separate functions that can be combined?

frobware · 2018-10-05T15:15:12Z

cloud/libvirt/actuators/machine/actuator.go

+	// build libvirtd client
+	client, err := libvirtutils.BuildClient(machineProviderConfig.URI)
+	if err != nil {
+		return fmt.Errorf("Failed to build libvirt client: %s", err)


nit: consistently use %v for error. The previous one was %v, this is %s and the next is %v.

frobware · 2018-10-05T15:15:58Z

cloud/libvirt/actuators/machine/actuator.go

+		return fmt.Errorf("Failed to build libvirt client: %s", err)
+	}
+
+	// TODO(alberto) create struct and function for converting machineconfig->libvirtConfig


Yeah; that would be way better. +1

frobware · 2018-10-05T15:16:50Z

cloud/libvirt/actuators/machine/actuator.go

+}
+
+// deleteVolumeAndDomain deletes a domain and its referenced volume
+func deleteVolumeAndDomain(machine *clusterv1.Machine) error {


nit: split the functionality into discrete steps.

it's already

// delete domain if err := libvirtutils.DeleteDomain(name, client); err != nil { return fmt.Errorf("error deleting domain: %v", err) } // delete volume if err := libvirtutils.DeleteVolume(name, client); err != nil { return fmt.Errorf("error deleting volume: %v", err) }

I was thinking of it (the func) doing one thing only, and one thing really well. This does two things. If the volume failed to delete I could subsequently call deleteDomain - here it tries to do too much and as a caller I can't distinguish between which failed.

frobware · 2018-10-05T15:17:49Z

cloud/libvirt/actuators/machine/actuator.go

+func deleteVolumeAndDomain(machine *clusterv1.Machine) error {
+	// decode config
+	machineProviderConfig, err := machineProviderConfigFromClusterAPIMachineSpec(&machine.Spec)
+	name := machine.Name


Can we not just use machine.Name where necessary?

frobware · 2018-10-05T15:46:06Z

cloud/libvirt/actuators/machine/utils/network.go

+	}
+
+	// use a bridge provided by the user, or create one otherwise (libvirt will assign on automatically when empty)
+	b := ""


This check seems redundant; if you pass "" it looks like libvirt will create one for you (based on the comment a few lines below), if you don't you'll use the name passed in, so either way we can just use the parameter. no?

frobware · 2018-10-05T15:47:04Z

cloud/libvirt/actuators/machine/utils/network.go

+			for _, address := range addresses {
+				_, ipNet, err := net.ParseCIDR(address)
+				if err != nil {
+					return fmt.Errorf("Error parsing addresses definition '%s': %s", address, err)


Use %q for quoting strings.

frobware · 2018-10-05T15:48:20Z

cloud/libvirt/actuators/machine/utils/network.go

+	}
+	log.Printf("[INFO] Created network %s [%s]", networkDef.Name, id)
+
+	if autostart == true {


Use if autostart - it is already a boolean value.

frobware · 2018-10-05T15:49:01Z

cloud/libvirt/actuators/machine/utils/network.go

+	networkDef.Forward = &libvirtxml.NetworkForward{
+		Mode: mode,
+	}
+	if networkDef.Forward.Mode == netModeIsolated || networkDef.Forward.Mode == netModeNat || networkDef.Forward.Mode == netModeRoute {


All of this is difficult to read; could it be split up into some helper functions?

this is mainly decoupling utils so we have a real DSL for libvirt and we can move it to pkg, so we can raise the bar and we can have subsequent PRs clean and tested, let's do farther logic improvements in follow ups

frobware · 2018-10-05T15:55:56Z

tests/actuator/test.go

+// Test cases
+
+const defaultLogLevel = "debug"
+const defaultlibvirtdURI = "qemu+tcp://127.0.0.1/system"


Is there any reason to not use the default of qemu:///system?

enxebre · 2018-10-08T08:00:37Z

let's get #37 first in

frobware · 2018-10-08T11:45:45Z

cloud/libvirt/actuators/machine/utils/network.go

@@ -129,29 +135,27 @@ func newNetworkDef() libvirtxml.Network {
 		  </forward>
 		</network>`
 	if d, err := newDefNetworkFromXML(defNetworkXML); err != nil {
-		panic(fmt.Sprintf("Unexpected error while parsing default network definition: %s", err))
+		return libvirtxml.Network{}, fmt.Errorf("unexpected error while parsing default network definition %v", err)


Missing : after "definition". So, fmt.Errof(.... definition: %v", err)

ingvagabund · 2018-10-08T12:27:18Z

/lgtm

openshift-ci-robot · 2018-10-08T12:27:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enxebre, ingvagabund

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:

~~OWNERS~~ [enxebre,ingvagabund]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 5, 2018

openshift-ci-robot requested review from ingvagabund and vikaschoudhary16 October 5, 2018 14:17

enxebre commented Oct 5, 2018

View reviewed changes

frobware reviewed Oct 5, 2018

View reviewed changes

ingvagabund mentioned this pull request Oct 7, 2018

Populate machine status with libvirt domain details #37

Merged

enxebre added the do-not-merge/blocked-paths Indicates that a PR should not merge because it touches files in blocked paths. label Oct 8, 2018

enxebre mentioned this pull request Oct 8, 2018

Describe how to run the libvirt actuator with user provided data #39

Merged

openshift-ci-robot removed the do-not-merge/blocked-paths Indicates that a PR should not merge because it touches files in blocked paths. label Oct 8, 2018

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 8, 2018

frobware reviewed Oct 8, 2018

View reviewed changes

enxebre mentioned this pull request Oct 8, 2018

machines/libvirt/worker.machineset.yaml: Drop /var/lib/libvirt/images openshift/machine-api-operator#70

Closed

enxebre force-pushed the test-refact branch from 11ff389 to cc92720 Compare October 8, 2018 11:54

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 8, 2018

enxebre force-pushed the test-refact branch 2 times, most recently from 950a503 to 0dd8222 Compare October 8, 2018 12:09

add actuator binary tests and decouples libvirt utils from machine api

dc55e66

enxebre force-pushed the test-refact branch from 0dd8222 to dc55e66 Compare October 8, 2018 12:14

openshift-ci-robot assigned ingvagabund Oct 8, 2018

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 8, 2018

openshift-merge-robot merged commit 964418e into openshift:master Oct 8, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add actuator binary tests and decouples libvirt utils from machine api #38

add actuator binary tests and decouples libvirt utils from machine api #38

enxebre commented Oct 5, 2018 •

edited

Loading

enxebre commented Oct 5, 2018

enxebre Oct 5, 2018

enxebre commented Oct 5, 2018 •

edited

Loading

frobware left a comment

frobware Oct 5, 2018

frobware Oct 5, 2018

frobware Oct 5, 2018

frobware Oct 5, 2018

enxebre Oct 8, 2018

frobware Oct 8, 2018

frobware Oct 5, 2018

frobware Oct 5, 2018

frobware Oct 5, 2018

frobware Oct 5, 2018

frobware Oct 5, 2018

enxebre Oct 8, 2018

frobware Oct 5, 2018

enxebre commented Oct 8, 2018

frobware Oct 8, 2018

ingvagabund commented Oct 8, 2018

openshift-ci-robot commented Oct 8, 2018

add actuator binary tests and decouples libvirt utils from machine api #38

add actuator binary tests and decouples libvirt utils from machine api #38

Conversation

enxebre commented Oct 5, 2018 • edited Loading

enxebre commented Oct 5, 2018

Choose a reason for hiding this comment

enxebre commented Oct 5, 2018 • edited Loading

frobware left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enxebre commented Oct 8, 2018

Choose a reason for hiding this comment

ingvagabund commented Oct 8, 2018

openshift-ci-robot commented Oct 8, 2018

enxebre commented Oct 5, 2018 •

edited

Loading

enxebre commented Oct 5, 2018 •

edited

Loading