feat: [FEP-0001] add work generator (1/) - #848
Conversation
Signed-off-by: michaelawyu <chenyu1@microsoft.com>
Signed-off-by: michaelawyu <chenyu1@microsoft.com>
|
Note also that this PR depends on #827. Will rebase after it is merged. |
There was a problem hiding this comment.
Pull request overview
Implements the initial v1 WorkGenerator controller for the placement policy APIs (FEP-0001), responsible for materializing Work objects from PlacementResourceSnapshots and syncing per-resource progress back onto PlacementBinding status.
Changes:
- Added a new
pkg/v1/controllers/workgeneratorcontroller with reconciliation, retrieval, work CRUD, status sync, and cleanup/finalizer handling. - Extended v1alpha1 placement APIs/CRDs to support work ownership metadata (labels/annotations) and tracking progress via
PlacementBindingStatus.LastProcessedResourceSnapshotName. - Added accessor interfaces in v1alpha1 to unify handling of namespaced vs cluster-scoped placement types.
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/v1/controllers/workgenerator/controller.go | WorkGenerator reconcile loop and controller watches for bindings and Works |
| pkg/v1/controllers/workgenerator/retrieval.go | Fetching PlacementBindings/PlacementResourceSnapshots and listing existing Works |
| pkg/v1/controllers/workgenerator/works.go | Build/update/delete logic for Work objects derived from snapshots |
| pkg/v1/controllers/workgenerator/status.go | Binding status/conditions refresh and progress reporting |
| pkg/v1/controllers/workgenerator/cleanup.go | Finalizer management and cleanup path for deletion/suspension |
| pkg/v1/controllers/workgenerator/uniquename.go | Stable, length-bounded Work name generation |
| apis/kubefleet.dev/placement/v1alpha1/interface.go | Accessor interfaces + implementations for placement APIs |
| apis/kubefleet.dev/placement/v1alpha1/placementbinding_types.go | Adds waiting reason + LastProcessedResourceSnapshotName status field |
| apis/kubefleet.dev/placement/v1alpha1/placementresourcesnapshot_types.go | Adds snapshot labeling/annotation constants for indexing/sub-indexing |
| apis/kubefleet.dev/placement/v1alpha1/work_types.go | Adds Work ownership label keys + derived-from annotation key constants |
| apis/kubefleet.dev/placement/v1alpha1/zz_generated.deepcopy.go | Deepcopy updates for new PlacementBindingStatus field |
| config/crd/bases/placement.kubefleet.dev_placementbindings.yaml | CRD schema update for lastProcessedResourceSnapshotName |
| config/crd/bases/placement.kubefleet.dev_clusterplacementbindings.yaml | CRD schema update for lastProcessedResourceSnapshotName |
Files not reviewed (1)
- apis/kubefleet.dev/placement/v1alpha1/zz_generated.deepcopy.go: Generated file
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| if lastProcessedSnapshotName != primarySnapshotName { | ||
| return false | ||
| } | ||
|
|
||
| // Check if the sync strategy of the placement binding still matches that on the work objects. |
There was a problem hiding this comment.
Work objects in general are considered KubeFleet internal API objects; and for simplicity and performance reasons we typically do not guard against byzantine type of errors (e.g., unexpected mishandling, such as users editing/deleting the work objects manually, leading to system inconsistencies).
This branch is added as a shortcut to save the controller from constantly spending cycles cross-referencing/validating work objects (which includes all the manifests to place). Will add a comment on the code for better clarity.
| // Set a false Synchronized condition with the WaitingForSynchronization reason on the placement binding. | ||
| meta.SetStatusCondition(&placementBindingStatus.Conditions, metav1.Condition{ | ||
| Type: placementv1alpha1.PlacementBindingCondTypeSynchronized, | ||
| Status: metav1.ConditionFalse, | ||
| ObservedGeneration: placementBinding.GetGeneration(), | ||
| Reason: placementv1alpha1.PlacementBindingSynchronizedCondReasonWaitingForSynchronization, | ||
| Message: "Waiting for the resources to be synchronized to the target cluster", | ||
| }) | ||
|
|
There was a problem hiding this comment.
Note: the available condition would become stale in this situation (observedGeneration), consistent with Kubernetes standard condition practices. With that being said I do not mind setting this condition in the False/Unknown status if we believe it does yield better clarity.
| // TO-DO (chenyu1): switch to field-based indexes for better performance when listing objects. | ||
|
|
||
| func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { | ||
| startTime := time.Now() |
There was a problem hiding this comment.
N/A -> WIP; tests will be submitted in separate PRs.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Signed-off-by: michaelawyu <chenyu1@microsoft.com>
Signed-off-by: michaelawyu <chenyu1@microsoft.com>
Signed-off-by: michaelawyu <chenyu1@microsoft.com>
Description of your changes
This PR adds the work generator implementation as specified in FEP-0001 for the placement policy APIs.
Related to #789
I have:
make reviewableto ensure this PR is ready for review.How has this code been tested
N/A; please see the notes.
Special notes for your reviewer
To control the PR size, tests + some logs/events/monitoring setup have not been included in this PR; they will be submitted later.