From 7983a7506ff645621256f9311716b54ed230a9ff Mon Sep 17 00:00:00 2001 From: Souhaila Noor Date: Fri, 11 Feb 2022 11:26:00 -0600 Subject: [PATCH 1/5] - Need to get code coverage on the server and client side - For server side, used an instrumented binary with a test that wraps around the main entrypoint for featurebase - Every time, the binary is called, a new coverage file is generated. - For the client side, used the standard -coverprofile flag for go test to generate code coverage - For backup test that's expected to fail, needed to call Run call in backup.go directly. The code coverage is not written to disk for an instrumented binary if there is an error. --- .gitlab/.gitlab-ci.yml | 11 +++- Dockerfile-clustertests | 10 ++-- Dockerfile-clustertests-client | 10 ++-- cmd/featurebase/main_test.go | 13 +++++ internal/clustertests/cluster_test.go | 64 +++++++++++++----------- internal/clustertests/docker-compose.yml | 15 ++++-- 6 files changed, 79 insertions(+), 44 deletions(-) create mode 100644 cmd/featurebase/main_test.go diff --git a/.gitlab/.gitlab-ci.yml b/.gitlab/.gitlab-ci.yml index 15ac7c1b2..84ee2acce 100644 --- a/.gitlab/.gitlab-ci.yml +++ b/.gitlab/.gitlab-ci.yml @@ -126,17 +126,18 @@ run go tests future: - aws upload to sonarcloud: - stage: test + stage: integration image: sonarsource/sonar-scanner-cli:4.6 variables: SONAR_TOKEN: $SONAR_TOKEN rules: - if: '$CI_PIPELINE_SOURCE == "push" || $CI_PIPELINE_SOURCE == "schedule" || $CI_PIPELINE_SOURCE == "web"' script: - - sonar-scanner -Dsonar.projectKey=molecula_featurebase -Dsonar.organization=molecula -Dsonar.sources=. -Dsonar.host.url=https://sonarcloud.io -Dsonar.go.coverage.reportPaths=coverage.out -Dsonar.go.tests.reportPaths=test-report.out -Dsonar.javascript.lcov.reportPaths=lattice/coverage/lcov.info + - sonar-scanner -Dsonar.projectKey=molecula_featurebase -Dsonar.organization=molecula -Dsonar.sources=. -Dsonar.host.url=https://sonarcloud.io -Dsonar.go.coverage.reportPaths=coverage.out,results/coverage*.out -Dsonar.go.tests.reportPaths=test-report.out,results/report* -Dsonar.javascript.lcov.reportPaths=lattice/coverage/lcov.info needs: - job: run go tests future - job: run jest tests + - job: clustertests build for linux amd64: stage: build @@ -289,7 +290,12 @@ clustertests: rules: - if: '$CI_PIPELINE_SOURCE == "push" || $CI_PIPELINE_SOURCE == "schedule" || $CI_PIPELINE_SOURCE == "web"' script: + - rm -rf internal/clustertests/results && mkdir -p internal/clustertests/results && chown gitlab-runner:gitlab-runner internal/clustertests/results - make clustertests + - mv internal/clustertests/results/ results/ + artifacts: + paths: + - results/coverage*.out authclustertests: variables: @@ -302,6 +308,7 @@ authclustertests: script: - make authclustertests + external lookup tests: stage: integration image: golang:$GOVERSION diff --git a/Dockerfile-clustertests b/Dockerfile-clustertests index bcf80ac95..be67afe75 100644 --- a/Dockerfile-clustertests +++ b/Dockerfile-clustertests @@ -7,8 +7,6 @@ LABEL maintainer "dev@pilosa.com" COPY . /go/src/github.com/molecula/featurebase/ -RUN cd /go/src/github.com/molecula/featurebase \ - && make install FLAGS="-a -mod=vendor" # download pumba for fault injection ADD https://github.com/alexei-led/pumba/releases/download/0.6.0/pumba_linux_amd64 /pumba @@ -22,7 +20,11 @@ RUN apt install -y docker.io ADD https://github.com/docker/compose/releases/latest/download/docker-compose-Linux-x86_64 /usr/local/bin/docker-compose RUN chmod +x /usr/local/bin/docker-compose -RUN cp /go/bin/featurebase /featurebase +# generate an instrumented binary to allow for calculating code coverage for clustertests +# the entrypoint for the binary is TestRunMain, which is wrapper for main +RUN cd /go/src/github.com/molecula/featurebase/cmd/featurebase && \ + go test -covermode=atomic -coverpkg=../../... -c -tags testrunmain -o featurebase && \ + cp /go/src/github.com/molecula/featurebase/cmd/featurebase/featurebase /featurebase COPY NOTICE /NOTICE @@ -30,4 +32,4 @@ EXPOSE 10101 VOLUME /data ENTRYPOINT ["bash", "-c"] -CMD ["/featurebase", "server", "--data-dir", "/data", "--bind", "http://0.0.0.0:10101"] +CMD ["/featurebase", "-test.run=TestRunMain", "-test.coverprofile=/results/coverage.out", "server", "--data-dir", "/data", "--bind", "http://0.0.0.0:10101"] diff --git a/Dockerfile-clustertests-client b/Dockerfile-clustertests-client index 553bffe03..2fb8cedf9 100644 --- a/Dockerfile-clustertests-client +++ b/Dockerfile-clustertests-client @@ -7,9 +7,6 @@ LABEL maintainer "dev@pilosa.com" COPY . /go/src/github.com/molecula/featurebase/ -RUN cd /go/src/github.com/molecula/featurebase \ - && make install FLAGS="-a -mod=vendor" - # download pumba for fault injection ADD https://github.com/alexei-led/pumba/releases/download/0.6.0/pumba_linux_amd64 /pumba RUN chmod +x /pumba @@ -22,7 +19,10 @@ RUN apt install -y docker.io ADD https://github.com/docker/compose/releases/latest/download/docker-compose-Linux-x86_64 /usr/local/bin/docker-compose RUN chmod +x /usr/local/bin/docker-compose -RUN cp /go/bin/featurebase /featurebase +RUN cd /go/src/github.com/molecula/featurebase/cmd/featurebase && \ + go test -covermode=atomic -coverpkg=../../... -c -tags testrunmain -o featurebase && \ + cp /go/src/github.com/molecula/featurebase/cmd/featurebase/featurebase /featurebase + COPY NOTICE /NOTICE @@ -32,4 +32,4 @@ EXPOSE 10101 VOLUME /data ENTRYPOINT ["bash", "-c"] -CMD ["/featurebase", "server", "--data-dir", "/data", "--bind", "http://0.0.0.0:10101"] +CMD ["/featurebase", "-test.run=TestRunMain", "-test.coverprofile=/results/coverage.out", "server", "--data-dir", "/data", "--bind", "http://0.0.0.0:10101"] diff --git a/cmd/featurebase/main_test.go b/cmd/featurebase/main_test.go new file mode 100644 index 000000000..c894d2e48 --- /dev/null +++ b/cmd/featurebase/main_test.go @@ -0,0 +1,13 @@ +//go:build testrunmain +// +build testrunmain + +package main + +import ( + "testing" +) + +// Wrapper test for main function used to get code coverage for end2end tests +func TestRunMain(t *testing.T) { + main() +} diff --git a/internal/clustertests/cluster_test.go b/internal/clustertests/cluster_test.go index a54f6256b..490121d0d 100644 --- a/internal/clustertests/cluster_test.go +++ b/internal/clustertests/cluster_test.go @@ -2,6 +2,7 @@ package clustertest import ( + "bufio" "bytes" "context" "fmt" @@ -16,6 +17,7 @@ import ( "github.com/golang-jwt/jwt" pilosa "github.com/molecula/featurebase/v3" "github.com/molecula/featurebase/v3/authn" + "github.com/molecula/featurebase/v3/ctl" "github.com/molecula/featurebase/v3/disco" "github.com/molecula/featurebase/v3/encoding/proto" "github.com/molecula/featurebase/v3/logger" @@ -178,17 +180,18 @@ func TestClusterStuff(t *testing.T) { var backupCmd *exec.Cmd tmpdir := t.TempDir() + // collect code coverage while doing backup using an instrumented binary by calling + // a wrapper test (TestRunMain) for the main entrypoint of featurebase + args := []string{"-test.run=TestRunMain", "-test.coverprofile=/results/coverage-backup.out", "backup", + "--host=pilosa1:10101", fmt.Sprintf("--output=%s", tmpdir+"/backuptest")} if auth { - if backupCmd, err = startCmd( - "featurebase", "backup", "--host=pilosa1:10101", fmt.Sprintf("--output=%s", tmpdir+"/backuptest"), "--auth-token", token); err != nil { - t.Fatalf("sending backup command: %v", err) - } - } else { - if backupCmd, err = startCmd( - "featurebase", "backup", "--host=pilosa1:10101", fmt.Sprintf("--output=%s", tmpdir+"/backuptest")); err != nil { - t.Fatalf("sending backup command: %v", err) - } + args = append(args, fmt.Sprintf("--auth-token=%s", token)) } + + if backupCmd, err = startCmd("/featurebase", args...); err != nil { + t.Fatalf("sending backup command: %v", err) + } + time.Sleep(time.Second * 5) if err = sendCmd("docker", "start", container(t, "pilosa1")); err != nil { t.Fatalf("sending start command: %v", err) @@ -216,15 +219,15 @@ func TestClusterStuff(t *testing.T) { } var restoreCmd *exec.Cmd + args = []string{"-test.run=TestRunMain", "-test.coverprofile=/results/coverage-restore.out", "restore", + "-s", tmpdir + "/backuptest", "--host", "pilosa1:10101"} if auth { - if restoreCmd, err = startCmd("featurebase", "restore", "-s", tmpdir+"/backuptest", "--host", "pilosa1:10101", "--auth-token", token); err != nil { - t.Fatalf("starting restore: %v", err) - } - } else { - if restoreCmd, err = startCmd("featurebase", "restore", "-s", tmpdir+"/backuptest", "--host", "pilosa1:10101"); err != nil { - t.Fatalf("starting restore: %v", err) - } + args = append(args, fmt.Sprintf("--auth-token=%s", token)) } + if restoreCmd, err = startCmd("/featurebase", args...); err != nil { + t.Fatalf("starting restore: %v", err) + } + time.Sleep(time.Millisecond * 50) if err = sendCmd("docker", "stop", container(t, "pilosa2")); err != nil { t.Fatalf("sending stop command: %v", err) @@ -250,16 +253,23 @@ func TestClusterStuff(t *testing.T) { // now do backup with all nodes down and too short a timeout // so it fails. Has be to be all 3 because the cluster has // replicas=3 and the backup command will retry on replicas. + // featurebase backup cmd can't be used for a test expected to fail + // because code coverage report won't be generated. + buf := bytes.Buffer{} + rder := []byte{} + stdin := bytes.NewReader(rder) + stdout := bufio.NewWriter(&buf) + stderr := bufio.NewWriter(&buf) + backup := ctl.NewBackupCommand(stdin, stdout, stderr) + backup.Host = "--host=pilosa1:10101" + backup.OutputDir = tmpdir + "/backuptest2" + backup.RetryPeriod = time.Millisecond * 200 if auth { - if backupCmd, err = startCmd( - "featurebase", "backup", "--host=pilosa1:10101", fmt.Sprintf("--output=%s", tmpdir+"/backuptest2"), "--retry-period=200ms", "--auth-token", token); err != nil { - t.Fatalf("sending second backup command: %v", err) - } - } else { - if backupCmd, err = startCmd( - "featurebase", "backup", "--host=pilosa1:10101", fmt.Sprintf("--output=%s", tmpdir+"/backuptest2"), "--retry-period=200ms"); err != nil { - t.Fatalf("sending second backup command: %v", err) - } + backup.AuthToken = token + } + + if err = backup.Run(context.Background()); err == nil { + t.Fatal("backup command should have errored but didn't") } t.Logf("sleeping 8s") @@ -275,10 +285,6 @@ func TestClusterStuff(t *testing.T) { 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") - } - }) } diff --git a/internal/clustertests/docker-compose.yml b/internal/clustertests/docker-compose.yml index 42476bb33..ee7ebc496 100644 --- a/internal/clustertests/docker-compose.yml +++ b/internal/clustertests/docker-compose.yml @@ -15,8 +15,10 @@ services: - PILOSA_CLUSTER_REPLICAS=3 networks: - pilosanet + volumes: + - ./results:/results command: - - "/featurebase server --bind pilosa1:10101 ${CLUSTERTESTS_FB_ARGS}" + - "cd /go/src/github.com/molecula/featurebase/cmd/featurebase && /featurebase -test.run=TestRunMain -test.coverprofile=/results/coverage-server1.out server --bind pilosa1:10101 ${CLUSTERTESTS_FB_ARGS}" pilosa2: build: context: ../.. @@ -32,8 +34,10 @@ services: - PILOSA_CLUSTER_REPLICAS=3 networks: - pilosanet + volumes: + - ./results:/results command: - - "/featurebase server --bind pilosa2:10101 ${CLUSTERTESTS_FB_ARGS}" + - "cd /go/src/github.com/molecula/featurebase/cmd/featurebase && /featurebase -test.run=TestRunMain -test.coverprofile=/results/coverage-server2.out server --bind pilosa2:10101 ${CLUSTERTESTS_FB_ARGS}" pilosa3: build: context: ../.. @@ -49,8 +53,10 @@ services: - PILOSA_CLUSTER_REPLICAS=3 networks: - pilosanet + volumes: + - ./results:/results command: - - "/featurebase server --bind pilosa3:10101 ${CLUSTERTESTS_FB_ARGS}" + - "cd /go/src/github.com/molecula/featurebase/cmd/featurebase && /featurebase -test.run=TestRunMain -test.coverprofile=/results/coverage-server3.out server --bind pilosa3:10101 ${CLUSTERTESTS_FB_ARGS}" client1: build: context: ../.. @@ -69,8 +75,9 @@ services: - pilosanet volumes: - /var/run/docker.sock:/var/run/docker.sock + - ./results:/results command: - - "cd /go/src/github.com/molecula/featurebase/ && go test -mod=vendor -v -count=1 github.com/molecula/featurebase/v3/internal/clustertests" + - "cd /go/src/github.com/molecula/featurebase/ && go test -mod=vendor -v -count=1 -covermode=atomic -coverprofile=/results/coverage-clustertests.out -coverpkg=./... -json github.com/molecula/featurebase/v3/internal/clustertests | tee /results/report-clustertests.out" fakeidp: build: context: . From 102a6e723b6574153f877f39ecd4d7fef5db9c48 Mon Sep 17 00:00:00 2001 From: Matthew Jaffee Date: Fri, 11 Feb 2022 08:51:10 -0600 Subject: [PATCH 2/5] more binding to 0==less port conflicts in CI --- cmd/server_test.go | 32 +++++++++++++++++++++++++++----- 1 file changed, 27 insertions(+), 5 deletions(-) diff --git a/cmd/server_test.go b/cmd/server_test.go index 91263c0ed..df47bed5f 100644 --- a/cmd/server_test.go +++ b/cmd/server_test.go @@ -37,7 +37,7 @@ func TestServerConfig(t *testing.T) { tests := []commandTest{ // TEST 0 { - args: []string{"server", "--data-dir", actualDataDir, "--bind", "localhost:42454", "--bind-grpc", "localhost:30112", "--translation.map-size", "100000"}, + args: []string{"server", "--data-dir", actualDataDir, "--translation.map-size", "100000"}, env: map[string]string{ "PILOSA_DATA_DIR": "/tmp/myEnvDatadir", "PILOSA_LONG_QUERY_TIME": "1m30s", @@ -56,6 +56,10 @@ func TestServerConfig(t *testing.T) { [cluster] replicas = 2 long-query-time = "1m10s" + [etcd] + listen-client-address = "http://localhost:0" + listen-peer-address = "http://localhost:0" + initial-cluster = "pilosa0=http://localhost:0" [profile] block-rate = 100 mutex-fraction = 10 @@ -63,7 +67,6 @@ func TestServerConfig(t *testing.T) { validation: func() error { v := validator{} v.Check(cmd.Server.Config.DataDir, actualDataDir) - v.Check(cmd.Server.Config.Bind, "localhost:42454") v.Check(cmd.Server.Config.Cluster.ReplicaN, 2) v.Check(cmd.Server.Config.LongQueryTime, toml.Duration(time.Second*90)) v.Check(cmd.Server.Config.Cluster.LongQueryTime, toml.Duration(time.Second*90)) @@ -83,7 +86,6 @@ func TestServerConfig(t *testing.T) { }, env: map[string]string{ "PILOSA_CLUSTER_HOSTS": "localhost:1110,localhost:1111", - "PILOSA_BIND": "localhost:1110", "PILOSA_TRANSLATION_MAP_SIZE": "100000", "PILOSA_PROFILE_BLOCK_RATE": "9123", "PILOSA_PROFILE_MUTEX_FRACTION": "444", @@ -92,6 +94,10 @@ func TestServerConfig(t *testing.T) { bind = ` + nextPort() + ` bind-grpc = ` + nextPort() + ` data-dir = "` + actualDataDir + `" + [etcd] + listen-client-address = "http://localhost:0" + listen-peer-address = "http://localhost:0" + initial-cluster = "pilosa0=http://localhost:0" [profile] block-rate = 100 mutex-fraction = 10 @@ -110,9 +116,13 @@ func TestServerConfig(t *testing.T) { args: []string{"server", "--log-path", logFile.Name(), "--translation.map-size", "100000"}, env: map[string]string{}, cfgFileContent: ` - bind = "localhost:19444" - bind-grpc = "localhost:29444" + bind = ` + nextPort() + ` + bind-grpc = ` + nextPort() + ` data-dir = "` + actualDataDir + `" + [etcd] + listen-client-address = "http://localhost:0" + listen-peer-address = "http://localhost:0" + initial-cluster = "pilosa0=http://localhost:0" [anti-entropy] interval = "11m0s" [metric] @@ -191,6 +201,10 @@ func TestServerConfig_DeprecateLongQueryTime(t *testing.T) { bind = ` + nextPort() + ` bind-grpc = ` + nextPort() + ` data-dir = "` + actualDataDir + `" + [etcd] + listen-client-address = "http://localhost:0" + listen-peer-address = "http://localhost:0" + initial-cluster = "pilosa0=http://localhost:0" `, validation: func() error { v := validator{} @@ -207,6 +221,10 @@ func TestServerConfig_DeprecateLongQueryTime(t *testing.T) { bind = ` + nextPort() + ` bind-grpc = ` + nextPort() + ` data-dir = "` + actualDataDir + `" + [etcd] + listen-client-address = "http://localhost:0" + listen-peer-address = "http://localhost:0" + initial-cluster = "pilosa0=http://localhost:0" `, validation: func() error { v := validator{} @@ -223,6 +241,10 @@ func TestServerConfig_DeprecateLongQueryTime(t *testing.T) { bind = ` + nextPort() + ` bind-grpc = ` + nextPort() + ` data-dir = "` + actualDataDir + `" + [etcd] + listen-client-address = "http://localhost:0" + listen-peer-address = "http://localhost:0" + initial-cluster = "pilosa0=http://localhost:0" `, validation: func() error { v := validator{} From 53a33134d9024aa19976fbf171b699b4767663d3 Mon Sep 17 00:00:00 2001 From: Matthew Jaffee Date: Fri, 11 Feb 2022 09:41:11 -0600 Subject: [PATCH 3/5] add verbose output to race tests --- .gitlab/.gitlab-ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.gitlab/.gitlab-ci.yml b/.gitlab/.gitlab-ci.yml index 84ee2acce..75bf09877 100644 --- a/.gitlab/.gitlab-ci.yml +++ b/.gitlab/.gitlab-ci.yml @@ -90,7 +90,7 @@ run go tests race: retry: 1 script: - echo "Running featurebase race tests..." - - go test -race -timeout=90m ./... + - go test -race -v -timeout=90m ./... tags: - aws From b5dae698ffd93ab0a5bf4cbc139c4ffb1a2bd7b3 Mon Sep 17 00:00:00 2001 From: Matthew Jaffee Date: Fri, 11 Feb 2022 09:48:18 -0600 Subject: [PATCH 4/5] remove unused env var from test cluster.hosts is no longer a config option since move to etcd --- cmd/server_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/cmd/server_test.go b/cmd/server_test.go index df47bed5f..e7b42002f 100644 --- a/cmd/server_test.go +++ b/cmd/server_test.go @@ -85,7 +85,6 @@ func TestServerConfig(t *testing.T) { "--profile.mutex-fraction", "8290", }, env: map[string]string{ - "PILOSA_CLUSTER_HOSTS": "localhost:1110,localhost:1111", "PILOSA_TRANSLATION_MAP_SIZE": "100000", "PILOSA_PROFILE_BLOCK_RATE": "9123", "PILOSA_PROFILE_MUTEX_FRACTION": "444", From 6d06f5550b92cccb1a52f2a201a580071bb5c9a7 Mon Sep 17 00:00:00 2001 From: Ben Johnson Date: Wed, 9 Feb 2022 14:05:57 -0700 Subject: [PATCH 5/5] Restrict max-memory to Extract() calls only --- executor.go | 2 +- pql/ast.go | 24 ++++++++++++++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/executor.go b/executor.go index 1736ce94a..3b09cd92c 100644 --- a/executor.go +++ b/executor.go @@ -257,7 +257,7 @@ func (e *executor) Execute(ctx context.Context, index string, q *pql.Query, shar opt = &execOptions{} } // Default maximum memory, if not passed in. - if opt.MaxMemory == 0 { + if opt.MaxMemory == 0 && q.HasCall("Extract") { opt.MaxMemory = e.maxMemory } diff --git a/pql/ast.go b/pql/ast.go index 2f9ec02e8..f61ebba95 100644 --- a/pql/ast.go +++ b/pql/ast.go @@ -35,6 +35,16 @@ func (q *Query) ExpandVars(vars map[string]interface{}) (*Query, error) { return &other, nil } +// HasCall returns true if q contains the given call name. +func (q *Query) HasCall(name string) bool { + for _, c := range q.Calls { + if c.HasCall(name) { + return true + } + } + return false +} + func (q *Query) startCall(name string) { // Coerce every name into a canonical form if we know of one. if canon, ok := canonicalCaps[strings.ToLower(name)]; ok { @@ -349,6 +359,20 @@ type Call struct { Precomputed map[uint64]interface{} } +// HasCall returns true if q contains the given call name. +func (c *Call) HasCall(name string) bool { + if c.Name == name { + return true + } + + for _, child := range c.Children { + if child.HasCall(name) { + return true + } + } + return false +} + // callInfo defines the arguments allowed for a particular PQL call, and // possibly things about its semantics. If allowUnknown is true, unfamiliar // non-reserved names are allowed on the assumption that they're field names.