Skip to content

Commit

Permalink
Merge pull request #105 from fedepaol/ocp/fixnoreloads
Browse files Browse the repository at this point in the history 
OCPBUGS-7082: skip unnecessary events
  • Loading branch information
@openshift-merge-robot
openshift-merge-robot authored Feb 14, 2023
2 parents 388ac00 + bb018d5 commit 8ea1ffd
Show file tree
Hide file tree
Showing 4 changed files with 248 additions and 18 deletions.
16 changes: 16 additions & 0 deletions internal/k8s/controllers/config_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package controllers

import (
"context"
"reflect"

"github.com/davecgh/go-spew/spew"
"github.com/go-kit/log"
Expand Down Expand Up @@ -45,9 +46,14 @@ type ConfigReconciler struct {
ValidateConfig config.Validate
ForceReload func()
BGPType string
currentConfig *config.Config
}

func (r *ConfigReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
return requestHandler(r, ctx, req)
}

var requestHandler = func(r *ConfigReconciler, ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
level.Info(r.Logger).Log("controller", "ConfigReconciler", "start reconcile", req.NamespacedName.String())
defer level.Info(r.Logger).Log("controller", "ConfigReconciler", "end reconcile", req.NamespacedName.String())
updates.Inc()
Expand Down Expand Up @@ -127,12 +133,22 @@ func (r *ConfigReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
}

level.Debug(r.Logger).Log("controller", "ConfigReconciler", "rendered config", spew.Sdump(cfg))
if r.currentConfig != nil && reflect.DeepEqual(r.currentConfig, cfg) {
level.Debug(r.Logger).Log("controller", "ConfigReconciler", "event", "configuration did not change, ignoring")
return ctrl.Result{}, nil
}

r.currentConfig = cfg

res := r.Handler(r.Logger, cfg)
switch res {
case SyncStateError:
configStale.Set(1)
updateErrors.Inc()
// if the configuration load failed, we reset the current config because this is gonna lead to a retry
// of the reconciliaton loop. If we don't reset, the retry will find the config identical and will exit,
// which is not what we want here.
r.currentConfig = nil
level.Error(r.Logger).Log("controller", "ConfigReconciler", "metallb CRs and Secrets", dumpClusterResources(&resources), "event", "reload failed, retry")
return ctrl.Result{}, retryError
case SyncStateReprocessAll:
Expand Down
95 changes: 84 additions & 11 deletions internal/k8s/controllers/config_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import (
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/types"
k8sscheme "k8s.io/client-go/kubernetes/scheme"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/envtest"
"sigs.k8s.io/controller-runtime/pkg/manager"
"sigs.k8s.io/controller-runtime/pkg/reconcile"
Expand Down Expand Up @@ -144,6 +145,80 @@ func TestConfigController(t *testing.T) {
}
}

func TestSecretShouldntTrigger(t *testing.T) {
initObjects := objectsFromResources(configControllerValidResources)
fakeClient, err := newFakeClient(initObjects)
if err != nil {
t.Fatalf("test failed to create fake client: %v", err)
}

handlerCalled := false
mockHandler := func(l log.Logger, cfg *config.Config) SyncState {
handlerCalled = true
return SyncStateSuccess
}

r := &ConfigReconciler{
Client: fakeClient,
Logger: log.NewNopLogger(),
Scheme: scheme,
Namespace: testNamespace,
ValidateConfig: config.DontValidate,
Handler: mockHandler,
ForceReload: func() {},
}
req := reconcile.Request{
NamespacedName: types.NamespacedName{
Namespace: testNamespace,
},
}

_, err = r.Reconcile(context.TODO(), req)
if err != nil {
t.Fatalf("reconcile failed: %v", err)
}
if !handlerCalled {
t.Fatalf("handler not called")
}
handlerCalled = false
err = fakeClient.Create(context.TODO(), &v1beta2.BGPPeer{
ObjectMeta: metav1.ObjectMeta{
Name: "peer2",
Namespace: testNamespace,
},
Spec: v1beta2.BGPPeerSpec{
MyASN: 42,
ASN: 142,
Address: "1.2.3.4",
BFDProfile: "default",
},
})
if err != nil {
t.Fatalf("create failed on peer2: %v", err)
}
_, err = r.Reconcile(context.TODO(), req)
if err != nil {
t.Fatalf("reconcile failed: %v", err)
}
if !handlerCalled {
t.Fatalf("handler not called")
}

handlerCalled = false
err = fakeClient.Create(context.TODO(), &corev1.Secret{Type: corev1.SecretTypeBasicAuth, ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: testNamespace},
Data: map[string][]byte{"password": []byte([]byte("nopass"))}})
if err != nil {
t.Fatalf("create failed on secret foo: %v", err)
}
_, err = r.Reconcile(context.TODO(), req)
if err != nil {
t.Fatalf("reconcile failed: %v", err)
}
if handlerCalled {
t.Fatalf("handler called")
}
}

func TestNodeEvent(t *testing.T) {
g := NewGomegaWithT(t)
testEnv := &envtest.Environment{
Expand All @@ -161,31 +236,27 @@ func TestNodeEvent(t *testing.T) {
g.Expect(err).To(BeNil())
err = v1beta2.AddToScheme(k8sscheme.Scheme)
g.Expect(err).To(BeNil())
m, err := manager.New(cfg, manager.Options{})
m, err := manager.New(cfg, manager.Options{MetricsBindAddress: "0"})
g.Expect(err).To(BeNil())

var configUpdate int
var mutex sync.Mutex
mockHandler := func(l log.Logger, cfg *config.Config) SyncState {
oldRequestHandler := requestHandler
defer func() { requestHandler = oldRequestHandler }()

requestHandler = func(r *ConfigReconciler, ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
mutex.Lock()
defer mutex.Unlock()
configUpdate++
return SyncStateSuccess
}
var forceReload int
mockForceReload := func() {
mutex.Lock()
defer mutex.Unlock()
forceReload++
return ctrl.Result{}, nil
}

r := &ConfigReconciler{
Client: m.GetClient(),
Logger: log.NewNopLogger(),
Scheme: scheme,
Namespace: testNamespace,
ValidateConfig: config.DontValidate,
Handler: mockHandler,
ForceReload: mockForceReload,
}
err = r.SetupWithManager(m)
g.Expect(err).To(BeNil())
Expand All @@ -209,6 +280,8 @@ func TestNodeEvent(t *testing.T) {
defer mutex.Unlock()
return configUpdate
}, 5*time.Second, 200*time.Millisecond).Should(Equal(1))
var forceReload int

g.Eventually(func() int {
mutex.Lock()
defer mutex.Unlock()
Expand Down
44 changes: 37 additions & 7 deletions internal/k8s/controllers/node_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,13 @@ import (
"github.com/go-kit/log"
"github.com/go-kit/log/level"

corev1 "k8s.io/api/core/v1"
v1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/labels"
"k8s.io/apimachinery/pkg/runtime"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/event"
"sigs.k8s.io/controller-runtime/pkg/predicate"
)

Expand Down Expand Up @@ -65,18 +68,45 @@ func (r *NodeReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.
}

func (r *NodeReconciler) SetupWithManager(mgr ctrl.Manager) error {
p := predicate.NewPredicateFuncs(
func(obj client.Object) bool {
node, ok := obj.(*v1.Node)
p := predicate.Funcs{
CreateFunc: func(e event.CreateEvent) bool {
return r.filterOtherNodes(e.Object)
},
DeleteFunc: func(e event.DeleteEvent) bool {
return r.filterOtherNodes(e.Object)
},
GenericFunc: func(e event.GenericEvent) bool {
return r.filterOtherNodes(e.Object)
},
UpdateFunc: func(e event.UpdateEvent) bool {
newNodeObj, ok := e.ObjectNew.(*corev1.Node)
if !ok {
level.Error(r.Logger).Log("controller", "NodeReconciler", "error", "object is not node", "name", obj.GetName())
level.Error(r.Logger).Log("controller", "NodeReconciler", "error", "new object is not node", "name", newNodeObj.GetName())
return true
}
oldNodeObj, ok := e.ObjectOld.(*corev1.Node)
if !ok {
level.Error(r.Logger).Log("controller", "NodeReconciler", "error", "old object is not node", "name", oldNodeObj.GetName())
return true
}
// If there is no changes in node labels, ignore event.
if labels.Equals(labels.Set(oldNodeObj.Labels), labels.Set(newNodeObj.Labels)) {
return false
}
return node.Name == r.NodeName
})

return r.filterOtherNodes(newNodeObj)
},
}
return ctrl.NewControllerManagedBy(mgr).
For(&v1.Node{}).
WithEventFilter(p).
Complete(r)
}

func (r *NodeReconciler) filterOtherNodes(obj client.Object) bool {
node, ok := obj.(*v1.Node)
if !ok {
level.Error(r.Logger).Log("controller", "NodeReconciler", "error", "object is not node", "name", obj.GetName())
return false
}
return node.Name == r.NodeName
}
111 changes: 111 additions & 0 deletions internal/k8s/controllers/node_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,24 @@ package controllers

import (
"context"
"path/filepath"
"reflect"
"sync"
"testing"
"time"

"github.com/go-kit/log"
"github.com/google/go-cmp/cmp"
. "github.com/onsi/gomega"
v1beta1 "go.universe.tf/metallb/api/v1beta1"
v1beta2 "go.universe.tf/metallb/api/v1beta2"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types"
k8sscheme "k8s.io/client-go/kubernetes/scheme"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/envtest"
"sigs.k8s.io/controller-runtime/pkg/manager"
"sigs.k8s.io/controller-runtime/pkg/reconcile"
)

Expand Down Expand Up @@ -107,3 +116,105 @@ func TestNodeController(t *testing.T) {
}
}
}

func TestNodeReconciler_SetupWithManager(t *testing.T) {
g := NewGomegaWithT(t)
testEnv := &envtest.Environment{
CRDDirectoryPaths: []string{filepath.Join("../../..", "config", "crd", "bases")},
ErrorIfCRDPathMissing: true,
Scheme: scheme,
}
cfg, err := testEnv.Start()
g.Expect(err).To(BeNil())
defer func() {
err = testEnv.Stop()
g.Expect(err).To(BeNil())
}()
err = v1beta1.AddToScheme(k8sscheme.Scheme)
g.Expect(err).To(BeNil())
err = v1beta2.AddToScheme(k8sscheme.Scheme)
g.Expect(err).To(BeNil())
m, err := manager.New(cfg, manager.Options{MetricsBindAddress: "0"})
g.Expect(err).To(BeNil())

var configUpdate int
var mutex sync.Mutex
mockHandler := func(l log.Logger, n *corev1.Node) SyncState {
mutex.Lock()
defer mutex.Unlock()
configUpdate++
return SyncStateSuccess
}
r := &NodeReconciler{
Client: m.GetClient(),
Logger: log.NewNopLogger(),
Scheme: scheme,
Namespace: testNamespace,
Handler: mockHandler,
NodeName: "test-node",
}
err = r.SetupWithManager(m)
g.Expect(err).To(BeNil())
ctx := context.Background()
go func() {
err = m.Start(ctx)
g.Expect(err).To(BeNil())
}()

// test new node event.
node := &corev1.Node{
ObjectMeta: metav1.ObjectMeta{Name: "test-node"},
Spec: corev1.NodeSpec{},
}
node.Labels = make(map[string]string)
node.Labels["test"] = "e2e"
err = m.GetClient().Create(ctx, node)
g.Expect(err).To(BeNil())
g.Eventually(func() int {
mutex.Lock()
defer mutex.Unlock()
return configUpdate
}, 5*time.Second, 200*time.Millisecond).Should(Equal(1))

// test update node event with no changes into node label.
g.Eventually(func() error {
err = m.GetClient().Get(ctx, types.NamespacedName{Name: "test-node"}, node)
if err != nil {
return err
}
node.Labels = make(map[string]string)
node.Spec.PodCIDR = "192.168.10.0/24"
node.Labels["test"] = "e2e"
err = m.GetClient().Update(ctx, node)
if err != nil {
return err
}
return nil
}, 5*time.Second, 200*time.Millisecond).Should(BeNil())
g.Eventually(func() int {
mutex.Lock()
defer mutex.Unlock()
return configUpdate
}, 5*time.Second, 200*time.Millisecond).Should(Equal(1))

// test update node event with changes into node label.
g.Eventually(func() error {
err = m.GetClient().Get(ctx, types.NamespacedName{Name: "test-node"}, node)
if err != nil {
return err
}
node.Labels = make(map[string]string)
node.Labels["test"] = "e2e"
node.Labels["test"] = "update"
err = m.GetClient().Update(ctx, node)
if err != nil {
return err
}
return nil
}, 5*time.Second, 200*time.Millisecond).Should(BeNil())
g.Eventually(func() int {
mutex.Lock()
defer mutex.Unlock()
return configUpdate
}, 5*time.Second, 200*time.Millisecond).Should(Equal(2))
}

0 comments on commit 8ea1ffd

Please sign in to comment.