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 · Dec 10, 2024 · 74297c3 · 74297c3
1 parent 4bf00f7
commit 74297c3
Show file tree

Hide file tree

Showing 8 changed files with 68 additions and 13 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
@@ -1054,10 +1054,23 @@ 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. This fucntion
+      should be used in conjunction with [receive_start2] *)
+      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)
@@ -1420,6 +1433,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
@@ -1608,6 +1623,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.import_activate (fun dbg dp sr vdi vm ->
         Impl.DATA.MIRROR.import_activate () ~dbg ~dp ~sr ~vdi ~vm

diff --git a/ocaml/xapi-idl/storage/storage_skeleton.ml b/ocaml/xapi-idl/storage/storage_skeleton.ml
@@ -172,6 +172,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
@@ -1836,6 +1836,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.TASK.cancel (u "TASK.cancel") ;
   S.VDI.attach (u "VDI.attach") ;

diff --git a/ocaml/xapi/storage_migrate.ml b/ocaml/xapi/storage_migrate.ml
@@ -38,6 +38,7 @@ module State = struct
       ; leaf_dp: dp
       ; parent_vdi: Vdi.t
       ; remote_vdi: Vdi.t
+      ; mirror_vm: Vm.t
     }
     [@@deriving rpcty]
 
@@ -761,7 +762,7 @@ module MigrateLocal = struct
         ; watchdog= None
         }
     in
-
+ 
     State.add mirror_id (State.Send_op alm) ;
     debug "%s Added mirror %s to active local mirrors" __FUNCTION__ mirror_id ;
     (* A list of cleanup actions to perform if the operation should fail. *)
@@ -920,15 +921,9 @@ module MigrateLocal = struct
       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 mirror_id)
@@ -1127,7 +1122,7 @@ end
 (** module [MigrateRemote] is similar to [MigrateLocal], but most of these functions
 tend to be executed on the receiver side. *)
 module MigrateRemote = struct
-  let receive_start2 ~dbg ~sr ~vdi_info ~id ~similar ~vm =
+  let receive_start_common ~dbg ~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 *)
@@ -1138,9 +1133,11 @@ module MigrateRemote = struct
       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 ;
+      (* dummy VDI is created so that the leaf VDI becomes a differencing disk,
+         useful for calling VDI.compose later on*)
       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"
+      debug "%s Created dummy snapshot for mirror receive: %s" __FUNCTION__
         (string_of_vdi_info dummy) ;
       let _ : backend = Local.VDI.attach3 dbg leaf_dp sr leaf.vdi vm true in
       Local.VDI.activate3 dbg leaf_dp sr leaf.vdi vm ;
@@ -1202,6 +1199,7 @@ module MigrateRemote = struct
               ; 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
@@ -1224,14 +1222,37 @@ module MigrateRemote = struct
       raise e
 
   let receive_start ~dbg ~sr ~vdi_info ~id ~similar =
-    receive_start2 ~dbg ~sr ~vdi_info ~id ~similar ~vm:(Vm.of_string "0")
+    receive_start_common ~dbg ~sr ~vdi_info ~id ~similar ~vm:(Vm.of_string "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 ->
+        SXM.info
+          "%s Mirror done. Compose on the dest sr %s parent %s and leaf %s"
+          __FUNCTION__ (Sr.string_of r.sr)
+          (Vdi.string_of r.parent_vdi)
+          (Vdi.string_of r.leaf_vdi) ;
+        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 ;
+        (* On SMAPIv3, compose would have removed the now invalid dummy vdi, so
+           there is no need to destroy it anymore, while this is necessary on SMAPIv1 SRs. *)
+        log_and_ignore_exn (fun () -> Local.VDI.destroy dbg r.sr r.dummy_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
@@ -1320,11 +1341,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
              )
@@ -1424,6 +1445,8 @@ let receive_start2 = MigrateRemote.receive_start2
 
 let receive_finalize = MigrateRemote.receive_finalize
 
+let receive_finalize2 = MigrateRemote.receive_finalize2
+
 let receive_cancel = MigrateRemote.receive_cancel
 
 (* The remote end of this call, SR.update_snapshot_info_dest, is implemented in

diff --git a/ocaml/xapi/storage_mux.ml b/ocaml/xapi/storage_mux.ml
@@ -863,6 +863,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
@@ -1231,6 +1231,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 import_activate _context ~dbg:_ ~dp:_ ~sr:_ ~vdi:_ ~vm:_ =

diff --git a/ocaml/xapi/storage_smapiv1_wrapper.ml b/ocaml/xapi/storage_smapiv1_wrapper.ml
@@ -1167,6 +1167,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