Skip to content

Commit f144c0f

Browse files
openshift-merge-robotopenshift-merge-robot
authored
Merge pull request #837 from jpeeler/unit-flake-fix
Refactor to avoid cache races
2 parents 6c13fcc + 03a7f6b commit f144c0f

File tree

4 files changed

+194
-60
lines changed

4 files changed

+194
-60
lines changed

‎Makefile‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ IMAGE_REPO := quay.io/operator-framework/olm
1515
IMAGE_TAG ?= "dev"
1616
KUBE_DEPS := api apiserver apimachinery apiextensions-apiserver kube-aggregator code-generator cli-runtime
1717
KUBE_RELEASE := release-1.12
18+
SPECIFIC_UNIT_TEST := $(if $(TEST),-run $(TEST),)
1819

1920
.PHONY: build test run clean vendor schema-check \
2021
vendor-update coverage coverage-html e2e .FORCE
@@ -24,7 +25,7 @@ all: test build
2425
test: clean cover.out
2526

2627
unit:
27-
go test $(MOD_FLAGS) -v -race -tags=json1 -count=1 ./pkg/...
28+
go test $(MOD_FLAGS) $(SPECIFIC_UNIT_TEST) -v -race -tags=json1 -count=1 ./pkg/...
2829

2930
schema-check:
3031

‎pkg/controller/operators/olm/operator.go‎

Lines changed: 20 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -582,6 +582,23 @@ func (a *Operator) syncClusterServiceVersion(obj interface{}) (syncError error)
582582
}
583583
}
584584

585+
operatorGroup := a.operatorGroupFromAnnotations(logger, clusterServiceVersion)
586+
if operatorGroup == nil {
587+
logger.WithField("reason", "no operatorgroup found for active CSV").Debug("skipping potential RBAC creation in target namespaces")
588+
return
589+
}
590+
591+
if len(operatorGroup.Status.Namespaces) == 1 && operatorGroup.Status.Namespaces[0] == operatorGroup.GetNamespace() {
592+
logger.Debug("skipping copy for OwnNamespace operatorgroup")
593+
return
594+
}
595+
// Ensure operator has access to targetnamespaces with cluster RBAC
596+
// (roles/rolebindings are checked for each target namespace in syncCopyCSV)
597+
if err := a.ensureRBACInTargetNamespace(clusterServiceVersion, operatorGroup); err != nil {
598+
logger.WithError(err).Info("couldn't ensure RBAC in target namespaces")
599+
syncError = err
600+
}
601+
585602
if !outCSV.IsUncopiable() {
586603
a.copyQueueIndexer.Enqueue(outCSV)
587604
}
@@ -607,7 +624,9 @@ func (a *Operator) syncCopyCSV(obj interface{}) (syncError error) {
607624

608625
operatorGroup := a.operatorGroupFromAnnotations(logger, clusterServiceVersion)
609626
if operatorGroup == nil {
610-
logger.WithField("reason", "no operatorgroup found for active CSV").Debug("skipping CSV resource copy to target namespaces")
627+
// since syncClusterServiceVersion is the only enqueuer, annotations should be present
628+
logger.WithField("reason", "no operatorgroup found for active CSV").Error("operatorgroup should have annotations")
629+
syncError = fmt.Errorf("operatorGroup for csv '%v' should have annotations", clusterServiceVersion.GetName())
611630
return
612631
}
613632

@@ -621,12 +640,6 @@ func (a *Operator) syncCopyCSV(obj interface{}) (syncError error) {
621640
syncError = err
622641
}
623642

624-
// Ensure operator has access to targetnamespaces
625-
if err := a.ensureRBACInTargetNamespace(clusterServiceVersion, operatorGroup); err != nil {
626-
logger.WithError(err).Info("couldn't ensure RBAC in target namespaces")
627-
syncError = err
628-
}
629-
630643
return
631644
}
632645

‎pkg/controller/operators/olm/operator_test.go‎

