CA-268511: Make PIF.disallow_unplug RO when clustering is enabled #3485

minishrink · 2018-03-05T10:41:27Z

This PR makes the following changes:

PIF.disallow_unplug is now dynamic RO
PIF.set_disallow_unplug throws cannot_change_pif_properties when a cluster object is attached to the network
adds two tests to test_clustering to check the behaviour of PIF.set_disallow_unplug with and without clustering

Questions for reviewers about possible changes:

should it raise a different exception if clustering is enabled?
is the try-with unnecessary for getting PIF.network
should the logging use info or debug?

gaborigloi · 2018-03-05T10:49:03Z

ocaml/idl/datamodel.ml

             Api_errors.pif_has_fcoe_sr_in_use]
      ()

+  let pif_set_disallow_unplug = call


We don't need to prefix this with pif_ as it's in already the PIF module - this seems to be the convention in this module.

minishrink · 2018-03-05T10:56:26Z

ocaml/idl/datamodel.ml

+             ; Bool, "value", "New value to set" ]
+    ~allowed_roles:_R_POOL_OP
+    ()
+


I assume this allowed role is fine?

If that was the original role than yes.

I've looked at the definitions of let create_obj and let field, and it seems that the field setter role will default to ~messages_default_allowed_roles, if I'm not mistaken, which is this, so this is perfect.

coveralls · 2018-03-05T11:38:50Z

Coverage increased (+0.04%) to 18.66% when pulling 5252972 on minishrink:feature/REQ477/CA-268511 into 7927595 on xapi-project:feature/REQ477/master.

gaborigloi · 2018-03-05T11:51:19Z

ocaml/tests/test_clustering.ml

+let make_host_network_pif ~__context =
+  let host    = Test_common.make_host    ~__context () in
+  let network = Test_common.make_network ~__context () in
+  let pif     = Test_common.make_pif     ~__context ~network ~host () in


We prefer not to align things because then we may have to change all the lines every time we add a new one.
This is one of the reasons we start list items with the semicolon: this way we only have to change one line when remove an item: http://elm-lang.org/docs/style-guide (and this also makes the diffs cleaner)

I think this would be a good addition to https://github.com/lindig/ocaml-style

Indeed, what do you think @lindig ? I think it'd be good to mention that we shouldn't worry too much about aligning things but should keep it simple and practical, to minimise the effort when viewing diffs or changing code or rebasing.

gaborigloi · 2018-03-05T12:00:52Z

ocaml/xapi/xapi_pif.ml

