Skip to content

Conversation

@igaw
Copy link
Collaborator

@igaw igaw commented Jan 29, 2025

The attach operation takes a bit of time and is asynchronous, thus we should monitor if the block device appears for a period, before returning an error.

#2668

@MaisenbacherD
Copy link
Contributor

I just tried running this on my fork with the updated debian.python container from ci-containers linux-nvme/ci-containers#4.
It looks like the pyinotify dependency is not yet correctly installed:
https://github.com/MaisenbacherD/nvme-cli/actions/runs/13047505313/job/36400515615

ERROR: nvme_attach_detach_ns_test (nose2.loader.LoadTestsFailure.nvme_attach_detach_ns_test)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/__w/nvme-cli/nvme-cli/tests/nvme_attach_detach_ns_test.py", line 32, in <module>
    from nvme_test import TestNVMe
  File "/__w/nvme-cli/nvme-cli/tests/nvme_test.py", line 34, in <module>
    import pyinotify
ModuleNotFoundError: No module named 'pyinotify'

I am already looking into fixing this dependency. Will update you here :)

@igaw
Copy link
Collaborator Author

igaw commented Jan 30, 2025

It looks like your build jobs pull from your repo the container images and not from upstream.

@MaisenbacherD
Copy link
Contributor

Yes it does. There is no public read access for ghcr.io/linux-nvme/ container images. That's why I have to build the container image by hand and push it to my container repository first. I made sure to update the image before running the workflow :)

Maybe ghcr.io/linux-nvme/ can allow public read access?

@igaw
Copy link
Collaborator Author

igaw commented Jan 30, 2025

Sure we can allow public read access. I think the old repo from me did have public access. When we moved it over we were a bit too paranoid.

@MaisenbacherD
Copy link
Contributor

pyinotify can't be installed via pipx because it uses the legacy 'setup.py install' method which does not configure the setup entry_point correctly. pyinotify seams to be abandoned.
We are using pipx to install nose2 and I was not able to link a pip or package installed pyinotify into the virtual python environment that pipx creates.

I am able to install the required packages in the classic way:

mkdir -p /python-env
python3 -m venv /python-env
source /python-env/bin/activate
pip install pyinotify
pip install nose2

However, we are getting a deprecation warning with that.

  DEPRECATION: pyinotify is being installed using the legacy 'setup.py install' method, because it does not have a 'pyproject.toml' and the 'wheel' package is not installed. pip 23.1 will enforce this behaviour change. A possible replacement is to enable the '--use-pep517' option. Discussion can be found at https://github.com/pypa/pip/issues/8559

Question: should we maybe find a replacement for pyinotify or do we want to start using it?
If we want to start using it I can post some patches for ci-containers and the GitHub CI workflow to make it work. :)

@igaw
Copy link
Collaborator Author

igaw commented Jan 30, 2025

I am not attached to pyinotify. I picked because it was packaged up for Fedora. Thinking about it, we could just go without it, and implement a polling loop. That avoids all this dependency hell and it's not that we would spend a lot of resources doing so.

@igaw
Copy link
Collaborator Author

igaw commented Jan 30, 2025

btw, pylint is complaining about a lot of style issues:

