Initial commit for the NVMeoF gateway to Ceph management daemon #3

anitashekar · 2021-09-16T23:58:48Z

No description provided.

trociny

So, it has been decided to use gRPC for client side communication instead of REST API?

I seemed to miss this discussion. Could you please tell about the reason? I suppose the REST API would be much easier for integration with third parties and users would like it more.

And I see that the current version of CLI just mimics the commands of nvmf_tgt. I was thinking it would provide a more abstract interface so it would be possible to manage all gateways in the group, not only one gateway. Though I guess it is going to be changed in the future?

And note, my comments are not objection for merging it. It is more for discussion. At the current project stage I would prefer a flow when we merge quickly, leaving the bits that need more discussion for further PRs.

proto/nvme_gw.proto

nvme_gw_server.py

README.md

anitashekar · 2021-09-20T18:47:11Z

So, it has been decided to use gRPC for client side communication instead of REST API?

I seemed to miss this discussion. Could you please tell about the reason? I suppose the REST API would be much easier for integration with third parties and users would like it more.

And I see that the current version of CLI just mimics the commands of nvmf_tgt. I was thinking it would provide a more abstract interface so it would be possible to manage all gateways in the group, not only one gateway. Though I guess it is going to be changed in the future?

And note, my comments are not objection for merging it. It is more for discussion. At the current project stage I would prefer a flow when we merge quickly, leaving the bits that need more discussion for further PRs.

Thank you for reviewing the PR, Mykola.

We had a discussion at the start of the project and based on that:

Why gRPC:

gRPC is attractive because it enforces strong versioning which will make it safer to make changes in the API in the future without accidentally causing unforeseen results
It is more performant. That may not be a huge impact initially, but will help when we scale up

And yes, the roadmap set a single gateway configuration as the 1st step and that's what you see reflected in the CLI. This will be changed in the future so that the CLI will talk to all gateways in the gateway group

Here is the etherpad that has some history of the discussions we have been having and the design choices based on the discussions. It will be great to see you at the bluejeans meetings on Tuesdays at 11AM US Central time for ongoing discussions.

https://pad.ceph.com/p/rbd_nvmeof
https://pad.ceph.com/p/rbd_nvmeof_management_layer

trociny

Otherwise LGTM (Reviewed by: Mykola Golub mykola.golub@clyso.com)

I am still not very happy about the decision to use gRPC instead of REST API, but this should not be a stopper for this PR to be merged.

nvme_gw_cli.py

nvme_gw_server.py

trociny

Just a couple notes after trying this.

nvme_gw.config

COPYING

nvme_gw_config.py

nvme_gw_cli.py

README.md

COPYING

nvme_gw_server.py

README.md

idryomov · 2021-10-11T08:32:33Z

Please squash "addressed review" commits into the initial commit and add a Signed-off-by.

idryomov · 2021-10-17T19:51:48Z

README.md

