-
Notifications
You must be signed in to change notification settings - Fork 46
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
don't require address when adding/removing listener #17
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
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 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. |
Yes, this is what I had in mind. The only headache is who is going to release IDs when the gateway is gone.
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?
This is not really solved with the local config solution since you would need to update it accordingly, no? |
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?
Yes, the config was auto generated on the container start. |
Actually listening to |
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). |
Ok, so I am going to implement it in the following way:
If both If only If only If no Does it make sense? |
Makes sense to me. Maybe we can add one addition: if |
I am not sure what So for me Actually, my current implementation (see the code) for the case if gateway_addr is not specified (or
It might be not what users would want in all cases, but is simple and probably good enough? |
Did you use addr family IPv6 otherwise you need to use 0.0.0.0
I don't think manually going through all interfaces is ever a good idea because of the mentioned issues.
I'm not a big fan of this approach I had a fair share of headaches with this in the past. |
Indeed
In the tgt log:
Although I allowed any host:
|
091a56e
to
a67bc8a
Compare
Updated. I think it is ready for the next (or final) iteration of review. |
a67bc8a
to
00c7bce
Compare
@PepperJo thanks. updated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Mykola. Minor comment otherwise LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
695b2c6
to
e268864
Compare
@idryomov thanks, updated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but needs a rebase
when adding/deleting a listener: let SPDK handle this. Signed-off-by: Mykola Golub <mykola.golub@clyso.com>
Now you may create a gateway listener specifying the gateway name, address and port, or you may create a group listener specifying only the port. In the second case all gateways in the group will create a listener using `gateway_addr` config param as `traddr`. Fixes: ceph#13 Signed-off-by: Mykola Golub <mykola.golub@clyso.com>
e268864
to
727cecf
Compare
@idryomov thanks. rebased |
use the gateway address instead
Fixes: #13
Signed-off-by: Mykola Golub mykola.golub@clyso.com