Towards a single code-path for enumeration + dev_open #126
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This backend adds the ability to test xNVMe without having the necessary NVMe hardware for the other backends. It also doubles as a way to test performance of xNVMe without the added latency of a physical NVMe drive. Signed-off-by: Mads Ynddal <m.ynddal@samsung.com>
Add small check to skip cleaning subproject SPDK on Mac, as their build script does not gracefully handle macOS. This scenario only happens, if a Linux/BSD build has been performed in the same directories -- i.e. through Docker. Signed-off-by: Mads Ynddal <m.ynddal@samsung.com>
New backend which supports macOS mostly using existing POSIX functionality. Although with the addition to use the NVMeSMARTLibExternal.h provided by Xcode to query some information from the target disk. This does not enable arbitrary NVMe commands to be sent to a disk, but rather support for Get Log Page and Identify commands. Signed-off-by: Mads Ynddal <m.ynddal@samsung.com>
Signed-off-by: Mads Ynddal <m.ynddal@samsung.com>
The shebang previously used '/bin/bash', which is not always the optimal version. On macOS, it is too old to have the features we need, while the default bash is up to date. Signed-off-by: Mads Ynddal <m.ynddal@samsung.com>
* Arch
- The 'pkg-config' is part of 'base-devel'. The package 'pkgconf'
should not be needed.
* CentOS 7, Leap, Tumbleweed, and Ubuntu (bionic, focal, impish)
- Added on on these for reference
Signed-off-by: Simon A. F. Lund <simon.lund@samsung.com>
On the distros Arch Linux, CentOS 7, CentOS Stream8, and Gentoo, then pkg-config '.pc' files installed with prefix ``/usr/local`` are not in the default pkg-config search path. To circumvent this, one can either adjust PKG_CONFIG_PATH or install into a location searched by pkg-config per default. This does the latter, adjusting/adding build instructions and providing a note in the documentation describing why ``--prefix=/usr`` is used. Signed-off-by: Simon A. F. Lund <simon.lund@samsung.com>
This is added to ensure that 'pkg-config' is behaving as expected. By adding it to the CI, it is ensure that the usage described in the documentation actually behaves as described when providing 'pkg-config' examples. Additionally, in preparation for Python packages, then 'pkg-config' will be used to dynamically locate and load xNVMe. Signed-off-by: Simon A. F. Lund <simon.lund@samsung.com>
Add error on use of undefined variables. Signed-off-by: Simon A. F. Lund <simon.lund@samsung.com>
Signed-off-by: Simon A. F. Lund <simon.lund@samsung.com>
Signed-off-by: Simon A. F. Lund <simon.lund@samsung.com>
These are changes cleaning up Python / pip usage. A general attempt is made to invoke 'pip' via 'python3 -m pip' instead of 'pip3'. By doing so one system-change less is needed, e.g. the fix of `pip3` executable. * alpine: dropping installation of 'pyelftools' which is needed by SPDK, since SPDK is not supported on Alpine anyway. * centos-stream8: has a recent enough version of Python3 available via the system package-manager, this is used instead of building Python from source. * macos: changed install of meson from 'pip' to 'homebrew' Signed-off-by: Simon A. F. Lund <simon.lund@samsung.com>
Signed-off-by: Simon A. F. Lund <simon.lund@samsung.com>
Signed-off-by: Simon A. F. Lund <simon.lund@samsung.com>
Signed-off-by: Simon A. F. Lund <simon.lund@samsung.com>
The package requires Python 3.6+, attempting to not go beyond 3.6+ due
to lack of availability on a range of Linux distributions.
* Makefile: misc. utilities for working with the Python package
* The Python package itself
- 'python/xnvme' home of the Python package
- 'python/xnvme/library_loader.py', provides helpers used for
autoloading the xNVMe shared library
- 'python/xnvme/ctypes_header.py', autogenerated ctypes encapsulation
of the xNVMe C API
* Examples
- Provided as executables and bundled with the Python package
- Available in: 'python/bin/'
- Serves as stand-alone usage-examples
* Tests
- Provided with the Python xnvme package
- Available in 'python/xnvme/tests'
- Runnable with: 'python3 -m pytest --pyargs xnvme'
- Requires pytest
* git-ignore:
- Added the generated headers, don't want them as part of the repos
* pcf: exclude the py-ctypes-mapping from flake8
The remaining files: LICENSE/README.rst/setup.py/pyproject.toml, as the
"usual" suspects for a Python package.
Signed-off-by: Simon A. F. Lund <simon.lund@samsung.com>
With the revival of the Python language bindings, documentation is needed to describe what they are, how to use them, and the state they are in. This extension to the documentation does so. Signed-off-by: Simon A. F. Lund <simon.lund@samsung.com>
This adds a workflow-job named 'build-python' building the Python language bindings and emitting an installable Python package as a build-artifact named 'python-pkg' / xnvme-py-MAJOR-MINOR-PATCH.tar.gz. The workflow-job, runs before 'build-linux' and 'build-macos' as these depend on it to consume the 'python-pkg' for testing. The downside is that the workflow-pipeline is delayed the time it takes to process the job which is ~30 seconds, arguable this time is reclaimed for the total pipeline to finish since the 'build-python' step runs Alpine and the various tools needed are deployed there much faster than on the other Linux distributions and Linux. A actual shortcoming is that the build of the Python package is only tested in a single environment. However, the main motivation is that the CI emits a single package which is then tested on the various targets, this package can then be promoted for release. Additionally, the package will be tested based on only on the runtime-dependencies defined in (setup.py) and verified that it runs without all the packages defined in requirements.txt. Signed-off-by: Simon A. F. Lund <simon.lund@samsung.com>
When adding new things, like Cython, or changing build-requirements, then it is useful to get all the errors from the CI once, instead of having to see them one at a time. Signed-off-by: Mads Ynddal <m.ynddal@samsung.com>
This provides multiple Python distribution packages to live in the same Python namespace. This is done to separate the requirements for building and distributing the xNVMe Python bindings, specifically in preparation for the following extensions to the bindings: * Cython API mapping * Python bindings using the Cython API mapping * Distribution of the entire xNVMe source archive via pip for build-and-install of the xNVMe libraries using Python infrastructure The 'python/README.rst' contains and should be updated on these goals. This change affects alot of files, however, these are just because they are moving into a different location. Consequently, then the pre-commit-hook-config needs updating as well as the CI workflow definition. One functional change are in the Python package itself, that is changing 'setup.py' and adjusting to the namespace-usage: import xnvme.ctypes_mapping as xnvme to import xnvme.ctypes_mapping.api as xnvme To work around this, a star-import is added to the subpackage, to avoid the increased nesting-level. note: namespace-packages aren't zip-safe, that means, they can be installed and distributed as a zip-file, no issue, however, when installed on the system it is extracted from the archive. note: pytest test-package discovery looks for '__init__.py', these are not there in the top-level namespace-package, just one has to well pytest which where to look. Thus the change: python3 -m pytest --pyargs xnvme to python3 -m pytest --pyargs xnvme.ctypes_mapping That sums it up. Signed-off-by: Simon A. F. Lund <simon.lund@samsung.com>
flake8 is strict about line-width, Black is not. To avoid the pedantic nature of flake8, then this changes disables line-width checks from flake8, leaving the control of line-width to Black. Signed-off-by: Mads Ynddal <m.ynddal@samsung.com>
Support of find_namespace_packages was added in this version, but we need to specify the requirement, as Python 3.6 on Ubuntu Bionic uses an older version.
Debian 9 "Stretch" will no longer be maintained after June 30, 2022, therefore we will also no longer support it either. Signed-off-by: Mads Ynddal <m.ynddal@samsung.com>
Signed-off-by: Mads Ynddal <m.ynddal@samsung.com>
Signed-off-by: Mads Ynddal <m.ynddal@samsung.com>
Signed-off-by: Mads Ynddal <m.ynddal@samsung.com>
Signed-off-by: Mads Ynddal <m.ynddal@samsung.com>
To better manage Python dependencies and build environment, the Makefile has been updated to use Python venv for both new and existing build steps. Signed-off-by: Mads Ynddal <m.ynddal@samsung.com>
This is in preparation for Cython. Signed-off-by: Simon A. F. Lund <simon.lund@samsung.com>
This updates the package-listings to support: * Building the Python package(s) in a virtual environment (python3-venv) * Building the Python Cython header and bindings (python3-dev) Additionally, this sorts the package-listings and removes an old astyle listing Signed-off-by: Mads Ynddal <m.ynddal@samsung.com>
Signed-off-by: Karl Bonde Torp <k.torp@samsung.com>
safl
force-pushed
the
fabrics/multi-v2
branch
8 times, most recently
from
4e0c251
to
8e628cc
Compare
For fio to utilize a fabrics target with multiple systems, it needs a way for the user to specify which subsystem to use. This is done by providing 'subqn' as fio-option. Signed-off-by: Simon A. F. Lund <simon.lund@samsung.com>
The previous implementation of enumerate() and dev_open() did not handle fabrics with multiple subsystems correctly as it did not take the subnqn into account. This re-implementation replaces the two distinct probe_cb() and attach_cb() for enumeration and dev_open(), with the same probe_cb() and attach_cb(). The difference now lies in the constructions of transport-identifiers (trid) and in the a post-device-probe selection of controllers and namespaces from attached controllers. Additionally, the selection of controller and namespace upon dev_open(), now does a system-probe first, then searches the discovered controllers. Signed-off-by: Simon A. F. Lund <simon.lund@samsung.com>
Signed-off-by: Liad Oz <liadozil@gmail.com>
The initial fabrics tutorial demonstrated the use of a single NVMe device over fabrics. However, this is lacking as the common use-case for fabrics are multiple devices. This extends the documentation, showing how to do fabrics setup using multiple NVMe controllers and "exporting" them over fabrics as multiple subsystems. Signed-off-by: Simon A. F. Lund <simon.lund@samsung.com>
1 task
|
@safl I think that xnvme_dev_open does not work correctly, I'm trying to run |
|
@LiadOz you have to provide the --subnqn. Via api: opts.subnqn, via cli: "xnvme info --dev-nsid 1 --subnqn ..." |
|
I am having regression issues with this branch, but I am not able to find a small test to reproduce and it fails sporadically. Do you know if there are changes that might cause this to happen? |
karlowich
reviewed
Jul 18, 2022
| snprintf(trid_str, sizeof(trid_str), | ||
| "trtype:%s subnqn:%s adrfam:%s traddr:%s trsvcid:%d", | ||
| spdk_nvme_transport_id_trtype_str(trtype), | ||
| SPDK_NVMF_DISCOVERY_NQN, opts->adrfam ? opts->adrfam : "IPv4", |
There was a problem hiding this comment.
In my opinion this should be changed to say:
opts->subnqn ? opts->subnqn : SPDK_NVMF_DISCOVERY_NQN, ...
Otherwise the --subnqn flag for enum is pointless
| snprintf(trid_str, sizeof(trid_str), | ||
| "trtype:%s subnqn:%s adrfam:%s traddr:%s trsvcid:%d", | ||
| spdk_nvme_transport_id_trtype_str(trtype), | ||
| SPDK_NVMF_DISCOVERY_NQN, opts->adrfam ? opts->adrfam : "IPv4", |
LiadOz
reviewed
Aug 14, 2022
| opts = &_spdk_opts; | ||
| spdk_env_opts_init(&env_opts); | ||
| env_opts.name = "xnvme"; | ||
| env_opts.shm_id = 0; |
There was a problem hiding this comment.
This line should be removed, spdk_env_opts_init sets this to -1 which will configure dpdk with the flag --no-shconf. using shm_id=0 will setup multiprocess environment, you may need to move it to inside the if block
|
Closing this without merge since the individual changes have gone in without the refactoring. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is an integration of the following:
This, seem to solve the multi-subsystem on fabrics-endpoint issues:
Things to address before merging this:
fabrics_target_spdk.shit is currently not constructing the subsystem correctly, it is listening but when using it, either vianvme-cliorxnvmethen the second device is not showing up.