Repository: rancher/fleet
Pull request #3027 (2024-10-29 15:07:22)

GitRepo resources list doesn't list resources multiple times
Refers to https://github.com/rancher/dashboard/issues/11643

This is a hotfix to avoid a conflict with UI code, which multiplies the resources list per cluster. Multiplying a GitRepo's resources from all clusters with the number of clusters potentially freezes the UI.

With this PR `gitrepo.Status.Resources` reports resources only once, even if they are deployed to multiple clusters. If any cluster has a nonready/modified status for the resource, it is still added to the `PerClusterState` field. 

That means, it's no longer possible to tell if a resource was not deployed to a cluster, e.g. because it was excluded via helm chart values.

Future code needs to respect that some resources are not present in each bundledeployment.
Users can for example use targetCustomization to deploy resources only to some clusters. Additionally, popular charts install different resources depending on the k8s version of the cluster. Some charts even allow to deploy "extraObjects" via a helm chart value.


* https://github.com/rancher/dashboard/blob/master/shell/models/fleet.cattle.io.gitrepo.js#L334-L339
* https://github.com/rancher/fleet/blob/main/internal/cmd/controller/gitops/reconciler/resourcekey.go#L38-L54



Patch:
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() {
+})