Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Only update namespaces when OperatorGroup labels need to change. #2809

Merged
merged 1 commit into from
Jul 6, 2022
Merged

Only update namespaces when OperatorGroup labels need to change. #2809

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 35 additions & 13 deletions pkg/controller/operators/olm/operator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -902,36 +902,58 @@ func (a *Operator) syncNamespace(obj interface{}) error {
"name": namespace.GetName(),
})

// Remove existing OperatorGroup labels
for label := range namespace.GetLabels() {
if operatorsv1.IsOperatorGroupLabel(label) {
delete(namespace.Labels, label)
}
}

operatorGroupList, err := a.lister.OperatorsV1().OperatorGroupLister().List(labels.Everything())
if err != nil {
logger.WithError(err).Warn("lister failed")
return err
}

desiredGroupLabels := make(map[string]string)
for _, group := range operatorGroupList {
namespaceSet := NewNamespaceSet(group.Status.Namespaces)

// Apply the label if not an All Namespaces OperatorGroup.
if namespaceSet.Contains(namespace.GetName()) && !namespaceSet.IsAllNamespaces() {
if namespace.Labels == nil {
namespace.Labels = make(map[string]string, 1)
}
ogLabelKey, ogLabelValue, err := group.OGLabelKeyAndValue()
k, v, err := group.OGLabelKeyAndValue()
if err != nil {
return err
}
namespace.Labels[ogLabelKey] = ogLabelValue
desiredGroupLabels[k] = v
}
}

if changed := func() bool {
for ke, ve := range namespace.Labels {
if !operatorsv1.IsOperatorGroupLabel(ke) {
continue
}
if vd, ok := desiredGroupLabels[ke]; !ok || vd != ve {
return true
}
}
for kd, vd := range desiredGroupLabels {
if ve, ok := namespace.Labels[kd]; !ok || ve != vd {
return true
}
}
return false
}(); !changed {
return nil
}

namespace = namespace.DeepCopy()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should also fix informer cache corruption.

for k := range namespace.Labels {
if operatorsv1.IsOperatorGroupLabel(k) {
delete(namespace.Labels, k)
}
}
if namespace.Labels == nil && len(desiredGroupLabels) > 0 {
namespace.Labels = make(map[string]string)
}
for k, v := range desiredGroupLabels {
namespace.Labels[k] = v
}
awgreene marked this conversation as resolved.
Show resolved Hide resolved

// Update the Namespace
_, err = a.opClient.KubernetesInterface().CoreV1().Namespaces().Update(context.TODO(), namespace, metav1.UpdateOptions{})

return err
Expand Down
184 changes: 184 additions & 0 deletions pkg/controller/operators/olm/operator_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ import (
"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/queueinformer"
"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/scoped"
opregistry "github.com/operator-framework/operator-registry/pkg/registry"
clienttesting "k8s.io/client-go/testing"
)

type TestStrategy struct{}
Expand Down Expand Up @@ -166,6 +167,7 @@ type fakeOperatorConfig struct {
k8sObjs []runtime.Object
extObjs []runtime.Object
regObjs []runtime.Object
actionLog *[]clienttesting.Action
}

// fakeOperatorOption applies an option to the given fake operator configuration.
Expand Down Expand Up @@ -229,6 +231,18 @@ func withRegObjs(regObjs ...runtime.Object) fakeOperatorOption {
}
}

func withActionLog(log *[]clienttesting.Action) fakeOperatorOption {
return func(config *fakeOperatorConfig) {
config.actionLog = log
}
}

func withLogger(logger *logrus.Logger) fakeOperatorOption {
return func(config *fakeOperatorConfig) {
config.logger = logger
}
}

