Add Synchronized condition to VMs

Report synchronization errors directly as a Synchronized condition, set to false. Don't update the failure on every retry. Signed-off-by: Roman Mohr <[email protected]>
cliffy94 · Oct 27, 2017 · 707fa0d · 707fa0d
1 parent 5dfbfb2
commit 707fa0d
Show file tree

Hide file tree

Showing 2 changed files with 70 additions and 38 deletions.
diff --git a/pkg/api/v1/types.go b/pkg/api/v1/types.go
@@ -266,25 +266,26 @@ func (vl *VirtualMachineList) UnmarshalJSON(data []byte) error {
 	return nil
 }
 
-type VMConditionType string
+type VirtualMachinConditionType string
 
 // These are valid conditions of VMs.
 const (
-	// PodCreated means that the VM request was translated into a Pod which can be scheduled and started by
-	// Kubernetes.
-	PodCreated VMConditionType = "PodCreated"
 	// VMReady means the pod is able to service requests and should be added to the
 	// load balancing pools of all matching services.
-	VMReady VMConditionType = "Ready"
+	VirtualMachineReady VirtualMachinConditionType = "Ready"
+
+	// If there happens any error while trying to synchronize the VM with the Domain,
+	// this is reported as false.
+	VirtualMachineSynchronized VirtualMachinConditionType = "Synchronized"
 )
 
 type VMCondition struct {
-	Type               VMConditionType       `json:"type"`
-	Status             k8sv1.ConditionStatus `json:"status"`
-	LastProbeTime      metav1.Time           `json:"lastProbeTime,omitempty"`
-	LastTransitionTime metav1.Time           `json:"lastTransitionTime,omitempty"`
-	Reason             string                `json:"reason,omitempty"`
-	Message            string                `json:"message,omitempty"`
+	Type               VirtualMachinConditionType `json:"type"`
+	Status             k8sv1.ConditionStatus      `json:"status"`
+	LastProbeTime      metav1.Time                `json:"lastProbeTime,omitempty"`
+	LastTransitionTime metav1.Time                `json:"lastTransitionTime,omitempty"`
+	Reason             string                     `json:"reason,omitempty"`
+	Message            string                     `json:"message,omitempty"`
 }
 
 // VMPhase is a label for the condition of a VM at the current time.

diff --git a/pkg/virt-handler/vm.go b/pkg/virt-handler/vm.go
@@ -131,11 +131,6 @@ func (d *VMHandlerDispatch) updateVMStatus(vm *v1.VirtualMachine, isPending bool
 	var err error
 	oldPhase := vm.Status.Phase
 
-	// TODO, update a VM condition, right now, do nothing
-	if syncError != nil {
-		return nil
-	}
-
 	// Don't update the VM if it is already in a final state
 	if vm.IsFinal() {
 		return nil
@@ -146,11 +141,6 @@ func (d *VMHandlerDispatch) updateVMStatus(vm *v1.VirtualMachine, isPending bool
 		return nil
 	}
 
-	// Failed/Succeeded is currently handled somewhere else, do nothing if the spec is not there
-	if newDomainSpec == nil {
-		return nil
-	}
-
 	// TODO update to Failed if VM e.g. crashes
 	// TODO update based on api.Domain succeeded/failed here, instead of the extra controller
 	if isPending {
@@ -164,29 +154,33 @@ func (d *VMHandlerDispatch) updateVMStatus(vm *v1.VirtualMachine, isPending bool
 
 	// Update devices if device status changed
 	// TODO needs caching, better position or init fetch
-	nodeIP, err := d.getVMNodeAddress(vm)
-	if err != nil {
-		return err
-	}
-
-	oldGraphics := vm.Status.Graphics
-	vm.Status.Graphics = []v1.VMGraphics{}
-	for _, src := range newDomainSpec.Devices.Graphics {
-		if (src.Type != "spice" && src.Type != "vnc") || src.Port == -1 {
-			continue
+	deviceChanged := false
+	if newDomainSpec != nil {
+		nodeIP, err := d.getVMNodeAddress(vm)
+		if err != nil {
+			return err
 		}
-		dst := v1.VMGraphics{
-			Type: src.Type,
-			Host: nodeIP,
-			Port: src.Port,
+
+		oldGraphics := vm.Status.Graphics
+		vm.Status.Graphics = []v1.VMGraphics{}
+		for _, src := range newDomainSpec.Devices.Graphics {
+			if (src.Type != "spice" && src.Type != "vnc") || src.Port == -1 {
+				continue
+			}
+			dst := v1.VMGraphics{
+				Type: src.Type,
+				Host: nodeIP,
+				Port: src.Port,
+			}
+			vm.Status.Graphics = append(vm.Status.Graphics, dst)
 		}
-		vm.Status.Graphics = append(vm.Status.Graphics, dst)
+		deviceChanged = reflect.DeepEqual(vm.Status.Graphics, oldGraphics)
 	}
 
 	phaseChanged := oldPhase != vm.Status.Phase
-	deviceChanged := reflect.DeepEqual(vm.Status.Graphics, oldGraphics)
+	errorChanged := d.checkFailure(vm, syncError, "Synchronizing with the Domain failed.")
 
-	if deviceChanged || phaseChanged {
+	if deviceChanged || phaseChanged || errorChanged {
 		_, err = d.clientset.VM(vm.ObjectMeta.Namespace).Update(vm)
 	}
 	return err
@@ -554,3 +548,40 @@ func (d *VMHandlerDispatch) isMigrationDestination(namespace string, vmName stri
 func isWorthSyncing(vm *v1.VirtualMachine) bool {
 	return !vm.IsFinal()
 }
+
+func (d *VMHandlerDispatch) checkFailure(vm *v1.VirtualMachine, syncErr error, reason string) (changed bool) {
+	if syncErr != nil && !d.hasCondition(vm, v1.VirtualMachineSynchronized) {
+		vm.Status.Conditions = append(vm.Status.Conditions, v1.VMCondition{
+			Type:               v1.VirtualMachineSynchronized,
+			Reason:             reason,
+			Message:            syncErr.Error(),
+			LastTransitionTime: metav1.Now(),
+			Status:             k8sv1.ConditionFalse,
+		})
+		return true
+	} else if syncErr == nil && d.hasCondition(vm, v1.VirtualMachineSynchronized) {
+		d.removeCondition(vm, v1.VirtualMachineSynchronized)
+		return true
+	}
+	return false
+}
+
+func (d *VMHandlerDispatch) hasCondition(vm *v1.VirtualMachine, cond v1.VirtualMachinConditionType) bool {
+	for _, c := range vm.Status.Conditions {
+		if c.Type == cond {
+			return true
+		}
+	}
+	return false
+}
+
+func (d *VMHandlerDispatch) removeCondition(vm *v1.VirtualMachine, cond v1.VirtualMachinConditionType) {
+	var conds []v1.VMCondition
+	for _, c := range vm.Status.Conditions {
+		if c.Type == cond {
+			continue
+		}
+		conds = append(conds, c)
+	}
+	vm.Status.Conditions = conds
+}