Merge pull request #6 from solo-io/release-0.16-chore/carry-labels-perf-regression

[release-0.16] 🏃 Make client.MatchingLabels faster

Author: timflannagan

pkg/cache/cache.go

@@ -22,8 +22,10 @@ import (
 	"net/http"
 	"time"
 
+	"golang.org/x/exp/maps"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/meta"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/fields"
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -121,6 +123,10 @@ type Informer interface {
 	HasSynced() bool
 }
 
+// AllNamespaces should be used as the map key to deliminate namespace settings
+// that apply to all namespaces that themselves do not have explicit settings.
+const AllNamespaces = metav1.NamespaceAll
+
 // Options are the optional arguments for creating a new Cache object.
 type Options struct {
 	// HTTPClient is the http client to use for the REST client
@@ -172,6 +178,11 @@ type Options struct {
 	// the namespaces in here will be watched and it will by used to default
 	// ByObject.Namespaces for all objects if that is nil.
 	//
+	// It is possible to have specific Config for just some namespaces
+	// but cache all namespaces by using the AllNamespaces const as the map key.
+	// This will then include all namespaces that do not have a more specific
+	// setting.
+	//
 	// The options in the Config that are nil will be defaulted from
 	// the respective Default* settings.
 	DefaultNamespaces map[string]Config
@@ -214,6 +225,11 @@ type ByObject struct {
 	// Settings in the map value that are unset will be defaulted.
 	// Use an empty value for the specific setting to prevent that.
 	//
+	// It is possible to have specific Config for just some namespaces
+	// but cache all namespaces by using the AllNamespaces const as the map key.
+	// This will then include all namespaces that do not have a more specific
+	// setting.
+	//
 	// A nil map allows to default this to the cache's DefaultNamespaces setting.
 	// An empty map prevents this and means that all namespaces will be cached.
 	//
@@ -392,6 +408,9 @@ func defaultOpts(config *rest.Config, opts Options) (Options, error) {
 
 	for namespace, cfg := range opts.DefaultNamespaces {
 		cfg = defaultConfig(cfg, optionDefaultsToConfig(&opts))
+		if namespace == metav1.NamespaceAll {
+			cfg.FieldSelector = fields.AndSelectors(appendIfNotNil(namespaceAllSelector(maps.Keys(opts.DefaultNamespaces)), cfg.FieldSelector)...)
+		}
 		opts.DefaultNamespaces[namespace] = cfg
 	}
 
@@ -418,6 +437,15 @@ func defaultOpts(config *rest.Config, opts Options) (Options, error) {
 			// 3. Default from the global defaults
 			config = defaultConfig(config, optionDefaultsToConfig(&opts))
 
+			if namespace == metav1.NamespaceAll {
+				config.FieldSelector = fields.AndSelectors(
+					appendIfNotNil(
+						namespaceAllSelector(maps.Keys(byObject.Namespaces)),
+						config.FieldSelector,
+					)...,
+				)
+			}
+
 			byObject.Namespaces[namespace] = config
 		}
 
@@ -457,3 +485,21 @@ func defaultConfig(toDefault, defaultFrom Config) Config {
 
 	return toDefault
 }
+
+func namespaceAllSelector(namespaces []string) fields.Selector {
+	selectors := make([]fields.Selector, 0, len(namespaces)-1)
+	for _, namespace := range namespaces {
+		if namespace != metav1.NamespaceAll {
+			selectors = append(selectors, fields.OneTermNotEqualSelector("metadata.namespace", namespace))
+		}
+	}
+
+	return fields.AndSelectors(selectors...)
+}
+
+func appendIfNotNil[T comparable](a, b T) []T {
+	if b != *new(T) {
+		return []T{a, b}
+	}
+	return []T{a}
+}

pkg/cache/cache_test.go

@@ -1543,6 +1543,9 @@ func CacheTest(createCacheFunc func(config *rest.Config, opts cache.Options) (ca
 					}
 					return obtainedPodNames
 				}, ConsistOf(tc.expectedPods)))
+				for _, pod := range obtainedStructuredPodList.Items {
+					Expect(informer.Get(context.Background(), client.ObjectKeyFromObject(&pod), &pod)).To(Succeed()) //nolint:gosec // We don't retain the pointer
+				}
 
 				By("Checking with unstructured")
 				obtainedUnstructuredPodList := unstructured.UnstructuredList{}
@@ -1560,6 +1563,9 @@ func CacheTest(createCacheFunc func(config *rest.Config, opts cache.Options) (ca
 					}
 					return obtainedPodNames
 				}, ConsistOf(tc.expectedPods)))
+				for _, pod := range obtainedUnstructuredPodList.Items {
+					Expect(informer.Get(context.Background(), client.ObjectKeyFromObject(&pod), &pod)).To(Succeed()) //nolint:gosec // We don't retain the pointer
+				}
 
 				By("Checking with metadata")
 				obtainedMetadataPodList := metav1.PartialObjectMetadataList{}
@@ -1577,6 +1583,9 @@ func CacheTest(createCacheFunc func(config *rest.Config, opts cache.Options) (ca
 					}
 					return obtainedPodNames
 				}, ConsistOf(tc.expectedPods)))
