make sure the namespaced binding works

Signed-off-by: Ryan Zhang <[email protected]>

Author: ryanzhang-oss

.github/.copilot/breadcrumbs/2024-12-19-1400-refactor-work-name-generation.md

@@ -0,0 +1,79 @@
+# Refactor Work Name Generation - Common Base Name Format
+
+## Requirements
+- Refactor `getWorkNamePrefixFromSnapshotName` function to use a common base name format for all work name generation
+- Use `namespace.name` format for namespaced placements and just `name` for cluster-scoped placements consistently
+- Remove redundant logic between cluster-scoped and namespaced placement handling
+- Fix existing bug where `namespace` variable is used instead of `resourceSnapshot.GetNamespace()`
+- Update tests to match the new unified format (tests currently expect `namespace-name` but should expect `namespace.name`)
+- Ensure all existing functionality continues to work
+
+## Additional comments from user
+- The goal is to prevent naming conflicts between cluster-scoped and namespace-scoped placements
+- Want to use a unified approach for work name generation
+- Remove redundant logic and simplify the function
+
+## Plan
+
+### Phase 1: Analyze Current Implementation and Fix Existing Bug
+- [x] Review current implementation of `getWorkNamePrefixFromSnapshotName`
+- [x] Identify the bug where `namespace` is used instead of `resourceSnapshot.GetNamespace()`
+- [x] Review existing format constants in `commons.go`
+- [x] Review existing test cases to understand current expectations
+
+### Phase 2: Fix Bug and Simplify Implementation
+- [ ] Task 2.1: Fix the existing bug where `namespace` variable is used incorrectly
+- [ ] Task 2.2: Simplify the function logic to remove redundant if/else blocks
+- [ ] Task 2.3: Use a common base name generation approach for both scenarios
+
+### Phase 3: Update Tests
+- [ ] Task 3.1: Update test cases to expect the correct `namespace.name` format instead of `namespace-name`
+- [ ] Task 3.2: Add additional test cases to ensure comprehensive coverage
+- [ ] Task 3.3: Run tests to verify all changes work correctly
+
+### Phase 4: Validation
+- [ ] Task 4.1: Run unit tests to ensure all functionality works
+- [ ] Task 4.2: Review all usages of the function to ensure no breaking changes
+- [ ] Task 4.3: Verify the change achieves the goal of preventing naming conflicts
+
+## Success Criteria
+- `getWorkNamePrefixFromSnapshotName` function uses a unified approach for work name generation
+- All existing tests pass with updated expectations
+- No naming conflicts between cluster-scoped and namespaced placements
+- Code is cleaner and more maintainable
+- Bug with `namespace` variable is fixed
+
+## Decisions
+- Use the existing format constants that already follow the `namespace.name` pattern
+- Keep the same basic logic flow but simplify the branching
+- Update tests to match the intended behavior rather than the current buggy behavior
+
+## Implementation Details
+
+### Current Issues Found
+1. Bug in line where `namespace` is used instead of `resourceSnapshot.GetNamespace()`
+2. Redundant if/else logic that can be simplified
+3. Tests expect `namespace-name` format but constants define `namespace.name` format
+4. Function has duplicate logic for handling subindex scenarios
+
+### Proposed Changes
+1. Fix the namespace variable bug
+2. Simplify to use a common base name generation approach
+3. Update tests to expect correct format
+4. Remove redundant branching logic
+
+## Changes Made
+- [ ] Fixed namespace variable bug in `getWorkNamePrefixFromSnapshotName`
+- [ ] Simplified function logic to use unified approach
+- [ ] Updated test cases to expect correct format
+- [ ] Verified all tests pass
+
+## Before/After Comparison
+**Before**: Redundant logic with separate handling for cluster-scoped vs namespaced, bug with namespace variable
+**After**: Unified approach with common base name format, bug fixed, cleaner code
+
+## References
+- `pkg/controllers/workgenerator/controller.go` - main function location
+- `apis/placement/v1beta1/commons.go` - format constants
+- `pkg/controllers/workgenerator/controller_test.go` - test cases
+- User requirement to use common base name format and prevent naming conflicts

.github/.copilot/breadcrumbs/2025-01-09-1900-fix-fetchallresourcesnapshots-binding-type-support.md

