From 911c5bddce34b3474e4915fab5e1ed43fbe33978 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Mon, 27 Jun 2022 11:56:37 +0000 Subject: [PATCH 1/5] Make saving logs from tests an option (default false) We currently have a bit of flaky logic which prevents the docker plugin from cleaning up the containers if the tests or setup fatals or crashes, this is due to a limitation in the save / passed stats handling. This change makes it an environment variable which by default ditches the logs and makes the containers clean up "correctly" in the teardown method. --- integration_common_test.go | 31 ++++++++++++++++- integration_embedded_derp_test.go | 58 ++++++++++++++++++++++--------- integration_test.go | 56 +++++++++++++++++++++-------- 3 files changed, 113 insertions(+), 32 deletions(-) diff --git a/integration_common_test.go b/integration_common_test.go index f1c4e868..4ee2d3b3 100644 --- a/integration_common_test.go +++ b/integration_common_test.go @@ -6,7 +6,10 @@ package headscale import ( "bytes" "encoding/json" + "errors" "fmt" + "os" + "strconv" "strings" "time" @@ -16,9 +19,13 @@ import ( "inet.af/netaddr" ) -const DOCKER_EXECUTE_TIMEOUT = 10 * time.Second +const ( + DOCKER_EXECUTE_TIMEOUT = 10 * time.Second +) var ( + errEnvVarEmpty = errors.New("getenv: environment variable empty") + IpPrefix4 = netaddr.MustParseIPPrefix("100.64.0.0/10") IpPrefix6 = netaddr.MustParseIPPrefix("fd7a:115c:a1e0::/48") @@ -283,3 +290,25 @@ func getMagicFQDN( return hostnames, nil } + +func GetEnvStr(key string) (string, error) { + v := os.Getenv(key) + if v == "" { + return v, errEnvVarEmpty + } + + return v, nil +} + +func GetEnvBool(key string) (bool, error) { + s, err := GetEnvStr(key) + if err != nil { + return false, err + } + v, err := strconv.ParseBool(s) + if err != nil { + return false, err + } + + return v, nil +} diff --git a/integration_embedded_derp_test.go b/integration_embedded_derp_test.go index 5f388694..d8918f4e 100644 --- a/integration_embedded_derp_test.go +++ b/integration_embedded_derp_test.go @@ -40,41 +40,50 @@ type IntegrationDERPTestSuite struct { pool dockertest.Pool networks map[int]dockertest.Network // so we keep the containers isolated headscale dockertest.Resource + saveLogs bool tailscales map[string]dockertest.Resource joinWaitGroup sync.WaitGroup } func TestDERPIntegrationTestSuite(t *testing.T) { + saveLogs, err := GetEnvBool("HEADSCALE_INTEGRATION_SAVE_LOG") + if err != nil { + saveLogs = false + } + s := new(IntegrationDERPTestSuite) s.tailscales = make(map[string]dockertest.Resource) s.networks = make(map[int]dockertest.Network) + s.saveLogs = saveLogs suite.Run(t, s) // HandleStats, which allows us to check if we passed and save logs // is called after TearDown, so we cannot tear down containers before // we have potentially saved the logs. - for _, tailscale := range s.tailscales { - if err := s.pool.Purge(&tailscale); err != nil { + if s.saveLogs { + for _, tailscale := range s.tailscales { + if err := s.pool.Purge(&tailscale); err != nil { + log.Printf("Could not purge resource: %s\n", err) + } + } + + if !s.stats.Passed() { + err := s.saveLog(&s.headscale, "test_output") + if err != nil { + log.Printf("Could not save log: %s\n", err) + } + } + if err := s.pool.Purge(&s.headscale); err != nil { log.Printf("Could not purge resource: %s\n", err) } - } - if !s.stats.Passed() { - err := s.saveLog(&s.headscale, "test_output") - if err != nil { - log.Printf("Could not save log: %s\n", err) - } - } - if err := s.pool.Purge(&s.headscale); err != nil { - log.Printf("Could not purge resource: %s\n", err) - } - - for _, network := range s.networks { - if err := network.Close(); err != nil { - log.Printf("Could not close network: %s\n", err) + for _, network := range s.networks { + if err := network.Close(); err != nil { + log.Printf("Could not close network: %s\n", err) + } } } } @@ -290,6 +299,23 @@ func (s *IntegrationDERPTestSuite) tailscaleContainer( } func (s *IntegrationDERPTestSuite) TearDownSuite() { + if !s.saveLogs { + for _, tailscale := range s.tailscales { + if err := s.pool.Purge(&tailscale); err != nil { + log.Printf("Could not purge resource: %s\n", err) + } + } + + if err := s.pool.Purge(&s.headscale); err != nil { + log.Printf("Could not purge resource: %s\n", err) + } + + for _, network := range s.networks { + if err := network.Close(); err != nil { + log.Printf("Could not close network: %s\n", err) + } + } + } } func (s *IntegrationDERPTestSuite) HandleStats( diff --git a/integration_test.go b/integration_test.go index 18f28b28..22cc0ae5 100644 --- a/integration_test.go +++ b/integration_test.go @@ -36,6 +36,7 @@ type IntegrationTestSuite struct { pool dockertest.Pool network dockertest.Network headscale dockertest.Resource + saveLogs bool namespaces map[string]TestNamespace @@ -43,6 +44,11 @@ type IntegrationTestSuite struct { } func TestIntegrationTestSuite(t *testing.T) { + saveLogs, err := GetEnvBool("HEADSCALE_INTEGRATION_SAVE_LOG") + if err != nil { + saveLogs = false + } + s := new(IntegrationTestSuite) s.namespaces = map[string]TestNamespace{ @@ -55,32 +61,35 @@ func TestIntegrationTestSuite(t *testing.T) { tailscales: make(map[string]dockertest.Resource), }, } + s.saveLogs = saveLogs suite.Run(t, s) // HandleStats, which allows us to check if we passed and save logs // is called after TearDown, so we cannot tear down containers before // we have potentially saved the logs. - for _, scales := range s.namespaces { - for _, tailscale := range scales.tailscales { - if err := s.pool.Purge(&tailscale); err != nil { - log.Printf("Could not purge resource: %s\n", err) + if s.saveLogs { + for _, scales := range s.namespaces { + for _, tailscale := range scales.tailscales { + if err := s.pool.Purge(&tailscale); err != nil { + log.Printf("Could not purge resource: %s\n", err) + } } } - } - if !s.stats.Passed() { - err := s.saveLog(&s.headscale, "test_output") - if err != nil { - log.Printf("Could not save log: %s\n", err) + if !s.stats.Passed() { + err := s.saveLog(&s.headscale, "test_output") + if err != nil { + log.Printf("Could not save log: %s\n", err) + } + } + if err := s.pool.Purge(&s.headscale); err != nil { + log.Printf("Could not purge resource: %s\n", err) } - } - if err := s.pool.Purge(&s.headscale); err != nil { - log.Printf("Could not purge resource: %s\n", err) - } - if err := s.network.Close(); err != nil { - log.Printf("Could not close network: %s\n", err) + if err := s.network.Close(); err != nil { + log.Printf("Could not close network: %s\n", err) + } } } @@ -338,6 +347,23 @@ func (s *IntegrationTestSuite) SetupSuite() { } func (s *IntegrationTestSuite) TearDownSuite() { + if !s.saveLogs { + for _, scales := range s.namespaces { + for _, tailscale := range scales.tailscales { + if err := s.pool.Purge(&tailscale); err != nil { + log.Printf("Could not purge resource: %s\n", err) + } + } + } + + if err := s.pool.Purge(&s.headscale); err != nil { + log.Printf("Could not purge resource: %s\n", err) + } + + if err := s.network.Close(); err != nil { + log.Printf("Could not close network: %s\n", err) + } + } } func (s *IntegrationTestSuite) HandleStats( From 8cae4f80d79a5baeccd2f41e4d09c1faa17b6f18 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Mon, 27 Jun 2022 11:58:16 +0000 Subject: [PATCH 2/5] Fail tests instead of fatal Currently we exit the program if the setup does not work, this can cause is to leave containers and other resources behind since we dont run TearDown. This change will just fail the test if we cant set up, which should mean that the TearDown runs aswell. --- integration_embedded_derp_test.go | 8 ++++---- integration_test.go | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/integration_embedded_derp_test.go b/integration_embedded_derp_test.go index d8918f4e..03501395 100644 --- a/integration_embedded_derp_test.go +++ b/integration_embedded_derp_test.go @@ -92,14 +92,14 @@ func (s *IntegrationDERPTestSuite) SetupSuite() { if ppool, err := dockertest.NewPool(""); err == nil { s.pool = *ppool } else { - log.Fatalf("Could not connect to docker: %s", err) + s.FailNow(fmt.Sprintf("Could not connect to docker: %s", err), "") } for i := 0; i < totalContainers; i++ { if pnetwork, err := s.pool.CreateNetwork(fmt.Sprintf("headscale-derp-%d", i)); err == nil { s.networks[i] = *pnetwork } else { - log.Fatalf("Could not create network: %s", err) + s.FailNow(fmt.Sprintf("Could not create network: %s", err), "") } } @@ -110,7 +110,7 @@ func (s *IntegrationDERPTestSuite) SetupSuite() { currentPath, err := os.Getwd() if err != nil { - log.Fatalf("Could not determine current path: %s", err) + s.FailNow(fmt.Sprintf("Could not determine current path: %s", err), "") } headscaleOptions := &dockertest.RunOptions{ @@ -133,7 +133,7 @@ func (s *IntegrationDERPTestSuite) SetupSuite() { if pheadscale, err := s.pool.BuildAndRunWithBuildOptions(headscaleBuildOptions, headscaleOptions, DockerRestartPolicy); err == nil { s.headscale = *pheadscale } else { - log.Fatalf("Could not start headscale container: %s", err) + s.FailNow(fmt.Sprintf("Could not start headscale container: %s", err), "") } log.Println("Created headscale container to test DERP") diff --git a/integration_test.go b/integration_test.go index 22cc0ae5..119fd4f7 100644 --- a/integration_test.go +++ b/integration_test.go @@ -218,13 +218,13 @@ func (s *IntegrationTestSuite) SetupSuite() { if ppool, err := dockertest.NewPool(""); err == nil { s.pool = *ppool } else { - log.Fatalf("Could not connect to docker: %s", err) + s.FailNow(fmt.Sprintf("Could not connect to docker: %s", err), "") } if pnetwork, err := s.pool.CreateNetwork("headscale-test"); err == nil { s.network = *pnetwork } else { - log.Fatalf("Could not create network: %s", err) + s.FailNow(fmt.Sprintf("Could not create network: %s", err), "") } headscaleBuildOptions := &dockertest.BuildOptions{ @@ -234,7 +234,7 @@ func (s *IntegrationTestSuite) SetupSuite() { currentPath, err := os.Getwd() if err != nil { - log.Fatalf("Could not determine current path: %s", err) + s.FailNow(fmt.Sprintf("Could not determine current path: %s", err), "") } headscaleOptions := &dockertest.RunOptions{ @@ -250,7 +250,7 @@ func (s *IntegrationTestSuite) SetupSuite() { if pheadscale, err := s.pool.BuildAndRunWithBuildOptions(headscaleBuildOptions, headscaleOptions, DockerRestartPolicy); err == nil { s.headscale = *pheadscale } else { - log.Fatalf("Could not start headscale container: %s", err) + s.FailNow(fmt.Sprintf("Could not start headscale container: %s", err), "") } log.Println("Created headscale container") From 3777de7133ebf0cb55c6c1820df264a60544e403 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Mon, 27 Jun 2022 12:00:21 +0000 Subject: [PATCH 3/5] Use failnow for cli tests aswell --- integration_cli_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/integration_cli_test.go b/integration_cli_test.go index 8ac6ee4d..11daae9d 100644 --- a/integration_cli_test.go +++ b/integration_cli_test.go @@ -40,13 +40,13 @@ func (s *IntegrationCLITestSuite) SetupTest() { if ppool, err := dockertest.NewPool(""); err == nil { s.pool = *ppool } else { - log.Fatalf("Could not connect to docker: %s", err) + s.FailNow(fmt.Sprintf("Could not connect to docker: %s", err), "") } if pnetwork, err := s.pool.CreateNetwork("headscale-test"); err == nil { s.network = *pnetwork } else { - log.Fatalf("Could not create network: %s", err) + s.FailNow(fmt.Sprintf("Could not create network: %s", err), "") } headscaleBuildOptions := &dockertest.BuildOptions{ @@ -56,7 +56,7 @@ func (s *IntegrationCLITestSuite) SetupTest() { currentPath, err := os.Getwd() if err != nil { - log.Fatalf("Could not determine current path: %s", err) + s.FailNow(fmt.Sprintf("Could not determine current path: %s", err), "") } headscaleOptions := &dockertest.RunOptions{ @@ -72,7 +72,7 @@ func (s *IntegrationCLITestSuite) SetupTest() { if pheadscale, err := s.pool.BuildAndRunWithBuildOptions(headscaleBuildOptions, headscaleOptions, DockerRestartPolicy); err == nil { s.headscale = *pheadscale } else { - log.Fatalf("Could not start headscale container: %s", err) + s.FailNow(fmt.Sprintf("Could not start headscale container: %s", err), "") } fmt.Println("Created headscale container") From 32a6151df95c38eb58ca53ea95609474f6b164ef Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Mon, 27 Jun 2022 12:02:29 +0000 Subject: [PATCH 4/5] Rerun integration tests 5 times if error --- .github/workflows/test-integration.yml | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/.github/workflows/test-integration.yml b/.github/workflows/test-integration.yml index 2ac023ac..70b36b1c 100644 --- a/.github/workflows/test-integration.yml +++ b/.github/workflows/test-integration.yml @@ -27,4 +27,9 @@ jobs: - name: Run Integration tests if: steps.changed-files.outputs.any_changed == 'true' - run: nix develop --command -- make test_integration + uses: nick-fields/retry@v2 + with: + timeout_minutes: 240 + max_attempts: 5 + retry_on: error + command: nix develop --command -- make test_integration From 566b8c3df34575fbf2fbd0d340059905c87616b2 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Mon, 27 Jun 2022 12:07:30 +0000 Subject: [PATCH 5/5] Fix issue were dockertest fails to start because of container mismatch --- integration_cli_test.go | 5 +++++ integration_embedded_derp_test.go | 5 +++++ integration_test.go | 5 +++++ 3 files changed, 15 insertions(+) diff --git a/integration_cli_test.go b/integration_cli_test.go index 11daae9d..f9ff5ec0 100644 --- a/integration_cli_test.go +++ b/integration_cli_test.go @@ -68,6 +68,11 @@ func (s *IntegrationCLITestSuite) SetupTest() { Cmd: []string{"headscale", "serve"}, } + err = s.pool.RemoveContainerByName(headscaleHostname) + if err != nil { + s.FailNow(fmt.Sprintf("Could not remove existing container before building test: %s", err), "") + } + fmt.Println("Creating headscale container") if pheadscale, err := s.pool.BuildAndRunWithBuildOptions(headscaleBuildOptions, headscaleOptions, DockerRestartPolicy); err == nil { s.headscale = *pheadscale diff --git a/integration_embedded_derp_test.go b/integration_embedded_derp_test.go index 03501395..ecca8ba5 100644 --- a/integration_embedded_derp_test.go +++ b/integration_embedded_derp_test.go @@ -129,6 +129,11 @@ func (s *IntegrationDERPTestSuite) SetupSuite() { }, } + err = s.pool.RemoveContainerByName(headscaleHostname) + if err != nil { + s.FailNow(fmt.Sprintf("Could not remove existing container before building test: %s", err), "") + } + log.Println("Creating headscale container") if pheadscale, err := s.pool.BuildAndRunWithBuildOptions(headscaleBuildOptions, headscaleOptions, DockerRestartPolicy); err == nil { s.headscale = *pheadscale diff --git a/integration_test.go b/integration_test.go index 119fd4f7..2214b893 100644 --- a/integration_test.go +++ b/integration_test.go @@ -246,6 +246,11 @@ func (s *IntegrationTestSuite) SetupSuite() { Cmd: []string{"headscale", "serve"}, } + err = s.pool.RemoveContainerByName(headscaleHostname) + if err != nil { + s.FailNow(fmt.Sprintf("Could not remove existing container before building test: %s", err), "") + } + log.Println("Creating headscale container") if pheadscale, err := s.pool.BuildAndRunWithBuildOptions(headscaleBuildOptions, headscaleOptions, DockerRestartPolicy); err == nil { s.headscale = *pheadscale