Skip to content

Loading…

CA-91480: Catch exception if blob doesn't exist on vm_destroy #911

Merged
merged 2 commits into from

3 participants

@mcclurmc

This was meant to fix CA-91480, but it turns out that this was a different issue. This issue is that vm_destroy doesn't catch an exception if it tries to destroy a VM which has a blob which also doesn't exist on the server, which could happen in a vm_import.

This is the first bug we've found that we've also included a regression unit test for, which is significant. Otherwise, this is an unlikely bug to encounter and fairly trivial to solve.

Mike McClurg added some commits
Mike McClurg Add test for CA-91480: VM destroy skips missing blobs
Xapi_vm_helpers.destroy would fail if a VM had a reference to a non-existant
blob, which would happen when the VM had been imported. This would prevent the
rest of the function from running, leaving ghost objects in the database after
an import was cancelled.

Signed-off-by: Mike McClurg <mike.mcclurg@citrix.com>
1fa3fb2
Mike McClurg CA-91480: Cancel vm_import cleans up temporary objects
Xapi_vm_helpers.destroy would fail if a VM had a reference to a non-existant
blob, which would happen when the VM had been imported. This would prevent the
rest of the function from running, leaving ghost objects in the database after
an import was cancelled.

Signed-off-by: Mike McClurg <mike.mcclurg@citrix.com>
fc9a4bf
@xen-git
Xapi Project member

mcclurmc/xen-api@fc9a4bfxapi-project/xen-api@dbd1565: Build succeeded. Can merge pull request.

@xen-git
Xapi Project member

mcclurmc/xen-api@fc9a4bfxapi-project/xen-api@2c0fbe1: Build succeeded. Can merge pull request.

@jonludlam
Xapi Project member

@xen-git Approved

@xen-git
Xapi Project member

mcclurmc/xen-api@fc9a4bfxapi-project/xen-api@4474f90: Build succeeded. Pull request merged.

@xen-git xen-git merged commit fc9a4bf into xapi-project:master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Nov 6, 2012
  1. Add test for CA-91480: VM destroy skips missing blobs

    Mike McClurg committed
    Xapi_vm_helpers.destroy would fail if a VM had a reference to a non-existant
    blob, which would happen when the VM had been imported. This would prevent the
    rest of the function from running, leaving ghost objects in the database after
    an import was cancelled.
    
    Signed-off-by: Mike McClurg <mike.mcclurg@citrix.com>
  2. CA-91480: Cancel vm_import cleans up temporary objects

    Mike McClurg committed
    Xapi_vm_helpers.destroy would fail if a VM had a reference to a non-existant
    blob, which would happen when the VM had been imported. This would prevent the
    rest of the function from running, leaving ghost objects in the database after
    an import was cancelled.
    
    Signed-off-by: Mike McClurg <mike.mcclurg@citrix.com>
Showing with 39 additions and 1 deletion.
  1. +1 −0 ocaml/test/OMakefile
  2. +1 −0 ocaml/test/suite.ml
  3. +32 −0 ocaml/test/test_ca91480.ml
  4. +5 −1 ocaml/xapi/xapi_vm_helpers.ml
View
1 ocaml/test/OMakefile
@@ -27,5 +27,6 @@ OCAML_OBJS = \
test_basic \
test_pool_db_backup \
test_xapi_db_upgrade \
+ test_ca91480 \
OCamlProgram(suite, suite $(OCAML_OBJS) )
View
1 ocaml/test/suite.ml
@@ -45,6 +45,7 @@ let base_suite =
test_basic;
test_db_backup;
test_db_upgrade;
+ Test_ca91480.test;
]
let _ = run_test_tt_main base_suite
View
32 ocaml/test/test_ca91480.ml
@@ -0,0 +1,32 @@
+(* Fixture: Create DB with VM. VM has records for Blobs, Appliances,
+ VBDs, VIFs, VGPUs, PCIs, VM_metrics, and VM_guest_metrics, but none
+ of these objects should actually exist in the DB. *)
+
+open OUnit
+open Test_common
+
+let setup_fixture () =
+ let __context = make_test_database () in
+ let self = make_vm ~__context () in
+
+ let fake_v f = f ~__context ~self ~value:(Ref.make ())
+ and fake_m f = f ~__context ~self ~key:"fake" ~value:(Ref.make ())
+ and fake_l f = f ~__context ~self ~value:[(Ref.make ())] in
+
+ fake_m Db.VM.add_to_blobs ;
+ fake_v Db.VM.set_appliance ;
+ fake_l Db.VM.set_attached_PCIs ;
+ fake_v Db.VM.set_metrics ;
+ fake_v Db.VM.set_guest_metrics ;
+
+ __context, self
+
+let test_vm_destroy () =
+ let __context, self = setup_fixture () in
+ Xapi_vm_helpers.destroy ~__context ~self
+
+let test =
+ "test_ca91480" >:::
+ [
+ "test_vm_destroy" >:: test_vm_destroy;
+ ]
View
6 ocaml/xapi/xapi_vm_helpers.ml
@@ -151,8 +151,12 @@ let destroy ~__context ~self =
(* given the call to 'assert_operation_valid' *)
debug "VM.destroy: deleting DB records";
+ (* Should we be destroying blobs? It's possible to create a blob and then
+ add its reference to multiple objects. Perhaps we want to just leave the
+ blob? Or only delete it if there is no other reference to it? Is that
+ even possible to know? *)
let blobs = Db.VM.get_blobs ~__context ~self in
- List.iter (fun (name,_ref) -> Xapi_blob.destroy ~__context ~self:_ref) blobs;
+ List.iter (fun (_,self) -> try Xapi_blob.destroy ~__context ~self with _ -> ()) blobs;
let other_config = Db.VM.get_other_config ~__context ~self in
if ((List.mem_assoc Xapi_globs.default_template_key other_config) &&
Something went wrong with that request. Please try again.