$ pylint tests/nvme_test.py
************* Module nvme_test
tests/nvme_test.py:70:0: C0301: Line too long (119/100) (line-too-long)
tests/nvme_test.py:77:0: C0301: Line too long (122/100) (line-too-long)
tests/nvme_test.py:274:0: C0325: Unnecessary parens after '=' keyword (superfluous-parens)
tests/nvme_test.py:276:0: C0325: Unnecessary parens after '=' keyword (superfluous-parens)
tests/nvme_test.py:39:0: R0902: Too many instance attributes (14/7) (too-many-instance-attributes)
tests/nvme_test.py:113:8: W0612: Unused variable 'x1' (unused-variable)
tests/nvme_test.py:113:12: W0612: Unused variable 'x2' (unused-variable)
tests/nvme_test.py:125:13: W1514: Using open without explicitly specifying an encoding (unspecified-encoding)
tests/nvme_test.py:157:15: R1732: Consider using 'with' for resource-allocating operations (consider-using-with)
tests/nvme_test.py:174:15: R1732: Consider using 'with' for resource-allocating operations (consider-using-with)
tests/nvme_test.py:190:15: R1732: Consider using 'with' for resource-allocating operations (consider-using-with)
tests/nvme_test.py:211:15: R1732: Consider using 'with' for resource-allocating operations (consider-using-with)
tests/nvme_test.py:232:15: R1732: Consider using 'with' for resource-allocating operations (consider-using-with)
tests/nvme_test.py:259:15: R1732: Consider using 'with' for resource-allocating operations (consider-using-with)
tests/nvme_test.py:297:15: R1732: Consider using 'with' for resource-allocating operations (consider-using-with)
tests/nvme_test.py:328:15: R1732: Consider using 'with' for resource-allocating operations (consider-using-with)
tests/nvme_test.py:354:15: R1732: Consider using 'with' for resource-allocating operations (consider-using-with)
tests/nvme_test.py:378:4: R0913: Too many arguments (6/5) (too-many-arguments)
tests/nvme_test.py:378:4: R0917: Too many positional arguments (6/5) (too-many-positional-arguments)
tests/nvme_test.py:416:20: E0602: Undefined variable 'time' (undefined-variable)
tests/nvme_test.py:417:14: E0602: Undefined variable 'time' (undefined-variable)
tests/nvme_test.py:418:15: E0602: Undefined variable 'handler' (undefined-variable)
tests/nvme_test.py:463:15: R1732: Consider using 'with' for resource-allocating operations (consider-using-with)
tests/nvme_test.py:483:15: R1732: Consider using 'with' for resource-allocating operations (consider-using-with)
tests/nvme_test.py:500:15: R1732: Consider using 'with' for resource-allocating operations (consider-using-with)
tests/nvme_test.py:529:17: R1732: Consider using 'with' for resource-allocating operations (consider-using-with)
tests/nvme_test.py:536:17: R1732: Consider using 'with' for resource-allocating operations (consider-using-with)
tests/nvme_test.py:90:8: W0201: Attribute 'dps' defined outside __init__ (attribute-defined-outside-init)
tests/nvme_test.py:94:8: W0201: Attribute 'nsze' defined outside __init__ (attribute-defined-outside-init)
tests/nvme_test.py:95:8: W0201: Attribute 'ncap' defined outside __init__ (attribute-defined-outside-init)
tests/nvme_test.py:96:8: W0201: Attribute 'ctrl_id' defined outside __init__ (attribute-defined-outside-init)
tests/nvme_test.py:39:0: R0904: Too many public methods (28/20) (too-many-public-methods)
tests/nvme_test.py:26:0: W0611: Unused import mmap (unused-import)

@MaisenbacherD
Copy link
Contributor

I have pylinting on my TODO list :)
The testcases are also not free from style issues.

The attach operation takes a bit of time and is asynchronous, thus we
should monitor if the block device appears for a period, before
returning an error.

Signed-off-by: Daniel Wagner <[email protected]>
stdout=subprocess.DEVNULL)
if err == 0:
# enumerate new namespace block device
self.nvme_reset_ctrl()
Copy link
Contributor

Choose a reason for hiding this comment

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

Let me confirm below.
Before the controller reset done before the blkdev checking but is it not necessary to be done for the changes as waiting for 5 seconds instead? (I am not sure the reason for the reset on the current TP.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The reset should not be necessary. When the device announce a new namespace (AEN) the driver exposes the new namespace (new block device) to userspace and announces it via udev. We could obviously also try to listen to udev and test this interface as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Understood. Thank you for your explaining.

@igaw igaw merged commit 56cf51b into linux-nvme:master Jan 31, 2025
17 checks passed
@igaw igaw deleted the ty branch January 31, 2025 10:59
@igaw
Copy link
Collaborator Author

igaw commented Jan 31, 2025

Let's see if this helps!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet