Skip to content
This repository was archived by the owner on Jul 11, 2023. It is now read-only.

Fix flaky e2e tests #4844

Merged
merged 13 commits into from
Jun 28, 2022
Merged

Fix flaky e2e tests #4844

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
5297e12
Add a GinkgoRecover to start debugging flaky tests
keithmattix Jun 23, 2022
1be6968
Trigger CI
keithmattix Jun 23, 2022
434a876
Add another ginko recover
keithmattix Jun 27, 2022
9e1ade9
Add locks that are missing
keithmattix Jun 27, 2022
e7bd74a
Re-run CI
keithmattix Jun 27, 2022
3e86f96
Re-run CI
keithmattix Jun 27, 2022
8a147fc
Fix test flakiness by only reacting to MeshConfigs and MRCs in target…
keithmattix Jun 27, 2022
882dab8
More elegant solution for namespacing
keithmattix Jun 28, 2022
14e3371
VSCode didn't save my file...thanks Go Language servier
keithmattix Jun 28, 2022
1eed0a2
Trigger CI
keithmattix Jun 28, 2022
a0322e5
Trigger CI
keithmattix Jun 28, 2022
75fb48b
Address PR comments
keithmattix Jun 28, 2022
16865c8
Add more info to logs
keithmattix Jun 28, 2022
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
2 changes: 1 addition & 1 deletion cmd/osm-bootstrap/osm-bootstrap.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion cmd/osm-controller/osm-controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion cmd/osm-injector/osm-injector.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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),
)

