mirror of
https://github.com/open-cluster-management-io/ocm.git
synced 2026-02-14 18:09:57 +00:00
Fix ManifestWorkReplicaSet not deleting ManifestWorks from old placement (#1206)
When a ManifestWorkReplicaSet's placementRef was changed, the ManifestWorks created for the old placement were not deleted, causing orphaned resources. The deployReconciler only processed placements currently in the spec and never cleaned up ManifestWorks from removed placements. This commit adds cleanup logic that: - Builds a set of current placement names from the spec - Lists all ManifestWorks belonging to the ManifestWorkReplicaSet - Deletes any ManifestWorks with placement labels not in current spec Also adds comprehensive tests: - Integration test verifying placement change cleanup - Unit tests for single and multiple placement change scenarios Fixes #1203 🤖 Generated with [Claude Code](https://claude.com/claude-code) Signed-off-by: Jian Qiu <jqiu@redhat.com> Co-authored-by: Claude <noreply@anthropic.com>
This commit is contained in:
Jian Qiu
GitHub
@@ -36,6 +36,31 @@ func (d *deployReconciler) reconcile(ctx context.Context, mwrSet *workapiv1alpha
|
||||
var plcsSummary []workapiv1alpha1.PlacementSummary
|
||||
minRequeue := maxRequeueTime
|
||||
count, total := 0, 0
|
||||
|
||||
// Clean up ManifestWorks from placements no longer in the spec
|
||||
currentPlacementNames := sets.New[string]()
|
||||
for _, placementRef := range mwrSet.Spec.PlacementRefs {
|
||||
currentPlacementNames.Insert(placementRef.Name)
|
||||
}
|
||||
|
||||
// Get all ManifestWorks belonging to this ManifestWorkReplicaSet
|
||||
allManifestWorks, err := listManifestWorksByManifestWorkReplicaSet(mwrSet, d.manifestWorkLister)
|
||||
if err != nil {
|
||||
return mwrSet, reconcileContinue, fmt.Errorf("failed to list manifestworks: %w", err)
|
||||
}
|
||||
|
||||
// Delete ManifestWorks that belong to placements no longer in the spec
|
||||
for _, mw := range allManifestWorks {
|
||||
placementName, ok := mw.Labels[ManifestWorkReplicaSetPlacementNameLabelKey]
|
||||
if !ok || !currentPlacementNames.Has(placementName) {
|
||||
// This ManifestWork belongs to a placement that's no longer in the spec, delete it
|
||||
err := d.workApplier.Delete(ctx, mw.Namespace, mw.Name)
|
||||
if err != nil {
|
||||
errs = append(errs, fmt.Errorf("failed to delete manifestwork %s/%s for removed placement %s: %w", mw.Namespace, mw.Name, placementName, err))
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// Getting the placements and the created ManifestWorks related to each placement
|
||||
for _, placementRef := range mwrSet.Spec.PlacementRefs {
|
||||
var existingRolloutClsStatus []clustersdkv1alpha1.ClusterRolloutStatus
|
||||
|
||||
@@ -629,3 +629,157 @@ func TestRequeueWithProgressDeadline(t *testing.T) {
|
||||
t.Errorf("expect to get err %t", err)
|
||||
}
|
||||
}
|
||||
|
||||
func TestDeployReconcileDeletesOrphanedManifestWorks(t *testing.T) {
|
||||
// Create a ManifestWorkReplicaSet that references placement2
|
||||
mwrSet := helpertest.CreateTestManifestWorkReplicaSet("mwrSet-test", "default", "place-test2")
|
||||
|
||||
// Create ManifestWorks for both placement1 (old, should be deleted) and placement2 (current, should be kept)
|
||||
mwOldPlacement, _ := CreateManifestWork(mwrSet, "cls1", "place-test1")
|
||||
mwCurrentPlacement, _ := CreateManifestWork(mwrSet, "cls2", "place-test2")
|
||||
|
||||
fWorkClient := fakeworkclient.NewSimpleClientset(mwrSet, mwOldPlacement, mwCurrentPlacement)
|
||||
workInformerFactory := workinformers.NewSharedInformerFactoryWithOptions(fWorkClient, 1*time.Second)
|
||||
|
||||
if err := workInformerFactory.Work().V1().ManifestWorks().Informer().GetStore().Add(mwOldPlacement); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if err := workInformerFactory.Work().V1().ManifestWorks().Informer().GetStore().Add(mwCurrentPlacement); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if err := workInformerFactory.Work().V1alpha1().ManifestWorkReplicaSets().Informer().GetStore().Add(mwrSet); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
mwLister := workInformerFactory.Work().V1().ManifestWorks().Lister()
|
||||
|
||||
// Create placement2 (the current placement in the spec)
|
||||
placement2, placementDecision2 := helpertest.CreateTestPlacement("place-test2", "default", "cls2")
|
||||
fClusterClient := fakeclusterclient.NewSimpleClientset(placement2, placementDecision2)
|
||||
clusterInformerFactory := clusterinformers.NewSharedInformerFactoryWithOptions(fClusterClient, 1*time.Second)
|
||||
|
||||
if err := clusterInformerFactory.Cluster().V1beta1().Placements().Informer().GetStore().Add(placement2); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if err := clusterInformerFactory.Cluster().V1beta1().PlacementDecisions().Informer().GetStore().Add(placementDecision2); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
|
||||
placementLister := clusterInformerFactory.Cluster().V1beta1().Placements().Lister()
|
||||
placementDecisionLister := clusterInformerFactory.Cluster().V1beta1().PlacementDecisions().Lister()
|
||||
|
||||
pmwDeployController := deployReconciler{
|
||||
workApplier: workapplier.NewWorkApplierWithTypedClient(fWorkClient, mwLister),
|
||||
manifestWorkLister: mwLister,
|
||||
placeDecisionLister: placementDecisionLister,
|
||||
placementLister: placementLister,
|
||||
}
|
||||
|
||||
// Run reconcile
|
||||
_, _, err := pmwDeployController.reconcile(context.TODO(), mwrSet)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
|
||||
// Verify that the ManifestWork from the old placement (place-test1) was deleted
|
||||
deletedMW, err := fWorkClient.WorkV1().ManifestWorks("cls1").Get(context.TODO(), mwrSet.Name, metav1.GetOptions{})
|
||||
if err == nil {
|
||||
t.Fatalf("Expected ManifestWork for old placement to be deleted, but it still exists: %v", deletedMW)
|
||||
}
|
||||
|
||||
// Verify that the ManifestWork from the current placement (place-test2) still exists
|
||||
currentMW, err := fWorkClient.WorkV1().ManifestWorks("cls2").Get(context.TODO(), mwrSet.Name, metav1.GetOptions{})
|
||||
if err != nil {
|
||||
t.Fatalf("Expected ManifestWork for current placement to exist, but got error: %v", err)
|
||||
}
|
||||
if currentMW == nil {
|
||||
t.Fatal("Expected ManifestWork for current placement to exist, but it is nil")
|
||||
}
|
||||
|
||||
// Verify the placement label is correct
|
||||
if currentMW.Labels[ManifestWorkReplicaSetPlacementNameLabelKey] != "place-test2" {
|
||||
t.Fatalf("Expected placement label to be 'place-test2', got '%s'", currentMW.Labels[ManifestWorkReplicaSetPlacementNameLabelKey])
|
||||
}
|
||||
}
|
||||
|
||||
func TestDeployReconcileWithMultiplePlacementChanges(t *testing.T) {
|
||||
// Create a ManifestWorkReplicaSet that references both placement2 and placement3
|
||||
allRollOut := clusterv1alpha1.RolloutStrategy{Type: clusterv1alpha1.All}
|
||||
mwrSet := helpertest.CreateTestManifestWorkReplicaSetWithRollOutStrategy("mwrSet-test", "default",
|
||||
map[string]clusterv1alpha1.RolloutStrategy{"place-test2": allRollOut, "place-test3": allRollOut})
|
||||
|
||||
// Create ManifestWorks for placement1 (old), placement2 (current), and placement3 (current)
|
||||
mwOldPlacement, _ := CreateManifestWork(mwrSet, "cls1", "place-test1")
|
||||
mwCurrentPlacement2, _ := CreateManifestWork(mwrSet, "cls2", "place-test2")
|
||||
mwCurrentPlacement3, _ := CreateManifestWork(mwrSet, "cls3", "place-test3")
|
||||
|
||||
fWorkClient := fakeworkclient.NewSimpleClientset(mwrSet, mwOldPlacement, mwCurrentPlacement2, mwCurrentPlacement3)
|
||||
workInformerFactory := workinformers.NewSharedInformerFactoryWithOptions(fWorkClient, 1*time.Second)
|
||||
|
||||
if err := workInformerFactory.Work().V1().ManifestWorks().Informer().GetStore().Add(mwOldPlacement); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if err := workInformerFactory.Work().V1().ManifestWorks().Informer().GetStore().Add(mwCurrentPlacement2); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if err := workInformerFactory.Work().V1().ManifestWorks().Informer().GetStore().Add(mwCurrentPlacement3); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if err := workInformerFactory.Work().V1alpha1().ManifestWorkReplicaSets().Informer().GetStore().Add(mwrSet); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
mwLister := workInformerFactory.Work().V1().ManifestWorks().Lister()
|
||||
|
||||
// Create placement2 and placement3 (the current placements in the spec)
|
||||
placement2, placementDecision2 := helpertest.CreateTestPlacement("place-test2", "default", "cls2")
|
||||
placement3, placementDecision3 := helpertest.CreateTestPlacement("place-test3", "default", "cls3")
|
||||
fClusterClient := fakeclusterclient.NewSimpleClientset(placement2, placementDecision2, placement3, placementDecision3)
|
||||
clusterInformerFactory := clusterinformers.NewSharedInformerFactoryWithOptions(fClusterClient, 1*time.Second)
|
||||
|
||||
if err := clusterInformerFactory.Cluster().V1beta1().Placements().Informer().GetStore().Add(placement2); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if err := clusterInformerFactory.Cluster().V1beta1().PlacementDecisions().Informer().GetStore().Add(placementDecision2); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if err := clusterInformerFactory.Cluster().V1beta1().Placements().Informer().GetStore().Add(placement3); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if err := clusterInformerFactory.Cluster().V1beta1().PlacementDecisions().Informer().GetStore().Add(placementDecision3); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
|
||||
placementLister := clusterInformerFactory.Cluster().V1beta1().Placements().Lister()
|
||||
placementDecisionLister := clusterInformerFactory.Cluster().V1beta1().PlacementDecisions().Lister()
|
||||
|
||||
pmwDeployController := deployReconciler{
|
||||
workApplier: workapplier.NewWorkApplierWithTypedClient(fWorkClient, mwLister),
|
||||
manifestWorkLister: mwLister,
|
||||
placeDecisionLister: placementDecisionLister,
|
||||
placementLister: placementLister,
|
||||
}
|
||||
|
||||
// Run reconcile
|
||||
_, _, err := pmwDeployController.reconcile(context.TODO(), mwrSet)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
|
||||
// Verify that the ManifestWork from the old placement (place-test1) was deleted
|
||||
_, err = fWorkClient.WorkV1().ManifestWorks("cls1").Get(context.TODO(), mwrSet.Name, metav1.GetOptions{})
|
||||
if err == nil {
|
||||
t.Fatal("Expected ManifestWork for old placement to be deleted, but it still exists")
|
||||
}
|
||||
|
||||
// Verify that ManifestWorks from current placements (place-test2 and place-test3) still exist
|
||||
currentMW2, err := fWorkClient.WorkV1().ManifestWorks("cls2").Get(context.TODO(), mwrSet.Name, metav1.GetOptions{})
|
||||
if err != nil {
|
||||
t.Fatalf("Expected ManifestWork for placement2 to exist, but got error: %v", err)
|
||||
}
|
||||
assert.Equal(t, currentMW2.Labels[ManifestWorkReplicaSetPlacementNameLabelKey], "place-test2")
|
||||
|
||||
currentMW3, err := fWorkClient.WorkV1().ManifestWorks("cls3").Get(context.TODO(), mwrSet.Name, metav1.GetOptions{})
|
||||
if err != nil {
|
||||
t.Fatalf("Expected ManifestWork for placement3 to exist, but got error: %v", err)
|
||||
}
|
||||
assert.Equal(t, currentMW3.Labels[ManifestWorkReplicaSetPlacementNameLabelKey], "place-test3")
|
||||
}
|
||||
|
||||
@@ -201,6 +201,168 @@ var _ = ginkgo.Describe("ManifestWorkReplicaSet", func() {
|
||||
Available: 0,
|
||||
}, manifestWorkReplicaSet), eventuallyTimeout, eventuallyInterval).Should(gomega.Succeed())
|
||||
})
|
||||
|
||||
ginkgo.It("should delete manifestworks from old placement when placementRef changes", func() {
|
||||
// Create first placement and placementDecision
|
||||
placement1 := &clusterv1beta1.Placement{
|
||||
ObjectMeta: metav1.ObjectMeta{
|
||||
Name: "test-placement-1",
|
||||
Namespace: namespaceName,
|
||||
},
|
||||
}
|
||||
_, err := hubClusterClient.ClusterV1beta1().Placements(placement1.Namespace).Create(context.TODO(), placement1, metav1.CreateOptions{})
|
||||
gomega.Expect(err).ToNot(gomega.HaveOccurred())
|
||||
|
||||
placementDecision1 := &clusterv1beta1.PlacementDecision{
|
||||
ObjectMeta: metav1.ObjectMeta{
|
||||
Name: "test-placement-decision-1",
|
||||
Namespace: namespaceName,
|
||||
Labels: map[string]string{
|
||||
clusterv1beta1.PlacementLabel: placement1.Name,
|
||||
clusterv1beta1.DecisionGroupIndexLabel: "0",
|
||||
},
|
||||
},
|
||||
}
|
||||
decision1, err := hubClusterClient.ClusterV1beta1().PlacementDecisions(placementDecision1.Namespace).Create(
|
||||
context.TODO(), placementDecision1, metav1.CreateOptions{})
|
||||
gomega.Expect(err).ToNot(gomega.HaveOccurred())
|
||||
|
||||
// Create clusters for first placement
|
||||
cluster1Names := sets.New[string]()
|
||||
for i := 0; i < 2; i++ {
|
||||
clusterName := "cluster-placement1-" + utilrand.String(5)
|
||||
ns := &corev1.Namespace{}
|
||||
ns.Name = clusterName
|
||||
_, err = spokeKubeClient.CoreV1().Namespaces().Create(context.TODO(), ns, metav1.CreateOptions{})
|
||||
gomega.Expect(err).ToNot(gomega.HaveOccurred())
|
||||
decision1.Status.Decisions = append(decision1.Status.Decisions, clusterv1beta1.ClusterDecision{ClusterName: clusterName})
|
||||
cluster1Names.Insert(clusterName)
|
||||
}
|
||||
_, err = hubClusterClient.ClusterV1beta1().PlacementDecisions(placementDecision1.Namespace).UpdateStatus(context.TODO(), decision1, metav1.UpdateOptions{})
|
||||
gomega.Expect(err).ToNot(gomega.HaveOccurred())
|
||||
|
||||
// Create manifestWorkReplicaSet with first placement
|
||||
manifests := []workapiv1.Manifest{
|
||||
util.ToManifest(util.NewConfigmap("defaut", cm1, map[string]string{"a": "b"}, nil)),
|
||||
}
|
||||
placementRef1 := workapiv1alpha1.LocalPlacementReference{
|
||||
Name: placement1.Name,
|
||||
RolloutStrategy: clusterv1alpha1.RolloutStrategy{Type: clusterv1alpha1.All},
|
||||
}
|
||||
|
||||
manifestWorkReplicaSet := &workapiv1alpha1.ManifestWorkReplicaSet{
|
||||
ObjectMeta: metav1.ObjectMeta{
|
||||
Name: "test-work",
|
||||
Namespace: namespaceName,
|
||||
},
|
||||
Spec: workapiv1alpha1.ManifestWorkReplicaSetSpec{
|
||||
ManifestWorkTemplate: workapiv1.ManifestWorkSpec{
|
||||
Workload: workapiv1.ManifestsTemplate{
|
||||
Manifests: manifests,
|
||||
},
|
||||
},
|
||||
PlacementRefs: []workapiv1alpha1.LocalPlacementReference{placementRef1},
|
||||
},
|
||||
}
|
||||
|
||||
_, err = hubWorkClient.WorkV1alpha1().ManifestWorkReplicaSets(namespaceName).Create(context.TODO(), manifestWorkReplicaSet, metav1.CreateOptions{})
|
||||
gomega.Expect(err).ToNot(gomega.HaveOccurred())
|
||||
|
||||
// Verify manifestworks are created for first placement
|
||||
gomega.Eventually(func() error {
|
||||
key := fmt.Sprintf("%s.%s", manifestWorkReplicaSet.Namespace, manifestWorkReplicaSet.Name)
|
||||
works, err := hubWorkClient.WorkV1().ManifestWorks(metav1.NamespaceAll).List(context.TODO(), metav1.ListOptions{
|
||||
LabelSelector: fmt.Sprintf("work.open-cluster-management.io/manifestworkreplicaset=%s,work.open-cluster-management.io/placementname=%s", key, placement1.Name),
|
||||
})
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
if len(works.Items) != 2 {
|
||||
return fmt.Errorf("expected 2 manifestworks, got %d", len(works.Items))
|
||||
}
|
||||
return nil
|
||||
}, eventuallyTimeout, eventuallyInterval).Should(gomega.Succeed())
|
||||
|
||||
ginkgo.By("Create second placement and update manifestWorkReplicaSet to use it")
|
||||
placement2 := &clusterv1beta1.Placement{
|
||||
ObjectMeta: metav1.ObjectMeta{
|
||||
Name: "test-placement-2",
|
||||
Namespace: namespaceName,
|
||||
},
|
||||
}
|
||||
_, err = hubClusterClient.ClusterV1beta1().Placements(placement2.Namespace).Create(context.TODO(), placement2, metav1.CreateOptions{})
|
||||
gomega.Expect(err).ToNot(gomega.HaveOccurred())
|
||||
|
||||
placementDecision2 := &clusterv1beta1.PlacementDecision{
|
||||
ObjectMeta: metav1.ObjectMeta{
|
||||
Name: "test-placement-decision-2",
|
||||
Namespace: namespaceName,
|
||||
Labels: map[string]string{
|
||||
clusterv1beta1.PlacementLabel: placement2.Name,
|
||||
clusterv1beta1.DecisionGroupIndexLabel: "0",
|
||||
},
|
||||
},
|
||||
}
|
||||
decision2, err := hubClusterClient.ClusterV1beta1().PlacementDecisions(placementDecision2.Namespace).Create(
|
||||
context.TODO(), placementDecision2, metav1.CreateOptions{})
|
||||
gomega.Expect(err).ToNot(gomega.HaveOccurred())
|
||||
|
||||
// Create clusters for second placement
|
||||
cluster2Names := sets.New[string]()
|
||||
for i := 0; i < 2; i++ {
|
||||
clusterName := "cluster-placement2-" + utilrand.String(5)
|
||||
ns := &corev1.Namespace{}
|
||||
ns.Name = clusterName
|
||||
_, err = spokeKubeClient.CoreV1().Namespaces().Create(context.TODO(), ns, metav1.CreateOptions{})
|
||||
gomega.Expect(err).ToNot(gomega.HaveOccurred())
|
||||
decision2.Status.Decisions = append(decision2.Status.Decisions, clusterv1beta1.ClusterDecision{ClusterName: clusterName})
|
||||
cluster2Names.Insert(clusterName)
|
||||
}
|
||||
_, err = hubClusterClient.ClusterV1beta1().PlacementDecisions(placementDecision2.Namespace).UpdateStatus(context.TODO(), decision2, metav1.UpdateOptions{})
|
||||
gomega.Expect(err).ToNot(gomega.HaveOccurred())
|
||||
|
||||
// Update manifestWorkReplicaSet to use second placement
|
||||
placementRef2 := workapiv1alpha1.LocalPlacementReference{
|
||||
Name: placement2.Name,
|
||||
RolloutStrategy: clusterv1alpha1.RolloutStrategy{Type: clusterv1alpha1.All},
|
||||
}
|
||||
|
||||
mwrs, err := hubWorkClient.WorkV1alpha1().ManifestWorkReplicaSets(namespaceName).Get(context.TODO(), manifestWorkReplicaSet.Name, metav1.GetOptions{})
|
||||
gomega.Expect(err).ToNot(gomega.HaveOccurred())
|
||||
mwrs.Spec.PlacementRefs = []workapiv1alpha1.LocalPlacementReference{placementRef2}
|
||||
_, err = hubWorkClient.WorkV1alpha1().ManifestWorkReplicaSets(namespaceName).Update(context.TODO(), mwrs, metav1.UpdateOptions{})
|
||||
gomega.Expect(err).ToNot(gomega.HaveOccurred())
|
||||
|
||||
ginkgo.By("Verify manifestworks from first placement are deleted")
|
||||
gomega.Eventually(func() error {
|
||||
key := fmt.Sprintf("%s.%s", manifestWorkReplicaSet.Namespace, manifestWorkReplicaSet.Name)
|
||||
works, err := hubWorkClient.WorkV1().ManifestWorks(metav1.NamespaceAll).List(context.TODO(), metav1.ListOptions{
|
||||
LabelSelector: fmt.Sprintf("work.open-cluster-management.io/manifestworkreplicaset=%s,work.open-cluster-management.io/placementname=%s", key, placement1.Name),
|
||||
})
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
if len(works.Items) != 0 {
|
||||
return fmt.Errorf("expected 0 manifestworks for placement1, got %d", len(works.Items))
|
||||
}
|
||||
return nil
|
||||
}, eventuallyTimeout, eventuallyInterval).Should(gomega.Succeed())
|
||||
|
||||
ginkgo.By("Verify manifestworks for second placement are created")
|
||||
gomega.Eventually(func() error {
|
||||
key := fmt.Sprintf("%s.%s", manifestWorkReplicaSet.Namespace, manifestWorkReplicaSet.Name)
|
||||
works, err := hubWorkClient.WorkV1().ManifestWorks(metav1.NamespaceAll).List(context.TODO(), metav1.ListOptions{
|
||||
LabelSelector: fmt.Sprintf("work.open-cluster-management.io/manifestworkreplicaset=%s,work.open-cluster-management.io/placementname=%s", key, placement2.Name),
|
||||
})
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
if len(works.Items) != 2 {
|
||||
return fmt.Errorf("expected 2 manifestworks for placement2, got %d", len(works.Items))
|
||||
}
|
||||
return nil
|
||||
}, eventuallyTimeout, eventuallyInterval).Should(gomega.Succeed())
|
||||
})
|
||||
})
|
||||
|
||||
ginkgo.It("rollout progressive", func() {
|
||||
|
||||
Reference in New Issue
Block a user