CP-45016: Delay VDI.compose in SXM

`VDI.compose` operation links a child volume to a parent, which changes the internal structure of the volume. This is unsafe to do while the mirroring datapath is activated. Instead, we move `VDI.compose` into `receive_finalize`, which is when the source VM is paused, and the mirroring datapath deactivated. Implement `receive_finalize2` for this purpose, and also maintain backwards compatibility. This change also makes the dummy snapshot of the mirror VDI on the destination host redundant, as the GC only will start if there is a parent with a single child in the VHD tree, which will not happen if compose is not called. Since compose is only called when mirroring is finished, we do not need to prevent GC from interfering the mirroring operation. Signed-off-by: Vincent Liu <[email protected]>
xapi-project · Nov 15, 2024 · c54a8da · c54a8da
1 parent e8b1d08
commit c54a8da
Show file tree

Hide file tree

Showing 8 changed files with 67 additions and 17 deletions.
diff --git a/ocaml/tests/test_storage_migrate_state.ml b/ocaml/tests/test_storage_migrate_state.ml
@@ -53,6 +53,7 @@ let sample_receive_state =
     ; leaf_dp= "leaf_dp"
     ; parent_vdi= Vdi.of_string "parent_vdi"
     ; remote_vdi= Vdi.of_string "remote_vdi"
+    ; mirror_vm= Vm.of_string "mirror_vm"
     }
 
 let sample_copy_state =

diff --git a/ocaml/xapi-idl/storage/storage_interface.ml b/ocaml/xapi-idl/storage/storage_interface.ml
@@ -1067,10 +1067,22 @@ module StorageAPI (R : RPC) = struct
           @-> returning result err
           )
 
+      (** Called on the receiving end 
+        @deprecated This function is deprecated, and is only here to keep backward 
+        compatibility with old xapis that call Remote.DATA.MIRROR.receive_finalize
+        during SXM.  Use the receive_finalize2 function instead. 
+      *)
       let receive_finalize =
         declare "DATA.MIRROR.receive_finalize" []
           (dbg_p @-> id_p @-> returning unit_p err)
 
+      (** [receive_finalize2 dbg id] will stop the mirroring process and compose 
+      the snapshot VDI with the mirror VDI. It also cleans up the storage resources 
+      used by mirroring. It is called after the the source VM is paused. *)
+      let receive_finalize2 =
+        declare "DATA.MIRROR.receive_finalize2" []
+          (dbg_p @-> id_p @-> returning unit_p err)
+
       let receive_cancel =
         declare "DATA.MIRROR.receive_cancel" []
           (dbg_p @-> id_p @-> returning unit_p err)
@@ -1443,6 +1455,8 @@ module type Server_impl = sig
 
       val receive_finalize : context -> dbg:debug_info -> id:Mirror.id -> unit
 
+      val receive_finalize2 : context -> dbg:debug_info -> id:Mirror.id -> unit
+
       val receive_cancel : context -> dbg:debug_info -> id:Mirror.id -> unit
 
       val list : context -> dbg:debug_info -> (Mirror.id * Mirror.t) list
@@ -1634,6 +1648,9 @@ module Server (Impl : Server_impl) () = struct
     S.DATA.MIRROR.receive_finalize (fun dbg id ->
         Impl.DATA.MIRROR.receive_finalize () ~dbg ~id
     ) ;
+    S.DATA.MIRROR.receive_finalize2 (fun dbg id ->
+        Impl.DATA.MIRROR.receive_finalize2 () ~dbg ~id
+    ) ;
     S.DATA.MIRROR.list (fun dbg -> Impl.DATA.MIRROR.list () ~dbg) ;
     S.DATA.MIRROR.get_nbd_server (fun dbg dp sr vdi vm ->
         Impl.DATA.MIRROR.get_nbd_server () ~dbg ~dp ~sr ~vdi ~vm

diff --git a/ocaml/xapi-idl/storage/storage_skeleton.ml b/ocaml/xapi-idl/storage/storage_skeleton.ml
@@ -174,6 +174,8 @@ module DATA = struct
 
     let receive_finalize ctx ~dbg ~id = u "DATA.MIRROR.receive_finalize"
 
+    let receive_finalize2 ctx ~dbg ~id = u "DATA.MIRROR.receive_finalize2"
+
     let receive_cancel ctx ~dbg ~id = u "DATA.MIRROR.receive_cancel"
 
     let list ctx ~dbg = u "DATA.MIRROR.list"

diff --git a/ocaml/xapi-storage-script/main.ml b/ocaml/xapi-storage-script/main.ml
@@ -1808,6 +1808,7 @@ let bind ~volume_script_dir =
   S.DATA.copy (u "DATA.copy") ;
   S.DP.stat_vdi (u "DP.stat_vdi") ;
   S.DATA.MIRROR.receive_finalize (u "DATA.MIRROR.receive_finalize") ;
+  S.DATA.MIRROR.receive_finalize2 (u "DATA.MIRROR.receive_finalize2") ;
   S.DP.create (u "DP.create") ;
   S.DP.attach_info (u "DP.attach_info") ;
   S.TASK.cancel (u "TASK.cancel") ;

diff --git a/ocaml/xapi/storage_migrate.ml b/ocaml/xapi/storage_migrate.ml
@@ -36,6 +36,7 @@ module State = struct
       ; leaf_dp: dp
       ; parent_vdi: Vdi.t
       ; remote_vdi: Vdi.t
+      ; mirror_vm: Vm.t
     }
     [@@deriving rpcty]
 
