CP-41972: update xcp to support ack py3 #21

alexhimmel · 2023-04-18T08:55:50Z

No description provided.

Signed-off-by: alexhimmel <alex.ac@qq.com>

psafont · 2023-04-18T09:59:21Z

There's a PR open for python 3 compatibility, I believe it's more complete than this one, could you take a look and review the other one? #17

bernhardkaindl · 2023-04-18T15:24:20Z

xcp/pci.py

+        fh = open(fn, 'r', encoding='UTF-8')
        for l in fh:
            line = l.rstrip()


The purpose of this change is not clear, the reason is:

open() does not change it's return value between Python2 and Python3:
See: https://docs.python.org/3.6/library/functions.html#open -> See "text mode (default)"

All of these work just fine and produce the same output:

python2 -c 'file=open("/etc/passwd"); lines = [line.decode() for line in file]; print(lines[0])' python3 -c 'file=open("/etc/passwd"); lines = [line.decode() for line in file]; print(lines[0])' python3 -c 'file=open("/etc/passwd", "r", encoding="UTF-8"); lines = [line for line in file]; print(lines[0])'

But encoding= is not supported by Python2:

python2.7 -c "open('/etc/passwd', 'r', encoding='UTF-8')" Traceback (most recent call last): File "<string>", line 1, in <module> TypeError: 'encoding' is an invalid keyword argument for this function

Also, it does not change anything because UTF-8 is already the default encoding, at least on XenServer 8.3.0 with Python3.6 installed:

[root@lcy2-dt76 ~]# python3 -c "import locale;print(locale.getpreferredencoding())" UTF-8 [root@lcy2-dt76 ~]# python2 -c "import locale;print(locale.getpreferredencoding())" UTF-8

If you use a different Python enviromnent for testing, and get different results, switch it to UTF-8 by default as well.

Therefore, revert this change:

Suggested change

fh = open(fn, 'r', encoding='UTF-8')

for l in fh:

line = l.rstrip()

fh = open(fn)

for l in fh:

line = l.rstrip()

Hi @bernhardkaindl
Got error when try the nwe branch with fh = open(fn)

About to call plugin 'get_network_devices' on host 'OpaqueRef:482bcea2-6914-9d49-ecc9-6708b236fa9e' with args '{}' Traceback (most recent call last): File "./ack_cli.py", line 536, in <module> main(config, test_run_file) File "./ack_cli.py", line 508, in main generate_test_config(session, config, test_run_file) File "./ack_cli.py", line 404, in generate_test_config ifs = network_interfaces_to_test(session, config) File "./ack_cli.py", line 335, in network_interfaces_to_test devices = utils.get_master_network_devices(session) File "/opt/xensource/packages/files/auto-cert-kit/utils.py", line 2427, in get_master_network_devices nics = call_ack_plugin(session, 'get_network_devices') File "/opt/xensource/packages/files/auto-cert-kit/utils.py", line 2226, in call_ack_plugin args) File "/usr/lib/python3.6/site-packages/XenAPI.py", line 260, in __call__ return self.__send(self.__name, args) File "/usr/lib/python3.6/site-packages/XenAPI.py", line 154, in xenapi_request result = _parse_result(getattr(self, methodname)(*full_params)) File "/usr/lib/python3.6/site-packages/XenAPI.py", line 234, in _parse_result raise Failure(result['ErrorDescription']) XenAPI.Failure: ['XENAPI_PLUGIN_FAILURE', 'get_network_devices', 'UnicodeDecodeError', "'ascii' codec can't decode byte 0xc2 in position 265: ordinal not in range(128)"]

Can fix this issue when specify the correct codec when opening the file or when calling the decode method e.g.
fh = open(fn, 'r', encoding='UTF-8') or somewhere decode the stdout with stdout.decode('UTF-8')

bernhardkaindl · 2023-04-18T15:31:01Z

xcp/pci.py

@@ -260,7 +260,7 @@ def __init__(self):
                               stdout = subprocess.PIPE)
        for l in cmd.stdout:
            line = l.rstrip()
-            el = [x for x in line.replace('"', '').split() if not x.startswith('-')]


