Skip to content

Commit d809ab4

Browse files
ryanzhang-ossryanzhang-oss
committed
fix more interfaces
1 parent a294ce8 commit d809ab4

File tree

14 files changed

+240
-68
lines changed

14 files changed

+240
-68
lines changed

‎.github/.copilot/breadcrumbs/2025-01-20-1700-policysnapshot-interface-refactor.md‎

Lines changed: 164 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,164 @@
1+
# PolicySnapshotObj Interface Refactor
2+
3+
## Requirements
4+
5+
- Replace all direct references to ClusterSchedulingPolicySnapshot with the PolicySnapshotObj interface throughout the scheduler framework
6+
- Use PolicySnapshotList interface for all listing operations
7+
- Follow the same pattern as the PlacementObj interface refactor
8+
- Maintain existing functionality while making the scheduler more flexible
9+
- Update tests to work with the interface abstraction
10+
- Ensure all import aliases are maintained consistently
11+
12+
## Additional comments from user
13+
14+
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.
15+
16+
## Plan
17+
18+
### Phase 7: PolicySnapshotObj Interface Refactor
19+
- [x] Task 7.1: Find all direct references to ClusterSchedulingPolicySnapshot in scheduler code
20+
- [x] Task 7.2: Examine PolicySnapshotObj interface and understand its capabilities
21+
- [x] Task 7.3: Create utility functions to handle both ClusterSchedulingPolicySnapshot and SchedulingPolicySnapshot
22+
- [x] Task 7.4: Update scheduler framework methods to use PolicySnapshotObj interface ✅ **COMPLETED**
23+
- [x] Updated framework.go method signatures to use PolicySnapshotObj interface
24+
- [x] Updated annotation utility functions to accept client.Object interface
25+
- [x] Converted direct status field access to interface methods with GetPolicySnapshotStatus()
26+
- [x] Updated plugin implementations to use PolicySnapshotObj interface ✅ **ALL PLUGINS ALREADY UPDATED**
27+
- [x] Task 7.5: Update any remaining list operations to use PolicySnapshotList interface ✅ **ALREADY DONE**
28+
- [ ] Task 7.6: Update tests to work with PolicySnapshotObj interface
29+
- [ ] Update scheduler_test.go to use PolicySnapshotObj interface
30+
- [ ] Update framework/dummyplugin_test.go to use PolicySnapshotObj interface
31+
- [ ] Update plugin test files to use PolicySnapshotObj interface
32+
- [ ] Task 7.7: Run tests to ensure no regressions
33+
- [ ] Task 7.8: Run code formatting and validation
34+
35+
## Decisions
36+
37+
- Will use the existing PolicySnapshotObj interface from apis/placement/v1beta1/interface.go
38+
- The interface includes client.Object, PolicySnapshotSpecGetSetter, and PolicySnapshotStatusGetSetter interfaces
39+
- Scheduler methods will be updated to accept PolicySnapshotObj interface instead of concrete types
40+
- Backward compatibility will be maintained
41+
- The existing interface already provides all necessary methods: GetPolicySnapshotSpec(), SetPolicySnapshotSpec(), GetPolicySnapshotStatus(), SetPolicySnapshotStatus()
42+
- Test files will be updated to use the interface while maintaining test coverage
43+
44+
## Implementation Details
45+
46+
### Interface Definition
47+
48+
The PolicySnapshotObj interface is defined in `apis/placement/v1beta1/interface.go`:
49+
50+
```go
51+
// A PolicySnapshotObj is for kubernetes policy snapshot object.
52+
type PolicySnapshotObj interface {
53+
client.Object
54+
PolicySnapshotSpecGetSetter
55+
PolicySnapshotStatusGetSetter
56+
}
57+
58+
type PolicySnapshotSpecGetSetter interface {
59+
GetPolicySnapshotSpec() *SchedulingPolicySnapshotSpec
60+
SetPolicySnapshotSpec(*SchedulingPolicySnapshotSpec)
61+
}
62+
63+
type PolicySnapshotStatusGetSetter interface {
64+
GetPolicySnapshotStatus() *SchedulingPolicySnapshotStatus
65+
SetPolicySnapshotStatus(*SchedulingPolicySnapshotStatus)
66+
}
67+
```
68+
69+
Both ClusterSchedulingPolicySnapshot and SchedulingPolicySnapshot implement this interface.
70+
71+
### Framework and Plugin Updates ✅ **ALREADY COMPLETED**
72+
73+
All scheduler framework and plugin files have been verified to already use the PolicySnapshotObj interface:
74+
75+
#### Framework Method Signatures (Already Updated)
76+
- `pkg/scheduler/framework/framework.go`: All methods use `placementv1beta1.PolicySnapshotObj`
77+
- Plugin interfaces correctly specify PolicySnapshotObj in method signatures
78+
79+
#### Plugin Implementations (Already Updated)
80+
- **clustereligibility plugin**: Filter method uses `placementv1beta1.PolicySnapshotObj`
81+
- **sameplacementaffinity plugin**: Filter and Score methods use `placementv1beta1.PolicySnapshotObj`
82+
- **tainttoleration plugin**: Filter method uses `placementv1beta1.PolicySnapshotObj`
83+
- **clusteraffinity plugin**: Filter, Score, and utility methods use `placementv1beta1.PolicySnapshotObj`
84+
- **topologyspreadconstraints plugin**: All methods (PostBatch, PreFilter, Filter, PreScore, Score) and utilities use `placementv1beta1.PolicySnapshotObj`
85+
86+
#### Usage Patterns (Already Implemented)
87+
- **Policy Access**: All code uses `policy.GetPolicySnapshotSpec().Policy` instead of `policy.Spec.Policy`
88+
- **Status Access**: All code uses `policy.GetPolicySnapshotStatus()` and `policy.SetPolicySnapshotStatus()` interface methods
89+
- **Annotation Utilities**: Already accept `client.Object` interface for policy snapshot operations
90+
91+
#### List Operations (Already Updated)
92+
- `lookupLatestPolicySnapshot` in scheduler.go already uses `PolicySnapshotList` interface with `GetPolicySnapshotObjs()` method
93+
94+
## Changes Made
95+
96+
### Files Already Using PolicySnapshotObj Interface
97+
98+
#### Framework Core
99+
- **pkg/scheduler/framework/framework.go** ✅ (method signatures use PolicySnapshotObj)
100+
- **pkg/scheduler/scheduler.go** ✅ (lookupLatestPolicySnapshot uses PolicySnapshotList interface)
101+
102+
#### Plugin Implementations
103+
- **pkg/scheduler/framework/plugins/clustereligibility/plugin.go** ✅
104+
- **pkg/scheduler/framework/plugins/sameplacementaffinity/filtering.go** ✅
105+
- **pkg/scheduler/framework/plugins/sameplacementaffinity/scoring.go** ✅
106+
- **pkg/scheduler/framework/plugins/tainttoleration/filtering.go** ✅
107+
- **pkg/scheduler/framework/plugins/clusteraffinity/filtering.go** ✅
108+
- **pkg/scheduler/framework/plugins/clusteraffinity/scoring.go** ✅
109+
- **pkg/scheduler/framework/plugins/clusteraffinity/state.go** ✅
110+
- **pkg/scheduler/framework/plugins/topologyspreadconstraints/plugin.go** ✅
111+
- **pkg/scheduler/framework/plugins/topologyspreadconstraints/utils.go** ✅
112+
113+
### Files Requiring Test Updates
114+
115+
#### Test Files (Current Task 7.6)
116+
- **pkg/scheduler/scheduler_test.go** - Contains 11 references to ClusterSchedulingPolicySnapshot
117+
- **pkg/scheduler/framework/dummyplugin_test.go** - Contains 10 references in method signatures
118+
- **pkg/scheduler/framework/plugins/tainttoleration/filtering_test.go** - Contains test cases with concrete types
119+
- **pkg/scheduler/framework/plugins/topologyspreadconstraints/plugin_test.go** - Contains test cases with concrete types
120+
- **pkg/scheduler/framework/plugins/clusteraffinity/filtering_test.go** - Contains test cases with concrete types
121+
- **pkg/scheduler/framework/plugins/clusteraffinity/scoring_test.go** - Contains test cases with concrete types
122+
- **pkg/scheduler/framework/plugins/topologyspreadconstraints/utils_test.go** - Contains test cases with concrete types
123+
124+
### Current Status
125+
126+
**Phase 7 Progress:**
127+
- ✅ Task 7.1-7.5: Framework and plugin implementation updates completed
128+
- 🔄 Task 7.6: Test file updates (IN PROGRESS)
129+
- ⏳ Task 7.7: Run tests to ensure no regressions
130+
- ⏳ Task 7.8: Run code formatting and validation
131+
132+
The scheduler framework and all plugins have been successfully converted to use PolicySnapshotObj interface. The remaining work is to update test files to use the interface instead of concrete types.
133+
134+
## Before/After Comparison
135+
136+
### Before (Test Files)
137+
```go
138+
// Test using concrete type
139+
policySnapshot *fleetv1beta1.ClusterSchedulingPolicySnapshot
140+
policy := &placementv1beta1.ClusterSchedulingPolicySnapshot{...}
141+
```
142+
143+
### After (Framework/Plugins - Already Done)
144+
```go
145+
// Framework using interface
146+
func (f *framework) RunFilterPlugins(ctx context.Context, state CycleStatePluginReadWriter,
147+
policy placementv1beta1.PolicySnapshotObj, cluster *clusterv1beta1.MemberCluster) PluginToStatus {
148+
// Use interface methods
149+
spec := policy.GetPolicySnapshotSpec()
150+
status := policy.GetPolicySnapshotStatus()
151+
}
152+
```
153+
154+
## References
155+
156+
### Domain Knowledge Files
157+
- None directly applicable
158+
159+
### Specification Files
160+
- None directly applicable
161+
162+
### Interface Definition
163+
- File: `apis/placement/v1beta1/interface.go` (PolicySnapshotObj and PolicySnapshotList interfaces)
164+
- File: `apis/placement/v1beta1/policysnapshot_types.go` (interface implementations)

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

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,11 @@
22

