control/cli: add ip requirement for listener

[CongMinYin]

discovert controller need a specific ip/port.

fixes: #87

[CongMinYin]

Hi @trociny and @sdpeters , please take a look. It is for #127.

[trociny]

I think changing just CLI part is not enough. We need to make sure the gateway has this requirement too. Theoretically one may talk to the gateway directly, not using cli. On the cli side it is needed just to catch an invalid input earlier and provide more meaningful error message.

[trociny]

You also need to update tests/test_cli.py.

And although the gateway and the address are required on cli part, it seems like we don't have any check the gateway name is not empty on the server side and the server may store omap with empty gateway name, which will never be used. This may be out of scope of this PR though, because we also do not check that the gateway name is valid (belongs to an existing gateway), so we may store omap with invalid names, which will never be used too, and this does not differ much from storing omap with the empty name.

[CongMinYin]

You also need to update tests/test_cli.py.

updated

And although the gateway and the address are required on cli part, it seems like we don't have any check the gateway name is not empty on the server side and the server may store omap with empty gateway name, which will never be used. This may be out of scope of this PR though, because we also do not check that the gateway name is valid (belongs to an existing gateway), so we may store omap with invalid names, which will never be used too, and this does not differ much from storing omap with the empty name.

gateway name can not be empty on server side. If gateway name is empty in conf, it will be assigned a value of hostname.

ceph-nvmeof/control/grpc.py

Line 44 in e783513

self.gateway_name = socket.gethostname()

[trociny]

gateway name can not be empty on server side. If gateway name is empty in conf, it will be assigned a value of hostname.

I mean the empty gateway name in the request. If such a requests comes to the gateway it will store it in omap (although it will not be used).

[CongMinYin]

I mean the empty gateway name in the request. If such a requests comes to the gateway it will store it in omap (although it will not be used).

Get your point. So I moved the judgment of gateway_name to the beginning, please check if this handling is appropriate.

[CongMinYin]

But there is still a problem with this. The error information will be output to the server's gateway daemon, and the client can only receive an error message and cannot know why it went wrong. If this method is possible, is there a better way to report errors.

error on server side:

INFO:control.grpc:Received request to create ceph TCP listener for nqn.2016-06.io.spdk:cnode1 at 10.239.241.113:5007.
ERROR:control.grpc:gateway name mismatch.
ERROR:grpc._common:Exception serializing message!
Traceback (most recent call last):
  File "/usr/local/lib/python3.8/dist-packages/grpc/_common.py", line 90, in _transform
    return transformer(message)
TypeError: descriptor 'SerializeToString' for 'google._upb._message.Message' objects doesn't apply to a 'bool' object

client side:

oot@cephs:~/git/ceph-nvmeof# python3 -m control.cli create_listener -n nqn.2016-06.io.spdk:cnode1 -g ceph -a 10.239.241.113 -s 5007
ERROR:__main__:Failed to create listener: 
 <_InactiveRpcError of RPC that terminated with:
	status = StatusCode.INTERNAL
	details = "Failed to serialize response!"
	debug_error_string = "UNKNOWN:Error received from peer ipv4:10.239.241.113:5500 {grpc_message:"Failed to serialize response!", grpc_status:13, created_time:"2023-05-15T16:22:01.938484478+08:00"}"
>

[PepperJo]

Thank you @CongMinYin LGTM

[idryomov]

@CongMinYin This needs to be rebased.

[CongMinYin]

Hello everyone, rebased. could you please help check it.

[caroav]

@CongMinYin I believe that this PR can be merged. Why do you think that there is an issue? The testing pass. For a single GW I see no issues.

[CongMinYin]

@CongMinYin I believe that this PR can be merged. Why do you think that there is an issue? The testing pass. For a single GW I see no issues.

Hi, it's about automated demo testing considerations. I added a variable to define the hostname (=gateway name) of the test server, which I get by checking the logs.
NVMEOF_HOSTNAME="nvmeof"
Now it is correct. If you agree to merge, I have no objection. But if the testing environment changes in the future, such as changing to a new testing server and hostname changed. Demo testing may encounter problems. It is best to get the hostname/gateway name through commands or define the gateway name in configure file.

[baum]

... Now it is correct. ... if the testing environment changes in the future, such as changing to a new testing server and hostname changed. Demo testing may encounter problems. It is best to get the hostname/gateway name through commands or define the gateway name in configure file.

@CongMinYin how are you? It is reasonable to assume that the tests would need to be updated as the codebase evolves, to keep them in sync. WDYT?

[idryomov]

But if the testing environment changes in the future, such as changing to a new testing server and hostname changed. Demo testing may encounter problems.

It's not about the testing server but about what socket.gethostname() returns in the container.

It would be more reliable to define gateway names in ceph-nvmeof.conf file but everyone seems to be in agreement that that can be deferred until the demo shows off multiple gateways (or something breaks).

[caroav]

I think its OK to merge it @CongMinYin. I think that in general we might have another issue as discussed in #154, but this is not related to this PR.

[CongMinYin]

Make sense. Thank you all. At this stage, there is no need to dwell on this small detail. We can consider this when testing changes in the future. @trociny also discussed the #154 with me in the previous part of this PR, and I will pay attention to it. thanks.