don't require address when adding/removing listener

[trociny]

use the gateway address instead

Fixes: #13
Signed-off-by: Mykola Golub mykola.golub@clyso.com

[trociny]

@sskaur @PepperJo

[PepperJo]

I think the listener configuration should be more flexible i.e. one should be able to have different number of listeners per gateway node and use different ports and possibly transports even if they are in the same group. What do you think?

[trociny]

I don't have a strong opinion. I just thought that having the same listener configuration for all gateways would be enough and would simplify the configuration procedure for a user, and that it was the main idea of gateway groups: all gateways in a group behave the same. But it indeed it may be considered as a unnecessary limitation.

So I am also fine with your proposal, just would like to confirm we agree about the details:

I can imagine we would need to introduce "add(create) gateway" command, that would assign the gateway ID. Then in create|delete_listener commands, additionally to transport, address and port, we will need to specify the gateway ID, which will be used in the storage key, and a gateway would only use its own keys (listeners).

Is it what you had in your mind? Agree, it is much more flexible, though it will require more steps for a user, potentially executing almost the same command for every listener. And when a new gateway is added to a group, one will also need to create all listeners for this gateway.

I would also like to mention another use case, when gateways are running e.g. in k8s containers and the IP address is changed when a container is restarted. Not sure how useful this use case would be, I had it when preparing a demo, but in real life you probably would not want a gateway changing its IP after restart, so we might want to ignore this. Just mentioned for full picture.

[PepperJo]

I can imagine we would need to introduce "add(create) gateway" command, that would assign the gateway ID. Then in create|delete_listener commands, additionally to transport, address and port, we will need to specify the gateway ID, which will be used in the storage key, and a gateway would only use its own keys (listeners).

Yes, this is what I had in mind. The only headache is who is going to release IDs when the gateway is gone.

Agree, it is much more flexible, though it will require more steps for a user, potentially executing almost the same command for every listener. And when a new gateway is added to a group, one will also need to create all listeners for this gateway.

Maybe we could allow to create listeners without gw id but just the port and use 0.0.0.0 to listen on. If the user is not interested in a more complex listener setup?

I would also like to mention another use case, when gateways are running e.g. in k8s containers and the IP address is changed when a container is restarted. Not sure how useful this use case would be, I had it when preparing a demo, but in real life you probably would not want a gateway changing its IP after restart, so we might want to ignore this. Just mentioned for full picture.

This is not really solved with the local config solution since you would need to update it accordingly, no?
We can always leave the option to listen on 0.0.0.0

[trociny]

Maybe we could allow to create listeners without gw id but just the port and use 0.0.0.0 to listen on. If the user is not interested in a more complex listener setup?

LGTM. May be other people have different thoughts? One note though, for me (probably wrongly) "0.0.0.0" would mean listen all IPv4 addresses, and "[::]" all IPv6. And I suppose we want either "any" or just "the gateway" ip that is specified in the static config. The second would be simpler (as it is currently implemented in this PR). To implement "any" would mean listing all node interfaces/addresses and creating listeners for every of this. Do we want something like this?

This is not really solved with the local config solution since you would need to update it accordingly, no?

Yes, the config was auto generated on the container start.

[PepperJo]

LGTM. May be other people have different thoughts? One note though, for me (probably wrongly) "0.0.0.0" would mean listen all IPv4 addresses, and "[::]" all IPv6. And I suppose we want either "any" or just "the gateway" ip that is specified in the static config. The second would be simpler (as it is currently implemented in this PR). To implement "any" would mean listing all node interfaces/addresses and creating listeners for every of this. Do we want something like this?

Actually listening to :: should be enough, on Linux machines it includes ipv4, so you don't need to go through all node interfaces etc. I have no strong opinion also allowing to set a static gateway ip in the local config. The only downside is that the user might be confused with all the options.

[sdpeters]