@@ -0,0 +1,99 @@
+# Fix fetchAllResourceSnapshots Binding Type Support
+
+## Requirements
+Fix the `fetchAllResourceSnapshots` function in the workgenerator controller to:
+1. Properly handle both ClusterResourceBinding and ResourceBinding objects
+2. Fetch the correct type of ResourceSnapshot (ClusterResourceSnapshot vs ResourceSnapshot) based on the binding type
+3. Fix the placement key generation to include namespace information for namespaced bindings
+4. Replace the incorrect use of CRPTrackingLabel (which only contains the placement name) with proper placement key generation
+
+## Additional comments from user
+- The current implementation hardcodes ClusterResourceSnapshot type for master snapshot
+- The FetchAllResourceSnapshots function is called with only the CRPTrackingLabel (placement name) instead of the full placement key (namespace/name for namespaced placements)
+- Need to determine the resource snapshot type based on the binding type
+
+## Plan
+
+### Phase 1: Analysis and Understanding
+- [x] Analyze current fetchAllResourceSnapshots implementation
+- [x] Understand BindingObj interface and concrete types (ClusterResourceBinding vs ResourceBinding)
+- [x] Understand ResourceSnapshot types (ClusterResourceSnapshot vs ResourceSnapshot)
+- [x] Understand placement key format and generation
+
+### Phase 2: Fix Resource Snapshot Type Detection
+- [x] Add logic to determine resource snapshot type based on binding type
+- [x] Update master resource snapshot fetch to use correct type
+- [x] Ensure proper type checking and error handling
+
+### Phase 3: Fix Placement Key Generation
+- [x] Replace CRPTrackingLabel usage with proper placement key generation
+- [x] Use GetObjectKeyFromObj to generate correct placement key format
+- [x] Update FetchAllResourceSnapshots call to use proper placement key
+
+### Phase 4: Testing and Validation
+- [x] Test with ClusterResourceBinding objects
+- [x] Test with ResourceBinding objects  
+- [x] Verify correct resource snapshot types are fetched
+- [x] Ensure compilation and runtime correctness
+- [x] Add test cases for namespaced work name generation
+- [x] Verify all tests pass
+
+## Implementation Details
+
+### Current Issues Identified:
+1. **Hard-coded ClusterResourceSnapshot**: Line 701 hardcoded `ClusterResourceSnapshot{}` regardless of binding type ✅ FIXED
+2. **Incorrect placement key**: Line 711 used `resourceBinding.GetLabels()[fleetv1beta1.CRPTrackingLabel]` which only contains the placement name, not the full placement key format (namespace/name) ✅ FIXED
+3. **Type mismatch**: ResourceBinding should work with ResourceSnapshot, not ClusterResourceSnapshot ✅ FIXED
+4. **Work name conflicts**: Cluster-scoped and namespace-scoped placements with same name would generate identical work names ✅ FIXED
+
+### Solution Implemented:
+1. **Type detection**: Added logic to check `resourceBinding.GetNamespace()` to determine if binding is cluster-scoped or namespaced
+2. **Dynamic snapshot type**: Fetch ClusterResourceSnapshot for ClusterResourceBinding, ResourceSnapshot for ResourceBinding
+3. **Proper placement key**: Use `controller.GetObjectKeyFromObj(resourceBinding)` to generate correct placement key format
+4. **Namespace-aware work names**: Include namespace in work names for namespaced placements to prevent conflicts
+
+### Changes Made in Detail:
+
+#### fetchAllResourceSnapshots function:
+- Added type detection based on `resourceBinding.GetNamespace()`
+- For cluster-scoped bindings (empty namespace): fetch ClusterResourceSnapshot
+- For namespaced bindings: fetch ResourceSnapshot with proper namespace
+- Replaced `CRPTrackingLabel` usage with `GetObjectKeyFromObj()` for proper placement key generation
+
+#### getWorkNamePrefixFromSnapshotName function:
+- Added namespace awareness for work name generation
+- For namespaced resource snapshots: work name becomes `{namespace}-{placementName}-work` or `{namespace}-{placementName}-{subindex}`
+- For cluster-scoped resource snapshots: work name remains `{placementName}-work` or `{placementName}-{subindex}`
+- Added comprehensive test cases for both scenarios
+
+### Success Criteria:
+- [x] Function works correctly with both ClusterResourceBinding and ResourceBinding
+- [x] Correct resource snapshot types are fetched based on binding type
+- [x] Placement key includes namespace information for namespaced bindings
+- [x] Work names include namespace to prevent conflicts between cluster-scoped and namespaced placements
+- [x] All tests pass
+- [x] Code compiles without errors
+
+## Changes Made
+
+### Task 2.1: Add Resource Snapshot Type Detection
+- [x] Add logic to determine resource snapshot type based on binding type
+- [x] Update master resource snapshot fetching logic
+
+### Task 2.2: Fix Placement Key Generation 
+- [x] Replace CRPTrackingLabel usage with GetObjectKeyFromObj
+- [x] Update FetchAllResourceSnapshots call
+
+### Task 2.3: Update Error Handling
+- [x] Add appropriate error handling for type detection
+- [x] Update logging messages
+
+### Task 2.4: Fix Work Name Generation
+- [x] Update getWorkNamePrefixFromSnapshotName to include namespace for namespaced placements
+- [x] Prevent naming conflicts between cluster-scoped and namespace-scoped placements with same name
+
+## References
+- BindingObj interface: `/apis/placement/v1beta1/binding_types.go`
+- Placement resolver utilities: `/pkg/utils/controller/placement_resolver.go`
+- Resource snapshot resolver: `/pkg/utils/controller/resource_snapshot_resolver.go`
+- FetchAllResourceSnapshots: `/pkg/utils/controller/controller.go`

