Remove leftovers during upgrades

Extend the UpgradePatches mechanism
to be able to remove leftovers during
upgrades according to a json configuration
file.
Start consuming it by removing v2v-vmware and
vm-import-controller-config (not owned by HCO)
ConfigMap when upgrading from versions <=1.6.0

In a future PR, other pieces of current code
(removal of old SSP CRDs, old metrics,
old quickstarts...) can be migrated to
this generic mechanism.

Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=2063991

Signed-off-by: Simone Tiraboschi <stirabos@redhat.com>

assets/upgradePatches.json

@@ -38,5 +38,31 @@
         }
       ]
     }
+  ],
+  "objectsToBeRemoved": [
+    {
+      "semverRange": "<=1.6.0",
+      "groupVersionKind": {
+        "group": "",
+        "version": "v1",
+        "kind": "ConfigMap"
+      },
+      "objectKey": {
+        "name": "v2v-vmware",
+        "namespace": "kubevirt-hyperconverged"
+      }
+    },
+    {
+      "semverRange": "<=1.6.0",
+      "groupVersionKind": {
+        "group": "",
+        "version": "v1",
+        "kind": "ConfigMap"
+      },
+      "objectKey": {
+        "name": "vm-import-controller-config",
+        "namespace": "kubevirt-hyperconverged"
+      }
+    }
   ]
 }

controllers/hyperconverged/hyperconverged_controller.go

