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

roachtest: zip the artifacts for each test on teamcity #65831

Merged
merged 2 commits into from
May 31, 2021

Conversation

tbg
Copy link
Member

@tbg tbg commented May 28, 2021

  • roachtest: don't duplicate log files
  • roachtest: zip the artifacts for each test on teamcity

@tbg tbg requested review from a team and stevendanna and removed request for a team May 28, 2021 10:36
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@stevendanna
Copy link
Collaborator

Nice:

Screenshot 2021-05-28 at 13 06 42

Copy link
Collaborator

@stevendanna stevendanna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r1, 1 of 1 files at r2, 1 of 1 files at r3.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @tbg)

a discussion (no related file):
I think this will be a nice addition. Approved (although don't forget to remove the last commit :D). I left some comments for your consideration, but nothing blocking.



pkg/cmd/roachtest/test_runner.go, line 1274 at r2 (raw file):

	defer f.Close()
	z := zip.NewWriter(f)
	rel := func(path2 string) string {

filepath.Rel calls this argument targetpath if you wanted to avoid path2


pkg/cmd/roachtest/test_runner.go, line 1303 at r2 (raw file):

			return err
		}
		return os.Remove(path)

I wonder if instead of removing these files immediately, we should collect the files to delete and then delete them after we are sure we've written the zip archive? The downside is that we'll need more available disk-space, but it might prevent us from losing logs if there is some problem writing the zip file.

tbg added 2 commits May 31, 2021 10:43
Since we want to have the logs for down nodes as well, roachtest copies
the log directories to artifacts. It is therefore not necessary to
include the logs in the `debug.zip` in addition to that.

Release note: None
Often you want to download the artifacts for a specific test,
but TeamCity doesn't let you do that. Zip the artifacts for
each test as a workaround.

TeamCity is good at navigating into zip files, so we don't lose
anything in terms of poking at a test directly from TeamCity.

cc @cockroachdb/test-eng

Release note: None
@tbg tbg force-pushed the roachtest-zip branch from f72216d to 7ad39e6 Compare May 31, 2021 09:08
@tbg tbg requested a review from stevendanna May 31, 2021 09:17
Copy link
Member Author

@tbg tbg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @stevendanna)

a discussion (no related file):

Previously, stevendanna (Steven Danna) wrote…

I think this will be nice addition. Approved (although don't forget to remove the last commit :D). I left some comments for your consideration, but nothing blocking.

TFTR!

bors r=stevendanna



pkg/cmd/roachtest/test_runner.go, line 1274 at r2 (raw file):

Previously, stevendanna (Steven Danna) wrote…

filepath.Rel calls this argument targetpath if you wanted to avoid path2

Done.


pkg/cmd/roachtest/test_runner.go, line 1303 at r2 (raw file):

Previously, stevendanna (Steven Danna) wrote…

I wonder if instead of removing these files immediately, we should collect the files to delete and then delete them after we are sure we've written the zip archive? The downside is that we'll need more available disk-space, but it might prevent us from losing logs if there is some problem writing the zip file.

Done.

@craig
Copy link
Contributor

craig bot commented May 31, 2021

Build succeeded:

@craig craig bot merged commit a3811d2 into cockroachdb:master May 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants