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

CP-48447+CA-390127: correct exception handling for func_output callbacks, fix saving all log messages #92

Conversation

bernhardkaindl
Copy link
Collaborator

@bernhardkaindl bernhardkaindl commented Mar 14, 2024

Fix two issues (their tests overlap):

CA-390127 (fix saving all log messages)

Fix adding even the last log() messages to the output archive

  • In the test added (extended) for CP-48447 (below) which extends test_main.py (which calls xen-bugtool.main()), the exception raised and logged as part of the test execution could not be found in the xen-bugtool.log file that is recorded in the bugtool output archive.
  • However, the exception was shown to be included in the xen-bugtool.log file itself (not in the output archive)
  • The reason for this behaviour was that xen-bugtool.log was collected before the exception was written to it:
  • Because there is no guarantee that this file is collected last, update it to be explicitly collected at the end of processing before generating the inventory.xml and closing the archive.
  • This was already approved by @GeraldEV in CA-390127: Fix adding even the last log() messages to the output archive #93, now added to this PR for merge.

CP-48447 (correct exception handling for func_output callbacks)

A major design goal of xen-bugtool is to continue as well as possible when an error occurs.
This is currently failing in a specific case when Python3 would be used to run xen-bugtool:

  • Given a raised Exception with these properties:
    • raised inside a callback function declared by func_output()
    • not caught inside the callback function itself
  • When logging that Exception,
    • the code misses encoding the backtrace of the exception from string to bytes when attempting to add it to the BytesIO-based file buffer to be included in the output archive.
      • The secondary exception caused is then fatal.

Traceback of an example (a real variant of caused me to notice it):

Initial exception (example):

Traceback (most recent call last):
  File "./xen-bugtool", line 724, in collect_data
    s = no_unicode(v["func"](cap))
  File "./xen-bugtool", line 1406, in dump_xapi_subprocess_info
    raise RuntimeError("error")
RuntimeError: error

This then results in a str (from the traceback) passed to io.BytesIO without encoding to bytes first, causing the 2nd, fatal Exception:

Traceback (most recent call last):
  File "./xen-bugtool", line 2399, in <module>
    sys.exit(main())
  File "./xen-bugtool", line 1316, in main
    collect_data(subdir, archive)
  File "./xen-bugtool", line 730, in collect_data
    v['output'] = StringIOmtime(s)
  File "./xen-bugtool", line 511, in __init__
    io.BytesIO.__init__(self, buf)
TypeError: a bytes-like object is required, not 'str'

That's an issue that is detected by pyright:

$ pyright xen-bugtool
xen-bugtool:730:45 - error: Argument of type "Unknown | str" cannot be assigned to parameter "buf" of type "bytes" in function "__init__"
    Type "Unknown | str" cannot be assigned to type "bytes"
      "str" is incompatible with "bytes" (reportArgumentType)

https://github.com/marketplace/actions/run-pyright-with-reviewdog
https://github.com/jordemort/action-pyright

Comment on lines -726 to +725
s = traceback.format_exc()
log(s)
backtrace = traceback.format_exc() # type: str
log(backtrace)
s = backtrace.encode()
Copy link
Collaborator Author

@bernhardkaindl bernhardkaindl Mar 19, 2024

Choose a reason for hiding this comment

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

This is the fix: It needs to convert the backtrace to bytes for the BytesIO that is used by tarfile and zipfile to include the data in the output archive.

The other changes are only the enhancements for tests/, to cover the change and to verify the logging of such exceptions (and fix the spelling dictionary for spelling checks in CI)

@bernhardkaindl bernhardkaindl changed the base branch from CP-48447/Py3-fix-abort-after-Exception-from-func_output to master March 21, 2024 02:39
GeraldEV
GeraldEV previously approved these changes Mar 21, 2024
Signed-off-by: Bernhard Kaindl <bernhard.kaindl@cloud.com>
Signed-off-by: Bernhard Kaindl <bernhard.kaindl@cloud.com>
@bernhardkaindl bernhardkaindl force-pushed the CP-48447/Py3-fix-abort-after-Exception-from-func_output branch from f6330e4 to 8187429 Compare March 21, 2024 17:10
Copy link

codecov bot commented Mar 21, 2024

Codecov Report

Merging #92 (60c73a5) into master (d0e569d) will increase coverage by 3%.
Report is 11 commits behind head on master.
The diff coverage is 100%.

Additional details and impacted files
@@          Coverage Diff          @@
##           master    #92   +/-   ##
=====================================
+ Coverage      88%    91%   +3%     
=====================================
  Files          18     19    +1     
  Lines        2196   2202    +6     
=====================================
+ Hits         1939   2011   +72     
+ Misses        257    191   -66     
Files Coverage Δ
tests/unit/test_main.py 100% <100%> (ø)
tests/unit/test_output.py 100% <100%> (ø)
xen-bugtool 88% <100%> (+3%) ⬆️
Flag Coverage Δ
python2.7 89% <100%> (+3%) ⬆️
python3.10.13 91% <100%> (+3%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

…ive (xenserver#93)

Signed-off-by: Bernhard Kaindl <bernhard.kaindl@cloud.com>
@bernhardkaindl bernhardkaindl changed the title CP-48447: Fix Python3 handling Exceptions from func_output calls CA-390127 (fix saving all log messages) and CP-48447 (correct exception handling for func_output callbacks) Mar 21, 2024
@xenserver xenserver deleted a comment from coveralls Mar 21, 2024
@bernhardkaindl bernhardkaindl changed the title CA-390127 (fix saving all log messages) and CP-48447 (correct exception handling for func_output callbacks) CP-48447+CA-390127: correct exception handling for func_output callbacks, fix saving all log messages Mar 23, 2024
@bernhardkaindl bernhardkaindl merged commit ed6b074 into xenserver:master Mar 25, 2024
13 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.

3 participants