Skip to content

CP-54207: Move VBD_attach outside of VM migrate downtime #6489

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

CP-54207: Move VBD_attach outside of VM migrate downtime #6489

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3ae8ff9
Use just id_of vbd for attached_vdis key instead of the (VM, VBD) tuple
snwoods Apr 2, 2025
5351b0b
CP-54207: Move VBD_attach outside of VM migrate downtime
snwoods Apr 2, 2025
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
1 change: 1 addition & 0 deletions ocaml/xapi-idl/xen/xenops_interface.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,6 +303,7 @@ module Vbd = struct
; extra_private_keys: (string * string) list [@default []]
; qos: qos option [@default None]
; persistent: bool [@default true]
; can_attach_early: bool [@default false]
}
[@@deriving rpcty]

Expand Down
12 changes: 12 additions & 0 deletions ocaml/xapi/xapi_sr.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1080,3 +1080,15 @@ let get_live_hosts ~__context ~sr =
Xapi_vm_helpers.assert_can_see_specified_SRs ~__context ~reqd_srs:[sr] ~host
in
Xapi_vm_helpers.possible_hosts ~__context ~choose_fn ()

let required_api_version_of_sr ~__context ~sr =
let sr_type = Db.SR.get_type ~__context ~self:sr in
let expr =
Xapi_database.Db_filter_types.(Eq (Field "type", Literal sr_type))
in
match Db.SM.get_records_where ~__context ~expr with
| (_, sm) :: _ ->
Some sm.API.sM_required_api_version
| [] ->
warn "Couldn't find SM with type %s" sr_type ;
None
30 changes: 30 additions & 0 deletions ocaml/xapi/xapi_xenops.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -553,6 +553,10 @@ let list_net_sriov_vf_pcis ~__context ~vm =
None
)

module StringMap = Map.Make (String)

let sr_version_cache = ref StringMap.empty

module MD = struct
(** Convert between xapi DB records and xenopsd records *)

