Skip to content

Commit

Permalink
Fix feature flag reconciler triggers. (#1036)
Browse files Browse the repository at this point in the history
The Feature Flag reconciler previously was building too long of a queue
by enqueueing _all_ namespaces when the config-defaults configmap
changed.

While it appears there wasn't any harm due to the reconciler ignoring
the keys, it was still leading to concerningly high queue depths and a
huge amount of unnecessary work for the controllers.
  • Loading branch information
josephlewis42 authored Sep 11, 2023
1 parent 6e5d0c5 commit 4ee30f2
Show file tree
Hide file tree
Showing 3 changed files with 67 additions and 2 deletions.
6 changes: 4 additions & 2 deletions pkg/reconciler/featureflag/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ import (
)

func NewController(ctx context.Context, cmw configmap.Watcher) *controller.Impl {
logger := reconciler.NewControllerLogger(ctx, "namespace")
logger := reconciler.NewControllerLogger(ctx, "featureflag")

namespaceInformer := namespaceinformer.Get(ctx)

Expand Down Expand Up @@ -58,7 +58,9 @@ func NewController(ctx context.Context, cmw configmap.Watcher) *controller.Impl
&config.DefaultsConfig{},
}
resync := configmap.TypeFilter(configsToResync...)(func(string, interface{}) {
impl.GlobalResync(namespaceInformer.Informer())
impl.FilteredGlobalResync(
controller.FilterWithName(v1alpha1.KfNamespace),
namespaceInformer.Informer())
})
configStore := config.NewStore(logger.Named("kf-config-store"), resync)
configStore.WatchConfigs(cmw)
Expand Down
20 changes: 20 additions & 0 deletions pkg/reconciler/featureflag/reconciler.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,15 @@ package featureflag
import (
"context"
"encoding/json"
"fmt"

"github.com/google/kf/v2/pkg/apis/kf/config"
"github.com/google/kf/v2/pkg/apis/kf/v1alpha1"
"github.com/google/kf/v2/pkg/reconciler"
v1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
v1listers "k8s.io/client-go/listers/core/v1"
"k8s.io/client-go/tools/cache"
"knative.dev/pkg/controller"
)

Expand All @@ -40,6 +42,11 @@ var _ controller.Reconciler = (*Reconciler)(nil)
func (r *Reconciler) Reconcile(ctx context.Context, key string) error {
ctx = r.configStore.ToContext(ctx)

// Make sure we only reconcile the Kf namespace.
if err := validateKfNamespaceName(key); err != nil {
return err
}

ns, nsErr := r.namespaceLister.Get(v1alpha1.KfNamespace)
if nsErr != nil {
return nsErr
Expand All @@ -51,6 +58,19 @@ func (r *Reconciler) Reconcile(ctx context.Context, key string) error {
return nil
}

func validateKfNamespaceName(key string) error {
namespace, _, err := cache.SplitMetaNamespaceKey(key)

switch {
case err != nil:
return err
case namespace != v1alpha1.KfNamespace:
return fmt.Errorf("invalid namespace %q queued, expect only %q", namespace, v1alpha1.KfNamespace)
}

return nil
}

func (r *Reconciler) reconcileFeatureFlags(ctx context.Context, namespace *v1.Namespace) (*v1.Namespace, error) {
// Don't modify the informers copy.
existing := namespace.DeepCopy()
Expand Down
43 changes: 43 additions & 0 deletions pkg/reconciler/featureflag/reconciler_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
// Copyright 2023 Google LLC
//
// 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 featureflag

import (
"errors"
"testing"

"github.com/google/kf/v2/pkg/kf/testutil"
)

func TestValidateKfNamespaceName(t *testing.T) {
t.Parallel()

cases := map[string]struct {
key string
want error
}{
"nominal": {key: "kf/", want: nil},
"invalid namespace": {key: "kf-system/", want: errors.New(`invalid namespace "kf-system" queued, expect only "kf"`)},
"invalid key": {key: "foo/bar/bazz", want: errors.New(`unexpected key format: "foo/bar/bazz"`)},
}

for tn, tc := range cases {
t.Run(tn, func(t *testing.T) {
got := validateKfNamespaceName(tc.key)

testutil.AssertErrorsEqual(t, tc.want, got)
})
}
}

0 comments on commit 4ee30f2

Please sign in to comment.