🐛 Fix ManifestWorkReplicaSet not deleting ManifestWorks from old placement

[qiujian16]

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

Summary

Related issue(s)

Fixes #

Summary by CodeRabbit

• Bug Fixes
  • Automatically cleans up outdated ManifestWorks when placement references change, preventing orphaned resources.
  • Ensures only ManifestWorks matching current placements are retained, improving consistency and resource hygiene.
• Tests
  • Added unit tests covering deletion of orphaned ManifestWorks and multi-placement updates.
  • Added integration test validating correct deletion/creation behavior when the placement reference changes across clusters.

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: qiujian16

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• pkg/work/OWNERS [qiujian16]
• test/OWNERS [qiujian16]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[coderabbitai]

Walkthrough

Adds a reconcile-time cleanup that deletes ManifestWorks tied to placements no longer referenced by a ManifestWorkReplicaSet. Introduces unit tests for single and multiple placement changes and an integration test validating deletion and recreation behavior when the placementRef set changes.

Changes

Cohort / File(s)	Summary of changes
Controller reconcile cleanup pkg/work/hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_deploy_reconcile.go	Inserts a pre-processing cleanup: builds a set of current placement names, lists existing ManifestWorks for the MWRSet, and deletes those with missing/outdated placement labels; returns early on list errors while preserving existing per-placement reconciliation.
Unit tests for cleanup logic pkg/work/hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_deploy_test.go	Adds two tests verifying that orphaned ManifestWorks are deleted when placements change, covering single and multiple current placements, and asserting correct placement labels remain.
Integration test for placementRef change test/integration/work/manifestworkreplicaset_test.go	Adds an end-to-end test that updates the MWRSet to a new placement, verifies deletion of ManifestWorks from the old placement and creation for the new placement, repeated across placement transitions.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested labels

lgtm

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Description Check	⚠️ Warning	The pull request description does not follow the repository’s required template because the “## Summary” and “## Related issue(s)” sections are present but left empty, and the detailed narrative is not organized under those headings.	Update the description to place the narrative under the “## Summary” heading and fill in the “## Related issue(s)” section with the linked issue number(s), removing any unused template comments.
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title Check	✅ Passed	The title clearly and concisely summarizes the main bug fix by indicating that ManifestWorkReplicaSet now deletes ManifestWorks from an old placement, and it includes the appropriate bug icon prefix.
Linked Issues Check	✅ Passed	The changes implement the deletion of ManifestWorks from previous placements as specified in issue #1203 by building the current placement set, listing associated ManifestWorks, and deleting orphans, and comprehensive unit and integration tests validate this behavior.
Out of Scope Changes Check	✅ Passed	All changes are focused on adding cleanup logic for orphaned ManifestWorks and corresponding tests, with no unrelated modifications detected outside the scope of issue #1203’s objectives.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9257d2e and 4a101a8.

📒 Files selected for processing (3)

• pkg/work/hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_deploy_reconcile.go (1 hunks)
• pkg/work/hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_deploy_test.go (1 hunks)
• test/integration/work/manifestworkreplicaset_test.go (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (3)

pkg/work/hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_deploy_test.go (2)

pkg/work/hub/test/helper.go (3)

• CreateTestManifestWorkReplicaSet (20-36)
• CreateTestPlacement (120-135)
• CreateTestManifestWorkReplicaSetWithRollOutStrategy (38-64)

pkg/work/hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_controller.go (1)

• ManifestWorkReplicaSetPlacementNameLabelKey (45-45)

pkg/work/hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_deploy_reconcile.go (1)

pkg/work/hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_controller.go (1)

• ManifestWorkReplicaSetPlacementNameLabelKey (45-45)

test/integration/work/manifestworkreplicaset_test.go (5)

vendor/open-cluster-management.io/api/cluster/v1beta1/types_placement.go (1)

• Placement (41-53)

vendor/open-cluster-management.io/api/cluster/v1beta1/types_placementdecision.go (4)

• PlacementDecision (20-27)
• PlacementLabel (32-32)
• DecisionGroupIndexLabel (34-34)
• ClusterDecision (50-61)

test/integration/util/structured.go (2)

• ToManifest (84-88)
• NewConfigmap (37-52)

vendor/open-cluster-management.io/api/work/v1alpha1/types_manifestworkreplicaset.go (3)

• LocalPlacementReference (98-108)
• ManifestWorkReplicaSet (45-54)
• ManifestWorkReplicaSetSpec (57-77)

vendor/open-cluster-management.io/api/work/v1/types.go (2)

• ManifestWorkSpec (37-57)
• ManifestsTemplate (67-71)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)

