Merge pull request xapi-project#1577 from johnelse/ca-122441

CA-122441: Update group supported/enabled types after GCing PGPUs

ocaml/test/OMakefile

@@ -33,6 +33,7 @@ OCAML_OBJS = \
 	test_pool_license \
 	test_platformdata \
 	test_sm_features \
+	test_gpu_group \
 	test_pci_db \
 	test_pci_helpers \
 	test_vgpu_type \

ocaml/test/suite.ml

@@ -25,6 +25,7 @@ let base_suite =
 			Test_pool_license.test;
 			Test_platformdata.test;
 			Test_sm_features.test;
+			Test_gpu_group.test;
 			Test_pci_db.test;
 			Test_pci_helpers.test;
 			Test_vgpu_type.test;

ocaml/test/test_common.ml

@@ -129,18 +129,20 @@ let make_pool ~__context ~master ?(name_label="") ?(name_description="")
 		~restrictions ~other_config;
 	pool_ref
 
-let make_sr ~__context ~ref ?(uuid=make_uuid ()) ?(name_label="") ?(name_description="") ?(allowed_operations=[])
+let make_sr ~__context ?(ref=Ref.make ()) ?(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
+		~local_cache_enabled ~introduced_by;
+	ref
 
-let make_pbd ~__context ~ref ?(uuid=make_uuid ()) ?(host=Ref.make ()) ?(sR=Ref.make ())
+let make_pbd ~__context ?(ref=Ref.make ()) ?(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
+	Db.PBD.create ~__context ~ref ~uuid ~host ~sR ~device_config ~currently_attached ~other_config;
+	ref
 
 let make_vbd ~__context ?(ref=Ref.make ()) ?(uuid=make_uuid ()) ?(allowed_operations=[])
 		?(current_operations=[]) ?(vM=Ref.make ()) ?(vDI=Ref.make ()) ?(device="")
@@ -153,7 +155,8 @@ let make_vbd ~__context ?(ref=Ref.make ()) ?(uuid=make_uuid ()) ?(allowed_operat
 	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
+		~qos_algorithm_params ~qos_supported_algorithms ~metrics;
+	ref
 
 let make_vdi ~__context ?(ref=Ref.make ()) ?(uuid=make_uuid ()) ?(name_label="")
 		?(name_description="") ?(allowed_operations=[]) ?(current_operations=[]) ?(sR=Ref.make ())
@@ -165,15 +168,17 @@ let make_vdi ~__context ?(ref=Ref.make ()) ?(uuid=make_uuid ()) ?(name_label="")
 	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
+		~snapshot_time ~tags ~allow_caching ~on_boot ~metadata_of_pool ~metadata_latest;
+	ref
 
 let make_pci ~__context ?(ref=Ref.make ()) ?(uuid=make_uuid ()) ?(class_id="")
 		?(class_name="") ?(vendor_id="") ?(vendor_name="") ?(device_id="")
 		?(device_name="") ?(host=Ref.null) ?(pci_id="") ?(functions=0L)
 		?(dependencies=[]) ?(other_config=[]) () =
 	Db.PCI.create ~__context ~ref ~uuid ~class_id ~class_name ~vendor_id
 		~vendor_name ~device_id ~device_name ~host ~pci_id ~functions ~dependencies
-		~other_config
+		~other_config;
+	ref
 
 let make_pgpu ~__context ?(ref=Ref.make ()) ?(uuid=make_uuid ()) ?(pCI=Ref.null)
 		?(gPU_group=Ref.null) ?(host=Ref.null) ?(other_config=[])
@@ -185,22 +190,28 @@ let make_pgpu ~__context ?(ref=Ref.make ()) ?(uuid=make_uuid ()) ?(pCI=Ref.null)
 	Db.PGPU.set_supported_VGPU_types ~__context ~self:ref
 		~value:supported_VGPU_types;
 	Db.PGPU.set_enabled_VGPU_types ~__context ~self:ref
-		~value:enabled_VGPU_types
+		~value:enabled_VGPU_types;
+	ref
 
 let make_gpu_group ~__context ?(ref=Ref.make ()) ?(uuid=make_uuid ())
 		?(name_label="") ?(name_description="") ?(gPU_types=[]) ?(other_config=[])
 		?(allocation_algorithm=`depth_first) () =
 	Db.GPU_group.create ~__context ~ref ~uuid ~name_label ~name_description
-		~gPU_types ~other_config ~allocation_algorithm
+		~gPU_types ~other_config ~allocation_algorithm;
+	ref
 
 let make_vgpu ~__context ?(ref=Ref.make ()) ?(uuid=make_uuid ()) ?(vM=Ref.null)
 		?(gPU_group=Ref.null) ?(device="0") ?(currently_attached=false)
 		?(other_config=[]) ?(_type=Ref.null) ?(resident_on=Ref.null) () =
 	Db.VGPU.create ~__context ~ref ~uuid ~vM ~gPU_group ~device ~currently_attached
-		~other_config ~_type ~resident_on
+		~other_config ~_type ~resident_on;
+	ref
 
 let make_vgpu_type ~__context ?(ref=Ref.make ()) ?(uuid=make_uuid ())
 		?(vendor_name="") ?(model_name="") ?(framebuffer_size=0L) ?(max_heads=0L)
-		?(size=0L) ?(internal_config=[]) () =
+		?(max_resolution_x=0L) ?(max_resolution_y=0L) ?(size=0L)
+		?(internal_config=[]) () =
 	Db.VGPU_type.create ~__context ~ref ~uuid ~vendor_name ~model_name
-		~framebuffer_size ~max_heads ~size ~internal_config
+		~framebuffer_size ~max_heads ~max_resolution_x ~max_resolution_y ~size
+		~internal_config;
+	ref

ocaml/test/test_gpu_group.ml

@@ -0,0 +1,79 @@
+(*
+ * Copyright (C) 2013 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_vgpu_common
+
+let test_supported_enabled_types () =
+	let __context = Test_common.make_test_database () in
+	(* Create a GPU group containing a single K2 PGPU. *)
+	let host = List.hd (Db.Host.get_all ~__context) in
+	let gPU_group = Test_common.make_gpu_group ~__context () in
+	let pgpu = make_pgpu ~__context ~host ~gPU_group default_k2 in
+	(* Update the group's enabled and supported types, and check that they
+	 * contain all the types enabled and supported by the PGPU. *)
+	Xapi_gpu_group.update_supported_VGPU_types ~__context ~self:gPU_group;
+	Xapi_gpu_group.update_enabled_VGPU_types ~__context ~self:gPU_group;
+	let vgpu_types_and_refs =
+		List.map
+			(fun vgpu_type ->
+				(vgpu_type, Xapi_vgpu_type.find_or_create ~__context vgpu_type))
+			k2_vgpu_types
+	in
+	let group_supported_types =
+		Db.GPU_group.get_supported_VGPU_types ~__context ~self:gPU_group
+	in
+	let group_enabled_types =
+		Db.GPU_group.get_enabled_VGPU_types ~__context ~self:gPU_group
+	in
+	List.iter
+		(fun (vgpu_type, vgpu_type_ref) ->
+			let msg_supported =
+				Printf.sprintf
+					"GPU group does not list %s as supported"
+					vgpu_type.Xapi_vgpu_type.model_name
+			in
+			let msg_enabled =
+				Printf.sprintf
+					"GPU group does not list %s as enabled"
+					vgpu_type.Xapi_vgpu_type.model_name
+			in
+			assert_bool msg_supported (List.mem vgpu_type_ref group_supported_types);
+			assert_bool msg_enabled (List.mem vgpu_type_ref group_enabled_types))
+		vgpu_types_and_refs;
+	(* Invalidate the PGPU's host ref, and run a GC pass; this should destroy the
+	 * pgpu, and clear the group's supported and enabled types. *)
+	Db.PGPU.set_host ~__context ~self:pgpu ~value:Ref.null;
+	Db_gc.gc_PGPUs ~__context;
+	let group_supported_types =
+		Db.GPU_group.get_supported_VGPU_types ~__context ~self:gPU_group
+	in
+	let group_enabled_types =
+		Db.GPU_group.get_enabled_VGPU_types ~__context ~self:gPU_group
+	in
+	assert_equal
+		~msg:"PGPU has not been destroyed"
+		(Db.is_valid_ref __context pgpu) false;
+	assert_equal
+		~msg:"GPU group still has supported types after GC"
+		group_supported_types [];
+	assert_equal
+		~msg:"GPU group still has enabled types after GC"
+		group_enabled_types []
+
+let test =
+	"test_gpu_group" >:::
+		[
+			"test_supported_enabled_types" >:: test_supported_enabled_types;
+		]

ocaml/test/test_helpers.ml

@@ -21,7 +21,7 @@ let test_vm_agility_with_vgpu () =
 	(* VM has no VIFs, VBDs or VGPUs, so should be agile. *)
 	Helpers.vm_assert_agile ~__context ~self:vm;
 	(* Create a VGPU - VM should no longer be agile. *)
-	make_vgpu ~__context ~vM:vm ();
+	let (_: API.ref_VGPU) = make_vgpu ~__context ~vM:vm () in
 	assert_raises_api_error
 		~args:[Ref.string_of vm]
 		Api_errors.vm_has_vgpu

ocaml/test/test_pgpu.ml

@@ -114,29 +114,30 @@ let test_remaining_capacity_decreases () =
 
 let test_set_GPU_group_succeeds_empty_pgpu () =
 	on_host_with_k2 (fun __context p ->
-		let group_ref = Ref.make () in
-		make_gpu_group ~__context ~ref:group_ref ();
+		let group_ref = make_gpu_group ~__context () in
 		Xapi_pgpu.set_GPU_group ~__context ~self:p ~value:group_ref)
 
 let test_set_GPU_group_succeeds_orphan_vgpu () =
 	(* This is OK since vGPUs can be created on empty GPU groups *)
 	on_host_with_k2 (fun __context p ->
-		match (Ref.make (), Ref.make ()) with
-		| (group, group') ->
-			List.iter (fun ref -> make_gpu_group ~__context ~ref ()) [group; group'];
-			Xapi_pgpu.set_GPU_group ~__context ~self:p ~value:group;
-			Test_common.make_vgpu ~__context ~gPU_group:group ();
-			Xapi_pgpu.set_GPU_group ~__context ~self:p ~value:group')
+		let group, group' =
+			(make_gpu_group ~__context (), make_gpu_group ~__context ())
+		in
+		Xapi_pgpu.set_GPU_group ~__context ~self:p ~value:group;
+		let (_: API.ref_VGPU) =
+			Test_common.make_vgpu ~__context ~gPU_group:group ()
+		in
+		Xapi_pgpu.set_GPU_group ~__context ~self:p ~value:group')
 
 let test_set_GPU_group_fails_resident_vgpu () =
 	on_host_with_k2 (fun __context p ->
-		match (Ref.make (), Ref.make ()) with
-		| (group, group') ->
-			List.iter (fun ref -> make_gpu_group ~__context ~ref ()) [group; group'];
-			Xapi_pgpu.set_GPU_group ~__context ~self:p ~value:group;
-			ignore (make_vgpu ~__context p k200);
-			assert_raises_api_error Api_errors.pgpu_in_use_by_vm (fun () ->
-				Xapi_pgpu.set_GPU_group ~__context ~self:p ~value:group'))
+		let group, group' =
+			(make_gpu_group ~__context (), make_gpu_group ~__context ())
+		in
+		Xapi_pgpu.set_GPU_group ~__context ~self:p ~value:group;
+		ignore (make_vgpu ~__context p k200);
+		assert_raises_api_error Api_errors.pgpu_in_use_by_vm (fun () ->
+			Xapi_pgpu.set_GPU_group ~__context ~self:p ~value:group'))
 
 let test =
 	"test_pgpu" >:::

ocaml/test/test_vdi_allowed_operations.ml

@@ -18,10 +18,9 @@ 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 sr_ref = make_sr ~__context () in
+	let (_: API.ref_PBD) = make_pbd ~__context ~sR:sr_ref () in
+	let vdi_ref = make_vdi ~__context ~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

ocaml/test/test_vgpu_common.ml

@@ -127,30 +127,25 @@ let make_vgpu ~__context pgpu_ref vgpu_type =
 			Some vm_ref
 		end else None
 	in
-	let vgpu_ref = Ref.make () in
 	Test_common.make_vgpu ~__context
 		~vM:(Opt.default Ref.null vm_ref_opt)
-		~ref:vgpu_ref ~_type:vgpu_type_ref
-		~resident_on:pgpu_ref ();
-	vgpu_ref
+		~_type:vgpu_type_ref
+		~resident_on:pgpu_ref ()
 
 let make_pgpu ~__context ?(host=Ref.null) ?(gPU_group=Ref.null) pgpu =
-	let pCI = Ref.make () in
-	Test_common.make_pci ~__context ~ref:pCI ~host ~functions:1L ();
+	let pCI = Test_common.make_pci ~__context ~host ~functions:1L () in
 	let supported_VGPU_types =
 		List.map (find_or_create ~__context) pgpu.supported_VGPU_types
 	in
 	let enabled_VGPU_types =
 		List.map (find_or_create ~__context) pgpu.supported_VGPU_types
 	in
-	let pgpu_ref = Ref.make () in
-	Test_common.make_pgpu ~__context
-		~ref:pgpu_ref
+	let pgpu_ref = Test_common.make_pgpu ~__context
 		~pCI
 		~host
 		~gPU_group
 		~supported_VGPU_types
-		~enabled_VGPU_types ();
+		~enabled_VGPU_types () in
 	List.iter
 		(fun vgpu_type ->
 			let (_: API.ref_VGPU) = (make_vgpu ~__context pgpu_ref vgpu_type) in ())

ocaml/test/test_vm_helpers.ml

@@ -33,8 +33,7 @@ let on_pool_of_k1s (f : Context.t -> API.ref_host -> API.ref_host -> API.ref_hos
 	(* Make two more hosts *)
 	match (make_host ~__context (), make_host ~__context ()) with
 	| (h', h'') ->
-		let gPU_group = Ref.make () in
-		make_gpu_group ~__context ~ref:gPU_group ();
+		let gPU_group = make_gpu_group ~__context () in
 		let rec make_k1s_on (host, num) =
 			if num > 0 then
 				let _ = make_pgpu ~__context ~host ~gPU_group default_k1 in

ocaml/xapi/db_gc.ml

@@ -16,6 +16,7 @@
  *)
 
 open API
+open Fun
 open Listext
 open Threadext
 
@@ -123,13 +124,34 @@ let gc_VGPUs ~__context =
   gc_connector ~__context Db.VGPU.get_all Db.VGPU.get_record (fun x->valid_ref __context x.vGPU_VM) (fun x->valid_ref __context x.vGPU_GPU_group)
     (fun ~__context ~self ->
        Db.VGPU.destroy ~__context ~self)
+
 let gc_PGPUs ~__context =
-  let pgpus = Db.PGPU.get_all ~__context in
-    List.iter
-      (fun pgpu ->
-        if not (valid_ref __context (Db.PGPU.get_host ~__context ~self:pgpu)) then
-          Db.PGPU.destroy ~__context ~self:pgpu
-      ) pgpus
+	let pgpus = Db.PGPU.get_all ~__context in
+	(* Go through the list of PGPUs, destroying any with an invalid host ref.
+	 * Keep a list of groups which contained PGPUs which were destroyed. *)
+	let affected_groups =
+		List.fold_left
+			(fun acc pgpu ->
+				if not (valid_ref __context (Db.PGPU.get_host ~__context ~self:pgpu))
+				then begin
+					let group = Db.PGPU.get_GPU_group ~__context ~self:pgpu in
+					Db.PGPU.destroy ~__context ~self:pgpu;
+					debug "GCed PGPU %s" (Ref.string_of pgpu);
+					group :: acc
+				end else
+					acc)
+			[] pgpus
+		|> List.filter (valid_ref __context)
+		|> List.setify
+	in
+	(* Update enabled/supported VGPU types on the groups which contained the
+	 * destroyed PGPUs. *)
+	List.iter
+		(fun group ->
+			Xapi_gpu_group.update_enabled_VGPU_types ~__context ~self:group;
+			Xapi_gpu_group.update_supported_VGPU_types ~__context ~self:group)
+		affected_groups
+
 let gc_PBDs ~__context =
   gc_connector ~__context Db.PBD.get_all Db.PBD.get_record (fun x->valid_ref __context x.pBD_host) (fun x->valid_ref __context x.pBD_SR) Db.PBD.destroy
 let gc_Host_patches ~__context =