Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor and unify test outputs #467

Merged
merged 22 commits into from Feb 5, 2020
Merged

Refactor and unify test outputs #467

merged 22 commits into from Feb 5, 2020

Conversation

@raulk
Copy link
Member

raulk commented Feb 4, 2020

Implements #439.

This PR introduces a unified test outputs solution for testground. Concretely, it introduces the following:

  • Runenv now uses zap to record structured run events (JSON). Zap is configured to tee to stdout and to $TEST_OUTPUTS_PATH/run.out.
  • Runenv methods for recording run events:
    • RecordStart() (new event, which outputs the entire runenv, we now do this only once).
    • RecordMessage()
    • RecordFailure()
    • RecordCrash()
    • RecordMetric()
  • Runners no longer need to pipe instance stdouts into files.
  • The whole EventManager + loggers construction has been simplified into a component called PrettyPrinter, which basically does that: it processes the JSON run events and prints them out in a tabular, coloured format. Also added INTERNAL_ERR and OTHER event types.
  • Sidecar code consuming stdout and stderr and writing to files has been removed.
  • Stderr is now teed into stderr and $TEST_OUTPUTS_PATH/run.err.
  • local:exec and local:docker correctly inject the TEST_OUTPUTS_PATH value into processes/containers.
  • Runenv now allows creating output assets via two methods: runenv.CreateRawAsset() and runenv.CreateStructuredAsset(), the latter of which returns a zap sugared logger.
  • I've merged #411 into this branch, and rejigged it to use the new APIs.
  • Removed env.json dumping.

TODO (to finish off this epic)

  • cluster:k8s runner to inject the correct value for TEST_OUTPUTS_PATH (@nonsense).
  • testground collect command (#376) (@nonsense + @raulk).
  • Mooaaaaar testing.
@raulk raulk mentioned this pull request Feb 4, 2020
raulk added 3 commits Feb 4, 2020
@raulk raulk marked this pull request as ready for review Feb 5, 2020
This was referenced Feb 5, 2020
@raulk

This comment has been minimized.

Copy link
Member Author

raulk commented Feb 5, 2020

The testground collect command is being worked on here: #463. It will be merged into this branch.

@nonsense

This comment has been minimized.

Copy link
Member

nonsense commented Feb 5, 2020

Looks like something in the tests is failing:

 ---> Running in 5700e2f9a706
# github.com/ipfs/testground/plans/network
./main.go:115:14: cannot use &(*runenv.TestSubnet) (type *"github.com/ipfs/testground/sdk/runtime".IPNet) as type *net.IPNet in assignment
2020-02-05T00:40:52.442Z	INFO	engine/engine.go:239	build failed	{"plan": "network", "group": "single", "builder": "docker:go", "error": "The command '/bin/sh -c cd ${PLAN_DIR}     && go env -w GOPROXY=\"${GO_PROXY}\"     && CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go build -o testplan ${TESTPLAN_EXEC_PKG}' returned a non-zero code: 2"}
2020-02-05T00:40:52.443Z	WARN	tgwriter/tgwriter.go:85	engine build error: The command '/bin/sh -c cd ${PLAN_DIR}     && go env -w GOPROXY="${GO_PROXY}"     && CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go build -o testplan ${TESTPLAN_EXEC_PKG}' returned a non-zero code: 2	{"ruid": "28f762d2"}
github.com/ipfs/testground/pkg/tgwriter.(*TgWriter).WriteError
	/home/travis/gopath/src/github.com/ipfs/testground/pkg/tgwriter/tgwriter.go:85
github.com/ipfs/testground/pkg/daemon.(*Daemon).buildHandler.func1
	/home/travis/gopath/src/github.com/ipfs/testground/pkg/daemon/build.go:32
net/http.HandlerFunc.ServeHTTP
	/home/travis/.gimme/versions/go1.13.7.linux.amd64/src/net/http/server.go:2007
github.com/ipfs/testground/pkg/daemon.New.func1.1
	/home/travis/gopath/src/github.com/ipfs/testground/pkg/daemon/daemon.go:43
net/http.HandlerFunc.ServeHTTP
	/home/travis/.gimme/versions/go1.13.7.linux.amd64/src/net/http/server.go:2007
github.com/gorilla/mux.(*Router).ServeHTTP
	/home/travis/gopath/pkg/mod/github.com/gorilla/mux@v1.7.3/mux.go:212
net/http.serverHandler.ServeHTTP
	/home/travis/.gimme/versions/go1.13.7.linux.amd64/src/net/http/server.go:2802
net/http.(*conn).serve
	/home/travis/.gimme/versions/go1.13.7.linux.amd64/src/net/http/server.go:1890
--- FAIL: TestSidecar (122.43s)
* wip: boilerplate collect outputs

License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>

* wip: boilerplate zip output

License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>

* wip: test if it works w/ example

License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>

* feat: log error

License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>

* fix: just panic for now

License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>

* fix: rebase issues

License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>

* wip

* collect: implement for local:exec and local:docker.

* cmd/run: print run ID on client side.

* use /outputs value

* remove bucket name from archive

* refactor CollectOutputs

* fix network plan

* remove return code. extract downloader

* local:docker: return runid

Co-authored-by: Henrique Dias <hacdias@gmail.com>
Co-authored-by: Raúl Kripalani <raul.kripalani@gmail.com>
return err
}

header, err := zip.FileInfoHeader(info)

This comment has been minimized.

Copy link
@nonsense

nonsense Feb 5, 2020

Member

I think this part can be simplified - just call zip.Create which does the FileHeader for you and returns a writer.

But if this works, it works :)

**/.gitignore
**/.gitkeep
**/.gitmodules
**/Dockerfile

This comment has been minimized.

Copy link
@nonsense

nonsense Feb 5, 2020

Member

Dockerfile in .dockerignore?

return fmt.Errorf("Couldn't add file to the zip archive: %v", err)
}

_, err = downloader.Download(FakeWriterAt{ww},

This comment has been minimized.

Copy link
@nonsense

nonsense Feb 5, 2020

Member

@raulk I wanted to add parallel downloads here, but it is hard, because the downloader.Download expects a io.WriterAt, and we are sequentially writing to the zip writer. So I am not exactly sure how we can have parallel downloads with parallel writers to the same zip archive - something to investigate if we can do better/faster.

@@ -68,7 +71,7 @@ func (r *LocalExecutableRunner) Run(ctx context.Context, input *api.RunInput, ow
logging.S().Info("temporary redis instance started successfully")
} else {
close(redisWaitCh)
r.redisLk.Unlock()
r.setupLk.Unlock()

This comment has been minimized.

Copy link
@nonsense

nonsense Feb 5, 2020

Member

nitpick: When you have multiple places that can err and have to add Unlock at multiple places, I find it easier to just create a closure with defer - this way if someone adds another statement later it is hard to miss adding the Unlock.

TestGroupInstanceCount: toInt(m[EnvTestGroupInstanceCount]),
TestOutputsPath: m[EnvTestOutputsPath],

structured: make(chan *zap.Logger, 32),

This comment has been minimized.

Copy link
@nonsense

nonsense Feb 5, 2020

Member

What this 32 about?

@nonsense nonsense merged commit fd36888 into master Feb 5, 2020
1 of 2 checks passed
1 of 2 checks passed
Travis CI - Branch Build Failed
Details
Travis CI - Pull Request Build Passed
Details
@nonsense nonsense deleted the feat/test-outputs branch Feb 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.