Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

CA-388210: use unique datapaths for concurrent VDI copies #5920

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

CA-388210: use unique datapaths for concurrent VDI copies #5920

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
10053ee
CA-388210: drop unused domain parameter
edwintorok Aug 7, 2024
616b390
CA-388210: factor out computing the domain parameter
edwintorok Aug 7, 2024
adc1b0f
CA-388210: SMAPIv3 concurrency safety: send the (unique) datapath arg…
edwintorok Aug 7, 2024
96afeb7
CA-388210: SMAPIv3 concurrency: turn on concurrent operations by default
edwintorok Aug 7, 2024
772855f
fixup! CA-388210: SMAPIv3 concurrency safety: send the (unique) datap…
edwintorok Aug 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 43 additions & 29 deletions ocaml/xapi-storage-script/main.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,23 @@ let backend_backtrace_error name args backtrace =
let missing_uri () =
backend_error "MISSING_URI" ["Please include a URI in the device-config"]

(** return a unique 'domain' string for Dom0, so that we can plug disks
multiple times (e.g. for copy).

XAPI should give us a unique 'dp' (datapath) string, e.g. a UUID for storage migration,
or vbd/domid/device.
For regular guests keep the domain as passed by XAPI (an integer).
*)
let domain_of ~dp ~vm' =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requiring a prime in named label is a bit awkward - is there a reason this is not just ~vm?

let vm = Storage_interface.Vm.string_of vm' in
match vm with
| "0" ->
(* SM tries to use this in filesystem paths, so cannot have /,
and systemd might be a bit unhappy with - *)
"u0-" ^ dp |> String.map ~f:(function '/' | '-' -> '_' | c -> c)
| _ ->
vm

(** Functions to wrap calls to the above client modules and convert their
exceptions and errors into SMAPIv2 errors of type
[Storage_interface.Exception.exnty]. The above client modules should only
Expand Down Expand Up @@ -734,7 +751,7 @@ let vdi_of_volume x =
; persistent= true
}

let choose_datapath ?(persistent = true) domain response =
let choose_datapath ?(persistent = true) response =
(* We can only use a URI with a valid scheme, since we use the scheme
to name the datapath plugin. *)
let possible =
Expand Down Expand Up @@ -779,7 +796,7 @@ let choose_datapath ?(persistent = true) domain response =
| [] ->
return (Error (missing_uri ()))
| (script_dir, scheme, u) :: _us ->
return (Ok (fork_exec_rpc ~script_dir, scheme, u, domain))
return (Ok (fork_exec_rpc ~script_dir, scheme, u))