Expand Down
2 changes: 1 addition & 1 deletion pkg/catalog/fake/fake.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
1 change: 1 addition & 0 deletions pkg/certificate/fake_manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ func FakeCertManager() (*Manager, error) {
getCertValidityDuration,
nil,
1*time.Hour,
"",
)
if err != nil {
return nil, fmt.Errorf("error creating fakeCertManager, err: %w", err)
Expand Down
14 changes: 11 additions & 3 deletions pkg/certificate/manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,14 @@ import (
)

// NewManager creates a new CertificateManager with the passed MRCClient and options
func NewManager(ctx context.Context, mrcClient MRCClient, getServiceCertValidityPeriod func() time.Duration, getIngressCertValidityDuration func() time.Duration, msgBroker *messaging.Broker, checkInterval time.Duration) (*Manager, error) {
func NewManager(ctx context.Context, mrcClient MRCClient, getServiceCertValidityPeriod func() time.Duration, getIngressCertValidityDuration func() time.Duration, msgBroker *messaging.Broker, checkInterval time.Duration, ns string) (*Manager, error) {
m := &Manager{
serviceCertValidityDuration: getServiceCertValidityPeriod,
ingressCertValidityDuration: getIngressCertValidityDuration,
msgBroker: msgBroker,
}

err := m.start(ctx, mrcClient)
err := m.start(ctx, mrcClient, ns)
if err != nil {
return nil, err
}
Expand All @@ -49,7 +49,7 @@ func (m *Manager) startRotationTicker(ctx context.Context, checkInterval time.Du
}()
}

func (m *Manager) start(ctx context.Context, mrcClient MRCClient) error {
func (m *Manager) start(ctx context.Context, mrcClient MRCClient, ns string) error {
// start a watch and we wait until the manager is initialized so that
// the caller gets a manager that's ready to be used
var once sync.Once
Expand Down Expand Up @@ -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:
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe if a MRC is in a state of IssuingRollback it should only be used for validating. I define the IssuingRollback state as the rollback of using this issuer to sign certificates.

Suggested change
case mrc.Status.State == constants.MRCStateValidatingRollback || mrc.Status.State == constants.MRCStateValidatingRollout:
case mrc.Status.State == constants.MRCStateIssuingRollback || mrc.Status.State == constants.MRCStateValidatingRollout:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I just got a chance to look at the state-machine logic here. Isn't there a scenario where there's no signing issuer on startup? If you get an MRC with status IssuingRollback and an MRC with status ValidatingRollout, there's no signing issuer. At least for startup purposes, IssuingRollback seems to imply that the MRC in question was the de-facto issuer

Copy link
Contributor

Choose a reason for hiding this comment

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

The IssuingRollback state is set when an MRC enters the issuingRollout state. I don't think there should ever be a case where MRCs have the IssuingRollback and ValidatingRollout states.

Copy link
Contributor

Choose a reason for hiding this comment

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

This comment is not blocking and can be addressed in a follow up PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm so what does it mean of one MRC is active, and the other is ValidatingRollout? Keep in mind that the control plane could restart between status updates of two different MRCs

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
Expand Down
2 changes: 1 addition & 1 deletion pkg/certificate/manager_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ func TestRotor(t *testing.T) {
stop := make(chan struct{})
defer close(stop)
msgBroker := messaging.NewBroker(stop)
certManager, err := NewManager(context.Background(), &fakeMRCClient{}, getServiceCertValidityPeriod, getIngressGatewayCertValidityPeriod, msgBroker, 5*time.Second)
certManager, err := NewManager(context.Background(), &fakeMRCClient{}, getServiceCertValidityPeriod, getIngressGatewayCertValidityPeriod, msgBroker, 5*time.Second, "")
require.NoError(err)

certA, err := certManager.IssueCertificate(cnPrefix, Service)
Expand Down
2 changes: 1 addition & 1 deletion pkg/certificate/providers/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ func NewCertificateManager(ctx context.Context, kubeClient kubernetes.Interface,
mrcClient = c
}

return certificate.NewManager(ctx, mrcClient, cfg.GetServiceCertValidityPeriod, cfg.GetIngressGatewayCertValidityPeriod, msgBroker, checkInterval)
return certificate.NewManager(ctx, mrcClient, cfg.GetServiceCertValidityPeriod, cfg.GetIngressGatewayCertValidityPeriod, msgBroker, checkInterval, providerNamespace)
}

// GetCertIssuerForMRC returns a certificate.Issuer generated from the provided MRC.
Expand Down
2 changes: 1 addition & 1 deletion pkg/certificate/providers/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
3 changes: 3 additions & 0 deletions pkg/certificate/providers/mrc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -45,6 +46,7 @@ func (m *MRCComposer) Watch(ctx context.Context) (<-chan certificate.MRCEvent, e
eventChan := make(chan certificate.MRCEvent)
m.informerCollection.AddEventHandler(informers.InformerKeyMeshRootCertificate, cache.ResourceEventHandlerFuncs{
AddFunc: func(obj interface{}) {
log.Debug().Msg("received MRC add event")
mrc := obj.(*v1alpha2.MeshRootCertificate)
eventChan <- certificate.MRCEvent{
Type: certificate.MRCEventAdded,
Expand All @@ -54,6 +56,7 @@ func (m *MRCComposer) Watch(ctx context.Context) (<-chan certificate.MRCEvent, e
// We don't really care about the previous version
// since the "state machine" of the MRC is well defined
UpdateFunc: func(_, newObj interface{}) {
log.Debug().Msg("received MRC update event")
mrc := newObj.(*v1alpha2.MeshRootCertificate)
eventChan <- certificate.MRCEvent{
Type: certificate.MRCEventUpdated,
Expand Down
2 changes: 1 addition & 1 deletion pkg/certificate/providers/tresor/fake/fake.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ func NewFake(msgBroker *messaging.Broker, checkInterval time.Duration) *certific

// NewFakeWithValidityDuration constructs a fake certificate manager with specified cert validity duration
func NewFakeWithValidityDuration(getCertValidityDuration func() time.Duration, msgBroker *messaging.Broker, checkInterval time.Duration) *certificate.Manager {
tresorCertManager, err := certificate.NewManager(context.Background(), &fakeMRCClient{}, getCertValidityDuration, getCertValidityDuration, msgBroker, checkInterval)
tresorCertManager, err := certificate.NewManager(context.Background(), &fakeMRCClient{}, getCertValidityDuration, getCertValidityDuration, msgBroker, checkInterval, "")
if err != nil {
log.Error().Err(err).Msg("error encountered creating fake cert manager")
return nil
Expand Down
4 changes: 2 additions & 2 deletions pkg/certificate/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
2 changes: 1 addition & 1 deletion pkg/configurator/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
4 changes: 2 additions & 2 deletions pkg/configurator/methods_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion pkg/envoy/sds/response_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
12 changes: 8 additions & 4 deletions pkg/k8s/informers/informers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
}
}

Expand Down
2 changes: 2 additions & 0 deletions tests/e2e/e2e_retry_policy_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down