[From REQ-477] Cherry-pick addition of iSCSI IQN and multipathing changes #3470

mcintyre94 · 2018-02-23T15:21:56Z

This adds the iSCSI IQN functionality and multipatching changes (they were closely intertwined, and only 2 commits were identified initially as multipathing anyway so I combined them here).

Note that the changes to datamodel.ml in a few commits were manually patched into datamodel_host.ml since the split was done since those commits were made.

jonludlam · 2018-02-23T15:33:56Z

This looks fine to me.

coveralls · 2018-02-23T15:39:34Z

Coverage increased (+0.09%) to 18.186% when pulling 2629e6b on mcintyre94:CP-26970-iscsiiqn into 694b31b on xapi-project:master.

mcintyre94 · 2018-02-23T17:40:45Z

Rebased on master now #3468 has merged :)

lindig · 2018-02-27T14:30:13Z

This needs a rebase

mcintyre94 · 2018-02-27T16:26:04Z

Fixup: Moved test_host_helpers.ml to ocaml/tests/

edwintorok · 2018-02-28T10:52:37Z

We should replace rel_jura with rel_kolkata, this change didn't make it into jura.

mcintyre94 · 2018-02-28T14:25:35Z

Replaced rel_jura with rel_kolkata per Edwin's comment ^

mseri · 2018-03-01T10:10:48Z

ocaml/idl/datamodel_host.ml

-           field ~qualifier:DynamicRO ~lifecycle:[Published, rel_falcon, ""] ~ty:(Set (Ref _feature)) "features" "List of features available on this host"
+         field ~qualifier:DynamicRO ~lifecycle:[Published, rel_falcon, ""] ~ty:(Set (Ref _feature)) "features" "List of features available on this host";
+         field ~qualifier:StaticRO ~lifecycle:[Published, rel_kolkata, ""] ~default_value:(Some (VString "")) ~ty:String "iscsi_iqn" "The initiator IQN for the host";
+         field ~qualifier:StaticRO ~lifecycle:[Published, rel_kolkata, ""] ~default_value:(Some (VBool false)) ~ty:Bool "multipathing" "Specifies whether multipathing is enabled";


This will likely need a run of ocp-indent

mseri

I mostly have minor changes. I am bit scared on some potential failures and the need of the weird set+other_config flow

mseri · 2018-03-01T10:11:45Z

ocaml/tests/suite.ml

    Test_network_event_loop.test;
    Test_network.test;
    Test_pusb.test;
+    Test_host_helpers.test;


From now on, when we introduce new tests we should probably use Alcotest from the start. No change needed, this can be ported later with the other tests

mseri · 2018-03-01T10:12:44Z

ocaml/idl/datamodel_host.ml

-           field ~qualifier:DynamicRO ~lifecycle:[Published, rel_falcon, ""] ~ty:(Set (Ref _feature)) "features" "List of features available on this host"
+         field ~qualifier:DynamicRO ~lifecycle:[Published, rel_falcon, ""] ~ty:(Set (Ref _feature)) "features" "List of features available on this host";
+         field ~qualifier:StaticRO ~lifecycle:[Published, rel_kolkata, ""] ~default_value:(Some (VString "")) ~ty:String "iscsi_iqn" "The initiator IQN for the host";
+         field ~qualifier:StaticRO ~lifecycle:[Published, rel_kolkata, ""] ~default_value:(Some (VBool false)) ~ty:Bool "multipathing" "Specifies whether multipathing is enabled";


Can the new fields break update/compatibility or they are safe? I am assuming the second, but I can never remember which fields are a problem and which not.

as discussed today new fields are safe.

mseri · 2018-03-01T10:14:19Z

ocaml/tests/test_host_helpers.ml

+  Db.Host.add_to_other_config ~__context ~self:host1 ~key:"multipathing" ~value:"false";
+  let _token = watcher token in
+  assert_equal !calls []
+


We are testing "success" cases, do we have understood failure cases that also would make sense to check?

I don't think that setting multipathing should fail (unless the DB itself fails)

mseri · 2018-03-01T10:18:29Z