33
## Requirements
44

5-
- Refactor the scheduler to use a PlacementObj interface instead of the concrete ClusterResourcePlacement type
5+
- Re- [x] Task 7.4: Update scheduler framework methods to use PolicySnapshotObj interface **COMPLETED**
6+
- [x] Updated framework.go method signatures to use PolicySnapshotObj interface
7+
- [x] Updated annotation utility functions to accept client.Object interface
8+
- [x] Converted direct status field access to interface methods with GetPolicySnapshotStatus()
9+
- [x] Updated plugin implementations to use PolicySnapshotObj interface ✅ **ALL PLUGINS ALREADY UPDATED**r the scheduler to use a PlacementObj interface instead of the concrete ClusterResourcePlacement type
610
- Support both ClusterResourcePlacement (cluster-scoped) and ResourcePlacement (namespaced) types through the interface
711
- Utilize domain knowledge about cluster vs namespaced resources naming conventions
812
- Maintain existing functionality while making the scheduler more flexible
@@ -64,7 +68,11 @@ User wants to refactor the scheduler to use PlacementObj interface instead of Cl
6468
- [x] Task 7.1: Find all direct references to ClusterSchedulingPolicySnapshot in scheduler code
6569
- [x] Task 7.2: Examine PolicySnapshotObj interface and understand its capabilities
6670
- [x] Task 7.3: Create utility functions to handle both ClusterSchedulingPolicySnapshot and SchedulingPolicySnapshot
67-
- [ ] Task 7.4: Update scheduler methods to use PolicySnapshotObj interface
71+
- [x] Task 7.4: Update scheduler framework methods to use PolicySnapshotObj interface (IN PROGRESS)
72+
- [x] Updated framework.go method signatures to use PolicySnapshotObj interface
73+
- [x] Updated annotation utility functions to accept client.Object interface
74+
- [x] Converted direct status field access to interface methods with GetPolicySnapshotStatus()
75+
- [ ] Update plugin implementations to use PolicySnapshotObj interface (CURRENT)
6876
- [ ] Task 7.5: Update any list operations to use PolicySnapshotList interface
6977
- [ ] Task 7.6: Update tests to work with PolicySnapshotObj interface
7078
- [ ] Task 7.7: Run tests to ensure no regressions