• GitHub Check: e2e-singleton
• GitHub Check: e2e-hosted
• GitHub Check: e2e
• GitHub Check: cloudevents-integration
• GitHub Check: unit
• GitHub Check: integration
• GitHub Check: verify
• GitHub Check: build

🔇 Additional comments (4)

pkg/work/hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_deploy_reconcile.go (1)

40-62: LGTM! Cleanup logic correctly removes orphaned ManifestWorks.

The implementation correctly:

• Builds a set of current placement names from the spec
• Lists all ManifestWorks belonging to this ManifestWorkReplicaSet
• Deletes ManifestWorks whose placement labels are missing or refer to placements no longer in the spec
• Collects deletion errors and continues processing other ManifestWorks

However, consider this minor improvement to the error message:

 		if !ok || !currentPlacementNames.Has(placementName) {
 			// This ManifestWork belongs to a placement that's no longer in the spec, delete it
+			placementNameForLog := placementName
+			if !ok {
+				placementNameForLog = "<missing label>"
+			}
 			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))
+				errs = append(errs, fmt.Errorf("failed to delete manifestwork %s/%s for removed placement %s: %w", mw.Namespace, mw.Name, placementNameForLog, err))
 			}
 		}

test/integration/work/manifestworkreplicaset_test.go (1)

205-365: LGTM! Comprehensive integration test for placement change scenario.

The test correctly validates the complete cleanup flow:

• Creates an initial placement and ManifestWorkReplicaSet
• Verifies ManifestWorks are created for the initial placement
• Updates the ManifestWorkReplicaSet to reference a different placement
• Confirms ManifestWorks from the old placement are deleted
• Confirms ManifestWorks for the new placement are created

The use of label-based filtering (work.open-cluster-management.io/placementname) ensures precise verification of the cleanup behavior.

pkg/work/hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_deploy_test.go (2)

633-702: LGTM! Unit test correctly validates cleanup of orphaned ManifestWorks.

The test effectively validates the single placement change scenario:

• Creates a ManifestWorkReplicaSet referencing a current placement
• Seeds ManifestWorks for both an old placement and the current placement
• Verifies the reconcile logic deletes the orphaned ManifestWork (from old placement)
• Confirms the current placement's ManifestWork is retained with the correct label

---

704-785: LGTM! Unit test correctly validates cleanup with multiple placements.

The test effectively extends coverage to multiple current placements:

• Creates a ManifestWorkReplicaSet referencing two current placements
• Seeds ManifestWorks for an old placement and both current placements
• Verifies the reconcile logic deletes only the orphaned ManifestWork
• Confirms both current placements' ManifestWorks are retained with correct labels

This test ensures the cleanup logic works correctly when multiple placements are active simultaneously.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[codecov]

Codecov Report

❌ Patch coverage is 66.66667% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 61.28%. Comparing base (35bab44) to head (4a101a8).
⚠️ Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
...troller/manifestworkreplicaset_deploy_reconcile.go	66.66%	4 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1206      +/-   ##
==========================================
- Coverage   61.33%   61.28%   -0.06%     
==========================================
  Files         207      209       +2     
  Lines       20545    20881     +336     
==========================================
+ Hits        12602    12797     +195     
- Misses       6846     6967     +121     
- Partials     1097     1117      +20     

Flag	Coverage Δ
unit	61.28% <66.66%> (-0.06%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.