CP-53446: Split plug and unplug atomics to enable live migration downtime reduction later (#6362)

Split the plug and unplug atomics into separate attach/activate and
deactivate/detach atomics. This is gated by two separate flags and both
are switched off by default. When switched on, this should still be a
no-op by calling them in serial where plug/unplug are currently used,
but will allow us to later use these atomics separately elsewhere for
optimisation.
Opening this as a draft PR as plug/unplug is fundamental so the logic
needs to be watertight before we consider merging.

Author: snwoods

ocaml/xenopsd/lib/storage.ml

@@ -62,20 +62,20 @@ let vm_of_domid vmdomid =
         "Invalid domid, could not be converted to int, passing empty string." ;
       Storage_interface.Vm.of_string ""
 
-let attach_and_activate ~task ~_vm ~vmdomid ~dp ~sr ~vdi ~read_write =
+let attach ~task ~_vm ~vmdomid ~dp ~sr ~vdi ~read_write =
+  let dbg = get_dbg task in
+  Xenops_task.with_subtask task
+    (Printf.sprintf "VDI.attach3 %s" dp)
+    (transform_exception (fun () ->
+         Client.VDI.attach3 dbg dp sr vdi vmdomid read_write
+     )
+    )
+
+let activate ~task ~_vm ~vmdomid ~dp ~sr ~vdi =
   let dbg = get_dbg task in
-  let result =
-    Xenops_task.with_subtask task
-      (Printf.sprintf "VDI.attach3 %s" dp)
-      (transform_exception (fun () ->
-           Client.VDI.attach3 dbg dp sr vdi vmdomid read_write
-       )
-      )
-  in
   Xenops_task.with_subtask task
     (Printf.sprintf "VDI.activate3 %s" dp)
-    (transform_exception (fun () -> Client.VDI.activate3 dbg dp sr vdi vmdomid)) ;
-  result
+    (transform_exception (fun () -> Client.VDI.activate3 dbg dp sr vdi vmdomid))
 
 let deactivate task dp sr vdi vmdomid =
   debug "Deactivating disk %s %s" (Sr.string_of sr) (Vdi.string_of vdi) ;

ocaml/xenopsd/lib/xenops_server.ml

@@ -37,6 +37,8 @@ let finally = Xapi_stdext_pervasives.Pervasiveext.finally
 
 let domain_shutdown_ack_timeout = ref 60.
 
+let xenopsd_vbd_plug_unplug_legacy = ref true
+
 type context = {
     transferred_fd: Unix.file_descr option
         (** some API calls take a file descriptor argument *)
@@ -122,10 +124,14 @@ type atomic =
   | VM_hook_script_stable of (Vm.id * Xenops_hooks.script * string * Vm.id)
   | VM_hook_script of (Vm.id * Xenops_hooks.script * string)
   | VBD_plug of Vbd.id
+  | VBD_attach of Vbd.id
+  | VBD_activate of Vbd.id
   | VBD_epoch_begin of (Vbd.id * disk * bool)
   | VBD_epoch_end of (Vbd.id * disk)
   | VBD_set_qos of Vbd.id
   | VBD_unplug of Vbd.id * bool
+  | VBD_deactivate of Vbd.id * bool
+  | VBD_detach of Vbd.id
   | VBD_insert of Vbd.id * disk
   | VBD_set_active of Vbd.id * bool
   | VM_remove of Vm.id
@@ -195,6 +201,10 @@ let rec name_of_atomic = function
       "VM_hook_script"
   | VBD_plug _ ->
       "VBD_plug"
+  | VBD_attach _ ->
+      "VBD_attach"
+  | VBD_activate _ ->
+      "VBD_activate"
   | VBD_epoch_begin _ ->
       "VBD_epoch_begin"
   | VBD_epoch_end _ ->
@@ -203,6 +213,10 @@ let rec name_of_atomic = function
       "VBD_set_qos"
   | VBD_unplug _ ->
       "VBD_unplug"
+  | VBD_deactivate _ ->
+      "VBD_deactivate"
+  | VBD_detach _ ->
+      "VBD_detach"
   | VBD_insert _ ->
       "VBD_insert"
   | VBD_set_active _ ->
@@ -1580,6 +1594,26 @@ let parallel_map name ~id lst f = parallel name ~id (List.concat_map f lst)
 
 let map_or_empty f x = Option.value ~default:[] (Option.map f x)
 
+(* Creates a Serial of 2 or more Atomics. If the number of Atomics could be
+   less than this, use serial or serial_concat *)
+let serial_of name ~id at1 at2 ats =
+  Serial (id, Printf.sprintf "%s VM=%s" name id, at1 :: at2 :: ats)
+
+let vbd_plug vbd_id =
+  if !xenopsd_vbd_plug_unplug_legacy then
+    VBD_plug vbd_id
+  else
+    serial_of "VBD.attach_and_activate" ~id:(VBD_DB.vm_of vbd_id)
+      (VBD_attach vbd_id) (VBD_activate vbd_id) []
+
+let vbd_unplug vbd_id force =
+  if !xenopsd_vbd_plug_unplug_legacy then
+    VBD_unplug (vbd_id, force)
+  else
+    serial_of "VBD.deactivate_and_detach" ~id:(VBD_DB.vm_of vbd_id)
+      (VBD_deactivate (vbd_id, force))
+      (VBD_detach vbd_id) []
+
 let rec atomics_of_operation = function
   | VM_start (id, force) ->
       let vbds_rw, vbds_ro = VBD_DB.vbds id |> vbd_plug_sets in
@@ -1604,7 +1638,7 @@ let rec atomics_of_operation = function
                     [VBD_epoch_begin (vbd.Vbd.id, x, vbd.Vbd.persistent)]
                   )
                   vbd.Vbd.backend
-              ; [VBD_plug vbd.Vbd.id]
+              ; [vbd_plug vbd.Vbd.id]
               ]
         )
       in
