🐛 Use generateName when creating ManifestWorks from ManifestWorkReplicaSet#1394
Open
shivansh-source wants to merge 3 commits intoopen-cluster-management-io:mainfrom
Open
🐛 Use generateName when creating ManifestWorks from ManifestWorkReplicaSet#1394shivansh-source wants to merge 3 commits intoopen-cluster-management-io:mainfrom
shivansh-source wants to merge 3 commits intoopen-cluster-management-io:mainfrom
Conversation
Signed-off-by: shivansh-source <shivanshsiddhi1234@gmail.com>
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: shivansh-source The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Related Documentation Checked 0 published document(s) in 1 knowledge base(s). No updates required. How did I do? Any feedback? |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughReconcile now lists ManifestWorks by placement/controller labels and deletes those whose namespace matches removed clusters; ManifestWork creation uses Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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. Comment |
Author
|
/cc @mikeshng |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/work/hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_deploy_reconcile.go (1)
157-168:⚠️ Potential issue | 🔴 CriticalCritical: Broken error handling in the cluster removal loop.
The refactored deletion block has multiple intertwined bugs:
- Line 159: Error from
listManifestWorksByMWRSetPlacementRefis silently discarded (_).- Line 162: Error return from
d.workApplier.Delete()is not captured (confirmed by static analysis:errcheck).- Line 164:
errs = append(errs, err)appends a staleerrfrom outer scope (e.g., lastApplyorCreateManifestWorkcall) for every iteration ofclusterWorks, regardless of whether the namespace matched. This is leftover code from the pre-refactor single-Delete pattern that doesn't fit the new innerforloop structure.- Line 165:
continueis now redundant (continues the inner loop at the last statement).Net effect: real Delete errors are lost; stale/nil errors are accumulated spuriously.
🐛 Proposed fix
for _, cls := range rolloutResult.ClustersRemoved { // Delete manifestWork for removed clusters - clusterWorks, _ := listManifestWorksByMWRSetPlacementRef(mwrSet, placementRef.Name, d.manifestWorkLister) - for _, mw := range clusterWorks { - if mw.Namespace == cls.ClusterName { - d.workApplier.Delete(ctx, mw.Namespace, mw.Name) - } - errs = append(errs, err) - continue - } + clusterWorks, listErr := listManifestWorksByMWRSetPlacementRef(mwrSet, placementRef.Name, d.manifestWorkLister) + if listErr != nil { + errs = append(errs, fmt.Errorf("failed to list manifestworks for removed cluster %s: %w", cls.ClusterName, listErr)) + continue + } + for _, mw := range clusterWorks { + if mw.Namespace == cls.ClusterName { + if delErr := d.workApplier.Delete(ctx, mw.Namespace, mw.Name); delErr != nil { + errs = append(errs, fmt.Errorf("failed to delete manifestwork %s/%s: %w", mw.Namespace, mw.Name, delErr)) + } + } + } existingClusterNames.Delete(cls.ClusterName) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/work/hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_deploy_reconcile.go` around lines 157 - 168, The loop discards the error from listManifestWorksByMWRSetPlacementRef and misuses a stale err variable while ignoring Delete errors; fix by capturing the error from listManifestWorksByMWRSetPlacementRef (e.g., clusterWorks, err := listManifestWorksByMWRSetPlacementRef(...)) and handle/append it to errs if non-nil, then inside the inner loop call d.workApplier.Delete(ctx, mw.Namespace, mw.Name) and capture its error (err := d.workApplier.Delete(...)) and only append that err to errs when non-nil (do not append any outer-scope err); remove the redundant continue and ensure existingClusterNames.Delete(cls.ClusterName) is only called after successful processing of the removed cluster entries.
🧹 Nitpick comments (1)
pkg/work/hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_deploy_test.go (1)
768-781: Inconsistent indentation and formatting throughout this block.The mix of tabs and spaces, and flush-left lines (e.g., Line 772-774, Line 781) make this hard to read and will likely fail
gofmtchecks.♻️ Proposed formatting fix
- works, err := listWorksByMWRS(context.TODO(),fWorkClient,"cls1", mwrSet.Namespace, mwrSet.Name, "place-test1") - if err != nil { - t.Fatal(err) - } -if len(works) != 0 { - t.Fatalf("Expected ManifestWork for old placement to be deleted, but it still exists") -} + works, err := listWorksByMWRS(context.TODO(), fWorkClient, "cls1", mwrSet.Namespace, mwrSet.Name, "place-test1") + if err != nil { + t.Fatal(err) + } + if len(works) != 0 { + t.Fatalf("Expected ManifestWork for old placement to be deleted, but it still exists") + } // Verify that the ManifestWork from the current placement (place-test2) still exists - works, err = listWorksByMWRS( context.TODO(), fWorkClient, "cls2", mwrSet.Namespace, mwrSet.Name, "place-test2",) - if err != nil { t.Fatal(err) } - if len(works) != 1 { - t.Fatalf("Expected ManifestWork for current placement to exist, but got %d", len(works)) - } -currentMW := works[0] + works, err = listWorksByMWRS(context.TODO(), fWorkClient, "cls2", mwrSet.Namespace, mwrSet.Name, "place-test2") + if err != nil { + t.Fatal(err) + } + if len(works) != 1 { + t.Fatalf("Expected ManifestWork for current placement to exist, but got %d", len(works)) + } + currentMW := works[0]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/work/hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_deploy_test.go` around lines 768 - 781, The test block around listWorksByMWRS has inconsistent tabs/spaces and misaligned lines (variables: listWorksByMWRS, fWorkClient, mwrSet, works, currentMW) which will fail gofmt; reformat the entire snippet to use consistent indentation (tabs per Go conventions), align the if blocks and error checks, fix spacing around commas/parentheses, and run gofmt/goimports to apply canonical formatting so the tests compile and pass linter checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@pkg/work/hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_deploy_test.go`:
- Around line 866-868: The test's failure message in
manifestworkreplicaset_deploy_test.go incorrectly prints err when checking the
length of works; update the t.Fatalf call in the block that checks if len(works)
!= 1 to report the actual count (use %d with len(works) or include the works
slice) so the message shows how many works were found instead of printing err.
- Around line 876-878: The test's failure message is misleading because it
reports "error: %v", err even though the check is about the length of works for
placement3 and err may be nil or not relevant; update the t.Fatalf in the
manifestworkreplicaset_deploy_test (the assertion that checks if len(works) != 1
for placement3) to report the actual length and/or the contents of works instead
of err (e.g., include len(works) and the works slice or a clear message like
"expected 1 ManifestWork for placement3, got %d: %#v"), so the failure output
accurately reflects the condition being tested.
---
Outside diff comments:
In
`@pkg/work/hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_deploy_reconcile.go`:
- Around line 157-168: The loop discards the error from
listManifestWorksByMWRSetPlacementRef and misuses a stale err variable while
ignoring Delete errors; fix by capturing the error from
listManifestWorksByMWRSetPlacementRef (e.g., clusterWorks, err :=
listManifestWorksByMWRSetPlacementRef(...)) and handle/append it to errs if
non-nil, then inside the inner loop call d.workApplier.Delete(ctx, mw.Namespace,
mw.Name) and capture its error (err := d.workApplier.Delete(...)) and only
append that err to errs when non-nil (do not append any outer-scope err); remove
the redundant continue and ensure existingClusterNames.Delete(cls.ClusterName)
is only called after successful processing of the removed cluster entries.
---
Nitpick comments:
In
`@pkg/work/hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_deploy_test.go`:
- Around line 768-781: The test block around listWorksByMWRS has inconsistent
tabs/spaces and misaligned lines (variables: listWorksByMWRS, fWorkClient,
mwrSet, works, currentMW) which will fail gofmt; reformat the entire snippet to
use consistent indentation (tabs per Go conventions), align the if blocks and
error checks, fix spacing around commas/parentheses, and run gofmt/goimports to
apply canonical formatting so the tests compile and pass linter checks.
pkg/work/hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_deploy_test.go
Show resolved
Hide resolved
pkg/work/hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_deploy_test.go
Show resolved
Hide resolved
Member
annelaucg
reviewed
Feb 24, 2026
| return &workv1.ManifestWork{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: mwrSet.Name, | ||
| GenerateName: mwrSet.Name + "-", |
Contributor
There was a problem hiding this comment.
This generateName will generate a random hash behind right? How will we guarnatee its the namespace thats included in the MW name?
I see this logic below in the helper.go , but wondering how it ties to this change?
Author
There was a problem hiding this comment.
generateName only ensures uniqueness
Author
There was a problem hiding this comment.
and controller logic already relies on labels for lookup & deletion so thats why
Author
There was a problem hiding this comment.
The name assignment in helper.go is only used for unit tests.
@annelaucg
Author
There was a problem hiding this comment.
This generateName will generate a random hash behind right? How will we guarnatee its the namespace thats included in the MW name?
Yes — generateName lets the API server append a unique suffix to the ManifestWork name to avoid collisions
Contributor
There was a problem hiding this comment.
Got it, so it ensures unique-ness, but we will not append the namespace to the MW name. Any concerns about human readability of the knowing which ManifestWork was from which namespace?
Signed-off-by: shivansh-source <shivanshsiddhi1234@gmail.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/work/hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_deploy_test.go (1)
866-878:⚠️ Potential issue | 🟡 MinorFix misleading failure messages for placement2/placement3 counts.
These branches are checking the count, but the message says “got error” and printslen(works). This was already flagged before and still appears. Consider reporting the count (and optionally the slice) directly.💡 Proposed fix
- if len(works) != 1 { - t.Fatalf("Expected ManifestWork for placement2 to exist, but got error: %v", len(works)) - } + if len(works) != 1 { + t.Fatalf("Expected 1 ManifestWork for placement2, got %d", len(works)) + } @@ - if len(works) != 1 { - t.Fatalf("Expected ManifestWork for placement3 to exist, but got error: %v", len(works)) - } + if len(works) != 1 { + t.Fatalf("Expected 1 ManifestWork for placement3, got %d", len(works)) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/work/hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_deploy_test.go` around lines 866 - 878, The failure messages for placement2 and placement3 incorrectly say "got error" and print len(works); update the t.Fatalf calls after the length checks in manifestworkreplicaset_deploy_test.go to report the actual count (and optionally the slice) instead of implying an error — specifically change the two t.Fatalf lines that follow the if len(works) != 1 checks (the block that sets currentMW2 and the block that calls listWorksByMWRS for "place-test3") to something like "Expected 1 ManifestWork for placementX, but got %d: %v" using len(works) and works to make the message accurate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@pkg/work/hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_deploy_test.go`:
- Around line 866-878: The failure messages for placement2 and placement3
incorrectly say "got error" and print len(works); update the t.Fatalf calls
after the length checks in manifestworkreplicaset_deploy_test.go to report the
actual count (and optionally the slice) instead of implying an error —
specifically change the two t.Fatalf lines that follow the if len(works) != 1
checks (the block that sets currentMW2 and the block that calls listWorksByMWRS
for "place-test3") to something like "Expected 1 ManifestWork for placementX,
but got %d: %v" using len(works) and works to make the message accurate.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/work/hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_deploy_test.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes a naming conflict when ManifestWorkReplicaSets create
ManifestWorks.
Previously, ManifestWorks were created using the
ManifestWorkReplicaSet name directly. This could cause collisions
when multiple ManifestWorkReplicaSets from different namespaces
targeted the same managed cluster namespace.
This PR switches ManifestWork creation to use
generateName,allowing the API server to assign unique names.
Changes
GenerateNameinstead ofNamewhen creating ManifestWorksRelated Issue
Fixes #1380
Testing
go test ./pkg/work/...passes locallySummary by CodeRabbit
Bug Fixes
Behavior Changes
Tests