diff --git a/ocaml/idl/datamodel_errors.ml b/ocaml/idl/datamodel_errors.ml index 80b36218f25..fed2f830db1 100644 --- a/ocaml/idl/datamodel_errors.ml +++ b/ocaml/idl/datamodel_errors.ml @@ -1818,6 +1818,18 @@ let _ = "The address specified is already in use by an existing PVS_server object" () ; + error Api_errors.pvs_vif_must_be_first_device [] + ~doc: + "The VIF used by PVS proxy must be the one with the lowest device number" + () ; + + error Api_errors.pvs_proxy_present_on_higher_vif_device ["device"] + ~doc: + "The VM has a VIF, with a higher device number than the new VIF, that \ + uses a PVS proxy. The VIF used by PVS proxy must be the one with the \ + lowest device number." + () ; + error Api_errors.usb_group_contains_vusb ["vusbs"] ~doc:"The USB group contains active VUSBs and cannot be deleted." () ; error Api_errors.usb_group_contains_pusb ["pusbs"] diff --git a/ocaml/tests/suite_alcotest.ml b/ocaml/tests/suite_alcotest.ml index c2e422c2379..7c425e39639 100644 --- a/ocaml/tests/suite_alcotest.ml +++ b/ocaml/tests/suite_alcotest.ml @@ -37,6 +37,7 @@ let () = ; ("Test_pvs_site", Test_pvs_site.test) ; ("Test_pvs_proxy", Test_pvs_proxy.test) ; ("Test_pvs_server", Test_pvs_server.test) + ; ("Test_vif_helpers", Test_vif_helpers.test) ; ("Test_vm_memory_constraints", Test_vm_memory_constraints.test) ; ("Test_xapi_xenops", Test_xapi_xenops.test) ; ("Test_network_event_loop", Test_network_event_loop.test) diff --git a/ocaml/tests/test_pvs_proxy.ml b/ocaml/tests/test_pvs_proxy.ml index 2c102c0ca32..47e41ab3c24 100644 --- a/ocaml/tests/test_pvs_proxy.ml +++ b/ocaml/tests/test_pvs_proxy.ml @@ -34,12 +34,28 @@ let test_create_ok () = "test_create_ok testing get_VIF" vIF (Db.PVS_proxy.get_VIF ~__context ~self:pvs_proxy) -let test_create_invalid_device () = +let test_create_ok_lowest_device_number () = let __context = T.make_test_database () in let site = T.make_pvs_site ~__context () in let vIF = T.make_vif ~__context ~device:"1" () in - Alcotest.check_raises "test_create_invalid_device should raise invalid_device" - Api_errors.(Server_error (invalid_device, ["1"])) + let _vIF' = T.make_vif ~__context ~device:"2" () in + let pvs_proxy = Xapi_pvs_proxy.create ~__context ~site ~vIF in + Alcotest.(check (Alcotest_comparators.ref ())) + "test_create_ok testing get_site" site + (Db.PVS_proxy.get_site ~__context ~self:pvs_proxy) ; + Alcotest.(check (Alcotest_comparators.ref ())) + "test_create_ok testing get_VIF" vIF + (Db.PVS_proxy.get_VIF ~__context ~self:pvs_proxy) + +let test_create_not_lowest_device_number () = + let __context = T.make_test_database () in + let site = T.make_pvs_site ~__context () in + let _vIF' = T.make_vif ~__context ~device:"0" () in + let vIF = T.make_vif ~__context ~device:"1" () in + Alcotest.check_raises + "test_create_not_lowest_device_number should raise \ + pvs_vif_must_be_first_device" + Api_errors.(Server_error (pvs_vif_must_be_first_device, [])) (fun () -> ignore (Xapi_pvs_proxy.create ~__context ~site ~vIF)) let test_create_invalid_site () = @@ -103,7 +119,14 @@ let test = [ ("test_unlicensed", `Quick, test_unlicensed) ; ("test_create_ok", `Quick, test_create_ok) - ; ("test_create_invalid_device", `Quick, test_create_invalid_device) + ; ( "test_create_ok_lowest_device_number" + , `Quick + , test_create_ok_lowest_device_number + ) + ; ( "test_create_not_lowest_device_number" + , `Quick + , test_create_not_lowest_device_number + ) ; ("test_create_invalid_site", `Quick, test_create_invalid_site) ; ("test_create_invalid_vif", `Quick, test_create_invalid_vif) ; ("test_destroy", `Quick, test_destroy) diff --git a/ocaml/tests/test_vif_helpers.ml b/ocaml/tests/test_vif_helpers.ml new file mode 100644 index 00000000000..2e92a3d7f01 --- /dev/null +++ b/ocaml/tests/test_vif_helpers.ml @@ -0,0 +1,90 @@ +(* + * Copyright (C) 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. + *) + +module T = Test_common + +(* Function under test *) +let create ~__context ~device ~network ~vM ?(mAC = "00:00:00:00:00:00") + ?(mTU = 1500L) ?(qos_algorithm_type = "") ?(qos_algorithm_params = []) + ?(currently_attached = false) ?(other_config = []) + ?(locking_mode = `unlocked) ?(ipv4_allowed = []) ?(ipv6_allowed = []) + ?(ipv4_configuration_mode = `None) ?(ipv4_addresses = []) + ?(ipv4_gateway = "") ?(ipv6_configuration_mode = `None) + ?(ipv6_addresses = []) ?(ipv6_gateway = "") () = + Xapi_vif_helpers.create ~__context ~device ~network ~vM ~mAC ~mTU + ~other_config ~qos_algorithm_type ~qos_algorithm_params ~currently_attached + ~locking_mode ~ipv4_allowed ~ipv6_allowed ~ipv4_configuration_mode + ~ipv4_addresses ~ipv4_gateway ~ipv6_configuration_mode ~ipv6_addresses + ~ipv6_gateway + +let test_create_ok () = + let __context = T.make_test_database () in + let vM = T.make_vm ~__context () in + let network = T.make_network ~__context () in + let vif = create ~__context ~device:"0" ~network ~vM () in + Alcotest.(check (Alcotest_comparators.ref ())) + "test_create_ok testing get_VM" vM + (Db.VIF.get_VM ~__context ~self:vif) + +let test_create_duplicate_device () = + let __context = T.make_test_database () in + let vM = T.make_vm ~__context () in + let network = T.make_network ~__context () in + let _vif0 = T.make_vif ~__context ~device:"0" ~vM ~network () in + Alcotest.check_raises + "test_create_duplicate_device should raise device_already_exists" + Api_errors.(Server_error (device_already_exists, ["0"])) + @@ fun () -> + let _ = create ~__context ~device:"0" ~network ~vM () in + () + +let test_create_with_pvs_proxy_ok () = + let __context = T.make_test_database () in + let vM = T.make_vm ~__context () in + let network = T.make_network ~__context () in + let vIF = T.make_vif ~__context ~device:"0" ~vM ~network () in + let _vIF2 = T.make_vif ~__context ~device:"2" ~vM ~network () in + let site = T.make_pvs_site ~__context () in + let _pvs_proxy = T.make_pvs_proxy ~__context ~site ~vIF () in + let vif1 = create ~__context ~device:"1" ~network ~vM () in + Alcotest.(check (Alcotest_comparators.ref ())) + "test_create_with_pvs_proxy_ok testing get_VM" vM + (Db.VIF.get_VM ~__context ~self:vif1) + +let test_create_with_pvs_proxy_not_ok () = + let __context = T.make_test_database () in + let vM = T.make_vm ~__context () in + let network = T.make_network ~__context () in + let vIF = T.make_vif ~__context ~device:"1" ~vM ~network () in + let _vIF2 = T.make_vif ~__context ~device:"2" ~vM ~network () in + let site = T.make_pvs_site ~__context () in + let _pvs_proxy = T.make_pvs_proxy ~__context ~site ~vIF () in + Alcotest.check_raises + "test_create_with_pvs_proxy_not_ok should raise \ + pvs_proxy_present_on_higher_vif_device" + Api_errors.(Server_error (pvs_proxy_present_on_higher_vif_device, ["1"])) + @@ fun () -> + let _ = create ~__context ~device:"0" ~network ~vM () in + () + +let test = + [ + ("test_create_ok", `Quick, test_create_ok) + ; ("test_create_duplicate_device", `Quick, test_create_duplicate_device) + ; ("test_create_with_pvs_proxy_ok", `Quick, test_create_with_pvs_proxy_ok) + ; ( "test_create_with_pvs_proxy_not_ok" + , `Quick + , test_create_with_pvs_proxy_not_ok + ) + ] diff --git a/ocaml/xapi-consts/api_errors.ml b/ocaml/xapi-consts/api_errors.ml index 53e9e06176b..54bdd6f6660 100644 --- a/ocaml/xapi-consts/api_errors.ml +++ b/ocaml/xapi-consts/api_errors.ml @@ -1246,6 +1246,11 @@ let pvs_proxy_already_present = add_error "PVS_PROXY_ALREADY_PRESENT" let pvs_server_address_in_use = add_error "PVS_SERVER_ADDRESS_IN_USE" +let pvs_vif_must_be_first_device = add_error "PVS_VIF_MUST_BE_FIRST_DEVICE" + +let pvs_proxy_present_on_higher_vif_device = + add_error "PVS_PROXY_PRESENT_ON_HIGHER_VIF_DEVICE" + let extension_protocol_failure = add_error "EXTENSION_PROTOCOL_FAILURE" let usb_group_contains_vusb = add_error "USB_GROUP_CONTAINS_VUSB" diff --git a/ocaml/xapi/xapi_pvs_proxy.ml b/ocaml/xapi/xapi_pvs_proxy.ml index 3f81ffc783e..5c3545952a5 100644 --- a/ocaml/xapi/xapi_pvs_proxy.ml +++ b/ocaml/xapi/xapi_pvs_proxy.ml @@ -32,9 +32,18 @@ let create ~__context ~site ~vIF = ) ; Helpers.assert_is_valid_ref ~__context ~name:"site" ~ref:site ; Helpers.assert_is_valid_ref ~__context ~name:"VIF" ~ref:vIF ; - let device = Db.VIF.get_device ~__context ~self:vIF in - if device <> "0" then - raise Api_errors.(Server_error (invalid_device, [device])) ; + let device = Db.VIF.get_device ~__context ~self:vIF |> int_of_string in + let min_device = + let open Xapi_database.Db_filter_types in + let vm = Db.VIF.get_VM ~__context ~self:vIF in + Db.VIF.get_records_where ~__context + ~expr:(Eq (Field "VM", Literal (Ref.string_of vm))) + |> List.fold_left + (fun m (_, {API.vIF_device= d; _}) -> min m (int_of_string d)) + device + in + if device <> min_device then + raise Api_errors.(Server_error (pvs_vif_must_be_first_device, [])) ; let pvs_proxy = Ref.make () in let uuid = Uuidx.(to_string (make ())) in Db.PVS_proxy.create ~__context ~ref:pvs_proxy ~uuid ~site ~vIF diff --git a/ocaml/xapi/xapi_vif_helpers.ml b/ocaml/xapi/xapi_vif_helpers.ml index 4a469b84368..da6ede482fa 100644 --- a/ocaml/xapi/xapi_vif_helpers.ml +++ b/ocaml/xapi/xapi_vif_helpers.ml @@ -287,13 +287,42 @@ let create ~__context ~device ~network ~vM ~mAC ~mTU ~other_config ) ; (* Check to make sure the device is unique *) Xapi_stdext_threads.Threadext.Mutex.execute m (fun () -> - let all = Db.VM.get_VIFs ~__context ~self:vM in - let all_devices = - List.map (fun self -> Db.VIF.get_device ~__context ~self) all + let all_vifs_with_devices = + Db.VM.get_VIFs ~__context ~self:vM + |> List.map (fun self -> + (self, int_of_string (Db.VIF.get_device ~__context ~self)) + ) in - if List.mem device all_devices then + let new_device = int_of_string device in + if List.exists (fun (_, d) -> d = new_device) all_vifs_with_devices then raise (Api_errors.Server_error (Api_errors.device_already_exists, [device])) ; + + (* If the VM uses a PVS_proxy, then the proxy _must_ be associated with + the VIF that has the lowest device number. Check that the new VIF + respects this. *) + ( match all_vifs_with_devices with + | [] -> + () + | hd :: tl -> + let min_vif, min_device = + List.fold_left + (fun ((_, d) as v) ((_, d') as v') -> if d' < d then v' else v) + hd tl + in + let vm_has_pvs_proxy = + Pvs_proxy_control.find_proxy_for_vif ~__context ~vif:min_vif <> None + in + if vm_has_pvs_proxy && new_device < min_device then + raise + Api_errors.( + Server_error + ( pvs_proxy_present_on_higher_vif_device + , [Printf.sprintf "%d" min_device] + ) + ) + ) ; + let metrics = Ref.make () and metrics_uuid = Uuidx.to_string (Uuidx.make ()) in Db.VIF_metrics.create ~__context ~ref:metrics ~uuid:metrics_uuid