Populate machine status with libvirt domain details #37

bison · 2018-10-04T21:46:19Z

This populates the machine's status subresource with the libvirt domain's details.

bison · 2018-10-04T21:46:40Z

/hold

Still a WIP. Almost there.

frobware

Overall, looks good to me. Going to try it in the wild next.

frobware · 2018-10-05T14:58:07Z

cloud/libvirt/actuators/machine/actuator.go

+	dom, err := libvirtutils.LookupDomainByUUID(machine, uuid)
+	if err != nil {
+		glog.Errorf("Could not lookup created libvirt machine: %v", err)
+		return fmt.Errorf("error looking up created machine %v", err)


Express which machine failed the lookup; ... %q ...", machine...

frobware · 2018-10-05T14:59:28Z

cloud/libvirt/actuators/machine/actuator.go

+
+	if err := a.updateStatus(machine, dom); err != nil {
+		glog.Errorf("Could not update machine status: %v", err)
+		return fmt.Errorf("error updating machine status: %v", err)


Ditto regarding which machine; essentially avoid:

printf("could not open file")

in favour of:

printf("could not open %q: %v", filename, error)

frobware · 2018-10-05T15:00:41Z

cloud/libvirt/actuators/machine/actuator.go

+	libvirtStatus, err := libvirtutils.ProviderStatus(machine)
+	if err != nil {
+		glog.Errorf("Could not get provider status for update: %v", err)
+		return err


Why do we, in general, log errors, then return an err? Isn't the caller going to log the error, and perhaps with additional context?

Mostly doing this in the "top-level" Update / Create / etc. methods because they are the boundary between the actuator and the upstream controller. I've run into cases where the controller didn't log the returned errors which caused a lot of wasted time debugging.

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

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

+		return nil, fmt.Errorf(LibVirtConIsNil)
+	}
+
+	dom, err := virConn.LookupDomainByUUID(uuid[:])


Having got this far... I'm wondering why we pass a pointer to the uuid? Is it because it's an underlying C/native type?

Nope, but it's represented as an array of 16 bytes, not a slice -- which is why it has to be sliced like this here. It seemed nicer to pass a pointer, especially in the case where the method might return an error and a nil UUID pointer rather than an error and a 16 byte array of zeros.

Ended up dropping the UUID thing altogether and just looking up by name. It's simpler and should be fine.

frobware · 2018-10-05T15:07:28Z

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

+
+	status, ok := obj.(*providerconfigv1.LibvirtMachineProviderStatus)
+	if !ok {
+		return nil, fmt.Errorf("unexpected object: %#v", gvk)


Would expressing %T be more useful here? i.e., what type did we get?

This gvk struct will have the group, version, and kind information. Honestly, this bit is just lifted from the AWS actuator, but it seems like a reasonable thing to log in this situation.

frobware · 2018-10-05T15:08:44Z

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

+		return "Shutoff"
+	}
+
+	return "Unknown"


Make this the default case?

Sure. Done.

bison · 2018-10-06T21:03:01Z

/hold cancel

bison · 2018-10-06T21:03:49Z

I think this is ready.

cloud/libvirt/actuators/machine/actuator.go

ingvagabund · 2018-10-07T18:14:42Z

cloud/libvirt/actuators/machine/actuator.go

+// object.
+func EncodeLibvirtMachineProviderStatus(status *providerconfigv1.LibvirtMachineProviderStatus) (*runtime.RawExtension, error) {
+	// TODO: Modifying this in place feels weird. Should it be copied?
+	status.TypeMeta = metav1.TypeMeta{


You should not modify it. Weird you have to set the APIVersion and the Kind.

This is what the AWS actuator does, but yeah, it felt weird. I don't think it's needed if we just set the metadata upfront when we first create a status, so I've done that instead and removed this bit.

ingvagabund · 2018-10-07T19:42:17Z

cloud/libvirt/actuators/machine/actuator.go

+		return error.WithLog(err, "error getting provider status")
+	}
+
+	uuid, err := uuid.Parse(*libvirtStatus.InstanceID)


This panics when libvirtStatus.InstanceID is nil. In case you call the Update from scratch without populating machine status.

It should be fine to look based on domain name (to be define under cloud/libvirt/actuators/machine/utils/domain.go):

func LookupDomainByName(machine *clusterv1.Machine) (*libvirt.Domain, error) { machineProviderConfig, err := MachineProviderConfigFromClusterAPIMachineSpec(&machine.Spec) client, err := buildClient(machineProviderConfig.URI) if err != nil { return nil, fmt.Errorf("Failed to build libvirt client: %s", err) } virConn := client.libvirt if virConn == nil { return nil, fmt.Errorf(LibVirtConIsNil) } dom, err := virConn.LookupDomainByName(machine.Name) if err != nil { return nil, fmt.Errorf("Failed to lookup domain by name: %v", err) } return dom, nil }

I ditched the whole UUID thing. It didn't make sense to look up the domain via a field stored in the status in order to update the status anyway. The domain is looked up by name now.

ingvagabund · 2018-10-07T21:21:44Z

Just few nits, otherwise lgtm. I was able to apply the patch, build it and run it to collect machine status. Thanks @bison !!!

ingvagabund · 2018-10-07T21:22:36Z

@enxebre let's merge this PR first before applying #38

ingvagabund · 2018-10-07T21:55:26Z

cloud/libvirt/actuators/machine/actuator.go

@@ -62,11 +105,139 @@ func (a *Actuator) Delete(cluster *clusterv1.Cluster, machine *clusterv1.Machine
 // Update updates a machine and is invoked by the Machine Controller
 func (a *Actuator) Update(cluster *clusterv1.Cluster, machine *clusterv1.Machine) error {
 	glog.Infof("Updating machine %v for cluster %v.", machine.Name, cluster.Name)
-	return fmt.Errorf("TODO: Not yet implemented")
+	error := errorWrapper{cluster: cluster, machine: machine}


Maybe errorWr? I got confused by the error.WithLog. The error is allocated for error type. We should not override it unless necessary.

Good point. Fixed.

paulfantom · 2018-10-08T05:48:19Z

Dockerfile

@@ -15,6 +15,8 @@
 # Reproducible builder image
 FROM openshift/origin-release:golang-1.10 as builder

+RUN yum install -y libvirt-devel


Do not add this as libvirt-devel is already in openshift/origin-release:golang-1.10 image

Looks like this was already fixed in master. This bit is gone after a rebase.

enxebre · 2018-10-08T08:29:48Z

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

+
+// UpdateProviderStatus updates the provider status in-place with info
+// from the given libvirt domain.
+func UpdateProviderStatus(status *providerconfigv1.LibvirtMachineProviderStatus, dom *libvirt.Domain) error {


any machine api specific logic does not belong here, we can address it after merging and rebasing #38

frobware

Going to wait until this is re-based.

cloud/libvirt/actuators/machine/actuator.go

frobware · 2018-10-08T14:02:00Z

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

 	if name == "" {
-		return fmt.Errorf("Failed to create domain, name is empty")
+		return nil, fmt.Errorf("Failed to create domain, name is empty")


I don't think this is 'failed to create domain'. Rather, 'name is empty'. :-/

frobware · 2018-10-08T14:03:22Z

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

 	}

 	// Get values from machineProviderConfig
 	if err := domainDefInit(&domainDef, name, *machineProviderConfig); err != nil {
-		return fmt.Errorf("Failed to init domain definition from machineProviderConfig: %v", err)
+		return nil, fmt.Errorf("Failed to init domain definition from machineProviderConfig: %v", err)


We should consistently use %v for error. In the few lines above we use %s.

frobware · 2018-10-08T14:03:55Z

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

 		}
 	} else {
-		return fmt.Errorf("machine does not has a IgnKey value")
+		return nil, fmt.Errorf("machine does not has a IgnKey value")


typo: machine does not _have_ a IgnKey value.

bison · 2018-10-09T20:56:06Z

Rebased and simplified this quite a bit. I think it's probably ready as-is, but a few open questions:

The status field names are copied from the AWS actuator. Should we update them to make more sense here? e.g. DomainUUID instead of InstanceID, or is there a compelling reason to have them match the AWS status?
I haven't included any conditions in the status. Happy to do that now if we think it's important, or can tackle it in a follow up PR.

paulfantom · 2018-10-10T14:03:23Z

/approve
/hold

ingvagabund · 2018-10-10T14:25:18Z

/lgtm
/hold cancel

openshift-ci-robot · 2018-10-10T14:25:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bison, ingvagabund, paulfantom

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~~ [bison,ingvagabund,paulfantom]

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

frobware · 2018-10-10T14:28:32Z

2. I haven't included any conditions in the status. Happy to do that now if we think it's important, or can tackle it in a follow up PR.

I vote we do that in a follow-up PR. Better to keep them small and easily reviewable (and revertable).

openshift-ci-robot requested review from enxebre and trawler October 4, 2018 21:46

openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Oct 4, 2018

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 4, 2018

frobware reviewed Oct 5, 2018

View reviewed changes

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 6, 2018

ingvagabund reviewed Oct 7, 2018

View reviewed changes

paulfantom reviewed Oct 8, 2018

View reviewed changes

enxebre mentioned this pull request Oct 8, 2018

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

Merged

enxebre reviewed Oct 8, 2018

View reviewed changes

frobware reviewed Oct 8, 2018

View reviewed changes

bison added 2 commits October 9, 2018 16:07

Fix final Dockerfile copy

8501ee3

Populate machine status with libvirt domain details

1d6cddb

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 9, 2018

openshift-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 10, 2018

openshift-ci-robot assigned ingvagabund Oct 10, 2018

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Oct 10, 2018

bison mentioned this pull request Oct 10, 2018

Fix leaking libvirt connections #55

Closed

openshift-merge-robot merged commit 8671908 into openshift:master Oct 10, 2018

bison deleted the machine-status branch October 10, 2018 14:44

Populate machine status with libvirt domain details #37

Populate machine status with libvirt domain details #37

Conversation

bison commented Oct 4, 2018

bison commented Oct 4, 2018

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

bison commented Oct 6, 2018

bison commented Oct 6, 2018

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

ingvagabund commented Oct 7, 2018

ingvagabund commented Oct 7, 2018

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

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

bison commented Oct 9, 2018

paulfantom commented Oct 10, 2018

ingvagabund commented Oct 10, 2018

openshift-ci-robot commented Oct 10, 2018

frobware commented Oct 10, 2018 • edited Loading

frobware commented Oct 10, 2018 •

edited

Loading