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

[collect] Fix exception while parsing sos help #3830

Merged

Conversation

jcastill
Copy link
Member

@jcastill jcastill commented Nov 1, 2024

While parsing the output of 'sos report -l', we
were attempting to split an empty line, and
getting the following exception:

[:_regex_sos_help] Error parsing sos help: list index out of range

Related: #3827


Please place an 'X' inside each '[]' to confirm you adhere to our Contributor Guidelines

  • Is the commit message split over multiple lines and hard-wrapped at 72 characters?
  • Is the subject and message clear and concise?
  • Does the subject start with [plugin_name] if submitting a plugin patch or a [section_name] if part of the core sosreport code?
  • Does the commit contain a Signed-off-by: First Lastname [email protected]?
  • Are any related Issues or existing PRs properly referenced via a Closes (Issue) or Resolved (PR) line?
  • Are all passwords or private data gathered by this PR obfuscated?

Copy link

Congratulations! One of the builds has completed. 🍾

You can install the built RPMs by following these steps:

  • sudo yum install -y dnf-plugins-core on RHEL 8
  • sudo dnf install -y dnf-plugins-core on Fedora
  • dnf copr enable packit/sosreport-sos-3830
  • And now you can install the packages.

Please note that the RPMs should be used only in a testing environment.

@@ -367,7 +367,8 @@ def _regex_sos_help(self, regex, sosinfo, is_list=False):
for line in result.splitlines():
if not is_list:
try:
res.append(line.split()[0])
if line.split():
Copy link
Member Author

Choose a reason for hiding this comment

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

We could do something else instead of calling split() twice, let me know if you have any other preference.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I am on torns here. Comparing performance on big text files:

if line.split():
    print(line.split()[0])

took:

real	0m5.488s
user	0m5.090s
sys	0m0.324s

while:

ls = line.split()
if ls:
    print(ls[0])

took:

real	0m4.161s
user	0m3.841s
sys	0m0.252s

Copy link
Member Author

Choose a reason for hiding this comment

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

Then lets go for the second option

@jcastill jcastill force-pushed the jcastillo-fix-exception-sosnode branch from 1a1252d to 95b7854 Compare November 1, 2024 12:00
@arif-ali
Copy link
Member

arif-ali commented Nov 1, 2024

restarting the failed job

@@ -367,7 +367,9 @@ def _regex_sos_help(self, regex, sosinfo, is_list=False):
for line in result.splitlines():
if not is_list:
try:
res.append(line.split()[0])
ls = line.split()
if ls:
Copy link
Member

Choose a reason for hiding this comment

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

If we're concerned about empty lines, and we're concerned about performance given the earlier conversation, then we should be applying the conditional before the .split().

for line in results.splitlines():
    if not is_list:
        if not line:
            continue
    [...]

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried this, and it covers all the empty lines, but there's a case where we print a single space and the exception gets printed. Playing with the output, this is what we get:

'a python.hashes             off             collect hashes for all python files'
'a rpm.rpmq                  on              query package information with rpm -q'
'a rpm.rpmva                 off             verify all packages'
'a rpm.rpmdb                 off             collect /var/lib/rpm'
'a selinux.fixfiles          off             collect incorrect file context labels'
'a services.servicestatus    off             collect status of all running services'
'a ssh.userconfs             on              Changes whether module will collect user .ssh configs'
''
' '
[localhost:_regex_sos_help] Error parsing sos help: list index out of range

That space is coming from here:

https://github.com/sosreport/sos/blob/main/sos/report/__init__.py#L1093

Any simple way to cover this case as well, without having to change the code in report?
A walrus operator here would be more useful and cover all cases, but we are still waiting for a decision on #3533 , right?

Copy link
Member

@arif-ali arif-ali Nov 3, 2024

Choose a reason for hiding this comment

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

@jcastill walrus is good, we already discussed this in #3811

Copy link
Member Author

Choose a reason for hiding this comment

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

Perfect, I'll push a new version soon

While parsing the output of 'sos report -l', we
were attempting to split an empty line, and
getting the following exception:

[<ip address>:_regex_sos_help] Error parsing sos help: list index out of range

Related: sosreport#3827

Signed-off-by: Jose Castillo <[email protected]>
@jcastill jcastill force-pushed the jcastillo-fix-exception-sosnode branch from 95b7854 to e598816 Compare November 3, 2024 20:40
@TurboTurtle TurboTurtle merged commit 44fae16 into sosreport:main Nov 7, 2024
33 checks passed
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.

4 participants