ocaml/xapi/xapi_host.ml

+  if value = "" then raise Api_errors.(Server_error (invalid_value, ["value"; value]));
+  (* Note, the following sequence is carefully written - see the
+     other-config watcher thread in xapi_host_helpers.ml *)
+  Db.Host.remove_from_other_config ~__context ~self:host ~key:"iscsi_iqn";


This is out of scope for this PR, and I suggest not to touch it. But, as a remark for the future, we really need a with_other_config_update and replace_in_other_config pair of helpers. I have seen quite a few instances of these patterns...

mseri · 2018-03-01T10:21:04Z

ocaml/xapi/xapi_host_helpers.ml

+      iqn hostname_chopped
+
+  let set_initiator_name iqn =
+    let hostname = Unix.gethostname () in


Should we protect this against Unix_errors?

We'd let this one to be treated as an internal error.

mseri · 2018-03-01T12:02:23Z

ocaml/xapi/xapi_host_helpers.ml

+    let flag = !Xapi_globs.multipathing_config_file in
+    if enabled then Unixext.touch_file flag
+    else begin
+      if Sys.file_exists flag then Sys.remove flag


I'm wary of Sys... we cannot catch the errors properly when using it. Can we do this using Unix and maybe handle potential exceptions with internal_error?

Replaced with Unixext.unlink_safe.

Thanks! That also prevents potential error in case of races, although I don't think it would have been the case for this.

mseri · 2018-03-01T12:05:21Z

ocaml/xapi/xapi_host.ml

+     other-config watcher thread in xapi_host_helpers.ml *)
+  Db.Host.remove_from_other_config ~__context ~self:host ~key:"iscsi_iqn";
+  Db.Host.set_iscsi_iqn ~__context ~self:host ~value;
+  Db.Host.add_to_other_config ~__context ~self:host ~key:"iscsi_iqn" ~value;


This re-triggers the loop ~~and an rpc call to set the iscsi_iqn, thing that we have "kind of" just done above.~~
Why such a complicated flow?

Update: the rpc call is ignored because we compare with the db record, still it is quite a compilcate flow and the comment on the careful ordering could benefit from more context

I've added a comment attempting to explain my understanding of it:

+ (* we need to first set the iscsi_iqn field and then the other-config field: + * setting the other-config field triggers and update on the iscsi_iqn + * field if they are different + * + * we want to keep the legacy `other_config:iscsi_iqn` and the new `iscsi_iqn` + * fields in sync: + * when you update the `iscsi_iqn` field we want to update `other_config`, + * but when updating `other_config` we want to update `iscsi_iqn` too. + * we have to be careful not to introduce an infinite loop of updates. + * *)

Yes, I think it's clearer. Thanks

mseri · 2018-03-01T12:06:01Z

ocaml/xapi/xapi_host_helpers.mli

