MGMT-20175: mark upgrade in progress status earlier (#101)

Signed-off-by: Riccardo Piccoli <rpiccoli@redhat.com>

Author: rccrdpccl

controlplane/internal/controller/openshiftassistedcontrolplane_controller.go

@@ -219,7 +219,24 @@ func (r *OpenshiftAssistedControlPlaneReconciler) Reconcile(ctx context.Context,
 }
 
 func (r *OpenshiftAssistedControlPlaneReconciler) upgradeWorkloadCluster(ctx context.Context, cluster *clusterv1.Cluster, oacp *controlplanev1alpha2.OpenshiftAssistedControlPlane, log logr.Logger, pullSecret []byte) (ctrl.Result, error) {
-	// Retrieve KubeConfig
+	var isUpdateInProgress bool
+	defer func() {
+		if isUpdateInProgress || !isWorkloadClusterRunningDesiredVersion(oacp) {
+			conditions.MarkFalse(
+				oacp,
+				controlplanev1alpha2.UpgradeCompletedCondition,
+				controlplanev1alpha2.UpgradeInProgressReason,
+				clusterv1.ConditionSeverityInfo,
+				"upgrade to version %s in progress",
+				oacp.Spec.DistributionVersion,
+			)
+			return
+		}
+		if conditions.IsFalse(oacp, controlplanev1alpha2.UpgradeCompletedCondition) {
+			conditions.MarkTrue(oacp, controlplanev1alpha2.UpgradeCompletedCondition)
+		}
+	}()
+
 	kubeConfig, err := util.GetWorkloadKubeconfig(ctx, r.Client, cluster.Name, cluster.Namespace)
 	if err != nil {
 		return ctrl.Result{}, err
@@ -229,20 +246,11 @@ func (r *OpenshiftAssistedControlPlaneReconciler) upgradeWorkloadCluster(ctx con
 	if err != nil {
 		return ctrl.Result{}, err
 	}
-	isUpdateInProgress, err := upgrader.IsUpgradeInProgress(ctx)
+	isUpdateInProgress, err = upgrader.IsUpgradeInProgress(ctx)
 	if err != nil {
 		return ctrl.Result{}, err
 	}
-	if isUpdateInProgress {
-		conditions.MarkFalse(
-			oacp,
-			controlplanev1alpha2.UpgradeCompletedCondition,
-			controlplanev1alpha2.UpgradeInProgressReason,
-			clusterv1.ConditionSeverityInfo,
-			"upgrade to version %s in progress",
-			oacp.Spec.DistributionVersion,
-		)
-	}
+
 	oacp.Status.DistributionVersion, err = upgrader.GetCurrentVersion(ctx)
 	if err != nil {
 		log.V(logutil.WarningLevel).Info("failed to get OpenShift version from ClusterVersion", "error", err.Error())
@@ -264,7 +272,6 @@ func (r *OpenshiftAssistedControlPlaneReconciler) upgradeWorkloadCluster(ctx con
 	if isWorkloadClusterRunningDesiredVersion(oacp) && !isUpdateInProgress {
 		log.V(logutil.WarningLevel).Info("Cluster is now running expected version, upgraded completed")
 
-		conditions.MarkTrue(oacp, controlplanev1alpha2.UpgradeCompletedCondition)
 		return ctrl.Result{}, nil
 	}
 

controlplane/internal/controller/openshiftassistedcontrolplane_controller_test.go

@@ -425,13 +425,24 @@ var _ = Describe("Upgrade scenarios", func() {
 		mockUpgrader.EXPECT().GetCurrentVersion(gomock.Any()).Return(desiredVersion, nil)
 		mockUpgrader.EXPECT().IsDesiredVersionUpdated(gomock.Any(), desiredVersion).Return(true, nil)
 
+		// Make sure upgrade is in progress: once we reconcile it should be marked as complete
+		conditions.MarkFalse(
+			openshiftAssistedControlPlane,
+			controlplanev1alpha2.UpgradeCompletedCondition,
+			controlplanev1alpha2.UpgradeInProgressReason,
+			clusterv1.ConditionSeverityWarning,
+			"upgrade in progress",
+		)
+		Expect(k8sClient.Status().Update(ctx, openshiftAssistedControlPlane)).To(Succeed())
+
 		result, err := controllerReconciler.Reconcile(ctx, reconcile.Request{NamespacedName: typeNamespacedName})
 		Expect(err).NotTo(HaveOccurred())
 		Expect(result).To(Equal(ctrlruntime.Result{}))
 
 		err = k8sClient.Get(ctx, typeNamespacedName, openshiftAssistedControlPlane)
 		Expect(err).NotTo(HaveOccurred())
 		Expect(openshiftAssistedControlPlane.Status.DistributionVersion).To(Equal(desiredVersion))
+
 		condition := conditions.Get(openshiftAssistedControlPlane, controlplanev1alpha2.UpgradeCompletedCondition)
 		Expect(condition).NotTo(BeNil())
 		Expect(condition.Status).To(Equal(corev1.ConditionTrue))
@@ -445,9 +456,23 @@ var _ = Describe("Upgrade scenarios", func() {
 		mockUpgrader.EXPECT().IsDesiredVersionUpdated(gomock.Any(), desiredVersion).Return(false, nil)
 		mockUpgrader.EXPECT().UpdateClusterVersionDesiredUpdate(gomock.Any(), desiredVersion, gomock.Any()).Return(expectedError)
 
+		// Make sure upgrade is in progress(not completed): even if we get no current version, now the upgrade is over
+		conditions.MarkFalse(
+			openshiftAssistedControlPlane,
+			controlplanev1alpha2.UpgradeCompletedCondition,
+			controlplanev1alpha2.UpgradeInProgressReason,
+			clusterv1.ConditionSeverityWarning,
+			"upgrade in progress",
+		)
+
 		_, err := controllerReconciler.Reconcile(ctx, reconcile.Request{NamespacedName: typeNamespacedName})
 		Expect(err).To(HaveOccurred())
 		Expect(err).To(Equal(expectedError))
+
+		// Upgrade is in progres: spec.distributionVersion is 4.15 and status.distributionVersion is 4.14
+		condition := conditions.Get(openshiftAssistedControlPlane, controlplanev1alpha2.UpgradeCompletedCondition)
+		Expect(condition).NotTo(BeNil())
+		Expect(condition.Status).To(Equal(corev1.ConditionFalse))
 	})
 
 	It("should handle errors getting current version", func() {
@@ -463,6 +488,10 @@ var _ = Describe("Upgrade scenarios", func() {
 		err = k8sClient.Get(ctx, typeNamespacedName, openshiftAssistedControlPlane)
 		Expect(err).NotTo(HaveOccurred())
 		Expect(openshiftAssistedControlPlane.Status.DistributionVersion).To(Equal(""))
+
+		// upgrade never requested, condition should still be nil
+		condition := conditions.Get(openshiftAssistedControlPlane, controlplanev1alpha2.UpgradeCompletedCondition)
+		Expect(condition).To(BeNil())
 	})
 
 	It("should handle upgrade with repository override", func() {
@@ -488,6 +517,12 @@ var _ = Describe("Upgrade scenarios", func() {
 
 		_, err := controllerReconciler.Reconcile(ctx, reconcile.Request{NamespacedName: typeNamespacedName})
 		Expect(err).NotTo(HaveOccurred())
+
+		err = k8sClient.Get(ctx, typeNamespacedName, openshiftAssistedControlPlane)
+		Expect(err).NotTo(HaveOccurred())
+		condition := conditions.Get(openshiftAssistedControlPlane, controlplanev1alpha2.UpgradeCompletedCondition)
+		Expect(condition).NotTo(BeNil())
+		Expect(condition.Status).To(Equal(corev1.ConditionFalse))
 	})
 })