+				for _, pod := range obtainedMetadataPodList.Items {
+					Expect(informer.Get(context.Background(), client.ObjectKeyFromObject(&pod), &pod)).To(Succeed()) //nolint:gosec // We don't retain the pointer
+				}
 			},
 				Entry("when selectors are empty it has to inform about all the pods", selectorsTestCase{
 					expectedPods: []string{"test-pod-1", "test-pod-2", "test-pod-3", "test-pod-4", "test-pod-5", "test-pod-6"},
@@ -1789,6 +1798,54 @@ func CacheTest(createCacheFunc func(config *rest.Config, opts cache.Options) (ca
 					},
 					expectedPods: []string{},
 				}),
+				Entry("Only NamespaceAll in DefaultNamespaces returns all pods", selectorsTestCase{
+					options: cache.Options{
+						DefaultNamespaces: map[string]cache.Config{
+							metav1.NamespaceAll: {},
+						},
+					},
+					expectedPods: []string{"test-pod-1", "test-pod-2", "test-pod-3", "test-pod-4", "test-pod-5", "test-pod-6"},
+				}),
+				Entry("Only NamespaceAll in ByObject.Namespaces returns all pods", selectorsTestCase{
+					options: cache.Options{
+						ByObject: map[client.Object]cache.ByObject{
+							&corev1.Pod{}: {
+								Namespaces: map[string]cache.Config{
+									metav1.NamespaceAll: {},
+								},
+							},
+						},
+					},
+					expectedPods: []string{"test-pod-1", "test-pod-2", "test-pod-3", "test-pod-4", "test-pod-5", "test-pod-6"},
+				}),
+				Entry("NamespaceAll in DefaultNamespaces creates a cache for all Namespaces that are not in DefaultNamespaces", selectorsTestCase{
+					options: cache.Options{
+						DefaultNamespaces: map[string]cache.Config{
+							metav1.NamespaceAll: {},
+							testNamespaceOne: {
+								// labels.Nothing when serialized matches everything, so we have to construct our own "match nothing" selector
+								LabelSelector: labels.SelectorFromSet(labels.Set{"no-present": "not-present"})},
+						},
+					},
+					// All pods that are not in NamespaceOne
+					expectedPods: []string{"test-pod-2", "test-pod-3", "test-pod-4", "test-pod-6"},
+				}),
+				Entry("NamespaceAll in ByObject.Namespaces creates a cache for all Namespaces that are not in ByObject.Namespaces", selectorsTestCase{
+					options: cache.Options{
+						ByObject: map[client.Object]cache.ByObject{
+							&corev1.Pod{}: {
+								Namespaces: map[string]cache.Config{
+									metav1.NamespaceAll: {},
+									testNamespaceOne: {
+										// labels.Nothing when serialized matches everything, so we have to construct our own "match nothing" selector
+										LabelSelector: labels.SelectorFromSet(labels.Set{"no-present": "not-present"})},
+								},
+							},
+						},
+					},
+					// All pods that are not in NamespaceOne
+					expectedPods: []string{"test-pod-2", "test-pod-3", "test-pod-4", "test-pod-6"},
+				}),
 			)
 		})
 		Describe("as an Informer", func() {

pkg/cache/multi_namespace_cache.go

@@ -23,6 +23,7 @@ import (
 
 	corev1 "k8s.io/api/core/v1"
 	apimeta "k8s.io/apimachinery/pkg/api/meta"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	toolscache "k8s.io/client-go/tools/cache"
@@ -210,6 +211,9 @@ func (c *multiNamespaceCache) Get(ctx context.Context, key client.ObjectKey, obj
 
 	cache, ok := c.namespaceToCache[key.Namespace]
 	if !ok {
+		if global, hasGlobal := c.namespaceToCache[metav1.NamespaceAll]; hasGlobal {
+			return global.Get(ctx, key, obj, opts...)
+		}
 		return fmt.Errorf("unable to get: %v because of unknown namespace for the cache", key)
 	}
 	return cache.Get(ctx, key, obj, opts...)

pkg/client/options.go

@@ -513,8 +513,16 @@ type MatchingLabels map[string]string
 // ApplyToList applies this configuration to the given list options.
 func (m MatchingLabels) ApplyToList(opts *ListOptions) {
 	// TODO(directxman12): can we avoid reserializing this over and over?
-	sel := labels.SelectorFromValidatedSet(map[string]string(m))
-	opts.LabelSelector = sel
+	if opts.LabelSelector == nil {
+		opts.LabelSelector = labels.SelectorFromValidatedSet(map[string]string(m))
+		return
+	}
+	// If there's already a selector, we need to AND the two together.
+	noValidSel := labels.SelectorFromValidatedSet(map[string]string(m))
+	reqs, _ := noValidSel.Requirements()
+	for _, req := range reqs {
+		opts.LabelSelector = opts.LabelSelector.Add(req)
+	}
 }
 
 // ApplyToDeleteAllOf applies this configuration to the given an List options.
@@ -528,14 +536,17 @@ type HasLabels []string
 
 // ApplyToList applies this configuration to the given list options.
 func (m HasLabels) ApplyToList(opts *ListOptions) {
-	sel := labels.NewSelector()
+	if opts.LabelSelector == nil {
+		opts.LabelSelector = labels.NewSelector()
+	}
+	// TODO: ignore invalid labels will result in an empty selector.
+	// This is inconsistent to the that of MatchingLabels.
 	for _, label := range m {
 		r, err := labels.NewRequirement(label, selection.Exists, nil)
 		if err == nil {
-			sel = sel.Add(*r)
+			opts.LabelSelector = opts.LabelSelector.Add(*r)
 		}
 	}
-	opts.LabelSelector = sel
 }
 
 // ApplyToDeleteAllOf applies this configuration to the given an List options.

pkg/client/options_test.go

@@ -237,6 +237,19 @@ var _ = Describe("MatchingLabels", func() {
 		expectedErrMsg := `values[0][k]: Invalid value: "axahm2EJ8Phiephe2eixohbee9eGeiyees1thuozi1xoh0GiuH3diewi8iem7Nui": must be no more than 63 characters`
 		Expect(err.Error()).To(Equal(expectedErrMsg))
 	})
+
+	It("Should add matchingLabels to existing selector", func() {
+		listOpts := &client.ListOptions{}
+
+		matchingLabels := client.MatchingLabels(map[string]string{"k": "v"})
+		matchingLabels2 := client.MatchingLabels(map[string]string{"k2": "v2"})
+
+		matchingLabels.ApplyToList(listOpts)
+		Expect(listOpts.LabelSelector.String()).To(Equal("k=v"))
+
+		matchingLabels2.ApplyToList(listOpts)
+		Expect(listOpts.LabelSelector.String()).To(Equal("k=v,k2=v2"))
+	})
 })
 
 var _ = Describe("FieldOwner", func() {
@@ -292,3 +305,35 @@ var _ = Describe("ForceOwnership", func() {
 		Expect(*o.Force).To(BeTrue())
 	})
 })
+
+var _ = Describe("HasLabels", func() {
+	It("Should produce hasLabels in given order", func() {
+		listOpts := &client.ListOptions{}
+
+		hasLabels := client.HasLabels([]string{"labelApe", "labelFox"})
+		hasLabels.ApplyToList(listOpts)
+		Expect(listOpts.LabelSelector.String()).To(Equal("labelApe,labelFox"))
+	})
+
+	It("Should add hasLabels to existing hasLabels selector", func() {
+		listOpts := &client.ListOptions{}
+
+		hasLabel := client.HasLabels([]string{"labelApe"})
+		hasLabel.ApplyToList(listOpts)
+
+		hasOtherLabel := client.HasLabels([]string{"labelFox"})
+		hasOtherLabel.ApplyToList(listOpts)
+		Expect(listOpts.LabelSelector.String()).To(Equal("labelApe,labelFox"))
+	})
+
+	It("Should add hasLabels to existing matchingLabels", func() {
+		listOpts := &client.ListOptions{}
+
+		matchingLabels := client.MatchingLabels(map[string]string{"k": "v"})
+		matchingLabels.ApplyToList(listOpts)
+
+		hasLabel := client.HasLabels([]string{"labelApe"})
+		hasLabel.ApplyToList(listOpts)
+		Expect(listOpts.LabelSelector.String()).To(Equal("k=v,labelApe"))
+	})
+})