CA-280423: Open/close xapi-clusterd port when starting/stopping daemon #3509
Conversation
ocaml/xapi/xapi_clustering.ml
Outdated
| let xapi_clusterd_port = "8896" | ||
| let enable ~__context = | ||
| debug "Enabling and starting the clustering daemon"; | ||
| maybe_call_script ~__context "/etc/xapi.d/plugins/firewall-port" ["open"; xapi_clusterd_port]; |
There was a problem hiding this comment.
This path, and the other strings in here, should be defined in xapi_globs.ml, such that they can be set from the config file.
There was a problem hiding this comment.
@robhoes Just to make sure I understand properly we should move the path, the port number and the "open"/"close" strings to xapi_globs.ml?
And then just call them here with Xapi_globs.xapi_clusterd_port?
There was a problem hiding this comment.
I think that you can leave "open"/"close", and just do the path and port. Grep for e.g. nbd_firewall_config_script in xapi_globs.ml to see how to refine them and register them with the config file logic.
There was a problem hiding this comment.
Should I actually update xapi_conf to include the port? looks like a look of ports are included but commented out. Thats what I've done atm.
There was a problem hiding this comment.
That's handy yes. How about adding the other one as well?
There was a problem hiding this comment.
Btw, they are commented out, with the default value filled in, as examples in the config file. That is common practice for such files. So you've done the right thing.
There was a problem hiding this comment.
I didn't see any of the other paths included in there but I can include the one I've added anyways. Seeing as it is commented out there is no harm.
ocaml/xapi/xapi_clustering.ml
Outdated
| let service = "xapi-clusterd" | ||
| let enable ~__context = | ||
| debug "Enabling and starting the clustering daemon"; | ||
| maybe_call_script ~__context !Xapi_globs.firewall_port_config_script ["open"; (string_of_int !Xapi_globs.xapi_clusterd_port)]; |
There was a problem hiding this comment.
I think using let would make this more readable and avoids overlong lines:
let enable ~__context =
let port = string_of_int !Xapi_globs.xapi_clusterd_port in
...
maybe_call_script ~__context !Xapi_globs.firewall_port_config_script ["open"; port];
There was a problem hiding this comment.
Sounds reasonable, I will amend it.
edwintorok
force-pushed
the
feature/REQ477/master
branch
from
d088ba1 to
350db35
Compare
Signed-off-by: Thomas Mckelvey <thomas.mckelvey@citrix.com>
Signed-off-by: Thomas Mckelvey <thomas.mckelvey@citrix.com>
Signed-off-by: Thomas Mckelvey <thomas.mckelvey@citrix.com>
|
Looks like a rebase went wrong on this branch: the PR now contains a bunch of unrelated commits. |
thomasmck
force-pushed
the
private/thomasmc/firewall
branch
from
b3ea901 to
5825027
Compare
|
Removed the other commits that got pulled in and made the other suggested changes. |
ocaml/xapi/xapi_clustering.ml
Outdated
| | None -> ignore (Helpers.call_script script params) | ||
|
|
||
| let service = "xapi-clusterd" | ||
| let port = (string_of_int !Xapi_globs.xapi_clusterd_port) |
There was a problem hiding this comment.
Careful: the port is defined by xapi startup and any changes to !Xapi_globs.xapi_clusterd_port over the lifetime of xapi will not be taken into account. You might want to put this inside enable() and disable() to read it again at the time of usage. You might even miss any changes from the command line because it is read before the changes take effect.
There was a problem hiding this comment.
Indeed, port may evaluate before the config file read takes place.
Signed-off-by: Thomas Mckelvey <thomas.mckelvey@citrix.com>
| # xapi_clusterd_port = 8896 | ||
|
|
||
| # Path for the firewall-port script | ||
| # firewall_port_config_script = /etc/xapi.d/plugins/firewall-port |
Signed-off-by: Thomas Mckelvey <thomas.mckelvey@citrix.com>
|
Ok I think all comments have now been addressed. Let me know if there is anything I've missed. |
|
I agree. Github has the ability to do that as part of the merge right? I do I need to do it myself and re-push? |
#3509) * CA-280423: Open/close xapi-clusterd port when starting/stopping daemon Signed-off-by: Thomas Mckelvey <thomas.mckelvey@citrix.com>
xapi-project#3509) * CA-280423: Open/close xapi-clusterd port when starting/stopping daemon Signed-off-by: Thomas Mckelvey <thomas.mckelvey@citrix.com>
Signed-off-by: Thomas Mckelvey thomas.mckelvey@citrix.com