Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

CA-94616: Check SR Capabilities before suspending VM for checkpoint #933

Merged
merged 1 commit into from

2 participants

@siddharthv

We check if the SR's on which a VM's VDI's reside has snapshot capability before actually suspending a VM for checkpoint operation.

@jonludlam jonludlam commented on the diff
ocaml/xapi/xapi_vm_snapshot.ml
@@ -178,6 +178,28 @@ let checkpoint ~__context ~vm ~new_name =
try
(* Save the state of the vm *)
snapshot_info := Xapi_vm_clone.snapshot_info ~power_state ~is_a_snapshot:true;
+
+ (* Get all the VM's VDI's except CD's *)
+ let vbds = Db.VM.get_VBDs ~__context ~self:vm in
+ let vbds = List.filter (fun x -> Db.VBD.get_type ~__context ~self:x <> `CD) vbds in
+ let vdis = List.map (fun self -> Db.VBD.get_VDI ~__context ~self) vbds in
+
@jonludlam Owner

These VDIs may not all be in the database - destroying a VDI doesn't cause all of the VBDs referencing it to be immediately GC'd - we should wrap the Db.VDI.get_SR with an exception handler.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jonludlam
Owner

From @siddharthv :
The entire patch is enclosed within a try with block.

Could you give me some more details as to why we need to wrap Db.VDI.get_SR with an exception handler and what we intend to do in it??

@jonludlam
Owner

The point is that a vdi that doesn't exist isn't necessarily an error, so we should cope with that. Dangling VBDs will be gc'd by db_gc, but it happens asynchronously. Although the current patch is enclosed by try/catch, that will just propagate the error in this case, when we should be ignoring the error.

@siddharthv

Does this modification look good??

let vdi_sr = List.fold_left (fun x vdi -> try (Db.VDI.get_SR ~__context ~self:vdi) :: x with _ -> x) [] vdis in

@jonludlam
Owner

Yep, that looks good - alternatively you can use Listext.filter_map, which might look nicer perhaps.

It may fail later in the function due to this condition, but at least we aren't introducing another instance of this class of issue :-)

@jonludlam
Owner

This looks good now

@jonludlam jonludlam merged commit 3921792 into xapi-project:master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 23 additions and 1 deletion.
  1. +23 −1 ocaml/xapi/xapi_vm_snapshot.ml
View
24 ocaml/xapi/xapi_vm_snapshot.ml
@@ -18,7 +18,7 @@
open Client
open Vmopshelpers
open Xenstore
-
+open Listext
open Client
module D = Debug.Debugger(struct let name="xapi" end)
open D
@@ -178,6 +178,28 @@ let checkpoint ~__context ~vm ~new_name =
try
(* Save the state of the vm *)
snapshot_info := Xapi_vm_clone.snapshot_info ~power_state ~is_a_snapshot:true;
+
+ (* Get all the VM's VDI's except CD's *)
+ let vbds = Db.VM.get_VBDs ~__context ~self:vm in
+ let vbds = List.filter (fun x -> Db.VBD.get_type ~__context ~self:x <> `CD) vbds in
+ let vdis = List.map (fun self -> Db.VBD.get_VDI ~__context ~self) vbds in
+
@jonludlam Owner

These VDIs may not all be in the database - destroying a VDI doesn't cause all of the VBDs referencing it to be immediately GC'd - we should wrap the Db.VDI.get_SR with an exception handler.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ (* Get SR of each VDI *)
+ let vdi_sr = List.filter_map (fun vdi -> try Some (Db.VDI.get_SR ~__context ~self:vdi) with _ -> None) vdis in
+ let vdi_sr = List.setify vdi_sr in
+ let sr_records = List.map (fun self -> Db.SR.get_record_internal ~__context ~self) vdi_sr in
+
+ (* Check if SR has snapshot capability *)
+ let sr_has_snapshot_capability sr =
+ if not (List.mem Smint.Vdi_snapshot (Xapi_sr_operations.capabilities_of_sr sr)) then false
+ else true
+ in
+
+ List.iter
+ (fun sr ->
+ if not (sr_has_snapshot_capability sr)
+ then raise (Api_errors.Server_error (Api_errors.sr_operation_not_supported, [Ref.string_of vm])) )
+ sr_records ;
(* suspend the VM *)
Xapi_xenops.suspend ~__context ~self:vm;
with
Something went wrong with that request. Please try again.