Thanks @alexhimmel because Popen() changes it's return value to bytes, this is a needed fix
(BTW: In general, it's always a very good idea to explain why a change is needed).

But, the current testsuite also needs to be update to reflect this behavior, otherwise it would fail to pass for Python3. I've done the needed testsuite updates in my own fork, see below.

Thus, this change must be applied with the testsuite fix to have working CI/PR checks.

Likewise, the tests for biosdevname have to be fixed and cmd.py is also missing the same fixes. I've done all this as well.

You can review, checkout and test the needed fixes here: xenserver-next#2

Please check if the lated branch works for you:
https://github.com/xenserver-next/python-libs/tree/testsuite-driven-py3+Popen

PS: We have to fix the Python3 testsuite for #17 before we can merge these fixes, but it's next on my TODO, and you can test the new branch https://github.com/xenserver-next/python-libs/tree/testsuite-driven-py3+Popen fixes all Python3 issues you detec in python-lib.

Best Regards, Bernhard

Hi @bernhardkaindl
Thank you for helping me to review this PR, i will test with new branch "testsuite-driven-py3+Popen".
Regards,
Tian

The fixes issues which the changes in this PR adressed are now merged to master.

Hi @alexhimmel, you could schedule re-testing the ACK XAPI plugin with the new master branch in an upcoming sprint!

PS: The Python3 migration is not done yet, there are still many issues (or at least untested cases) which need testing and likely fixing.

For example the github action summay page is currently still showing pytype errors in xcp/accessor.py and xcp/repository.py.

If the ACK plugin uses such modules or you run into any other error, please open an issue here to report the problem, thanks!

Fixes one part of xenserver#21 by calling Popen(lspci -nm, universal_newlines=True) This makes Popen().stdout it return an __inter__: string, which the code expects (otherwise, it opens the stdout pipe as binary stream): From: https://docs.python.org/3.6/library/subprocess.html#popen-constructor: "If encoding or errors are specified, the file objects stdin, stdout and stderr are opened in text mode with the specified encoding and errors, as described above in Frequently Used Arguments. If universal_newlines is True, they are opened in text mode with default encoding. >>>__Otherwise, they are opened as binary streams.__<<<" The new test tests xcp.pci.PCIDevices() by setting PATH to a shell script which simluates "lspci -mn" with "cat tests/data/lspci-mn", so calls the real Popen. Final thoughts on using mock: Mocking xcp.pci.subprocess.Popen.stdout using open("tests/data/lspci-mn") does not model the actual behavior of subprocess.Popen().std* in Python3: By it's nature, mocking Popen() does not validate it's correct use in xcp.pci and therfore, it failed to detect the Python3 problem in `xcp/pci.py` Correctly modeling subprocess.Popen() without really calling it is nontrivial, and would require something like the testfixtures library and even that would still be a mock and use the actual subprocess class. Mocking is better suited when the test knows how an API must be called, which isn't the case here. Signed-off-by: Bernhard Kaindl <bernhard.kaindl@cloud.com>

bernhardkaindl · 2023-06-11T02:32:23Z

@alexhimmel tested https://github.com/xenserver/auto-cert-kit with the fixes from my merged PRs.

…/py3-update-tuple-lambda-to-loop xen-bugtool/make_inventory(): Py3: Update tuple lambda to for loop

CP-41972: update xcp to support ack py3

7dd5b6a

Signed-off-by: alexhimmel <alex.ac@qq.com>

bernhardkaindl reviewed Apr 18, 2023

View reviewed changes

alexhimmel mentioned this pull request Apr 20, 2023

CP-41972: update ACK to python3 and fixed some problems xenserver/auto-cert-kit#201

Merged

This was referenced May 10, 2023

xcp.pci: Fix xcp.pci.PCIIds.read() to support UTF-8 in hwdata/pci.ids #35

Closed

pci.PCIDevices(): Fix Popen("lspci -mn") for Py3 using universal_newlines=True #38

Merged

bernhardkaindl marked this pull request as draft May 15, 2023 21:37

bernhardkaindl added Will be fixed by other PRs Leave until as issues in it are fixed by other PRs bug labels May 15, 2023

bernhardkaindl added Waiting for confirmation that the bugs are fixed and removed Will be fixed by other PRs Leave until as issues in it are fixed by other PRs labels May 24, 2023

bernhardkaindl closed this Jun 11, 2023

bernhardkaindl added a commit to rosslagerwall/python-libs that referenced this pull request May 8, 2024

Merge pull request xenserver#21 from xenserver-next/private/bernhardk…

529752b

…/py3-update-tuple-lambda-to-loop xen-bugtool/make_inventory(): Py3: Update tuple lambda to for loop

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CP-41972: update xcp to support ack py3 #21

CP-41972: update xcp to support ack py3 #21

alexhimmel commented Apr 18, 2023

psafont commented Apr 18, 2023

bernhardkaindl Apr 18, 2023 •

edited

Loading

alexhimmel Apr 19, 2023 •

edited by bernhardkaindl

Loading

bernhardkaindl Apr 18, 2023 •

edited

Loading

alexhimmel Apr 19, 2023

bernhardkaindl May 24, 2023

bernhardkaindl commented Jun 11, 2023

CP-41972: update xcp to support ack py3 #21

CP-41972: update xcp to support ack py3 #21

Conversation

alexhimmel commented Apr 18, 2023

psafont commented Apr 18, 2023

bernhardkaindl Apr 18, 2023 • edited Loading

Choose a reason for hiding this comment

alexhimmel Apr 19, 2023 • edited by bernhardkaindl Loading

Choose a reason for hiding this comment

bernhardkaindl Apr 18, 2023 • edited Loading

Choose a reason for hiding this comment

alexhimmel Apr 19, 2023

Choose a reason for hiding this comment

bernhardkaindl May 24, 2023

Choose a reason for hiding this comment

bernhardkaindl commented Jun 11, 2023

bernhardkaindl Apr 18, 2023 •

edited

Loading

alexhimmel Apr 19, 2023 •

edited by bernhardkaindl

Loading

bernhardkaindl Apr 18, 2023 •

edited

Loading