Don't ignore earlier completed deployments

This sometimes breaks update workflow as we set the containerImages
from the completed deployments and if the nodeset misses
a reconciliation after `ovn-update` deployment, containerImage
for ovn-controller won't be changed as it would pick only the last
`update` deployment, which does not have the changed ovn-controller image
and it would not match with the openstackversion images.

Signed-off-by: rabi <[email protected]>

Author: rabi

controllers/dataplane/openstackdataplanenodeset_controller.go

@@ -476,7 +476,9 @@ func (r *OpenStackDataPlaneNodeSetReconciler) Reconcile(ctx context.Context, req
 }
 
 func checkDeployment(ctx context.Context, helper *helper.Helper,
-	instance *dataplanev1.OpenStackDataPlaneNodeSet) (isDeploymentReady bool, isDeploymentRunning bool, isDeploymentFailed bool, failedDeploymentName string, err error) {
+	instance *dataplanev1.OpenStackDataPlaneNodeSet) (
+	isNodeSetDeploymentReady bool, isNodeSetDeploymentRunning bool,
+	isNodeSetDeploymentFailed bool, failedDeploymentName string, err error) {
 
 	// Get all completed deployments
 	deployments := &dataplanev1.OpenStackDataPlaneDeploymentList{}
@@ -486,7 +488,7 @@ func checkDeployment(ctx context.Context, helper *helper.Helper,
 	err = helper.GetClient().List(ctx, deployments, opts...)
 	if err != nil {
 		helper.GetLogger().Error(err, "Unable to retrieve OpenStackDataPlaneDeployment CRs %v")
-		return isDeploymentReady, isDeploymentRunning, isDeploymentFailed, failedDeploymentName, err
+		return isNodeSetDeploymentReady, isNodeSetDeploymentRunning, isNodeSetDeploymentFailed, failedDeploymentName, err
 	}
 
 	// Collect deployments that target this nodeset (excluding deleted ones)
@@ -528,33 +530,41 @@ func checkDeployment(ctx context.Context, helper *helper.Helper,
 		// Apply filtering for overall nodeset deployment state logic
 		isLatestDeployment := latestRelevantDeployment != nil && deployment.Name == latestRelevantDeployment.Name
 		deploymentCondition := deploymentConditions.Get(dataplanev1.NodeSetDeploymentReadyCondition)
-		isRunningDeployment := deploymentConditions.IsFalse(dataplanev1.NodeSetDeploymentReadyCondition) && !condition.IsError(deploymentCondition)
 
-		// Skip processing for overall nodeset state if not running AND not latest
-		// (This handles both completed and failed deployments that aren't the latest)
-		if !isRunningDeployment && !isLatestDeployment {
+		// Skip failed/error deployments that aren't the latest
+		// All running and completed deployments are processed
+		isCurrentDeploymentFailed := condition.IsError(deploymentCondition)
+		if isCurrentDeploymentFailed && !isLatestDeployment {
 			continue
 		}
 
+		isCurrentDeploymentRunning := deploymentConditions.IsFalse(dataplanev1.NodeSetDeploymentReadyCondition) && !isCurrentDeploymentFailed
+		isCurrentDeploymentReady := deploymentConditions.IsTrue(dataplanev1.NodeSetDeploymentReadyCondition)
+
 		// Reset the vars for every deployment that affects overall state
-		isDeploymentReady = false
-		isDeploymentRunning = false
-		if condition.IsError(deploymentCondition) {
+		isNodeSetDeploymentReady = false
+		isNodeSetDeploymentRunning = false
+		isNodeSetDeploymentFailed = false
+
+		if isCurrentDeploymentFailed {
 			err = fmt.Errorf("%s", deploymentCondition.Message)
-			isDeploymentFailed = true
 			failedDeploymentName = deployment.Name
+			isNodeSetDeploymentFailed = true
 			break
-		} else if deploymentConditions.IsFalse(dataplanev1.NodeSetDeploymentReadyCondition) {
-			isDeploymentRunning = true
-		} else if deploymentConditions.IsTrue(dataplanev1.NodeSetDeploymentReadyCondition) {
+		}
+		if isCurrentDeploymentRunning {
+			isNodeSetDeploymentRunning = true
+		}
+
+		if isCurrentDeploymentReady {
 			// If the nodeset configHash does not match with what's in the deployment or
 			// deployedBmhHash is different from current bmhRefHash.
 			if (deployment.Status.NodeSetHashes[instance.Name] != instance.Status.ConfigHash) ||
 				(!instance.Spec.PreProvisioned &&
 					deployment.Status.BmhRefHashes[instance.Name] != instance.Status.BmhRefHash) {
 				continue
 			}
-			isDeploymentReady = true
+			isNodeSetDeploymentReady = true
 			for k, v := range deployment.Status.ConfigMapHashes {
 				instance.Status.ConfigMapHashes[k] = v
 			}
@@ -587,7 +597,7 @@ func checkDeployment(ctx context.Context, helper *helper.Helper,
 				err := helper.GetClient().Get(ctx, name, service)
 				if err != nil {
 					helper.GetLogger().Error(err, "Unable to retrieve OpenStackDataPlaneService %v")
-					return isDeploymentReady, isDeploymentRunning, isDeploymentFailed, failedDeploymentName, err
+					return isNodeSetDeploymentReady, isNodeSetDeploymentRunning, isNodeSetDeploymentFailed, failedDeploymentName, err
 				}
 
 				if service.Spec.EDPMServiceType != "update" && service.Spec.EDPMServiceType != "update-services" {
@@ -602,7 +612,7 @@ func checkDeployment(ctx context.Context, helper *helper.Helper,
 		}
 	}
 
-	return isDeploymentReady, isDeploymentRunning, isDeploymentFailed, failedDeploymentName, err
+	return isNodeSetDeploymentReady, isNodeSetDeploymentRunning, isNodeSetDeploymentFailed, failedDeploymentName, err
 }
 
 // SetupWithManager sets up the controller with the Manager.

tests/functional/dataplane/openstackdataplanenodeset_controller_test.go

@@ -1494,17 +1494,12 @@ var _ = Describe("Dataplane NodeSet Test", func() {
 
 	When("Testing deployment filtering logic", func() {
 		var secondDeploymentName types.NamespacedName
-		var thirdDeploymentName types.NamespacedName
 
 		BeforeEach(func() {
 			secondDeploymentName = types.NamespacedName{
 				Name:      "edpm-deployment-2",
 				Namespace: namespace,
 			}
-			thirdDeploymentName = types.NamespacedName{
-				Name:      "edpm-deployment-3",
-				Namespace: namespace,
-			}
 		})
 
 		When("Multiple deployments exist with ServicesOverride", func() {
@@ -1546,7 +1541,7 @@ var _ = Describe("Dataplane NodeSet Test", func() {
 				}, th.Timeout, th.Interval).Should(Succeed())
 
 				// Both deployments should be in status (for visibility)
-				// But only latest deployment affects overall nodeset state
+				// All completed deployments are processed, with latest deployment determining final state
 				Eventually(func(g Gomega) {
 					instance := GetDataplaneNodeSet(dataplaneNodeSetName)
 					g.Expect(instance.Status.DeploymentStatuses).Should(HaveKey(dataplaneDeploymentName.Name))
@@ -1581,7 +1576,7 @@ var _ = Describe("Dataplane NodeSet Test", func() {
 				SimulateDNSDataComplete(dataplaneNodeSetName)
 			})
 
-			It("Should show all deployments in status but only latest affects overall state", func() {
+			It("Should show all deployments in status and process all completed deployments", func() {
 				// Complete the first deployment
 				Eventually(func(g Gomega) {
 					ansibleeeName := types.NamespacedName{
@@ -1594,7 +1589,7 @@ var _ = Describe("Dataplane NodeSet Test", func() {
 				}, th.Timeout, th.Interval).Should(Succeed())
 
 				// Both deployments should be included (for visibility)
-				// But only latest deployment affects overall nodeset state
+				// All completed deployments are processed, with latest determining final state
 				Eventually(func(g Gomega) {
 					instance := GetDataplaneNodeSet(dataplaneNodeSetName)
 					g.Expect(instance.Status.DeploymentStatuses).Should(HaveKey(dataplaneDeploymentName.Name))
@@ -1642,10 +1637,10 @@ var _ = Describe("Dataplane NodeSet Test", func() {
 
 				// Leave second deployment running (don't complete it)
 				// Both deployments should be in status (for visibility)
-				// Second deployment affects overall state (running and latest)
+				// First deployment is processed (completed), second affects final state (running and latest)
 				Eventually(func(g Gomega) {
 					instance := GetDataplaneNodeSet(dataplaneNodeSetName)
-					// Should have first deployment (for visibility)
+					// Should have first deployment (completed, processed)
 					g.Expect(instance.Status.DeploymentStatuses).Should(HaveKey(dataplaneDeploymentName.Name))
 					// Should have second deployment (running and latest)
 					g.Expect(instance.Status.DeploymentStatuses).Should(HaveKey(secondDeploymentName.Name))
@@ -1702,10 +1697,10 @@ var _ = Describe("Dataplane NodeSet Test", func() {
 				}, th.Timeout, th.Interval).Should(Succeed())
 
 				// Both error deployments should be in status (for visibility)
-				// But only latest error affects overall nodeset state
+				// First failed deployment is skipped (not latest), second error affects final state (latest)
 				Eventually(func(g Gomega) {
 					instance := GetDataplaneNodeSet(dataplaneNodeSetName)
-					// Should have first deployment (for visibility)
+					// Should have first deployment (for visibility only)
 					g.Expect(instance.Status.DeploymentStatuses).Should(HaveKey(dataplaneDeploymentName.Name))
 					// Should have second deployment (latest - affects overall state)
 					g.Expect(instance.Status.DeploymentStatuses).Should(HaveKey(secondDeploymentName.Name))
@@ -1721,7 +1716,7 @@ var _ = Describe("Dataplane NodeSet Test", func() {
 			})
 		})
 
-		When("Mixed deployment states exist", func() {
+		When("Failed deployment followed by completed deployment", func() {
 			BeforeEach(func() {
 				nodeSetSpec := DefaultDataPlaneNodeSetSpec("edpm-compute")
 				nodeSetSpec["preProvisioned"] = true
@@ -1731,18 +1726,13 @@ var _ = Describe("Dataplane NodeSet Test", func() {
 				DeferCleanup(th.DeleteInstance, CreateDNSMasq(dnsMasqName, DefaultDNSMasqSpec()))
 				DeferCleanup(th.DeleteInstance, CreateDataplaneNodeSet(dataplaneNodeSetName, nodeSetSpec))
 
-				// Create first deployment (will have error - should be ignored)
+				// Create first deployment (will fail)
 				firstDeploymentSpec := DefaultDataPlaneDeploymentSpec()
 				DeferCleanup(th.DeleteInstance, CreateDataplaneDeployment(dataplaneDeploymentName, firstDeploymentSpec))
 
-				// Create second deployment with ServicesOverride (should be ignored)
-				overrideDeploymentSpec := DefaultDataPlaneDeploymentSpec()
-				overrideDeploymentSpec["servicesOverride"] = []string{"bootstrap"}
-				DeferCleanup(th.DeleteInstance, CreateDataplaneDeployment(secondDeploymentName, overrideDeploymentSpec))
-
-				// Create third deployment (will succeed - should be processed)
-				thirdDeploymentSpec := DefaultDataPlaneDeploymentSpec()
-				DeferCleanup(th.DeleteInstance, CreateDataplaneDeployment(thirdDeploymentName, thirdDeploymentSpec))
+				// Create second deployment (will complete successfully)
+				secondDeploymentSpec := DefaultDataPlaneDeploymentSpec()
+				DeferCleanup(th.DeleteInstance, CreateDataplaneDeployment(secondDeploymentName, secondDeploymentSpec))
 
 				CreateSSHSecret(dataplaneSSHSecretName)
 				CreateCABundleSecret(caBundleSecretName)
@@ -1751,7 +1741,7 @@ var _ = Describe("Dataplane NodeSet Test", func() {
 				SimulateDNSDataComplete(dataplaneNodeSetName)
 			})
 
-			It("Should show all deployments in status but only latest affects overall state", func() {
+			It("Should show all deployments in status and process completed deployment", func() {
 				// Fail the first deployment
 				Eventually(func(g Gomega) {
 					ansibleeeName := types.NamespacedName{
@@ -1763,27 +1753,31 @@ var _ = Describe("Dataplane NodeSet Test", func() {
 					g.Expect(th.K8sClient.Status().Update(th.Ctx, ansibleEE)).To(Succeed())
 				}, th.Timeout, th.Interval).Should(Succeed())
 
-				// Complete the third deployment
+				// Complete the second deployment
 				Eventually(func(g Gomega) {
 					ansibleeeName := types.NamespacedName{
-						Name:      "bootstrap-" + thirdDeploymentName.Name + "-" + dataplaneNodeSetName.Name,
+						Name:      "bootstrap-" + secondDeploymentName.Name + "-" + dataplaneNodeSetName.Name,
 						Namespace: namespace,
 					}
 					ansibleEE := GetAnsibleee(ansibleeeName)
 					ansibleEE.Status.Succeeded = 1
 					g.Expect(th.K8sClient.Status().Update(th.Ctx, ansibleEE)).To(Succeed())
 				}, th.Timeout, th.Interval).Should(Succeed())
 
-				// All deployments should be in status (for visibility)
-				// But only latest deployment affects overall nodeset state
+				// Wait for deployment controller to populate hashes
+				Eventually(func(g Gomega) {
+					deployment := GetDataplaneDeployment(secondDeploymentName)
+					g.Expect(deployment.Status.NodeSetHashes).Should(HaveKey(dataplaneNodeSetName.Name))
+				}, th.Timeout, th.Interval).Should(Succeed())
+
+				// Both deployments should be in status (for visibility)
+				// First failed deployment is skipped (not latest), second completed deployment is processed and determines final state
 				Eventually(func(g Gomega) {
 					instance := GetDataplaneNodeSet(dataplaneNodeSetName)
-					// Should have first deployment (for visibility)
+					// Should have first deployment (for visibility only)
 					g.Expect(instance.Status.DeploymentStatuses).Should(HaveKey(dataplaneDeploymentName.Name))
-					// Should have second deployment (for visibility)
+					// Should have second deployment (completed and latest - affects overall state)
 					g.Expect(instance.Status.DeploymentStatuses).Should(HaveKey(secondDeploymentName.Name))
-					// Should have third deployment (latest - affects overall state)
-					g.Expect(instance.Status.DeploymentStatuses).Should(HaveKey(thirdDeploymentName.Name))
 				}, th.Timeout, th.Interval).Should(Succeed())
 
 				// The overall deployment condition should be true from the successful deployment