idl/datamodel_errors: add reason to ha_operation_would_break_failover_plan

psafont · psafont · commit 6cc4afa7564c · 2025-09-12T12:00:33.000+01:00
This allows users to better see the cause of this error without having
to go to the logs of the host running the operation.

Also remove the additional logline now that the reason is going to be
logged as part of the error.

Signed-off-by: Pau Ruiz Safont &lt;pau.safont@vates.tech&gt;
diff --git a/ocaml/idl/datamodel_errors.ml b/ocaml/idl/datamodel_errors.ml
@@ -1604,7 +1604,7 @@ let _ =
        must be physically present via a currently_attached PIF on every host."
     () ;
 
-  error Api_errors.ha_operation_would_break_failover_plan []
+  error Api_errors.ha_operation_would_break_failover_plan ["reason"]
     ~doc:
       "This operation cannot be performed because it would invalidate VM \
        failover planning such that the system would be unable to guarantee to \
diff --git a/ocaml/xapi/xapi_ha_vm_failover.ml b/ocaml/xapi/xapi_ha_vm_failover.ml
@@ -1223,12 +1223,13 @@ let assert_configuration_change_preserves_ha_plan ~__context c =
             "assert_configuration_change_preserves_ha_plan: plan exists after \
              change"
       | Plan_exists_excluding_non_agile_VMs | No_plan_exists ->
-          debug
+          let reason =
             "assert_configuration_change_preserves_ha_plan: proposed change \
-             breaks plan" ;
+             breaks plan"
+          in
           raise
-            (Api_errors.Server_error
-               (Api_errors.ha_operation_would_break_failover_plan, [])
+            Api_errors.(
+              Server_error (ha_operation_would_break_failover_plan, [reason])
             )
     )
 
