Skip to content

Fix Python 3 'importlib' bug; Add support for Python 2 back in sonic-py-common#6933

Merged
jleveque merged 4 commits intosonic-net:masterfrom
jleveque:continue_support_py2_load_source
Mar 3, 2021
Merged

Fix Python 3 'importlib' bug; Add support for Python 2 back in sonic-py-common#6933
jleveque merged 4 commits intosonic-net:masterfrom
jleveque:continue_support_py2_load_source

Conversation

@jleveque
Copy link
Copy Markdown
Contributor

@jleveque jleveque commented Mar 1, 2021

Why I did it

Fix a strange bug introduced by #6832 which would only occur in environments with both Python 2 and Python 3 installed (e.g., the PMon container). Error messages such as the following would be seen:

ERR pmon#ledd[29]: Failed to load ledutil: module 'importlib' has no attribute 'machinery'

This is very odd, and it seems like the Python 2 version of importlib, which is basically just a stub, is taking precedence over the Python 3 version. I found that this occurs when calling import importlib. However, calling import importlib.machinery and import importlib.util causes the proper package to be referenced, and the machinery and util modules are loaded successfully. This is how it is specified in examples in the official documentation, however there is nothing mentioned regarding that it should be done this way or that import importlib is unreliable.

Also, since sonic-py-common is still used in environments with Python 2 installed we should maintain support for both Python 2 and 3 until we completely deprecate Python 2, so I have added this back in.

How I did it

  • Call import importlib.machinery and import importlib.util explicitly
  • Re-introduce Python 2 support in _load_module_from_file() method. If Python 2, load source via the imp module, if Python 3, use the importlib module.

How to verify it

Ensure ledd daemon starts properly with both Python 2 and Python 3

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

  • 201811
  • 201911
  • 202006
  • 202012

yxieca
yxieca previously approved these changes Mar 1, 2021
@lguohan
Copy link
Copy Markdown
Collaborator

lguohan commented Mar 1, 2021

as my understanding, as part of the pr #6832, you only tested python3, but not python2, therefore python2 is broken?

@lguohan
Copy link
Copy Markdown
Collaborator

lguohan commented Mar 1, 2021

why is that the unit test determine-reboot-cause_test.py does not catch the issue?

@jleveque
Copy link
Copy Markdown
Contributor Author

jleveque commented Mar 1, 2021

as my understanding, as part of the pr #6832, you only tested python3, but not python2, therefore python2 is broken?

No. Python 3 was broken in the PMon container, where Python 2 is still installed, because it breaks the import mechanism when calling import importlib. I also decided to add Python 2 support back, just in case it is needed on other platforms until we completely deprecate Python 2.

@jleveque
Copy link
Copy Markdown
Contributor Author

jleveque commented Mar 1, 2021

why is that the unit test determine-reboot-cause_test.py does not catch the issue?

It appears to be environment-specific. I have not been able to determine the exact conditions necessary to produce the issue.

The documentation shows examples of importing the machinery and util modules by their full paths, but does not explicitly state it should be done that way. @lguohan: I believe I should update this PR to do the same for all other instances to prevent any potential issues. What do you think?

@jleveque jleveque changed the title [sonic_py_common.daemon_base] Fix Python 3 bug; Add support for Python 2 back for the time being Fix Python 3 'importlib' bug; Add support for Python 2 back for the time being Mar 2, 2021
@jleveque jleveque changed the title Fix Python 3 'importlib' bug; Add support for Python 2 back for the time being Fix Python 3 'importlib' bug; Add support for Python 2 back in sonic-py-common Mar 2, 2021
@jleveque jleveque merged commit 980a024 into sonic-net:master Mar 3, 2021
@jleveque jleveque deleted the continue_support_py2_load_source branch March 3, 2021 02:31
yxieca pushed a commit that referenced this pull request Mar 4, 2021
…py-common (#6933)

Fix a strange bug introduced by #6832 which would only occur in environments with both Python 2 and Python 3 installed (e.g., the PMon container). Error messages such as the following would be seen:

```
ERR pmon#ledd[29]: Failed to load ledutil: module 'importlib' has no attribute 'machinery'
```

This is very odd, and it seems like the Python 2 version of importlib, which is basically just a stub, is taking precedence over the Python 3 version. I found that this occurs when calling `import importlib`. However, calling `import importlib.machinery` and `import importlib.util` causes the proper package to be referenced, and the `machinery` and `util` modules are loaded successfully. This is how it is specified in examples in the official documentation, however there is nothing mentioned regarding that it *should* be done this way or that `import importlib` is unreliable.

Also, since sonic-py-common is still used in environments with Python 2 installed we should maintain support for both Python 2 and 3 until we completely deprecate Python 2, so I have added this back in.
carl-nokia pushed a commit to carl-nokia/sonic-buildimage that referenced this pull request Aug 7, 2021
…py-common (sonic-net#6933)

Fix a strange bug introduced by sonic-net#6832 which would only occur in environments with both Python 2 and Python 3 installed (e.g., the PMon container). Error messages such as the following would be seen:

```
ERR pmon#ledd[29]: Failed to load ledutil: module 'importlib' has no attribute 'machinery'
```

This is very odd, and it seems like the Python 2 version of importlib, which is basically just a stub, is taking precedence over the Python 3 version. I found that this occurs when calling `import importlib`. However, calling `import importlib.machinery` and `import importlib.util` causes the proper package to be referenced, and the `machinery` and `util` modules are loaded successfully. This is how it is specified in examples in the official documentation, however there is nothing mentioned regarding that it *should* be done this way or that `import importlib` is unreliable.

Also, since sonic-py-common is still used in environments with Python 2 installed we should maintain support for both Python 2 and 3 until we completely deprecate Python 2, so I have added this back in.
lolyu pushed a commit to lolyu/sonic-buildimage that referenced this pull request Sep 13, 2021
…py-common (sonic-net#6933)

Fix a strange bug introduced by sonic-net#6832 which would only occur in environments with both Python 2 and Python 3 installed (e.g., the PMon container). Error messages such as the following would be seen:

```
ERR pmon#ledd[29]: Failed to load ledutil: module 'importlib' has no attribute 'machinery'
```

This is very odd, and it seems like the Python 2 version of importlib, which is basically just a stub, is taking precedence over the Python 3 version. I found that this occurs when calling `import importlib`. However, calling `import importlib.machinery` and `import importlib.util` causes the proper package to be referenced, and the `machinery` and `util` modules are loaded successfully. This is how it is specified in examples in the official documentation, however there is nothing mentioned regarding that it *should* be done this way or that `import importlib` is unreliable.

Also, since sonic-py-common is still used in environments with Python 2 installed we should maintain support for both Python 2 and 3 until we completely deprecate Python 2, so I have added this back in.
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.

3 participants