Fix flaky e2e tests (#4844)

* Add a GinkgoRecover to start debugging flaky tests Signed-off-by: Keith Mattix II <keithmattix2@gmail.com> * Trigger CI Signed-off-by: Keith Mattix II <keithmattix2@gmail.com> * Add another ginko recover Signed-off-by: Keith Mattix II <keithmattix2@gmail.com> * Add locks that are missing Signed-off-by: Keith Mattix II <keithmattix2@gmail.com> * Re-run CI Signed-off-by: Keith Mattix II <keithmattix2@gmail.com> * Re-run CI Signed-off-by: Keith Mattix II <keithmattix2@gmail.com> * Fix test flakiness by only reacting to MeshConfigs and MRCs in target namespaces Signed-off-by: Keith Mattix II <keithmattix2@gmail.com> * More elegant solution for namespacing Signed-off-by: Keith Mattix II <keithmattix2@gmail.com> * VSCode didn't save my file...thanks Go Language servier Signed-off-by: Keith Mattix II <keithmattix2@gmail.com> * Trigger CI Signed-off-by: Keith Mattix II <keithmattix2@gmail.com> * Trigger CI Signed-off-by: Keith Mattix II <keithmattix2@gmail.com> * Address PR comments Signed-off-by: Keith Mattix II <keithmattix2@gmail.com> * Add more info to logs Signed-off-by: Keith Mattix II <keithmattix2@gmail.com>
openservicemesh · Jun 28, 2022 · 4a3d57d · 4a3d57d
1 parent f3966a3
commit 4a3d57d
Show file tree

Hide file tree

Showing 13 changed files with 32 additions and 15 deletions.
diff --git a/cmd/osm-bootstrap/osm-bootstrap.go b/cmd/osm-bootstrap/osm-bootstrap.go
@@ -198,7 +198,7 @@ func main() {
 
 	informerCollection, err := informers.NewInformerCollection(meshName, stop,
 		informers.WithKubeClient(kubeClient),
-		informers.WithConfigClient(configClient),
+		informers.WithConfigClient(configClient, osmMeshConfigName, osmNamespace),
 	)
 
 	if err != nil {

diff --git a/cmd/osm-controller/osm-controller.go b/cmd/osm-controller/osm-controller.go
@@ -187,7 +187,7 @@ func main() {
 	informerCollection, err := informers.NewInformerCollection(meshName, stop,
 		informers.WithKubeClient(kubeClient),
 		informers.WithSMIClients(smiTrafficSplitClientSet, smiTrafficSpecClientSet, smiTrafficTargetClientSet),
-		informers.WithConfigClient(configClient),
+		informers.WithConfigClient(configClient, osmMeshConfigName, osmNamespace),
 		informers.WithPolicyClient(policyClient),
 	)
 	if err != nil {

diff --git a/cmd/osm-injector/osm-injector.go b/cmd/osm-injector/osm-injector.go
@@ -183,7 +183,7 @@ func main() {
 	informerCollection, err := informers.NewInformerCollection(meshName, stop,
 		informers.WithKubeClient(kubeClient),
 		informers.WithSMIClients(smiTrafficSplitClientSet, smiTrafficSpecClientSet, smiTrafficTargetClientSet),
-		informers.WithConfigClient(configClient),
+		informers.WithConfigClient(configClient, osmMeshConfigName, osmNamespace),
 		informers.WithPolicyClient(policyClient),
 	)
 

diff --git a/pkg/catalog/fake/fake.go b/pkg/catalog/fake/fake.go
@@ -48,7 +48,7 @@ func NewFakeMeshCatalog(kubeClient kubernetes.Interface, meshConfigClient config
 
 	osmNamespace := "-test-osm-namespace-"
 	osmMeshConfigName := "-test-osm-mesh-config-"
-	ic, err := informers.NewInformerCollection("osm", stop, informers.WithKubeClient(kubeClient), informers.WithConfigClient(meshConfigClient))
+	ic, err := informers.NewInformerCollection("osm", stop, informers.WithKubeClient(kubeClient), informers.WithConfigClient(meshConfigClient, osmMeshConfigName, osmNamespace))
 	if err != nil {
 		return nil
 	}

diff --git a/pkg/certificate/manager.go b/pkg/certificate/manager.go
@@ -129,15 +129,23 @@ func (m *Manager) handleMRCEvent(mrcClient MRCClient, event MRCEvent) error {
 		c := &issuer{Issuer: client, ID: clientID, CertificateAuthority: ca}
 		switch {
 		case mrc.Status.State == constants.MRCStateActive:
+			m.mu.Lock()
 			m.signingIssuer = c
 			m.validatingIssuer = c
+			m.mu.Unlock()
 		case mrc.Status.State == constants.MRCStateIssuingRollback || mrc.Status.State == constants.MRCStateIssuingRollout:
+			m.mu.Lock()
 			m.signingIssuer = c
+			m.mu.Unlock()
 		case mrc.Status.State == constants.MRCStateValidatingRollback || mrc.Status.State == constants.MRCStateValidatingRollout:
+			m.mu.Lock()
 			m.validatingIssuer = c
+			m.mu.Unlock()
 		default:
+			m.mu.Lock()
 			m.signingIssuer = c
 			m.validatingIssuer = c
+			m.mu.Unlock()
 		}
 	case MRCEventUpdated:
 		// TODO

diff --git a/pkg/certificate/providers/config_test.go b/pkg/certificate/providers/config_test.go
@@ -139,7 +139,7 @@ func TestGetCertificateManager(t *testing.T) {
 				},
 			}),
 			informerCollectionFunc: func(tc testCase) (*informers.InformerCollection, error) {
-				ic, err := informers.NewInformerCollection("osm", nil, informers.WithKubeClient(tc.kubeClient), informers.WithConfigClient(tc.configClient))
+				ic, err := informers.NewInformerCollection("osm", nil, informers.WithKubeClient(tc.kubeClient), informers.WithConfigClient(tc.configClient, "", "osm-namespace"))
 				if err != nil {
 					return nil, err
 				}

diff --git a/pkg/certificate/providers/mrc.go b/pkg/certificate/providers/mrc.go
@@ -3,6 +3,7 @@ package providers
 import (
 	"context"
 
+	"github.com/rs/zerolog/log"
 	"k8s.io/client-go/tools/cache"
 
 	"github.com/openservicemesh/osm/pkg/apis/config/v1alpha2"
@@ -46,6 +47,7 @@ func (m *MRCComposer) Watch(ctx context.Context) (<-chan certificate.MRCEvent, e
 	m.informerCollection.AddEventHandler(informers.InformerKeyMeshRootCertificate, cache.ResourceEventHandlerFuncs{
 		AddFunc: func(obj interface{}) {
 			mrc := obj.(*v1alpha2.MeshRootCertificate)
+			log.Debug().Msgf("received MRC add event for MRC %s/%s", mrc.GetNamespace(), mrc.GetName())
 			eventChan <- certificate.MRCEvent{
 				Type: certificate.MRCEventAdded,
 				MRC:  mrc,
@@ -55,6 +57,7 @@ func (m *MRCComposer) Watch(ctx context.Context) (<-chan certificate.MRCEvent, e
 		// since the "state machine" of the MRC is well defined
 		UpdateFunc: func(_, newObj interface{}) {
 			mrc := newObj.(*v1alpha2.MeshRootCertificate)
+			log.Debug().Msgf("received MRC update event for MRC %s/%s", mrc.GetNamespace(), mrc.GetName())
 			eventChan <- certificate.MRCEvent{
 				Type: certificate.MRCEventUpdated,
 				MRC:  mrc,

diff --git a/pkg/certificate/types.go b/pkg/certificate/types.go
@@ -147,7 +147,7 @@ var (
 // MRCEventBroker describes any type that allows the caller to Watch() MRCEvents
 type MRCEventBroker interface {
 	// Watch allows the caller to subscribe to events surrounding
-	// MRCs that belong to this particular mesh. Watch returns a channel
-	// that emits events, and an error if the subscription goes awry.
+	// MRCs. Watch returns a channel that emits events, and
+	// an error if the subscription goes awry.
 	Watch(context.Context) (<-chan MRCEvent, error)
 }
diff --git a/pkg/configurator/client_test.go b/pkg/configurator/client_test.go
@@ -24,7 +24,7 @@ func TestGetMeshConfig(t *testing.T) {
 	meshConfigClient := fakeConfig.NewSimpleClientset()
 	stop := make(chan struct{})
 
-	ic, err := informers.NewInformerCollection("osm", stop, informers.WithConfigClient(meshConfigClient))
+	ic, err := informers.NewInformerCollection("osm", stop, informers.WithConfigClient(meshConfigClient, osmMeshConfigName, osmNamespace))
 	a.Nil(err)
 
 	c := NewConfigurator(ic, osmNamespace, osmMeshConfigName, nil)

diff --git a/pkg/configurator/methods_test.go b/pkg/configurator/methods_test.go
@@ -31,7 +31,7 @@ func TestCreateUpdateConfig(t *testing.T) {
 		meshConfigClientSet := testclient.NewSimpleClientset()
 		stop := make(chan struct{})
 
-		ic, err := informers.NewInformerCollection("osm", stop, informers.WithConfigClient(meshConfigClientSet))
+		ic, err := informers.NewInformerCollection("osm", stop, informers.WithConfigClient(meshConfigClientSet, osmMeshConfigName, osmNamespace))
 		tassert.Nil(t, err)
 
 		cfg := NewConfigurator(ic, osmNamespace, osmMeshConfigName, nil)
@@ -465,7 +465,7 @@ func TestCreateUpdateConfig(t *testing.T) {
 			stop := make(chan struct{})
 			defer close(stop)
 
-			ic, err := informers.NewInformerCollection("osm", stop, informers.WithConfigClient(meshConfigClientSet))
+			ic, err := informers.NewInformerCollection("osm", stop, informers.WithConfigClient(meshConfigClientSet, osmMeshConfigName, osmNamespace))
 			assert.Nil(err)
 
 			cfg := NewConfigurator(ic, osmNamespace, osmMeshConfigName, nil)

diff --git a/pkg/envoy/sds/response_test.go b/pkg/envoy/sds/response_test.go
@@ -58,7 +58,7 @@ func TestNewResponse(t *testing.T) {
 	podID := uuid.New()
 
 	proxy := envoy.NewProxy(envoy.KindSidecar, podID, proxySvcAccount.ToServiceIdentity(), nil)
-	ic, err := informers.NewInformerCollection("osm", stop, informers.WithKubeClient(fakeKubeClient), informers.WithConfigClient(fakeConfigClient))
+	ic, err := informers.NewInformerCollection("osm", stop, informers.WithKubeClient(fakeKubeClient), informers.WithConfigClient(fakeConfigClient, "-the-mesh-config-name-", "-osm-namespace-"))
 	assert.Nil(err)
 
 	cfg := configurator.NewConfigurator(ic, "-osm-namespace-", "-the-mesh-config-name-", nil)

diff --git a/pkg/k8s/informers/informers.go b/pkg/k8s/informers/informers.go
@@ -86,12 +86,16 @@ func WithSMIClients(smiTrafficSplitClient smiTrafficSplitClient.Interface, smiTr
 }
 
 // WithConfigClient sets the config client for the InformerCollection
-func WithConfigClient(configClient configClientset.Interface) InformerCollectionOption {
+func WithConfigClient(configClient configClientset.Interface, meshConfigName, osmNamespace string) InformerCollectionOption {
 	return func(ic *InformerCollection) {
-		informerFactory := configInformers.NewSharedInformerFactory(configClient, DefaultKubeEventResyncInterval)
+		listOption := configInformers.WithTweakListOptions(func(opt *metav1.ListOptions) {
+			opt.FieldSelector = fields.OneTermEqualSelector(metav1.ObjectNameField, meshConfigName).String()
+		})
+		meshConfiginformerFactory := configInformers.NewSharedInformerFactoryWithOptions(configClient, DefaultKubeEventResyncInterval, configInformers.WithNamespace(osmNamespace), listOption)
+		mrcInformerFactory := configInformers.NewSharedInformerFactoryWithOptions(configClient, DefaultKubeEventResyncInterval, configInformers.WithNamespace(osmNamespace))
 
-		ic.informers[InformerKeyMeshConfig] = informerFactory.Config().V1alpha2().MeshConfigs().Informer()
-		ic.informers[InformerKeyMeshRootCertificate] = informerFactory.Config().V1alpha2().MeshRootCertificates().Informer()
+		ic.informers[InformerKeyMeshConfig] = meshConfiginformerFactory.Config().V1alpha2().MeshConfigs().Informer()
+		ic.informers[InformerKeyMeshRootCertificate] = mrcInformerFactory.Config().V1alpha2().MeshRootCertificates().Informer()
 	}
 }
 

diff --git a/tests/e2e/e2e_retry_policy_test.go b/tests/e2e/e2e_retry_policy_test.go
@@ -125,6 +125,7 @@ var _ = OSMDescribe("Test Retry Policy",
 
 					By("A request that will be retried NumRetries times then fail")
 					err = wait.Poll(time.Second*3, time.Second*30, func() (bool, error) {
+						defer GinkgoRecover()
 						result := Td.HTTPRequest(req)
 
 						stdout, stderr, err := Td.RunLocal(filepath.FromSlash("../../bin/osm"), "proxy", "get", "stats", clientPod.Name, "--namespace", client)
@@ -240,6 +241,7 @@ var _ = OSMDescribe("Test Retry Policy",
 
 					By("A request that will be retried 0 times and then fail")
 					err = wait.Poll(time.Second*3, time.Second*30, func() (bool, error) {
+						defer GinkgoRecover()
 						result := Td.HTTPRequest(req)
 
 						stdout, stderr, err := Td.RunLocal(filepath.FromSlash("../../bin/osm"), "proxy", "get", "stats", clientPod.Name, "--namespace", client)