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.

Sign up for GitHub

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

Jump to bottom

Unify all logfile handling into utils_logfile #3782

Merged
merged 3 commits into from
Dec 11, 2023
Merged

Unify all logfile handling into utils_logfile #3782

merged 3 commits into from
Dec 11, 2023

Conversation

pevogam
Copy link
Contributor

@pevogam pevogam commented Oct 16, 2023

Most importantly, for better decoupling with aexpect we provide sessions with close hooks that should have the same effect of closing the open log file descriptors but in a more decoupled fashion.

The logging functionality in utils_misc has been fully deprecated for the sake of the same functionality in a fully devoted module.

pevogam referenced this pull request in avocado-framework/aexpect Oct 16, 2023
@ldoktor
Fix pylint issues in "genio"/"log_file" and warn about it's issues
2d31545 
The "genio"/"log_file" is quite dangerous and requires using private
members of "genio" module. Unfortunatelly "Avocado-vt" heavily depends
on this so let's just fix style issues and add docstrings explaining the
issues.

Signed-off-by: Lukáš Doktor <[email protected]>
@pevogam
Unify all logfile handling into utils_logfile
f1c09e7 
Most importantly, for better decoupling with aexpect we provide
sessions with close hooks that should have the same effect of
closing the open log file descriptors but in a more decoupled
fashion.

The logging functionality in utils_misc has been fully deprecated
for the sake of the same functionality in a fully devoted module.

Signed-off-by: Plamen Dimitrov <[email protected]>
@pevogam pevogam force-pushed the logfile-unification branch from 58b7035 to f1c09e7 Compare October 16, 2023 20:12
ldoktor
ldoktor approved these changes Oct 19, 2023
View reviewed changes
Copy link
Contributor

@ldoktor ldoktor left a comment

Choose a reason for hiding this comment

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

LGTM, seems to cover all utils_misc.*log occurrences while keeping the deprecated interface for tp-* and other consumers. As far as I understand this PR needs to be merged before avocado-framework/aexpect#124 in order for avocado-vt to work with the aexpect after the avocado-framework/aexpect#124 is merged, right (it will still work with the released aexpect, though)

@pevogam
Copy link
Contributor Author

pevogam commented Oct 22, 2023

LGTM, seems to cover all utils_misc.*log occurrences while keeping the deprecated interface for tp-* and other consumers. As far as I understand this PR needs to be merged before avocado-framework/aexpect#124 in order for avocado-vt to work with the aexpect after the avocado-framework/aexpect#124 is merged, right (it will still work with the released aexpect, though)

Exactly, this is also my understanding.

luckyh
luckyh reviewed Nov 3, 2023
View reviewed changes
Copy link
Contributor

@luckyh luckyh left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Hi @pevogam , thank you for bringing up this proposal! it indeed improves the code quality for both VT and aexpect and prevents some potential issues. However I have some comments regarding the following details.

virttest/libvirt_vm.py Outdated Show resolved Hide resolved
virttest/libvirt_vm.py Outdated Show resolved Hide resolved
virttest/utils_logfile.py Outdated Show resolved Hide resolved
virttest/utils_misc.py Outdated Show resolved Hide resolved
@pevogam
Drop usage of placeholder session log file attribute
68c0b39 
Since we are handling the complete log file setup and cleanup on
the Avocado VT side we can use the more powerful close hook approach
to provide the log file path via currying and a function wrapper.

Signed-off-by: Plamen Dimitrov <[email protected]>
@pevogam pevogam force-pushed the logfile-unification branch from ff2689b to 68c0b39 Compare November 25, 2023 08:59
@pevogam
Copy link
Contributor Author

pevogam commented Nov 25, 2023

I pushed a small fix for some cases of Failed to print_line that were caused by the last commit incorrectly removing additional output parameters of the output function in one case. All tested now also in integration with our own products in addition to this CI.

luckyh
luckyh reviewed Dec 5, 2023
View reviewed changes
virttest/utils_logfile.py Outdated Show resolved Hide resolved
@pevogam pevogam force-pushed the logfile-unification branch from 784f3e5 to 237a8c8 Compare December 7, 2023 13:38
@pevogam
Allow for both filtered and unfiltered log file closing
04a89c0 
The environment setup module could close all log files once all
sessions are closed but we have to keep backwards compatibility
with other uses cases of closing just a single or few log files.

Signed-off-by: Plamen Dimitrov <[email protected]>
@pevogam pevogam force-pushed the logfile-unification branch from 237a8c8 to 04a89c0 Compare December 7, 2023 13:45
luckyh
luckyh approved these changes Dec 8, 2023
View reviewed changes
Copy link
Contributor

@luckyh luckyh left a comment

Choose a reason for hiding this comment

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

LGTM

@pevogam
Copy link
Contributor Author

pevogam commented Dec 8, 2023

@ldoktor Does your LGTM ack remain valid? Or would you like to suggest other changes?

@ldoktor
Copy link
Contributor

ldoktor commented Dec 8, 2023

@ldoktor Does your LGTM ack remain valid? Or would you like to suggest other changes?

Yep, looks good. I mean using * to match all is slightly controversial as the file can be named * and people might expect the use of glob based on this argument so perhaps it'd be safer to use a non-string argument for such case (be it a named object, None or True). But not a strong opinion.

@pevogam
Copy link
Contributor Author

pevogam commented Dec 11, 2023

Yep, looks good. I mean using * to match all is slightly controversial as the file can be named *

I think using * will be a fairly exotic action when it comes to Avocado VT log file naming but a good observation nevertheless. A previous push of this pull request had None in the usage but indeed perhaps all of this is not too important since it is only meant to be used for Avocado VT log file management to begin with.

@luckyh I have integration tested the recent changes fully (with the corresponding aexpect side adapted too) and everything checks out. As this pull request has two approvals we can either merge or ask another VT maintainer to review this pull request too if you like.

@luckyh
Copy link
Contributor

luckyh commented Dec 11, 2023

ok, let's merge this, thanks to all!

@luckyh luckyh merged commit 24b13b1 into avocado-framework:master Dec 11, 2023
49 checks passed
@pevogam pevogam deleted the logfile-unification branch December 11, 2023 06:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PaulYuuu PaulYuuu PaulYuuu left review comments

@ldoktor ldoktor ldoktor approved these changes

@luckyh luckyh luckyh approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pevogam @ldoktor @luckyh @PaulYuuu