@@ -1668,7 +1702,7 @@ let rec atomics_of_operation = function
         ]
       ; parallel_concat "Devices.unplug" ~id
           [
-            List.map (fun vbd -> VBD_unplug (vbd.Vbd.id, true)) vbds
+            List.map (fun vbd -> vbd_unplug vbd.Vbd.id true) vbds
           ; List.map (fun vif -> VIF_unplug (vif.Vif.id, true)) vifs
           ; List.map (fun pci -> PCI_unplug pci.Pci.id) pcis
           ]
@@ -1692,7 +1726,7 @@ let rec atomics_of_operation = function
         let name_one = pf "VBD.activate_and_plug %s" typ in
         parallel_map name_multi ~id vbds (fun vbd ->
             serial name_one ~id
-              [VBD_set_active (vbd.Vbd.id, true); VBD_plug vbd.Vbd.id]
+              [VBD_set_active (vbd.Vbd.id, true); vbd_plug vbd.Vbd.id]
         )
       in
       [
@@ -1825,9 +1859,9 @@ let rec atomics_of_operation = function
       ]
       |> List.concat
   | VBD_hotplug id ->
-      [VBD_set_active (id, true); VBD_plug id]
+      [VBD_set_active (id, true); vbd_plug id]
   | VBD_hotunplug (id, force) ->
-      [VBD_unplug (id, force); VBD_set_active (id, false)]
+      [vbd_unplug id force; VBD_set_active (id, false)]
   | VIF_hotplug id ->
       [VIF_set_active (id, true); VIF_plug id]
   | VIF_hotunplug (id, force) ->
@@ -2017,7 +2051,16 @@ let rec perform_atomic ~progress_callback ?result (op : atomic)
       Xenops_hooks.vm ~script ~reason ~id ~extra_args
   | VBD_plug id ->
       debug "VBD.plug %s" (VBD_DB.string_of_id id) ;
-      B.VBD.plug t (VBD_DB.vm_of id) (VBD_DB.read_exn id) ;
+      B.VBD.attach t (VBD_DB.vm_of id) (VBD_DB.read_exn id) ;
+      B.VBD.activate t (VBD_DB.vm_of id) (VBD_DB.read_exn id) ;
+      VBD_DB.signal id
+  | VBD_attach id ->
+      debug "VBD.attach %s" (VBD_DB.string_of_id id) ;
+      B.VBD.attach t (VBD_DB.vm_of id) (VBD_DB.read_exn id) ;
+      VBD_DB.signal id
+  | VBD_activate id ->
+      debug "VBD.activate %s" (VBD_DB.string_of_id id) ;
+      B.VBD.activate t (VBD_DB.vm_of id) (VBD_DB.read_exn id) ;
       VBD_DB.signal id
   | VBD_set_active (id, b) ->
       debug "VBD.set_active %s %b" (VBD_DB.string_of_id id) b ;
@@ -2036,8 +2079,22 @@ let rec perform_atomic ~progress_callback ?result (op : atomic)
   | VBD_unplug (id, force) ->
       debug "VBD.unplug %s" (VBD_DB.string_of_id id) ;
       finally
-        (fun () -> B.VBD.unplug t (VBD_DB.vm_of id) (VBD_DB.read_exn id) force)
+        (fun () ->
+          B.VBD.deactivate t (VBD_DB.vm_of id) (VBD_DB.read_exn id) force ;
+          B.VBD.detach t (VBD_DB.vm_of id) (VBD_DB.read_exn id)
+        )
         (fun () -> VBD_DB.signal id)
