feat: add status back-reporting supprt (2/, new controller) #329
Conversation
Signed-off-by: michaelawyu <chenyu1@microsoft.com>
|
Note: as this controller requires other components to fully function; it is NOT currently enabled. |
Signed-off-by: michaelawyu <chenyu1@microsoft.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
| } | ||
|
|
||
| // Validate the scheduling policy of the placement object. | ||
| schedulingPolicy := placementObj.GetPlacementSpec().Policy |
There was a problem hiding this comment.
Will you do these validation in the CRP validator when someone specifies the report back strategy? If so does it make sense to move some of these validations into the common validator package?
There was a problem hiding this comment.
Hi Wei! We are switching to CEL-based validation as I understand (a separate PR will be prepared to add the CEL rule for this part). The CRP validator methods are currently only used for the webhooks as I understand.
There was a problem hiding this comment.
Though honestly speaking I do not have a very strong opinion on this -> adding the logic to the webhook part as well definitely helps, though because of the reason that webhooks are not 100% bullet-proof we still need to do some sanity check here.
| // formatResourceIdentifier formats a ResourceIdentifier object to a string for keying purposes. | ||
| // | ||
| // The format in use is `[API-GROUP]/[API-VERSION]/[API-KIND]/[NAMESPACE]/[NAME]`, e.g., `/v1/Namespace//work`. | ||
| func formatResourceIdentifier(resourceIdentifier *placementv1beta1.ResourceIdentifier) string { |
There was a problem hiding this comment.
this might be useful beyond this package? I am wondering if this belongs to the pkg/utils
similarly formatWorkResourceIdentifier might also belong there?
There was a problem hiding this comment.
Yeah, I guess... though at this moment I think only this controller and the work applier need the formatter (the work applier only uses the work resource identifier one); and for the work applier the logic is bit different (it needs to handle cases where manifests cannot be decoded).
| // improves, the statuses back-reporter should read the label instead for CRP/RP tracking here. | ||
| placementObj, err := r.validatePlacementObjectForOriginalResourceStatusBackReporting(ctx, work) | ||
| if err != nil { | ||
| klog.ErrorS(err, "Failed to validate the placement object associated with the Work object for back-reporting statuses to original resources", "work", workRef) |
There was a problem hiding this comment.
Is this a user facing error that we should report using EventRecorder?
There was a problem hiding this comment.
Hi Wei! For this method there are some errors (e.g., API GET errors) that are not user-facing; and for the sanity check part the plan is to have CEL expressions in place so users are going to see errors when they create the CRP object.
| } | ||
| ) | ||
|
|
||
| // createWorkObject creates a new Work object with the given name, manifests, and apply strategy. |
There was a problem hiding this comment.
Copy/paste error from workapplier/controller_integration_test.go? Should we consolidate the 2 functions?
There was a problem hiding this comment.
Yeah, let me fix the comment. Thanks!
There was a problem hiding this comment.
For the consolidation part, we do not have a test utilities pkg yet and due to the usage of Gomega/Ginkgo deps such pkgs are going to look a bit tricky -> allow me a bit more time to think this through if it is OK
| return statusBackReportingWrapperData, nil | ||
| } | ||
|
|
||
| func workObjectRemovedActual(workName string) func() error { |
There was a problem hiding this comment.
also a duplicated utility from workapplier/controller_integration_test.go
|
|
||
| // Prepare a child context. | ||
| // Cancel the child context anyway to avoid leaks. | ||
| childCtx, cancel := context.WithCancel(ctx) |
There was a problem hiding this comment.
why create a childCtx instead of just passing ctx into ParallelizeUntil?
There was a problem hiding this comment.
Hi Wei! The child context is created to allow this parallelization to be cancelled without impacting the main context (the reconciliation loop context) (and the parent context's cancellation will still propagate to the child context). Though I totally understand your concern that currently the cancel method is not invoked explicitly...
| func TestAPIs(t *testing.T) { | ||
| RegisterFailHandler(Fail) | ||
|
|
||
| RunSpecs(t, "Work Applier Integration Test Suite") |
There was a problem hiding this comment.
copy paste error from Work Applier integration test
There was a problem hiding this comment.
Thanks! Let me fix that.
| return ctrl.Result{}, errorsutil.NewAggregate(errs) | ||
| } | ||
|
|
||
| // validatePlacementObjectForOriginalResourceStatusBackReporting validatess whether |
There was a problem hiding this comment.
nit: validatess typo
There was a problem hiding this comment.
Thanks; let me fix that.
2 participants
Description of your changes
This PR adds a new controller that enables mirroring of status between the hub cluster side and the member cluster side.
It is a part of a series of PRs to enable status back-reporting in KubeFleet.
I have:
make reviewableto ensure this PR is ready for review.How has this code been tested
Special notes for your reviewer
To control the PR size, additional integration tests will be submitted in separate PRs.