CA-98944: Replaced potentially lots of db calls (one per VBD) by two big get_internal_where calls

Signed-off-by: Vincent Bernardoff <[email protected]>

Author: Vincent Bernardoff

ocaml/test/OMakefile

@@ -31,6 +31,7 @@ OCAML_OBJS = \
 	test_pool_db_backup \
 	test_xapi_db_upgrade \
 	test_ca91480 \
+	test_ca98944 \
 	test_pr1510 \
 	test_pool_license \
 

ocaml/test/suite.ml

@@ -21,6 +21,7 @@ let base_suite =
 			Test_pool_db_backup.test;
 			Test_xapi_db_upgrade.test;
 			Test_ca91480.test;
+			Test_ca98944.test;
 			Test_pr1510.suite;
 			Test_pool_license.test;
 		]

ocaml/test/test_ca98944.ml

@@ -0,0 +1,78 @@
+(*
+ * Copyright (C) 2006-2012 Citrix Systems Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License as published
+ * by the Free Software Foundation; version 2.1 only. with the special
+ * exception on linking described in file LICENSE.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Lesser General Public License for more details.
+ *)
+
+open OUnit
+open Test_common
+
+(* Helpers for testing Xapi_vdi.check_operation_error *)
+
+let setup_test ~__context vbd_fun =
+	let pbd_ref, sr_ref, vdi_ref = Ref.make (), Ref.make (), Ref.make () in
+	let () = make_sr ~__context ~ref:sr_ref () in
+	let () = make_pbd ~__context ~ref:pbd_ref ~sR:sr_ref () in
+	let () = make_vdi ~__context ~ref:vdi_ref ~sR:sr_ref () in
+	let vdi_record = Db.VDI.get_record_internal ~__context ~self:vdi_ref in
+	vbd_fun vdi_ref;
+	vdi_ref, vdi_record
+
+let my_cmp a b = match a,b with
+	| Some aa, Some bb	-> fst aa = fst bb
+	| None, None -> a = b
+	| _	-> false
+
+let run_assert_equal_with_vdi ~__context ?(cmp = my_cmp) ?(ha_enabled=false) vbd_list op exc =
+	let vdi_ref, vdi_record = setup_test ~__context vbd_list in
+	assert_equal ~cmp (Xapi_vdi.check_operation_error ~__context ha_enabled vdi_record vdi_ref op) exc
+
+(* This is to test Xapi_vdi.check_operation_error against CA-98944
+   code. This DO NOT fully test the aforementionned function *)
+let test_vdi () =
+	let __context = Mock.make_context_with_new_db "Mock context" in
+	(* Should raise vdi_in_use *)
+	run_assert_equal_with_vdi ~__context
+		(fun vdi_ref ->
+			make_vbd ~vDI:vdi_ref ~__context
+				~reserved:true ~currently_attached:false ~current_operations:["", `attach] ())
+		`update (Some (Api_errors.vdi_in_use, []));
+
+	(* Should raise vdi_in_use *)
+	run_assert_equal_with_vdi ~__context
+		(fun vdi_ref ->
+			make_vbd ~vDI:vdi_ref
+				~__context ~reserved:false ~currently_attached:true ~current_operations:["", `attach] ())
+		`update (Some (Api_errors.vdi_in_use, []));
+
+	(* Should raise vdi_in_use *)
+	run_assert_equal_with_vdi ~__context
+		(fun vdi_ref -> make_vbd ~vDI:vdi_ref
+			~__context ~reserved:true ~currently_attached:true ~current_operations:["", `attach] ())
+		`update (Some (Api_errors.vdi_in_use, []));
+
+	(* Should raise other_operation_in_progress *)
+	run_assert_equal_with_vdi ~__context
+		(fun vdi_ref -> make_vbd ~vDI:vdi_ref
+			~__context ~reserved:false ~currently_attached:false ~current_operations:["", `attach] ())
+		`update (Some (Api_errors.other_operation_in_progress, []));
+
+	(* Should pass *)
+	run_assert_equal_with_vdi ~__context
+		(fun vdi_ref -> make_vbd ~vDI:vdi_ref
+			~__context ~reserved:false ~currently_attached:false ~current_operations:[] ())
+		`forget None
+
+let test =
+	"test_ca98944" >:::
+		[
+			"test_vdi" >:: test_vdi
+		]

ocaml/test/test_common.ml

@@ -15,7 +15,9 @@
 open API
 open OUnit
 
+(** Utility functions *)
 let skip str = skip_if true str
+let make_uuid () = Uuid.string_of_uuid (Uuid.make_uuid ())
 
 (** Make a simple in-memory database containing a single host and dom0 VM record. *)
 let make_test_database () = 
@@ -63,7 +65,7 @@ let make_vm ~__context ?(name_label="name_label") ?(name_description="descriptio
 		~ha_always_run ~ha_restart_priority ~tags ~blocked_operations ~protection_policy
 		~is_snapshot_from_vmpp ~appliance ~start_delay ~shutdown_delay ~order ~suspend_SR ~version
 
-let make_host ~__context ?(uuid=Uuid.string_of_uuid (Uuid.make_uuid ())) ?(name_label="host")
+let make_host ~__context ?(uuid=make_uuid ()) ?(name_label="host")
 		?(name_description="description") ?(hostname="localhost") ?(address="127.0.0.1")
 		?(external_auth_type="") ?(external_auth_service_name="") ?(external_auth_configuration=[])
 		?(license_params=[]) ?(edition="free") ?(license_server=[]) ?(local_cache_sr=Ref.null) ?(chipset_info=[]) () = 
@@ -95,11 +97,49 @@ let make_pool ~__context ~master ?(name_label="") ?(name_description="")
 		?(other_config=[Xapi_globs.memory_ratio_hvm; Xapi_globs.memory_ratio_pv]) () =
 	let pool_ref = Ref.make () in
 	Db.Pool.create ~__context ~ref:pool_ref
-		~uuid:(Uuid.to_string (Uuid.make_uuid ())) ~name_label ~name_description
+		~uuid:(make_uuid ()) ~name_label ~name_description
 		~master ~default_SR ~suspend_image_SR ~crash_dump_SR ~ha_enabled
 		~ha_configuration ~ha_statefiles ~ha_host_failures_to_tolerate
 		~ha_plan_exists_for ~ha_allow_overcommit ~ha_overcommitted ~blobs ~tags
 		~gui_config ~wlb_url ~wlb_username ~wlb_password ~wlb_enabled
 		~wlb_verify_cert ~redo_log_enabled ~redo_log_vdi ~vswitch_controller
 		~restrictions ~other_config;
 	pool_ref
+
+let make_sr ~__context ~ref ?(uuid=make_uuid ()) ?(name_label="") ?(name_description="") ?(allowed_operations=[])
+		?(current_operations=[]) ?(virtual_allocation=0L) ?(physical_utilisation=0L) ?(physical_size=0L) ?(_type="")
+		?(content_type="") ?(shared=true) ?(other_config=[]) ?(tags=[]) ?(default_vdi_visibility=true)
+		?(sm_config=[]) ?(blobs=[]) ?(local_cache_enabled=false) ?(introduced_by=Ref.make ()) () =
+	Db.SR.create ~__context ~ref ~uuid ~name_label ~name_description ~allowed_operations
+		~current_operations ~virtual_allocation ~physical_utilisation ~physical_size ~_type
+		~content_type ~shared ~other_config ~tags ~default_vdi_visibility ~sm_config ~blobs
+		~local_cache_enabled ~introduced_by
+
+let make_pbd ~__context ~ref ?(uuid=make_uuid ()) ?(host=Ref.make ()) ?(sR=Ref.make ())
+		?(device_config=[]) ?(currently_attached=true) ?(other_config=[]) () =
+	Db.PBD.create ~__context ~ref ~uuid ~host ~sR ~device_config ~currently_attached ~other_config
+
+let make_vbd ~__context ?(ref=Ref.make ()) ?(uuid=make_uuid ()) ?(allowed_operations=[])
+		?(current_operations=[]) ?(vM=Ref.make ()) ?(vDI=Ref.make ()) ?(device="")
+		?(userdevice="") ?(bootable=true) ?(mode=`RW) ?(_type=`Disk)
+		?(unpluggable=false) ?(storage_lock=false) ?(empty=false)
+		?(reserved=false) ?(other_config=[]) ?(currently_attached=false)
+		?(status_code=0L) ?(status_detail="") ?(runtime_properties=[])
+		?(qos_algorithm_type="") ?(qos_algorithm_params=[]) ?(qos_supported_algorithms=[])
+		?(metrics = Ref.make ()) () =
+	Db.VBD.create ~__context ~ref ~uuid ~allowed_operations ~current_operations ~vM ~vDI ~device
+		~userdevice ~bootable ~mode ~_type ~unpluggable ~storage_lock ~empty ~reserved ~other_config
+		~currently_attached ~status_code ~status_detail ~runtime_properties ~qos_algorithm_type
+		~qos_algorithm_params ~qos_supported_algorithms ~metrics
+
+let make_vdi ~__context ?(ref=Ref.make ()) ?(uuid=make_uuid ()) ?(name_label="")
+		?(name_description="") ?(allowed_operations=[]) ?(current_operations=[]) ?(sR=Ref.make ())
+		?(virtual_size=0L) ?(physical_utilisation=0L) ?(_type=`user) ?(sharable=true) ?(read_only=false)
+		?(other_config=[]) ?(storage_lock=false) ?(location="") ?(managed=false) ?(missing=false)
+		?(parent=Ref.null) ?(xenstore_data=[]) ?(sm_config=[]) ?(is_a_snapshot=false)
+		?(snapshot_of=Ref.null) ?(snapshot_time=API.Date.never) ?(tags=[]) ?(allow_caching=true)
+		?(on_boot=`persist) ?(metadata_of_pool=Ref.make ()) ?(metadata_latest=true) () =
+	Db.VDI.create ~__context ~ref ~uuid ~name_label ~name_description ~allowed_operations
+		~current_operations ~sR ~virtual_size ~physical_utilisation ~_type ~sharable ~read_only ~other_config
+		~storage_lock ~location ~managed ~missing ~parent ~xenstore_data ~sm_config ~is_a_snapshot ~snapshot_of
+		~snapshot_time ~tags ~allow_caching ~on_boot ~metadata_of_pool ~metadata_latest

ocaml/xapi/xapi_vdi.ml

@@ -29,7 +29,6 @@ open Printf
 let check_operation_error ~__context ?(sr_records=[]) ?(pbd_records=[]) ?(vbd_records=[]) ha_enabled record _ref' op =
 	let _ref = Ref.string_of _ref' in
 	let current_ops = record.Db_actions.vDI_current_operations in
-
 	let reset_on_boot = record.Db_actions.vDI_on_boot = `reset in
 
 	(* Policy:
@@ -58,35 +57,47 @@ let check_operation_error ~__context ?(sr_records=[]) ?(pbd_records=[]) ?(vbd_re
 		then Some(Api_errors.sr_no_pbds, [Ref.string_of sr])
 		else
 			(* check to see whether VBDs exist which are using this VDI *)
-			let my_vbd_records = match vbd_records with
-			| [] -> List.map (fun vbd -> Db.VBD.get_record_internal ~__context ~self:vbd) record.Db_actions.vDI_VBDs
-			| _ -> List.map snd (List.filter (fun (_, vbd_record) -> vbd_record.Db_actions.vBD_VDI = _ref') vbd_records)
-			in
 
 			(* Only a 'live' operation can be performed if there are active (even RO) devices *)
-			let is_active v = v.Db_actions.vBD_currently_attached || v.Db_actions.vBD_reserved in
+			let my_active_vbd_records = match vbd_records with
+			| [] -> List.map snd (Db.VBD.get_internal_records_where ~__context
+									  ~expr:(
+										  And(Eq (Field "VDI", Literal _ref),
+											  Or(
+												  Eq (Field "currently_attached", Literal "true"),
+												  Eq (Field "reserved", Literal "true")))
+									  ))
+			| _ -> List.map snd (List.filter (fun (_, vbd_record) ->
+				vbd_record.Db_actions.vBD_VDI = _ref' && (vbd_record.Db_actions.vBD_currently_attached || vbd_record.Db_actions.vBD_reserved)
+			) vbd_records)
+			in
+
 			(* VBD operations (plug/unplug) (which should be transient) cause us to serialise *)
-			let has_current_operation v = v.Db_actions.vBD_current_operations <> [] in
+			let my_has_current_operation_vbd_records = match vbd_records with
+			| [] -> List.map snd (Db.VBD.get_internal_records_where ~__context
+									  ~expr:(
+										  And(Eq (Field "VDI", Literal _ref), Not (Eq (Field "current_operations", Literal "()")))
+									  ))
+			| _ -> List.map snd (List.filter (fun (_, vbd_record) ->
+				vbd_record.Db_actions.vBD_VDI = _ref' && vbd_record.Db_actions.vBD_current_operations <> []
+			) vbd_records)
+			in
 
 			(* If the VBD is currently_attached then some operations can still be performed ie:
 			   VDI.clone (if the VM is suspended we have to have the 'allow_clone_suspended_vm'' flag)
 			   VDI.snapshot; VDI.resize_online; 'blocked' (CP-831) *)
 			let operation_can_be_performed_live = match op with
-			| `snapshot -> true
-			| `resize_online -> true
-			| `blocked -> true
-			| `clone -> true
+			| `snapshot | `resize_online | `blocked | `clone -> true
 			| _ -> false in
 
 			(* NB RO vs RW sharing checks are done in xapi_vbd.ml *)
 
 			let sr_uuid = Db.SR.get_uuid ~__context ~self:sr in
 			let sm_features = Xapi_sr_operations.features_of_sr_internal ~_type:sr_type ~uuid:sr_uuid in
 
-			let any_vbd p = List.fold_left (||) false (List.map p my_vbd_records) in
-			if not operation_can_be_performed_live && (any_vbd is_active)
+			if not operation_can_be_performed_live && (my_active_vbd_records <> [])
 			then Some (Api_errors.vdi_in_use,[_ref; (Record_util.vdi_operation_to_string op)])
-			else if any_vbd has_current_operation
+			else if my_has_current_operation_vbd_records <> []
 			then Some (Api_errors.other_operation_in_progress, [ "VDI"; _ref ])
 			else (
 				match op with