CA-282006: Prevent IP reconfiguration on a live cluster network #3500
Conversation
minishrink
force-pushed
the
feature/REQ477/CA-282006
branch
from
54c3e8f to
4f3a76f
Compare
ocaml/xapi/xapi_pif.ml
Outdated
| (* return ref of newly created pif record *) | ||
| pif | ||
|
|
||
| let assert_no_clustering_enabled ~__context ~network = |
There was a problem hiding this comment.
You moved this function twice in this PR, could you put it in the correct place in the first commit please?
| ~params: [ Ref _pif, "self", "Reference to the object" | ||
| ; Bool, "value", "New value to set" ] | ||
| ~allowed_roles:_R_POOL_OP | ||
| ~errs:[Api_errors.clustering_enabled_on_network] |
There was a problem hiding this comment.
Looks like we are missing errs declarations on all the cluster APIs too. It would be nice if these errors were checked at compile time by the type system (e.g. by using a Result type) but that is probably a huge amount of work. For now can you open an internal minor ticket to fix the API error declarations on the cluster APIs?
| ~expr:Db_filter_types.(Eq(Field "network", Literal (Ref.string_of network)))) | ||
| |> function | ||
| | [] -> () | ||
| | _::_ -> raise Api_errors.(Server_error (clustering_enabled_on_network, [Ref.string_of network])) |
There was a problem hiding this comment.
We still want to allow IP address changes when in maintenance mode though, where clustering is temporarily disabled on a host. I think this function also needs to take a host parameter, and then use Xapi_clustering.is_clustering_disabled_on_host. If clustering is disabled allow the ip to be changed, otherwise forbid it.
You can test this with XenCenter by putting one host into maintenance mode, and then trying to change the IP. XenCenter should internally disable the cluster host, change IP, enable it again.
Or if you want to do it from the cli you can xe cluster-host-disable.
There was a problem hiding this comment.
So we should only check the network for clusters if clustering is enabled on the host?
minishrink
force-pushed
the
feature/REQ477/CA-282006
branch
from
4f3a76f to
87715b8
Compare
There was a problem hiding this comment.
Thanks the code looks ok now, one minor comment about the location of the tests.
Could you also please squash the last 2 commits into the earlier commits that they fix? (btw you can use git commit --fixup <commitid> when making the change so its easier to remember later which commits have to be squashed where.
ocaml/quicktest/quicktest.ml
Outdated
| finally | ||
| (fun () -> | ||
| (try | ||
| maybe_run_test "reconfigure-ip-cluster" (fun () -> Quicktest_ca282006.test s); |
There was a problem hiding this comment.
Could this go in the same file as the other clustering tests?
There was a problem hiding this comment.
The quicktests compile to a different target than the unit tests. I think it might be doable with a bit of jbuild fiddling, but I'm not sure how well it would work.
Alternatively, if you think more quicktests might be necessary, there could just be a quicktest_cluster.ml so we don't just have a random quicktest for one ticket.
There was a problem hiding this comment.
quicktest_cluster.ml would be fine, you are right they are different from unit tests, and this is the first clustering quicktest.
minishrink
force-pushed
the
feature/REQ477/CA-282006
branch
from
4ebe276 to
01d8046
Compare
Refactor pif_has_clusters into assert_no_clustering_enabled, add assertion to PIF.forget and PIF.reconfigure_ip(v4,v6) CA-282006: Add host check to assert_no_clustering_enabled Now assertion only checks for clusters attached to network if clustering is enabled on the host, otherwise this check suffices. Signed-off-by: Akanksha Mathur <akanksha.mathur@citrix.com> Conflicts: ocaml/xapi/xapi_pif.ml
… datamodel with new errors Signed-off-by: Akanksha Mathur <akanksha.mathur@citrix.com>
…configuring CA-282006: Skip tests if no clusters found, don't fail on all API errors - Remove logic to check for clusters - Don't test PIF.reconfigure_IP without clustering - Minor edits to debug lines and comments Signed-off-by: Akanksha Mathur <akanksha.mathur@citrix.com> CA-282006: Rename test to quicktest_cluster Signed-off-by: Akanksha Mathur <akanksha.mathur@citrix.com>
minishrink
force-pushed
the
feature/REQ477/CA-282006
branch
from
01d8046 to
65e6a90
Compare
CA-282006 raises the issue of host fencing when trying to reconfigure IP on a live cluster network. This PR asserts no clustering is enabled on the chosen network for calls such as
PIF.reconfigure_IP(v6). andPIF.forget, as well as adding the relevant errors to the datamodel for these calls.