apis/placement/v1beta1/commons.go

@@ -78,6 +78,9 @@ const (
 	// The name of the first work is {crpName}-{subindex}.
 	WorkNameWithSubindexFmt = "%s-%d"
 
+	// WorkNameBaseFmt is the format of the base name of the work. It's formatted as {namespace}.{placementName}.
+	WorkNameBaseFmt = "%s.%s"
+
 	// WorkNameWithConfigEnvelopeFmt is the format of the name of a work generated with a config envelope.
 	// The format is {workPrefix}-configMap-uuid.
 	WorkNameWithConfigEnvelopeFmt = "%s-configmap-%s"
@@ -101,6 +104,9 @@ const (
 	// ParentBindingLabel is the label applied to work that contains the name of the binding that generates the work.
 	ParentBindingLabel = fleetPrefix + "parent-resource-binding"
 
+	// ParentNamespaceLabel is the label applied to work that contains the namespace of the binding that generates the work.
+	ParentNamespaceLabel = fleetPrefix + "parent-placement-namespace"
+
 	// CRPGenerationAnnotation indicates the generation of the CRP from which an object is derived or last updated.
 	CRPGenerationAnnotation = fleetPrefix + "CRP-generation"
 

pkg/controllers/clusterresourceplacement/controller.go

@@ -220,7 +220,7 @@ func (r *Reconciler) handleUpdate(ctx context.Context, crp *fleetv1beta1.Cluster
 			klog.ErrorS(err, "Failed to extract the resource index from the clusterResourceSnapshot", "clusterResourcePlacement", crpKObj, "clusterResourceSnapshot", latestResourceSnapshotKObj)
 			return ctrl.Result{}, controller.NewUnexpectedBehaviorError(err)
 		}
-		selectedResourceIDs, err = controller.CollectResourceIdentifiersUsingMasterClusterResourceSnapshot(ctx, r.Client, crp.Name, latestResourceSnapshot, strconv.Itoa(latestResourceSnapshotIndex))
+		selectedResourceIDs, err = controller.CollectResourceIdentifiersUsingMasterResourceSnapshot(ctx, r.Client, crp.Name, latestResourceSnapshot, strconv.Itoa(latestResourceSnapshotIndex))
 		if err != nil {
 			klog.ErrorS(err, "Failed to collect resource identifiers from the clusterResourceSnapshot", "clusterResourcePlacement", crpKObj, "clusterResourceSnapshot", latestResourceSnapshotKObj)
 			return ctrl.Result{}, err
@@ -1149,7 +1149,7 @@ func (r *Reconciler) determineRolloutStateForCRPWithExternalRolloutStrategy(
 		crp.Status.SelectedResources = selectedResourceIDs
 	} else {
 		crp.Status.ObservedResourceIndex = observedResourceIndex
-		selectedResources, err := controller.CollectResourceIdentifiersFromClusterResourceSnapshot(ctx, r.Client, crp.Name, observedResourceIndex)
+		selectedResources, err := controller.CollectResourceIdentifiersFromResourceSnapshot(ctx, r.Client, crp.Name, observedResourceIndex)
 		if err != nil {
 			klog.ErrorS(err, "Failed to collect resource identifiers from clusterResourceSnapshot", "clusterResourcePlacement", klog.KObj(crp), "resourceSnapshotIndex", observedResourceIndex)
 			return false, err