Lines changed: 98 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ import (
2727
apiextensionsfake "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset/fake"
2828
aextv1beta1 "k8s.io/apiextensions-apiserver/pkg/client/informers/externalversions"
2929
"k8s.io/apimachinery/pkg/api/equality"
30+
k8serrors "k8s.io/apimachinery/pkg/api/errors"
3031
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
3132
"k8s.io/apimachinery/pkg/labels"
3233
"k8s.io/apimachinery/pkg/runtime"
@@ -231,6 +232,10 @@ func NewFakeOperator(clientObjs []runtime.Object, k8sObjs []runtime.Object, extO
231232
op.lister.APIRegistrationV1().RegisterAPIServiceLister(apiServiceInformer.Lister())
232233
op.lister.APIExtensionsV1beta1().RegisterCustomResourceDefinitionLister(customResourceDefinitionInformer.Lister())
233234

235+
// TODO: are there other resources that query all namespaces?
236+
csvInformerAll := externalversions.NewSharedInformerFactory(clientFake, wakeupInterval).Operators().V1alpha1().ClusterServiceVersions()
237+
op.lister.OperatorsV1alpha1().RegisterClusterServiceVersionLister(metav1.NamespaceAll, csvInformerAll.Lister())
238+
234239
var hasSyncedCheckFns []cache.InformerSynced
235240
for _, informer := range informerList {
236241
op.RegisterInformer(informer)
@@ -3244,6 +3249,9 @@ func TestUpdates(t *testing.T) {
32443249
if expectedCurrent != expectedPrevious {
32453250
err = wait.PollImmediate(1*time.Millisecond, 5*time.Second, func() (bool, error) {
32463251
updated, err := op.lister.OperatorsV1alpha1().ClusterServiceVersionLister().ClusterServiceVersions(namespace).Get(csv.GetName())
3252+
if k8serrors.IsNotFound(err) {
3253+
return false, nil
3254+
}
32473255
return !equality.Semantic.DeepEqual(updated, fetched), err
32483256
})
32493257
require.NoError(t, err)
@@ -3470,11 +3478,12 @@ func TestSyncOperatorGroups(t *testing.T) {
34703478
objects map[string][]runtime.Object
34713479
}
34723480
tests := []struct {
3473-
initial initial
3474-
name string
3475-
expectedEqual bool
3476-
expectedStatus v1.OperatorGroupStatus
3477-
final final
3481+
initial initial
3482+
name string
3483+
expectedEqual bool
3484+
expectedStatus v1.OperatorGroupStatus
3485+
final final
3486+
ignoreCopyError bool
34783487
}{
34793488
{
34803489
name: "NoMatchingNamespace/NoCSVs",
@@ -3546,6 +3555,7 @@ func TestSyncOperatorGroups(t *testing.T) {
35463555
withAnnotations(operatorCSVFailedNoTargetNS.DeepCopy(), map[string]string{v1.OperatorGroupAnnotationKey: "operator-group-1", v1.OperatorGroupNamespaceAnnotationKey: operatorNamespace}),
35473556
},
35483557
}},
3558+
ignoreCopyError: true,
35493559
},
35503560
{
35513561
name: "MatchingNamespace/NoCSVs",
@@ -4151,25 +4161,67 @@ func TestSyncOperatorGroups(t *testing.T) {
41514161
err = op.syncOperatorGroups(tt.initial.operatorGroup)
41524162
require.NoError(t, err)
41534163

4164+
// wait on operator group updated status to be in the cache as it is required for later CSV operations
4165+
err = wait.PollImmediate(1*time.Millisecond, 5*time.Second, func() (bool, error) {
4166+
operatorGroup, err := op.lister.OperatorsV1().OperatorGroupLister().OperatorGroups(tt.initial.operatorGroup.GetNamespace()).Get(tt.initial.operatorGroup.GetName())
4167+
if err != nil {
4168+
return false, err
4169+
}
4170+
sort.Strings(tt.expectedStatus.Namespaces)
4171+
sort.Strings(operatorGroup.Status.Namespaces)
4172+
if !reflect.DeepEqual(tt.expectedStatus, operatorGroup.Status) {
4173+
return false, err
4174+
}
4175+
return true, nil
4176+
})
4177+
require.NoError(t, err)
4178+
4179+
// this must be done twice to have annotateCSVs run in syncOperatorGroups
4180+
// and to catch provided API changes
4181+
err = op.syncOperatorGroups(tt.initial.operatorGroup)
4182+
require.NoError(t, err)
4183+
41544184
// Sync csvs enough to get them back to succeeded state
41554185
for i := 0; i < 8; i++ {
41564186
opGroupCSVs, err := op.client.OperatorsV1alpha1().ClusterServiceVersions(operatorNamespace).List(metav1.ListOptions{})
41574187
require.NoError(t, err)
41584188

4159-
for _, obj := range opGroupCSVs.Items {
4189+
for i, obj := range opGroupCSVs.Items {
41604190

41614191
err = op.syncClusterServiceVersion(&obj)
41624192
require.NoError(t, err, "%#v", obj)
41634193

41644194
err = op.syncCopyCSV(&obj)
4165-
require.NoError(t, err, "%#v", obj)
4195+
if !tt.ignoreCopyError {
4196+
require.NoError(t, err, "%#v", obj)
4197+
}
4198+
4199+
if i == 0 {
4200+
err = wait.PollImmediate(1*time.Millisecond, 5*time.Second, func() (bool, error) {
4201+
for namespace, objects := range tt.final.objects {
4202+
if err := RequireObjectsInCache(t, op.lister, namespace, objects, false); err != nil {
4203+
return false, nil
4204+
}
4205+
}
4206+
return true, nil
4207+
})
4208+
require.NoError(t, err)
4209+
}
4210+
4211+
if i == 8 {
4212+
err = wait.PollImmediate(1*time.Millisecond, 5*time.Second, func() (bool, error) {
4213+
for namespace, objects := range tt.final.objects {
4214+
if err := RequireObjectsInCache(t, op.lister, namespace, objects, true); err != nil {
4215+
return false, nil
4216+
}
4217+
}
4218+
return true, nil
4219+
})
4220+
require.NoError(t, err)
4221+
}
41664222
}
41674223
}
41684224

4169-
// Sync again to catch provided API changes
4170-
err = op.syncOperatorGroups(tt.initial.operatorGroup)
4171-
require.NoError(t, err)
4172-
41734225
operatorGroup, err := op.GetClient().OperatorsV1().OperatorGroups(tt.initial.operatorGroup.GetNamespace()).Get(tt.initial.operatorGroup.GetName(), metav1.GetOptions{})
41744226
require.NoError(t, err)
41754227
sort.Strings(tt.expectedStatus.Namespaces)
@@ -4183,6 +4235,41 @@ func TestSyncOperatorGroups(t *testing.T) {
41834235
}
41844236
}
41854237

4238+
func RequireObjectsInCache(t *testing.T, lister operatorlister.OperatorLister, namespace string, objects []runtime.Object, doCompare bool) error {
4239+
for _, object := range objects {
4240+
var err error
4241+
var fetched runtime.Object
4242+
switch o := object.(type) {
4243+
case *appsv1.Deployment:
4244+
fetched, err = lister.AppsV1().DeploymentLister().Deployments(namespace).Get(o.GetName())
4245+
case *rbacv1.ClusterRole:
4246+
fetched, err = lister.RbacV1().ClusterRoleLister().Get(o.GetName())
4247+
case *rbacv1.Role:
4248+
fetched, err = lister.RbacV1().RoleLister().Roles(namespace).Get(o.GetName())
4249+
case *rbacv1.ClusterRoleBinding:
4250+
fetched, err = lister.RbacV1().ClusterRoleBindingLister().Get(o.GetName())
4251+
case *rbacv1.RoleBinding:
4252+
fetched, err = lister.RbacV1().RoleBindingLister().RoleBindings(namespace).Get(o.GetName())
4253+
case *v1alpha1.ClusterServiceVersion:
4254+
fetched, err = lister.OperatorsV1alpha1().ClusterServiceVersionLister().ClusterServiceVersions(namespace).Get(o.GetName())
4255+
case *v1.OperatorGroup:
4256+
fetched, err = lister.OperatorsV1().OperatorGroupLister().OperatorGroups(namespace).Get(o.GetName())
4257+
default:
4258+
require.Failf(t, "couldn't find expected object", "%#v", object)
4259+
}
4260+
if err != nil {
4261+
return fmt.Errorf("namespace: %v, error: %v", namespace, err)
4262+
}
4263+
if doCompare {
4264+
if !reflect.DeepEqual(object, fetched) {
4265+
diff.ObjectDiff(object, fetched)
4266+
return fmt.Errorf("expected object didn't match %v", object)
4267+
}
4268+
}
4269+
}
4270+
return nil
4271+
}
4272+
41864273
func RequireObjectsInNamespace(t *testing.T, opClient operatorclient.ClientInterface, client versioned.Interface, namespace string, objects []runtime.Object) {
41874274
for _, object := range objects {
41884275
var err error

0 commit comments

Comments
 (0)