Change GitRepo deletion. #4361

0xavi0 · 2025-11-24T11:23:05Z

This PR introduces changes to how GitRepos are deleted, as well as their dependent resources.

Two main changes are introduced:

The function that deletes the GitRepo has been modified to check for any pending resources, especially Bundles and ImageScans, since previously the GitRepo was removed before these resources were fully deleted.
A new controller is introduced for Content resources to track the number of BundleDeployments that reference the same one. This controller aims to eliminate the conflicts that occurred when updating or deleting BundleDeployments, as they were being updated or removed concurrently while modifying a shared Content resource.

With these changes, the goal is to eliminate both update conflicts (which triggered additional Reconcile calls in the controllers) and potential race conditions when deleting and recreating GitRepos that still had dependent resources pending deletion.

Refers to #4192
Refers to #4251

Additional Information

Checklist

- [ ] I have updated the documentation via a pull request in the
fleet-docs repository.

This PR introduces changes to how GitRepos are deleted, as well as their dependent resources. Two main changes are introduced: * The function that deletes the GitRepo has been modified to check for any pending resources, especially *Bundles* and *ImageScans*, since previously the *GitRepo* was removed before these resources were fully deleted. * A new controller is introduced for *Content* resources to track the number of *BundleDeployments* that reference the same one. This controller aims to eliminate the conflicts that occurred when updating or deleting *BundleDeployments*, as they were being updated or removed concurrently while modifying a shared *Content* resource. With these changes, the goal is to eliminate both update conflicts (which triggered additional Reconcile calls in the controllers) and potential race conditions when deleting and recreating *GitRepos* that still had dependent resources pending deletion. Signed-off-by: Xavi Garcia <[email protected]>

weyfonk

This feels cleaner than the previous approach, if slightly more "manual".
Finalizers were not the right tool for the job, it seems :)

weyfonk · 2025-11-27T14:24:07Z

pkg/apis/fleet.cattle.io/v1alpha1/content_types.go

 	// resource, so the downstream agent can deploy them.
 	// +nullable
-	Content []byte `json:"content,omitempty"`
+	Content []byte `json:"content,omitempty"` // Content is a byte array, which contains the manifests of a bundle. The bundle resources are copied into the bundledeployment's content resource, so the downstream agent can deploy them.


Is this second sentence needed here? It already appears on L27-28.

weyfonk · 2025-11-27T16:09:37Z

internal/cmd/controller/root.go

 		workersOpts.Schedule = w
 	}

+	if d := os.Getenv("CONTENT_RECONCILER_WORKERS"); d != "" {


This env var should probably be set in the fleet-controller deployment.

weyfonk · 2025-11-27T16:26:24Z

internal/cmd/controller/gitops/reconciler/gitjob_controller.go

+func GetNamespaceAndNameID(namespace, name string) string {
+	return fmt.Sprintf("%s/%s", namespace, name)
+}


If we get rid of the commented out code above, then this function is no longer needed.

weyfonk · 2025-11-27T16:32:09Z

internal/cmd/controller/gitops/reconciler/gitjob_controller.go

+		return nil // continue iterating no matter what
+	})
+
+	return errors.Join(errs...)


This will format errors with a newline between each error message (skipping nil errors). I'm not sure how well that would work in the Rancher UI.
This may not be critical, but perhaps something to test.

weyfonk · 2025-11-27T16:36:58Z

internal/cmd/controller/operator.go