+      failwith (Printf.sprintf "Could not find network of PIF: %s" (Ref.string_of self))
+  in
+  info "Got network: %s" (Ref.string_of network_of_pif);
+  (Db.Cluster.get_records_where ~__context


We can use get_refs_where here - we don't need to obtain the whole record.

gaborigloi · 2018-03-05T12:01:24Z

ocaml/xapi/message_forwarding.ml

+      info "PIF.set_disallow_unplug: PIF: %s; value = %s" (Ref.string_of self) (string_of_bool value);
+      let host = Db.PIF.get_host ~__context ~self in
+      let local_fn = Local.PIF.set_disallow_unplug ~self ~value in
+      do_op_on ~local_fn ~__context ~host (fun session_id rpc -> Client.PIF.set_disallow_unplug rpc session_id self value)


I think we should not forward it but just do it locally on the master which has the DB, since we only do DB queries. (Like for vdi_set_cbt_enabled)

Yes, if the function acts on the xapi DB only, than we should just call Local... directly.

gaborigloi · 2018-03-05T13:36:17Z

ocaml/xapi/xapi_pif.ml

+
+let set_disallow_unplug ~__context ~self ~value =
+  assert_no_clustering_enabled ~__context ~self;
+  if Db.PIF.get_disallow_unplug ~__context ~self <> value


I think this idempotency is not necessary here, as there is nothing in the other if branch

Indeed you don't save very much by this if-statement. In fact, the common case would be that the function takes 2 DB calls rather than just one. However, this is not such a big deal if this only runs on the master.

gaborigloi · 2018-03-05T13:41:08Z

ocaml/xapi/xapi_pif.ml

+
+let assert_no_clustering_enabled ~__context ~self =
+  if pif_has_clusters ~__context self
+  then raise Api_errors.(Server_error(Api_errors.cannot_change_pif_properties, [Ref.string_of self]))


One issue with this exception is that its description currently talks about bonded PIFs (in datamodel.ml)

Let's introduce a new API error that is specific to clustering.

Minor: you can remove the second mention of Api_errors..

gaborigloi

Nice, well-tested - left some minor comments.
We'll have to decide about the API error we raise, that'll require another reviewer.

robhoes · 2018-03-05T14:16:38Z

ocaml/xapi/xapi_pif.ml

+  assert_no_clustering_enabled ~__context ~self;
+  if Db.PIF.get_disallow_unplug ~__context ~self <> value
+  then begin
+    info "PIF.set_disallow_unplug: PIF: %s, value: %s" (Ref.string_of self) (string_of_bool value);


I would remove this debug line, since it's almost the same as the one in message_forwarding.

Also when you add a debug message it would be nice if it printed UUIDs instead of OpaqueRefs, it makes debugging easier with xe. I've fixed that in a few places in the clustering code.
Don't fix it on this one since you're dropping it anyway, but in the future.

I think that's not a huge problem if we have the database dump, because then we can match UUIDs to opaquerefs. Also, in some debug messages both the UUID and ref are shown, for VDIs at least.

robhoes · 2018-03-05T14:23:35Z

ocaml/xapi/xapi_pif.ml

+let pif_has_clusters ~__context (self : API.ref_PIF) =
+  debug "Looking up clusters for PIF: %s" (Ref.string_of self);
+  let network_of_pif =
+    try Db.PIF.get_network ~__context ~self with _ ->


If PIF has a network by construction, and will be GC'ed if the network is deleted. So this is a little overly defensive.

robhoes · 2018-03-05T14:24:35Z

ocaml/xapi/xapi_pif.ml

+    try Db.PIF.get_network ~__context ~self with _ ->
+      failwith (Printf.sprintf "Could not find network of PIF: %s" (Ref.string_of self))
+  in
+  info "Got network: %s" (Ref.string_of network_of_pif);


I would remove the logging in this function. We are logging far too much already...

robhoes · 2018-03-05T14:26:40Z

ocaml/xapi/xapi_pif.ml

+  in
+  info "Got network: %s" (Ref.string_of network_of_pif);
+  (Db.Cluster.get_records_where ~__context
+    ~expr:Db_filter_types.(Eq(Literal (Ref.string_of network_of_pif),Field "network")))


This actually works? I always see Field first and then Literal...

robhoes

Added some comments inline.

gaborigloi · 2018-03-06T14:05:53Z

ocaml/xapi/message_forwarding.ml


+    let set_disallow_unplug ~__context ~self ~value =
+      let pif_uuid = Db.PIF.get_uuid ~__context ~self in
+      info "PIF.set_disallow_unplug: PIF uuid = %s; value = %s" (pif_uuid) (string_of_bool value);


You can use the existing pif_uuid helper: https://github.com/xapi-project/xen-api/pull/3485/files#diff-9a639b670fb7e2432894cc91df8b27fbR3101

gaborigloi · 2018-03-06T14:07:23Z

ocaml/xapi/xapi_pif.ml

+
+let set_disallow_unplug ~__context ~self ~value =
+  if pif_has_clustering_enabled ~__context self
+  then raise Api_errors.(Server_error(cannot_change_pif_properties, [Ref.string_of self]))


As we Rob said, we should introduce a new API error that is specific to clustering.

gaborigloi · 2018-03-06T14:08:56Z

ocaml/xapi/xapi_pif.ml

+let pif_has_clustering_enabled ~__context (self : API.ref_PIF) =
+  let network_ref = Db.PIF.get_network ~__context ~self |> Ref.string_of in
+  (Db.Cluster.get_refs_where ~__context
+     ~expr:Db_filter_types.(Eq(Literal network_ref,Field "network")))


As discussed, maybe it's safer to put Field first and then Literal to follow the existing conventions.

gaborigloi

Nice, I personally really like the new XenAPI error, as it's general and therefore reusable, but it's best if others also review it.

I think all the review comments have been addressed.

edwintorok

One minor comment on checking 'set disallow unplug' with false in the tests too since that is what we actually want to prevent.
Before merging don't forget to squash the commits.
@robhoes are there further changes needed?

edwintorok · 2018-03-07T12:27:01Z

ocaml/tests/test_clustering.ml

+  Alcotest.check_raises
+    "check_disallow_unplug called by test_disallow_unplug_with_clustering after attaching cluster and cluster_host to network"
+    (Api_errors.(Server_error(clustering_enabled_on_network, [Ref.string_of network])))
+    (fun () -> Xapi_pif.set_disallow_unplug ~__context ~self:pif ~value:true)


You might want to add a second check that setting disallow_unplug to false raises as well. In the current implementation it is obvious it will, but the real goal is that we reject setting it to false.

robhoes · 2018-03-07T13:30:08Z

ocaml/xapi/xapi_pif.ml

+let set_disallow_unplug ~__context ~self ~value =
+  if pif_has_clustering_enabled ~__context self
+  then
+    let network_ref = Db.PIF.get_network ~__context ~self |> Ref.string_of in


We already got this from the DB in the function above, so this could be optimised a little. Not a big deal though.

robhoes · 2018-03-07T13:30:38Z

Just added a minor comment, but generally LGTM.

lindig · 2018-03-07T15:06:30Z

I would like to see commits to include the CA unless the history is cleaned.

edwintorok · 2018-03-07T15:12:06Z

I think all the commits could be squashed into one, it is logically one set of self-consistent changes.

- define set_disallow_unplug as dynamicRO in datamodel - add new error for toggling disallow_unplug with clustering enabled - add assertion and manual setter - add unit tests for set_disallow_unplug with and without clustering enabled CA-268511: Declare new API error clustering_enabled_on_network CA-268511: Implement PIF.set_disallow_unplug CA-268511: Add logging to function message wrapper CA-268511: Test PIF.set_disallow_unplug for idempotency and exceptions Signed-off-by: Akanksha Mathur <akanksha.mathur@citrix.com>

gaborigloi self-requested a review March 5, 2018 10:45

gaborigloi reviewed Mar 5, 2018

View reviewed changes

minishrink force-pushed the feature/REQ477/CA-268511 branch from 4c6f400 to c7cec95 Compare March 5, 2018 10:50

minishrink commented Mar 5, 2018

View reviewed changes

minishrink force-pushed the feature/REQ477/CA-268511 branch from 0c98292 to fb18629 Compare March 5, 2018 11:19

gaborigloi reviewed Mar 5, 2018

View reviewed changes

gaborigloi approved these changes Mar 5, 2018

View reviewed changes

robhoes reviewed Mar 5, 2018

View reviewed changes

robhoes requested changes Mar 5, 2018

View reviewed changes

minishrink force-pushed the feature/REQ477/CA-268511 branch from 79c6aae to 787b1a8 Compare March 5, 2018 20:16

gaborigloi reviewed Mar 6, 2018

View reviewed changes

gaborigloi approved these changes Mar 7, 2018

View reviewed changes

edwintorok approved these changes Mar 7, 2018

View reviewed changes

robhoes reviewed Mar 7, 2018

View reviewed changes

robhoes approved these changes Mar 7, 2018

View reviewed changes

lindig closed this Mar 7, 2018

lindig reopened this Mar 7, 2018

minishrink force-pushed the feature/REQ477/CA-268511 branch 3 times, most recently from 64caeac to 18b976a Compare March 7, 2018 17:47

minishrink force-pushed the feature/REQ477/CA-268511 branch from 18b976a to 5252972 Compare March 8, 2018 10:59

edwintorok merged commit db790e6 into xapi-project:feature/REQ477/master Mar 8, 2018

minishrink deleted the feature/REQ477/CA-268511 branch March 8, 2018 11:13

CA-268511: Make PIF.disallow_unplug RO when clustering is enabled #3485

CA-268511: Make PIF.disallow_unplug RO when clustering is enabled #3485

Uh oh!

Conversation

minishrink commented Mar 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Mar 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gaborigloi Mar 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robhoes Mar 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaborigloi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaborigloi Mar 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robhoes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaborigloi left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edwintorok left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

minishrink commented Mar 5, 2018 •

edited

Loading

coveralls commented Mar 5, 2018 •

edited

Loading

gaborigloi Mar 5, 2018 •

edited

Loading

robhoes Mar 5, 2018 •

edited

Loading

gaborigloi Mar 6, 2018 •

edited

Loading

gaborigloi left a comment •

edited

Loading

lindig commented Mar 7, 2018 •

edited

Loading