From 74daa1da642ef455c52fb365bd5f6a1d06f52abe Mon Sep 17 00:00:00 2001 From: Monis Khan Date: Wed, 25 Aug 2021 15:34:33 -0400 Subject: [PATCH 1/2] test/integration: run parallel tests concurrently with serial tests Signed-off-by: Monis Khan --- .../integration/examplecontroller_test.go | 2 +- test/integration/cli_test.go | 3 +- .../concierge_credentialrequest_test.go | 12 ++- .../concierge_kubecertagent_test.go | 3 +- test/integration/concierge_kubectl_test.go | 2 +- test/integration/formposthtml_test.go | 5 +- test/integration/ldap_client_test.go | 3 +- test/integration/leaderelection_test.go | 5 +- test/integration/main_test.go | 91 +++++++++++++++++++ test/integration/supervisor_secrets_test.go | 3 +- ...ervisor_storage_garbage_collection_test.go | 7 +- test/integration/whoami_test.go | 18 ++-- test/testlib/env.go | 2 +- test/testlib/skip.go | 4 +- 14 files changed, 132 insertions(+), 28 deletions(-) create mode 100644 test/integration/main_test.go diff --git a/internal/controllerlib/test/integration/examplecontroller_test.go b/internal/controllerlib/test/integration/examplecontroller_test.go index 695b8072..93bf7a0e 100644 --- a/internal/controllerlib/test/integration/examplecontroller_test.go +++ b/internal/controllerlib/test/integration/examplecontroller_test.go @@ -20,7 +20,7 @@ import ( ) func TestExampleController(t *testing.T) { - testlib.SkipUnlessIntegration(t) + _ = testlib.IntegrationEnv(t) config := testlib.NewClientConfig(t) diff --git a/test/integration/cli_test.go b/test/integration/cli_test.go index ea710e92..26ba36bd 100644 --- a/test/integration/cli_test.go +++ b/test/integration/cli_test.go @@ -37,7 +37,8 @@ import ( "go.pinniped.dev/test/testlib/browsertest" ) -func TestCLIGetKubeconfigStaticToken(t *testing.T) { +// safe to run in parallel with serial tests since it only interacts with a test local webhook, see main_test.go. +func TestCLIGetKubeconfigStaticToken_Parallel(t *testing.T) { env := testlib.IntegrationEnv(t).WithCapability(testlib.ClusterSigningKeyIsAvailable) // Create a test webhook configuration to use with the CLI. diff --git a/test/integration/concierge_credentialrequest_test.go b/test/integration/concierge_credentialrequest_test.go index 61782de3..30a771e7 100644 --- a/test/integration/concierge_credentialrequest_test.go +++ b/test/integration/concierge_credentialrequest_test.go @@ -22,7 +22,8 @@ import ( "go.pinniped.dev/test/testlib" ) -func TestUnsuccessfulCredentialRequest(t *testing.T) { +// TCRs are non-mutating and safe to run in parallel with serial tests, see main_test.go. +func TestUnsuccessfulCredentialRequest_Parallel(t *testing.T) { env := testlib.IntegrationEnv(t).WithCapability(testlib.AnonymousAuthenticationSupported) ctx, cancel := context.WithTimeout(context.Background(), time.Minute) @@ -44,7 +45,8 @@ func TestUnsuccessfulCredentialRequest(t *testing.T) { require.Equal(t, "authentication failed", *response.Status.Message) } -func TestSuccessfulCredentialRequest(t *testing.T) { +// TCRs are non-mutating and safe to run in parallel with serial tests, see main_test.go. +func TestSuccessfulCredentialRequest_Parallel(t *testing.T) { env := testlib.IntegrationEnv(t).WithCapability(testlib.ClusterSigningKeyIsAvailable) ctx, cancel := context.WithTimeout(context.Background(), 6*time.Minute) @@ -129,7 +131,8 @@ func TestSuccessfulCredentialRequest(t *testing.T) { } } -func TestFailedCredentialRequestWhenTheRequestIsValidButTheTokenDoesNotAuthenticateTheUser(t *testing.T) { +// TCRs are non-mutating and safe to run in parallel with serial tests, see main_test.go. +func TestFailedCredentialRequestWhenTheRequestIsValidButTheTokenDoesNotAuthenticateTheUser_Parallel(t *testing.T) { _ = testlib.IntegrationEnv(t).WithCapability(testlib.ClusterSigningKeyIsAvailable) // Create a testWebhook so we have a legitimate authenticator to pass to the @@ -149,7 +152,8 @@ func TestFailedCredentialRequestWhenTheRequestIsValidButTheTokenDoesNotAuthentic require.Equal(t, pointer.StringPtr("authentication failed"), response.Status.Message) } -func TestCredentialRequest_ShouldFailWhenRequestDoesNotIncludeToken(t *testing.T) { +// TCRs are non-mutating and safe to run in parallel with serial tests, see main_test.go. +func TestCredentialRequest_ShouldFailWhenRequestDoesNotIncludeToken_Parallel(t *testing.T) { _ = testlib.IntegrationEnv(t).WithCapability(testlib.ClusterSigningKeyIsAvailable) // Create a testWebhook so we have a legitimate authenticator to pass to the diff --git a/test/integration/concierge_kubecertagent_test.go b/test/integration/concierge_kubecertagent_test.go index a8e2ee0c..e44ad68e 100644 --- a/test/integration/concierge_kubecertagent_test.go +++ b/test/integration/concierge_kubecertagent_test.go @@ -93,7 +93,8 @@ func findSuccessfulStrategy(credentialIssuer *conciergev1alpha.CredentialIssuer, return nil } -func TestLegacyPodCleaner(t *testing.T) { +// safe to run in parallel with serial tests since it only interacts with a test local pod, see main_test.go. +func TestLegacyPodCleaner_Parallel(t *testing.T) { env := testlib.IntegrationEnv(t).WithCapability(testlib.ClusterSigningKeyIsAvailable) ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute) defer cancel() diff --git a/test/integration/concierge_kubectl_test.go b/test/integration/concierge_kubectl_test.go index 04ba3bbd..a113c465 100644 --- a/test/integration/concierge_kubectl_test.go +++ b/test/integration/concierge_kubectl_test.go @@ -15,7 +15,7 @@ import ( // Smoke test to see if the kubeconfig works and the cluster is reachable. func TestGetNodes(t *testing.T) { - testlib.SkipUnlessIntegration(t) + _ = testlib.IntegrationEnv(t) cmd := exec.Command("kubectl", "get", "nodes") cmd.Stdout = os.Stdout cmd.Stderr = os.Stderr diff --git a/test/integration/formposthtml_test.go b/test/integration/formposthtml_test.go index f44e1ae5..bfc900df 100644 --- a/test/integration/formposthtml_test.go +++ b/test/integration/formposthtml_test.go @@ -24,7 +24,10 @@ import ( "go.pinniped.dev/test/testlib/browsertest" ) -func TestFormPostHTML(t *testing.T) { +// safe to run in parallel with serial tests since it only interacts with a test local server, see main_test.go. +func TestFormPostHTML_Parallel(t *testing.T) { + _ = testlib.IntegrationEnv(t) + // Run a mock callback handler, simulating the one running in the CLI. callbackURL, expectCallback := formpostCallbackServer(t) diff --git a/test/integration/ldap_client_test.go b/test/integration/ldap_client_test.go index 99a6b7fb..9c21698c 100644 --- a/test/integration/ldap_client_test.go +++ b/test/integration/ldap_client_test.go @@ -24,7 +24,8 @@ import ( "go.pinniped.dev/test/testlib" ) -func TestLDAPSearch(t *testing.T) { +// safe to run in parallel with serial tests since it only makes read requests to our test LDAP server, see main_test.go. +func TestLDAPSearch_Parallel(t *testing.T) { // This test does not interact with Kubernetes itself. It is a test of our LDAP client code, and only interacts // with our test OpenLDAP server, which is exposed directly to this test via kubectl port-forward. // Theoretically we should always be able to run this test, but something about the kubectl port forwarding diff --git a/test/integration/leaderelection_test.go b/test/integration/leaderelection_test.go index caecb59e..2e1fe044 100644 --- a/test/integration/leaderelection_test.go +++ b/test/integration/leaderelection_test.go @@ -27,11 +27,10 @@ import ( "go.pinniped.dev/test/testlib" ) -func TestLeaderElection(t *testing.T) { +// safe to run in parallel with serial tests since it only interacts with a test local lease, see main_test.go. +func TestLeaderElection_Parallel(t *testing.T) { _ = testlib.IntegrationEnv(t) - t.Parallel() - ctx, cancel := context.WithTimeout(context.Background(), 10*time.Minute) t.Cleanup(cancel) diff --git a/test/integration/main_test.go b/test/integration/main_test.go new file mode 100644 index 00000000..b3b670ae --- /dev/null +++ b/test/integration/main_test.go @@ -0,0 +1,91 @@ +// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package integration + +import ( + "os" + "reflect" + "strings" + "testing" + "unsafe" + + "go.pinniped.dev/test/testlib" +) + +func TestMain(m *testing.M) { + splitIntegrationTestsIntoBuckets(m) + os.Exit(m.Run()) +} + +func splitIntegrationTestsIntoBuckets(m *testing.M) { + // this is some dark magic to set a private field + testsField := reflect.ValueOf(m).Elem().FieldByName("tests") + testsPointer := (*[]testing.InternalTest)(unsafe.Pointer(testsField.UnsafeAddr())) + + tests := *testsPointer + + if len(tests) == 0 { + return + } + + var serialTests, parallelTests, finalTests []testing.InternalTest + + for _, test := range tests { + test := test + + // top level integration tests the end with the string _Parallel + // are indicating that they are safe to run in parallel with + // other serial tests (which Go does not let you easily express). + // top level tests that want the standard Go behavior of only running + // parallel tests with other parallel tests should use the regular + // t.Parallel() approach. this has no effect on any subtest. + if strings.HasSuffix(test.Name, "_Parallel") { + parallelTests = append(parallelTests, test) + } else { + serialTests = append(serialTests, test) + } + } + + serialTest := testing.InternalTest{ + Name: "TestIntegrationSerial", + F: func(t *testing.T) { + _ = testlib.IntegrationEnv(t) // make sure these tests do not run during unit tests + t.Parallel() // outer test runs in parallel always + + for _, test := range serialTests { + test := test + t.Run(test.Name, func(t *testing.T) { + test.F(t) // inner serial tests do not run in parallel + }) + } + }, + } + + parallelTest := testing.InternalTest{ + Name: "TestIntegrationParallel", + F: func(t *testing.T) { + _ = testlib.IntegrationEnv(t) // make sure these tests do not run during unit tests + t.Parallel() // outer test runs in parallel always + + for _, test := range parallelTests { + test := test + t.Run(test.Name, func(t *testing.T) { + t.Parallel() // inner parallel tests do run in parallel + + test.F(t) + }) + } + }, + } + + if len(serialTests) > 0 { + finalTests = append(finalTests, serialTest) + } + + if len(parallelTests) > 0 { + finalTests = append(finalTests, parallelTest) + } + + *testsPointer = finalTests +} diff --git a/test/integration/supervisor_secrets_test.go b/test/integration/supervisor_secrets_test.go index 949a27cc..65ff64a8 100644 --- a/test/integration/supervisor_secrets_test.go +++ b/test/integration/supervisor_secrets_test.go @@ -18,7 +18,8 @@ import ( "go.pinniped.dev/test/testlib" ) -func TestSupervisorSecrets(t *testing.T) { +// safe to run in parallel with serial tests since it only interacts with a test local federation domain, see main_test.go. +func TestSupervisorSecrets_Parallel(t *testing.T) { env := testlib.IntegrationEnv(t) kubeClient := testlib.NewKubernetesClientset(t) supervisorClient := testlib.NewSupervisorClientset(t) diff --git a/test/integration/supervisor_storage_garbage_collection_test.go b/test/integration/supervisor_storage_garbage_collection_test.go index 52d70157..588c0779 100644 --- a/test/integration/supervisor_storage_garbage_collection_test.go +++ b/test/integration/supervisor_storage_garbage_collection_test.go @@ -19,11 +19,8 @@ import ( "go.pinniped.dev/test/testlib" ) -func TestStorageGarbageCollection(t *testing.T) { - // Run this test in parallel with the other integration tests because it does a lot of waiting - // and will not impact other tests, or be impacted by other tests, when run in parallel. - t.Parallel() - +// safe to run in parallel with serial tests since it only interacts with test local secrets, see main_test.go. +func TestStorageGarbageCollection_Parallel(t *testing.T) { env := testlib.IntegrationEnv(t) client := testlib.NewKubernetesClientset(t) secrets := client.CoreV1().Secrets(env.SupervisorNamespace) diff --git a/test/integration/whoami_test.go b/test/integration/whoami_test.go index c613cc1a..17bae321 100644 --- a/test/integration/whoami_test.go +++ b/test/integration/whoami_test.go @@ -29,7 +29,8 @@ import ( "go.pinniped.dev/test/testlib" ) -func TestWhoAmI_Kubeadm(t *testing.T) { +// whoami requests are non-mutating and safe to run in parallel with serial tests, see main_test.go. +func TestWhoAmI_Kubeadm_Parallel(t *testing.T) { // use the cluster signing key being available as a proxy for this being a kubeadm cluster // we should add more robust logic around skipping clusters based on vendor _ = testlib.IntegrationEnv(t).WithCapability(testlib.ClusterSigningKeyIsAvailable) @@ -60,7 +61,8 @@ func TestWhoAmI_Kubeadm(t *testing.T) { ) } -func TestWhoAmI_ServiceAccount_Legacy(t *testing.T) { +// whoami requests are non-mutating and safe to run in parallel with serial tests, see main_test.go. +func TestWhoAmI_ServiceAccount_Legacy_Parallel(t *testing.T) { _ = testlib.IntegrationEnv(t) ctx, cancel := context.WithTimeout(context.Background(), time.Minute) @@ -133,7 +135,8 @@ func TestWhoAmI_ServiceAccount_Legacy(t *testing.T) { ) } -func TestWhoAmI_ServiceAccount_TokenRequest(t *testing.T) { +// whoami requests are non-mutating and safe to run in parallel with serial tests, see main_test.go. +func TestWhoAmI_ServiceAccount_TokenRequest_Parallel(t *testing.T) { env := testlib.IntegrationEnv(t) ctx, cancel := context.WithTimeout(context.Background(), time.Minute) @@ -242,7 +245,8 @@ func TestWhoAmI_ServiceAccount_TokenRequest(t *testing.T) { ) } -func TestWhoAmI_CSR(t *testing.T) { +// whoami requests are non-mutating and safe to run in parallel with serial tests, see main_test.go. +func TestWhoAmI_CSR_Parallel(t *testing.T) { // use the cluster signing key being available as a proxy for this not being an EKS cluster // we should add more robust logic around skipping clusters based on vendor _ = testlib.IntegrationEnv(t).WithCapability(testlib.ClusterSigningKeyIsAvailable) @@ -330,7 +334,8 @@ func TestWhoAmI_CSR(t *testing.T) { ) } -func TestWhoAmI_Anonymous(t *testing.T) { +// whoami requests are non-mutating and safe to run in parallel with serial tests, see main_test.go. +func TestWhoAmI_Anonymous_Parallel(t *testing.T) { _ = testlib.IntegrationEnv(t).WithCapability(testlib.AnonymousAuthenticationSupported) ctx, cancel := context.WithTimeout(context.Background(), time.Minute) @@ -360,7 +365,8 @@ func TestWhoAmI_Anonymous(t *testing.T) { ) } -func TestWhoAmI_ImpersonateDirectly(t *testing.T) { +// whoami requests are non-mutating and safe to run in parallel with serial tests, see main_test.go. +func TestWhoAmI_ImpersonateDirectly_Parallel(t *testing.T) { _ = testlib.IntegrationEnv(t) ctx, cancel := context.WithTimeout(context.Background(), time.Minute) diff --git a/test/testlib/env.go b/test/testlib/env.go index 5447743c..b1e2176a 100644 --- a/test/testlib/env.go +++ b/test/testlib/env.go @@ -120,7 +120,7 @@ func IntegrationEnv(t *testing.T) *TestEnv { } t.Helper() - SkipUnlessIntegration(t) + skipUnlessIntegration(t) capabilitiesDescriptionYAML := os.Getenv("PINNIPED_TEST_CLUSTER_CAPABILITY_YAML") capabilitiesDescriptionFile := os.Getenv("PINNIPED_TEST_CLUSTER_CAPABILITY_FILE") diff --git a/test/testlib/skip.go b/test/testlib/skip.go index 75e0e9fc..6f7de643 100644 --- a/test/testlib/skip.go +++ b/test/testlib/skip.go @@ -5,8 +5,8 @@ package testlib import "testing" -// SkipUnlessIntegration skips the current test if `-short` has been passed to `go test`. -func SkipUnlessIntegration(t *testing.T) { +// skipUnlessIntegration skips the current test if `-short` has been passed to `go test`. +func skipUnlessIntegration(t *testing.T) { t.Helper() if testing.Short() { t.Skip("skipping integration test because of '-short' flag") From e2cf9f6b74f4db9336789158aa7340a4cf4de8dd Mon Sep 17 00:00:00 2001 From: Monis Khan Date: Thu, 26 Aug 2021 08:39:44 -0400 Subject: [PATCH 2/2] leader election test: approximate that followers have observed change Instead of blindly waiting long enough for a disruptive change to have been observed by the old leader and followers, we instead rely on the approximation that checkOnlyLeaderCanWrite provides - i.e. only a single actor believes they are the leader. This does not account for clients that were in the followers list before and after the disruptive change, but it serves as a reasonable approximation. Signed-off-by: Monis Khan --- test/integration/leaderelection_test.go | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/test/integration/leaderelection_test.go b/test/integration/leaderelection_test.go index 2e1fe044..d9da6c57 100644 --- a/test/integration/leaderelection_test.go +++ b/test/integration/leaderelection_test.go @@ -31,7 +31,7 @@ import ( func TestLeaderElection_Parallel(t *testing.T) { _ = testlib.IntegrationEnv(t) - ctx, cancel := context.WithTimeout(context.Background(), 10*time.Minute) + ctx, cancel := context.WithTimeout(context.Background(), 15*time.Minute) t.Cleanup(cancel) leaseName := "leader-election-" + rand.String(5) @@ -197,14 +197,17 @@ func waitForIdentity(ctx context.Context, t *testing.T, namespace *corev1.Namesp testlib.RequireEventuallyWithoutError(t, func() (bool, error) { lease, err := pickRandomLeaderElectionClient(clients).Kubernetes.CoordinationV1().Leases(namespace.Name).Get(ctx, leaseName, metav1.GetOptions{}) if apierrors.IsNotFound(err) { + t.Logf("lease %s/%s does not exist", namespace.Name, leaseName) return false, nil } if err != nil { return false, err } out = lease + t.Logf("lease %s/%s - current leader identity: %s, valid leader identities: %s", + namespace.Name, leaseName, pointer.StringDeref(lease.Spec.HolderIdentity, ""), identities.List()) return lease.Spec.HolderIdentity != nil && identities.Has(*lease.Spec.HolderIdentity), nil - }, 5*time.Minute, time.Second) + }, 10*time.Minute, 10*time.Second) return out } @@ -256,7 +259,7 @@ func checkOnlyLeaderCanWrite(ctx context.Context, t *testing.T, namespace *corev } requireEventually.Equal(1, leaders, "did not see leader") requireEventually.Equal(len(clients)-1, nonLeaders, "did not see non-leader") - }, time.Minute, time.Second) + }, 3*time.Minute, 3*time.Second) return lease } @@ -273,7 +276,7 @@ func forceTransition(ctx context.Context, t *testing.T, namespace *corev1.Namesp startTime = *startLease.Spec.AcquireTime startLease = startLease.DeepCopy() - startLease.Spec.HolderIdentity = pointer.String("some-other-client" + rand.String(5)) + startLease.Spec.HolderIdentity = pointer.String("some-other-client-" + rand.String(5)) _, err := pickCurrentLeaderClient(ctx, t, namespace, leaseName, clients). Kubernetes.CoordinationV1().Leases(namespace.Name).Update(ctx, startLease, metav1.UpdateOptions{}) @@ -288,8 +291,6 @@ func forceTransition(ctx context.Context, t *testing.T, namespace *corev1.Namesp require.Greater(t, finalTransitions, startTransitions) require.Greater(t, finalTime.UnixNano(), startTime.UnixNano()) - time.Sleep(2 * time.Minute) // need to give clients time to notice this change because leader election is polling based - return finalLease } @@ -306,8 +307,6 @@ func forceRestart(ctx context.Context, t *testing.T, namespace *corev1.Namespace require.Zero(t, *newLease.Spec.LeaseTransitions) require.Greater(t, newLease.Spec.AcquireTime.UnixNano(), startLease.Spec.AcquireTime.UnixNano()) - time.Sleep(2 * time.Minute) // need to give clients time to notice this change because leader election is polling based - return newLease }