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

testcloud: save console logs as a file #3511

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

coiby
Copy link

@coiby coiby commented Feb 6, 2025

Resolves: https://issues.redhat.com/browse/KEX-189

Partially fixes: #2624

Console logs are crucial are debugging tests involving a kernel panic/crash as it's the only way to know what happens after a kernel panic/crash. tmt will make use testcloud's new feature [1] to save console logs to {plan.workdir}/{guest_name}_console.log.

[1] teemtee/testcloud#7

Pull Request Checklist

  • implement the feature

@coiby coiby requested a review from frantisekz as a code owner February 6, 2025 08:22
@@ -939,6 +939,8 @@ def start(self) -> None:
# Prepare DomainConfiguration object before Instance object
self._domain = DomainConfiguration(self.instance_name)

self._domain.console_log_file = f"{self.parent.plan.workdir}/{self.name}_console.log"
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. self.safe_name should be used instead of self.name, name may contain weird characters while safe_name is suitable for paths.
  2. I'd say self.workdir would be more fitting destination. The path alone does not matter that much, but self.parent.plan.workdir would put the file into the plan workdir, and you need to distinguish between guests by adding name to the filename, while every guest should already have its own workdir, nicely isolating files between guests.
  3. IIUIC, testcloud should accept Path instance as well as strings, therefore Path should be preferred.

All together, would self._domain.console_log_file = self.workdir / 'console.log' still work for your use case?

What testcloud version is required? Would tmt with older-than-needed testcloud package crash when trying to assign to console_log_file?

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for quickly reviewing the patch!

  1. self.safe_name should be used instead of self.name, name may contain weird characters while safe_name is suitable for paths.

Good suggestion!

  1. I'd say self.workdir would be more fitting destination. The path alone does not matter that much, but self.parent.plan.workdir would put the file into the plan workdir, and you need to distinguish between guests by adding name to the filename, while every guest should already have its own workdir, nicely isolating files between guests.
  1. IIUIC, testcloud should accept Path instance as well as strings, therefore Path should be preferred.

All together, would self._domain.console_log_file = self.workdir / 'console.log' still work for your use case?

Thanks for proposing a new idea and listing the pros of using provision.workdir. On the other hand, the pros of using plan.workdir are 1) there is no need for code to preserve the console log as provision.workdir will be deleted by default 2) I feel more natural to find logs in the root folder of a test plan. I've worked out the code for using provision.workdir in b2fe0b5. Anyways self._domain.console_log_file = self.workdir / 'console.log' will work for my use case. I'll leave you to make the choice as you are way more familiar with tmt than me:) Btw, somehow self._guest._domain.console_log_file is always empty for mutlihost testing, is it a bug?

What testcloud version is required? Would tmt with older-than-needed testcloud package crash when trying to assign to console_log_file?

tmt works with older testcloud because it simply adds a class member console_log_file to self._domain and older testcloud won't do anything about. I just tested this case and it works.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for proposing a new idea and listing the pros of using provision.workdir. On the other hand, the pros of using plan.workdir are 1) there is no need for code to preserve the console log as provision.workdir will be deleted by default 2) I feel more natural to find logs in the root folder of a test plan. I've worked out the code for using provision.workdir in b2fe0b5. Anyways self._domain.console_log_file = self.workdir / 'console.log' will work for my use case. I'll leave you to make the choice as you are way more familiar with tmt than me:)

tmt tries to isolate steps, plugins, guests, tests, and other objects from others of the same kind, therefore keeping logs of one guest together in the same directory will be more aligned with tmt behavior. Plus, there will be more logs, that is absolutely sure. Besides the console log, clouds add one or two (or many, like Beaker or kickstart in general) logs, therefore putting them all into the same directory would become impractical very fast.

Btw, somehow self._guest._domain.console_log_file is always empty for mutlihost testing, is it a bug?

No idea, could be, feel free to investigate. But from tmt's point of view, every plan is a multihost plan, the provisioning code works in the same way for one guest as well as for seven.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the clarification! I've push the commit using self._domain.console_log_file = self.workdir / 'console.log.

Copy link
Collaborator

Choose a reason for hiding this comment

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

LGTM. can you also add a test to let's say tests/provision/virtual.testcloud/test.sh? A simple provision & finish followed by checking the file exists and contains some interesting characters should do a lot for the confidence.

Copy link
Author

Choose a reason for hiding this comment

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

Sure, thanks for the suggestion! I've added a test.

Note I haven't been able to run the test locally,

# tmt plan ls plans/provision/virtual/provision
/plans/provision/virtual/provision

# tmt run -B finish plans --name /plans/provision/virtual/provision
/var/tmp/tmt/run-038

No plans found.

@happz happz added this to the 1.44 milestone Feb 10, 2025
Copy link
Collaborator

@skycastlelily skycastlelily left a comment

Choose a reason for hiding this comment

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

checked,works.Please extend the test as asked by Milos, thanks.

@happz happz mentioned this pull request Feb 17, 2025
8 tasks
Resolves: https://issues.redhat.com/browse/KEX-189

Partially fixes: teemtee#2624

Console logs are crucial are debugging tests involving a kernel
panic/crash as it's the only way to know what happens after a kernel
panic/crash. tmt will make use testcloud's new feature [1] to save
console logs to {plan.workdir}/provision/{guest_name}/console.log.

[1] teemtee/testcloud#7
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.

there are no guest logs accessiable to tmt users
3 participants