Merge pull request #5023 from robhoes/sxm

CP-42064: Fix storage migration for NBD-backed storage
xapi-project · May 25, 2023 · c545492 · c545492
2 parents 65a1dbf + 5d02ca1
commit c545492
Show file tree

Hide file tree

Showing 5 changed files with 91 additions and 55 deletions.
diff --git a/ocaml/tapctl/tapctl.ml b/ocaml/tapctl/tapctl.ml
@@ -73,6 +73,8 @@ type driver = Vhd | Aio
 
 let string_of_driver = function Vhd -> "vhd" | Aio -> "aio"
 
+let tapdev_of ~pid ~minor = {minor; tapdisk_pid= pid}
+
 (* DUMMY MODE FUNCTIONS *)
 
 let get_minor tapdev = tapdev.minor

diff --git a/ocaml/tapctl/tapctl.mli b/ocaml/tapctl/tapctl.mli
@@ -33,6 +33,8 @@ val tapdev_of_rpc : Rpc.t -> tapdev
 
 val rpc_of_tapdev : tapdev -> Rpc.t
 
+val tapdev_of : pid:int -> minor:int -> tapdev
+
 val get_minor : tapdev -> int
 
 val get_tapdisk_pid : tapdev -> int

diff --git a/ocaml/xapi/attach_helpers.ml b/ocaml/xapi/attach_helpers.ml
@@ -132,3 +132,44 @@ let with_vbds rpc session_id __context vm vdis mode f =
             !vbds
       )
     )
+
+module NbdClient = struct
+  let print_fork_error f =
+    try f ()
+    with Forkhelpers.Spawn_internal_error (stderr, stdout, status) as e -> (
+      match status with
+      | Unix.WEXITED n ->
+          error "Forkhelpers.Spawn_internal_error(%s, %s, WEXITED %d)" stderr
+            stdout n ;
+          raise e
+      | Unix.WSIGNALED n ->
+          error "Forkhelpers.Spawn_internal_error(%s, %s, WSIGNALED %d)" stderr
+            stdout n ;
+          raise e
+      | Unix.WSTOPPED n ->
+          error "Forkhelpers.Spawn_internal_error(%s, %s, WSTOPPED %d)" stderr
+            stdout n ;
+          raise e
+    )
+
+  let run_command cmd args =
+    debug "running %s %s" cmd (String.concat " " args) ;
+    let stdout, _ =
+      print_fork_error (fun () ->
+          Forkhelpers.execute_command_get_output cmd args
+      )
+    in
+    stdout
+
+  let start_nbd_client ~unix_socket_path ~export_name =
+    run_command
+      !Xapi_globs.nbd_client_manager_script
+      ["connect"; "--path"; unix_socket_path; "--exportname"; export_name]
+    |> String.trim
+
+  let stop_nbd_client ~nbd_device =
+    run_command
+      !Xapi_globs.nbd_client_manager_script
+      ["disconnect"; "--device"; nbd_device]
+    |> ignore
+end
diff --git a/ocaml/xapi/storage_migrate.ml b/ocaml/xapi/storage_migrate.ml
@@ -323,12 +323,12 @@ module Local = StorageAPI (Idl.Exn.GenClient (struct
 end))
 
 let tapdisk_of_attach_info (backend : Storage_interface.backend) =
-  let xendisks, _, _, _ =
+  let _, blockdevices, _, nbds =
     Storage_interface.implementations_of_backend backend
   in
