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

Uploading docker logs on test failures #3236

Merged
merged 16 commits into from
Mar 6, 2023

Conversation

chatton
Copy link
Contributor

@chatton chatton commented Mar 3, 2023

Description

closes: #2666

This PR adds a cleanup hook to our E2E which fetches the container logs of all validator nodes and the relayer(s) as well as some key files which should aid debugging.

These files will be uploaded to the summary view of the github workflow.

When running these tests locally, these files will end up under e2e/diagnostics.

There's a temporary FailNow in one of the tests to verify the log collection. Not anymore!

Commit Message / Changelog Entry

N/A

see the guidelines for commit messages. (view raw markdown for examples)


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (see CONTRIBUTING.md).
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards and Go style guide.
  • Wrote unit and integration tests.
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/).
  • Added relevant godoc comments.
  • Provide a commit message to be used for the changelog entry in the PR description for review.
  • Re-reviewed Files changed in the Github PR explorer.
  • Review Codecov Report in the comment section below once CI passes.

@chatton chatton marked this pull request as ready for review March 6, 2023 09:46
Comment on lines +62 to +65
hdr, err := tr.Next()
if err != nil {
return nil, err
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this function is assuming as single file exists with the name provided, if this is no longer a valid assumption to make we can make this function more flexible.

Comment on lines +76 to +77
diagnosticFiles := chainDiagnosticAbsoluteFilePaths(*cfg.ChainAConfig)
diagnosticFiles = append(diagnosticFiles, chainDiagnosticAbsoluteFilePaths(*cfg.ChainBConfig)...)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

note: this will mean that we are basically trying to get the chain-a files from chain-b and chain-b files from chain-a. They will just be skipped. I chose to make it simpler and just try them all for every container instead of finding only the correct files on a per container basis which would make things a little more complex. I think this doesn't really matter, as this is simply a tear-down function anyway.

- name: Upload Diagnostics
uses: actions/upload-artifact@v3
if: ${{ failure() && inputs.upload-logs }}
continue-on-error: true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this means uploading logs failing will not cause a test failure.

with:
name: "${{ matrix.entrypoint }}-${{ matrix.test }}"
path: e2e/diagnostics
retention-days: 5
Copy link
Contributor Author

Choose a reason for hiding this comment

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

arbitrary value, can probably increase this without any problems.

Copy link
Member

@damiannolan damiannolan left a comment

Choose a reason for hiding this comment

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

Awesome work, should be a big improvement for debugging!

Copy link
Contributor

@colin-axner colin-axner left a comment

Choose a reason for hiding this comment

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

Yay! Great work, code was really easy to follow

e2e/testsuite/testsuite.go Show resolved Hide resolved
e2e/testsuite/testsuite.go Show resolved Hide resolved
Comment on lines +146 to +148
for ; !strings.HasSuffix(wd, e2eDir) || count > maxAttempts; wd = ospath.Dir(wd) {
count++
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit confused by this. Is it possible to run these e2e's when your wd isn't the e2e directory?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is not strictly necessary, I just wanted to make sure that the test errors out if for some reason this case is ever true instead of hanging forever. It's probably safe to remove this entirely but I just went the extra careful route

@colin-axner
Copy link
Contributor

It looks like the logs are actually being split. The logs uploaded include everything except the tail which interchain test is writing to stdout (outputted in our CI results). This is not intentional?

@chatton chatton merged commit b4503f4 into main Mar 6, 2023
@chatton chatton deleted the cian/issue#2666-upload-e2e-logs-upon-failure branch March 6, 2023 16:58
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.

Upload E2E logs upon failure
3 participants