@@ -1413,16 +1414,19 @@ let restart_auto_run_vms ~__context ~last_live_set ~live_set n =
   let open TaskChains.Infix in
   (* execute the plan *)
   Helpers.call_api_functions ~__context (fun rpc session_id ->
-      (* Helper function to start a VM somewhere. If the HA overcommit protection stops us then disable it and try once more.
-         			   Returns true if the VM was restarted and false otherwise. *)
+      (* Helper function to start a VM somewhere. If the HA overcommit
+         protection stops us then disable it and try once more. Returns true if
+         the VM was restarted and false otherwise. *)
       let restart_vm vm ?host () =
         let go () =
           ( if Xapi_fist.simulate_restart_failure () then
               match Random.int 3 with
               | 0 ->
+                  let reason = "Simulated restart failure" in
                   raise
-                    (Api_errors.Server_error
-                       (Api_errors.ha_operation_would_break_failover_plan, [])
+                    Api_errors.(
+                      Server_error
+                        (ha_operation_would_break_failover_plan, [reason])
                     )
               | 1 ->
                   raise
@@ -1579,10 +1583,11 @@ let restart_auto_run_vms ~__context ~last_live_set ~live_set n =
       in
       gc_table last_start_attempt ;
       gc_table restart_failed ;
-      (* Consider restarting the best-effort VMs we *think* have failed (but we might get this wrong --
-         			   ok since this is 'best-effort'). NOTE we do not use the restart_vm function above as this will mark the
-         			   pool as overcommitted if an HA_OPERATION_WOULD_BREAK_FAILOVER_PLAN is received (although this should never
-         			   happen it's better safe than sorry) *)
+      (* Consider restarting the best-effort VMs we *think* have failed (but we
+         might get this wrong -- ok since this is 'best-effort'). NOTE we do
+         not use the restart_vm function above as this will mark the pool as
+         overcommitted if an HA_OPERATION_WOULD_BREAK_FAILOVER_PLAN is received
+         (although this should never happen it's better safe than sorry) *)
       let is_best_effort r =
         r.API.vM_ha_restart_priority = Constants.ha_restart_best_effort
         && r.API.vM_power_state = `Halted
diff --git a/ocaml/xapi/xapi_pbd.ml b/ocaml/xapi/xapi_pbd.ml
@@ -113,16 +113,17 @@ let abort_if_storage_attached_to_protected_vms ~__context ~self =
         List.iter
           (fun vbd ->
             let vdi = Db.VBD.get_VDI ~__context ~self:vbd in
-            if List.mem vdi vdis then (
-              warn
-                "PBD.unplug will make protected VM %s not agile since it has a \
-                 VBD attached to VDI %s"
-                (Ref.string_of vm_ref) (Ref.string_of vdi) ;
+            if List.mem vdi vdis then
+              let reason =
+                Printf.sprintf
+                  "PBD.unplug will make protected VM %s not agile since it has \
+                   a VBD attached to VDI %s"
+                  (Ref.string_of vm_ref) (Ref.string_of vdi)
+              in
               raise
-                (Api_errors.Server_error
-                   (Api_errors.ha_operation_would_break_failover_plan, [])
+                Api_errors.(
+                  Server_error (ha_operation_would_break_failover_plan, [reason])
                 )
-            )
           )
           vbds
       )
diff --git a/ocaml/xapi/xapi_pif.ml b/ocaml/xapi/xapi_pif.ml
@@ -270,16 +270,17 @@ let abort_if_network_attached_to_protected_vms ~__context ~self =
     let vms = List.map (fun vif -> Db.VIF.get_VM ~__context ~self:vif) vifs in
     List.iter
       (fun vm ->
-        if Helpers.is_xha_protected ~__context ~self:vm then (
-          warn
-            "PIF.unplug will make protected VM %s not agile since it has a VIF \
-             attached to network %s"
-            (Ref.string_of vm) (Ref.string_of net) ;
+        if Helpers.is_xha_protected ~__context ~self:vm then
+          let reason =
+            Printf.sprintf
+              "PIF.unplug will make protected VM %s not agile since it has a \
+               VIF attached to network %s"
+              (Ref.string_of vm) (Ref.string_of net)
+          in
           raise
-            (Api_errors.Server_error
-               (Api_errors.ha_operation_would_break_failover_plan, [])
+            Api_errors.(
+              Server_error (ha_operation_would_break_failover_plan, [reason])
             )
-        )
       )
       vms
 
diff --git a/ocaml/xapi/xapi_vbd_helpers.ml b/ocaml/xapi/xapi_vbd_helpers.ml
@@ -368,14 +368,15 @@ let assert_doesnt_make_vm_non_agile ~__context ~vm ~vdi =
     && (not (Db.Pool.get_ha_allow_overcommit ~__context ~self:pool))
     && Helpers.is_xha_protected ~__context ~self:vm
     && not properly_shared
-  then (
-    warn "Attaching VDI %s makes VM %s not agile" (Ref.string_of vdi)
-      (Ref.string_of vm) ;
+  then
+    let reason =
+      Printf.sprintf "Attaching VDI %s makes VM %s not agile"
+        (Ref.string_of vdi) (Ref.string_of vm)
+    in
     raise
-      (Api_errors.Server_error
-         (Api_errors.ha_operation_would_break_failover_plan, [])
+      Api_errors.(
+        Server_error (ha_operation_would_break_failover_plan, [reason])
       )
-  )
 
 let update_allowed_operations ~__context ~self : unit =
   let all = Db.VBD.get_record_internal ~__context ~self in
diff --git a/ocaml/xapi/xapi_vif_helpers.ml b/ocaml/xapi/xapi_vif_helpers.ml
@@ -267,19 +267,21 @@ let create ~__context ~device ~network ~vM ~mAC ~mTU ~other_config
     raise (Api_errors.Server_error (Api_errors.mac_invalid, [mAC])) ;
   (* Make people aware that non-shared networks being added to VMs makes them not agile *)
   let pool = Helpers.get_pool ~__context in
-  if
-    true
-    && Db.Pool.get_ha_enabled ~__context ~self:pool
-    && (not (Db.Pool.get_ha_allow_overcommit ~__context ~self:pool))
-    && Helpers.is_xha_protected ~__context ~self:vM
-    && not (Agility.is_network_properly_shared ~__context ~self:network)
-  then (
-    warn "Creating VIF %s makes VM %s not agile" (Ref.string_of ref)
-      (Ref.string_of vM) ;
-    raise
-      (Api_errors.Server_error
-         (Api_errors.ha_operation_would_break_failover_plan, [])
-      )
+  ( if
+      true
+      && Db.Pool.get_ha_enabled ~__context ~self:pool
+      && (not (Db.Pool.get_ha_allow_overcommit ~__context ~self:pool))
+      && Helpers.is_xha_protected ~__context ~self:vM
+      && not (Agility.is_network_properly_shared ~__context ~self:network)
+    then
+      let reason =
+        Printf.sprintf "Creating VIF %s makes VM %s not agile"
+          (Ref.string_of ref) (Ref.string_of vM)
+      in
+      raise
+        Api_errors.(
+          Server_error (ha_operation_would_break_failover_plan, [reason])
+        )
   ) ;
   (* Check to make sure the device is unique *)
   Xapi_stdext_threads.Threadext.Mutex.execute m (fun () ->
@@ -291,8 +293,7 @@ let create ~__context ~device ~network ~vM ~mAC ~mTU ~other_config
       in
       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])) ;
+        raise Api_errors.(Server_error (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