@@ -872,15 +873,9 @@ let start' ~task ~dbg:_ ~sr ~vdi ~dp ~mirror_vm ~copy_vm ~url ~dest ~verify_dest
     debug "Local VDI %s = remote VDI %s"
       (Storage_interface.Vdi.string_of snapshot.vdi)
       (Storage_interface.Vdi.string_of new_parent.vdi) ;
-    Remote.VDI.compose dbg dest result.Mirror.copy_diffs_to
-      result.Mirror.mirror_vdi.vdi ;
-    Remote.VDI.remove_from_sm_config dbg dest result.Mirror.mirror_vdi.vdi
-      "base_mirror" ;
     debug "Local VDI %s now mirrored to remote VDI: %s"
       (Storage_interface.Vdi.string_of local_vdi.vdi)
       (Storage_interface.Vdi.string_of result.Mirror.mirror_vdi.vdi) ;
-    debug "Destroying dummy VDI on remote" ;
-    Remote.VDI.destroy dbg dest result.Mirror.dummy_vdi ;
     debug "Destroying snapshot on src" ;
     Local.VDI.destroy dbg sr snapshot.vdi ;
     Some (Mirror_id id)
@@ -1022,7 +1017,8 @@ let killall ~dbg =
     recv_ops ;
   State.clear ()
 
-let receive_start2 ~dbg ~sr ~vdi_info ~id ~similar ~vm =
+let receive_start_common ~dbg ?(create_dummy = false) ~sr ~vdi_info ~id ~similar
+    ~vm () =
   let on_fail : (unit -> unit) list ref = ref [] in
   let vdis = Local.SR.scan dbg sr in
   (* We drop cbt_metadata VDIs that do not have any actual data *)
@@ -1033,10 +1029,16 @@ let receive_start2 ~dbg ~sr ~vdi_info ~id ~similar ~vm =
     let leaf = Local.VDI.create dbg sr vdi_info in
     info "Created leaf VDI for mirror receive: %s" (string_of_vdi_info leaf) ;
     on_fail := (fun () -> Local.VDI.destroy dbg sr leaf.vdi) :: !on_fail ;
-    let dummy = Local.VDI.snapshot dbg sr leaf in
-    on_fail := (fun () -> Local.VDI.destroy dbg sr dummy.vdi) :: !on_fail ;
-    debug "Created dummy snapshot for mirror receive: %s"
-      (string_of_vdi_info dummy) ;
+    let dummy_vdi =
+      if create_dummy then (
+        let d = Local.VDI.snapshot dbg sr leaf in
+        on_fail := (fun () -> Local.VDI.destroy dbg sr d.vdi) :: !on_fail ;
+        debug "Created dummy snapshot for mirror receive: %s"
+          (string_of_vdi_info d) ;
+        d.vdi
+      ) else
+        Vdi.of_string "NOVDI"
+    in
     let _ : backend = Local.VDI.attach3 dbg leaf_dp sr leaf.vdi vm true in
     Local.VDI.activate3 dbg leaf_dp sr leaf.vdi vm ;
     let nearest =
@@ -1092,11 +1094,12 @@ let receive_start2 ~dbg ~sr ~vdi_info ~id ~similar ~vm =
           Receive_state.
             {
               sr
-            ; dummy_vdi= dummy.vdi
+            ; dummy_vdi
             ; leaf_vdi= leaf.vdi
             ; leaf_dp
             ; parent_vdi= parent.vdi
             ; remote_vdi= vdi_info.vdi
+            ; mirror_vm= vm
             }
       ) ;
     let nearest_content_id = Option.map (fun x -> x.content_id) nearest in
@@ -1106,7 +1109,7 @@ let receive_start2 ~dbg ~sr ~vdi_info ~id ~similar ~vm =
       ; mirror_datapath= leaf_dp
       ; copy_diffs_from= nearest_content_id
       ; copy_diffs_to= parent.vdi
