Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
8934209
interface: Add v1 Override APIs, CRDs (#146)
Arvindthiru Jul 29, 2025
4aedc78
feat: add crp mutating webhook & tests (#120)
britaniar Jul 30, 2025
df041ef
feat: refactor clusterresourceplacementwatcher to support rp (#149)
zhiying-lin Jul 31, 2025
6604ed5
feat: support resourceBinding watcher (#144)
zhiying-lin Jul 31, 2025
4e66856
feat: refactor the scheduler clusterresourcebinding watcher to suppor…
zhiying-lin Jul 31, 2025
2aaa087
interface: add placementScope in override API (#159)
jwtty Jul 31, 2025
4d6e6d7
feat: refactor CRP controller to work on both CRP and RP (#137)
ryanzhang-oss Aug 1, 2025
c8b3a86
feat: refactor crp watcher for the scheduler (#157)
zhiying-lin Aug 1, 2025
c349d65
feat: support rp in clusterSchedulingPolicySnapshot controller (#160)
jwtty Aug 1, 2025
b5a8b79
feat: create kubectl-fleet plugin (#154)
jwtty Aug 2, 2025
f153ee1
feat: Add RP condition types and change the internal per cluster cond…
ryanzhang-oss Aug 4, 2025
b7ed047
feat: refactor the triggerAffectedPlacementsForUpdatedClusterRes to h…
zhiying-lin Aug 4, 2025
570b44c
feat: rename the conditions so they are not confusing (#165)
ryanzhang-oss Aug 4, 2025
d431faa
interface: add support select ns only for CRP (#163)
zhiying-lin Aug 6, 2025
c06ca95
feat: refactor rollout controller for RP (#158)
jwtty Aug 6, 2025
8f12258
feat: enable resource change detector for RP (#169)
jwtty Aug 6, 2025
845ed65
feat: refactor policy snapshot watcher for the scheduler (#164)
zhiying-lin Aug 6, 2025
dee9f57
feat: add CEL validation for CRO placement scope (#168)
jwtty Aug 6, 2025
24f16b5
interface: add status proxy for namespace scoped access only API (#161)
britaniar Aug 6, 2025
121b571
test: add more tests to the library for RP cases (#166)
ryanzhang-oss Aug 6, 2025
266cdd2
feat: refactor resource change controller to handle deleted resource …
zhiying-lin Aug 7, 2025
74f2bd0
feat: add CEL validation for NamespaceAccessible StatusReportingScope…
Arvindthiru Aug 8, 2025
3540536
test: add rollout controller integration tests for RP (#175)
jwtty Aug 8, 2025
fa16f5e
feat: update resource change controller to support ns only for CRP (#…
zhiying-lin Aug 8, 2025
df94676
feat: refactor work generator to support RP (#173)
zhiying-lin Aug 8, 2025
67d877d
feat: make CRP controller handle RP status (#170)
ryanzhang-oss Aug 11, 2025
5b35631
fix: CVE-2025-47907 (#177)
ryanzhang-oss Aug 11, 2025
b2759a6
Merge remote-tracking branch 'cncf/main' into backport-08-11
Aug 11, 2025
67940ba
fix crdInstaller UT
Aug 11, 2025
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
# Extend getOrCreateClusterResourceSnapshot to Handle Both Snapshot Types

## Context

Following successful interface refactoring and deletion function optimizations, user requested: "can you modify the function getOrCreateClusterResourceSnapshot to handle both clusterresourcesnapshot and resourcesnapshot?"

## Problem

The `getOrCreateClusterResourceSnapshot` function in `/Users/ryanzhang/Workspace/github/kubefleet/pkg/controllers/clusterresourceplacement/controller.go` (lines 468-650) currently only handles `ClusterResourceSnapshot` (cluster-scoped) resources. We need to extend it to also handle `ResourceSnapshot` (namespace-scoped) resources using the common `ResourceSnapshotObj` interface.

## Research Findings

### ResourceSnapshotObj Interface Structure
```go
type ResourceSnapshotObj interface {
apis.ConditionedObj
ResourceSnapshotSpecGetterSetter
ResourceSnapshotStatusGetterSetter
}

type ResourceSnapshotSpecGetterSetter interface {
GetResourceSnapshotSpec() *ResourceSnapshotSpec
SetResourceSnapshotSpec(ResourceSnapshotSpec)
}

type ResourceSnapshotStatusGetterSetter interface {
GetResourceSnapshotStatus() *ResourceSnapshotStatus
SetResourceSnapshotStatus(ResourceSnapshotStatus)
}
```

### Implementation Types
- **ClusterResourceSnapshot** (cluster-scoped, v1beta1) - implements ResourceSnapshotObj
- **ResourceSnapshot** (namespace-scoped, v1beta1) - implements ResourceSnapshotObj

Both types share:
- Same `ResourceSnapshotSpec` structure
- Same interface methods for spec/status access
- Same labeling and annotation patterns
- Different scoping (cluster vs namespace)

### Pattern Precedent
The delete functions already follow this pattern:
- `deleteClusterSchedulingPolicySnapshots` handles both ClusterSchedulingPolicySnapshot and SchedulingPolicySnapshot
- `deleteResourceSnapshots` uses `DeleteAllOf` with namespace awareness

## Plan

### Phase 1: Function Signature Updates
- [x] Task 1.1: Change function signature to accept `PlacementObj` interface instead of concrete type
- [x] Task 1.2: Update return type to use `ResourceSnapshotObj` interface
- [x] Task 1.3: Update function name to `getOrCreateResourceSnapshot` for clarity

### Phase 2: Namespace Detection Logic
- [x] Task 2.1: Add helper function to determine if placement is namespace-scoped
- [x] Task 2.2: Implement logic to choose between ClusterResourceSnapshot and ResourceSnapshot based on placement scope

### Phase 3: Snapshot Creation Logic
- [ ] Task 3.1: Extend `buildMasterClusterResourceSnapshot` to handle both types
- [ ] Task 3.2: Extend `buildSubIndexResourceSnapshot` to handle both types
- [ ] Task 3.3: Update snapshot creation logic to use proper type based on scope

### Phase 4: Helper Functions Updates
- [ ] Task 4.1: Update `lookupLatestResourceSnapshot` to handle both types
- [ ] Task 4.2: Update `listSortedResourceSnapshots` to handle both types
- [ ] Task 4.3: Update `createResourceSnapshot` to handle both types

### Phase 5: Testing and Validation
- [ ] Task 5.1: Run existing unit tests to ensure no regression
- [ ] Task 5.2: Verify function works with both cluster-scoped and namespace-scoped placements
- [ ] Task 5.3: Ensure interface compatibility throughout the codebase

## Success Criteria
- Function accepts both ClusterResourcePlacement and ResourcePlacement
- Creates appropriate snapshot type based on placement scope
- Maintains all existing functionality for cluster-scoped resources
- Uses ResourceSnapshotObj interface throughout
- No breaking changes to existing API
- All tests pass

## Implementation Notes
- Follow established namespace-aware pattern from delete functions
- Use ResourceSnapshotObj interface consistently
- Maintain backward compatibility
- Apply same labeling/annotation patterns for both types
Original file line number Diff line number Diff line change
@@ -0,0 +1,223 @@
# ClusterResourcePlacement Controller Interface Refactor

## Requirements

Refactor the ClusterResourcePlacement controller (`pkg/controllers/clusterresourceplacement/controller.go`) to use interface objects instead of concrete types like ClusterResourcePlacement. Instead, use PlacementObj. The controller should use interface types like BindingObj, ResourceSnapshotObj, and PolicySnapshotObj throughout instead of concrete implementations.

**Specific Requirements**:
- Replace all function signatures that use `*fleetv1beta1.ClusterResourcePlacement` with `fleetv1beta1.PlacementObj`
- Replace all concrete types like `*fleetv1beta1.ClusterResourceBinding`, `*fleetv1beta1.ClusterResourceSnapshot`, `*fleetv1beta1.ClusterSchedulingPolicySnapshot` with their respective interface types: `BindingObj`, `ResourceSnapshotObj`, `PolicySnapshotObj`
- Use helper functions defined in `pkg/utils/controller` when need to get the concrete type from the clusters
- Update integration and unit tests to work with interface types
- Follow the patterns established in previous breadcrumb refactoring work, particularly `2025-06-24-0900-rollout-controller-binding-interface-refactor.md`

## Additional comments from user

This follows the breadcrumb protocol and builds on previous successful interface refactoring work. The goal is to make the controller work with both cluster-scoped and namespace-scoped placement objects through unified interfaces.

## Plan

### Phase 1: Analysis and Preparation
- [x] Task 1.1: Analyze current concrete type usage patterns in the controller.go
- [x] Task 1.2: Identify all functions that need signature updates for PlacementObj interface
- [x] Task 1.3: Review available interface methods (PlacementObj, BindingObj, ResourceSnapshotObj, PolicySnapshotObj)
- [x] Task 1.4: Examine helper functions in pkg/utils/controller for fetching concrete types
- [x] Task 1.5: Identify test files that will need updates

### Phase 2: Core Reconciler Function Updates
- [x] Task 2.1: Update `Reconcile` function to use PlacementObj interface ✅
- [x] Task 2.2: Update `handleDelete` function signature to use PlacementObj interface ✅
- [x] Task 2.3: Update `handleUpdate` function signature to use PlacementObj interface ✅
- [x] Task 2.4: Update placement fetch logic to use helper functions from pkg/utils/controller ✅

### Functions Updated So Far ✅
- `Reconcile()` - Now uses `FetchPlacementFromKey()` and works with `PlacementObj`
- `handleDelete()` - Updated to use `PlacementObj` interface
- `deleteClusterSchedulingPolicySnapshots()` - Updated to use `PlacementObj` interface
- `deleteClusterResourceSnapshots()` - Updated to use `PlacementObj` interface
- `selectResourcesForPlacement()` - Updated to use `PlacementObj` interface
- `emitPlacementStatusMetric()` - Updated to use `PlacementObj` interface
- `determineExpectedCRPAndResourcePlacementStatusCondType()` - Updated to use `PlacementObj` interface
- `handleUpdate()` - COMPLETED ✅ Updated to use `PlacementObj` interface with type assertions for snapshot management
- `getOrCreateClusterSchedulingPolicySnapshot()` - Updated to accept `PlacementObj` interface
- `setPlacementStatus()` - Updated to use `PlacementObj` interface

### Phase 3: Update Remaining Utility Functions ✅
- [x] Task 3.1: Update `isRolloutCompleted()` function to use PlacementObj interface ✅
- [x] Task 3.2: Update `isCRPScheduled()` function to use PlacementObj interface ✅
- [x] Task 3.3: Update `buildScheduledCondition()` function to use PlacementObj interface ✅
- [x] Task 3.4: Update `buildResourcePlacementStatusMap()` function to use PlacementObj interface ✅
- [x] Task 3.5: Update `calculateFailedToScheduleClusterCount()` function to use PlacementObj interface ✅

### Additional Functions Updated in Phase 3 ✅
- `isRolloutCompleted()` - Now uses `PlacementObj` interface with `GetCondition()` and `GetGeneration()`
- `isCRPScheduled()` - Now uses `PlacementObj` interface with `GetCondition()` and `GetGeneration()`
- `buildScheduledCondition()` - Now uses `PlacementObj` interface with `GetGeneration()`
- `buildResourcePlacementStatusMap()` - Now uses `PlacementObj` interface with `GetPlacementStatus()`
- `calculateFailedToScheduleClusterCount()` - Now uses `PlacementObj` interface with `GetPlacementSpec()`

### Interface Pattern Successfully Established 🎯
✅ **No Type Assertions Needed**: All functions now work with `PlacementObj` interface
✅ **Automatic Interface Satisfaction**: Concrete types (`*ClusterResourcePlacement`) automatically satisfy the interface
✅ **Clean Separation**: Interface methods (`GetPlacementSpec()`, `GetPlacementStatus()`, `GetCondition()`, `GetGeneration()`) used throughout
✅ **Future Ready**: Ready to support namespace-scoped placements when they implement the same interface

### Phase 4: Testing and Validation ✅
- [x] Task 4.1: Verify unit tests still pass after interface refactoring ✅
- [x] Task 4.2: Verify specific refactored functions work correctly ✅
- [x] Task 4.3: Ensure no compilation errors ✅
- [x] Task 4.4: Validate core functionality maintained ✅

### Testing Results ✅
**Unit Tests**: All existing unit tests continue to pass
- ✅ `TestBuildResourcePlacementStatusMap` - Passed all sub-tests
- ✅ `TestGetOrCreateClusterSchedulingPolicySnapshot` - Passed all cases
- ✅ `TestGetOrCreateClusterResourceSnapshot` - Passed all cases
- ✅ General controller tests - All passing

**Compilation**: Clean compilation with no errors
- ✅ `go build ./pkg/controllers/clusterresourceplacement/` - Success
- ✅ `go test -run ^$` - Compilation-only test passed

**Interface Integration**: All refactored functions work correctly with interfaces
- ✅ Functions automatically accept concrete types via interface satisfaction
- ✅ No type assertion issues in main business logic
- ✅ Error handling preserved and working correctly

### Phase 5: Refactor placement_status.go Functions ⚠️
- [ ] Task 5.1: Update `appendFailedToScheduleResourcePlacementStatuses()` to use PlacementObj interface
- [ ] Task 5.2: Update `appendScheduledResourcePlacementStatuses()` to use PlacementObj and interface types
- [ ] Task 5.3: Update `buildClusterResourceBindings()` to use PlacementObj and return BindingObj interfaces
- [ ] Task 5.4: Update `findClusterResourceSnapshotIndexForBindings()` to use interface types
- [ ] Task 5.5: Update `setResourcePlacementStatusPerCluster()` to use interface types
- [ ] Task 5.6: Update `setResourcePlacementStatusBasedOnBinding()` to use interface types

### Functions to Refactor in placement_status.go 🎯
**Current concrete type usage identified:**
- `appendFailedToScheduleResourcePlacementStatuses()` - Takes `*ClusterResourcePlacement`
- `appendScheduledResourcePlacementStatuses()` - Takes `*ClusterResourcePlacement`, `*ClusterSchedulingPolicySnapshot`, `*ClusterResourceSnapshot`
- `buildClusterResourceBindings()` - Takes `*ClusterResourcePlacement`, `*ClusterSchedulingPolicySnapshot`, returns `map[string]*ClusterResourceBinding`
- `findClusterResourceSnapshotIndexForBindings()` - Takes `*ClusterResourcePlacement`, `map[string]*ClusterResourceBinding`
- `setResourcePlacementStatusPerCluster()` - Takes `*ClusterResourcePlacement`, `*ClusterResourceSnapshot`, `*ClusterResourceBinding`
- `setResourcePlacementStatusBasedOnBinding()` - Takes `*ClusterResourcePlacement`, `*ClusterResourceBinding`

### Phase 3: Snapshot Management Function Updates
- [ ] Task 3.1: Update `getOrCreateClusterSchedulingPolicySnapshot` to return PolicySnapshotObj
- [ ] Task 3.2: Update `getOrCreateClusterResourceSnapshot` to return ResourceSnapshotObj
- [ ] Task 3.3: Update all snapshot listing and deletion functions to use interface types
- [ ] Task 3.4: Update snapshot creation and management helper functions

### Phase 4: Status and Condition Management Updates
- [ ] Task 4.1: Update `setPlacementStatus` function to use PlacementObj interface
- [ ] Task 4.2: Update condition building functions to use interface types
- [ ] Task 4.3: Update resource placement status calculation functions

### Phase 5: Builder and Utility Function Updates
- [ ] Task 5.1: Update snapshot builder functions to return interface types
- [ ] Task 5.2: Update parsing and utility functions to work with interface types
- [ ] Task 5.3: Update all logging references to use interface objects

### Phase 6: Test Updates
- [ ] Task 6.1: Update unit tests in the same package to use interface types
- [ ] Task 6.2: Update integration tests to use interface types
- [ ] Task 6.3: Run tests to ensure all interface usage is correct

### Phase 7: Interface Method Integration
- [ ] Task 7.1: Replace direct field access with interface methods where applicable
- [ ] Task 7.2: Ensure proper use of `GetPlacementSpec()`, `SetPlacementSpec()` methods
- [ ] Task 7.3: Verify all interface implementations are working correctly

## Analysis

### Current State Analysis ✅

Based on the grep search, identified 20+ functions that need refactoring:

**Main reconciler functions**:
- `handleDelete(ctx context.Context, crp *fleetv1beta1.ClusterResourcePlacement)`
- `handleUpdate(ctx context.Context, crp *fleetv1beta1.ClusterResourcePlacement)`

**Snapshot management functions**:
- `getOrCreateClusterSchedulingPolicySnapshot(ctx context.Context, crp *fleetv1beta1.ClusterResourcePlacement, revisionHistoryLimit int) (*fleetv1beta1.ClusterSchedulingPolicySnapshot, error)`
- `getOrCreateClusterResourceSnapshot(ctx context.Context, crp *fleetv1beta1.ClusterResourcePlacement, envelopeObjCount int, resourceSnapshotSpec *fleetv1beta1.ResourceSnapshotSpec, revisionHistoryLimit int) (ctrl.Result, *fleetv1beta1.ClusterResourceSnapshot, error)`

**Builder functions**:
- `buildMasterClusterResourceSnapshot(...) *fleetv1beta1.ClusterResourceSnapshot`
- `buildSubIndexResourceSnapshot(...) *fleetv1beta1.ClusterResourceSnapshot`

**Status and condition functions**:
- `buildScheduledCondition(crp *fleetv1beta1.ClusterResourcePlacement, latestSchedulingPolicySnapshot *fleetv1beta1.ClusterSchedulingPolicySnapshot) metav1.Condition`
- `buildResourcePlacementStatusMap(crp *fleetv1beta1.ClusterResourcePlacement) map[string][]metav1.Condition`

### Interface Patterns Available ✅

**PlacementObj interface** provides:
- `GetPlacementSpec() *ResourcePlacementSpec`
- `SetPlacementSpec(*ResourcePlacementSpec)`
- `GetPlacementStatus() *ResourcePlacementStatus`
- `SetPlacementStatus(*ResourcePlacementStatus)`

**ResourceSnapshotObj interface** provides:
- `GetResourceSnapshotSpec() *ResourceSnapshotSpec`
- `SetResourceSnapshotSpec(*ResourceSnapshotSpec)`
- `GetResourceSnapshotStatus() *ResourceSnapshotStatus`
- `SetResourceSnapshotStatus(*ResourceSnapshotStatus)`

**PolicySnapshotObj interface** provides:
- `GetPolicySnapshotSpec() *SchedulingPolicySnapshotSpec`
- `SetPolicySnapshotSpec(*SchedulingPolicySnapshotSpec)`
- `GetPolicySnapshotStatus() *SchedulingPolicySnapshotStatus`
- `SetPolicySnapshotStatus(*SchedulingPolicySnapshotStatus)`

### Helper Functions Available ✅

From `pkg/utils/controller`:
- `FetchPlacementFromKey(ctx context.Context, c client.Reader, placementKey queue.PlacementKey) (fleetv1beta1.PlacementObj, error)`
- `FetchLatestMasterResourceSnapshot(ctx context.Context, k8Client client.Reader, placementKey types.NamespacedName) (fleetv1beta1.ResourceSnapshotObj, error)`
- Various resolver and conversion helper functions

## Decisions

### Refactoring Strategy
1. Follow the exact same pattern as the rollout controller refactor in breadcrumb `2025-06-24-0900-rollout-controller-binding-interface-refactor.md`
2. Replace concrete types with interfaces in function signatures
3. Use interface methods instead of direct field access
4. Keep type assertions only at system boundaries (controller-runtime event handling)
5. Use helper functions from `pkg/utils/controller` for fetching concrete types when needed

### Implementation Order
1. Start with the main reconciler entry point and work downward
2. Update function signatures first, then implement interface method usage
3. Handle tests after core functionality is complete
4. Follow the principle of minimal changes - avoid large complex refactoring

## Implementation Status

### Analysis Complete ✅
- Identified all functions requiring refactoring
- Confirmed available interface types and methods
- Located helper functions for type conversion
- Planned implementation strategy based on successful previous patterns

### Key Understanding ✅

**Current Controller Logic**:
1. `Reconcile()` function assumes key is a simple string (cluster-scoped name only)
2. Directly fetches `ClusterResourcePlacement` using `types.NamespacedName{Name: name}`
3. Uses concrete types throughout all functions (`*fleetv1beta1.ClusterResourcePlacement`, `*fleetv1beta1.ClusterResourceSnapshot`, etc.)
4. All business logic operates on concrete types

**Target Interface Pattern**:
1. `Reconcile()` should accept placement keys that include namespace/name format
2. Use `FetchPlacementFromKey()` helper to get appropriate placement type (cluster-scoped or namespaced)
3. Work with `PlacementObj` interface throughout business logic
4. Use interface methods like `GetPlacementSpec()`, `SetPlacementSpec()` instead of direct field access
5. Keep type assertions only at system boundaries (controller-runtime interactions)

**Critical Functions That Need Interface Refactoring**:
- `Reconcile()` - entry point, needs to use placement resolver
- `handleDelete()` / `handleUpdate()` - core business logic, use PlacementObj
- `selectResourcesForPlacement()` - resource selection, use PlacementObj
- `getOrCreateClusterSchedulingPolicySnapshot()` - return PolicySnapshotObj
- `getOrCreateClusterResourceSnapshot()` - return ResourceSnapshotObj
- `setPlacementStatus()` - status management, use PlacementObj + interface snapshots
Loading
Loading