(* Bind the implementations *)
let bind ~volume_script_dir =
Expand Down Expand Up @@ -857,7 +874,7 @@ let bind ~volume_script_dir =
stat ~dbg ~sr ~vdi:temporary
)
>>= fun response ->
choose_datapath domain response >>= fun (rpc, _datapath, uri, domain) ->
choose_datapath response >>= fun (rpc, _datapath, uri) ->
return_data_rpc (fun () -> Datapath_client.attach (rpc ~dbg) dbg uri domain)
in
let wrap th = Rpc_async.T.put th in
Expand Down Expand Up @@ -1344,9 +1361,9 @@ let bind ~volume_script_dir =
|> wrap
in
S.VDI.introduce vdi_introduce_impl ;
let vdi_attach3_impl dbg _dp sr vdi' vm' _readwrite =
let vdi_attach3_impl dbg dp sr vdi' vm' _readwrite =
(let vdi = Storage_interface.Vdi.string_of vdi' in
let domain = Storage_interface.Vm.string_of vm' in
let domain = domain_of ~dp ~vm' in
vdi_attach_common dbg sr vdi domain >>>= fun response ->
let convert_implementation = function
| Xapi_storage.Data.XenDisk {params; extra; backend_type} ->
Expand All @@ -1368,9 +1385,9 @@ let bind ~volume_script_dir =
|> wrap
in
S.VDI.attach3 vdi_attach3_impl ;
let vdi_activate_common dbg sr vdi' vm' readonly =
let vdi_activate_common dbg dp sr vdi' vm' readonly =
(let vdi = Storage_interface.Vdi.string_of vdi' in
let domain = Storage_interface.Vm.string_of vm' in
let domain = domain_of ~dp ~vm' in
Attached_SRs.find sr >>>= fun sr ->
(* Discover the URIs using Volume.stat *)
stat ~dbg ~sr ~vdi >>>= fun response ->
Expand All @@ -1385,7 +1402,7 @@ let bind ~volume_script_dir =
stat ~dbg ~sr ~vdi:temporary
)
>>>= fun response ->
choose_datapath domain response >>>= fun (rpc, _datapath, uri, domain) ->
choose_datapath response >>>= fun (rpc, _datapath, uri) ->
return_data_rpc (fun () ->
let rpc = rpc ~dbg in
if readonly then
Expand All @@ -1396,17 +1413,17 @@ let bind ~volume_script_dir =
)
|> wrap
in
let vdi_activate3_impl dbg _dp sr vdi' vm' =
vdi_activate_common dbg sr vdi' vm' false
let vdi_activate3_impl dbg dp sr vdi' vm' =
vdi_activate_common dbg dp sr vdi' vm' false
in
S.VDI.activate3 vdi_activate3_impl ;
let vdi_activate_readonly_impl dbg _dp sr vdi' vm' =
vdi_activate_common dbg sr vdi' vm' true
let vdi_activate_readonly_impl dbg dp sr vdi' vm' =
vdi_activate_common dbg dp sr vdi' vm' true
in
S.VDI.activate_readonly vdi_activate_readonly_impl ;
let vdi_deactivate_impl dbg _dp sr vdi' vm' =
let vdi_deactivate_impl dbg dp sr vdi' vm' =
(let vdi = Storage_interface.Vdi.string_of vdi' in
let domain = Storage_interface.Vm.string_of vm' in
let domain = domain_of ~dp ~vm' in
Attached_SRs.find sr >>>= fun sr ->
(* Discover the URIs using Volume.stat *)
stat ~dbg ~sr ~vdi >>>= fun response ->
Expand All @@ -1420,17 +1437,17 @@ let bind ~volume_script_dir =
stat ~dbg ~sr ~vdi:temporary
)
>>>= fun response ->
choose_datapath domain response >>>= fun (rpc, _datapath, uri, domain) ->
choose_datapath response >>>= fun (rpc, _datapath, uri) ->
return_data_rpc (fun () ->
Datapath_client.deactivate (rpc ~dbg) dbg uri domain
)
)
|> wrap
in
S.VDI.deactivate vdi_deactivate_impl ;
let vdi_detach_impl dbg _dp sr vdi' vm' =
let vdi_detach_impl dbg dp sr vdi' vm' =
(let vdi = Storage_interface.Vdi.string_of vdi' in
let domain = Storage_interface.Vm.string_of vm' in
let domain = domain_of ~dp ~vm' in
Attached_SRs.find sr >>>= fun sr ->
(* Discover the URIs using Volume.stat *)
stat ~dbg ~sr ~vdi >>>= fun response ->
Expand All @@ -1444,7 +1461,7 @@ let bind ~volume_script_dir =
stat ~dbg ~sr ~vdi:temporary
)
>>>= fun response ->
choose_datapath domain response >>>= fun (rpc, _datapath, uri, domain) ->
choose_datapath response >>>= fun (rpc, _datapath, uri) ->
return_data_rpc (fun () -> Datapath_client.detach (rpc ~dbg) dbg uri domain)
)
|> wrap
Expand Down Expand Up @@ -1479,14 +1496,12 @@ let bind ~volume_script_dir =
|> wrap
in
S.SR.stat sr_stat_impl ;
let vdi_epoch_begin_impl dbg sr vdi' vm' persistent =
let vdi_epoch_begin_impl dbg sr vdi' _vm' persistent =
(let vdi = Storage_interface.Vdi.string_of vdi' in
let domain = Storage_interface.Vm.string_of vm' in
Attached_SRs.find sr >>>= fun sr ->
(* Discover the URIs using Volume.stat *)
stat ~dbg ~sr ~vdi >>>= fun response ->
choose_datapath ~persistent domain response
>>>= fun (rpc, datapath, uri, _domain) ->
choose_datapath ~persistent response >>>= fun (rpc, datapath, uri) ->
(* If non-persistent and the datapath plugin supports NONPERSISTENT
then we delegate this to the datapath plugin. Otherwise we will
make a temporary clone now and attach/detach etc this file. *)
Expand Down Expand Up @@ -1518,13 +1533,12 @@ let bind ~volume_script_dir =
|> wrap
in
S.VDI.epoch_begin vdi_epoch_begin_impl ;
let vdi_epoch_end_impl dbg sr vdi' vm' =
let vdi_epoch_end_impl dbg sr vdi' _vm' =
(let vdi = Storage_interface.Vdi.string_of vdi' in
let domain = Storage_interface.Vm.string_of vm' in
Attached_SRs.find sr >>>= fun sr ->
(* Discover the URIs using Volume.stat *)
stat ~dbg ~sr ~vdi >>>= fun response ->
choose_datapath domain response >>>= fun (rpc, datapath, uri, _domain) ->
choose_datapath response >>>= fun (rpc, datapath, uri) ->
if Datapath_plugins.supports_feature datapath _nonpersistent then
return_data_rpc (fun () -> Datapath_client.close (rpc ~dbg) dbg uri)
else
Expand All @@ -1547,9 +1561,9 @@ let bind ~volume_script_dir =
Deferred.Result.return () |> wrap
in
S.VDI.set_persistent vdi_set_persistent_impl ;
let dp_destroy2 dbg _dp sr vdi' vm' _allow_leak =
let dp_destroy2 dbg dp sr vdi' vm' _allow_leak =
(let vdi = Storage_interface.Vdi.string_of vdi' in
let domain = Storage_interface.Vm.string_of vm' in
let domain = domain_of ~dp ~vm' in
Attached_SRs.find sr >>>= fun sr ->
(* Discover the URIs using Volume.stat *)
stat ~dbg ~sr ~vdi >>>= fun response ->
Expand All @@ -1563,7 +1577,7 @@ let bind ~volume_script_dir =
stat ~dbg ~sr ~vdi:temporary
)
>>>= fun response ->
choose_datapath domain response >>>= fun (rpc, _datapath, uri, domain) ->
choose_datapath response >>>= fun (rpc, _datapath, uri) ->
return_data_rpc (fun () ->
Datapath_client.deactivate (rpc ~dbg) dbg uri domain
)
Expand Down Expand Up @@ -1695,7 +1709,7 @@ let rec diff a b =

(* default false due to bugs in SMAPIv3 plugins,
once they are fixed this should be set to true *)
let concurrent = ref false
let concurrent = ref true

let watch_volume_plugins ~volume_root ~switch_path ~pipe =
let create volume_plugin_name =
Expand Down
Loading