LGTM. May be other people have different thoughts? One note though, for me (probably wrongly) "0.0.0.0" would mean listen all IPv4 addresses, and "[::]" all IPv6. And I suppose we want either "any" or just "the gateway" ip that is specified in the static config. The second would be simpler (as it is currently implemented in this PR). To implement "any" would mean listing all node interfaces/addresses and creating listeners for every of this. Do we want something like this?

Actually listening to :: should be enough, on Linux machines it includes ipv4, so you don't need to go through all node interfaces etc. I have no strong opinion also allowing to set a static gateway ip in the local config. The only downside is that the user might be confused with all the options.

I think we need the ability to specify the IP address of the gateway, but I can see why some users would rather not have to specify it. If that argument is optional it can default to "any".

It probably makes sense to take this in subnet form (a.b.c.d/s). That enables the user to put a gateway on a specific network, without knowing the gateway node's IP in that subnet. It also enables the gateway's exact IP to be specified (with /32, or by omitting /s). There could be a need to specify the exact IP when there are multiple gateways in the same node (as you might want in a two-socket dual-NIC OSD node when using ADNN), or if the user doesn't want the gateway to bind to all the gateway node's addresses in that subnet for some reason.

If this is agreeable, we can do it incrementally (e.g. make the IP parameter optional, and add subnet support later).

[trociny]

Ok, so I am going to implement it in the following way:

• add gateway_name param in the static config. If not specified the hostname is used. It is going to be used to identify the gateway when creating/removing a listener (and storing in the persistent config).
• create_listener will continue to have address (-a) option, but optional now, and additionally it will have gateway (-g) option.

If both -g and -a option are specified the listener is created on this gateway with the specified address.

If only -g option is specified the listener is created on this gateway with the address from the gateway's config (gateway_addr).

If only -a option is specified the error "unknown gateway" is returned.

If no -g and -a options are specified the listener is created on all gateways with the address specified in the gateway's config.

Does it make sense?

[PepperJo]

Ok, so I am going to implement it in the following way:

* add `gateway_name` param in the static config. If not specified the hostname is used. It is going to be used to identify the gateway when creating/removing a listener (and storing in the persistent config).

* `create_listener` will continue to have address (`-a`) option, but optional now, and additionally it will have gateway (`-g`) option.

If both -g and -a option are specified the listener is created on this gateway with the specified address.

If only -g option is specified the listener is created on this gateway with the address from the gateway's config (gateway_addr).

If only -a option is specified the error "unknown gateway" is returned.

If no -g and -a options are specified the listener is created on all gateways with the address specified in the gateway's config.

Does it make sense?

Makes sense to me. Maybe we can add one addition: if gateway_addr is not specified in the config listen on ::.

[trociny]

Maybe we can add one addition: if gateway_addr is not specified in the config listen on ::.

I am not sure what :: would mean? What should I send in the nvmf_subsystem_add_listener command to the nvmf_tgt socket as the address argument? :: does not work.

So for me :: would mean "create listeners on all available addresses". I.e. the gw server would need to list all host's interfaces and assigned addresses and call nvmf_subsystem_add_listener for every of them. I may do this. But the problem with this approach is that the interfaces and addresses may dynamically change (be added/removed) and the gatway would need to track this and update the nvmf_tgt accordingly.

Actually, my current implementation (see the code) for the case if gateway_addr is not specified (or :: is specified) is to use the IP for a hostname returned by gethostname:

hostname = socket.gethostname()
self.traddr = socket.gethostbyname(hostname)

It might be not what users would want in all cases, but is simple and probably good enough?

[PepperJo]

I am not sure what :: would mean? What should I send in the nvmf_subsystem_add_listener command to the nvmf_tgt socket as the address argument? :: does not work.

Did you use addr family IPv6 otherwise you need to use 0.0.0.0

So for me :: would mean "create listeners on all available addresses". I.e. the gw server would need to list all host's interfaces and assigned addresses and call nvmf_subsystem_add_listener for every of them. I may do this. But the problem with this approach is that the interfaces and addresses may dynamically change (be added/removed) and the gatway would need to track this and update the nvmf_tgt accordingly.

