nvme_gw_server: allow to change rpc socket address #4
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
|
@anitashekar @PepperJo please review |
anitashekar
reviewed
Nov 8, 2021
| @@ -25,8 +25,7 @@ client_cert = ./client.crt | |||
|
|
|||
| [spdk] | |||
|
|
|||
| spdk_server_addr = /var/tmp/spdk.sock | |||
| spdk_port = 5260 | |||
| rpc_socket = /var/tmp/spdk.sock | |||
There was a problem hiding this comment.
I'd prefer this to be called spdk_socket
There was a problem hiding this comment.
As it is stated in the commit log message, I don't see much sens to use spdk_ prefix for parameters in [spdk] section.
anitashekar
reviewed
Nov 8, 2021
nvme_gw_server.py
Outdated
| @@ -35,8 +35,9 @@ def start_spdk(self): | |||
| self.spdk_rpc = spdk_rpc | |||
| spdk_tgt = self.nvme_config.get("config", "spdk_tgt") | |||
| spdk_cmd = os.path.join(spdk_path, spdk_tgt) | |||
| rpc_socket = self.nvme_config.get("spdk", "rpc_socket") | |||
There was a problem hiding this comment.
Similar comment here & the rest of the places - spdk_rpc_socket or spdk_socket seems more appropriate to me
Previously it was supposed that you could change the address by using spdk_server_addr and spdk_port configuration parameters. But the parameters were used only by rcp client, while the spdk target was always started with the default socket address. This commit fixes it. Additionally, there is no sense to provide the port configuration option because the target is always listening on a unix socket only. So the two parameters spdk_server_addr and spdk_port are replaced with one rpc_socket. There is not much sense to add "spdk_" prefix to the option name, because it is in [spdk] section. And while here remove unused "all" argument in nvme_tgt command, which seemed to be unintentionally committed after testing with "-L all". Signed-off-by: Mykola Golub <mykola.golub@clyso.com>
trociny
force-pushed
the
wip-rpc_socket
branch
from
68de071
to
aaaa51d
Compare
|
@anitashekar I addressed one of your comments. Still for another (about naming the config param Do you agree with the current version or do you still prefer |
barakda
added a commit
to barakda/ceph-nvmeof
that referenced
this pull request
Feb 19, 2024
barakda
added a commit
to barakda/ceph-nvmeof
that referenced
this pull request
Feb 19, 2024
barakda
added a commit
to barakda/ceph-nvmeof
that referenced
this pull request
Feb 19, 2024
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.
Previously it was supposed that you could change the address by using
spdk_server_addr and spdk_port configuration parameters. But the
parameters were used only by rcp client, while the spdk target was
always started with the default socket address. This commit fixes it.
Additionally, there is no sense to provide the port configuration
option because the target is always listening on a unix socket
only. So the two parameters spdk_server_addr and spdk_port are
replaced with one rpc_socket. There is not much sense to add "spdk_"
prefix to the option name, because it is in [spdk] section.
And while here remove unused "all" argument in nvme_tgt command, which
seemed to be unintentionally committed after testing with "-L all".
Signed-off-by: Mykola Golub mykola.golub@clyso.com