+  | VBD_deactivate (id, force) ->
+      debug "VBD.deactivate %s" (VBD_DB.string_of_id id) ;
+      finally
+        (fun () ->
+          B.VBD.deactivate t (VBD_DB.vm_of id) (VBD_DB.read_exn id) force
+        )
+        (fun () -> VBD_DB.signal id)
+  | VBD_detach id ->
+      debug "VBD.detach %s" (VBD_DB.string_of_id id) ;
+      B.VBD.detach t (VBD_DB.vm_of id) (VBD_DB.read_exn id) ;
+      VBD_DB.signal id
   | VBD_insert (id, disk) -> (
       (* NB this is also used to "refresh" ie signal a qemu that it should
          re-open a device, useful for when a physical CDROM is inserted into the
@@ -2445,11 +2502,15 @@ and trigger_cleanup_after_failure_atom op t =
   match op with
   | VBD_eject id
   | VBD_plug id
+  | VBD_attach id
+  | VBD_activate id
   | VBD_set_active (id, _)
   | VBD_epoch_begin (id, _, _)
   | VBD_epoch_end (id, _)
   | VBD_set_qos id
   | VBD_unplug (id, _)
+  | VBD_deactivate (id, _)
+  | VBD_detach id
   | VBD_insert (id, _) ->
       immediate_operation dbg (fst id) (VBD_check_state id)
   | VIF_plug id

ocaml/xenopsd/lib/xenops_server_plugin.ml

@@ -207,9 +207,13 @@ module type S = sig
 
     val epoch_end : Xenops_task.task_handle -> Vm.id -> disk -> unit
 
-    val plug : Xenops_task.task_handle -> Vm.id -> Vbd.t -> unit
+    val attach : Xenops_task.task_handle -> Vm.id -> Vbd.t -> unit
 
-    val unplug : Xenops_task.task_handle -> Vm.id -> Vbd.t -> bool -> unit
+    val activate : Xenops_task.task_handle -> Vm.id -> Vbd.t -> unit
+
+    val deactivate : Xenops_task.task_handle -> Vm.id -> Vbd.t -> bool -> unit
+
+    val detach : Xenops_task.task_handle -> Vm.id -> Vbd.t -> unit
 
     val insert : Xenops_task.task_handle -> Vm.id -> Vbd.t -> disk -> unit
 

ocaml/xenopsd/lib/xenops_server_simulator.ml

@@ -673,9 +673,13 @@ module VBD = struct
 
   let epoch_end _ (_vm : Vm.id) (_disk : disk) = ()
 
-  let plug _ (vm : Vm.id) (vbd : Vbd.t) = with_lock m (add_vbd vm vbd)
+  let attach _ (vm : Vm.id) (vbd : Vbd.t) = with_lock m (add_vbd vm vbd)
 
-  let unplug _ vm vbd _ = with_lock m (remove_vbd vm vbd)
+  let activate _ (_vm : Vm.id) (_vbd : Vbd.t) = ()
+
+  let deactivate _ vm vbd _ = with_lock m (remove_vbd vm vbd)
+
+  let detach _ _vm _vbd = ()
 
   let insert _ _vm _vbd _disk = ()
 

ocaml/xenopsd/lib/xenops_server_skeleton.ml

@@ -145,9 +145,13 @@ module VBD = struct
 
   let epoch_end _ _ _ = ()
 
-  let plug _ _ _ = unimplemented "VBD.plug"
+  let attach _ _ _ = unimplemented "VBD.attach"
 
-  let unplug _ _ _ _ = unimplemented "VBD.unplug"
+  let activate _ _ _ = unimplemented "VBD.activate"
+
+  let deactivate _ _ _ _ = unimplemented "VBD.deactivate"
+
+  let detach _ _ _ = unimplemented "VBD.detach"
 
   let insert _ _ _ _ = unimplemented "VBD.insert"
 

ocaml/xenopsd/lib/xenopsd.ml

@@ -283,6 +283,11 @@ let options =
     , (fun () -> string_of_int !test_open)
     , "TESTING only: open N file descriptors"
     )
+  ; ( "xenopsd-vbd-plug-unplug-legacy"
+    , Arg.Bool (fun x -> Xenops_server.xenopsd_vbd_plug_unplug_legacy := x)
+    , (fun () -> string_of_bool !Xenops_server.xenopsd_vbd_plug_unplug_legacy)
+    , "False if we want to split the plug atomic into attach/activate"
+    )
   ]
 
 let path () = Filename.concat !sockets_path "xenopsd"