Skip to content

Collect logs from integration test runs#675

Merged
blackboxsw merged 2 commits into
canonical:masterfrom
TheRealFalcon:test-logs
Nov 24, 2020
Merged

Collect logs from integration test runs#675
blackboxsw merged 2 commits into
canonical:masterfrom
TheRealFalcon:test-logs

Conversation

@TheRealFalcon
Copy link
Copy Markdown
Contributor

@TheRealFalcon TheRealFalcon commented Nov 18, 2020

Proposed Commit Message

Collect logs from integration test runs

During teardown of every cloud instance, run 'cloud-init collect-logs',
then transfer and unpack locally. Two new integration settings have
been added to specify when to perform this action (ALWAYS,
ON_ERROR, NEVER), and where to store these logs.

Additional Context

Note that it runs once per client, so if you're using the class client, you'll get one set of the logs for the entire class. Currently logs take about 5M of space locally. The entire tree of files created after a single test run can be seen here: https://paste.ubuntu.com/p/v4rF9m9tPF/ . It includes a couple test_example runs that don't exist in the repo because I was testing parameterized tests.

Test Steps

Without overriding COLLECT_LOGS or LOCAL_LOG_PATH in the integration test settings, run:
pytest -s tests/integration_tests
A timestamped directory should get created at /tmp/cloud_init_test_logs containing all of the logging information collected from each test instance run during tests.

Checklist:

  • My code follows the process laid out in the documentation
  • I have updated or added any unit tests accordingly
  • I have updated or added any documentation accordingly

@TheRealFalcon TheRealFalcon self-assigned this Nov 18, 2020
@paride
Copy link
Copy Markdown
Contributor

paride commented Nov 18, 2020

@TheRealFalcon one flake8 failure:

tests/integration_tests/conftest.py:157:1: E303 too many blank lines (3)

Copy link
Copy Markdown
Contributor

@paride paride left a comment

Choose a reason for hiding this comment

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

LGTM, but will require a +1 from a committer.



def _collect_logs(instance, node_id):
instance.execute(
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Future me wants a docstring describing params, specifically I'd like an example of what node_id value is generally expected to be so I don't have to look it up from the PR subtext.

Comment thread tests/integration_tests/conftest.py Outdated
user_data=user_data, launch_kwargs=launch_kwargs
) as instance:
yield instance
if integration_settings.COLLECT_LOGS:
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Since this is a fairly expensive operation, do we want to really do this on every instance, or gate it behind either a test failure or a force value? This request kind of asks whether the COLLECT_LOGS value should be an enum instead of bool. values could by ALWAYS|ON_ERROR|NEVER?

what do folks think?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think the thinking was that for the SRU logs we'd need to collect the logs from most/all of the tests anyway. I could default it to False too. But having an ON_ERROR option seems like a good idea too. I'll take a look at adding that.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks a lot for this @TheRealFalcon +1.

@paride
Copy link
Copy Markdown
Contributor

paride commented Nov 24, 2020

There a merge conflict to be resolved

Copy link
Copy Markdown
Collaborator

@blackboxsw blackboxsw left a comment

Choose a reason for hiding this comment

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

+1 Thanks for this, please update your suggested squash commit message to reference the something about how collect_logs is optionally collected ON_ERROR.

During teardown of every cloud instance, run 'cloud-init collect-logs',
then transfer and unpack locally. Two new integration settings have
been added to specify whether to perform this action, and where
to store these logs.
@TheRealFalcon
Copy link
Copy Markdown
Contributor Author

@blackboxsw I updated the commit message and rebased the branch

@blackboxsw blackboxsw merged commit bc9c6c2 into canonical:master Nov 24, 2020
@TheRealFalcon TheRealFalcon deleted the test-logs branch November 24, 2020 21:56
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