Repository: rancher/fleet
Pull request #1857 (2023-10-13 10:41:32)

Use index when listing BundleDeployments by Bundle
<!-- Specify the issue ID that this pull request is solving -->
Part of #1842

These changes add an indexer for BundleDeployments' Informer, which will build an index of BundleDeployment objects based on the Bundle that generated them (uniquely identified by their namespace + name). This provides an optimization over constantly using `List` with multiple label selectors.

This will be also back-ported to 0.7 and 0.8 to provide a fix for the original reporters.

## Test

Unit tests have been adapted to match the new code.

To test this pull request, you can run the following commands:

```shell
go test ./internal/cmd/controller/target
```

## Additional Information

### Trade-off

No expected side effects despite a marginal memory usage increase due to keeping the new index in memory.

### Potential improvement

CPU usage drop proportional to the number of bundles and downstream clusters.


Patch:
diff --git a/internal/cmd/controller/target/target.go b/internal/cmd/controller/target/target.go
index 0e3e9dfc..d362588c 100644
--- a/internal/cmd/controller/target/target.go
+++ b/internal/cmd/controller/target/target.go
@@ -8,6 +8,7 @@ package target
 import (
 	"bytes"
 	"fmt"
+	"slices"
 	"sort"
 	"strconv"
 	"strings"
@@ -45,6 +46,7 @@ var (
 const (
 	maxTemplateRecursionDepth = 50
 	clusterLabelPrefix        = "global.fleet.clusterLabels."
+	byBundleIndexerName       = "fleet.byBundle"
 )
 
 type Manager struct {
@@ -65,6 +67,16 @@ func New(
 	namespaceCache corecontrollers.NamespaceCache,
 	contentStore manifest.Store,
 	bundleDeployments fleetcontrollers.BundleDeploymentCache) *Manager {
+	bundleDeployments.AddIndexer(byBundleIndexerName, func(bd *fleet.BundleDeployment) ([]string, error) {
+		if bdLabels := bd.GetLabels(); bdLabels != nil {
+			bundleNamespace := bdLabels[fleet.BundleNamespaceLabel]
+			bundleName := bdLabels[fleet.BundleLabel]
+			if bundleNamespace != "" && bundleName != "" {
+				return []string{bundleNamespace + "/" + bundleName}, nil
+			}
+		}
+		return nil, nil
+	})
 
 	return &Manager{
 		clusterGroups:               clusterGroups,
@@ -193,15 +205,13 @@ func (m *Manager) BundlesForCluster(cluster *fleet.Cluster) (bundlesToRefresh, b
 }
 
 func (m *Manager) GetBundleDeploymentsForBundleInCluster(bundle *fleet.Bundle, cluster *fleet.Cluster) (result []*fleet.BundleDeployment, err error) {
-	bundleDeployments, err := m.bundleDeploymentCache.List("", labels.SelectorFromSet(labels.Set{
-		fleet.BundleLabel:          bundle.Name,
-		fleet.BundleNamespaceLabel: bundle.Namespace,
-		fleet.ClusterLabel:         cluster.Name,
-	}))
-
+	bundleDeployments, err := m.bundleDeploymentCache.GetByIndex(byBundleIndexerName, bundleIndexKey(bundle))
 	if err != nil {
 		return nil, err
 	}
+	bundleDeployments = slices.DeleteFunc(bundleDeployments, func(bd *fleet.BundleDeployment) bool {
+		return bd.Labels[fleet.ClusterLabel] != cluster.Name
+	})
 
 	return bundleDeployments, nil
 }
@@ -378,10 +388,7 @@ func toDict(values map[string]string) map[string]interface{} {
 
 // foldInDeployments adds the existing bundledeployments to the targets.
 func (m *Manager) foldInDeployments(bundle *fleet.Bundle, targets []*Target) error {
-	bundleDeployments, err := m.bundleDeploymentCache.List("", labels.SelectorFromSet(labels.Set{
-		fleet.BundleLabel:          bundle.Name,
-		fleet.BundleNamespaceLabel: bundle.Namespace,
-	}))
+	bundleDeployments, err := m.bundleDeploymentCache.GetByIndex(byBundleIndexerName, bundleIndexKey(bundle))
 	if err != nil {
 		return err
 	}
@@ -398,6 +405,10 @@ func (m *Manager) foldInDeployments(bundle *fleet.Bundle, targets []*Target) err
 	return nil
 }
 
+func bundleIndexKey(bundle *fleet.Bundle) string {
+	return bundle.Namespace + "/" + bundle.Name
+}
+
 type Target struct {
 	Deployment    *fleet.BundleDeployment
 	ClusterGroups []*fleet.ClusterGroup


Test patch:
diff --git a/internal/cmd/controller/target/target_test.go b/internal/cmd/controller/target/target_test.go
index d0779f70..8620a161 100644
--- a/internal/cmd/controller/target/target_test.go
+++ b/internal/cmd/controller/target/target_test.go
@@ -3,6 +3,7 @@ package target
 //go:generate mockgen --build_flags=--mod=mod -destination=../mocks/bundle_deployment_cache_mock.go -package=mocks github.com/rancher/fleet/pkg/generated/controllers/fleet.cattle.io/v1alpha1 BundleDeploymentCache
 
 import (
+	"fmt"
 	"testing"
 
 	"github.com/golang/mock/gomock"
@@ -10,7 +11,6 @@ import (
 	"github.com/rancher/wrangler/pkg/yaml"
 	"github.com/stretchr/testify/assert"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/labels"
 
 	"github.com/rancher/fleet/internal/cmd/controller/mocks"
 	"github.com/rancher/fleet/pkg/apis/fleet.cattle.io/v1alpha1"
@@ -473,6 +473,39 @@ func TestDisablePreProcessFlagMissing(t *testing.T) {
 }
 
 func TestGetBundleDeploymentForBundleInCluster(t *testing.T) {
+	bundleDeployment := func(namespace, clusterName string) *v1alpha1.BundleDeployment {
+		return &v1alpha1.BundleDeployment{
+			ObjectMeta: metav1.ObjectMeta{
+				Namespace: fmt.Sprintf("cluster-%s-%s-1df72965a9b5", namespace, clusterName),
+				Labels: map[string]string{
+					"fleet.cattle.io/cluster": clusterName,
+				},
+			},
+		}
+	}
+	bundle := func(name, namespace string) *v1alpha1.Bundle {
+		return &v1alpha1.Bundle{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      name,
+				Namespace: namespace,
+			},
+		}
+	}
+	cluster := func(name string) *v1alpha1.Cluster {
+		return &v1alpha1.Cluster{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: name,
+			},
+		}
+	}
+	bundleName := "my-bundle"
+	bundleNamespace := "fleet-default"
+	clusterName := "my-cluster"
+	bundleDeployments := []*v1alpha1.BundleDeployment{
+		bundleDeployment(bundleNamespace, clusterName),
+		bundleDeployment(bundleNamespace, "another-cluster"),
+	}
+
 	testCases := []struct {
 		name                       string
 		bundleName                 string
@@ -484,34 +517,27 @@ func TestGetBundleDeploymentForBundleInCluster(t *testing.T) {
 	}{
 		{
 			name:            "returns listed bundle deployments",
-			bundleName:      "my-bundle-my-cluster",
-			bundleNamespace: "fleet-default",
-			clusterName:     "my-cluster",
+			bundleName:      bundleName,
+			bundleNamespace: bundleNamespace,
+			clusterName:     clusterName,
 			expectedBundleDeployments: []*v1alpha1.BundleDeployment{
-				{
-					ObjectMeta: metav1.ObjectMeta{
-						Namespace: "cluster-fleet-default-my-cluster-1df72965a9b5",
-						Labels: map[string]string{
-							"fleet.cattle.io/cluster": "my-cluster",
-						},
-					},
-				},
+				bundleDeployment(bundleNamespace, clusterName),
 			},
 		},
 		{
 			name:                       "returns error from bundle deployment cache listing",
-			bundleName:                 "my-bundle-my-cluster",
-			bundleNamespace:            "fleet-default",
+			bundleName:                 bundleName,
+			bundleNamespace:            bundleNamespace,
 			listBundleDeploymentsError: errors.New("something happened"),
 			expectedBundleDeployments:  nil,
 			wantError:                  true,
 		},
 		{
 			name:                      "returns no bundle deployments when none are listed",
-			bundleName:                "my-bundle-my-cluster",
-			bundleNamespace:           "fleet-default",
-			clusterName:               "my-cluster",
-			expectedBundleDeployments: nil,
+			bundleName:                bundleName,
+			bundleNamespace:           bundleNamespace,
+			clusterName:               "yet-another-cluster",
+			expectedBundleDeployments: []*v1alpha1.BundleDeployment{},
 		},
 	}
 
@@ -521,27 +547,12 @@ func TestGetBundleDeploymentForBundleInCluster(t *testing.T) {
 
 			mockBundleDeploymentCache := mocks.NewMockBundleDeploymentCache(ctrl)
 
-			bundle := v1alpha1.Bundle{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:      tc.bundleName,
-					Namespace: tc.bundleNamespace,
-				},
-			}
-
-			cluster := v1alpha1.Cluster{
-				ObjectMeta: metav1.ObjectMeta{
-					Name: tc.clusterName,
-				},
-			}
-
-			mockBundleDeploymentCache.EXPECT().List("", labels.SelectorFromSet(labels.Set{
-				v1alpha1.BundleLabel:          bundle.Name,
-				v1alpha1.BundleNamespaceLabel: bundle.Namespace,
-				v1alpha1.ClusterLabel:         cluster.Name,
-			})).Return(tc.expectedBundleDeployments, tc.listBundleDeploymentsError)
+			mockBundleDeploymentCache.EXPECT().AddIndexer(byBundleIndexerName, gomock.Any())
+			mockBundleDeploymentCache.EXPECT().GetByIndex(byBundleIndexerName, fmt.Sprintf("%s/%s", tc.bundleNamespace, tc.bundleName)).
+				Return(bundleDeployments, tc.listBundleDeploymentsError)
 
 			manager := New(nil, nil, nil, nil, nil, nil, mockBundleDeploymentCache)
-			result, err := manager.GetBundleDeploymentsForBundleInCluster(&bundle, &cluster)
+			result, err := manager.GetBundleDeploymentsForBundleInCluster(bundle(tc.bundleName, tc.bundleNamespace), cluster(tc.clusterName))
 
 			if tc.wantError {
 				assert.Error(t, err)