Merge pull request #108 from Fedosin/needpredelete

🐛 Avoid failures on unchanged provider version

internal/controller/genericprovider_controller_test.go

@@ -224,17 +224,18 @@ func TestReconcilerPreflightConditions(t *testing.T) {
 func TestUpgradeDowngradeProvider(t *testing.T) {
 	testCases := []struct {
 		name       string
-		namespace  string
 		newVersion string
 	}{
+		{
+			name:       "same provider version",
+			newVersion: "v0.4.2",
+		},
 		{
 			name:       "upgrade provider version",
-			namespace:  "test-core-provider",
 			newVersion: "v0.4.3",
 		},
 		{
 			name:       "downgrade provider version",
-			namespace:  "test-core-provider",
 			newVersion: "v0.4.1",
 		},
 	}
@@ -256,13 +257,15 @@ func TestUpgradeDowngradeProvider(t *testing.T) {
 				},
 			}
 
-			t.Log("Ensure namespace exists", tc.namespace)
-			g.Expect(env.EnsureNamespaceExists(ctx, tc.namespace)).To(Succeed())
+			namespace := "test-upgrades-downgrades"
 
-			g.Expect(env.CreateAndWait(ctx, dummyConfigMap(tc.namespace, testCurrentVersion))).To(Succeed())
+			t.Log("Ensure namespace exists", namespace)
+			g.Expect(env.EnsureNamespaceExists(ctx, namespace)).To(Succeed())
+
+			g.Expect(env.CreateAndWait(ctx, dummyConfigMap(namespace, testCurrentVersion))).To(Succeed())
 
 			insertDummyConfig(provider)
-			provider.SetNamespace(tc.namespace)
+			provider.SetNamespace(namespace)
 			t.Log("creating test provider", provider.GetName())
 			g.Expect(env.CreateAndWait(ctx, provider.GetObject())).To(Succeed())
 
@@ -288,12 +291,23 @@ func TestUpgradeDowngradeProvider(t *testing.T) {
 			}, timeout).Should(BeEquivalentTo(true))
 
 			// creating another configmap with another version
-			g.Expect(env.CreateAndWait(ctx, dummyConfigMap(tc.namespace, tc.newVersion))).To(Succeed())
+			if tc.newVersion != testCurrentVersion {
+				g.Expect(env.CreateAndWait(ctx, dummyConfigMap(namespace, tc.newVersion))).To(Succeed())
+			}
 
 			// Change provider version
 			providerSpec := provider.GetSpec()
 			providerSpec.Version = tc.newVersion
 			provider.SetSpec(providerSpec)
+
+			// Set label (needed to start a reconciliation of the provider)
+			labels := provider.GetLabels()
+			if labels == nil {
+				labels = map[string]string{}
+			}
+			labels["provider-version"] = tc.newVersion
+			provider.SetLabels(labels)
+
 			g.Expect(env.Client.Update(ctx, provider.GetObject())).To(Succeed())
 
 			g.Eventually(func() bool {
@@ -305,6 +319,10 @@ func TestUpgradeDowngradeProvider(t *testing.T) {
 					return false
 				}
 
+				if provider.GetLabels()["provider-version"] != tc.newVersion {
+					return false
+				}
+
 				for _, cond := range provider.GetStatus().Conditions {
 					if cond.Type == operatorv1.PreflightCheckCondition {
 						t.Log(t.Name(), provider.GetName(), cond)
@@ -322,14 +340,14 @@ func TestUpgradeDowngradeProvider(t *testing.T) {
 			objs = append(objs, &corev1.ConfigMap{
 				ObjectMeta: metav1.ObjectMeta{
 					Name:      testCurrentVersion,
-					Namespace: tc.namespace,
+					Namespace: namespace,
 				},
 			})
 
 			objs = append(objs, &corev1.ConfigMap{
 				ObjectMeta: metav1.ObjectMeta{
 					Name:      tc.newVersion,
-					Namespace: tc.namespace,
+					Namespace: namespace,
 				},
 			})
 

internal/controller/phases.go

@@ -334,43 +334,58 @@ func (p *phaseReconciler) fetch(ctx context.Context) (reconcile.Result, error) {
 func (p *phaseReconciler) preInstall(ctx context.Context) (reconcile.Result, error) {
 	log := ctrl.LoggerFrom(ctx)
 
-	needPreDelete, err := p.updateRequiresPreDeletion()
-	if err != nil || !needPreDelete {
-		return reconcile.Result{}, wrapPhaseError(err, "failed getting clusterctl Provider", operatorv1.ProviderInstalledCondition)
+	needPreDelete, err := p.versionChanged()
+	if err != nil {
+		return reconcile.Result{}, wrapPhaseError(err, "failed getting clusterctl Provider version", operatorv1.ProviderInstalledCondition)
+	}
+
+	// we need to delete existing components only if their version changes and something has already been installed.
+	if !needPreDelete || p.provider.GetStatus().InstalledVersion == nil {
+		return reconcile.Result{}, nil
 	}
 
 	log.Info("Upgrade detected, deleting existing components")
 
 	return p.delete(ctx)
 }
 
-// updateRequiresPreDeletion try to get installed version from provider status and decide if it's an upgrade.
-func (s *phaseReconciler) updateRequiresPreDeletion() (bool, error) {
-	installedVersion := s.provider.GetStatus().InstalledVersion
+// versionChanged try to get installed version from provider status and decide if it has changed.
+func (p *phaseReconciler) versionChanged() (bool, error) {
+	installedVersion := p.provider.GetStatus().InstalledVersion
 	if installedVersion == nil {
-		return false, nil
+		return true, nil
 	}
 
 	currentVersion, err := versionutil.ParseSemantic(*installedVersion)
 	if err != nil {
 		return false, err
 	}
 
-	res, err := currentVersion.Compare(s.components.Version())
+	res, err := currentVersion.Compare(p.components.Version())
 	if err != nil {
 		return false, err
 	}
 
 	// we need to delete installed components if versions are different
-	needPreDelete := res != 0
+	versionChanged := res != 0
 
-	return needPreDelete, nil
+	return versionChanged, nil
 }
 
 // install installs the provider components using clusterctl library.
 func (p *phaseReconciler) install(ctx context.Context) (reconcile.Result, error) {
 	log := ctrl.LoggerFrom(ctx)
 
+	versionChanged, err := p.versionChanged()
+	if err != nil {
+		return reconcile.Result{}, wrapPhaseError(err, "failed getting clusterctl Provider version", operatorv1.ProviderInstalledCondition)
+	}
+
+	// skip installation if the version hasn't changed.
+	if !versionChanged {
+		return reconcile.Result{}, nil
+	}
+
 	clusterClient := p.newClusterClient()
 
 	log.Info("Installing provider")
@@ -438,9 +453,9 @@ func clusterctlProviderName(provider genericprovider.GenericProvider) client.Obj
 }
 
 // newClusterClient returns a clusterctl client for interacting with management cluster.
-func (s *phaseReconciler) newClusterClient() cluster.Client {
-	return cluster.New(cluster.Kubeconfig{}, s.configClient, cluster.InjectProxy(&controllerProxy{
-		ctrlClient: s.ctrlClient,
-		ctrlConfig: s.ctrlConfig,
+func (p *phaseReconciler) newClusterClient() cluster.Client {
+	return cluster.New(cluster.Kubeconfig{}, p.configClient, cluster.InjectProxy(&controllerProxy{
+		ctrlClient: p.ctrlClient,
+		ctrlConfig: p.ctrlConfig,
 	}))
 }