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

Improve driver_updates logging #4149

Merged
merged 4 commits into from Oct 21, 2022
Merged

Improve driver_updates logging #4149

merged 4 commits into from Oct 21, 2022

Conversation

jkonecny12
Copy link
Member

Debugging of the driver_updates script could be a pain. To improve that we need much more logs in the whole file in a way that it won't bother users during boot. To solve that we are adding debug logs and showing them on the console only in case that inst.debug or rd.debug is used.

@jkonecny12 jkonecny12 added the f37 Fedora 37 label May 27, 2022
@pep8speaks
Copy link

pep8speaks commented May 27, 2022 •
edited

Hello @jkonecny12! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 619:39: E127 continuation line over-indented for visual indent

Line 34:1: E402 module level import not at top of file
Line 159:100: E501 line too long (113 > 99 characters)

Comment last updated at 2022-08-17 11:37:29 UTC

@jkonecny12
Copy link
Member Author

@jstodola what do you think, any ideas for improvements?

@jkonecny12
Copy link
Member Author

/kickstart-test --testtype smoke

@jkonecny12 jkonecny12 force-pushed the master-improve-driver-updates-logging branch from f174727 to 884a4d2 Compare May 27, 2022 17:54
rvykydal
rvykydal approved these changes May 30, 2022
View reviewed changes
Copy link
Contributor

@rvykydal rvykydal left a comment

Choose a reason for hiding this comment

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

Looks good to me, thank you.

VladimirSlavik
VladimirSlavik reviewed May 30, 2022
View reviewed changes
Copy link
Contributor

@VladimirSlavik VladimirSlavik left a comment •
edited

Choose a reason for hiding this comment

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

Thank you! I have a few suggestions for the logging strings, but otherwise looks good.

dracut/driver_updates.py Outdated Show resolved Hide resolved
dracut/driver_updates.py Outdated Show resolved Hide resolved
dracut/driver_updates.py Outdated Show resolved Hide resolved
dracut/driver_updates.py Outdated Show resolved Hide resolved
@jstodola
Copy link
Contributor

jstodola commented Jun 1, 2022

@jkonecny12, I'm on my phone only for a while, so it's quite challenging to suggest any improvements. So, no useful feedback from my side :-)

@jkonecny12
Copy link
Member Author

This change has time. I know that I will wait for you to have time, no problem with that.

@jkonecny12 jkonecny12 force-pushed the master-improve-driver-updates-logging branch 2 times, most recently from 574f49a to 01b784b Compare June 1, 2022 14:29
@jkonecny12
Copy link
Member Author

UPDATED:
*Apply notes from @VladimirSlavik - thanks!
*Add new commit to move the file into the pep8.

poncovka
poncovka approved these changes Jun 1, 2022
View reviewed changes
dracut/driver_updates.py
subprocess.call(["depmod", "-a"])

log.debug("load_drivers: %s", list(moddict.keys()))
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like a leftover.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not really left-over. The other print statements are under if so they could be missed. It's for robustness.

@jkonecny12
Copy link
Member Author

/kickstart-test --testtype smoke

@jkonecny12 jkonecny12 added the manual testing required This issue can't be merged without manual testing label Jun 1, 2022
@github-actions
Copy link

github-actions bot commented Aug 1, 2022

This PR is stale because it has been open 60 days with no activity.
Remove stale label or comment or this will be closed in 30 days.

@github-actions github-actions bot added the stale label Aug 1, 2022
VladimirSlavik
VladimirSlavik approved these changes Aug 1, 2022
View reviewed changes
Copy link
Contributor

@VladimirSlavik VladimirSlavik left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thank you!

@github-actions github-actions bot removed the stale label Aug 2, 2022
@jkonecny12 jkonecny12 force-pushed the master-improve-driver-updates-logging branch from 01b784b to 803f698 Compare August 10, 2022 13:39
@jkonecny12
Copy link
Member Author

UPDATED:
Seems that my previous implementation did not worked as I expected. There were duplicate messages for DEBUG and INFO to the console.

So I changed the behavior a bit to always have a prefix DD (<LEVEL>). It could feel a kind a weird so I'm thinking if I shouldn't remove that to when logging into console. What do you think?