+		gateway_addr = <IP address at which the client can reach the gateway>
+		gateway_port = <port at which the client can reach the gateway>
+
+2. To [enable mTLS](#mtls-configuration-for-testing-purposes) using self signed certificates, edit the config file to set:


Nit: numbering is off here. I know it doesn't matter for rendering but some people use plain text editors ;)

idryomov · 2021-10-17T19:52:29Z

README.md

+
+		spdk_path = <complete path to SPDK parent directory>
+		spdk_tgt = <relative path to SPDK target executable>
+5. Setup SPDK


add an empty line above

idryomov · 2021-10-17T19:57:41Z

README.md

+
+This daemon runs as root. It provides the ability to export existing RBD images as NVMeoF namespaces. Creation of RBD images is not within the scope of this daemon.
+
+# Initial configuration:


Suggested change

# Initial configuration:

# Initial configuration

Note, in Ilya's suggested change it is without the colon symbol.

idryomov · 2021-10-17T19:58:43Z

README.md

+
+# CLI Usage
+
+The CLI code can be used to initiate a connection to the gateway and run commands to configure the NVMe targets. 


Suggested change

The CLI code can be used to initiate a connection to the gateway and run commands to configure the NVMe targets.

The CLI tool can be used to initiate a connection to the gateway and run commands to configure the NVMe targets.

idryomov · 2021-10-17T19:58:53Z

README.md

+
+The CLI code can be used to initiate a connection to the gateway and run commands to configure the NVMe targets. 
+
+Run the code with the -h flag to see a list of available commands:


Suggested change

Run the code with the -h flag to see a list of available commands:

Run the tool with the -h flag to see a list of available commands:

idryomov · 2021-10-17T20:22:17Z

nvme_gw_cli.py

+    @cli.cmd([
+        argument("-i", "--image", help="RBD image name", required=True),
+        argument("-p", "--pool", help="Ceph pool name", required=True),
+        argument("-b", "--bdev_name", help="Bdev name"),


need to be consistent with create_namespace

Suggested change

argument("-b", "--bdev_name", help="Bdev name"),

argument("-b", "--bdev", help="Bdev name"),

idryomov · 2021-10-17T20:27:54Z

nvme_gw_server.py

+        # cmd = [spdk_cmd]
+        # cmd += ["all"]
+        # cmd += ["-u"]


Suggested change

# cmd = [spdk_cmd]

# cmd += ["all"]

# cmd += ["-u"]

idryomov · 2021-10-17T20:33:00Z

nvme_gw_server.py

+import subprocess
+import grpc
+from concurrent import futures
+from distutils import util


unused import

idryomov · 2021-10-17T20:37:41Z

README.md

+
+		enable_auth = True  # Setting this to False will open an insecure port		
+
+3. Compiling protobuf files for grpc:


Suggested change

3. Compiling protobuf files for grpc:

3. Compile protobuf files for gRPC:

idryomov · 2021-10-17T20:37:57Z

README.md

+
+		$ ./scripts/pkgdep.sh
+
+	Intialize configuration:


Suggested change

Intialize configuration:

Initialize configuration:

idryomov · 2021-10-17T20:49:43Z

README.md

+
+		$ python3 nvme_gw_server.py
+		INFO:root:SPDK PATH: /path/to/spdk
+		INFO:root:Starting /path.to/spdk/tgt/nvmf_tgt all -u


Suggested change

INFO:root:Starting /path.to/spdk/tgt/nvmf_tgt all -u

INFO:root:Starting /path/to/spdk/tgt/nvmf_tgt all -u

idryomov · 2021-10-17T20:53:48Z

Note that there are few unresolved nits from the previous review -- GitHub unhelpfully hides them under "X hidden conversations".

trociny

And as I understand not all Ilya's comments are addressed yet?

trociny · 2021-10-27T10:43:22Z

README.md

+
+This daemon runs as root. It provides the ability to export existing RBD images as NVMeoF namespaces. Creation of RBD images is not within the scope of this daemon.
+
+# Initial configuration:


Note, in Ilya's suggested change it is without the colon symbol.

trociny

Otherwise LGTM. When you address these and squash I am going to merge it.

README.md

This daemon sets up access to Ceph storage over NVMeoF Signed-off-by: Anita Shekar <anita.shekar@ibm.com>

trociny reviewed Sep 19, 2021

View reviewed changes

proto/nvme_gw.proto Show resolved Hide resolved

nvme_gw_server.py Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

trociny approved these changes Sep 23, 2021

View reviewed changes

nvme_gw_cli.py Outdated Show resolved Hide resolved

nvme_gw_cli.py Outdated Show resolved Hide resolved

nvme_gw_cli.py Outdated Show resolved Hide resolved

nvme_gw_server.py Outdated Show resolved Hide resolved

PepperJo approved these changes Sep 27, 2021

View reviewed changes

trociny reviewed Sep 27, 2021

View reviewed changes

nvme_gw.config Outdated Show resolved Hide resolved

nvme_gw.config Outdated Show resolved Hide resolved

nvme_gw.config Outdated Show resolved Hide resolved

idryomov reviewed Oct 11, 2021

View reviewed changes

idryomov reviewed Oct 17, 2021

View reviewed changes

trociny reviewed Oct 27, 2021

View reviewed changes

anitashekar force-pushed the devel branch from 83c466a to 159c5ff Compare October 27, 2021 16:46

trociny approved these changes Oct 29, 2021

View reviewed changes

README.md Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

Initial commit for the NVMeoF gateway to Ceph management daemon

a293095

This daemon sets up access to Ceph storage over NVMeoF Signed-off-by: Anita Shekar <anita.shekar@ibm.com>

anitashekar force-pushed the devel branch from 3c5618c to a293095 Compare November 2, 2021 16:51

trociny merged commit a3f8d5a into ceph:devel Nov 3, 2021

sskaur mentioned this pull request Apr 4, 2022

cli: implicitly create transports #12

Closed

epuertat added this to the Milestone 1 milestone Nov 18, 2022

leonidc mentioned this pull request Aug 7, 2023

GW server logs notice on keep alive timeout and disconnects host from subsystem #161

Closed

sarwanjassi mentioned this pull request Nov 3, 2023

installation is failed #308

Closed

barakda pushed a commit to barakda/ceph-nvmeof that referenced this pull request Feb 19, 2024

change ceph#3

43181bd

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial commit for the NVMeoF gateway to Ceph management daemon #3

Initial commit for the NVMeoF gateway to Ceph management daemon #3

anitashekar commented Sep 16, 2021

trociny left a comment

anitashekar commented Sep 20, 2021

trociny left a comment •

edited

Loading

trociny left a comment

idryomov commented Oct 11, 2021

idryomov Oct 17, 2021

idryomov Oct 17, 2021

idryomov Oct 17, 2021

trociny Oct 27, 2021

idryomov Oct 17, 2021

idryomov Oct 17, 2021

idryomov Oct 17, 2021

idryomov Oct 17, 2021

idryomov Oct 17, 2021

idryomov Oct 17, 2021

idryomov Oct 17, 2021

idryomov Oct 17, 2021

idryomov commented Oct 17, 2021

trociny left a comment

trociny Oct 27, 2021

trociny left a comment


		This daemon runs as root. It provides the ability to export existing RBD images as NVMeoF namespaces. Creation of RBD images is not within the scope of this daemon.

		# Initial configuration:


		# CLI Usage

		The CLI code can be used to initiate a connection to the gateway and run commands to configure the NVMe targets.

	The CLI code can be used to initiate a connection to the gateway and run commands to configure the NVMe targets.
	The CLI tool can be used to initiate a connection to the gateway and run commands to configure the NVMe targets.


		The CLI code can be used to initiate a connection to the gateway and run commands to configure the NVMe targets.

		Run the code with the -h flag to see a list of available commands:

	Run the code with the -h flag to see a list of available commands:
	Run the tool with the -h flag to see a list of available commands:

	argument("-b", "--bdev_name", help="Bdev name"),
	argument("-b", "--bdev", help="Bdev name"),


		enable_auth = True # Setting this to False will open an insecure port

		3. Compiling protobuf files for grpc:

	3. Compiling protobuf files for grpc:
	3. Compile protobuf files for gRPC:

	INFO:root:Starting /path.to/spdk/tgt/nvmf_tgt all -u
	INFO:root:Starting /path/to/spdk/tgt/nvmf_tgt all -u

Initial commit for the NVMeoF gateway to Ceph management daemon #3

Initial commit for the NVMeoF gateway to Ceph management daemon #3

Conversation

anitashekar commented Sep 16, 2021

trociny left a comment

Choose a reason for hiding this comment

anitashekar commented Sep 20, 2021

trociny left a comment • edited Loading

Choose a reason for hiding this comment

trociny left a comment

Choose a reason for hiding this comment

idryomov commented Oct 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

idryomov commented Oct 17, 2021

trociny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trociny left a comment

Choose a reason for hiding this comment

trociny left a comment •

edited

Loading