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

[utilities] catch SoSTimeoutError in sos_get_command_output #3331

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

Conversation

pmoravec
Copy link
Contributor

@pmoravec pmoravec commented Aug 9, 2023

When calling collect_cmd_output and timeout is hit, SoSTimeoutError is raised and never caught (until in plugin wrapper).

Resolves: #3331


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?

@pmoravec
Copy link
Contributor Author

pmoravec commented Aug 9, 2023

Reproducer: on a system without virsh, run:

echo "sleep 10" > ~/bin/virsh
chmod a+x ~/bin/virsh
sos report -o virsh --cmd-timeout=3 --batch --build

which reports:

caught exception in plugin method "virsh.setup()"
writing traceback to sos_logs/virsh-plugin-errors.txt

with content:

Traceback (most recent call last):
  File "/root/sos-main/sos/report/__init__.py", line 1232, in setup
    plug.setup()
  File "/root/sos-main/sos/report/plugins/virsh.py", line 54, in setup
    foreground=True)
  File "/root/sos-main/sos/report/plugins/__init__.py", line 2534, in collect_cmd_output
    subdir=subdir, tags=tags
  File "/root/sos-main/sos/report/plugins/__init__.py", line 2370, in _collect_cmd_output
    to_file=out_file
  File "/root/sos-main/sos/utilities.py", line 267, in sos_get_command_output
    raise e
  File "/root/sos-main/sos/utilities.py", line 236, in sos_get_command_output
    _check_poller(p)
  File "/root/sos-main/sos/utilities.py", line 186, in _check_poller
    raise SoSTimeoutError
sos.utilities.SoSTimeoutError

The exception is raised directly in sos_get_command_output in _check_poller sub-routine.

@packit-as-a-service
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-3331
  • And now you can install the packages.

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

@pmoravec
Copy link
Contributor Author

The patch should rather be like:

@@ -232,8 +232,16 @@ def sos_get_command_output(command, time
             reader = FakeReader(p, binary)
 
         if poller:
-            while reader.running:
-                _check_poller(p)
+            try:
+                while reader.running:
+                    _check_poller(p)
+            except SoSTimeoutError:
+                p.terminate()
+                if to_file:
+                    _output.close()
+                reader.running = False
+                return {'status': 124, 'output': reader.get_contents(),
+                        'truncated': reader.is_full}
         else:
             try:
                 # override timeout=0 to timeout=None, as Popen will treat the

but even then, sos gets stuck forever before shutdown, with many threads pending on sos/utilities.py, line 484 (read from pipe of stuck virsh command - I can reproduce this with a real frozing virsh command only, not by mocked sleep) . We might add a poller to the read from pipe, but.. isn't it generally fragile and overkill?

When calling collect_cmd_output and timeout is hit, SoSTimeoutError is
raised and never caught (until in plugin wrapper).

Resolves: sosreport#3331

Signed-off-by: Pavel Moravec <[email protected]>
@pmoravec pmoravec force-pushed the sos-pmoravec-catch-SoSTimeoutError-in-sos_get_command_output branch from c194ab4 to df49224 Compare August 11, 2023 13:45
@arif-ali
Copy link
Member

@pmoravec still need this to go through?

@pmoravec
Copy link
Contributor Author

Well the reproducer is still valid on current upstream and I still dont have an idea of a proper fix /o.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants