Skip to content

Commit a294ce8

Browse files
ryanzhang-ossryanzhang-oss
committed
refactor
Signed-off-by: Ryan Zhang <[email protected]>
1 parent 49671bf commit a294ce8

File tree

16 files changed

+1099
-237
lines changed

16 files changed

+1099
-237
lines changed

‎.github/.copilot/breadcrumbs/2025-01-22-1430-scheduler-placement-obj-refactor.md‎

Lines changed: 232 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,232 @@
1+
# Scheduler PlacementObj Interface Refactor
2+
3+
## Requirements
4+
5+
- Refactor the scheduler to use a PlacementObj interface instead of the concrete ClusterResourcePlacement type
6+
- Support both ClusterResourcePlacement (cluster-scoped) and ResourcePlacement (namespaced) types through the interface
7+
- Utilize domain knowledge about cluster vs namespaced resources naming conventions
8+
- Maintain existing functionality while making the scheduler more flexible
9+
- Update all scheduler methods to work with the interface abstraction
10+
11+
## Additional comments from user
12+
13+
User wants to refactor the scheduler to use PlacementObj interface instead of ClusterResourcePlacement and use the domain knowledge shared in cluster vs namespaced resources.
14+
15+
**UPDATE**: User wants to keep using `SchedulerCRPCleanupFinalizer` consistently in the scheduler.go file instead of the dynamic finalizer selection logic that was implemented. This means reverting the finalizer logic to always use `SchedulerCRPCleanupFinalizer` for both cluster-scoped and namespaced placements.
16+
17+
**UPDATE 2**: User wants to use `PolicySnapshotObj` interface to replace all direct references of `ClusterSchedulingPolicySnapshot` in the scheduler directory, following the same pattern as the PlacementObj interface refactor. User also wants to ensure we use `PolicySnapshotList` interface for all listing operations.
18+
19+
## Plan
20+
21+
### Phase 1: Interface Definition and Analysis
22+
- [x] Task 1.1: Examine domain knowledge about cluster vs namespaced resources
23+
- [x] Task 1.2: Review existing ClusterResourcePlacement and ResourcePlacement type definitions
24+
- [x] Task 1.3: PlacementObj interface already exists in apis/interface.go - will use existing interface
25+
- [x] Task 1.4: Interface tests not needed - already exists and tested
26+
27+
### Phase 2: PlacementKey Resolution ✅ **COMPLETED**
28+
- [x] Task 2.1: Create utility functions to resolve PlacementKey to concrete placement objects
29+
- [x] Task 2.2: Handle both cluster-scoped and namespaced placement object retrieval
30+
- [x] Task 2.3: Update queue integration to work with both placement types
31+
32+
### Phase 3: Scheduler Core Refactoring ✅ **COMPLETED**
33+
- [x] Task 3.1: Find all direct references to ClusterResourcePlacement in scheduler code
34+
- [x] Task 3.2: Refactor scheduleOnce method to use PlacementObj
35+
- [x] Task 3.3: Update cleanUpAllBindingsFor method for interface support
36+
- [x] Task 3.4: Modify helper methods (lookupLatestPolicySnapshot, addSchedulerCleanUpFinalizer)
37+
- [x] Task 3.5: Update scheduler tests to work with new interface signatures
38+
39+
### Phase 4: Testing and Validation
40+
- [x] Task 4.1: Write unit tests for refactored scheduler methods
41+
- [x] Task 4.2: Update existing tests to work with PlacementObj interface
42+
- [x] Task 4.3: Run tests to ensure no regressions
43+
- [x] Task 4.4: Validate that ClusterResourcePlacement work
44+
45+
### Phase 5: Documentation and Cleanup
46+
- [x] Task 5.1: Update code comments and documentation
47+
- [x] Task 5.2: Run goimports and code formatting
48+
- [x] Task 5.3: Run golangci-lint and go vet for validation
49+
50+
### Phase 6: Finalizer Consistency Update ✅ **COMPLETED**
51+
- [x] Task 6.1: Update getCleanupFinalizerForPlacement to always return SchedulerCRPCleanupFinalizer
52+
- [x] Task 6.2: Simplify cleanUpAllBindingsFor to use SchedulerCRPCleanupFinalizer consistently
53+
- [x] Task 6.3: Update TestGetCleanupFinalizerForPlacement test to expect SchedulerCRPCleanupFinalizer for both placement types
54+
- [x] Task 6.4: Remove getCleanupFinalizerForPlacement function entirely and use SchedulerCRPCleanupFinalizer directly
55+
- [x] Task 6.5: Remove TestGetCleanupFinalizerForPlacement test since the function no longer exists
56+
- [x] Task 6.6: Update all method calls to use the finalizer constant directly
57+
- [x] Task 6.4: Update TestAddSchedulerCleanUpFinalizerWithInterface test to expect SchedulerCRPCleanupFinalizer for both placement types
58+
- [x] Task 6.5: Update TestCleanUpAllBindingsForWithInterface test to use SchedulerCRPCleanupFinalizer for both test cases
59+
- [x] Task 6.6: Run all scheduler tests to ensure no regressions
60+
- [x] Task 6.7: Run placement resolver tests to ensure compatibility
61+
- [x] Task 6.8: Run code formatting and validation (go fmt, go vet)
62+
63+
### Phase 7: PolicySnapshotObj Interface Refactor
64+
- [x] Task 7.1: Find all direct references to ClusterSchedulingPolicySnapshot in scheduler code
65+
- [x] Task 7.2: Examine PolicySnapshotObj interface and understand its capabilities
66+
- [x] Task 7.3: Create utility functions to handle both ClusterSchedulingPolicySnapshot and SchedulingPolicySnapshot
67+
- [ ] Task 7.4: Update scheduler methods to use PolicySnapshotObj interface
68+
- [ ] Task 7.5: Update any list operations to use PolicySnapshotList interface
69+
- [ ] Task 7.6: Update tests to work with PolicySnapshotObj interface
70+
- [ ] Task 7.7: Run tests to ensure no regressions
71+
- [ ] Task 7.8: Run code formatting and validation
72+
73+
## Decisions
74+
75+
- Will use the existing PlacementObj interface from apis/interface.go which includes client.Object, PlacementSpec, and PlacementStatus interfaces
76+
- PlacementKey naming convention will be used to determine placement type (cluster vs namespaced)
77+
- Type assertions shall be avoided as much as possible and refactored into utility functions
78+
- Scheduler methods will be updated to accept PlacementObj interface instead of concrete types
79+
- Backward compatibility will be maintained
80+
- The existing interface already provides all necessary methods: GetPlacementSpec(), SetPlacementSpec(), GetPlacementStatus(), SetPlacementStatus()
81+
82+
## Implementation Details
83+
84+
### Core Scheduler Refactoring
85+
86+
The scheduler has been successfully refactored to use the `PlacementObj` interface instead of the concrete `ClusterResourcePlacement` type. Key changes include:
87+
88+
#### 1. Placement Key Resolution Utilities
89+
90+
**File: `pkg/scheduler/placement_resolver.go`**
91+
- `FetchPlacementFromKey()`: Resolves a PlacementKey to a concrete placement object (ClusterResourcePlacement or ResourcePlacement)
92+
- `GetPlacementKeyFromObj()`: Generates a PlacementKey from a placement object
93+
- Handles namespace separator logic: cluster-scoped placements use name only, namespaced placements use "namespace/name" format
94+
95+
```go
96+
// Example: Resolving placement from key
97+
func FetchPlacementFromKey(ctx context.Context, c client.Client, placementKey queue.PlacementKey) (apis.PlacementObj, error) {
98+
keyStr := string(placementKey)
99+
100+
if strings.Contains(keyStr, namespaceSeparator) {
101+
// Handle namespaced ResourcePlacement
102+
parts := strings.SplitN(keyStr, namespaceSeparator, 2)
103+
// ... retrieve ResourcePlacement
104+
} else {
105+
// Handle cluster-scoped ClusterResourcePlacement
106+
// ... retrieve ClusterResourcePlacement
107+
}
108+
}
109+
```
110+
111+
#### 2. Scheduler Method Updates
112+
113+
**File: `pkg/scheduler/scheduler.go`**
114+
115+
Key methods updated to work with `apis.PlacementObj` interface:
116+
117+
- **`scheduleOnce()`**: Now retrieves placement using `FetchPlacementFromKey()` and works with the interface
118+
- **`cleanUpAllBindingsFor()`**: Accepts `apis.PlacementObj` and uses `GetPlacementKeyFromObj()` for binding lookup
119+
- **`lookupLatestPolicySnapshot()`**: Works with placement interface using placement key resolution
120+
- **`addSchedulerCleanUpFinalizer()`**: Uses helper method to determine correct finalizer based on placement type
121+
- **`getCleanupFinalizerForPlacement()`**: New utility method that returns appropriate finalizer based on namespace presence
122+
123+
```go
124+
// Example: Dynamic finalizer selection
125+
func (s *Scheduler) getCleanupFinalizerForPlacement(placement apis.PlacementObj) string {
126+
if placement.GetNamespace() == "" {
127+
return fleetv1beta1.SchedulerCRPCleanupFinalizer // Cluster-scoped
128+
}
129+
return fleetv1beta1.PlacementCleanupFinalizer // Namespaced
130+
}
131+
```
132+
133+
#### 3. Interface Usage Pattern
134+
135+
The refactor follows a consistent pattern:
136+
1. **Type Detection**: Use `placement.GetNamespace() == ""` to distinguish between cluster-scoped and namespaced placements
137+
2. **Key Generation**: Use `GetPlacementKeyFromObj()` to generate consistent placement keys
138+
3. **Finalizer Selection**: Use `getCleanupFinalizerForPlacement()` to select appropriate finalizers
139+
4. **Binding Tracking**: Use placement key for CRPTrackingLabel values
140+
141+
#### 4. Test Coverage
142+
143+
**File: `pkg/scheduler/placement_resolver_test.go`**
144+
- Unit tests for both placement key resolution functions
145+
- Coverage for both ClusterResourcePlacement and ResourcePlacement scenarios
146+
- Error handling tests for non-existent placements
147+
148+
**File: `pkg/scheduler/scheduler_test.go`**
149+
- Updated existing tests to work with PlacementObj interface
150+
- Added interface-specific test cases (`TestCleanUpAllBindingsForWithInterface`, `TestAddSchedulerCleanUpFinalizerWithInterface`, etc.)
151+
- Tests verify correct behavior for both placement types
152+
153+
## Changes Made
154+
155+
### Core Files Modified
156+
157+
#### 1. **pkg/scheduler/placement_resolver.go** (NEW FILE)
158+
- Added `FetchPlacementFromKey()` function to resolve PlacementKey to concrete placement objects
159+
- Added `GetPlacementKeyFromObj()` function to generate PlacementKey from placement objects
160+
- Implements namespace separator logic (`"/"`) to distinguish cluster vs namespaced placements
161+
- Handles both ClusterResourcePlacement and ResourcePlacement retrieval
162+
163+
#### 2. **pkg/scheduler/scheduler.go** (REFACTORED)
164+
- **`scheduleOnce()`**: Updated to use `FetchPlacementFromKey()` instead of direct ClusterResourcePlacement access
165+
- **`cleanUpAllBindingsFor()`**: Modified to accept `apis.PlacementObj` interface and use `GetPlacementKeyFromObj()`
166+
- **`lookupLatestPolicySnapshot()`**: Updated to work with PlacementObj interface using placement key resolution
167+
- **`addSchedulerCleanUpFinalizer()`**: Modified to use `getCleanupFinalizerForPlacement()` helper
168+
- **`getCleanupFinalizerForPlacement()`**: NEW - Returns appropriate finalizer based on placement type
169+
170+
#### 3. **pkg/scheduler/placement_resolver_test.go** (NEW FILE)
171+
- Added comprehensive unit tests for `FetchPlacementFromKey()`
172+
- Added unit tests for `GetPlacementKeyFromObj()`
173+
- Tests cover both ClusterResourcePlacement and ResourcePlacement scenarios
174+
- Error handling tests for non-existent placements
175+
176+
#### 4. **pkg/scheduler/scheduler_test.go** (ENHANCED)
177+
- Updated existing tests to work with PlacementObj interface
178+
- Added `TestCleanUpAllBindingsForWithInterface()` for interface-specific testing
179+
- Added `TestAddSchedulerCleanUpFinalizerWithInterface()` for finalizer logic testing
180+
- Added `TestGetCleanupFinalizerForPlacement()` for finalizer selection testing
181+
182+
### **UPDATE**: Finalizer Consistency Change
183+
184+
Per user request, reverted finalizer logic to consistently use `SchedulerCRPCleanupFinalizer` for all placement types:
185+
186+
#### 5. **pkg/scheduler/scheduler.go** (FINALIZER UPDATES)
187+
- **Removed `getCleanupFinalizerForPlacement()` method entirely**
188+
- **`scheduleOnce()`**: Updated to use `fleetv1beta1.SchedulerCRPCleanupFinalizer` directly
189+
- **`cleanUpAllBindingsFor()`**: Simplified to always use `fleetv1beta1.SchedulerCRPCleanupFinalizer`
190+
- **`addSchedulerCleanUpFinalizer()`**: Updated to use `fleetv1beta1.SchedulerCRPCleanupFinalizer` directly
191+
192+
#### 6. **pkg/scheduler/scheduler_test.go** (TEST UPDATES)
193+
- **Removed `TestGetCleanupFinalizerForPlacement()` test** since the method no longer exists
194+
- **Updated `TestAddSchedulerCleanUpFinalizerWithInterface()`**: All test cases now expect `SchedulerCRPCleanupFinalizer`
195+
- **Updated `TestCleanUpAllBindingsForWithInterface()`**: Namespaced placement test now uses `SchedulerCRPCleanupFinalizer`
196+
197+
This ensures consistent finalizer usage across all placement types (both cluster-scoped and namespaced).
198+
199+
**User requested to use `SchedulerCRPCleanupFinalizer` consistently for all placement types instead of dynamic finalizer selection.**
200+
201+
#### Modified Files for Finalizer Consistency:
202+
203+
##### **pkg/scheduler/scheduler.go** (UPDATED)
204+
- **`getCleanupFinalizerForPlacement()`**: Simplified to always return `SchedulerCRPCleanupFinalizer` regardless of placement type
205+
- **`cleanUpAllBindingsFor()`**: Updated to use `SchedulerCRPCleanupFinalizer` consistently instead of dynamic selection
206+
207+
##### **pkg/scheduler/scheduler_test.go** (UPDATED)
208+
- **`TestGetCleanupFinalizerForPlacement()`**: Updated both test cases to expect `SchedulerCRPCleanupFinalizer`
209+
- **`TestAddSchedulerCleanUpFinalizerWithInterface()`**: Updated both test cases to expect `SchedulerCRPCleanupFinalizer`
210+
- **`TestCleanUpAllBindingsForWithInterface()`**: Updated namespaced placement test to use `SchedulerCRPCleanupFinalizer`
211+
212+
### PolicySnapshotObj Interface Refactor
213+
214+
### Phase 7: PolicySnapshotObj Interface Refactor
215+
- [x] Task 7.1: Find all direct references to ClusterSchedulingPolicySnapshot in scheduler code
216+
- [x] Task 7.2: Analyze PolicySnapshotObj and PolicySnapshotList interface definitions
217+
- [x] Task 7.3: Create utility functions for PolicySnapshotObj interface (if needed)
218+
- [ ] Task 7.4: Update scheduler framework methods to use PolicySnapshotObj interface
219+
- [ ] Task 7.5: Update any remaining list operations to use PolicySnapshotList interface
220+
- [ ] Task 7.6: Update tests to work with PolicySnapshotObj interface
221+
- [ ] Task 7.7: Run tests to ensure no regressions
222+
- [ ] Task 7.8: Run code formatting and validation
223+
224+
#### Phase 7 Analysis Results:
225+
- **Found 51+ references** to `ClusterSchedulingPolicySnapshot` in scheduler directory
226+
- **Interface Analysis**:
227+
- `PolicySnapshotObj` extends `client.Object`, `PolicySnapshotSpecGetSetter`, and `PolicySnapshotStatusGetSetter`
228+
- `PolicySnapshotList` extends `client.ObjectList` and `PolicySnapshotListItemGetter`
229+
- Both `ClusterSchedulingPolicySnapshot` and `SchedulingPolicySnapshot` implement these interfaces
230+
- **Key Discovery**: `lookupLatestPolicySnapshot` in scheduler.go already uses PolicySnapshotObj interface correctly
231+
- **Files Requiring Updates**: framework.go, frameworkutils.go, interface.go, and various plugin files
232+
- **Test Files**: scheduler_test.go and plugin test files need interface updates