anaconda-dd-debugging

@VladimirSlavik
Copy link
Contributor

Do you mean removing only the prefixes in console? That sounds good.

@jkonecny12
Copy link
Member Author

Yes, I can leave the prefixes in the syslog but can remove them in the console. It will be problem to distinguish between debug and info message but that shouldn't be problem thought.

@jkonecny12
Copy link
Member Author

@jstodola any opinion about this?

@jkonecny12 jkonecny12 added f38 Fedora 38 and removed f37 Fedora 37 labels Aug 16, 2022
@jkonecny12
Copy link
Member Author

Switching to Fedora 38

@jkonecny12 jkonecny12 force-pushed the master-improve-driver-updates-logging branch from 803f698 to 583efad Compare August 16, 2022 12:54
@jkonecny12
Copy link
Member Author

Updated the behavior.

Remove prefixes from the console prints when debug is on:
driver_updates_debug_without_prefix
mounting and unmounting is the debug output

Leave prefixes in the syslog:
driver_updates_no_debug_journal

@jkonecny12
Copy link
Member Author

/kickstart-test --testtype smoke

@jkonecny12 jkonecny12 force-pushed the master-improve-driver-updates-logging branch from 583efad to c67c20c Compare August 16, 2022 13:25
@jkonecny12
Copy link
Member Author

/kickstart-test --testtype smoke

@jkonecny12 jkonecny12 force-pushed the master-improve-driver-updates-logging branch from c67c20c to 9207f2a Compare August 17, 2022 11:37
@jkonecny12
Copy link
Member Author

UPDATED:

  • fix tests

@jkonecny12
Copy link
Member Author

/kickstart-test --testtype smoke

@jkonecny12
driver_updates: extend where we print log messages
493e686 
- Log INFO and DEBUG messages to syslog always.
- Log INFO messages to console always.
- Log DEBUG messages to console only if 'inst.debug' or 'rd.debug' is on the
- kernel command line.
@jkonecny12
driver_updates: add lot of debug logs for easier debugging
68e72fe 
Without these it could be real pain to solve bugs here. Most of the time special
HW is required to be able to reproduce this and even with the HW there is not
100% sure that we are able to reproduce the issue.

These logs will always go to journal log and with 'inst.debug' or 'rd.debug' on
command line they will show up also in the console during boot. That is great
help in case the code will freeze and boot process blocked.

Also fix test scanning files for mount keyword. It is now used in the debug logs.
@jkonecny12
driver_updates: adding tests for the new logging solution
2f1e3ee 
Do not test logs but where the logs are printed and if the debug mode detection
is working correctly.
@jkonecny12
driver_updates: migrate driver_updates.py to pep8 format
2ac0e95 
There were plenty of issues in readability so let's move this file into pep8
style to solve these and make the script consistent with the rest of the
project.
@jkonecny12 jkonecny12 force-pushed the master-improve-driver-updates-logging branch from 9207f2a to 2ac0e95 Compare October 7, 2022 14:43
@jkonecny12 jkonecny12 removed the manual testing required This issue can't be merged without manual testing label Oct 7, 2022
@jkonecny12
Copy link
Member Author

jkonecny12 commented Oct 7, 2022 •
edited

UPDATED:

  • Fix an issue when inst.debug was last on the command line (the \n broke detection)
  • Add a new test for this

Tested and it seems that everything works as it should now.

@jkonecny12
Copy link
Member Author

/kickstart-test --testtype smoke

@jkonecny12
Copy link
Member Author

/packit build

@jkonecny12 jkonecny12 merged commit e8ec4a5 into rhinstaller:master Oct 21, 2022
This was referenced Apr 28, 2023
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rvykydal rvykydal rvykydal approved these changes

@poncovka poncovka poncovka approved these changes

@VladimirSlavik VladimirSlavik VladimirSlavik approved these changes

Assignees
No one assigned
Labels
f38 Fedora 38
Milestone
No milestone
6 participants
@jkonecny12 @pep8speaks @jstodola @VladimirSlavik @rvykydal @poncovka