Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create "ceph-nvmeof" CLI command to use nvmeof-cli #752

Merged
merged 4 commits into from
Jul 23, 2024

Conversation

VallariAg
Copy link
Member

@VallariAg VallariAg commented Jul 10, 2024

Add "ceph-nvmeof" cli command to package this as python cli tool instead of using via containers.

How to build it:

git clone https://github.com/ceph/ceph-nvmeof.git
cd ceph-nvmeof/
dnf install -y python3-pip
python3 -m venv venv
source venv/bin/activate
pip install -U pip setuptools
pip install pdm==2.7.4
pdm "sync" -v --no-isolation --no-self --no-editable
pdm run protoc
pdm build

pip install dist/ceph_nvmeof-1.2.15-py3-none-any.whl
ceph-nvmeof

Usage:

[root@singleESXiS1in ~]# pip3 install ceph-nvmeof
[root@singleESXiS1in ~]# ceph-nvmeof --server-address $NVMEOF_DEFAULT_GATEWAY_IP_ADDRESS --server-port $NVMEOF_SRPORT --format json subsystem list
{
    "error_message": "Success",
    "status": 0,
    "subsystems": []
}
[root@singleESXiS1in ~]# ceph-nvmeof  --server-address $NVMEOF_DEFAULT_GATEWAY_IP_ADDRESS --server-port $NVMEOF_SRPORT subsystem add --subsystem nqn.2016-06.io.spdk:cnode1
Adding subsystem nqn.2016-06.io.spdk:cnode1: Successful
[root@singleESXiS1in ~]# ceph-nvmeof --server-address $NVMEOF_DEFAULT_GATEWAY_IP_ADDRESS --server-port $NVMEOF_SRPORT --format json subsystem list
{
    "error_message": "Success",
    "subsystems": [
        {
            "nqn": "nqn.2016-06.io.spdk:cnode1",
            "enable_ha": true,
            "serial_number": "Ceph36100720827891",
            "model_number": "Ceph bdev Controller",
            "min_cntlid": 1,
            "max_cntlid": 2040,
            "subtype": "NVMe",
            "max_namespaces": 256,
            "namespace_count": 0
        }
    ],
    "status": 0
}

Package: https://pypi.org/project/ceph-nvmeof/

Copy link
Collaborator

@baum baum left a comment

Choose a reason for hiding this comment

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

lgtm 🖖

Add "ceph-nvmeof" cli command to use it as a CLI
instead of containers.

Signed-off-by: Vallari Agrawal <[email protected]>
@VallariAg VallariAg changed the title pyproject.toml: add "[project.scripts]" section Create "ceph-nvmeof" CLI command to use nvmeof-cli Jul 11, 2024
Copy link
Member

@epuertat epuertat left a comment

Choose a reason for hiding this comment

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

Great work, Vallari! Now I see the new CLI usage, it'd probably make sense to enable fetching the server address/port from a global env. var too (that will save a lot of repetitive input).

CEPH_NVMEOF_SERVER_ADDRESS=192.168.1.20
CEPH_NVMEOF_SERVER_PORT=5500

ceph-nvmeof gateway info

.github/workflows/testpypi.yml Outdated Show resolved Hide resolved
.github/workflows/release.yml Outdated Show resolved Hide resolved
@VallariAg
Copy link
Member Author

@epuertat I have merged the workflows into one - it looks much better now! Thank you!

Let me know if my solution of reading default server-address/port in control/cli.py is okay!

Copy link
Member

@epuertat epuertat left a comment

Choose a reason for hiding this comment

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

Great work, @VallariAg ! Thank you for addressing my comments.

Just left a few more comments that might improve the PR.

.github/workflows/release.yml Outdated Show resolved Hide resolved
.github/workflows/release.yml Outdated Show resolved Hide resolved
.github/workflows/release.yml Outdated Show resolved Hide resolved
control/cli.py Outdated Show resolved Hide resolved
control/cli.py Outdated Show resolved Hide resolved
Official releases would be published to
https://pypi.org/project/ceph-nvmeof/
on every tag releases on github.

Dev releases would be published to
https://test.pypi.org/project/ceph-nvmeof/
on every merge to "devel" branch.

Package will override if the version already exists.
For example, if "1.2.15" already exists on test.pypi
and the new commit also has same version (in pyproject.toml),
then that package would be overwritten (by using a bigger
build number).

Signed-off-by: Vallari Agrawal <[email protected]>
"--server-address" defaults to env variable
CEPH_NVMEOF_SERVER_ADDRESS (otherwise "localhost").

"--server-port" defaults to env variable
CEPH_NVMEOF_SERVER_PORT (otherwise 5500).

This is to avoid repetitive inputs. If these
environment variables are set, we can just do:
`ceph-nvmeof gw info`.
(instead of current `ceph-nvmeof --server-address /
$NVMEOF_SERVER_ADDRESS --server-port /
$NVMEOF_SERVER_PORT gw info`)

Signed-off-by: Vallari Agrawal <[email protected]>
Copy link
Member

@epuertat epuertat left a comment

Choose a reason for hiding this comment

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

LGMT! Great work @VallariAg !

@VallariAg VallariAg merged commit 4006d19 into ceph:devel Jul 23, 2024
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

3 participants