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

Update pcieutil error message on loading common pcie module #2786

Merged
merged 7 commits into from
May 20, 2023

Conversation

cytsao1
Copy link
Contributor

@cytsao1 cytsao1 commented Apr 10, 2023

Currently, pcieutil will print the following message to console every time it is called if loading the common module:
root@sonic:/home/cisco# pcieutil show
Failed to load platform Pcie module. Error : No module named 'sonic_platform.pcie', fallback to load Pcie common utility.
==============================Display PCIe Device===============================

What I did

Update the error message to only be logged in syslog, rather than print to console.

How to verify it

Run any pcieutil command

Previous command output (if the output of a command-line utility has changed)

root@sonic:/home/cisco# pcieutil show
Failed to load platform Pcie module. Error : No module named 'sonic_platform.pcie', fallback to load Pcie common utility.
==============================Display PCIe Device===============================
......

New command output (if the output of a command-line utility has changed)

root@sonic:/home/cisco# pcieutil show
==============================Display PCIe Device===============================
.....

@cytsao1
Copy link
Contributor Author

cytsao1 commented Apr 10, 2023

@abdosi Can this be reviewed?

@prgeor
Copy link
Contributor

prgeor commented Apr 13, 2023

@cytsao1 please add code coverage for your changes

@prgeor prgeor self-requested a review April 13, 2023 16:39
@prgeor prgeor self-assigned this Apr 13, 2023
@cytsao1
Copy link
Contributor Author

cytsao1 commented Apr 13, 2023

please add code coverage for your changes

@prgeor Do you mean to update this test file? https://github.com/sonic-net/sonic-utilities/blob/master/tests/pcieutil_test.py
or something else? This code should be invoked by any pcieutil command.

@prgeor
Copy link
Contributor

prgeor commented Apr 18, 2023

please add code coverage for your changes

@prgeor Do you mean to update this test file? https://github.com/sonic-net/sonic-utilities/blob/master/tests/pcieutil_test.py or something else? This code should be invoked by any pcieutil command.

@cytsao1 yes

@cytsao1
Copy link
Contributor Author

cytsao1 commented May 5, 2023

@prgeor Can this be reviewed now? Added test.

except ImportError:
pass
sys.stdout = stdout
assert pcieutil_load_module_warning_msg not in result.getvalue()
Copy link
Contributor

Choose a reason for hiding this comment

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

@cytsao1 what does this ASSERT confirm? There could be platform which may not see this import Error and in which case this ASSERT may fail?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test is to confirm that the import warning message will not be in the console output, whether the Import error is raised or not.

Copy link
Contributor

Choose a reason for hiding this comment

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

@cytsao1 result value is empty string at the start, so when the test run at line 209, result is not updated at all and at line 213 ofcourse pcieutil_load_module_warning_msg won't match result value, so i don't understand why even run pcieutil.load_platform_pcieutils() at line 209?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sys.stdout is updated to be same IO stream as result, so running load_platform_pcieutil output will be captured in result instead of output to console stdout.
After, the stdout is set back to the sys.stdout stream.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Example of how it would look without this PR

>>> import sys
>>> from io import StringIO
>>> import pcieutil.main as pcieutil
>>> stdout = sys.stdout
>>> sys.stdout = result = StringIO()
>>> pcieutil.load_platform_pcieutil()
>>> sys.stdout = stdout
>>> result.getvalue()
"Failed to load platform Pcie module. Error : No module named 'sonic_platform.pcie', fallback to load Pcie common utility.\n"

Copy link
Contributor

Choose a reason for hiding this comment

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

@cytsao1 thanks for clarifying....did not realize that stdout was captured in a string output...

@cytsao1
Copy link
Contributor Author

cytsao1 commented May 10, 2023

Can someone with permissions merge this?

@bmridul
Copy link
Contributor

bmridul commented May 12, 2023

@abdosi @prgeor
Pls help merge this.

@abdosi
Copy link
Contributor

abdosi commented May 19, 2023

@prgeor can we merge this ?

@abdosi
Copy link
Contributor

abdosi commented May 20, 2023

@prgeor can we merge this ?

@prgeor you comment is addressed above. Do we need more clarification ?

@prgeor prgeor merged commit 3d89589 into sonic-net:master May 20, 2023
yxieca pushed a commit that referenced this pull request May 26, 2023
* Update pcieutil load module error message

* Add pcieutil test for load module warning to not print to output

* Update pcieutil import test

* Update pcieutil import test

* Fix pcieutil import test
pdhruv-marvell pushed a commit to pdhruv-marvell/sonic-utilities that referenced this pull request Aug 23, 2023
…t#2786)

* Update pcieutil load module error message

* Add pcieutil test for load module warning to not print to output

* Update pcieutil import test

* Update pcieutil import test

* Fix pcieutil import test
yxieca pushed a commit that referenced this pull request Aug 29, 2023
* Update pcieutil load module error message

* Add pcieutil test for load module warning to not print to output

* Update pcieutil import test

* Update pcieutil import test

* Fix pcieutil import test
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.

5 participants