Skip to content

Commit

Permalink
Merge pull request #54 from sttts/sttts-kube-informer-by-default
Browse files Browse the repository at this point in the history
🐛 Instantiate kube index informers by default
  • Loading branch information
kcp-ci-bot committed Apr 29, 2024
2 parents e86462c + e50f901 commit 2605363
Show file tree
Hide file tree
Showing 7 changed files with 97 additions and 25 deletions.
3 changes: 1 addition & 2 deletions pkg/cache/cache.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ import (
"net/http"
"time"

"github.com/kcp-dev/apimachinery/v2/third_party/informers"
"golang.org/x/exp/maps"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/meta"
Expand Down Expand Up @@ -486,7 +485,7 @@ func defaultOpts(config *rest.Config, opts Options) (Options, error) {
}

if opts.NewInformerFunc == nil {
opts.NewInformerFunc = informers.NewSharedIndexInformer
opts.NewInformerFunc = toolscache.NewSharedIndexInformer
}
return opts, nil
}
Expand Down
8 changes: 1 addition & 7 deletions pkg/cache/cache_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ import (
"strings"
"time"

kcpcache "github.com/kcp-dev/apimachinery/v2/pkg/cache"
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
corev1 "k8s.io/api/core/v1"
Expand Down Expand Up @@ -545,14 +544,9 @@ func NonBlockingGetTest(createCacheFunc func(config *rest.Config, opts cache.Opt
Expect(err).NotTo(HaveOccurred())

By("creating the informer cache")
v := reflect.ValueOf(&opts).Elem()
newInformerField := v.FieldByName("NewInformerFunc")
newFakeInformer := func(_ kcache.ListerWatcher, _ runtime.Object, _ time.Duration, _ kcache.Indexers) kcpcache.ScopeableSharedIndexInformer {
opts.NewInformerFunc = func(_ kcache.ListerWatcher, _ runtime.Object, _ time.Duration, _ kcache.Indexers) kcache.SharedIndexInformer {
return &controllertest.FakeInformer{Synced: false}
}
reflect.NewAt(newInformerField.Type(), newInformerField.Addr().UnsafePointer()).
Elem().
Set(reflect.ValueOf(newFakeInformer))
informerCache, err = createCacheFunc(cfg, opts)
Expect(err).NotTo(HaveOccurred())
By("running the cache and waiting for it to sync")
Expand Down
8 changes: 3 additions & 5 deletions pkg/cache/internal/informers.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,6 @@ import (
"sync"
"time"

kcpcache "github.com/kcp-dev/apimachinery/v2/pkg/cache"
"github.com/kcp-dev/apimachinery/v2/third_party/informers"
apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand All @@ -48,7 +46,7 @@ type InformersOpts struct {
Mapper meta.RESTMapper
ResyncPeriod time.Duration
Namespace string
NewInformer func(cache.ListerWatcher, runtime.Object, time.Duration, cache.Indexers) kcpcache.ScopeableSharedIndexInformer
NewInformer func(cache.ListerWatcher, runtime.Object, time.Duration, cache.Indexers) cache.SharedIndexInformer
Selector Selector
Transform cache.TransformFunc
UnsafeDisableDeepCopy bool
Expand All @@ -57,7 +55,7 @@ type InformersOpts struct {

// NewInformers creates a new InformersMap that can create informers under the hood.
func NewInformers(config *rest.Config, options *InformersOpts) *Informers {
newInformer := informers.NewSharedIndexInformer
newInformer := cache.NewSharedIndexInformer
if options.NewInformer != nil {
newInformer = options.NewInformer
}
Expand Down Expand Up @@ -177,7 +175,7 @@ type Informers struct {
unsafeDisableDeepCopy bool

// NewInformer allows overriding of the shared index informer constructor for testing.
newInformer func(cache.ListerWatcher, runtime.Object, time.Duration, cache.Indexers) kcpcache.ScopeableSharedIndexInformer
newInformer func(cache.ListerWatcher, runtime.Object, time.Duration, cache.Indexers) cache.SharedIndexInformer

// WatchErrorHandler allows the shared index informer's
// watchErrorHandler to be set by overriding the options
Expand Down
90 changes: 90 additions & 0 deletions pkg/cache/kcp_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
/*
Copyright 2024 The Kubernetes Authors.
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package cache_test

import (
"context"

. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
corev1 "k8s.io/api/core/v1"
"sigs.k8s.io/controller-runtime/pkg/cache"
"sigs.k8s.io/controller-runtime/pkg/client"
)

var _ = Describe("KCP cluster-unaware informer cache", func() {
// Test whether we can have a cluster-unaware informer cache against a single workspace.
// I.e. every object has a kcp.io/cluster annotation, but it should not be taken
// into consideration by the cache to compute the key.
It("should be able to get the default namespace despite kcp.io/cluster annotation", func() {
ctx, cancel := context.WithCancel(context.Background())
defer cancel()

c, err := cache.New(cfg, cache.Options{})
Expect(err).NotTo(HaveOccurred())

By("Annotating the default namespace with kcp.io/cluster")
cl, err := client.New(cfg, client.Options{})
Expect(err).NotTo(HaveOccurred())
ns := &corev1.Namespace{}
err = cl.Get(ctx, client.ObjectKey{Name: "default"}, ns)
Expect(err).NotTo(HaveOccurred())
ns.Annotations = map[string]string{"kcp.io/cluster": "cluster1"}
err = cl.Update(ctx, ns)
Expect(err).NotTo(HaveOccurred())

go c.Start(ctx) //nolint:errcheck // Start is blocking, and error not relevant here.
c.WaitForCacheSync(ctx)

By("By getting the default namespace with the informer")
err = c.Get(ctx, client.ObjectKey{Name: "default"}, ns)
Expect(err).NotTo(HaveOccurred())
})
})

// TODO: get envtest in place with kcp
/*
var _ = Describe("KCP cluster-aware informer cache", func() {
It("should be able to get the default namespace with kcp.io/cluster annotation", func() {
ctx, cancel := context.WithCancel(context.Background())
defer cancel()
c, err := kcp.NewClusterAwareCache(cfg, cache.Options{})
Expect(err).NotTo(HaveOccurred())
By("Annotating the default namespace with kcp.io/cluster")
cl, err := client.New(cfg, client.Options{})
Expect(err).NotTo(HaveOccurred())
ns := &corev1.Namespace{}
err = cl.Get(ctx, client.ObjectKey{Name: "default"}, ns)
Expect(err).NotTo(HaveOccurred())
ns.Annotations = map[string]string{"kcp.io/cluster": "cluster1"}
err = cl.Update(ctx, ns)
Expect(err).NotTo(HaveOccurred())
go c.Start(ctx) //nolint:errcheck // Start is blocking, and error not relevant here.
c.WaitForCacheSync(ctx)
By("By getting the default namespace with the informer, but cluster-less key should fail")
err = c.Get(ctx, client.ObjectKey{Name: "default"}, ns)
Expect(err).To(HaveOccurred())
By("By getting the default namespace with the informer, but cluster-aware key should succeed")
err = c.Get(kontext.WithCluster(ctx, "cluster1"), client.ObjectKey{Name: "default", Namespace: "cluster1"}, ns)
})
})
*/
3 changes: 1 addition & 2 deletions pkg/client/interfaces.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import (
"context"
"time"

kcpcache "github.com/kcp-dev/apimachinery/v2/pkg/cache"
apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/client-go/tools/cache"
Expand Down Expand Up @@ -49,7 +48,7 @@ type Patch interface {

// NewInformerFunc describes a function that creates SharedIndexInformers.
// Its signature matches cache.NewSharedIndexInformer from client-go.
type NewInformerFunc func(cache.ListerWatcher, runtime.Object, time.Duration, cache.Indexers) kcpcache.ScopeableSharedIndexInformer
type NewInformerFunc func(cache.ListerWatcher, runtime.Object, time.Duration, cache.Indexers) cache.SharedIndexInformer

// TODO(directxman12): is there a sane way to deal with get/delete options?

Expand Down
8 changes: 0 additions & 8 deletions pkg/controller/controllertest/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,11 @@ package controllertest
import (
"time"

kcpcache "github.com/kcp-dev/apimachinery/v2/pkg/cache"
"github.com/kcp-dev/logicalcluster/v3"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/client-go/tools/cache"
)

var _ cache.SharedIndexInformer = &FakeInformer{}
var _ kcpcache.ScopeableSharedIndexInformer = &FakeInformer{}

// FakeInformer provides fake Informer functionality for testing.
type FakeInformer struct {
Expand Down Expand Up @@ -81,11 +78,6 @@ func (e eventHandlerWrapper) OnDelete(obj interface{}) {
e.handler.(legacyResourceEventHandler).OnDelete(obj)
}

// Cluster returns the fake Informer.
func (f *FakeInformer) Cluster(clusterName logicalcluster.Name) cache.SharedIndexInformer {
return f
}

// AddIndexers does nothing. TODO(community): Implement this.
func (f *FakeInformer) AddIndexers(indexers cache.Indexers) error {
return nil
Expand Down
2 changes: 1 addition & 1 deletion pkg/kcp/wrappers.go
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ func NewClusterAwareCache(config *rest.Config, opts cache.Options) (cache.Cache,
c := rest.CopyConfig(config)
c.Host += "/clusters/*"

opts.NewInformerFunc = func(lw k8scache.ListerWatcher, obj runtime.Object, syncPeriod time.Duration, indexers k8scache.Indexers) kcpcache.ScopeableSharedIndexInformer {
opts.NewInformerFunc = func(lw k8scache.ListerWatcher, obj runtime.Object, syncPeriod time.Duration, indexers k8scache.Indexers) k8scache.SharedIndexInformer {
indexers[kcpcache.ClusterIndexName] = kcpcache.ClusterIndexFunc
indexers[kcpcache.ClusterAndNamespaceIndexName] = kcpcache.ClusterAndNamespaceIndexFunc

Expand Down

0 comments on commit 2605363

Please sign in to comment.