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

[system-health] Convert to Python 3 #5886

Merged
merged 3 commits into from
Dec 29, 2020
Merged

[system-health] Convert to Python 3 #5886

merged 3 commits into from
Dec 29, 2020

Conversation

jleveque
Copy link
Contributor

@jleveque jleveque commented Nov 10, 2020 •
edited
Loading

- Why I did it

As part of moving all SONiC code from Python 2 (no longer supported) to Python 3.

- How I did it

  • Convert system-health scripts to Python 3
  • Build and install system-health as a Python 3 wheel
  • Also convert newlines from DOS to UNIX.

- How to verify it

Ensure system-health package builds, installs and works properly

- Which release branch to backport (provide reason below if selected)

  • 201811
  • 201911
  • 202006

Fixes #6104

@jleveque jleveque self-assigned this Nov 10, 2020
@jleveque jleveque marked this pull request as ready for review November 11, 2020 18:20
@jleveque
Copy link
Contributor Author

@Junchao-Mellanox: Please review.

@Junchao-Mellanox
Copy link
Collaborator

See https://github.com/Azure/sonic-buildimage/blob/6c362a08e72e14da8880586f089d4860e786947c/src/system-health/scripts/healthd#L71, system health depends on platform API to set the chassis LED. It means that we need the platform API to support python3 first.

@jleveque
Copy link
Contributor Author

@Junchao-Mellanox: I believe currently only Mellanox supports the system-health daemon. I will set this back to draft until Mellanox has a Python 3 sonic-platform package installed. Please keep me posted.

@jleveque jleveque marked this pull request as draft November 12, 2020 05:43
@Junchao-Mellanox
Copy link
Collaborator

Sure.

@jleveque jleveque mentioned this pull request Dec 3, 2020
Closed
@jleveque jleveque mentioned this pull request Dec 10, 2020
Merged
3 tasks
@keboliu
Copy link
Collaborator

keboliu commented Dec 11, 2020

a preliminary check, seems that we still need some change in this here https://github.com/Azure/sonic-buildimage/blob/master/src/system-health/health_checker/utils.py#L11

@keboliu
Copy link
Collaborator

keboliu commented Dec 11, 2020

in these two lines
https://github.com/Azure/sonic-buildimage/blob/master/src/system-health/health_checker/service_checker.py#L43
https://github.com/Azure/sonic-buildimage/blob/master/src/system-health/health_checker/service_checker.py#L48

need decode the 'output' before using : output.decode('utf-8')

@jleveque
Copy link
Contributor Author

jleveque commented Dec 11, 2020 •
edited
Loading

in these two lines
https://github.com/Azure/sonic-buildimage/blob/master/src/system-health/health_checker/service_checker.py#L43
https://github.com/Azure/sonic-buildimage/blob/master/src/system-health/health_checker/service_checker.py#L48

need decode the 'output' before using : output.decode('utf-8')

Thanks for catching this! Rather than decoding, I added a text=True parameter to the subprocess.Popen() call. This is an alias of the universal_newlines=True parameter which is only available in Python 3. Since we no longer need to support Python 2 with this package, I used the new, shorter, more understandable alias. This will force the output to always be a string.

EDIT: I replaced text=True with universal_newlines=True, because LGTM is still processing these files as Python 2, even though the setup.py file has been updated to Python 3 only. It must be looking at some other setup.py file in the repo. So, for the time being, I changed it to universal_newlines=True, which works with both Python 2 and Python 3.

Also, note that I found that the Python files all had DOS line endings, so I changed them all to UNIX, which is why the diff is now much larger.

@lgtm-com

This comment has been minimized.

Sign in to view
@jleveque
Copy link
Contributor Author

@keboliu, @Junchao-Mellanox: Are we good to merge this PR now? If so, please review again.

@jleveque jleveque marked this pull request as ready for review December 28, 2020 18:23
keboliu
keboliu approved these changes Dec 29, 2020
View reviewed changes
Copy link
Collaborator

@keboliu keboliu left a comment

Choose a reason for hiding this comment

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

LGTM

@jleveque jleveque merged commit 566ea4f into sonic-net:master Dec 29, 2020
@jleveque jleveque deleted the system_health_py3 branch December 29, 2020 22:04
@jleveque jleveque mentioned this pull request Jan 6, 2021
Merged
4 tasks
lguohan pushed a commit that referenced this pull request Jan 7, 2021
@jleveque
[system-health] Make run_command() Python 3-compliant (#6371)
2d77a36 
Pass universal_newlines=True parameter to subprocess.Popen(); no longer use .encode('utf-8') on resulting stdout.
This was missed in #5886

Note: I would prefer to use text=True instead of universal_newlines=True, as the former is an alias only available in Python 3 and is more understandable than the latter. However, Even though the setup.py file for this package only specifies Python 3, the LGTM tool finds other Python 2 code in the repo and validates the code as Python 2 code and alerts that text=True is an invalid parameter. Will stick with universal_newlines=True for now. Once all Python code in the repo has been converted to Python 3, I will change all universal_newlines=True to text=True.
lguohan pushed a commit that referenced this pull request Jan 9, 2021
@jleveque @lguohan
[system-health] Make run_command() Python 3-compliant (#6371)
984c833 
Pass universal_newlines=True parameter to subprocess.Popen(); no longer use .encode('utf-8') on resulting stdout.
This was missed in #5886

Note: I would prefer to use text=True instead of universal_newlines=True, as the former is an alias only available in Python 3 and is more understandable than the latter. However, Even though the setup.py file for this package only specifies Python 3, the LGTM tool finds other Python 2 code in the repo and validates the code as Python 2 code and alerts that text=True is an invalid parameter. Will stick with universal_newlines=True for now. Once all Python code in the repo has been converted to Python 3, I will change all universal_newlines=True to text=True.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Junchao-Mellanox Junchao-Mellanox Junchao-Mellanox approved these changes

@keboliu keboliu keboliu approved these changes

@lguohan lguohan lguohan approved these changes

Assignees

@jleveque jleveque

Labels
Python 3
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

"show system-health summary" crashes due to missing module health_checker.
4 participants
@jleveque @Junchao-Mellanox @keboliu @lguohan