+		ShardID: shardID,
+		Workers: workersOpts.Content,
+	}).SetupWithManager(mgr); err != nil {
+		setupLog.Error(err, "unable to content controller", "controller", "Content")


This typo is very relatable :D

Suggested change

setupLog.Error(err, "unable to content controller", "controller", "Content")

setupLog.Error(err, "unable to create controller", "controller", "Content")

weyfonk · 2025-11-27T16:40:44Z

internal/cmd/controller/reconciler/bundle_controller.go

 		bd.Spec.ValuesHash = valuesHash

+		// When content resources are stored in etcd, we need to keep track of the content resource so they
+		// are properly gargabe collected by the content controller.


nit:

Suggested change

// are properly gargabe collected by the content controller.

// are properly garbage-collected by the content controller.

weyfonk · 2025-11-27T16:47:59Z

internal/cmd/controller/reconciler/content_controller.go

+// SetupWithManager sets up the controller with the Manager.
+func (r *ContentReconciler) SetupWithManager(mgr ctrl.Manager) error {
+	return ctrl.NewControllerManagedBy(mgr).
+		Named("content-controller").


The docs for this state:

By default, controllers are named using the lowercase version of their kind.

Except for a few (drift, gitrepo status and helmops status), we don't name our reconcilers. We may want to harmonise their names for metrics at some point.

In the meantime, for consistency, I'd be tempted to set:

Suggested change

Named("content-controller").

Named("content"). // equivalent to the default, as per the docs, so we might as well skip this line

or

Suggested change

Named("content-controller").

Named("content-reconciler"). // same format as in the drift controller

That being said, I'm not sure how useful metrics for this controller would be, so that's definitely not a major issue ;)

weyfonk · 2025-11-27T16:58:01Z

internal/cmd/controller/reconciler/bundle_controller.go

 ) (controllerutil.OperationResult, *fleet.BundleDeployment, error) {
 	logger := l.WithValues("deploymentID", bd.Spec.DeploymentID)

-	// When content resources are stored in etcd, we need to add finalizers.


We will want to update unit tests for this controller (e.g. here), to reflect the fact that content management is now owned by another controller.

weyfonk · 2025-11-27T16:58:48Z

internal/cmd/controller/reconciler/content_controller.go

+		}
+	}
+
+	return ctrl.Result{}, nil


It could be nice to (unit-)test this, just to be on the safe side. WDYT?

0xavi0 self-assigned this Nov 24, 2025

0xavi0 added the kind/bug label Nov 24, 2025

0xavi0 force-pushed the 4192-gitrepo-delete-cascade branch from a24d029 to fcc8657 Compare November 24, 2025 11:33

0xavi0 force-pushed the 4192-gitrepo-delete-cascade branch from fcc8657 to f59b831 Compare November 24, 2025 12:43

0xavi0 added this to Fleet Nov 24, 2025

0xavi0 added this to the v2.13.1 milestone Nov 24, 2025

0xavi0 marked this pull request as ready for review November 24, 2025 13:09

0xavi0 requested a review from a team as a code owner November 24, 2025 13:09

0xavi0 moved this to Needs QA review in Fleet Nov 25, 2025

0xavi0 moved this from Needs QA review to 👀 In review in Fleet Nov 25, 2025

weyfonk reviewed Nov 27, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Change GitRepo deletion. #4361

Change GitRepo deletion. #4361

0xavi0 commented Nov 24, 2025 •

edited

Loading

Uh oh!

weyfonk left a comment

Uh oh!

weyfonk Nov 27, 2025

Uh oh!

weyfonk Nov 27, 2025

Uh oh!

weyfonk Nov 27, 2025

Uh oh!

weyfonk Nov 27, 2025

Uh oh!

weyfonk Nov 27, 2025

Uh oh!

weyfonk Nov 27, 2025

Uh oh!

weyfonk Nov 27, 2025

Uh oh!

weyfonk Nov 27, 2025

Uh oh!

weyfonk Nov 27, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	setupLog.Error(err, "unable to content controller", "controller", "Content")
	setupLog.Error(err, "unable to create controller", "controller", "Content")

	// are properly gargabe collected by the content controller.
	// are properly garbage-collected by the content controller.

	Named("content-controller").
	Named("content"). // equivalent to the default, as per the docs, so we might as well skip this line

	Named("content-controller").
	Named("content-reconciler"). // same format as in the drift controller

Change GitRepo deletion. #4361

Are you sure you want to change the base?

Change GitRepo deletion. #4361

Conversation

0xavi0 commented Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Additional Information

Checklist

Uh oh!

weyfonk left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

0xavi0 commented Nov 24, 2025 •

edited

Loading