chore: ensure we patch RGD status for any Reconcile errors #702

michaelhtm · 2025-09-29T20:56:35Z

Description:
During RGD reconcile, if there are any issues when setting RGD CR as
managed or unmanaged, any issues that may occur are not reflected on the
RGD CR status.

As part of these changes, I'm introducing two new conditions.

Finalized
CleanedUp

Finalized will be marked to True if the controller was successfully able to add/remove the kro finalizer from the RGD.
If not, it will set it to false with the reason for error.

CleanedUp will mainly be used during RGD deletions. kro will set this condition to False if it was not able to do a clean up on RGD deletion. If the cleanup is successful, kro will set the condition to True

linux-foundation-easycla · 2025-09-29T20:56:42Z

The committers listed above are authorized under a signed CLA.

✅ login: michaelhtm / name: Michael Tewoldemedhin (41ad875, 57b20ba)

k8s-ci-robot · 2025-09-29T20:56:43Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: michaelhtm
Once this PR has been reviewed and has the lgtm label, please assign matthchr for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot · 2025-09-29T20:56:45Z

Hi @michaelhtm. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

barney-s · 2025-09-29T21:25:02Z

pkg/controller/resourcegraphdefinition/controller.go

+	var resourcesInformation []v1alpha1.ResourceInformation
+	defer func() {
+		if err := r.updateStatus(ctx, o, topologicalOrder, resourcesInformation); err != nil {
+			reconcileErr = errors.Join(reconcileErr, err)


Did your deferlinter catch this @jakobmoellerdev ?

yeah, https://github.com/jakobmoellerdev/deferrlint spotted this:

deferrlint ./... .../kro/pkg/controller/instance/controller_reconcile.go:120:6: deferred function assigns to error-typed variable `err`, but it is not a named return value .../kro/pkg/controller/resourcegraphdefinition/controller.go:166:6: deferred function assigns to error-typed variable `err`, but it is not a named return value

I will say this is a strange one and the linter doesnt detect this correctly though, I believe reconcileErr should be fine here because it is reassigned

pkg/controller/resourcegraphdefinition/controller.go

barney-s · 2025-09-29T21:45:00Z

/ok-to-test

jakobmoellerdev · 2025-09-30T07:21:10Z

pkg/controller/resourcegraphdefinition/controller.go

+			if !apierrors.IsNotFound(err) {
+				reconcileErr = errors.Join(reconcileErr, err)
+			} else {
+				reconcileErr = nil


why would you set reconcileErr to nil explicitly here? because youre using err there is no conflict anyhow right?

also I hate to be a nit picker here, but would it be possible to write a test to make sure the RGD status is patched on error case?

I wanted to add tests for patch behavior, but currently there's no package that mocks finalize failures.

why would you set reconcileErr to nil explicitly here?

The main reason was to avoid triggering another reconciliation if the resource no longer exists..

During RGD reconcile, if there are any issues when setting RGD CR as managed or unmanaged, any issues that may occur are not reflected on the RGD CR status. With these changes, we will ensure any issues are patched to the RGD status

cheeseandcereal · 2025-10-07T21:37:57Z

pkg/controller/resourcegraphdefinition/controller.go

+	defer func() {
+		// if the updateStatus error is a k8s NotFound error we want to skip it.
+		// If not, we will merge it to the returned error
+		if patchErr := r.updateStatus(ctx, o, topologicalOrder, resourcesInformation); patchErr != nil {


If this updateStatus is happening in a deferred func, now we ALWAYS will run this function. This does not match the behavior that exists below. For example, if !o.DeletionTimestamp.IsZero(), we run r.cleanupResourceGraphDefinition instead and then return.

With this change, we would run r.cleanupResourceGraphDefinition, try to return, and then try to run r.updateStatus afterwards which definitely isn't correct.

This change is mainly addressing issues when either cleanup or finalizer add/remove fails. In those cases, we want kro to report this information in the RGD status. This would allow users to see any issues in the resource status instead of looking for them in the logs..

But doesn't this now run even if the cleanup succeeds where it wasn't before?

It does. The information of the successful cleanup may be useful to users. Especially if the RGD has other finalizers that are preventing it from being removed.

Ok, so it's explicit/intentional that we are changing from only updating this sometime to always updating it. If that's the case then this makes sense

michaelhtm · 2025-10-07T21:11:48Z

api/v1alpha1/conditions.go

-const (
-	// ResourceGraphDefinitionConditionTypeGraphVerified indicates the state of the directed
-	// acyclic graph (DAG) that kro uses to manage the resources in a
-	// ResourceGraphDefinition.
-	ResourceGraphDefinitionConditionTypeGraphVerified ConditionType = "GraphVerified"
-	// ResourceGraphDefinitionConditionTypeCustomResourceDefinitionSynced indicates the state of the
-	// CustomResourceDefinition (CRD) that kro uses to manage the resources in a
-	// ResourceGraphDefinition.
-	ResourceGraphDefinitionConditionTypeCustomResourceDefinitionSynced ConditionType = "CustomResourceDefinitionSynced"
-	// ResourceGraphDefinitionConditionTypeReconcilerReady indicates the state of the reconciler.
-	// Whenever an ResourceGraphDefinition resource is created, kro will spin up a
-	// reconciler for that resource. This condition indicates the state of the
-	// reconciler.
-	ResourceGraphDefinitionConditionTypeReconcilerReady ConditionType = "ReconcilerReady"
-)
-


these are not used anywhere in the code base

michaelhtm · 2025-10-07T21:12:19Z

pkg/controller/resourcegraphdefinition/controller.go

+			if apierrors.IsNotFound(patchErr) {
+				// we don't want to return an error and requeue if 
+				// rgd is not found (deleted)
+				err = nil


ignore the error if resource no longer exists

michaelhtm · 2025-10-07T21:53:11Z

pkg/controller/resourcegraphdefinition/controller.go

+	defer func() {
+		// if the updateStatus error is a k8s NotFound error we want to skip it.
+		// If not, we will merge it to the returned error
+		if patchErr := r.updateStatus(ctx, o, topologicalOrder, resourcesInformation); patchErr != nil {


This change is mainly addressing issues when either cleanup or finalizer add/remove fails. In those cases, we want kro to report this information in the RGD status. This would allow users to see any issues in the resource status instead of looking for them in the logs..

fabianburth · 2025-10-12T13:43:33Z

pkg/controller/resourcegraphdefinition/controller.go

+		mark.ResourceGraphUnfinalized(err.Error())
 		return ctrl.Result{}, err
 	}
+	mark.ResourceGraphFinalized("added")


IMHO, ResourceGraphFinalized feels a little off here, semantically. Still rather a nit though, because the message describes it reasonably well.

fabianburth · 2025-10-12T13:48:47Z

pkg/controller/resourcegraphdefinition/controller.go

+		// if the updateStatus error is a k8s NotFound error we want to skip it.
+		// If not, we will merge it to the returned error
+		if patchErr := r.updateStatus(ctx, o, topologicalOrder, resourcesInformation); patchErr != nil {
+			if apierrors.IsNotFound(patchErr) {


Do we need an additional check for err == nil here, to prevent suppressing potential other errors? Or maybe at least log err if err != nil?

Suggested change

if apierrors.IsNotFound(patchErr) {

if apierrors.IsNotFound(patchErr) && err == nil {

k8s-ci-robot · 2025-10-16T08:00:25Z

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

k8s-ci-robot requested review from bridgetkromhout and jakobmoellerdev September 29, 2025 20:56

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 29, 2025

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Sep 29, 2025

michaelhtm mentioned this pull request Sep 29, 2025

Contributors Needing to Sign the Kubernetes CLA #668

Closed

barney-s reviewed Sep 29, 2025

View reviewed changes

pkg/controller/resourcegraphdefinition/controller.go Outdated Show resolved Hide resolved

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 29, 2025

michaelhtm force-pushed the chore/patchrgdstatusforanyerror branch from 8a13805 to 1f5e594 Compare September 29, 2025 22:49

bridgetkromhout mentioned this pull request Sep 30, 2025

Removing michaelhtm from NOTICE - signed CLA #703

Merged

michaelhtm force-pushed the chore/patchrgdstatusforanyerror branch 2 times, most recently from 44318ed to 3929110 Compare September 30, 2025 01:14

jakobmoellerdev reviewed Sep 30, 2025

View reviewed changes

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 2, 2025

michaelhtm force-pushed the chore/patchrgdstatusforanyerror branch from a80ed36 to 32114c6 Compare October 2, 2025 17:17

cheeseandcereal reviewed Oct 7, 2025

View reviewed changes

michaelhtm commented Oct 7, 2025

View reviewed changes

temp changes adding conditions

41ad875

michaelhtm force-pushed the chore/patchrgdstatusforanyerror branch from 32114c6 to 41ad875 Compare October 8, 2025 23:10

fabianburth reviewed Oct 12, 2025

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 16, 2025

	if apierrors.IsNotFound(patchErr) {
	if apierrors.IsNotFound(patchErr) && err == nil {

chore: ensure we patch RGD status for any Reconcile errors #702

Are you sure you want to change the base?

chore: ensure we patch RGD status for any Reconcile errors #702

Uh oh!

Conversation

michaelhtm commented Sep 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

linux-foundation-easycla bot commented Sep 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Sep 29, 2025

Uh oh!

k8s-ci-robot commented Sep 29, 2025

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!

barney-s commented Sep 29, 2025

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!

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!

k8s-ci-robot commented Oct 16, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

michaelhtm commented Sep 29, 2025 •

edited

Loading

linux-foundation-easycla bot commented Sep 29, 2025 •

edited

Loading