fix: Edit deployment action to remove one of two clusters (#293)

Author: guptagunjan

.gitignore

@@ -47,7 +47,7 @@ env/
 node_modules/
 
 # Build outputs
-*/build/
+*/build/*
 !*/build/Dockerfile.*
 */out/
 */bin/app-*

app-deployment-manager/build/Dockerfile.gateway

@@ -2,7 +2,7 @@
 #
 # SPDX-License-Identifier: Apache-2.0
 
-FROM golang:1.24.9@sha256:5034fa44b36163a4109b71ed75c67dbdcb52c3cd9750953befe00054315d9fd2 AS build
+FROM golang:1.25.5@sha256:6cc2338c038bc20f96ab32848da2b5c0641bb9bb5363f2c33e9b7c8838f9a208 AS build
 
 ENV APP_ROOT=$GOPATH/src/github.com/open-edge-platform/app-orch-deployment/app-deployment-manager
 ENV CGO_ENABLED=0

app-deployment-manager/controllers/deployment/deployment_controller.go

@@ -563,6 +563,89 @@ func (r *Reconciler) cleanupOrphanedDeploymentClusters(ctx context.Context, d *v
 	return nil
 }
 
+// cleanupRemovedTargetClustersPhase is a reconcile phase wrapper for cleanupRemovedTargetClusters
+func (r *Reconciler) cleanupRemovedTargetClustersPhase(ctx context.Context, d *v1beta1.Deployment) (ctrl.Result, error) {
+	return ctrl.Result{}, r.cleanupRemovedTargetClusters(ctx, d)
+}
+
+// cleanupRemovedTargetClusters removes DeploymentCluster resources for clusters that are no longer
+// in the deployment's target list (handles the case when a cluster is removed during edit)
+func (r *Reconciler) cleanupRemovedTargetClusters(ctx context.Context, d *v1beta1.Deployment) error {
+	log := log.FromContext(ctx)
+
+	// Only applies to targeted/manual deployments
+	if d.Spec.DeploymentType != v1beta1.Targeted {
+		return nil
+	}
+
+	log.V(2).Info("Checking for removed target clusters", "deploymentID", d.GetId(), "deploymentName", d.Name)
+
+	// Build a set of current target cluster IDs from the deployment spec
+	currentTargets := make(map[string]bool)
+	for _, app := range d.Spec.Applications {
+		for _, target := range app.Targets {
+			if clusterName, ok := target[string(v1beta1.ClusterName)]; ok && clusterName != "" {
+				currentTargets[clusterName] = true
+			}
+		}
+	}
+
+	// Find all DeploymentClusters that belong to this deployment
+	dcList := &v1beta1.DeploymentClusterList{}
+	labels := map[string]string{
+		string(v1beta1.DeploymentID): d.GetId(),
+	}
+
+	if err := r.List(ctx, dcList, client.MatchingLabels(labels)); err != nil {
+		log.Error(err, "Failed to list DeploymentClusters", "deploymentID", d.GetId())
+		return err
+	}
+
+	if len(dcList.Items) == 0 {
+		log.V(2).Info("No DeploymentClusters found", "deploymentID", d.GetId())
+		return nil
+	}
+
+	// Delete DeploymentClusters for clusters no longer in the target list
+	deletedCount := 0
+	for i := range dcList.Items {
+		dc := &dcList.Items[i]
+		clusterID := dc.Spec.ClusterID
+
+		// If this cluster is not in the current targets, delete the DeploymentCluster
+		if !currentTargets[clusterID] {
+			log.Info("Deleting DeploymentCluster for removed target cluster",
+				"deploymentCluster", dc.Name,
+				"namespace", dc.Namespace,
+				"deploymentID", d.GetId(),
+				"clusterID", clusterID)
+
+			if err := r.Client.Delete(ctx, dc); err != nil && !apierrors.IsNotFound(err) {
+				log.Error(err, "Failed to delete DeploymentCluster for removed target",
+					"deploymentCluster", dc.Name,
+					"deploymentID", d.GetId(),
+					"clusterID", clusterID)
+				return err
+			}
+			deletedCount++
+			log.Info("Successfully deleted DeploymentCluster for removed target",
+				"deploymentCluster", dc.Name,
+				"deploymentID", d.GetId(),
+				"clusterID", clusterID)
+		}
+	}
+
+	if deletedCount > 0 {
+		log.Info("Completed cleanup of removed target clusters",
+			"deleted", deletedCount,
+			"deploymentID", d.GetId())
+	} else {
+		log.V(2).Info("No removed target clusters to cleanup", "deploymentID", d.GetId())
+	}
+
+	return nil
+}
+
 func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrlRes ctrl.Result, reterr error) {
 	log := log.FromContext(ctx)
 
@@ -707,6 +790,7 @@ func (r *Reconciler) delete(ctx context.Context, d *v1beta1.Deployment) (ctrl.Re
 func (r *Reconciler) reconcile(ctx context.Context, d *v1beta1.Deployment) (ctrl.Result, error) {
 	phases := []func(context.Context, *v1beta1.Deployment) (ctrl.Result, error){
 		r.reconcileState,
+		r.cleanupRemovedTargetClustersPhase,
 		r.reconcileDependency,
 		r.reconcileRepository,
 		r.reconcileGitRepo,

app-deployment-manager/controllers/deployment/deployment_controller_test.go

@@ -20,6 +20,7 @@ import (
 	"github.com/rancher/wrangler/v3/pkg/genericcondition"
 	batchv1 "k8s.io/api/batch/v1"
 	v1 "k8s.io/api/core/v1"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
@@ -1375,4 +1376,156 @@ var _ = Describe("Deployment controller", func() {
 			})
 		})
 	})
+
+	When("cleaning up removed target clusters", func() {
+	var (
+	ctx context.Context
+	r   *Reconciler
+	c   client.WithWatch
+	d   v1beta1.Deployment
+	dc1 v1beta1.DeploymentCluster
+	dc2 v1beta1.DeploymentCluster
+	dc3 v1beta1.DeploymentCluster
+	)
+
+	BeforeEach(func() {
+	ctx = context.Background()
+
+	d = v1beta1.Deployment{
+	ObjectMeta: metav1.ObjectMeta{
+	Name:      "test-deployment",
+	Namespace: "default",
+	UID:       "test-uid-12345",
+	},
+	Spec: v1beta1.DeploymentSpec{
+	DeploymentType: v1beta1.Targeted,
+	Applications: []v1beta1.Application{
+	{
+	Name:      "app1",
+	Version:   "1.0.0",
+	Namespace: "default",
+	Targets: []map[string]string{
+	{string(v1beta1.ClusterName): "cluster-1"},
+	{string(v1beta1.ClusterName): "cluster-2"},
+	},
+	},
+	},
+	},
+	}
+
+	dc1 = v1beta1.DeploymentCluster{
+	ObjectMeta: metav1.ObjectMeta{
+	Name:      "test-deployment-cluster-1",
+	Namespace: "default",
+	Labels: map[string]string{
+	string(v1beta1.BundleName):   "test-deployment",
+	string(v1beta1.DeploymentID): "test-uid-12345",
+	},
+	OwnerReferences: []metav1.OwnerReference{
+	{APIVersion: "app.edge-orchestrator.intel.com/v1beta1", Kind: "Deployment", Name: "test-deployment", UID: "test-uid-12345"},
+	},
+	},
+	Spec: v1beta1.DeploymentClusterSpec{ClusterID: "cluster-1"},
+	}
+
+	dc2 = v1beta1.DeploymentCluster{
+	ObjectMeta: metav1.ObjectMeta{
+	Name:      "test-deployment-cluster-2",
+	Namespace: "default",
+	Labels: map[string]string{
+	string(v1beta1.BundleName):   "test-deployment",
+	string(v1beta1.DeploymentID): "test-uid-12345",
+	},
+	OwnerReferences: []metav1.OwnerReference{
+	{APIVersion: "app.edge-orchestrator.intel.com/v1beta1", Kind: "Deployment", Name: "test-deployment", UID: "test-uid-12345"},
+	},
+	},
+	Spec: v1beta1.DeploymentClusterSpec{ClusterID: "cluster-2"},
+	}
+
+	dc3 = v1beta1.DeploymentCluster{
+	ObjectMeta: metav1.ObjectMeta{
+	Name:      "test-deployment-cluster-3",
+	Namespace: "default",
+	Labels: map[string]string{
+	string(v1beta1.BundleName):   "test-deployment",
+	string(v1beta1.DeploymentID): "test-uid-12345",
+	},
+	OwnerReferences: []metav1.OwnerReference{
+	{APIVersion: "app.edge-orchestrator.intel.com/v1beta1", Kind: "Deployment", Name: "test-deployment", UID: "test-uid-12345"},
+	},
+	},
+	Spec: v1beta1.DeploymentClusterSpec{ClusterID: "cluster-3"},
+	}
+
+	c = fake.NewClientBuilder().
+	WithScheme(scheme.Scheme).
+	WithObjects(&d, &dc1, &dc2, &dc3).
+	WithStatusSubresource(&v1beta1.Deployment{}).
+	Build()
+
+	r = &Reconciler{
+	Client:   c,
+	Scheme:   scheme.Scheme,
+	recorder: record.NewFakeRecorder(1024),
+	}
+	})
+
+	Context("when cluster is removed from targeted deployment", func() {
+	It("should delete the DeploymentCluster for removed cluster", func() {
+	err := r.cleanupRemovedTargetClusters(ctx, &d)
+	Expect(err).NotTo(HaveOccurred())
+
+	err = r.Get(ctx, types.NamespacedName{Name: dc1.Name, Namespace: dc1.Namespace}, &dc1)
+	Expect(err).NotTo(HaveOccurred())
+
+	err = r.Get(ctx, types.NamespacedName{Name: dc2.Name, Namespace: dc2.Namespace}, &dc2)
+	Expect(err).NotTo(HaveOccurred())
+
+	err = r.Get(ctx, types.NamespacedName{Name: dc3.Name, Namespace: dc3.Namespace}, &dc3)
+	Expect(apierrors.IsNotFound(err)).To(BeTrue())
+	})
+	})
+
+	Context("when deployment is auto-scaling type", func() {
+	It("should not delete any DeploymentClusters", func() {
+	d.Spec.DeploymentType = v1beta1.AutoScaling
+	err := c.Update(ctx, &d)
+	Expect(err).NotTo(HaveOccurred())
+
+	err = r.cleanupRemovedTargetClusters(ctx, &d)
+	Expect(err).NotTo(HaveOccurred())
+
+	err = r.Get(ctx, types.NamespacedName{Name: dc1.Name, Namespace: dc1.Namespace}, &dc1)
+	Expect(err).NotTo(HaveOccurred())
+
+	err = r.Get(ctx, types.NamespacedName{Name: dc2.Name, Namespace: dc2.Namespace}, &dc2)
+	Expect(err).NotTo(HaveOccurred())
+
+	err = r.Get(ctx, types.NamespacedName{Name: dc3.Name, Namespace: dc3.Namespace}, &dc3)
+	Expect(err).NotTo(HaveOccurred())
+	})
+	})
+
+	Context("when all clusters are removed from deployment", func() {
+	It("should delete all DeploymentClusters", func() {
+	d.Spec.Applications[0].Targets = []map[string]string{}
+	err := c.Update(ctx, &d)
+	Expect(err).NotTo(HaveOccurred())
+
+	err = r.cleanupRemovedTargetClusters(ctx, &d)
+	Expect(err).NotTo(HaveOccurred())
+
+	err = r.Get(ctx, types.NamespacedName{Name: dc1.Name, Namespace: dc1.Namespace}, &dc1)
+	Expect(apierrors.IsNotFound(err)).To(BeTrue())
+
+	err = r.Get(ctx, types.NamespacedName{Name: dc2.Name, Namespace: dc2.Namespace}, &dc2)
+	Expect(apierrors.IsNotFound(err)).To(BeTrue())
+
+	err = r.Get(ctx, types.NamespacedName{Name: dc3.Name, Namespace: dc3.Namespace}, &dc3)
+	Expect(apierrors.IsNotFound(err)).To(BeTrue())
+	})
+	})
+	})
+
 })

