Repository: rancher/fleet
Pull request #3032 (2024-10-30 08:48:27)

Patch bundle deployment status in drift controller
This prevents that new controller from making heavy-handed updates to bundle deployment statuses, reducing the risk of conflicts, as already done for the bundle deployment reconciler.

Follow-up to #3021
Refers to #3020


Patch:
diff --git a/internal/cmd/agent/controller/drift_controller.go b/internal/cmd/agent/controller/drift_controller.go
index 18f39f41..d60ed6a5 100644
--- a/internal/cmd/agent/controller/drift_controller.go
+++ b/internal/cmd/agent/controller/drift_controller.go
@@ -13,9 +13,7 @@ import (
 
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/runtime"
-	"k8s.io/apimachinery/pkg/types"
 	errutil "k8s.io/apimachinery/pkg/util/errors"
-	"k8s.io/client-go/util/retry"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/event"
@@ -65,6 +63,8 @@ func (r *DriftReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl
 		return ctrl.Result{}, err
 	}
 
+	orig := bd.DeepCopy()
+
 	if bd.Spec.Paused {
 		logger.V(1).Info("Bundle paused, clearing drift detection")
 		err := r.DriftDetect.Clear(req.String())
@@ -106,7 +106,7 @@ func (r *DriftReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl
 
 	// final status update
 	logger.V(1).Info("Reconcile finished, updating the bundledeployment status")
-	err = r.updateStatus(ctx, req.NamespacedName, bd.Status)
+	err = r.updateStatus(ctx, orig, bd)
 	if apierrors.IsNotFound(err) {
 		merr = append(merr, fmt.Errorf("bundledeployment has been deleted: %w", err))
 	} else if err != nil {
@@ -116,14 +116,11 @@ func (r *DriftReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl
 	return ctrl.Result{}, errutil.NewAggregate(merr)
 }
 
-func (r *DriftReconciler) updateStatus(ctx context.Context, req types.NamespacedName, status fleetv1.BundleDeploymentStatus) error {
-	return retry.RetryOnConflict(DefaultRetry, func() error {
-		newBD := &fleetv1.BundleDeployment{}
-		err := r.Get(ctx, req, newBD)
-		if err != nil {
-			return err
-		}
-		newBD.Status = status
-		return r.Status().Update(ctx, newBD)
-	})
+func (r *DriftReconciler) updateStatus(
+	ctx context.Context,
+	orig *fleetv1.BundleDeployment,
+	obj *fleetv1.BundleDeployment,
+) error {
+	statusPatch := client.MergeFrom(orig)
+	return r.Status().Patch(ctx, obj, statusPatch)
 }
diff --git a/internal/cmd/controller/gitops/reconciler/resourcekey.go b/internal/cmd/controller/gitops/reconciler/resourcekey.go
index 81d4cad4..25f610e3 100644
--- a/internal/cmd/controller/gitops/reconciler/resourcekey.go
+++ b/internal/cmd/controller/gitops/reconciler/resourcekey.go
@@ -9,34 +9,61 @@ import (
 )
 
 func setResources(list *fleet.BundleDeploymentList, gitrepo *fleet.GitRepo) {
-	state := bundleErrorState(gitrepo.Status.Summary)
-	gitrepo.Status.Resources, gitrepo.Status.ResourceErrors = fromResources(list, state)
-	gitrepo.Status = countResources(gitrepo.Status)
+	s := summaryState(gitrepo.Status.Summary)
+	r, errors := fromResources(list, s)
+	gitrepo.Status.ResourceErrors = errors
+	gitrepo.Status.ResourceCounts = countResources(r)
+	gitrepo.Status.Resources = merge(r)
 }
 
-func bundleErrorState(summary fleet.BundleSummary) string {
-	bundleErrorState := ""
+func merge(resources []fleet.GitRepoResource) []fleet.GitRepoResource {
+	merged := map[string]fleet.GitRepoResource{}
+	for _, resource := range resources {
+		key := key(resource)
+		if existing, ok := merged[key]; ok {
+			existing.PerClusterState = append(existing.PerClusterState, resource.PerClusterState...)
+			merged[key] = existing
+		} else {
+			merged[key] = resource
+		}
+	}
+
+	var result []fleet.GitRepoResource
+	for _, resource := range merged {
+		result = append(result, resource)
+	}
+
+	sort.Slice(result, func(i, j int) bool {
+		return key(result[i]) < key(result[j])
+	})
+	return result
+}
+
+func key(resource fleet.GitRepoResource) string {
+	return resource.Type + "/" + resource.ID
+}
+
+func summaryState(summary fleet.BundleSummary) string {
 	if summary.WaitApplied > 0 {
-		bundleErrorState = "WaitApplied"
+		return "WaitApplied"
 	}
 	if summary.ErrApplied > 0 {
-		bundleErrorState = "ErrApplied"
+		return "ErrApplied"
 	}
-	return bundleErrorState
+	return ""
 }
 
-// fromResources lists all bundledeployments for this GitRepo and returns a list of
-// GitRepoResource states for all resources
+// fromResources inspects all bundledeployments for this GitRepo and returns a list of
+// GitRepoResources and error messages.
 //
 // It populates gitrepo status resources from bundleDeployments. BundleDeployment.Status.Resources is the list of deployed resources.
-func fromResources(list *fleet.BundleDeploymentList, bundleErrorState string) ([]fleet.GitRepoResource, []string) {
+func fromResources(list *fleet.BundleDeploymentList, summaryState string) ([]fleet.GitRepoResource, []string) {
 	var (
 		resources []fleet.GitRepoResource
 		errors    []string
 	)
 
 	for _, bd := range list.Items {
-		bd := bd // fix gosec warning regarding "Implicit memory aliasing in for loop"
 		bdResources := bundleDeploymentResources(bd)
 		incomplete, err := addState(bd, bdResources)
 
@@ -47,32 +74,29 @@ func fromResources(list *fleet.BundleDeploymentList, bundleErrorState string) ([
 			}
 		}
 
-		for k, state := range bdResources {
-			resource := toResourceState(k, state, incomplete, bundleErrorState)
+		for k, perCluster := range bdResources {
+			resource := toResourceState(k, perCluster, incomplete, summaryState)
 			resources = append(resources, resource)
 		}
 	}
 
 	sort.Strings(errors)
-	sort.Slice(resources, func(i, j int) bool {
-		return resources[i].Type+"/"+resources[i].ID < resources[j].Type+"/"+resources[j].ID
-	})
 
 	return resources, errors
 }
 
-func toResourceState(k fleet.ResourceKey, state []fleet.ResourcePerClusterState, incomplete bool, bundleErrorState string) fleet.GitRepoResource {
+func toResourceState(k fleet.ResourceKey, perCluster []fleet.ResourcePerClusterState, incomplete bool, summaryState string) fleet.GitRepoResource {
 	resource := fleet.GitRepoResource{
 		APIVersion:      k.APIVersion,
 		Kind:            k.Kind,
 		Namespace:       k.Namespace,
 		Name:            k.Name,
 		IncompleteState: incomplete,
-		PerClusterState: state,
+		PerClusterState: perCluster,
 	}
 	resource.Type, resource.ID = toType(resource)
 
-	for _, state := range state {
+	for _, state := range perCluster {
 		resource.State = state.State
 		resource.Error = state.Error
 		resource.Transitioning = state.Transitioning
@@ -80,22 +104,23 @@ func toResourceState(k fleet.ResourceKey, state []fleet.ResourcePerClusterState,
 		break
 	}
 
+	// fallback to state from gitrepo summary
 	if resource.State == "" {
 		if resource.IncompleteState {
-			if bundleErrorState != "" {
-				resource.State = bundleErrorState
+			if summaryState != "" {
+				resource.State = summaryState
 			} else {
 				resource.State = "Unknown"
 			}
-		} else if bundleErrorState != "" {
-			resource.State = bundleErrorState
+		} else if summaryState != "" {
+			resource.State = summaryState
 		} else {
 			resource.State = "Ready"
 		}
 	}
 
-	sort.Slice(state, func(i, j int) bool {
-		return state[i].ClusterID < state[j].ClusterID
+	sort.Slice(perCluster, func(i, j int) bool {
+		return perCluster[i].ClusterID < perCluster[j].ClusterID
 	})
 	return resource
 }
@@ -114,6 +139,9 @@ func toType(resource fleet.GitRepoResource) (string, string) {
 	return t, resource.Namespace + "/" + resource.Name
 }
 
+// addState adds per-cluster state information for nonReady and modified resources in a bundleDeployment.
+// It only adds up to 10 entries to not overwhelm the status.
+// It mutates resources and returns whether the reported state is incomplete and any errors encountered.
 func addState(bd fleet.BundleDeployment, resources map[fleet.ResourceKey][]fleet.ResourcePerClusterState) (bool, []error) {
 	var (
 		incomplete bool
@@ -200,28 +228,28 @@ func bundleDeploymentResources(bd fleet.BundleDeployment) map[fleet.ResourceKey]
 	return bdResources
 }
 
-func countResources(status fleet.GitRepoStatus) fleet.GitRepoStatus {
-	status.ResourceCounts = fleet.GitRepoResourceCounts{}
+func countResources(resources []fleet.GitRepoResource) fleet.GitRepoResourceCounts {
+	counts := fleet.GitRepoResourceCounts{}
 
-	for _, resource := range status.Resources {
-		status.ResourceCounts.DesiredReady++
+	for _, resource := range resources {
+		counts.DesiredReady++
 		switch resource.State {
 		case "Ready":
-			status.ResourceCounts.Ready++
+			counts.Ready++
 		case "WaitApplied":
-			status.ResourceCounts.WaitApplied++
+			counts.WaitApplied++
 		case "Modified":
-			status.ResourceCounts.Modified++
+			counts.Modified++
 		case "Orphan":
-			status.ResourceCounts.Orphaned++
+			counts.Orphaned++
 		case "Missing":
-			status.ResourceCounts.Missing++
+			counts.Missing++
 		case "Unknown":
-			status.ResourceCounts.Unknown++
+			counts.Unknown++
 		default:
-			status.ResourceCounts.NotReady++
+			counts.NotReady++
 		}
 	}
 
-	return status
+	return counts
 }


Test patch:
diff --git a/internal/cmd/controller/gitops/reconciler/resourcekey_test.go b/internal/cmd/controller/gitops/reconciler/resourcekey_test.go
new file mode 100644
index 00000000..b53839ee
--- /dev/null
+++ b/internal/cmd/controller/gitops/reconciler/resourcekey_test.go
@@ -0,0 +1,173 @@
+package reconciler
+
+import (
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
+	fleet "github.com/rancher/fleet/pkg/apis/fleet.cattle.io/v1alpha1"
+	"github.com/rancher/fleet/pkg/apis/fleet.cattle.io/v1alpha1/summary"
+)
+
+var _ = Describe("Resourcekey", func() {
+	var (
+		gitrepo *fleet.GitRepo
+		list    *fleet.BundleDeploymentList
+	)
+
+	BeforeEach(func() {
+		gitrepo = &fleet.GitRepo{
+			Status: fleet.GitRepoStatus{
+				Summary: fleet.BundleSummary{
+					Ready:       2,
+					WaitApplied: 1,
+				},
+			},
+		}
+		list = &fleet.BundleDeploymentList{
+			Items: []fleet.BundleDeployment{
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "bd1",
+						Labels: map[string]string{
+							fleet.RepoLabel:             "gitrepo1",
+							fleet.ClusterLabel:          "cluster1",
+							fleet.ClusterNamespaceLabel: "c-ns1",
+						},
+					},
+					Status: fleet.BundleDeploymentStatus{
+						Resources: []fleet.BundleDeploymentResource{
+							{
+								Kind:       "Deployment",
+								APIVersion: "v1",
+								Name:       "web",
+								Namespace:  "default",
+							},
+							{
+								// extra service for one cluster
+								Kind:       "Service",
+								APIVersion: "v1",
+								Name:       "web-svc",
+								Namespace:  "default",
+							},
+						},
+					},
+				},
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "bd1",
+						Labels: map[string]string{
+							fleet.RepoLabel:             "gitrepo1",
+							fleet.ClusterLabel:          "cluster2",
+							fleet.ClusterNamespaceLabel: "c-ns1",
+						},
+					},
+					Status: fleet.BundleDeploymentStatus{
+						Resources: []fleet.BundleDeploymentResource{
+							{
+								Kind:       "Deployment",
+								APIVersion: "v1",
+								Name:       "web",
+								Namespace:  "default",
+							},
+						},
+					},
+				},
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "bd1",
+						Labels: map[string]string{
+							fleet.RepoLabel:             "gitrepo1",
+							fleet.ClusterLabel:          "cluster1",
+							fleet.ClusterNamespaceLabel: "c-ns2",
+						},
+					},
+					Status: fleet.BundleDeploymentStatus{
+						NonReadyStatus: []fleet.NonReadyStatus{
+							{
+								Kind:       "Deployment",
+								APIVersion: "v1",
+								Name:       "web",
+								Namespace:  "default",
+								Summary: summary.Summary{
+									State:         "Pending",
+									Error:         true,
+									Transitioning: true,
+									Message:       []string{"message1", "message2"},
+								},
+							},
+						},
+						Resources: []fleet.BundleDeploymentResource{
+							{
+								Kind:       "Deployment",
+								APIVersion: "v1",
+								Name:       "web",
+								Namespace:  "default",
+							},
+						},
+					},
+				},
+			},
+		}
+
+	})
+
+	It("returns a list", func() {
+		setResources(list, gitrepo)
+
+		Expect(gitrepo.Status.Resources).To(HaveLen(2))
+		Expect(gitrepo.Status.Resources).To(ContainElement(fleet.GitRepoResource{
+			APIVersion: "v1",
+			Kind:       "Deployment",
+			Type:       "deployment",
+			ID:         "default/web",
+
+			Namespace: "default",
+			Name:      "web",
+
+			IncompleteState: false,
+			State:           "WaitApplied",
+			Error:           false,
+			Transitioning:   false,
+			Message:         "",
+			PerClusterState: []fleet.ResourcePerClusterState{
+				{
+					State:         "Pending",
+					ClusterID:     "c-ns2/cluster1",
+					Error:         true,
+					Transitioning: true,
+					Message:       "message1; message2",
+					Patch:         nil,
+				},
+			},
+		}))
+		Expect(gitrepo.Status.Resources).To(ContainElement(fleet.GitRepoResource{
+			APIVersion: "v1",
+			Kind:       "Service",
+			Type:       "service",
+			ID:         "default/web-svc",
+
+			Namespace: "default",
+			Name:      "web-svc",
+
+			IncompleteState: false,
+			State:           "WaitApplied",
+			Error:           false,
+			Transitioning:   false,
+			Message:         "",
+			PerClusterState: []fleet.ResourcePerClusterState{},
+		}))
+
+		Expect(gitrepo.Status.ResourceErrors).To(BeEmpty())
+
+		Expect(gitrepo.Status.ResourceCounts.Ready).To(Equal(0))
+		Expect(gitrepo.Status.ResourceCounts.DesiredReady).To(Equal(4))
+		Expect(gitrepo.Status.ResourceCounts.WaitApplied).To(Equal(3))
+		Expect(gitrepo.Status.ResourceCounts.Modified).To(Equal(0))
+		Expect(gitrepo.Status.ResourceCounts.Orphaned).To(Equal(0))
+		Expect(gitrepo.Status.ResourceCounts.Missing).To(Equal(0))
+		Expect(gitrepo.Status.ResourceCounts.Unknown).To(Equal(0))
+		Expect(gitrepo.Status.ResourceCounts.NotReady).To(Equal(1))
+	})
+})
diff --git a/internal/cmd/controller/gitops/reconciler/suite_test.go b/internal/cmd/controller/gitops/reconciler/suite_test.go
new file mode 100644
index 00000000..82ce4c57
--- /dev/null
+++ b/internal/cmd/controller/gitops/reconciler/suite_test.go
@@ -0,0 +1,16 @@
+package reconciler
+
+import (
+	"testing"
+
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+)
+
+func TestGitOpsReconciler(t *testing.T) {
+	RegisterFailHandler(Fail)
+	RunSpecs(t, "GitOps Reconciler Suite")
+}
+
+var _ = BeforeSuite(func() {
+})