‎.github/.copilot/domain_knowledge/cluster vs namespace scoped resources‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
# naming convention
2+
All APIs whose name starts with Cluster are clusterScoped resources while its counterpart whose name matches the remainder of the API represents a namespace scoped resource.
3+
For example, we have ClusterResourcePlacement API and ResourcePlacement API. The former is cluster scoped while the latter is namespace scoped.
4+
# The difference between cluster scoped and namespace scoped resources
5+
The main difference between cluster scoped and namespace scoped resources is that cluster scoped resources are not bound to a specific namespace and can be accessed across the entire cluster, while namespace scoped resources are bound to a specific namespace and can only be accessed within that namespace. This translates to the following differences in how to get, list, create, update, and delete these resources:
6+
## Cluster Scoped Resources
7+
When one does CRUD on a cluster scoped resources, it only needs to know its name and type, i.e. something like this client.Get(ctx, types.NamespacedName{Name: string(name)}, crp)
8+
## Namespace Scoped Resources
9+
When one does CRUD on a namespace scoped resources, it needs to know its name, type, and namespace, i.e. something like this client.Get(ctx, types.NamespacedName{Name: string(name), Namespace: namespace}, rp)

‎.github/copilot-instructions.md‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,9 +10,6 @@ The main idea is that we are creating a multi-cluster application management sol
1010
- If you're waiting for my confirmation ("OK"), proceed without further prompting.
1111
- Follow the [Uber Go Style Guide](https://github.com/uber-go/guide/blob/master/style.md) if possible.
1212
- Favor using the standard library over third-party libraries.
13-
- Run "goimports" on save.
14-
- Run "golangci-lint" and "go vet" to check for errors.
15-
- Use "go mod tidy" if the dependencies are changed.
1613
- Run "make reviewable" before submitting a pull request to ensure the code is formatted correctly and all dependencies are up to date.
1714

1815
## Terminology
@@ -124,6 +121,7 @@ A breadcrumb is a collaborative scratch pad that allow the user and agent to get
124121
- **Get explicit approval** on the plan before implementation.
125122
- Update the breadcrumb **AFTER completing each significant change**.
126123
- Keep the breadcrumb as our single source of truth as it contains the most recent information.
124+
- Do not ask for approval **BEFORE** running unit tests or integration tests.
127125

128126
5. Ask me to verify the plan with: "Are you happy with this implementation plan?" before proceeding with code changes.
129127

‎apis/interface.go‎

Lines changed: 0 additions & 89 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@ limitations under the License.
1818
package apis
1919

2020
import (
21-
"github.com/kubefleet-dev/kubefleet/apis/placement/v1beta1"
2221
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2322
"sigs.k8s.io/controller-runtime/pkg/client"
2423
)
@@ -37,91 +36,3 @@ type ConditionedObj interface {
3736
client.Object
3837
Conditioned
3938
}
40-
41-
// A PlacementSpec contains placementSpec
42-
// +kubebuilder:object:generate=false
43-
type PlacementSpec interface {
44-
GetPlacementSpec() *v1beta1.PlacementSpec
45-
SetPlacementSpec(*v1beta1.PlacementSpec)
46-
}
47-
48-
// A PlacementStatus contains placementStatus
49-
// +kubebuilder:object:generate=false
50-
type PlacementStatus interface {
51-
GetPlacementStatus() *v1beta1.PlacementStatus
52-
SetPlacementStatus(*v1beta1.PlacementStatus)
53-
}
54-
55-
// A PlacementObj is for kubernetes resource placement object.
56-
// +kubebuilder:object:generate=false
57-
type PlacementObj interface {
58-
client.Object
59-
PlacementSpec
60-
PlacementStatus
61-
}
62-
63-
// A BindingSpec contains bindingSpec
64-
// +kubebuilder:object:generate=false
65-
type BindingSpec interface {
66-
GetBindingSpec() *v1beta1.ResourceBindingSpec
67-
SetBindingSpec(*v1beta1.ResourceBindingSpec)
68-
}
69-
70-
// A BindingStatus contains bindingStatus
71-
// +kubebuilder:object:generate=false
72-
type BindingStatus interface {
73-
GetBindingStatus() *v1beta1.ResourceBindingStatus
74-
SetBindingStatus(*v1beta1.ResourceBindingStatus)
75-
}
76-
77-
// A BindingObj is for kubernetes resource binding object.
78-
// +kubebuilder:object:generate=false
79-
type BindingObj interface {
80-
client.Object
81-
BindingSpec
82-
BindingStatus
83-
}
84-
85-
// A PolicySnapshotSpec contains policy snapshot spec
86-
// +kubebuilder:object:generate=false
87-
type PolicySnapshotSpec interface {
88-
GetPolicySnapshotSpec() *v1beta1.SchedulingPolicySnapshotSpec
89-
SetPolicySnapshotSpec(*v1beta1.SchedulingPolicySnapshotSpec)
90-
}
91-
92-
// A PolicySnapshotStatus contains policy snapshot status
93-
// +kubebuilder:object:generate=false
94-
type PolicySnapshotStatus interface {
95-
GetPolicySnapshotStatus() *v1beta1.SchedulingPolicySnapshotStatus
96-
SetPolicySnapshotStatus(*v1beta1.SchedulingPolicySnapshotStatus)
97-
}
98-
99-
// A PolicySnapshotObj is for kubernetes policy snapshot object.
100-
// +kubebuilder:object:generate=false
101-
type PolicySnapshotObj interface {
102-
client.Object
103-
PolicySnapshotSpec
104-
PolicySnapshotStatus
105-
}
106-
107-
// A ResourceSnapshotSpec contains resource snapshot spec
108-
// +kubebuilder:object:generate=false
109-
type ResourceSnapshotSpec interface {
110-
GetResourceSnapshotSpec() *v1beta1.ResourceSnapshotSpec
111-
SetResourceSnapshotSpec(*v1beta1.ResourceSnapshotSpec)
112-
}
113-
114-
// A ResourceSnapshotStatus contains resource snapshot status
115-
// +kubebuilder:object:generate=false
116-
type ResourceSnapshotStatus interface {
117-
GetResourceSnapshotStatus() *v1beta1.ResourceSnapshotStatus
118-
SetResourceSnapshotStatus(*v1beta1.ResourceSnapshotStatus)
119-
}
120-
121-
// A ResourceSnapshotObj is for kubernetes resource snapshot object.
122-
// +kubebuilder:object:generate=false
123-
type ResourceSnapshotObj interface {
124-
client.Object
125-
ResourceSnapshotSpec
126-
ResourceSnapshotStatus
127-
}

0 commit comments

Comments
 (0)