From 06235c3d7084153511828d67a2310bee9d6ed026 Mon Sep 17 00:00:00 2001 From: Matthew Jaffee Date: Wed, 2 Feb 2022 11:56:19 -0600 Subject: [PATCH] get container ID via "docker-compose" call in clustertests this should be a lot more reliable than trying to construct it based on the project name as the exact construction can differ between docker-compose versions. There was also an issue with the backups succeeding when they should fail in the test. There's an arcane maze of HTTP timeouts to navigate here, but basically there are situations where the client will just wait forever rather than erroring if the server is paused at the right(wrong) time. I'm not convinced we've solved every possible case of this, so we still may see the backup succeed even when it's supposed to fail. The ultimate hammer is to add Client.Timeout, but that's a very blunt instrument and I'm afraid it could cause a timeout when really we just have a lot of data to download or something. There may be a better way to say "only time out if you literally haven't heard a peep from the server in this long", but I haven't been able to figure it out yet. I also fixed how the authclustertests are run as they weren't using the PROJECT parameter correctly. Now they can run concurrently with clustertests, and with other copies of authclustertests without having conflicts. --- .gitlab/.gitlab-ci.yml | 13 +++- Makefile | 11 ++- http/client.go | 1 - http/handler.go | 7 +- internal/clustertests/cluster_test.go | 88 +++++++++++++----------- internal/clustertests/pause_node_test.go | 12 ++-- 6 files changed, 75 insertions(+), 57 deletions(-) diff --git a/.gitlab/.gitlab-ci.yml b/.gitlab/.gitlab-ci.yml index a44f9e30c..37dbf92e7 100644 --- a/.gitlab/.gitlab-ci.yml +++ b/.gitlab/.gitlab-ci.yml @@ -278,9 +278,18 @@ clustertests: - shell rules: - if: '$CI_PIPELINE_SOURCE == "push" || $CI_PIPELINE_SOURCE == "schedule" || $CI_PIPELINE_SOURCE == "web"' - allow_failure: true script: - make clustertests + +authclustertests: + variables: + PROJECT: authclustertests_${CI_CONCURRENT_ID} + stage: integration + tags: + - shell + rules: + - if: '$CI_PIPELINE_SOURCE == "push" || $CI_PIPELINE_SOURCE == "schedule" || $CI_PIPELINE_SOURCE == "web"' + script: - make authclustertests external lookup tests: @@ -444,4 +453,4 @@ s3 dump: - job: build for darwin amd64 - job: build for darwin arm64 - job: build for linux amd64 - - job: build for linux arm64 \ No newline at end of file + - job: build for linux arm64 diff --git a/Makefile b/Makefile index 5ee669d17..442e31764 100644 --- a/Makefile +++ b/Makefile @@ -159,13 +159,12 @@ clustertests: vendor $(DOCKER_COMPOSE) -f internal/clustertests/docker-compose.yml down # Run the cluster tests with authentication enabled -DOCKER_COMPOSE_AUTH = docker-compose -p authclustertests authclustertests: vendor - $(DOCKER_COMPOSE_AUTH) -f internal/authclustertests/docker-compose.yml down - $(DOCKER_COMPOSE_AUTH) -f internal/authclustertests/docker-compose.yml build - $(DOCKER_COMPOSE_AUTH) -f internal/authclustertests/docker-compose.yml up -d pilosa1 pilosa2 pilosa3 - $(DOCKER_COMPOSE_AUTH) -f internal/authclustertests/docker-compose.yml run client1 - $(DOCKER_COMPOSE_AUTH) -f internal/authclustertests/docker-compose.yml down + $(DOCKER_COMPOSE) -f internal/authclustertests/docker-compose.yml down + $(DOCKER_COMPOSE) -f internal/authclustertests/docker-compose.yml build + $(DOCKER_COMPOSE) -f internal/authclustertests/docker-compose.yml up -d pilosa1 pilosa2 pilosa3 + PROJECT=$(PROJECT) $(DOCKER_COMPOSE) -f internal/authclustertests/docker-compose.yml run client1 + $(DOCKER_COMPOSE) -f internal/authclustertests/docker-compose.yml down # Install Pilosa install: diff --git a/http/client.go b/http/client.go index 362ac053c..684c9f726 100644 --- a/http/client.go +++ b/http/client.go @@ -94,7 +94,6 @@ func WithClientRetryPeriod(period time.Duration) InternalClientOption { rc.RetryWaitMin = min rc.RetryMax = int(attempts) rc.CheckRetry = retryWith400Policy - rc.Logger = logger.NopLogger c.retryableClient = rc } } diff --git a/http/handler.go b/http/handler.go index 2439fa539..173988485 100644 --- a/http/handler.go +++ b/http/handler.go @@ -2866,8 +2866,8 @@ type ClientOption func(client *http.Client, dialer *net.Dialer) *http.Client func GetHTTPClient(t *tls.Config, opts ...ClientOption) *http.Client { dialer := &net.Dialer{ - Timeout: 30 * time.Second, - KeepAlive: 30 * time.Second, + Timeout: 5 * time.Second, + KeepAlive: 15 * time.Second, DualStack: true, } transport := &http.Transport{ @@ -2875,9 +2875,10 @@ func GetHTTPClient(t *tls.Config, opts ...ClientOption) *http.Client { DialContext: dialer.DialContext, MaxIdleConns: 1000, MaxIdleConnsPerHost: 200, - IdleConnTimeout: 90 * time.Second, + IdleConnTimeout: 20 * time.Second, TLSHandshakeTimeout: 10 * time.Second, ExpectContinueTimeout: 1 * time.Second, + ResponseHeaderTimeout: 4 * time.Second, } if t != nil { transport.TLSClientConfig = t diff --git a/internal/clustertests/cluster_test.go b/internal/clustertests/cluster_test.go index 4ad532e8a..ad31172e2 100644 --- a/internal/clustertests/cluster_test.go +++ b/internal/clustertests/cluster_test.go @@ -2,12 +2,14 @@ package clustertest import ( + "bytes" "context" "fmt" "io" "net/http" "os" "os/exec" + "strings" "testing" "time" @@ -17,24 +19,22 @@ import ( "github.com/molecula/featurebase/v3/disco" picli "github.com/molecula/featurebase/v3/http" "github.com/molecula/featurebase/v3/logger" + "github.com/pkg/errors" ) -// container turns a docker-compose service name into a container name -// assuming the project name is set in the enviroment as PROJECT. This -// refers to the "-p" argument to docker-compose. NOTE: this assumes -// docker-compose joins the project name with a separating -// underscore... this may not always be true as I've seen a dash used -// as well, but I think it is true in recent versions. -func container(svc string) string { +// container turns a docker-compose service name into a container ID +// by calling "docker-compose ps" +func container(t *testing.T, svc string) string { project := "clustertests" - if os.Getenv("ENABLE_AUTH") == "1" { - project = "authclustertests" - } - if p := os.Getenv("PROJECT"); p != "" { project = p } - return project + "_" + svc + "_1" + stdout, stderr, err := runCmd("docker-compose", "-p", project, "ps", "-q", svc) + if err != nil { + t.Fatalf("couldn't construct container name, err: %v, stderr:\n%s\nstdout:\n%s", err, stderr, stdout) + } + name := strings.Trim(stdout, "\n") + return name } func GetAuthToken(t *testing.T) string { @@ -144,18 +144,14 @@ func TestClusterStuff(t *testing.T) { } } t.Run("long pause", func(t *testing.T) { - pcmd := exec.Command("/pumba", "pause", container("pilosa3"), "--duration", "10s") - pcmd.Stdout = os.Stdout - pcmd.Stderr = os.Stderr + if err := sendCmd("docker", "pause", container(t, "pilosa3")); err != nil { + t.Fatalf("sending pause: %v", err) + } t.Log("pausing pilosa3 for 10s") - - if err := pcmd.Start(); err != nil { - t.Fatalf("starting pumba command: %v", err) + time.Sleep(time.Second * 10) + if err := sendCmd("docker", "unpause", container(t, "pilosa3")); err != nil { + t.Fatalf("sending unpause: %v", err) } - if err := pcmd.Wait(); err != nil { - t.Fatalf("waiting on pumba pause cmd: %v", err) - } - t.Log("done with pause, waiting for stability") waitForStatus(t, cli1.Status, string(disco.ClusterStateNormal), 30, time.Second, ctx) t.Log("done waiting for stability") @@ -174,7 +170,7 @@ func TestClusterStuff(t *testing.T) { t.Run("backup", func(t *testing.T) { // do backup with node 1 down, but restart it after a few seconds - if err := sendCmd("docker", "stop", container("pilosa1")); err != nil { + if err := sendCmd("docker", "stop", container(t, "pilosa1")); err != nil { t.Fatalf("sending stop command: %v", err) } var backupCmd *exec.Cmd @@ -192,7 +188,7 @@ func TestClusterStuff(t *testing.T) { } } time.Sleep(time.Second * 5) - if err = sendCmd("docker", "start", container("pilosa1")); err != nil { + if err = sendCmd("docker", "start", container(t, "pilosa1")); err != nil { t.Fatalf("sending start command: %v", err) } @@ -228,12 +224,12 @@ func TestClusterStuff(t *testing.T) { } } time.Sleep(time.Millisecond * 50) - if err = sendCmd("docker", "stop", container("pilosa2")); err != nil { + if err = sendCmd("docker", "stop", container(t, "pilosa2")); err != nil { t.Fatalf("sending stop command: %v", err) } time.Sleep(time.Second * 10) - if err = sendCmd("docker", "start", container("pilosa2")); err != nil { + if err = sendCmd("docker", "start", container(t, "pilosa2")); err != nil { t.Fatalf("sending stop command: %v", err) } if err := restoreCmd.Wait(); err != nil { @@ -255,26 +251,29 @@ func TestClusterStuff(t *testing.T) { } } time.Sleep(time.Millisecond * 10) // want the backup to get started, then fail - if err = sendCmd("docker", "stop", container("pilosa1")); err != nil { - t.Fatalf("sending stop command: %v", err) + fmt.Println("pausing all featurebasen") + if err = sendCmd("docker", "pause", container(t, "pilosa1")); err != nil { + t.Fatalf("sending pause command: %v", err) } - if err = sendCmd("docker", "stop", container("pilosa2")); err != nil { - t.Fatalf("sending stop command: %v", err) + if err = sendCmd("docker", "pause", container(t, "pilosa2")); err != nil { + t.Fatalf("sending pause command: %v", err) } - if err = sendCmd("docker", "stop", container("pilosa3")); err != nil { - t.Fatalf("sending stop command: %v", err) + if err = sendCmd("docker", "pause", container(t, "pilosa3")); err != nil { + t.Fatalf("sending pause command: %v", err) } - time.Sleep(time.Second * 5) + fmt.Println("sleeping long") + time.Sleep(time.Second * 10) + fmt.Println("restarting") - if err = sendCmd("docker", "start", container("pilosa1")); err != nil { - t.Fatalf("sending start command: %v", err) + if err = sendCmd("docker", "unpause", container(t, "pilosa1")); err != nil { + t.Fatalf("sending unpause command: %v", err) } - if err = sendCmd("docker", "start", container("pilosa2")); err != nil { - t.Fatalf("sending start command: %v", err) + if err = sendCmd("docker", "unpause", container(t, "pilosa2")); err != nil { + t.Fatalf("sending unpause command: %v", err) } - if err = sendCmd("docker", "start", container("pilosa3")); err != nil { - t.Fatalf("sending start command: %v", err) + if err = sendCmd("docker", "unpause", container(t, "pilosa3")); err != nil { + t.Fatalf("sending unpause command: %v", err) } if err = backupCmd.Wait(); err == nil { t.Fatal("backup command should have errored but didn't") @@ -308,3 +307,14 @@ func waitForStatus(t *testing.T, stator func(context.Context) (string, error), s t.Fatalf("waited %s for status: %s, got: %s", waited.String(), status, s) } } + +// runCmd is a helper which uses os.Exec to run a command and returns +// stdout and stderr as separate strings, and any error returned from +// Command.Run +func runCmd(name string, args ...string) (sout, serr string, err error) { + cmd := exec.Command(name, args...) + stdout, stderr := &bytes.Buffer{}, &bytes.Buffer{} + cmd.Stdout, cmd.Stderr = stdout, stderr + err = cmd.Run() + return stdout.String(), stderr.String(), errors.Wrap(err, "running command") +} diff --git a/internal/clustertests/pause_node_test.go b/internal/clustertests/pause_node_test.go index af053a6bf..b59dc9be1 100644 --- a/internal/clustertests/pause_node_test.go +++ b/internal/clustertests/pause_node_test.go @@ -43,13 +43,13 @@ func sendCmd(cmd string, args ...string) error { return nil } -func unpauseNode(node string) error { - unpauseArgs := []string{"container", "unpause", container(node)} +func unpauseNode(t *testing.T, node string) error { + unpauseArgs := []string{"container", "unpause", container(t, node)} return sendCmd("docker", unpauseArgs...) } -func pauseNode(node string) error { - pauseArgs := []string{"container", "pause", container(node)} +func pauseNode(t *testing.T, node string) error { + pauseArgs := []string{"container", "pause", container(t, node)} return sendCmd("docker", pauseArgs...) } @@ -346,7 +346,7 @@ func TestPauseReplica(t *testing.T) { // pause node t.Logf("pause %s", nodeToPause) - err = pauseNode(nodeToPause) + err = pauseNode(t, nodeToPause) if err != nil { t.Fatalf("error on pause node %s: %v", nodeToPause, err) } @@ -369,7 +369,7 @@ func TestPauseReplica(t *testing.T) { // wait for cluster status to get back to normal t.Logf("unpause %s", nodeToPause) - err = unpauseNode(nodeToPause) + err = unpauseNode(t, nodeToPause) if err != nil { t.Fatalf("error on unpause node %s: %v", nodeToPause, err) }