CA-390883: Add unit test for usb_reset.py (code fix done by CA-388318)

[bernhardkaindl]

Info: This test uses pytest, but before you say that tests (without pytest) run in XS8 xapi build, show where these tests run: As shown below, unittest likely cannot work, and nosetests is not installed in xapi.spec.

Add a unit test that uses the equivalent of /usr/bin/unshare --map-root-user --mount (which creates a temporary, root-less mount namespace) for testing the actual, not mocked mount() and unmount() implementations of scripts/usb_reset.py.

This serves just as an example that testing code that would otherwise require root privileges is perfectly possible and should be done to avoid committing any mistakes.

The actual fix for updating the mount() and umount() functions of scripts/usb_reset.py for Python3 can be pushed as a 2nd commit to the PR, at which, the test can be enabled for Python3 (no longer be skipped for Python3).

Cc: @ashwin9390

Test usb_reset.mount/.umount to work in a rootless container

[psafont]

Correct me if I'm wrong, but I believe pytest is not available in xenserver 8. Does this mean these will not be run when building the xapi package?

[bernhardkaindl]

I could not find the invocation of a Python unittest in the xen-api build so far.

nosetests was used before, but it looks like it is no longer used.

Trying python -m unittest, I found this:

In scripts/test_usb_scan.py, the decorator @nottest is used to declare some functions as not being tests.

But Python unittest does not know about @nottest (from the previously used nosetest) and tries to execute these functions as if they were tests, which fails:

# (cd scripts;python2 -m unittest discover 2>&1 |grep ERROR)
# (cd scripts;python3 -m unittest discover 2>&1 |grep ERROR)
ERROR: test_usb_common (test_usb_scan.TestUsbScan)
TypeError: test_usb_common() takes at least 4 arguments (1 given)
ERROR: test_usb_config_error_common (test_usb_scan.TestUsbScan)
(likewise)
ERROR: test_usb_dongle (test_usb_scan.TestUsbScan)
ERROR: test_usb_dongle_on_hub (test_usb_scan.TestUsbScan)
ERROR: test_usb_dongle_unbinded (test_usb_scan.TestUsbScan)
ERROR: test_usb_exit (test_usb_scan.TestUsbScan)
ERROR: test_usb_keyboard (test_usb_scan.TestUsbScan)

unittest only passes the self parameter, but not the other parameter, thus unittest fails.

I concluded that there is no way that the unit tests are executed in the xapi build if pytest (or the formerly used nosetests) is not available.

python3 -m unittest discover just does nothing:

----------------------------------------------------------------------
Ran 0 tests in 0.000s

OK

This is a big difference between unittest and pytest:

• unittest passes if it finds no tests
• pytest exits with exit status 5 if it finds no tests.

This weakness, and the other reasons (you lose so much if not using pytest), should be enough to justify adding pytest to XS8 if we want proper unit testing in XS8.

[acefei]

From the past revert cases, adding UT is beneficial, but it's not a silver bullet.
We often involves mocking the builtin/3rd party modules in UT, and these are precisely the areas that require encoding/parameter changing.

Let's all adhere to the same PR process as we discussed in toolstack channel.

• the change compiles in koji
• it passes a Ring3 BVT+BST
• adding more CI-level testing of python code

Finally, please follow what @robhoes emphasized: And verify manually. Always do that.

[liulinC]

why not put the module into sys.modules

[liulinC]

Given we are sure that we do have XenRT to cover this,

Doubting if it is necessary to add such complicated unittest, which requires to maintain anyway.

Also, do not forget to do rpm/spec build to make sure it is not broken.

[liulinC]

this function is not called, did you intention to call it automatically when leave the session?

[liulinC]

After reconsider,
Given we are pretty sure XenRT already cover this,

Here we involve so much code just to run mount/umount,
which involve unnecessary maintain effort.
How about just include the tiny fix for scripts/usb_reset.py, and leave others part to XenRT? @psafont

[acefei]

We might consider encapsulating these two functions as the following pseudo code.
The Mounter class encapsulates the loading of libc and the management of mount/umount methods, that could enhance the code's reusability and won't need to loading libc repeatly and implementing the enter and exit methods, enabling the Mounter object to be used within a with statement.

import ctypes
import os
import logging

class Mounter:
    def __init__(self):
        self.libc = ctypes.CDLL(ctypes.util.find_library("c"), use_errno=True)
        self.target = None

    def mount(self, source, target, fs, flags=0):
        if self.libc.mount(source.encode(), target.encode(), fs.encode(), flags, None) < 0:
            error_message = "Failed to mount {} ({}) to {} with flags {}: {}".format(
                source, fs, target, flags, os.strerror(ctypes.get_errno())
            )
            logging.error(error_message)
            return error_message
        else:
            self.target = target
            return "Mount successful"

    def umount(self, target, flags=0):
        if self.libc.umount2(target.encode(), flags) < 0:
            error_message = "Failed to unmount {} with flags {}: {}".format(
                target, flags, os.strerror(ctypes.get_errno())
            )
            logging.error(error_message)
            return error_message
        else:
            return "Unmount successful"
    
    def __enter__(self):
        return self

    def __exit__(self, exc_type, exc_value, traceback):
        if self.target is not None:
            self.umount(self.target)

BTW, as libc manual mentioned https://www.gnu.org/software/libc/manual/html_node/Mount_002dUnmount_002dRemount.html

umount does the same thing as umount2 with flags set to zeroes. It is more widely available than umount2 but since it lacks the possibility to forcefully unmount a filesystem is deprecated when umount2 is also available.

Should we consider using umount2 instead?

[edwintorok]

Given we are pretty sure XenRT already cover this,

The previous PR that broke usb_reset.py was apparently tested in XenRT, but the wrong commit was used for the test: one that had #!/usr/bin/env python (which worked) instead of #!/usr/bin/env python3 (which was on the github PR and what got merged).
So having a test that runs together with the commit hopefully avoids those kinds of mistakes.

[edwintorok]

(Although if it was up to me we'd just rewrite these python scripts in OCaml and avoid these problems altogether, but that would take a long time).
In particular this script is not very long.

Maybe we should have a new rule: whenever a python2->3 port breaks a python script we rewrite that script in OCaml. And no new Python scripts.

[liulinC]

Given we are pretty sure XenRT already cover this,

The previous PR that broke usb_reset.py was apparently tested in XenRT, but the wrong commit was used for the test: one that had #!/usr/bin/env python (which worked) instead of #!/usr/bin/env python3 (which was on the github PR and what got merged). So having a test that runs together with the commit hopefully avoids those kinds of mistakes.

This happens because we did not have feature branch, and new joiners may not familiar with such.
In case of feature branch, this should not happens. Also, XenRT will find the issue, sooner or later, as in this case, which does not affect customer.

This is introducing maintain effort to toolstack CoP anyway, if the CoP decide to take it, I have no objection.

[liulinC]

python->ocaml is right and better choice in long term, and I agree with it.
But it is CoP work, and should not block XS9,
Considering the current XS9 BaseOS developers has not even touch ocaml (except myself),
I would like to suggest the CoP members take this work.

[psafont]

It'd be nice to get this merged into the feature branch, can the testing for python 2 be dropped?

[psafont]

Requesting changes in the interface to make it obvious that the PR needs work

[psafont]

Closing due to inactivity, will speak with other engineers to merge this thing