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

Capture logs for all containers used in tests #1869

Merged
merged 9 commits into from
Oct 1, 2024

Conversation

Molter73
Copy link
Collaborator

@Molter73 Molter73 commented Sep 27, 2024

Description

Some flakes we have in CI may be due to errors on the auxiliar containers we run alongside collector. In order to make it easier to debug this type of flakes, container logs could be used to get more information on what is going on.

Checklist

  • Investigated and inspected CI test results
  • Updated documentation accordingly

Automated testing

  • Added unit tests
  • Added integration tests
  • Added regression tests

If any of these don't apply, please comment below.

Testing Performed

CI should be enough.

  • Check log artifacts don't become huge

@Molter73 Molter73 marked this pull request as ready for review September 27, 2024 11:36
@Molter73 Molter73 requested a review from a team as a code owner September 27, 2024 11:36
@Molter73
Copy link
Collaborator Author

I checked manually and the artifacts created in the integration tests now hold the logs for the containers that run alongside collector.

@Stringy
Copy link
Collaborator

Stringy commented Sep 27, 2024

For a slight quality of life improvement, can we change the log files to be in a directory per test (or per suite?) rather than prefixed with the test name? There's more and more files now, and it'd be nice to separate them to make it easier to go through them

@Molter73
Copy link
Collaborator Author

For a slight quality of life improvement, can we change the log files to be in a directory per test (or per suite?) rather than prefixed with the test name? There's more and more files now, and it'd be nice to separate them to make it easier to go through them

Makes sense, I'll get it done.

@Molter73
Copy link
Collaborator Author

Because of how the ansible fetch module works, either flattening files directly onto the destination, overwriting files with the same name, or keeping a long path that includes the hostname and the full path to the files, I decided to use archives and fetch those to ensure no collisions happen. Now when you download the zip archive from GHA and unzip it you get the following structure:

.
├── container-logs
│   ├── cos_cos-beta-core_bpf.tar.gz
│   ├── cos_cos-dev-core_bpf.tar.gz
│   ├── cos_cos-stable-core_bpf.tar.gz
│   ├── integration-test.log
│   └── integration-test-report.xml
└── cos-logs.zip

If you tar xzf one of those archives you get:

.
├── container-logs
│   ├── cos_cos-beta
│   │   └── core_bpf
│   │       ├── perf.json
│   │       ├── TestAsyncConnectionBlocked
│   │       │   ├── client.log
│   │       │   ├── collector.log
│   │       │   ├── container-stats.log
│   │       │   └── events.log
│   │       ├── TestAsyncConnectionBlockedWithDisableTracking
│   │       │   ├── client.log
│   │       │   ├── collector.log
│   │       │   ├── container-stats.log
│   │       │   └── events.log
│   │       ├── TestAsyncConnectionSuccess
│   │       │   ├── client-stderr.log
│   │       │   ├── client-stdout.log
│   │       │   ├── collector.log
│   │       │   ├── container-stats.log
│   │       │   ├── events.log
│   │       │   └── server.log
...

It's not ideal, but I think it's good enough for now.

Some flakes we have in CI may be due to errors on the auxiliar
containers we run alongside collector. In order to make it easier to
debug this type of flakes, container logs could be used to get more
information on what is going on.
This is due to the ansible fetch module either flattening files directly
onto the destination directory, overwritten files with the same name, or
keeping a long path that includes the hostname and the full path to the
files. Best workaround I could come up with is to generate uniquely
named archives that hold all logs with the proper paths and files, then
move those with the flat option.
@Molter73 Molter73 force-pushed the mauro/capture-all-logs branch from 0748254 to a5966ce Compare September 30, 2024 15:40
return sbStdErr.String(), nil
defer logFile.Close()

_, err = logFile.WriteString(log.content)
Copy link
Collaborator

Choose a reason for hiding this comment

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

this error is currently ignored - if intentional, we should just use _

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is this a rustc reference?

Copy link
Collaborator

Choose a reason for hiding this comment

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

it isn't 😅, not intentionally anyway

Co-authored-by: Giles Hutton <[email protected]>
Copy link
Collaborator

@Stringy Stringy left a comment

Choose a reason for hiding this comment

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

LGTM!

@Molter73 Molter73 merged commit 854b357 into master Oct 1, 2024
67 of 68 checks passed
@Molter73 Molter73 deleted the mauro/capture-all-logs branch October 1, 2024 12:42
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.

2 participants