-  match xendisks with
-  | xendisk :: _ -> (
-      let path = xendisk.Storage_interface.params in
+  match (blockdevices, nbds) with
+  | blockdevice :: _, _ -> (
+      let path = blockdevice.Storage_interface.path in
       try
         match Tapctl.of_device (Tapctl.create ()) path with
         | tapdev, _, _ ->
@@ -344,8 +344,19 @@ let tapdisk_of_attach_info (backend : Storage_interface.backend) =
           debug "Device %s has an unknown driver" path ;
           None
     )
-  | [] ->
-      debug "No XenDisk implementation in backend: %s"
+  | _, nbd :: _ -> (
+    try
+      let path, _ = Storage_interface.parse_nbd_uri nbd in
+      let filename = Unix.realpath path |> Filename.basename in
+      Scanf.sscanf filename "nbd%d.%d" (fun pid minor ->
+          Some (Tapctl.tapdev_of ~pid ~minor)
+      )
+    with _ ->
+      debug "No tapdisk found for NBD backend: %s" nbd.Storage_interface.uri ;
+      None
+  )
+  | _ ->
+      debug "No tapdisk found for backend: %s"
         (Storage_interface.(rpc_of backend) backend |> Rpc.to_string) ;
       None
 
@@ -360,23 +371,32 @@ let with_activated_disk ~dbg ~sr ~vdi ~dp f =
   in
   finally
     (fun () ->
-      let path =
+      let path_and_nbd =
         Option.map
           (fun (vdi, backend) ->
-            let _, blockdevs, files, _ =
+            let _xendisks, blockdevs, files, nbds =
               Storage_interface.implementations_of_backend backend
             in
-            match (blockdevs, files) with
-            | {path} :: _, _ | _, {path} :: _ ->
+            match (files, blockdevs, nbds) with
+            | {path} :: _, _, _ | _, {path} :: _, _ ->
+                Local.VDI.activate3 dbg dp sr vdi (vm_of_s "0") ;
+                (path, false)
+            | _, _, nbd :: _ ->
                 Local.VDI.activate3 dbg dp sr vdi (vm_of_s "0") ;
-                path
-            | [], [] ->
+                let unix_socket_path, export_name =
+                  Storage_interface.parse_nbd_uri nbd
+                in
+                ( Attach_helpers.NbdClient.start_nbd_client ~unix_socket_path
+                    ~export_name
+                , true
+                )
+            | [], [], [] ->
                 raise
                   (Storage_interface.Storage_error
                      (Backend_error
                         ( Api_errors.internal_error
                         , [
-                            "No BlockDevice or File implementation in \
+                            "No File, BlockDevice or Nbd implementation in \
                              Datapath.attach response: "
                             ^ (Storage_interface.(rpc_of backend) backend
                               |> Jsonrpc.to_string
@@ -389,8 +409,16 @@ let with_activated_disk ~dbg ~sr ~vdi ~dp f =
           attached_vdi
       in
       finally
-        (fun () -> f path)
+        (fun () -> f (Option.map (function path, _ -> path) path_and_nbd))
         (fun () ->
+          Option.iter
+            (function
+              | path, true ->
+                  Attach_helpers.NbdClient.stop_nbd_client ~nbd_device:path
+              | _ ->
+                  ()
+              )
+            path_and_nbd ;
           Option.iter
             (fun vdi -> Local.VDI.deactivate dbg dp sr vdi (vm_of_s "0"))
             vdi

diff --git a/ocaml/xapi/xapi_vbd.ml b/ocaml/xapi/xapi_vbd.ml
@@ -26,45 +26,6 @@ let update_allowed_operations ~__context ~self : unit =
 let assert_attachable ~__context ~self : unit =
   assert_attachable ~__context ~self
 
-let print_fork_error f =
-  try f ()
-  with Forkhelpers.Spawn_internal_error (stderr, stdout, status) as e -> (
-    match status with
-    | Unix.WEXITED n ->
-        error "Forkhelpers.Spawn_internal_error(%s, %s, WEXITED %d)" stderr
-          stdout n ;
-        raise e
-    | Unix.WSIGNALED n ->
-        error "Forkhelpers.Spawn_internal_error(%s, %s, WSIGNALED %d)" stderr
-          stdout n ;
-        raise e
-    | Unix.WSTOPPED n ->
-        error "Forkhelpers.Spawn_internal_error(%s, %s, WSTOPPED %d)" stderr
-          stdout n ;
-        raise e
-  )
-
-let run_command cmd args =
-  debug "running %s %s" cmd (String.concat " " args) ;
-  let stdout, _ =
-    print_fork_error (fun () -> Forkhelpers.execute_command_get_output cmd args)
-  in
-  stdout
-
-module NbdClient = struct
-  let start_nbd_client ~unix_socket_path ~export_name =
-    run_command
-      !Xapi_globs.nbd_client_manager_script
-      ["connect"; "--path"; unix_socket_path; "--exportname"; export_name]
-    |> String.trim
-
-  let stop_nbd_client ~nbd_device =
-    run_command
-      !Xapi_globs.nbd_client_manager_script
-      ["disconnect"; "--device"; nbd_device]
-    |> ignore
-end
-
 let set_mode ~__context ~self ~value =
   let vm = Db.VBD.get_VM ~__context ~self in
   Xapi_vm_lifecycle.assert_initial_power_state_is ~__context ~self:vm
@@ -96,7 +57,8 @@ let plug ~__context ~self =
               let unix_socket_path, export_name =
                 Storage_interface.parse_nbd_uri nbd
               in
-              NbdClient.start_nbd_client ~unix_socket_path ~export_name
+              Attach_helpers.NbdClient.start_nbd_client ~unix_socket_path
+                ~export_name
           | [], [], [] ->
               raise
                 (Storage_interface.Storage_error
@@ -145,7 +107,8 @@ let unplug ~__context ~self =
     let device = Db.VBD.get_device ~__context ~self in
     let nbd_device_prefix = "nbd" in
     let is_nbd = Astring.String.is_prefix ~affix:nbd_device_prefix device in
-    if is_nbd then NbdClient.stop_nbd_client ~nbd_device:("/dev/" ^ device) ;
+    if is_nbd then
+      Attach_helpers.NbdClient.stop_nbd_client ~nbd_device:("/dev/" ^ device) ;
     Storage_access.deactivate_and_detach ~__context ~vbd:self ~domid ;
     Db.VBD.set_currently_attached ~__context ~self ~value:false
   ) else