app-deployment-manager/internal/northbound/northbound.go

@@ -962,6 +962,16 @@ func (s *DeploymentSvc) CreateDeployment(ctx context.Context, in *deploymentpb.C
 		return nil, errors.Status(err).Err()
 	}
 
+	// Propagate targets to child deployments (dependencies)
+	// When the deployment is created with multiple clusters, ensure child deployments
+	// (dependencies) are also deployed to the same clusters
+	if len(createInstance.Spec.ChildDeploymentList) > 0 {
+		err = s.propagateTargetsToChildDeployments(ctx, deploymentCR)
+		if err != nil {
+			log.Warnf("failed to propagate targets to child deployments during creation: %v", err)
+		}
+	}
+
 	utils.LogActivity(ctx, "create", "ADM", "deployment-name "+d.Name, "deploy-id "+d.DeployID, "deployment-app-version "+d.AppVersion)
 	return &deploymentpb.CreateDeploymentResponse{DeploymentId: d.DeployID}, nil
 }

app-deployment-manager/internal/northbound/utils.go

@@ -128,12 +128,13 @@ func createDeploymentCr(d *Deployment, scenario string, resourceVersion string,
 				}
 			}
 
-			// For update scenario, start with existing targets from the deployed CR
-			// This ensures targets accumulate across updates (UI sends one target at a time)
-			if scenario == "update" && existingDeployment != nil {
+			// For update scenario:
+			// - For AutoScaling: accumulate targets (UI sends one label set at a time)
+			// - For Targeted: replacement logic (UI sends complete desired state)
+			if scenario == "update" && existingDeployment != nil && d.DeploymentType == string(deploymentv1beta1.AutoScaling) {
 				for _, existingApp := range existingDeployment.Spec.Applications {
 					if existingApp.Name == app.Name {
-						// Start with existing targets
+						// Start with existing targets for AutoScaling only
 						targetsList = append(targetsList, existingApp.Targets...)
 						break
 					}