|
| 1 | +# Extend getOrCreateClusterResourceSnapshot to Handle Both Snapshot Types |
| 2 | + |
| 3 | +## Context |
| 4 | + |
| 5 | +Following successful interface refactoring and deletion function optimizations, user requested: "can you modify the function getOrCreateClusterResourceSnapshot to handle both clusterresourcesnapshot and resourcesnapshot?" |
| 6 | + |
| 7 | +## Problem |
| 8 | + |
| 9 | +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. |
| 10 | + |
| 11 | +## Research Findings |
| 12 | + |
| 13 | +### ResourceSnapshotObj Interface Structure |
| 14 | +```go |
| 15 | +type ResourceSnapshotObj interface { |
| 16 | + apis.ConditionedObj |
| 17 | + ResourceSnapshotSpecGetterSetter |
| 18 | + ResourceSnapshotStatusGetterSetter |
| 19 | +} |
| 20 | + |
| 21 | +type ResourceSnapshotSpecGetterSetter interface { |
| 22 | + GetResourceSnapshotSpec() *ResourceSnapshotSpec |
| 23 | + SetResourceSnapshotSpec(ResourceSnapshotSpec) |
| 24 | +} |
| 25 | + |
| 26 | +type ResourceSnapshotStatusGetterSetter interface { |
| 27 | + GetResourceSnapshotStatus() *ResourceSnapshotStatus |
| 28 | + SetResourceSnapshotStatus(ResourceSnapshotStatus) |
| 29 | +} |
| 30 | +``` |
| 31 | + |
| 32 | +### Implementation Types |
| 33 | +- **ClusterResourceSnapshot** (cluster-scoped, v1beta1) - implements ResourceSnapshotObj |
| 34 | +- **ResourceSnapshot** (namespace-scoped, v1beta1) - implements ResourceSnapshotObj |
| 35 | + |
| 36 | +Both types share: |
| 37 | +- Same `ResourceSnapshotSpec` structure |
| 38 | +- Same interface methods for spec/status access |
| 39 | +- Same labeling and annotation patterns |
| 40 | +- Different scoping (cluster vs namespace) |
| 41 | + |
| 42 | +### Pattern Precedent |
| 43 | +The delete functions already follow this pattern: |
| 44 | +- `deleteClusterSchedulingPolicySnapshots` handles both ClusterSchedulingPolicySnapshot and SchedulingPolicySnapshot |
| 45 | +- `deleteResourceSnapshots` uses `DeleteAllOf` with namespace awareness |
| 46 | + |
| 47 | +## Plan |
| 48 | + |
| 49 | +### Phase 1: Function Signature Updates |
| 50 | +- [x] Task 1.1: Change function signature to accept `PlacementObj` interface instead of concrete type |
| 51 | +- [x] Task 1.2: Update return type to use `ResourceSnapshotObj` interface |
| 52 | +- [x] Task 1.3: Update function name to `getOrCreateResourceSnapshot` for clarity |
| 53 | + |
| 54 | +### Phase 2: Namespace Detection Logic |
| 55 | +- [x] Task 2.1: Add helper function to determine if placement is namespace-scoped |
| 56 | +- [x] Task 2.2: Implement logic to choose between ClusterResourceSnapshot and ResourceSnapshot based on placement scope |
| 57 | + |
| 58 | +### Phase 3: Snapshot Creation Logic |
| 59 | +- [ ] Task 3.1: Extend `buildMasterClusterResourceSnapshot` to handle both types |
| 60 | +- [ ] Task 3.2: Extend `buildSubIndexResourceSnapshot` to handle both types |
| 61 | +- [ ] Task 3.3: Update snapshot creation logic to use proper type based on scope |
| 62 | + |
| 63 | +### Phase 4: Helper Functions Updates |
| 64 | +- [ ] Task 4.1: Update `lookupLatestResourceSnapshot` to handle both types |
| 65 | +- [ ] Task 4.2: Update `listSortedResourceSnapshots` to handle both types |
| 66 | +- [ ] Task 4.3: Update `createResourceSnapshot` to handle both types |
| 67 | + |
| 68 | +### Phase 5: Testing and Validation |
| 69 | +- [ ] Task 5.1: Run existing unit tests to ensure no regression |
| 70 | +- [ ] Task 5.2: Verify function works with both cluster-scoped and namespace-scoped placements |
| 71 | +- [ ] Task 5.3: Ensure interface compatibility throughout the codebase |
| 72 | + |
| 73 | +## Success Criteria |
| 74 | +- Function accepts both ClusterResourcePlacement and ResourcePlacement |
| 75 | +- Creates appropriate snapshot type based on placement scope |
| 76 | +- Maintains all existing functionality for cluster-scoped resources |
| 77 | +- Uses ResourceSnapshotObj interface throughout |
| 78 | +- No breaking changes to existing API |
| 79 | +- All tests pass |
| 80 | + |
| 81 | +## Implementation Notes |
| 82 | +- Follow established namespace-aware pattern from delete functions |
| 83 | +- Use ResourceSnapshotObj interface consistently |
| 84 | +- Maintain backward compatibility |
| 85 | +- Apply same labeling/annotation patterns for both types |
0 commit comments