From a4e55f52d859be2a6935752980fe25c82d83ef7f Mon Sep 17 00:00:00 2001 From: Andrii Sultanov Date: Fri, 13 Dec 2024 12:03:58 +0000 Subject: [PATCH 1/3] IH-747 - xapi/import: Don't lie in VDI import logs Instead of logging errors like this and then immediately handling them: ``` [error|1141 |VM metadata import |import] Found no VDI with location = dedeeb44-62b3-460e-b55c-6de45ba10cc0: treating as fatal and abandoning import [debug|1141 |VM metadata import |import] Ignoring missing disk Ref:4 - this will be mirrored during a real live migration. ``` Log once in the handler: ``` [ warn|VM metadata import |import] Ignoring missing disk Ref:16 - this will be mirrored during a real live migration. (Suppressed error: 'Found no VDI with location = c208b47c-cf87-495f-bd3c-a4bc8167ef83') ``` Signed-off-by: Andrii Sultanov --- ocaml/xapi/import.ml | 50 +++++++++++++++++++++++++------------------- 1 file changed, 28 insertions(+), 22 deletions(-) diff --git a/ocaml/xapi/import.ml b/ocaml/xapi/import.ml index 2a9d37b7972..39e069b2768 100644 --- a/ocaml/xapi/import.ml +++ b/ocaml/xapi/import.ml @@ -939,7 +939,7 @@ module VDI : HandlerTools = struct | Found_iso of API.ref_VDI | Found_no_iso | Found_disk of API.ref_VDI - | Found_no_disk of exn + | Found_no_disk of string * exn | Skip | Create of API.vDI_t @@ -1090,27 +1090,31 @@ module VDI : HandlerTools = struct | Some vdi -> Found_disk vdi | None -> - error "Found no VDI with location = %s: %s" - vdi_record.API.vDI_location - ( if config.force then - "ignoring error because '--force' is set" - else - "treating as fatal and abandoning import" - ) ; - if config.force then - Skip - else if exists vdi_record.API.vDI_SR state.table then + let error_string = + Printf.sprintf "Found no VDI with location = %s%s" + vdi_record.API.vDI_location + ( if config.force then + ": ignoring error because '--force' is set" + else + "" + ) + in + if config.force then ( + warn "%s" error_string ; Skip + ) else if exists vdi_record.API.vDI_SR state.table then let sr = lookup vdi_record.API.vDI_SR state.table in Found_no_disk - (Api_errors.Server_error - ( Api_errors.vdi_location_missing - , [Ref.string_of sr; vdi_record.API.vDI_location] - ) + ( error_string + , Api_errors.Server_error + ( Api_errors.vdi_location_missing + , [Ref.string_of sr; vdi_record.API.vDI_location] + ) ) else Found_no_disk - (Api_errors.Server_error - (Api_errors.vdi_content_id_missing, []) + ( error_string + , Api_errors.Server_error + (Api_errors.vdi_content_id_missing, []) ) ) ) @@ -1123,18 +1127,19 @@ module VDI : HandlerTools = struct state.table <- (x.cls, x.id, Ref.string_of vdi) :: state.table | Found_no_iso -> () (* VDI will be ejected. *) - | Found_no_disk e -> ( + | Found_no_disk (error_string, e) -> ( match config.import_type with | Metadata_import {live= true; _} -> (* We expect the disk to be missing during a live migration dry run. *) - debug + info "Ignoring missing disk %s - this will be mirrored during a real \ - live migration." - x.id ; + live migration. (Suppressed error: '%s')" + x.id error_string ; (* Create a dummy disk in the state table so the VBD import has a disk to look up. *) let dummy_vdi = Ref.make () in state.table <- (x.cls, x.id, Ref.string_of dummy_vdi) :: state.table | _ -> + error "%s - treating as fatal and abandoning import" error_string ; raise e ) | Skip -> @@ -1161,7 +1166,8 @@ module VDI : HandlerTools = struct with Not_found -> () ) Xapi_globs.vdi_other_config_sync_keys - | Found_no_disk e -> + | Found_no_disk (error_string, e) -> + error "%s - treating as fatal and abandoning import" error_string ; raise e | Create vdi_record -> (* Make a new VDI for streaming data into; adding task-id to sm-config on VDI.create so SM backend can see this is an import *) From cfc6594e1d2a70660a7800882b07892af63a3577 Mon Sep 17 00:00:00 2001 From: Andrii Sultanov Date: Fri, 13 Dec 2024 15:41:53 +0000 Subject: [PATCH 2/3] IH-747 - database: Add an internal get_by_uuid_opt method Instead of raising an exception in case of an error like get_by_uuid, return None to be handled gracefully later. Do not expose it in the datamodel. This will later be used when an object is checked to exist before its creation (during migration, for example), and so its absence is expected - no need to raise a backtrace and pollute the logs with errors. Signed-off-by: Andrii Sultanov --- ocaml/database/db_cache_impl.ml | 15 +++++++++ ocaml/database/db_interface.ml | 5 +++ ocaml/database/db_rpc_client_v1.ml | 4 +++ ocaml/database/db_rpc_client_v2.ml | 7 +++++ ocaml/database/db_rpc_common_v1.ml | 2 ++ ocaml/database/db_rpc_common_v2.ml | 1 + ocaml/idl/ocaml_backend/gen_db_actions.ml | 37 ++++++++++++++++++++++- 7 files changed, 70 insertions(+), 1 deletion(-) diff --git a/ocaml/database/db_cache_impl.ml b/ocaml/database/db_cache_impl.ml index b4f23b0af00..4c4f33b728a 100644 --- a/ocaml/database/db_cache_impl.ml +++ b/ocaml/database/db_cache_impl.ml @@ -240,6 +240,21 @@ let db_get_by_uuid t tbl uuid_val = | _ -> raise (Too_many_values (tbl, "", uuid_val)) +let db_get_by_uuid_opt t tbl uuid_val = + match + read_field_where t + { + table= tbl + ; return= Db_names.ref + ; where_field= Db_names.uuid + ; where_value= uuid_val + } + with + | [r] -> + Some r + | _ -> + None + (** Return reference fields from tbl that matches specified name_label field *) let db_get_by_name_label t tbl label = read_field_where t diff --git a/ocaml/database/db_interface.ml b/ocaml/database/db_interface.ml index 834c12cd8a1..081abc687bd 100644 --- a/ocaml/database/db_interface.ml +++ b/ocaml/database/db_interface.ml @@ -56,6 +56,11 @@ module type DB_ACCESS = sig (** [db_get_by_uuid tbl uuid] returns the single object reference associated with [uuid] *) + val db_get_by_uuid_opt : Db_ref.t -> string -> string -> string option + (** [db_get_by_uuid_opt tbl uuid] returns [Some obj] with the single object + reference associated with [uuid] if one exists and [None] otherwise, + instead of raising an exception like [get_by_uuid] *) + val db_get_by_name_label : Db_ref.t -> string -> string -> string list (** [db_get_by_name_label tbl label] returns the list of object references associated with [label] *) diff --git a/ocaml/database/db_rpc_client_v1.ml b/ocaml/database/db_rpc_client_v1.ml index ecde5c4060b..7adbcd6bbed 100644 --- a/ocaml/database/db_rpc_client_v1.ml +++ b/ocaml/database/db_rpc_client_v1.ml @@ -88,6 +88,10 @@ functor do_remote_call marshall_db_get_by_uuid_args unmarshall_db_get_by_uuid_response "db_get_by_uuid" (t, u) + let db_get_by_uuid_opt _ t u = + do_remote_call marshall_db_get_by_uuid_args + unmarshall_db_get_by_uuid_opt_response "db_get_by_uuid_opt" (t, u) + let db_get_by_name_label _ t l = do_remote_call marshall_db_get_by_name_label_args unmarshall_db_get_by_name_label_response "db_get_by_name_label" (t, l) diff --git a/ocaml/database/db_rpc_client_v2.ml b/ocaml/database/db_rpc_client_v2.ml index 3c85dd82fcf..3a32b3149e9 100644 --- a/ocaml/database/db_rpc_client_v2.ml +++ b/ocaml/database/db_rpc_client_v2.ml @@ -77,6 +77,13 @@ functor | _ -> raise Remote_db_server_returned_bad_message + let db_get_by_uuid_opt _ t u = + match process (Request.Db_get_by_uuid (t, u)) with + | Response.Db_get_by_uuid_opt y -> + y + | _ -> + raise Remote_db_server_returned_bad_message + let db_get_by_name_label _ t l = match process (Request.Db_get_by_name_label (t, l)) with | Response.Db_get_by_name_label y -> diff --git a/ocaml/database/db_rpc_common_v1.ml b/ocaml/database/db_rpc_common_v1.ml index 1966595938f..cced73dd9ca 100644 --- a/ocaml/database/db_rpc_common_v1.ml +++ b/ocaml/database/db_rpc_common_v1.ml @@ -194,6 +194,8 @@ let marshall_db_get_by_uuid_response s = XMLRPC.To.string s let unmarshall_db_get_by_uuid_response xml = XMLRPC.From.string xml +let unmarshall_db_get_by_uuid_opt_response xml = unmarshall_stringopt xml + (* db_get_by_name_label *) let marshall_db_get_by_name_label_args (s1, s2) = marshall_2strings (s1, s2) diff --git a/ocaml/database/db_rpc_common_v2.ml b/ocaml/database/db_rpc_common_v2.ml index 5ecf1b3e797..4cd9d7541ab 100644 --- a/ocaml/database/db_rpc_common_v2.ml +++ b/ocaml/database/db_rpc_common_v2.ml @@ -59,6 +59,7 @@ module Response = struct | Find_refs_with_filter of string list | Read_field_where of string list | Db_get_by_uuid of string + | Db_get_by_uuid_opt of string option | Db_get_by_name_label of string list | Create_row of unit | Delete_row of unit diff --git a/ocaml/idl/ocaml_backend/gen_db_actions.ml b/ocaml/idl/ocaml_backend/gen_db_actions.ml index 06f54f228ba..86e9f426883 100644 --- a/ocaml/idl/ocaml_backend/gen_db_actions.ml +++ b/ocaml/idl/ocaml_backend/gen_db_actions.ml @@ -93,6 +93,9 @@ let dm_to_string tys : O.Module.t = "fun x -> x |> SecretString.rpc_of_t |> Rpc.string_of_rpc" | DT.Record _ -> failwith "record types never stored in the database" + | DT.Option (DT.Ref _ as ty) -> + String.concat "" + ["fun s -> set "; OU.alias_of_ty ty; "(Option.to_list s)"] | DT.Option _ -> failwith "option types never stored in the database" in @@ -148,6 +151,13 @@ let string_to_dm tys : O.Module.t = "SecretString.of_string" | DT.Record _ -> failwith "record types never stored in the database" + | DT.Option (DT.Ref _ as ty) -> + String.concat "" + [ + "fun s -> match set " + ; OU.alias_of_ty ty + ; " s with [] -> None | x::_ -> Some x" + ] | DT.Option _ -> failwith "option types never stored in the database" in @@ -515,7 +525,32 @@ let db_action api : O.Module.t = (Escaping.escape_obj obj.DT.name) (OU.escape name) in - _string_to_dm ^ "." ^ OU.alias_of_ty result_ty ^ " (" ^ query ^ ")" + let func = + _string_to_dm + ^ "." + ^ OU.alias_of_ty result_ty + ^ " (" + ^ query + ^ ")" + in + let query_opt = + Printf.sprintf "DB.db_get_by_uuid_opt __t \"%s\" %s" + (Escaping.escape_obj obj.DT.name) + (OU.escape name) + in + String.concat "\n\t\t" + ([func] + @ [ + String.concat "\n\t\t " + (["and get_by_uuid_opt ~__context ~uuid ="] + @ open_db_module + @ [ + Printf.sprintf "Option.map %s.%s (%s)" _string_to_dm + (OU.alias_of_ty result_ty) query_opt + ] + ) + ] + ) | _ -> failwith "GetByUuid call should have only one parameter and a result!" From 81c2a6af0b4965e50ebb0e34850b402a7ed4c487 Mon Sep 17 00:00:00 2001 From: Andrii Sultanov Date: Fri, 13 Dec 2024 12:06:40 +0000 Subject: [PATCH 3/3] IH-747 - xapi/import: Use get_by_uuid_opt to not log backtraces when failure is expected Migration logs are always full of exceptions that are expected and immediately handled: ``` [error|backtrace] SR.get_by_uuid D:8651cc0c9fb6 failed with exception Db_exn.Read_missing_uuid("SR", "", "a94bf103-0169-6d70-8874-334261f5098e") [error|backtrace] Raised Db_exn.Read_missing_uuid("SR", "", "a94bf103-0169-6d70-8874-334261f5098e") [error|backtrace] 1/9 xapi Raised at file ocaml/database/db_cache_impl.ml, line 237 [error|backtrace] 2/9 xapi Called from file ocaml/xapi/db_actions.ml, line 13309 [error|backtrace] 3/9 xapi Called from file ocaml/xapi/rbac.ml, line 188 [error|backtrace] 4/9 xapi Called from file ocaml/xapi/rbac.ml, line 197 [error|backtrace] 5/9 xapi Called from file ocaml/xapi/server_helpers.ml, line 74 [error|backtrace] 6/9 xapi Called from file ocaml/xapi/server_helpers.ml, line 96 [error|backtrace] 7/9 xapi Called from file ocaml/libs/xapi-stdext/lib/xapi-stdext-pervasives/pervasiveext.ml, line 24 [error|backtrace] 8/9 xapi Called from file ocaml/libs/xapi-stdext/lib/xapi-stdext-pervasives/pervasiveext.ml, line 39 [error|backtrace] 9/9 xapi Called from file ocaml/libs/log/debug.ml, line 250 [ warn|import] Failed to find SR with UUID: a94bf103-0169-6d70-8874-334261f5098e content-type: user - will still try to find individual VDIs [....] [debug|import] Importing 1 VM_guest_metrics(s) [debug|import] Importing 1 VM_metrics(s) [debug|import] Importing 1 VM(s) [debug|import] Importing 1 network(s) [debug|import] Importing 0 GPU_group(s) [debug|import] Importing 1 VBD(s) [error|backtrace] VBD.get_by_uuid D:3a12311e8be4 failed with exception Db_exn.Read_missing_uuid("VBD", "", "026d61e9-ed8a-fc72-7fd3-77422585baff") [error|backtrace] Raised Db_exn.Read_missing_uuid("VBD", "", "026d61e9-ed8a-fc72-7fd3-77422585baff") [error|backtrace] 1/9 xapi Raised at file ocaml/database/db_cache_impl.ml, line 237 [error|backtrace] 2/9 xapi Called from file ocaml/xapi/db_actions.ml, line 14485 [error|backtrace] 3/9 xapi Called from file ocaml/xapi/rbac.ml, line 188 [error|backtrace] 4/9 xapi Called from file ocaml/xapi/rbac.ml, line 197 [error|backtrace] 5/9 xapi Called from file ocaml/xapi/server_helpers.ml, line 74 [error|backtrace] 6/9 xapi Called from file ocaml/xapi/server_helpers.ml, line 96 [error|backtrace] 7/9 xapi Called from file ocaml/libs/xapi-stdext/lib/xapi-stdext-pervasives/pervasiveext.ml, line 24 [error|backtrace] 8/9 xapi Called from file ocaml/libs/xapi-stdext/lib/xapi-stdext-pervasives/pervasiveext.ml, line 39 [error|backtrace] 9/9 xapi Called from file ocaml/libs/log/debug.ml, line 250 [debug|import] Importing 1 VIF(s) [error|backtrace] VIF.get_by_uuid D:2bc78449e0bc failed with exception Db_exn.Read_missing_uuid("VIF", "", "7d14aee4-47a4-e271-4f64-fe9f9ef6d50b") [error|backtrace] Raised Db_exn.Read_missing_uuid("VIF", "", "7d14aee4-47a4-e271-4f64-fe9f9ef6d50b") [error|backtrace] 1/9 xapi Raised at file ocaml/database/db_cache_impl.ml, line 237 [error|backtrace] 2/9 xapi Called from file ocaml/xapi/db_actions.ml, line 10813 [error|backtrace] 3/9 xapi Called from file ocaml/xapi/rbac.ml, line 188 [error|backtrace] 4/9 xapi Called from file ocaml/xapi/rbac.ml, line 197 [error|backtrace] 5/9 xapi Called from file ocaml/xapi/server_helpers.ml, line 74 [error|backtrace] 6/9 xapi Called from file ocaml/xapi/server_helpers.ml, line 96 [error|backtrace] 7/9 xapi Called from file ocaml/libs/xapi-stdext/lib/xapi-stdext-pervasives/pervasiveext.ml, line 24 [error|backtrace] 8/9 xapi Called from file ocaml/libs/xapi-stdext/lib/xapi-stdext-pervasives/pervasiveext.ml, line 39 [error|backtrace] 9/9 xapi Called from file ocaml/libs/log/debug.ml, line 250 ``` Use an internal get_by_uuid_opt call and match on the Option instead, with the logs looking much clearer: ``` [debug|import] Importing 1 host(s) [debug|import] Importing 2 SR(s) [ warn|import] Failed to find SR with UUID: 8568e308-c61c-3b10-3953-45606cfecede content-type: - will still try to find individual VDIs [ warn|import] Failed to find SR with UUID: 40e9e252-46ac-ed3d-7a4c-6db175212195 content-type: user - will still try to find individual VDIs [...] [debug|import] Importing 2 VM_guest_metrics(s) [debug|import] Importing 2 VM(s) [debug|import] Importing 1 network(s) [debug|import] Importing 1 GPU_group(s) [debug|import] Importing 4 VBD(s) [ info|import] Did not find an already existing VBD with the same uuid=569d0e60-6a89-d1fa-2ed6-38b8eebe9065, try to create a new one [ info|import] Did not find an already existing VBD with the same uuid=533306da-cff1-7ada-71f7-2c4de8a0065b, try to create a new one [ info|import] Did not find an already existing VBD with the same uuid=f9dec620-0180-f67f-6711-7f9e5222a682, try to create a new one [ info|import] Did not find an already existing VBD with the same uuid=05e55076-b559-9b49-c247-e7850984ddae, try to create a new one [debug|import] Importing 2 VIF(s) [ info|import] Did not find an already existing VIF with the same uuid=a5a731d5-622c-5ca5-5b2a-a0053a11ef07, try to create a new one [ info|import] Did not find an already existing VIF with the same uuid=1738bf20-8d16-0d69-48cd-8f3d9e7ea791, try to create a new one ``` Signed-off-by: Andrii Sultanov --- ocaml/xapi/import.ml | 481 +++++++++++++++++++++++-------------------- 1 file changed, 262 insertions(+), 219 deletions(-) diff --git a/ocaml/xapi/import.ml b/ocaml/xapi/import.ml index 39e069b2768..3dd311d72e9 100644 --- a/ocaml/xapi/import.ml +++ b/ocaml/xapi/import.ml @@ -887,27 +887,31 @@ module SR : HandlerTools = struct | Will_use_SR of API.ref_SR | SR_not_needed - let precheck __context config rpc session_id _state x = + let precheck __context config _rpc _session_id _state x = let sr_record = API.sR_t_of_rpc x.snapshot in match config.import_type with | Metadata_import _ -> ( - try + match (* Look up the existing SR record *) - let sr = - Client.SR.get_by_uuid ~rpc ~session_id ~uuid:sr_record.API.sR_uuid - in - Found_SR sr - with _ -> - let msg = - match sr_record.API.sR_content_type with - | "iso" -> - "- will eject disk" (* Will be handled specially in handle_vdi *) - | _ -> - "- will still try to find individual VDIs" - in - warn "Failed to find SR with UUID: %s content-type: %s %s" - sr_record.API.sR_uuid sr_record.API.sR_content_type msg ; - Found_no_SR + (* Use an internal DB call - this avoids raising an exception and logging + the backtrace internally in case of a (reasonably expected) absence of + the object with this UUID *) + Db.SR.get_by_uuid_opt ~__context ~uuid:sr_record.API.sR_uuid + with + | Some sr -> + Found_SR sr + | None -> + let msg = + match sr_record.API.sR_content_type with + | "iso" -> + "- will eject disk" + (* Will be handled specially in handle_vdi *) + | _ -> + "- will still try to find individual VDIs" + in + warn "Failed to find SR with UUID: %s content-type: %s %s" + sr_record.API.sR_uuid sr_record.API.sR_content_type msg ; + Found_no_SR ) | Full_import sr -> if sr_record.API.sR_content_type = "iso" then @@ -1365,80 +1369,94 @@ end module VBD : HandlerTools = struct type precheck_t = Found_VBD of API.ref_VBD | Skip | Create of API.vBD_t - let precheck __context config rpc session_id state x = + let precheck __context config _rpc _session_id state x = let vbd_record = API.vBD_t_of_rpc x.snapshot in let get_vbd () = - Client.VBD.get_by_uuid ~rpc ~session_id ~uuid:vbd_record.API.vBD_uuid + (* Use an internal DB call - this avoids raising an exception and logging + the backtrace internally in case of a (reasonably expected) absence of + the object with this UUID *) + Db.VBD.get_by_uuid_opt ~__context ~uuid:vbd_record.API.vBD_uuid in - let vbd_exists () = - try - ignore (get_vbd ()) ; - true - with _ -> false + let vbd_opt = + (* If there's already a VBD with the same UUID and we're preserving UUIDs, use that one. *) + if config.full_restore then ( + match get_vbd () with + | Some x -> + Some x + | None -> + info + "Did not find an already existing VBD with the same uuid=%s, try \ + to create a new one" + vbd_record.API.vBD_uuid ; + None + ) else + None in - if config.full_restore && vbd_exists () then - let vbd = get_vbd () in - Found_VBD vbd - else - let vm = - log_reraise - ("Failed to find VBD's VM: " ^ Ref.string_of vbd_record.API.vBD_VM) - (lookup vbd_record.API.vBD_VM) - state.table - in - (* If the VBD is supposed to be attached to a PV guest (which doesn't support - currently_attached empty drives) then throw a fatal error. *) - let original_vm = - get_vm_record - (find_in_export (Ref.string_of vbd_record.API.vBD_VM) state.export) - in - (* Note: the following is potentially inaccurate: the find out whether a running or - * suspended VM has booted HVM, we must consult the VM metrics, but those aren't - * available in the exported metadata. *) - let has_qemu = Helpers.will_have_qemu_from_record original_vm in - (* In the case of dry_run live migration, don't check for - missing disks as CDs will be ejected before the real migration. *) - let dry_run, live = - match config.import_type with - | Metadata_import {dry_run; live; _} -> - (dry_run, live) - | _ -> - (false, false) - in - ( if - vbd_record.API.vBD_currently_attached - && not (exists vbd_record.API.vBD_VDI state.table) - then - (* It's only ok if it's a CDROM attached to an HVM guest, or it's part of SXM and we know the sender would eject it. *) - let will_eject = - dry_run && live && original_vm.API.vM_power_state <> `Suspended - in - if not (vbd_record.API.vBD_type = `CD && (has_qemu || will_eject)) + match vbd_opt with + | Some vbd -> + Found_VBD vbd + | None -> ( + let vm = + log_reraise + ("Failed to find VBD's VM: " ^ Ref.string_of vbd_record.API.vBD_VM) + (lookup vbd_record.API.vBD_VM) + state.table + in + (* If the VBD is supposed to be attached to a PV guest (which doesn't support + currently_attached empty drives) then throw a fatal error. *) + let original_vm = + get_vm_record + (find_in_export (Ref.string_of vbd_record.API.vBD_VM) state.export) + in + (* Note: the following is potentially inaccurate: the find out whether a running or + * suspended VM has booted HVM, we must consult the VM metrics, but those aren't + * available in the exported metadata. *) + let has_qemu = Helpers.will_have_qemu_from_record original_vm in + (* In the case of dry_run live migration, don't check for + missing disks as CDs will be ejected before the real migration. *) + let dry_run, live = + match config.import_type with + | Metadata_import {dry_run; live; _} -> + (dry_run, live) + | _ -> + (false, false) + in + ( if + vbd_record.API.vBD_currently_attached + && not (exists vbd_record.API.vBD_VDI state.table) then - raise (IFailure Attached_disks_not_found) - ) ; - let vbd_record = {vbd_record with API.vBD_VM= vm} in - match - (vbd_record.API.vBD_type, exists vbd_record.API.vBD_VDI state.table) - with - | `CD, false | `Floppy, false -> - if has_qemu || original_vm.API.vM_power_state <> `Suspended then - Create {vbd_record with API.vBD_VDI= Ref.null; API.vBD_empty= true} - (* eject *) - else - Create vbd_record - | `Disk, false -> - (* omit: cannot have empty disks *) - warn - "Cannot import VM's disk: was it an .iso attached as a disk rather \ - than CD?" ; - Skip - | _, true -> - Create - { - vbd_record with - API.vBD_VDI= lookup vbd_record.API.vBD_VDI state.table - } + (* It's only ok if it's a CDROM attached to an HVM guest, or it's part of SXM and we know the sender would eject it. *) + let will_eject = + dry_run && live && original_vm.API.vM_power_state <> `Suspended + in + if not (vbd_record.API.vBD_type = `CD && (has_qemu || will_eject)) + then + raise (IFailure Attached_disks_not_found) + ) ; + let vbd_record = {vbd_record with API.vBD_VM= vm} in + match + (vbd_record.API.vBD_type, exists vbd_record.API.vBD_VDI state.table) + with + | `CD, false | `Floppy, false -> + if has_qemu || original_vm.API.vM_power_state <> `Suspended then + Create + {vbd_record with API.vBD_VDI= Ref.null; API.vBD_empty= true} + (* eject *) + else + Create vbd_record + | `Disk, false -> + (* omit: cannot have empty disks *) + warn + "Cannot import VM's disk: was it an .iso attached as a disk \ + rather than CD?" ; + Skip + | _, true -> + Create + { + vbd_record with + API.vBD_VDI= lookup vbd_record.API.vBD_VDI state.table + } + ) let handle_dry_run __context _config _rpc _session_id state x precheck_result = @@ -1493,88 +1511,99 @@ end module VIF : HandlerTools = struct type precheck_t = Found_VIF of API.ref_VIF | Create of API.vIF_t - let precheck __context config rpc session_id state x = + let precheck __context config _rpc _session_id state x = let vif_record = API.vIF_t_of_rpc x.snapshot in let get_vif () = - Client.VIF.get_by_uuid ~rpc ~session_id ~uuid:vif_record.API.vIF_uuid + (* Use an internal DB call - this avoids raising an exception and logging + the backtrace internally in case of a (reasonably expected) absence of + the object with this UUID *) + Db.VIF.get_by_uuid_opt ~__context ~uuid:vif_record.API.vIF_uuid in - let vif_exists () = - try - ignore (get_vif ()) ; - true - with _ -> false + let vif_opt = + if config.full_restore then ( + (* If there's already a VIF with the same UUID and we're preserving UUIDs, use that one. *) + match get_vif () with + | Some x -> + Some x + | None -> + info + "Did not find an already existing VIF with the same uuid=%s, try \ + to create a new one" + vif_record.API.vIF_uuid ; + None + ) else + None in - if config.full_restore && vif_exists () then - (* If there's already a VIF with the same UUID and we're preserving UUIDs, use that one. *) - let vif = get_vif () in - Found_VIF vif - else - (* If not restoring a full backup then blank the MAC so it is regenerated *) - let vif_record = - { - vif_record with - API.vIF_MAC= - (if config.full_restore then vif_record.API.vIF_MAC else "") - } - in - (* Determine the VM to which we're going to attach this VIF. *) - let vm = - log_reraise - ("Failed to find VIF's VM: " ^ Ref.string_of vif_record.API.vIF_VM) - (lookup vif_record.API.vIF_VM) - state.table - in - (* Determine the network to which we're going to attach this VIF. *) - let net = - (* If we find the cross-pool migration key, attach the VIF to that network... *) - if - List.mem_assoc Constants.storage_migrate_vif_map_key - vif_record.API.vIF_other_config - then - Ref.of_string - (List.assoc Constants.storage_migrate_vif_map_key - vif_record.API.vIF_other_config - ) - else - (* ...otherwise fall back to looking up the network from the state table. *) + match vif_opt with + | Some vif -> + Found_VIF vif + | None -> + (* If not restoring a full backup then blank the MAC so it is regenerated *) + let vif_record = + { + vif_record with + API.vIF_MAC= + (if config.full_restore then vif_record.API.vIF_MAC else "") + } + in + (* Determine the VM to which we're going to attach this VIF. *) + let vm = log_reraise - ("Failed to find VIF's Network: " - ^ Ref.string_of vif_record.API.vIF_network - ) - (lookup vif_record.API.vIF_network) + ("Failed to find VIF's VM: " ^ Ref.string_of vif_record.API.vIF_VM) + (lookup vif_record.API.vIF_VM) state.table - in - (* Make sure we remove the cross-pool migration VIF mapping key from the other_config - * before creating a VIF - otherwise we'll risk sending this key on to another pool - * during a future cross-pool migration and it won't make sense. *) - let other_config = - List.filter - (fun (k, _) -> k <> Constants.storage_migrate_vif_map_key) - vif_record.API.vIF_other_config - in - (* Construct the VIF record we're going to try to create locally. *) - let vif_record = - if Pool_features.is_enabled ~__context Features.VIF_locking then - vif_record - else if vif_record.API.vIF_locking_mode = `locked then + in + (* Determine the network to which we're going to attach this VIF. *) + let net = + (* If we find the cross-pool migration key, attach the VIF to that network... *) + if + List.mem_assoc Constants.storage_migrate_vif_map_key + vif_record.API.vIF_other_config + then + Ref.of_string + (List.assoc Constants.storage_migrate_vif_map_key + vif_record.API.vIF_other_config + ) + else + (* ...otherwise fall back to looking up the network from the state table. *) + log_reraise + ("Failed to find VIF's Network: " + ^ Ref.string_of vif_record.API.vIF_network + ) + (lookup vif_record.API.vIF_network) + state.table + in + (* Make sure we remove the cross-pool migration VIF mapping key from the other_config + * before creating a VIF - otherwise we'll risk sending this key on to another pool + * during a future cross-pool migration and it won't make sense. *) + let other_config = + List.filter + (fun (k, _) -> k <> Constants.storage_migrate_vif_map_key) + vif_record.API.vIF_other_config + in + (* Construct the VIF record we're going to try to create locally. *) + let vif_record = + if Pool_features.is_enabled ~__context Features.VIF_locking then + vif_record + else if vif_record.API.vIF_locking_mode = `locked then + { + vif_record with + API.vIF_locking_mode= `network_default + ; API.vIF_ipv4_allowed= [] + ; API.vIF_ipv6_allowed= [] + } + else + {vif_record with API.vIF_ipv4_allowed= []; API.vIF_ipv6_allowed= []} + in + let vif_record = { vif_record with - API.vIF_locking_mode= `network_default - ; API.vIF_ipv4_allowed= [] - ; API.vIF_ipv6_allowed= [] + API.vIF_VM= vm + ; API.vIF_network= net + ; API.vIF_other_config= other_config } - else - {vif_record with API.vIF_ipv4_allowed= []; API.vIF_ipv6_allowed= []} - in - let vif_record = - { - vif_record with - API.vIF_VM= vm - ; API.vIF_network= net - ; API.vIF_other_config= other_config - } - in - Create vif_record + in + Create vif_record let handle_dry_run __context _config _rpc _session_id state x precheck_result = @@ -1710,74 +1739,88 @@ end module VGPU : HandlerTools = struct type precheck_t = Found_VGPU of API.ref_VGPU | Create of API.vGPU_t - let precheck __context config rpc session_id state x = + let precheck __context config _rpc _session_id state x = let vgpu_record = API.vGPU_t_of_rpc x.snapshot in let get_vgpu () = - Client.VGPU.get_by_uuid ~rpc ~session_id ~uuid:vgpu_record.API.vGPU_uuid + (* Use an internal DB call - this avoids raising an exception and logging + the backtrace internally in case of a (reasonably expected) absence of + the object with this UUID *) + Db.VGPU.get_by_uuid_opt ~__context ~uuid:vgpu_record.API.vGPU_uuid in - let vgpu_exists () = - try - ignore (get_vgpu ()) ; - true - with _ -> false + let vgpu_opt = + if config.full_restore then ( + (* If there's already a VGPU with the same UUID and we're preserving UUIDs, use that one. *) + match get_vgpu () with + | Some x -> + Some x + | None -> + info + "Did not find an already existing VGPU with the same uuid=%s, \ + try to create a new one" + vgpu_record.API.vGPU_uuid ; + None + ) else + None in - if config.full_restore && vgpu_exists () then - let vgpu = get_vgpu () in - Found_VGPU vgpu - else - let vm = - log_reraise - ("Failed to find VGPU's VM: " ^ Ref.string_of vgpu_record.API.vGPU_VM) - (lookup vgpu_record.API.vGPU_VM) - state.table - in - let group = - (* If we find the cross-pool migration key, attach the vgpu to the provided gpu_group... *) - if - List.mem_assoc Constants.storage_migrate_vgpu_map_key - vgpu_record.API.vGPU_other_config - then - Ref.of_string - (List.assoc Constants.storage_migrate_vgpu_map_key - vgpu_record.API.vGPU_other_config + match vgpu_opt with + | Some vgpu -> + Found_VGPU vgpu + | None -> + let vm = + log_reraise + ("Failed to find VGPU's VM: " + ^ Ref.string_of vgpu_record.API.vGPU_VM ) - else - (* ...otherwise fall back to looking up the vgpu from the state table. *) + (lookup vgpu_record.API.vGPU_VM) + state.table + in + let group = + (* If we find the cross-pool migration key, attach the vgpu to the provided gpu_group... *) + if + List.mem_assoc Constants.storage_migrate_vgpu_map_key + vgpu_record.API.vGPU_other_config + then + Ref.of_string + (List.assoc Constants.storage_migrate_vgpu_map_key + vgpu_record.API.vGPU_other_config + ) + else + (* ...otherwise fall back to looking up the vgpu from the state table. *) + log_reraise + ("Failed to find VGPU's GPU group: " + ^ Ref.string_of vgpu_record.API.vGPU_GPU_group + ) + (lookup vgpu_record.API.vGPU_GPU_group) + state.table + in + let _type = log_reraise - ("Failed to find VGPU's GPU group: " - ^ Ref.string_of vgpu_record.API.vGPU_GPU_group + ("Failed to find VGPU's type: " + ^ Ref.string_of vgpu_record.API.vGPU_type ) - (lookup vgpu_record.API.vGPU_GPU_group) + (lookup vgpu_record.API.vGPU_type) state.table - in - let _type = - log_reraise - ("Failed to find VGPU's type: " - ^ Ref.string_of vgpu_record.API.vGPU_type - ) - (lookup vgpu_record.API.vGPU_type) - state.table - in - (* Make sure we remove the cross-pool migration VGPU mapping key from the other_config - * before creating a VGPU - otherwise we'll risk sending this key on to another pool - * during a future cross-pool migration and it won't make sense. *) - let other_config = - List.filter - (fun (k, _) -> k <> Constants.storage_migrate_vgpu_map_key) - vgpu_record.API.vGPU_other_config - in - let vgpu_record = - { - vgpu_record with - API.vGPU_VM= vm - ; API.vGPU_GPU_group= group - ; API.vGPU_type= _type - ; API.vGPU_other_config= other_config - } - in - if is_live config then - assert_can_live_import_vgpu ~__context vgpu_record ; - Create vgpu_record + in + (* Make sure we remove the cross-pool migration VGPU mapping key from the other_config + * before creating a VGPU - otherwise we'll risk sending this key on to another pool + * during a future cross-pool migration and it won't make sense. *) + let other_config = + List.filter + (fun (k, _) -> k <> Constants.storage_migrate_vgpu_map_key) + vgpu_record.API.vGPU_other_config + in + let vgpu_record = + { + vgpu_record with + API.vGPU_VM= vm + ; API.vGPU_GPU_group= group + ; API.vGPU_type= _type + ; API.vGPU_other_config= other_config + } + in + if is_live config then + assert_can_live_import_vgpu ~__context vgpu_record ; + Create vgpu_record let handle_dry_run __context _config _rpc _session_id state x precheck_result =