-      ; dummy_vdi= dummy.vdi
+      ; dummy_vdi
       }
   with e ->
     List.iter
@@ -1119,14 +1122,30 @@ let receive_start2 ~dbg ~sr ~vdi_info ~id ~similar ~vm =
     raise e
 
 let receive_start ~dbg ~sr ~vdi_info ~id ~similar =
-  receive_start2 ~dbg ~sr ~vdi_info ~id ~similar ~vm:(vm_of_s "0")
+  receive_start_common ~dbg ~sr ~create_dummy:true ~vdi_info ~id ~similar
+    ~vm:(vm_of_s "0") ()
+
+let receive_start2 ~dbg ~sr ~vdi_info ~id ~similar ~vm =
+  receive_start_common ~dbg ~sr ~vdi_info ~id ~similar ~vm ()
 
 let receive_finalize ~dbg ~id =
   let recv_state = State.find_active_receive_mirror id in
   let open State.Receive_state in
   Option.iter (fun r -> Local.DP.destroy dbg r.leaf_dp false) recv_state ;
   State.remove_receive_mirror id
 
+let receive_finalize2 ~dbg ~id =
+  let recv_state = State.find_active_receive_mirror id in
+  let open State.Receive_state in
+  Option.iter
+    (fun r ->
+      Local.DP.destroy2 dbg r.leaf_dp r.sr r.leaf_vdi r.mirror_vm false ;
+      Local.VDI.compose dbg r.sr r.parent_vdi r.leaf_vdi ;
+      Local.VDI.remove_from_sm_config dbg r.sr r.leaf_vdi "base_mirror"
+    )
+    recv_state ;
+  State.remove_receive_mirror id
+
 let receive_cancel ~dbg ~id =
   let receive_state = State.find_active_receive_mirror id in
   let open State.Receive_state in
@@ -1214,11 +1233,11 @@ let post_detach_hook ~sr ~vdi ~dp:_ =
          let t =
            Thread.create
              (fun () ->
-               debug "Calling receive_finalize" ;
+               debug "Calling receive_finalize2" ;
                log_and_ignore_exn (fun () ->
-                   Remote.DATA.MIRROR.receive_finalize "Mirror-cleanup" id
+                   Remote.DATA.MIRROR.receive_finalize2 "Mirror-cleanup" id
                ) ;
-               debug "Finished calling receive_finalize" ;
+               debug "Finished calling receive_finalize2" ;
                State.remove_local_mirror id ;
                debug "Removed active local mirror: %s" id
              )

diff --git a/ocaml/xapi/storage_mux.ml b/ocaml/xapi/storage_mux.ml
@@ -860,6 +860,10 @@ module Mux = struct
         with_dbg ~name:"DATA.MIRROR.receive_finalize" ~dbg
         @@ fun {log= dbg; _} -> Storage_migrate.receive_finalize ~dbg
 
+      let receive_finalize2 () ~dbg =
+        with_dbg ~name:"DATA.MIRROR.receive_finalize2" ~dbg
+        @@ fun {log= dbg; _} -> Storage_migrate.receive_finalize2 ~dbg
+
       let receive_cancel () ~dbg =
         with_dbg ~name:"DATA.MIRROR.receive_cancel" ~dbg @@ fun {log= dbg; _} ->
         Storage_migrate.receive_cancel ~dbg

diff --git a/ocaml/xapi/storage_smapiv1.ml b/ocaml/xapi/storage_smapiv1.ml
@@ -1236,6 +1236,8 @@ module SMAPIv1 : Server_impl = struct
 
       let receive_finalize _context ~dbg:_ ~id:_ = assert false
 
+      let receive_finalize2 _context ~dbg:_ ~id:_ = assert false
+
       let receive_cancel _context ~dbg:_ ~id:_ = assert false
 
       let get_nbd_server _context ~dbg:_ ~dp:_ ~sr:_ ~vdi:_ ~vm:_ = assert false

diff --git a/ocaml/xapi/storage_smapiv1_wrapper.ml b/ocaml/xapi/storage_smapiv1_wrapper.ml
@@ -1172,6 +1172,10 @@ functor
           info "DATA.MIRROR.receive_finalize dbg:%s id:%s" dbg id ;
           Impl.DATA.MIRROR.receive_finalize context ~dbg ~id
 
+        let receive_finalize2 context ~dbg ~id =
+          info "DATA.MIRROR.receive_finalize2 dbg:%s id:%s" dbg id ;
+          Impl.DATA.MIRROR.receive_finalize2 context ~dbg ~id
+
         let receive_cancel context ~dbg ~id =
           info "DATA.MIRROR.receive_cancel dbg:%s id:%s" dbg id ;
           Impl.DATA.MIRROR.receive_cancel context ~dbg ~id