// NewFakeOperator creates and starts a new operator using fake clients.
func NewFakeOperator(ctx context.Context, options ...fakeOperatorOption) (*Operator, error) {
// Apply options to default config
Expand All @@ -247,6 +261,7 @@ func NewFakeOperator(ctx context.Context, options ...fakeOperatorOption) (*Opera
recorder: &record.FakeRecorder{},
// default expected namespaces
namespaces: []string{"default", "kube-system", "kube-public"},
actionLog: &[]clienttesting.Action{},
}
for _, option := range options {
option(config)
Expand All @@ -258,6 +273,10 @@ func NewFakeOperator(ctx context.Context, options ...fakeOperatorOption) (*Opera
// For now, directly use a SimpleClientset instead.
k8sClientFake := k8sfake.NewSimpleClientset(config.k8sObjs...)
k8sClientFake.Resources = apiResourcesForObjects(append(config.extObjs, config.regObjs...))
k8sClientFake.PrependReactor("*", "*", clienttesting.ReactionFunc(func(action clienttesting.Action) (bool, runtime.Object, error) {
*config.actionLog = append(*config.actionLog, action)
return false, nil, nil
}))
config.operatorClient = operatorclient.NewClient(k8sClientFake, apiextensionsfake.NewSimpleClientset(config.extObjs...), apiregistrationfake.NewSimpleClientset(config.regObjs...))
config.configClient = configfake.NewSimpleClientset()

Expand Down Expand Up @@ -3936,6 +3955,171 @@ func TestUpdates(t *testing.T) {
}
}

type tDotLogWriter struct {
*testing.T
}

func (w tDotLogWriter) Write(p []byte) (int, error) {
w.T.Logf("%s", string(p))
return len(p), nil
}

func testLogrusLogger(t *testing.T) *logrus.Logger {
l := logrus.New()
l.SetOutput(tDotLogWriter{t})
return l
}

func TestSyncNamespace(t *testing.T) {
namespace := func(name string, labels map[string]string) corev1.Namespace {
return corev1.Namespace{
ObjectMeta: metav1.ObjectMeta{
Name: name,
Labels: labels,
},
}
}

operatorgroup := func(name string, targets []string) operatorsv1.OperatorGroup {
return operatorsv1.OperatorGroup{
ObjectMeta: metav1.ObjectMeta{
Name: name,
UID: types.UID(fmt.Sprintf("%s-uid", name)),
},
Status: operatorsv1.OperatorGroupStatus{
Namespaces: targets,
},
}
}

for _, tc := range []struct {
name string
before corev1.Namespace
operatorgroups []operatorsv1.OperatorGroup
noop bool
expected []string
}{
{
name: "adds missing labels",
before: namespace("test-namespace", map[string]string{"unrelated": ""}),
operatorgroups: []operatorsv1.OperatorGroup{
operatorgroup("test-group-1", []string{"test-namespace"}),
operatorgroup("test-group-2", []string{"test-namespace"}),
},
expected: []string{
"olm.operatorgroup.uid/test-group-1-uid",
"olm.operatorgroup.uid/test-group-2-uid",
"unrelated",
},
},
{
name: "removes stale labels",
before: namespace("test-namespace", map[string]string{
"olm.operatorgroup.uid/test-group-1-uid": "",
"olm.operatorgroup.uid/test-group-2-uid": "",
}),
operatorgroups: []operatorsv1.OperatorGroup{
operatorgroup("test-group-2", []string{"test-namespace"}),
},
expected: []string{
"olm.operatorgroup.uid/test-group-2-uid",
},
},
{
name: "does not add label if namespace is not a target namespace",
before: namespace("test-namespace", nil),
operatorgroups: []operatorsv1.OperatorGroup{
operatorgroup("test-group-1", []string{"test-namespace"}),
operatorgroup("test-group-2", []string{"not-test-namespace"}),
},
expected: []string{
"olm.operatorgroup.uid/test-group-1-uid",
},
},
{
name: "no update if labels are in sync",
before: namespace("test-namespace", map[string]string{
"olm.operatorgroup.uid/test-group-1-uid": "",
"olm.operatorgroup.uid/test-group-2-uid": "",
}),
operatorgroups: []operatorsv1.OperatorGroup{
operatorgroup("test-group-1", []string{"test-namespace"}),
operatorgroup("test-group-2", []string{"test-namespace"}),
},
noop: true,
expected: []string{
"olm.operatorgroup.uid/test-group-1-uid",
"olm.operatorgroup.uid/test-group-2-uid",
},
},
} {
t.Run(tc.name, func(t *testing.T) {
ctx, cancel := context.WithCancel(context.Background())
defer cancel()

var ogs []runtime.Object
for i := range tc.operatorgroups {
ogs = append(ogs, &tc.operatorgroups[i])
}

var actions []clienttesting.Action

o, err := NewFakeOperator(
ctx,
withClientObjs(ogs...),
withK8sObjs(&tc.before),
withActionLog(&actions),
withLogger(testLogrusLogger(t)),
)
if err != nil {
t.Fatalf("setup failed: %v", err)
}

actions = actions[:0]

err = o.syncNamespace(&tc.before)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}

if tc.noop {
for _, action := range actions {
if action.GetResource().Resource != "namespaces" {
continue
}
if namer, ok := action.(interface{ GetName() string }); ok {
if namer.GetName() != tc.before.Name {
continue
}
} else if objer, ok := action.(interface{ GetObject() runtime.Object }); ok {
if namer, ok := objer.GetObject().(interface{ GetName() string }); ok {
if namer.GetName() != tc.before.Name {
continue
}
}
}
t.Errorf("unexpected client operation: %v", action)
}
}

after, err := o.opClient.KubernetesInterface().CoreV1().Namespaces().Get(ctx, tc.before.Name, metav1.GetOptions{})
if err != nil {
t.Fatalf("unexpected error: %v", err)
}

if len(after.Labels) != len(tc.expected) {
t.Errorf("expected %d labels, got %d", len(tc.expected), len(after.Labels))
}

for _, l := range tc.expected {
if _, ok := after.Labels[l]; !ok {
t.Errorf("missing expected label %q", l)
}
}
})
}
}

func TestSyncOperatorGroups(t *testing.T) {
logrus.SetLevel(logrus.WarnLevel)
clockFake := utilclocktesting.NewFakeClock(time.Date(2006, time.January, 2, 15, 4, 5, 0, time.FixedZone("MST", -7*3600)))
Expand Down