+ * GNU Lesser General Public License for more details.
+ *)
+
+(** Common code between for dealing with Hosts.


Probably between is a leftover from an old version

mseri · 2018-03-01T12:07:32Z

ocaml/xapi/xapi_host_helpers.mli

+      intended to be run on the master. The returned function has type
+      [token -> token], where [token] is a string. The initial value should be
+      the empty string, and the returned value should be used for further
+      invocations. This function is exposed only for unit testing, and should


It is annoying that for unit testing we need to do this...

There is a ppx to do inline testing, but that would require using yet another ppx...

Yes, I know, I wasn't suggesting to do anything. It's unfortunate, I only wish there was some compiler level support for tests behind mli

mseri · 2018-03-01T14:31:43Z

As per current discussion, we need to update:
https://github.com/xapi-project/xen-api/blob/master/scripts/xe-set-iscsi-iqn
and/or the firstboot script calling it.

Signed-off-by: Jon Ludlam <jonathan.ludlam@citrix.com> Note: Manually applied the patch from datamodel.ml to datamodel_host.ml Signed-off-by: Callum McIntyre <callum.mcintyre@citrix.com>

Signed-off-by: Jon Ludlam <jonathan.ludlam@citrix.com>

This adds the functions that set the initiatorname.iscsi file and the thread that watches the database for host.other-config:iscsi_iqn changes. When that thread is running a change to the other-config field will trigger a call to Host.set_iscsi_iqn. Signed-off-by: Jon Ludlam <jonathan.ludlam@citrix.com>

Signed-off-by: Jon Ludlam <jonathan.ludlam@citrix.com>

mseri

For what concerns me, with this last comment (that practically reflects also what happens in the multipath case), this is good to go.

Before merging, please squash the fixups

It's got to coexist with the other-config watcher Signed-off-by: Jon Ludlam <jonathan.ludlam@citrix.com>

Signed-off-by: Jon Ludlam <jonathan.ludlam@citrix.com>

Signed-off-by: Gabor Igloi <gabor.igloi@citrix.com>

Signed-off-by: Jon Ludlam <jonathan.ludlam@citrix.com>

And keep it up to date with the value of the multipath key in other_config for backward compatibility. The file /var/run/nonpersistent/multipath_enabled is touched / removed when multipathing is enabled / disabled. Signed-off-by: Gabor Igloi <gabor.igloi@citrix.com> Note: Manually applied the patch from datamodel.ml to datamodel_host.ml Signed-off-by: Callum McIntyre <callum.mcintyre@citrix.com>

We have to make sure these are handled correctly on startup, in case there was a hard reboot and had to recover from a redo log, or when one of these are in a nonpersistent file. We cannot know the state of these config files before xapi started, so it's the safest to sync them at startup. Signed-off-by: Gabor Igloi <gabor.igloi@citrix.com>

Because it's invalid. Signed-off-by: Gabor Igloi <gabor.igloi@citrix.com>

Signed-off-by: Mark Syms <mark.syms@citrix.com>

I tested this on a DT box and it properly updated the other-config and iscsi_iqn host param. Signed-off-by: Edwin Török <edvin.torok@citrix.com>

edwintorok · 2018-03-01T17:44:05Z

Thanks for the review, lets wait until Travis build finishes.

mcintyre94 force-pushed the CP-26970-iscsiiqn branch from dcdea9f to 2c2fb14 Compare February 23, 2018 17:39

mcintyre94 mentioned this pull request Feb 23, 2018

[From REQ-477] Cherry-pick of shutdown changes #3473

Merged

lindig added the needs updating label Feb 27, 2018

edwintorok force-pushed the CP-26970-iscsiiqn branch from 2c2fb14 to cbde7b3 Compare February 27, 2018 15:02

edwintorok added please review and removed needs updating please review labels Feb 27, 2018

edwintorok requested a review from gaborigloi February 27, 2018 15:03

mcintyre94 force-pushed the CP-26970-iscsiiqn branch from cbde7b3 to 2382edb Compare February 27, 2018 16:25

edwintorok removed the request for review from gaborigloi February 27, 2018 16:53

edwintorok added please review needs updating and removed please review labels Feb 27, 2018

mcintyre94 force-pushed the CP-26970-iscsiiqn branch from 2382edb to 198c509 Compare February 28, 2018 14:24

edwintorok added please review and removed needs updating labels Feb 28, 2018

mseri reviewed Mar 1, 2018

View reviewed changes

mseri suggested changes Mar 1, 2018

View reviewed changes

edwintorok mentioned this pull request Mar 1, 2018

[477 merge review draft] GFS2 feature branch #3482

Merged

jonludlam added 3 commits March 1, 2018 16:56

CP-25621: Add Host.iscsi_iqn to datamodel

eb684d8

Signed-off-by: Jon Ludlam <jonathan.ludlam@citrix.com> Note: Manually applied the patch from datamodel.ml to datamodel_host.ml Signed-off-by: Callum McIntyre <callum.mcintyre@citrix.com>

CP-25621: Add stub set_iscsi_iqn function

b9330d0

Signed-off-by: Jon Ludlam <jonathan.ludlam@citrix.com> Note: Manually applied the patch from datamodel.ml to datamodel_host.ml Signed-off-by: Callum McIntyre <callum.mcintyre@citrix.com>

CP-25621: Add iscsi_iqn to the CLI

1c1db16

Signed-off-by: Jon Ludlam <jonathan.ludlam@citrix.com>

jonludlam added 3 commits March 1, 2018 16:56

CP-25621: Start other-config watcher thread

eebcc10

Signed-off-by: Jon Ludlam <jonathan.ludlam@citrix.com>

CP-25621: Actually do the set in Host.set_iscsi_iqn

dc39113

Signed-off-by: Jon Ludlam <jonathan.ludlam@citrix.com>

edwintorok force-pushed the CP-26970-iscsiiqn branch from 198c509 to bc3f1d4 Compare March 1, 2018 16:58

edwintorok added the needs updating label Mar 1, 2018

mseri approved these changes Mar 1, 2018

View reviewed changes

jonludlam and others added 11 commits March 1, 2018 17:43

CP-25621: Fix implementation of Host.set_iscsi_iqn

437e263

It's got to coexist with the other-config watcher Signed-off-by: Jon Ludlam <jonathan.ludlam@citrix.com>

CP-25621: Add a unit test for xapi_host_helpers.ml

0d2a4df

Signed-off-by: Jon Ludlam <jonathan.ludlam@citrix.com>

CP-25621: Add a test of the updated implementation of set_iscsi_iqn

3bd02c0

Signed-off-by: Jon Ludlam <jonathan.ludlam@citrix.com>

Xapi_host_helpers: re-add comment about MD3000i alias bug workaround

f7ebddb

Signed-off-by: Gabor Igloi <gabor.igloi@citrix.com>

CP-25621: Add an mli file for xapi_host_helpers

2734c37

Signed-off-by: Jon Ludlam <jonathan.ludlam@citrix.com>

CP-25621: Respond to review comments from @gabori

41708bd

Signed-off-by: Jon Ludlam <jonathan.ludlam@citrix.com>

Host.set_iscsi_iqn: reject empty string

a4a94b3

Because it's invalid. Signed-off-by: Gabor Igloi <gabor.igloi@citrix.com>

Add SR Multipath capability

d194a39

Signed-off-by: Mark Syms <mark.syms@citrix.com>

Update firstboot scripts to use the new API for setting the iSCSI IQN

2629e6b

I tested this on a DT box and it properly updated the other-config and iscsi_iqn host param. Signed-off-by: Edwin Török <edvin.torok@citrix.com>

edwintorok force-pushed the CP-26970-iscsiiqn branch from f7c6931 to 2629e6b Compare March 1, 2018 17:43

edwintorok added reviewed and removed needs updating please review labels Mar 1, 2018

edwintorok merged commit abe969c into xapi-project:master Mar 1, 2018

edwintorok deleted the CP-26970-iscsiiqn branch March 1, 2018 18:05

[From REQ-477] Cherry-pick addition of iSCSI IQN and multipathing changes #3470

[From REQ-477] Cherry-pick addition of iSCSI IQN and multipathing changes #3470

Uh oh!

Conversation

mcintyre94 commented Feb 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonludlam commented Feb 23, 2018

Uh oh!

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

Uh oh!

Uh oh!

mcintyre94 commented Feb 23, 2018

Uh oh!

lindig commented Feb 27, 2018

Uh oh!

mcintyre94 commented Feb 27, 2018

Uh oh!

edwintorok commented Feb 28, 2018

Uh oh!

mcintyre94 commented Feb 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mseri 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!

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!

mseri Mar 1, 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!

mseri Mar 1, 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!

mseri Mar 1, 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!

edwintorok Mar 1, 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!

mseri commented Mar 1, 2018

Uh oh!

mseri left a comment

Choose a reason for hiding this comment

Uh oh!

edwintorok commented Mar 1, 2018

Uh oh!

mcintyre94 commented Feb 23, 2018 •

edited

Loading

coveralls commented Feb 23, 2018 •

edited

Loading

mseri Mar 1, 2018 •

edited

Loading

mseri Mar 1, 2018 •

edited

Loading

mseri Mar 1, 2018 •

edited

Loading

edwintorok Mar 1, 2018 •

edited

Loading