I don't think manually going through all interfaces is ever a good idea because of the mentioned issues.

Actually, my current implementation (see the code) for the case if gateway_addr is not specified (or :: is specified) is to use the IP for a hostname returned by gethostname:

I'm not a big fan of this approach I had a fair share of headaches with this in the past.

[trociny]

Did you use addr family IPv6 otherwise you need to use 0.0.0.0

Indeed nvmf_subsystem_add_listener -a 0.0.0.0 seems to work, though not sure how well without through testing. The immediate issues found:

nvme discover returns traddr: 0.0.0.0 (this actually may be not an issue, just does not look very useful, but what it could be?):

adonis:~/ceph/ceph-nvmeof.upstream% sudo nvme discover -t tcp -a 127.0.0.1 -s 4401                                        

Discovery Log Number of Records 1, Generation counter 6
=====Discovery Log Entry 0======
trtype:  tcp
adrfam:  ipv4
subtype: nvme subsystem
treq:    not required
portid:  0
trsvcid: 4401
subnqn:  nqn.2016-06.io.spdk:cnode1
traddr:  0.0.0.0
sectype: none

nvme connect fails:

adonis:~/ceph/ceph-nvmeof.upstream% sudo nvme connect -t tcp -n "nqn.2016-06.io.spdk:cnode1" -a 127.0.0.1 -s 4401
Failed to write to /dev/nvme-fabrics: Input/output error

In the tgt log:

[2022-05-25 12:38:06.765436] ctrlr.c: 680:nvmf_qpair_access_allowed: *ERROR*: Subsystem 'nqn.2016-06.io.spdk:cnode1' does not allow host 'nqn.2014-08.org.nvmexpress:uuid:00000000-0000-0000-0000-ac1f6b0c860a' to connect at this address.

Although I allowed any host:

adonis:~/ceph/ceph-nvmeof.upstream% sudo python3 ./spdk/scripts/rpc.py nvmf_subsystem_allow_any_host nqn.2016-06.io.spdk:cnode1 
adonis:~/ceph/ceph-nvmeof.upstream% sudo python3 ./spdk/scripts/rpc.py nvmf_get_subsystems                                      
[
  {
    "nqn": "nqn.2014-08.org.nvmexpress.discovery",
    "subtype": "Discovery",
    "listen_addresses": [],
    "allow_any_host": true,
    "hosts": []
  },
  {
    "nqn": "nqn.2016-06.io.spdk:cnode1",
    "subtype": "NVMe",
    "listen_addresses": [
      {
        "transport": "TCP",
        "trtype": "TCP",
        "adrfam": "IPv4",
        "traddr": "0.0.0.0",
        "trsvcid": "4401"
      }
    ],
    "allow_any_host": true,
    "hosts": [],
    "serial_number": "SPDK00000000000001",
    "model_number": "SPDK bdev Controller",
    "max_namespaces": 32,
    "min_cntlid": 1,
    "max_cntlid": 65519,
    "namespaces": [
      {
        "nsid": 1,
        "bdev_name": "test1",
        "name": "test1",
        "nguid": "65B171A5F192432BAE8FAFCCEA60920A",
        "uuid": "65b171a5-f192-432b-ae8f-afccea60920a"
      }
    ]
  }
]

[PepperJo]

Thanks for testing it. This seems to be a bug in SPDK.
Why don't we start with not allowing empty gateway_addr? And keep the rest as discussed. If others are in favor of using the hostname approach I'm also fine with it. @sskaur @idryomov ?

[trociny]

Updated. I think it is ready for the next (or final) iteration of review.

[trociny]

@PepperJo thanks. updated

[PepperJo]

Thanks Mykola. Minor comment otherwise LGTM

[PepperJo]

LGTM

[trociny]

@idryomov thanks, updated

[idryomov]

LGTM but needs a rebase

[trociny]

@idryomov thanks. rebased