‎pkg/scheduler/framework/dummyplugin_test.go‎

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -30,11 +30,11 @@ const (
3030
// A no-op, dummy plugin which connects to all extension points.
3131
type DummyAllPurposePlugin struct {
3232
name string
33-
postBatchRunner func(ctx context.Context, state CycleStatePluginReadWriter, policy *placementv1beta1.ClusterSchedulingPolicySnapshot) (size int, status *Status)
34-
preFilterRunner func(ctx context.Context, state CycleStatePluginReadWriter, policy *placementv1beta1.ClusterSchedulingPolicySnapshot) (status *Status)
35-
filterRunner func(ctx context.Context, state CycleStatePluginReadWriter, policy *placementv1beta1.ClusterSchedulingPolicySnapshot, cluster *clusterv1beta1.MemberCluster) (status *Status)
36-
preScoreRunner func(ctx context.Context, state CycleStatePluginReadWriter, policy *placementv1beta1.ClusterSchedulingPolicySnapshot) (status *Status)
37-
scoreRunner func(ctx context.Context, state CycleStatePluginReadWriter, policy *placementv1beta1.ClusterSchedulingPolicySnapshot, cluster *clusterv1beta1.MemberCluster) (score *ClusterScore, status *Status)
33+
postBatchRunner func(ctx context.Context, state CycleStatePluginReadWriter, policy placementv1beta1.PolicySnapshotObj) (size int, status *Status)
34+
preFilterRunner func(ctx context.Context, state CycleStatePluginReadWriter, policy placementv1beta1.PolicySnapshotObj) (status *Status)
35+
filterRunner func(ctx context.Context, state CycleStatePluginReadWriter, policy placementv1beta1.PolicySnapshotObj, cluster *clusterv1beta1.MemberCluster) (status *Status)
36+
preScoreRunner func(ctx context.Context, state CycleStatePluginReadWriter, policy placementv1beta1.PolicySnapshotObj) (status *Status)
37+
scoreRunner func(ctx context.Context, state CycleStatePluginReadWriter, policy placementv1beta1.PolicySnapshotObj, cluster *clusterv1beta1.MemberCluster) (score *ClusterScore, status *Status)
3838
}
3939

4040
// Check that the dummy plugin implements all the interfaces at compile time.
@@ -52,27 +52,27 @@ func (p *DummyAllPurposePlugin) Name() string {
5252
}
5353

5454
// PostBatch implements the PostBatch interface for the dummy plugin.
55-
func (p *DummyAllPurposePlugin) PostBatch(ctx context.Context, state CycleStatePluginReadWriter, policy *placementv1beta1.ClusterSchedulingPolicySnapshot) (size int, status *Status) { //nolint:revive
55+
func (p *DummyAllPurposePlugin) PostBatch(ctx context.Context, state CycleStatePluginReadWriter, policy placementv1beta1.PolicySnapshotObj) (size int, status *Status) { //nolint:revive
5656
return p.postBatchRunner(ctx, state, policy)
5757
}
5858

5959
// PreFilter implements the PreFilter interface for the dummy plugin.
60-
func (p *DummyAllPurposePlugin) PreFilter(ctx context.Context, state CycleStatePluginReadWriter, policy *placementv1beta1.ClusterSchedulingPolicySnapshot) (status *Status) { //nolint:revive
60+
func (p *DummyAllPurposePlugin) PreFilter(ctx context.Context, state CycleStatePluginReadWriter, policy placementv1beta1.PolicySnapshotObj) (status *Status) { //nolint:revive
6161
return p.preFilterRunner(ctx, state, policy)
6262
}
6363

6464
// Filter implements the Filter interface for the dummy plugin.
65-
func (p *DummyAllPurposePlugin) Filter(ctx context.Context, state CycleStatePluginReadWriter, policy *placementv1beta1.ClusterSchedulingPolicySnapshot, cluster *clusterv1beta1.MemberCluster) (status *Status) { //nolint:revive
65+
func (p *DummyAllPurposePlugin) Filter(ctx context.Context, state CycleStatePluginReadWriter, policy placementv1beta1.PolicySnapshotObj, cluster *clusterv1beta1.MemberCluster) (status *Status) { //nolint:revive
6666
return p.filterRunner(ctx, state, policy, cluster)
6767
}
6868

6969
// PreScore implements the PreScore interface for the dummy plugin.
70-
func (p *DummyAllPurposePlugin) PreScore(ctx context.Context, state CycleStatePluginReadWriter, policy *placementv1beta1.ClusterSchedulingPolicySnapshot) (status *Status) { //nolint:revive
70+
func (p *DummyAllPurposePlugin) PreScore(ctx context.Context, state CycleStatePluginReadWriter, policy placementv1beta1.PolicySnapshotObj) (status *Status) { //nolint:revive
7171
return p.preScoreRunner(ctx, state, policy)
7272
}
7373

7474
// Score implements the Score interface for the dummy plugin.
75-
func (p *DummyAllPurposePlugin) Score(ctx context.Context, state CycleStatePluginReadWriter, policy *placementv1beta1.ClusterSchedulingPolicySnapshot, cluster *clusterv1beta1.MemberCluster) (score *ClusterScore, status *Status) { //nolint:revive
75+
func (p *DummyAllPurposePlugin) Score(ctx context.Context, state CycleStatePluginReadWriter, policy placementv1beta1.PolicySnapshotObj, cluster *clusterv1beta1.MemberCluster) (score *ClusterScore, status *Status) { //nolint:revive
7676
return p.scoreRunner(ctx, state, policy, cluster)
7777
}
7878

‎pkg/scheduler/framework/plugins/clusteraffinity/filtering.go‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -28,13 +28,13 @@ import (
2828
func (p *Plugin) PreFilter(
2929
_ context.Context,
3030
_ framework.CycleStatePluginReadWriter,
31-
ps *placementv1beta1.ClusterSchedulingPolicySnapshot,
31+
ps placementv1beta1.PolicySnapshotObj,
3232
) (status *framework.Status) {
33-
noRequiredClusterAffinityTerms := ps.Spec.Policy == nil ||
34-
ps.Spec.Policy.Affinity == nil ||
35-
ps.Spec.Policy.Affinity.ClusterAffinity == nil ||
36-
ps.Spec.Policy.Affinity.ClusterAffinity.RequiredDuringSchedulingIgnoredDuringExecution == nil ||
37-
len(ps.Spec.Policy.Affinity.ClusterAffinity.RequiredDuringSchedulingIgnoredDuringExecution.ClusterSelectorTerms) == 0
33+
noRequiredClusterAffinityTerms := ps.GetPolicySnapshotSpec().Policy == nil ||
34+
ps.GetPolicySnapshotSpec().Policy.Affinity == nil ||
35+
ps.GetPolicySnapshotSpec().Policy.Affinity.ClusterAffinity == nil ||
36+
ps.GetPolicySnapshotSpec().Policy.Affinity.ClusterAffinity.RequiredDuringSchedulingIgnoredDuringExecution == nil ||
37+
len(ps.GetPolicySnapshotSpec().Policy.Affinity.ClusterAffinity.RequiredDuringSchedulingIgnoredDuringExecution.ClusterSelectorTerms) == 0
3838
if noRequiredClusterAffinityTerms {
3939
// There are no required cluster affinity terms to enforce; consider all clusters
4040
// eligible for resource placement in the scope of this plugin.
@@ -50,15 +50,15 @@ func (p *Plugin) PreFilter(
5050
func (p *Plugin) Filter(
5151
_ context.Context,
5252
_ framework.CycleStatePluginReadWriter,
53-
ps *placementv1beta1.ClusterSchedulingPolicySnapshot,
53+
ps placementv1beta1.PolicySnapshotObj,
5454
cluster *clusterv1beta1.MemberCluster,
5555
) (status *framework.Status) {
5656
// Note that this extension point assumes that previous extension point (PreFilter) has
5757
// guaranteed that if scheduling policy reaches this stage, it must have at least one
5858
// required cluster affinity term to enforce.
5959

60-
for idx := range ps.Spec.Policy.Affinity.ClusterAffinity.RequiredDuringSchedulingIgnoredDuringExecution.ClusterSelectorTerms {
61-
t := &ps.Spec.Policy.Affinity.ClusterAffinity.RequiredDuringSchedulingIgnoredDuringExecution.ClusterSelectorTerms[idx]
60+
for idx := range ps.GetPolicySnapshotSpec().Policy.Affinity.ClusterAffinity.RequiredDuringSchedulingIgnoredDuringExecution.ClusterSelectorTerms {
61+
t := &ps.GetPolicySnapshotSpec().Policy.Affinity.ClusterAffinity.RequiredDuringSchedulingIgnoredDuringExecution.ClusterSelectorTerms[idx]
6262
r := clusterRequirement(*t)
6363
isMatched, err := r.Matches(cluster)
6464
if err != nil {

‎pkg/scheduler/framework/plugins/clusteraffinity/scoring.go‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -30,12 +30,12 @@ import (
3030
func (p *Plugin) PreScore(
3131
_ context.Context,
3232
state framework.CycleStatePluginReadWriter,
33-
policy *placementv1beta1.ClusterSchedulingPolicySnapshot,
33+
policy placementv1beta1.PolicySnapshotObj,
3434
) (status *framework.Status) {
35-
noPreferredClusterAffinityTerms := policy.Spec.Policy == nil ||
36-
policy.Spec.Policy.Affinity == nil ||
37-
policy.Spec.Policy.Affinity.ClusterAffinity == nil ||
38-
len(policy.Spec.Policy.Affinity.ClusterAffinity.PreferredDuringSchedulingIgnoredDuringExecution) == 0
35+
noPreferredClusterAffinityTerms := policy.GetPolicySnapshotSpec().Policy == nil ||
36+
policy.GetPolicySnapshotSpec().Policy.Affinity == nil ||
37+
policy.GetPolicySnapshotSpec().Policy.Affinity.ClusterAffinity == nil ||
38+
len(policy.GetPolicySnapshotSpec().Policy.Affinity.ClusterAffinity.PreferredDuringSchedulingIgnoredDuringExecution) == 0
3939
if noPreferredClusterAffinityTerms {
4040
// There are no preferred cluster affinity terms specified in the scheduling policy;
4141
// skip the step.
@@ -62,7 +62,7 @@ func (p *Plugin) PreScore(
6262
func (p *Plugin) Score(
6363
_ context.Context,
6464
state framework.CycleStatePluginReadWriter,
65-
policy *placementv1beta1.ClusterSchedulingPolicySnapshot,
65+
policy placementv1beta1.PolicySnapshotObj,
6666
cluster *clusterv1beta1.MemberCluster,
6767
) (score *framework.ClusterScore, status *framework.Status) {
6868
// Read the plugin state.
@@ -74,7 +74,7 @@ func (p *Plugin) Score(
7474
}
7575

7676
score = &framework.ClusterScore{}
77-
for _, t := range policy.Spec.Policy.Affinity.ClusterAffinity.PreferredDuringSchedulingIgnoredDuringExecution {
77+
for _, t := range policy.GetPolicySnapshotSpec().Policy.Affinity.ClusterAffinity.PreferredDuringSchedulingIgnoredDuringExecution {
7878
if t.Weight != 0 {
7979
cp := clusterPreference(t)
8080
ts, err := cp.Scores(ps, cluster)

0 commit comments

Comments
 (0)