feat: add status back-reporting supprt (2/, new controller) #329

michaelawyu · 2025-11-13T17:33:44Z

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:

Run make reviewable to ensure this PR is ready for review.

How has this code been tested

Unit tests
Integration tests (limited)

Special notes for your reviewer

To control the PR size, additional integration tests will be submitted in separate PRs.

Signed-off-by: michaelawyu <chenyu1@microsoft.com>

michaelawyu · 2025-11-13T17:34:09Z

Note: as this controller requires other components to fully function; it is NOT currently enabled.

Signed-off-by: michaelawyu <chenyu1@microsoft.com>

codecov · 2025-11-13T18:14:51Z

Codecov Report

❌ Patch coverage is 72.91667% with 39 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
pkg/controllers/statusbackreporter/controller.go	72.91%	32 Missing and 7 partials ⚠️

📢 Thoughts on this report? Let us know!

weng271190436 · 2025-11-14T17:04:32Z

pkg/controllers/statusbackreporter/controller.go

+	}
+
+	// Validate the scheduling policy of the placement object.
+	schedulingPolicy := placementObj.GetPlacementSpec().Policy


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?

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.

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.

weng271190436 · 2025-11-14T17:08:58Z

pkg/controllers/statusbackreporter/controller.go

+// 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 {


this might be useful beyond this package? I am wondering if this belongs to the pkg/utils

similarly formatWorkResourceIdentifier might also belong there?

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).

I will add a TO-DO comment to note the possible factoring opportunity should the situation materializes.

pkg/controllers/statusbackreporter/controller.go

weng271190436 · 2025-11-14T17:18:21Z

pkg/controllers/statusbackreporter/controller.go

+	// 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)


Is this a user facing error that we should report using EventRecorder?

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.

weng271190436 · 2025-11-14T17:24:29Z

pkg/controllers/statusbackreporter/controller_integration_test.go

+	}
+)
+
+// createWorkObject creates a new Work object with the given name, manifests, and apply strategy.


Copy/paste error from workapplier/controller_integration_test.go? Should we consolidate the 2 functions?

Yeah, let me fix the comment. Thanks!

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

I made some edits that do some wrapping; it's a bit overkilling but I think it is better than introducing Gomega pkg into regular code.

weng271190436 · 2025-11-14T17:29:32Z

pkg/controllers/statusbackreporter/controller_integration_test.go

+	return statusBackReportingWrapperData, nil
+}
+
+func workObjectRemovedActual(workName string) func() error {


also a duplicated utility from workapplier/controller_integration_test.go

I moved them to a separate pkg though I have to say I have some minor reservation about this. It's nothing major, obviously, but this feels like adding another layer of redirection just for the sake of adding redirection. It just brings so little benefits (not really making the code more maintainable/readable) and at this moment does not seem to be a well-used pattern.

weng271190436 · 2025-11-14T19:19:12Z

pkg/controllers/statusbackreporter/controller.go

+
+	// Prepare a child context.
+	// Cancel the child context anyway to avoid leaks.
+	childCtx, cancel := context.WithCancel(ctx)


why create a childCtx instead of just passing ctx into ParallelizeUntil?

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...

pkg/controllers/statusbackreporter/controller_integration_test.go

pkg/controllers/statusbackreporter/suite_test.go

pkg/controllers/statusbackreporter/controller.go

ryanzhang-oss

i think we need an e2e to try something like a job

pkg/controllers/statusbackreporter/controller.go

ryanzhang-oss · 2025-12-09T00:53:59Z

pkg/controllers/statusbackreporter/controller.go

+		// Note that if the applied resource has a status sub-resource, it is usually safe for us to assume that
+		// the original resource should also have a status sub-resource of the same format.
+		unstructured.Object["status"] = statusWrapper["status"]
+		_, err = r.hubDynamicClient.Resource(gvr).Namespace(nsName).UpdateStatus(ctx, unstructured, metav1.UpdateOptions{})


I wonder this will trigger resource change dector to trigger another placement reconcile?

I think it will trigger a change detector action but will be ignored by placement controller when it finds out that there is no meaningful diff

Yeah, it will trigger the placement controller for once. We could update the onResourceUpdated method, but it would need a deep equal (current the method uses resource versions), which might be a bit too resource-consuming considering that this method is on the critical path? I could add a TO-DO here just in case we need to evaluate this possibility.

ryanzhang-oss · 2025-12-09T00:55:40Z

pkg/controllers/statusbackreporter/controller.go

+			// TO-DO (chenyu1): check for cases where the API definition is inconsistent between the member cluster
+			// side and the hub cluster side, and single out the errors as user errors instead.


good point, it seems that we probably should start small? like only start with job/deployment i.e. something with stable API.

Hi Ryan! Yeah, there will be a whitelist based mechanism that controls which APIs can have their status back-reported; as mentioned to Wei earlier this will be submitted as a separate PR.

ryanzhang-oss · 2025-12-09T01:01:36Z

pkg/controllers/statusbackreporter/controller.go

+
+		// Note that if the applied resource has a status sub-resource, it is usually safe for us to assume that
+		// the original resource should also have a status sub-resource of the same format.
+		unstructured.Object["status"] = statusWrapper["status"]


do we not check the GKV that we put in the statusWrapper?

Hi Ryan! The GVK/GVR we put in the wrapper are there only for completeness reasons (otherwise the serialization/deserialization would fail) -> the info are directly copied from the work resource identifier, which we already use in this controller.

michaelawyu · 2025-12-18T14:19:50Z

i think we need an e2e to try something like a job

Hi Ryan! Yeah; this controller has not been enabled yet -> will add E2Es in a separate PR.

Signed-off-by: michaelawyu <chenyu1@microsoft.com>

…o feat/status-backreporting-controller

Added status back-reporting controller

8504f8c

Signed-off-by: michaelawyu <chenyu1@microsoft.com>

Minor fixes

eb286f6

Signed-off-by: michaelawyu <chenyu1@microsoft.com>

weng271190436 reviewed Nov 14, 2025

View reviewed changes

ryanzhang-oss reviewed Dec 9, 2025

View reviewed changes

michaelawyu added 2 commits December 19, 2025 02:32

Minor fixes

df26f8b

Signed-off-by: michaelawyu <chenyu1@microsoft.com>

Merge branch 'main' of https://github.com/kubefleet-dev/kubefleet int…

b0066c9

…o feat/status-backreporting-controller

		// TO-DO (chenyu1): check for cases where the API definition is inconsistent between the member cluster
		// side and the hub cluster side, and single out the errors as user errors instead.

feat: add status back-reporting supprt (2/, new controller) #329

Are you sure you want to change the base?

feat: add status back-reporting supprt (2/, new controller) #329

Uh oh!

Conversation

michaelawyu commented Nov 13, 2025

Description of your changes

How has this code been tested

Special notes for your reviewer

Uh oh!

michaelawyu commented Nov 13, 2025

Uh oh!

codecov bot commented Nov 13, 2025

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaelawyu Dec 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ryanzhang-oss left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaelawyu commented Dec 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

michaelawyu Dec 18, 2025 •

edited

Loading