Merge pull request #408 from ydayagi/vmimport-metric-cancelled-imports

New metric counter for cancelled imports

docs/metrics.md

@@ -2,6 +2,6 @@
 
 Virtual Machine Import uses Prometheus for metrics reporting. The metrics can be used for real-time monitoring. Virtual Machine Import does not persist its metrics, if a member restarts, the metrics will be reset.
 
-| Name               | Description                                                     | Type    | Labels                       |
-|--------------------|-----------------------------------------------------------------|---------|------------------------------|
-| kubevirt_vmimport  | The total number of successfull/failed Virtual Machine imports. | Counter | result=<successful>/<failed> |
+| Name               | Description                                                               | Type    | Labels                                   |
+|--------------------|---------------------------------------------------------------------------|---------|------------------------------------------|
+| kubevirt_vmimport  | The total number of successfull/failed/cancelled Virtual Machine imports. | Counter | result=successful\|failed\|cancelled     |

go.mod

@@ -24,6 +24,7 @@ require (
 	github.com/ovirt/go-ovirt v4.3.4+incompatible
 	github.com/pkg/errors v0.9.1
 	github.com/prometheus/client_golang v1.2.1
+	github.com/prometheus/client_model v0.2.0
 	github.com/spf13/pflag v1.0.5
 	github.com/tomnomnom/linkheader v0.0.0-20180905144013-02ca5825eb80 // indirect
 	github.com/vmware/govmomi v0.23.1

pkg/controller/virtualmachineimport/virtualmachineimport_controller.go

@@ -232,6 +232,37 @@ func (r *ReconcileVirtualMachineImport) Reconcile(request reconcile.Request) (re
 		return reconcile.Result{}, err
 	}
 
+	// Add finalizer to handle cancelled import
+	if !r.vmImportInProgress(instance) {
+		err := utils.AddFinalizer(instance, utils.CancelledImportFinalizer, r.client)
+		if err != nil {
+			return reconcile.Result{}, err
+		}
+	}
+
+	// Handle deleted import
+	if instance.DeletionTimestamp != nil {
+
+		// We know that additional finalizers after this point are created when VM import is in progress
+		// Therefore, if one of them fails and we return to Reconcile again ==> the code above will not add cancel finalizer again
+		// Finalizers that do not rely on import state should be handled here
+
+		// Cancelled import finalizer
+		if utils.HasFinalizer(instance, utils.CancelledImportFinalizer) {
+			err := utils.RemoveFinalizer(instance, utils.CancelledImportFinalizer, r.client)
+			if err != nil {
+				return reconcile.Result{}, err
+			}
+
+			metrics.ImportCounter.IncCancelled()
+		}
+
+		// If no more finalizers then return so resource can be deleted
+		if len(instance.GetFinalizers()) == 0 {
+			return reconcile.Result{}, nil
+		}
+	}
+
 	// Init provider:
 	provider, err := r.createProvider(instance)
 	if err != nil {
@@ -1096,6 +1127,9 @@ func (r *ReconcileVirtualMachineImport) updateProgress(instance *v2vv1.VirtualMa
 }
 
 func (r *ReconcileVirtualMachineImport) afterSuccess(vmName types.NamespacedName, p provider.Provider, instance *v2vv1.VirtualMachineImport) error {
+
+	r.removeFinalizer(utils.CancelledImportFinalizer, instance)
+
 	metrics.ImportCounter.IncSuccessful()
 	vmiName := types.NamespacedName{Name: instance.Name, Namespace: instance.Namespace}
 	var errs []error
@@ -1117,6 +1151,9 @@ func (r *ReconcileVirtualMachineImport) afterSuccess(vmName types.NamespacedName
 
 //TODO: use in proper places
 func (r *ReconcileVirtualMachineImport) afterFailure(p provider.Provider, instance *v2vv1.VirtualMachineImport) error {
+
+	r.removeFinalizer(utils.CancelledImportFinalizer, instance)
+
 	metrics.ImportCounter.IncFailed()
 	vmiName := types.NamespacedName{Name: instance.Name, Namespace: instance.Namespace}
 	var errs []error
@@ -1359,3 +1396,12 @@ func disksImportProgress(dvsImportProgress map[string]float64, dvCount float64)
 	disksAverageProgress := sumProgress / dvCount
 	return fmt.Sprintf("%v", startProgress+int(disksAverageProgress*progressCopyDiskRange))
 }
+
+func (r *ReconcileVirtualMachineImport) removeFinalizer(finalizer string, instance *v2vv1.VirtualMachineImport) {
+	err := utils.RemoveFinalizer(instance, finalizer, r.client)
+	if err != nil {
+		reqLogger := log.WithValues("Request.Namespace", instance.Namespace, "Request.Name", instance.Name)
+		reqLogger.Error(err, "Failed to remove finalizer "+finalizer)
+
+	}
+}

pkg/controller/virtualmachineimport/virtualmachineimport_controller_test.go

@@ -7,6 +7,7 @@ import (
 	batchv1 "k8s.io/api/batch/v1"
 
 	ctrlConfig "github.com/kubevirt/vm-import-operator/pkg/config/controller"
+	"github.com/kubevirt/vm-import-operator/pkg/metrics"
 
 	kvConfig "github.com/kubevirt/vm-import-operator/pkg/config/kubevirt"
 
@@ -1149,6 +1150,92 @@ var _ = Describe("Reconcile steps", func() {
 		})
 	})
 
+	Describe("afterSuccess and afterFailure steps", func() {
+		var (
+			config *v2vv1.VirtualMachineImport
+		)
+		BeforeEach(func() {
+			config = &v2vv1.VirtualMachineImport{}
+			config.Spec = v2vv1.VirtualMachineImportSpec{
+				Source: v2vv1.VirtualMachineImportSourceSpec{
+					Ovirt: &v2vv1.VirtualMachineImportOvirtSourceSpec{},
+				},
+			}
+			conditions := []v2vv1.VirtualMachineImportCondition{}
+			conditions = append(conditions, v2vv1.VirtualMachineImportCondition{
+				Status: corev1.ConditionTrue,
+				Type:   v2vv1.Valid,
+			})
+			conditions = append(conditions, v2vv1.VirtualMachineImportCondition{
+				Status: corev1.ConditionTrue,
+				Type:   v2vv1.MappingRulesVerified,
+			})
+			config.Status.Conditions = conditions
+			name := "test"
+			config.Spec.TargetVMName = &name
+			vmName = types.NamespacedName{Name: "test", Namespace: "default"}
+			get = func(ctx context.Context, key client.ObjectKey, obj runtime.Object) error {
+				switch obj.(type) {
+				case *kubevirtv1.VirtualMachine:
+					volumes := []kubevirtv1.Volume{}
+					volumes = append(volumes, kubevirtv1.Volume{
+						Name: "test",
+						VolumeSource: kubevirtv1.VolumeSource{
+							DataVolume: &kubevirtv1.DataVolumeSource{
+								Name: "test",
+							},
+						},
+					})
+					obj.(*kubevirtv1.VirtualMachine).Spec.Template = &kubevirtv1.VirtualMachineInstanceTemplateSpec{
+						Spec: kubevirtv1.VirtualMachineInstanceSpec{
+							Volumes: volumes,
+						},
+					}
+					refs := []v1.OwnerReference{}
+					isController := true
+					refs = append(refs, v1.OwnerReference{
+						Controller: &isController,
+					})
+					obj.(*kubevirtv1.VirtualMachine).ObjectMeta.OwnerReferences = refs
+				case *v2vv1.VirtualMachineImport:
+					obj.(*v2vv1.VirtualMachineImport).Spec = v2vv1.VirtualMachineImportSpec{}
+					obj.(*v2vv1.VirtualMachineImport).Annotations = map[string]string{sourceVMInitialState: string(provider.VMStatusUp)}
+				}
+				return nil
+			}
+			statusPatch = func(ctx context.Context, obj runtime.Object, patch client.Patch) error {
+				switch obj.(type) {
+				case *v2vv1.VirtualMachineImport:
+					obj.(*v2vv1.VirtualMachineImport).DeepCopyInto(config)
+				default:
+					return nil
+				}
+
+				return nil
+			}
+		})
+		It("should increment success counter", func() {
+			counterValueBefore := getCounterSuccessful()
+
+			err := reconciler.afterSuccess(vmName, &mockProvider{}, config)
+
+			Expect(err).To(BeNil())
+			counterValueAfter := getCounterSuccessful()
+			Expect(counterValueAfter).To(Equal(counterValueBefore + 1))
+			Expect(config.Finalizers).To(BeNil())
+		})
+		It("should increment failed counter", func() {
+			counterValueBefore := getCounterFailed()
+
+			err := reconciler.afterFailure(&mockProvider{}, config)
+
+			Expect(err).To(BeNil())
+			counterValueAfter := getCounterFailed()
+			Expect(counterValueAfter).To(Equal(counterValueBefore + 1))
+			Expect(config.Finalizers).To(BeNil())
+		})
+	})
+
 	Describe("Reconcile step", func() {
 
 		var (
@@ -1546,6 +1633,106 @@ var _ = Describe("Reconcile steps", func() {
 			Expect(err).To(BeNil())
 			Expect(result).To(Equal(reconcile.Result{}))
 		})
+
+		Describe("Cancelled import", func() {
+			var (
+				config *v2vv1.VirtualMachineImport
+			)
+			BeforeEach(func() {
+				config = &v2vv1.VirtualMachineImport{}
+				config.Spec = v2vv1.VirtualMachineImportSpec{
+					Source: v2vv1.VirtualMachineImportSourceSpec{
+						Ovirt: &v2vv1.VirtualMachineImportOvirtSourceSpec{},
+					},
+				}
+				conditions := []v2vv1.VirtualMachineImportCondition{}
+				conditions = append(conditions, v2vv1.VirtualMachineImportCondition{
+					Status: corev1.ConditionTrue,
+					Type:   v2vv1.Valid,
+				})
+				conditions = append(conditions, v2vv1.VirtualMachineImportCondition{
+					Status: corev1.ConditionTrue,
+					Type:   v2vv1.MappingRulesVerified,
+				})
+				config.Status.Conditions = conditions
+				name := "test"
+				config.Spec.TargetVMName = &name
+				get = func(ctx context.Context, key client.ObjectKey, obj runtime.Object) error {
+					switch obj.(type) {
+					case *v2vv1.VirtualMachineImport:
+						config.DeepCopyInto(obj.(*v2vv1.VirtualMachineImport))
+					case *corev1.Secret:
+						obj.(*corev1.Secret).Data = map[string][]byte{"ovirt": getSecret()}
+					}
+					return nil
+				}
+				statusPatch = func(ctx context.Context, obj runtime.Object, patch client.Patch) error {
+					switch obj.(type) {
+					case *v2vv1.VirtualMachineImport:
+						obj.(*v2vv1.VirtualMachineImport).DeepCopyInto(config)
+					default:
+						return nil
+					}
+
+					return nil
+				}
+			})
+
+			It("should increment counter for not in progress import: ", func() {
+				config.SetDeletionTimestamp(&v1.Time{})
+
+				counterValueBefore := getCounterCancelled()
+
+				result, err := reconciler.Reconcile(request)
+
+				Expect(err).To(BeNil())
+				Expect(result).To(Equal(reconcile.Result{}))
+
+				counterValueAfter := getCounterCancelled()
+				Expect(counterValueAfter).To(Equal(counterValueBefore + 1))
+				Expect(config.Finalizers).To(BeNil())
+			})
+
+			It("should not increment counter for done import: ", func() {
+				config.SetDeletionTimestamp(&v1.Time{})
+				config.Annotations = make(map[string]string)
+				config.Annotations[AnnCurrentProgress] = progressDone
+
+				counterValueBefore := getCounterCancelled()
+
+				result, err := reconciler.Reconcile(request)
+
+				Expect(err).To(BeNil())
+				Expect(result).To(Equal(reconcile.Result{}))
+
+				counterValueAfter := getCounterCancelled()
+				Expect(counterValueAfter).To(Equal(counterValueBefore))
+				Expect(config.Annotations[AnnCurrentProgress]).To(Equal(progressDone))
+				Expect(config.Finalizers).To(BeNil())
+			})
+
+			It("should increment counter for in progress import: ", func() {
+				counterValueBefore := getCounterCancelled()
+				// First call Reconcile to make it in progress
+				result, err := reconciler.Reconcile(request)
+
+				Expect(err).To(BeNil())
+				Expect(result).To(Equal(reconcile.Result{}))
+				Expect(config.Finalizers).To(Not(BeNil()))
+				Expect(config.Annotations[AnnCurrentProgress]).To(Not(BeNil()))
+
+				// Now make the resource deleted and call Reocncile
+				config.SetDeletionTimestamp(&v1.Time{})
+
+				result, err = reconciler.Reconcile(request)
+
+				Expect(err).To(BeNil())
+				Expect(result).To(Equal(reconcile.Result{}))
+				Expect(config.Finalizers).To(BeNil())
+				counterValueAfter := getCounterCancelled()
+				Expect(counterValueAfter).To(Equal(counterValueBefore + 1))
+			})
+		})
 	})
 })
 
@@ -1880,3 +2067,21 @@ func newVM() *ovirtsdk.Vm {
 		Affinity(ovirtsdk.VMAFFINITY_MIGRATABLE).MustBuild())
 	return &vm
 }
+
+func getCounterFailed() float64 {
+	value, err := metrics.ImportCounter.GetFailed()
+	Expect(err).To(BeNil())
+	return value
+}
+
+func getCounterCancelled() float64 {
+	value, err := metrics.ImportCounter.GetCancelled()
+	Expect(err).To(BeNil())
+	return value
+}
+
+func getCounterSuccessful() float64 {
+	value, err := metrics.ImportCounter.GetSuccessful()
+	Expect(err).To(BeNil())
+	return value
+}

pkg/metrics/metrics.go

@@ -2,6 +2,7 @@ package metrics
 
 import (
 	"github.com/prometheus/client_golang/prometheus"
+	dto "github.com/prometheus/client_model/go"
 	"sigs.k8s.io/controller-runtime/pkg/metrics"
 )
 
@@ -36,12 +37,36 @@ type importCounter struct {
 	importCounterVec *prometheus.CounterVec
 }
 
+// Return current value of counter. If error is not nil then value is undefined
+func (ic *importCounter) getValue(labels prometheus.Labels) (float64, error) {
+	var m = &dto.Metric{}
+	err := ic.importCounterVec.With(labels).Write(m)
+	return m.Counter.GetValue(), err
+}
+
 // IncFailed increment failed label
 func (ic *importCounter) IncFailed() {
 	ic.importCounterVec.With(prometheus.Labels{"result": "failed"}).Inc()
 }
 
+func (ic *importCounter) GetFailed() (float64, error) {
+	return ic.getValue(prometheus.Labels{"result": "failed"})
+}
+
 // IncSuccessful increment successfull label
 func (ic *importCounter) IncSuccessful() {
 	ic.importCounterVec.With(prometheus.Labels{"result": "successful"}).Inc()
 }
+
+func (ic *importCounter) GetSuccessful() (float64, error) {
+	return ic.getValue(prometheus.Labels{"result": "successful"})
+}
+
+// IncCancelled increment successfull label
+func (ic *importCounter) IncCancelled() {
+	ic.importCounterVec.With(prometheus.Labels{"result": "cancelled"}).Inc()
+}
+
+func (ic *importCounter) GetCancelled() (float64, error) {
+	return ic.getValue(prometheus.Labels{"result": "cancelled"})
+}