Expand Down Expand Up @@ -684,6 +688,31 @@ module MD = struct
) else
disk_of_vdi ~__context ~self:vbd.API.vBD_VDI
in
let can_attach_early =
let sr_opt =
try Some (Db.VDI.get_SR ~__context ~self:vbd.API.vBD_VDI)
with _ -> None
in
match sr_opt with
| Some sr -> (
let sr_key = Ref.string_of sr in
match StringMap.find_opt sr_key !sr_version_cache with
| Some cached_api_version ->
Version.String.ge cached_api_version "3.0"
| None -> (
match Xapi_sr.required_api_version_of_sr ~__context ~sr with
| Some api_version ->
sr_version_cache :=
StringMap.add sr_key api_version !sr_version_cache ;
Version.String.ge api_version "3.0"
| None ->
false
)
)
| None ->
(* If we can't get the SR, we have to default to false *)
false
in
{
id= (vm.API.vM_uuid, Device_number.to_linux_device device_number)
; position= Some device_number
Expand All @@ -707,6 +736,7 @@ module MD = struct
( try Db.VDI.get_on_boot ~__context ~self:vbd.API.vBD_VDI = `persist
with _ -> true
)
; can_attach_early
}

let of_pvs_proxy ~__context vif proxy =
Expand Down
1 change: 1 addition & 0 deletions ocaml/xenopsd/cli/xn.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,6 +278,7 @@ let vbd_of_disk_info vm_id info =
; extra_private_keys= []
; qos= None
; persistent= true
; can_attach_early= false
}

let print_disk vbd =
Expand Down
55 changes: 45 additions & 10 deletions ocaml/xenopsd/lib/xenops_server.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1763,7 +1763,8 @@ let rec atomics_of_operation = function
serial "VIF.activate_and_plug" ~id
[VIF_set_active (vif.Vif.id, true); VIF_plug vif.Vif.id]
)
| VM_restore_devices (id, restore_vifs) ->
| VM_restore_devices (id, migration) ->
let restore_vifs = not migration in
let vbds_rw, vbds_ro = VBD_DB.vbds id |> vbd_plug_sets in
let vgpus = VGPU_DB.vgpus id in
let pcis = PCI_DB.pcis id |> pci_plug_order in
Expand All @@ -1773,8 +1774,22 @@ let rec atomics_of_operation = function
let name_multi = pf "VBDs.activate_and_plug %s" typ in
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]
(* When migrating, attach early if the vbd's SM allows it.
Note: there is a bug here for SxM if migrating between API
versions as the Vbd's new SR won't have propagated to xenopsd
yet. This means can_attach_early will be based on the origin SR.
This is a non-issue as v1 <-> v3 migration is still experimental
and v1 is already early-attaching in SxM through mirroring.
*)
if
migration
&& (not !xenopsd_vbd_plug_unplug_legacy)
&& vbd.Vbd.can_attach_early
then
[VBD_activate vbd.Vbd.id]
else
serial name_one ~id
[VBD_set_active (vbd.Vbd.id, true); vbd_plug vbd.Vbd.id]
)
in
[
Expand Down Expand Up @@ -1897,7 +1912,7 @@ let rec atomics_of_operation = function
]
; vgpu_start_operations
; [VM_restore (id, data, vgpu_data)]
; atomics_of_operation (VM_restore_devices (id, true))
; atomics_of_operation (VM_restore_devices (id, false))
; [
(* At this point the domain is considered survivable. *)
VM_set_domain_action_request (id, None)
Expand Down Expand Up @@ -2696,9 +2711,9 @@ and perform_exn ?result (op : operation) (t : Xenops_task.task_handle) : unit =
| VM_restore_vifs id ->
debug "VM_restore_vifs %s" id ;
perform_atomics (atomics_of_operation op) t
| VM_restore_devices (id, restore_vifs) ->
| VM_restore_devices (id, migration) ->
(* XXX: this is delayed due to the 'attach'/'activate' behaviour *)
debug "VM_restore_devices %s %b" id restore_vifs ;
debug "VM_restore_devices %s %b" id migration ;
perform_atomics (atomics_of_operation op) t
| VM_resume (id, _data) ->
debug "VM.resume %s" id ;
Expand Down Expand Up @@ -3022,11 +3037,31 @@ and perform_exn ?result (op : operation) (t : Xenops_task.task_handle) : unit =
( try
let no_sharept = VGPU_DB.vgpus id |> List.exists is_no_sharept in
debug "VM %s no_sharept=%b (%s)" id no_sharept __LOC__ ;
(* If plug is split into activate and attach, we could attach
early so that it is outside of the VM downtime (if the SM
supports this) *)
let early_attach =
parallel_map "VBDs.set_active_and_attach" ~id (VBD_DB.vbds id)
(fun vbd ->
if
(not !xenopsd_vbd_plug_unplug_legacy)
&& vbd.Vbd.can_attach_early
then
serial "VBD.set_active_and_attach" ~id
[
VBD_set_active (vbd.Vbd.id, true)
; VBD_attach vbd.Vbd.id
]
else
[]
)
in
perform_atomics
([VM_create (id, Some memory_limit, Some final_id, no_sharept)]
@ (* Perform as many operations as possible on the destination
domain before pausing the original domain *)
atomics_of_operation (VM_restore_vifs id)
(* Perform as many operations as possible on the destination
domain before pausing the original domain *)
@ atomics_of_operation (VM_restore_vifs id)
@ early_attach
)
t ;
Handshake.send s Handshake.Success
Expand Down Expand Up @@ -3142,7 +3177,7 @@ and perform_exn ?result (op : operation) (t : Xenops_task.task_handle) : unit =
) ;
debug "VM.receive_memory: restoring remaining devices and unpausing" ;
perform_atomics
(atomics_of_operation (VM_restore_devices (final_id, false))
(atomics_of_operation (VM_restore_devices (final_id, true))
@ [
VM_unpause final_id
; VM_set_domain_action_request (final_id, None)
Expand Down
16 changes: 10 additions & 6 deletions ocaml/xenopsd/xc/xenops_server_xen.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@ module VmExtra = struct
; pv_drivers_detected: bool [@default false]
; xen_platform: (int * int) option (* (device_id, revision) for QEMU *)
; platformdata: (string * string) list [@default []]
; attached_vdis: (Vbd.id * attached_vdi) list [@default []]
; attached_vdis: (string * attached_vdi) list [@default []]
Copy link
Contributor Author

@snwoods snwoods May 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure if we would need to do anything special when updating to this?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I follow the explanation of the commit, this was an ID of a Virtual Block Device (VBD), not a VM ID. Are you sure this is the right thing to do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AIUI, a Vbd.id is actually a tuple (string * string). The first is the vm id and the second is the vbd id.

We're using the vmextra DB (indexed by VM id) and store the (Vbd.id, vdi) in attached_vdis after VBD_attach and retrieve this value to use in VBD_activate. Temporary VM ids are used during migration but currently this doesn't matter as we do activate immediately after attach, so the VM ids are the same.

In this PR, we are doing attach earlier, so the VM id it's using at this point is something like xxxx-00000001, so when we later try to retrieve this value for the VBD_attach, the VM id has changed to xxxx-xxxxxxxxx so we can't find it.

In VM_rename, the VM id key xxxx-00000001 is correctly updated to xxxx-xxxxxxxxx. However, if we're using Vbd.id in attached_vdis, this isn't updated so we still fail to find it as the key is (xxxx-00000001, <vbd_id>) rather than (xxxx-xxxxxxxxx, <vbd_id>). As vmextra is already indexed by VM, using the full vbd.id is unnecessary, so we can avoid this whole issue by not using the VM id part of vbd.id.

}
[@@deriving rpcty]

Expand Down Expand Up @@ -3682,9 +3682,13 @@ module VBD = struct
persistent=
{
vm_t.VmExtra.persistent with
(* Index by id_of vbd rather than vbd.id as VmExtra is
already indexed by VM id, so the VM id part of
vbd.id is unnecessary and causes issues finding the
attached_vdi when the VM is renamed. *)
attached_vdis=
(vbd.Vbd.id, vdi)
:: List.remove_assoc vbd.Vbd.id
(id_of vbd, vdi)
:: List.remove_assoc (id_of vbd)
vm_t.persistent.attached_vdis
}
}
Expand All @@ -3706,7 +3710,7 @@ module VBD = struct

let activate task vm vbd =
let vmextra = DB.read_exn vm in
match List.assoc_opt vbd.id vmextra.persistent.attached_vdis with
match List.assoc_opt (id_of vbd) vmextra.persistent.attached_vdis with
| None ->
debug "No attached_vdi info, so not activating"
| Some vdi ->
Expand Down Expand Up @@ -3857,7 +3861,7 @@ module VBD = struct
)
vm
)
(fun () -> cleanup_attached_vdis vm vbd.id)
(fun () -> cleanup_attached_vdis vm (id_of vbd))

let deactivate task vm vbd force =
with_xc_and_xs (fun xc xs ->
Expand Down Expand Up @@ -4021,7 +4025,7 @@ module VBD = struct
| _ ->
()
) ;
cleanup_attached_vdis vm vbd.id
cleanup_attached_vdis vm (id_of vbd)

let insert task vm vbd d =
on_frontend
Expand Down
Loading