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

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.

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"]

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)

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)

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
+    )
+  ]

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"

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

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