@@ -7,6 +7,8 @@ import (
 	"os"
 	"reflect"
 
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+
 	"github.com/blang/semver/v4"
 	jsonpatch "github.com/evanphx/json-patch"
 	"github.com/google/uuid"
@@ -264,6 +266,11 @@ func (r *ReconcileHyperConverged) Reconcile(ctx context.Context, request reconci
 
 	if r.firstLoop {
 		r.firstLoopInitialization(hcoRequest)
+		if err := validateUpgradePatches(hcoRequest); err != nil {
+			logger.Error(err, "Failed validating upgrade patches file")
+			r.eventEmitter.EmitEvent(hcoRequest.Instance, corev1.EventTypeWarning, "Failed validating upgrade patches file", err.Error())
+			os.Exit(1)
+		}
 	}
 
 	result, err := r.doReconcile(hcoRequest)
@@ -355,7 +362,6 @@ func (r *ReconcileHyperConverged) doReconcile(req *common.HcoRequest) (reconcile
 		// get into upgrade mode
 
 		r.upgradeMode = true
-
 		r.eventEmitter.EmitEvent(req.Instance, corev1.EventTypeNormal, "UpgradeHCO", "Upgrading the HyperConverged to version "+r.ownVersion)
 		req.Logger.Info(fmt.Sprintf("Start upgrading from version %s to version %s", knownHcoVersion, r.ownVersion))
 	}
@@ -372,10 +378,6 @@ func (r *ReconcileHyperConverged) doReconcile(req *common.HcoRequest) (reconcile
 }
 
 func (r *ReconcileHyperConverged) handleUpgrade(req *common.HcoRequest, init bool) (*reconcile.Result, error) {
-	err := validateUpgradePatches(req)
-	if err != nil {
-		return &reconcile.Result{Requeue: true}, err
-	}
 
 	crdStatusUpdated, err := r.updateCrdStoredVersions(req)
 	if err != nil {
@@ -1080,6 +1082,13 @@ func (r ReconcileHyperConverged) applyUpgradePatches(req *common.HcoRequest) (bo
 		}
 	}
 
+	for _, p := range hcoUpgradeChanges.ObjectsToBeRemoved {
+		removed, err := r.removeLeftover(req, knownHcoSV, p)
+		if err != nil {
+			return removed, err
+		}
+	}
+
 	tmpInstance := &hcov1beta1.HyperConverged{}
 	err = json.Unmarshal(hcoJson, tmpInstance)
 	if err != nil {
@@ -1115,6 +1124,31 @@ func (r ReconcileHyperConverged) applyUpgradePatch(req *common.HcoRequest, hcoJs
 	return hcoJson, nil
 }
 
+func (r ReconcileHyperConverged) removeLeftover(req *common.HcoRequest, knownHcoSV semver.Version, p objectToBeRemoved) (bool, error) {
+
+	affectedRange, err := semver.ParseRange(p.SemverRange)
+	if err != nil {
+		return false, err
+	}
+	if affectedRange(knownHcoSV) {
+		u := &unstructured.Unstructured{}
+		u.SetGroupVersionKind(p.GroupVersionKind)
+		gerr := r.client.Get(req.Ctx, p.ObjectKey, u)
+		if gerr != nil {
+			if apierrors.IsNotFound(gerr) {
+				return false, nil
+			} else {
+				req.Logger.Error(gerr, "failed looking for leftovers", "objectToBeRemoved", p)
+				return false, gerr
+			}
+		}
+		removeRelatedObject(req, p.GroupVersionKind, p.ObjectKey)
+		return r.deleteObj(req, u, false)
+
+	}
+	return false, nil
+}
+
 var (
 	operatorMetrics = "hyperconverged-cluster-operator-metrics"
 	webhookMetrics  = "hyperconverged-cluster-webhook-metrics"
@@ -1151,7 +1185,7 @@ func (r ReconcileHyperConverged) removeOldMetricsObjs(req *common.HcoRequest) er
 	initOldMetricsObjects(req)
 
 	for name, object := range oldMetricsObjects {
-		removed, err := r.deleteObj(req, object)
+		removed, err := r.deleteObj(req, object, true)
 
 		if err != nil {
 			return err
@@ -1165,8 +1199,8 @@ func (r ReconcileHyperConverged) removeOldMetricsObjs(req *common.HcoRequest) er
 	return nil
 }
 
-func (r *ReconcileHyperConverged) deleteObj(req *common.HcoRequest, obj client.Object) (bool, error) {
-	removed, err := hcoutil.EnsureDeleted(req.Ctx, r.client, obj, req.Instance.Name, req.Logger, false, false)
+func (r *ReconcileHyperConverged) deleteObj(req *common.HcoRequest, obj client.Object, protectNonHCOObjects bool) (bool, error) {
+	removed, err := hcoutil.EnsureDeleted(req.Ctx, r.client, obj, req.Instance.Name, req.Logger, false, false, protectNonHCOObjects)
 
 	if err != nil {
 		req.Logger.Error(
@@ -1206,7 +1240,7 @@ func removeOldQuickStartGuides(req *common.HcoRequest, cl client.Client, require
 		for name, existQs := range existingQSNames {
 			if !hcoutil.ContainsString(requiredQSList, name) {
 				req.Logger.Info("deleting ConsoleQuickStart", "name", name)
-				if _, err = hcoutil.EnsureDeleted(req.Ctx, cl, &existQs, req.Instance.Name, req.Logger, false, false); err != nil {
+				if _, err = hcoutil.EnsureDeleted(req.Ctx, cl, &existQs, req.Instance.Name, req.Logger, false, false, true); err != nil {
 					req.Logger.Error(err, "failed to delete ConsoleQuickStart", "name", name)
 				}
 			}
@@ -1238,6 +1272,25 @@ func removeRelatedQSObjects(req *common.HcoRequest, requiredNames []string) {
 
 }
 
+func removeRelatedObject(req *common.HcoRequest, gvk schema.GroupVersionKind, objectKey types.NamespacedName) {
+	refs := make([]corev1.ObjectReference, 0, len(req.Instance.Status.RelatedObjects))
+	foundRO := false
+
+	for _, obj := range req.Instance.Status.RelatedObjects {
+		if obj.GroupVersionKind() == gvk && obj.Namespace == objectKey.Namespace && obj.Name == objectKey.Name {
+			foundRO = true
+			continue
+		}
+		refs = append(refs, obj)
+	}
+
+	if foundRO {
+		req.Instance.Status.RelatedObjects = refs
+		req.StatusDirty = true
+	}
+
+}
+
 // getHyperConvergedNamespacedName returns the name/namespace of the HyperConverged resource
 func getHyperConvergedNamespacedName() (types.NamespacedName, error) {
 	hco := types.NamespacedName{

controllers/hyperconverged/hyperconverged_controller_test.go

@@ -1866,6 +1866,183 @@ var _ = Describe("HyperconvergedController", func() {
 					Expect(apierrors.IsNotFound(err)).To(BeTrue())
 				})
 			})
+
+			Context("remove leftovers on upgrades", func() {
+
+				It("should remove ConfigMap v2v-vmware upgrading from <= 1.6.0", func() {
+
+					cmToBeRemoved1 := &corev1.ConfigMap{
+						ObjectMeta: metav1.ObjectMeta{
+							Name:      "v2v-vmware",
+							Namespace: namespace,
+							Labels: map[string]string{
+								hcoutil.AppLabel: expected.hco.Name,
+							},
+						},
+					}
+					cmToBeRemoved2 := &corev1.ConfigMap{
+						ObjectMeta: metav1.ObjectMeta{
+							Name:      "vm-import-controller-config",
+							Namespace: namespace,
+						},
+					}
+					cmNotToBeRemoved1 := &corev1.ConfigMap{
+						ObjectMeta: metav1.ObjectMeta{
+							Name:      "v2v-vmware",
+							Namespace: "different" + namespace,
+							Labels: map[string]string{
+								hcoutil.AppLabel: expected.hco.Name,
+							},
+						},
+					}
+					cmNotToBeRemoved2 := &corev1.ConfigMap{
+						ObjectMeta: metav1.ObjectMeta{
+							Name:      "other",
+							Namespace: namespace,
+							Labels: map[string]string{
+								hcoutil.AppLabel: expected.hco.Name,
+							},
+						},
+					}
+
+					toBeRemovedRelatedObjects := []corev1.ObjectReference{
+						{
+							APIVersion:      "v1",
+							Kind:            "ConfigMap",
+							Name:            cmToBeRemoved1.Name,
+							Namespace:       cmToBeRemoved1.Namespace,
+							ResourceVersion: "999",
+						},
+					}
+					otherRelatedObjects := []corev1.ObjectReference{
+						{
+							APIVersion:      "v1",
+							Kind:            "ConfigMap",
+							Name:            cmNotToBeRemoved1.Name,
+							Namespace:       cmNotToBeRemoved1.Namespace,
+							ResourceVersion: "999",
+						},
+						{
+							APIVersion:      "v1",
+							Kind:            "ConfigMap",
+							Name:            cmNotToBeRemoved2.Name,
+							Namespace:       cmNotToBeRemoved2.Namespace,
+							ResourceVersion: "999",
+						},
+					}
+
+					UpdateVersion(&expected.hco.Status, hcoVersionName, "1.4.99")
+
+					for _, objRef := range toBeRemovedRelatedObjects {
+						Expect(v1.SetObjectReference(&expected.hco.Status.RelatedObjects, objRef)).ToNot(HaveOccurred())
+					}
+					for _, objRef := range otherRelatedObjects {
+						Expect(v1.SetObjectReference(&expected.hco.Status.RelatedObjects, objRef)).ToNot(HaveOccurred())
+					}
+
+					resources := append(expected.toArray(), cmToBeRemoved1, cmToBeRemoved2, cmNotToBeRemoved1, cmNotToBeRemoved2)
+
+					cl := commonTestUtils.InitClient(resources)
+					foundResource, reconciler, requeue := doReconcile(cl, expected.hco, nil)
+					Expect(requeue).To(BeTrue())
+					checkAvailability(foundResource, metav1.ConditionTrue)
+
+					foundResource, _, requeue = doReconcile(cl, expected.hco, reconciler)
+					Expect(requeue).To(BeFalse())
+					checkAvailability(foundResource, metav1.ConditionTrue)
+
+					foundCM := &corev1.ConfigMap{}
+
+					err := cl.Get(context.TODO(), client.ObjectKeyFromObject(cmToBeRemoved1), foundCM)
+					Expect(err).To(HaveOccurred())
+					Expect(apierrors.IsNotFound(err)).To(BeTrue())
+
+					err = cl.Get(context.TODO(), client.ObjectKeyFromObject(cmToBeRemoved2), foundCM)
+					Expect(err).To(HaveOccurred())
+					Expect(apierrors.IsNotFound(err)).To(BeTrue())
+
+					err = cl.Get(context.TODO(), client.ObjectKeyFromObject(cmNotToBeRemoved1), foundCM)
+					Expect(err).ToNot(HaveOccurred())
+
+					err = cl.Get(context.TODO(), client.ObjectKeyFromObject(cmNotToBeRemoved2), foundCM)
+					Expect(err).ToNot(HaveOccurred())
+
+					for _, objRef := range toBeRemovedRelatedObjects {
+						Expect(foundResource.Status.RelatedObjects).ToNot(ContainElement(objRef))
+					}
+					for _, objRef := range otherRelatedObjects {
+						Expect(foundResource.Status.RelatedObjects).To(ContainElement(objRef))
+					}
+
+				})
+
+				It("should not remove ConfigMap v2v-vmware upgrading from >= 1.6.1", func() {
+
+					cmToBeRemoved1 := &corev1.ConfigMap{
+						ObjectMeta: metav1.ObjectMeta{
+							Name:      "v2v-vmware",
+							Namespace: namespace,
+							Labels: map[string]string{
+								hcoutil.AppLabel: expected.hco.Name,
+							},
+						},
+					}
+					cmToBeRemoved2 := &corev1.ConfigMap{
+						ObjectMeta: metav1.ObjectMeta{
+							Name:      "vm-import-controller-config",
+							Namespace: namespace,
+						},
+					}
+					cmNotToBeRemoved1 := &corev1.ConfigMap{
+						ObjectMeta: metav1.ObjectMeta{
+							Name:      "v2v-vmware",
+							Namespace: "different" + namespace,
+							Labels: map[string]string{
+								hcoutil.AppLabel: expected.hco.Name,
+							},
+						},
+					}
+					cmNotToBeRemoved2 := &corev1.ConfigMap{
+						ObjectMeta: metav1.ObjectMeta{
+							Name:      "other",
+							Namespace: namespace,
+							Labels: map[string]string{
+								hcoutil.AppLabel: expected.hco.Name,
+							},
+						},
+					}
+
+					UpdateVersion(&expected.hco.Status, hcoVersionName, "1.6.1")
+
+					resources := append(expected.toArray(), cmToBeRemoved1, cmToBeRemoved2, cmNotToBeRemoved1, cmNotToBeRemoved2)
+
+					cl := commonTestUtils.InitClient(resources)
+					foundResource, reconciler, requeue := doReconcile(cl, expected.hco, nil)
+					Expect(requeue).To(BeTrue())
+					checkAvailability(foundResource, metav1.ConditionTrue)
+
+					foundResource, _, requeue = doReconcile(cl, expected.hco, reconciler)
+					Expect(requeue).To(BeFalse())
+					checkAvailability(foundResource, metav1.ConditionTrue)
+
+					foundCM := &corev1.ConfigMap{}
+
+					err := cl.Get(context.TODO(), client.ObjectKeyFromObject(cmToBeRemoved1), foundCM)
+					Expect(err).ToNot(HaveOccurred())
+
+					err = cl.Get(context.TODO(), client.ObjectKeyFromObject(cmToBeRemoved2), foundCM)
+					Expect(err).ToNot(HaveOccurred())
+
+					err = cl.Get(context.TODO(), client.ObjectKeyFromObject(cmNotToBeRemoved1), foundCM)
+					Expect(err).ToNot(HaveOccurred())
+
+					err = cl.Get(context.TODO(), client.ObjectKeyFromObject(cmNotToBeRemoved2), foundCM)
+					Expect(err).ToNot(HaveOccurred())
+
+				})
+
+			})
+
 		})
 
 		Context("Aggregate Negative Conditions", func() {

controllers/hyperconverged/test-files/upgradePatches/badObject1.json

@@ -0,0 +1,16 @@
+{
+  "objectsToBeRemoved": [
+    {
+      "semverRange": "<=1.6.0",
+      "groupVersionKind": {
+        "group": "",
+        "version": "v1",
+        "kind": ""
+      },
+      "objectKey": {
+        "name": "v2v-vmware",
+        "namespace": "kubevirt-hyperconverged"
+      }
+    }
+  ]
+}

controllers/hyperconverged/test-files/upgradePatches/badObject1m.json

@@ -0,0 +1,15 @@
+{
+  "objectsToBeRemoved": [
+    {
+      "semverRange": "<=1.6.0",
+      "groupVersionKind": {
+        "group": "",
+        "version": "v1"
+      },
+      "objectKey": {
+        "name": "v2v-vmware",
+        "namespace": "kubevirt-hyperconverged"
+      }
+    }
+  ]
+}