Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CA-402921: Relax VIF constraint for PVS proxy #6189

Merged
merged 4 commits into from
Dec 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions ocaml/idl/datamodel_errors.ml
Original file line number Diff line number Diff line change
Expand Up @@ -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"]
Expand Down
1 change: 1 addition & 0 deletions ocaml/tests/suite_alcotest.ml
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
31 changes: 27 additions & 4 deletions ocaml/tests/test_pvs_proxy.ml
Original file line number Diff line number Diff line change
Expand Up @@ -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 () =
Expand Down Expand Up @@ -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)
Expand Down
90 changes: 90 additions & 0 deletions ocaml/tests/test_vif_helpers.ml
Original file line number Diff line number Diff line change
@@ -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
)
]
5 changes: 5 additions & 0 deletions ocaml/xapi-consts/api_errors.ml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
15 changes: 12 additions & 3 deletions ocaml/xapi/xapi_pvs_proxy.ml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be clearer to define a function minimum that takes a non-empty list of integers.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then we'd have to separately handle the conversion from strings and the empy list case?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

val minimum: min:int -> int list -> int (** minimum of the list or min if the list is empty *)
...
client code like

| hd::tl -> minimum min:hd tl
..

Maybe it's nice but verbose.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO the current fold_left is more to the point.

(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
Expand Down
37 changes: 33 additions & 4 deletions ocaml/xapi/xapi_vif_helpers.ml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading