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.

Sign up for GitHub

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

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d08c6cb
CA-402921: Relax VIF constraint for PVS proxy
robhoes Dec 9, 2024
2f2ee29
CA-402921: Update PVS-proxy tests
robhoes Dec 11, 2024
14406ba
CA-402921: Restrict VIF.create
robhoes Dec 18, 2024
8192792
CA-402921: Add some unit tests for Xapi_vif_helpers
robhoes Dec 18, 2024
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
Next Next commit
CA-402921: Relax VIF constraint for PVS proxy 
The current constraint is that the VIF used for PVS proxy must have
device number 0. It turned out that this can be relaxed. It is
sufficient to enforce that the VIF is the one with the lowest device
number for the VM.

Signed-off-by: Rob Hoes <rob.hoes@citrix.com>
  • Loading branch information
@robhoes
robhoes committed Dec 18, 2024
commit d08c6cb8a2581726690b1eb6f78951f9a5abf1a4
5 changes: 5 additions & 0 deletions ocaml/idl/datamodel_errors.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1818,6 +1818,11 @@ 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.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
2 changes: 2 additions & 0 deletions ocaml/xapi-consts/api_errors.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1246,6 +1246,8 @@ 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 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
View file
Open in desktop
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