core: leverage the vm-lock in SetVmTicket

Previously, we used optimisic locking to see if the user that tries to connect to the VM can do that or not. However, now that we lock the VM using VmManager's lock, we don't need that anymore - we can rather inspect VmDynamic in the command itself to discover it. Signed-off-by: Arik Hadas <ahadas@redhat.com>
oVirt · May 19, 2022 · ac2476d · ac2476d
1 parent c59f909
commit ac2476d
Show file tree

Hide file tree

Showing 5 changed files with 20 additions and 91 deletions.
diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/SetVmTicketCommand.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/SetVmTicketCommand.java
@@ -1,6 +1,7 @@
 package org.ovirt.engine.core.bll;
 
 import java.util.List;
+import java.util.Objects;
 
 import javax.inject.Inject;
 
@@ -14,6 +15,7 @@
 import org.ovirt.engine.core.common.businessentities.OriginType;
 import org.ovirt.engine.core.common.businessentities.VM;
 import org.ovirt.engine.core.common.businessentities.VMStatus;
+import org.ovirt.engine.core.common.businessentities.VmDynamic;
 import org.ovirt.engine.core.common.businessentities.aaa.DbUser;
 import org.ovirt.engine.core.common.errors.EngineMessage;
 import org.ovirt.engine.core.common.vdscommands.SetVmTicketVDSCommandParameters;
@@ -113,9 +115,7 @@ protected boolean validate() {
         // Check that the virtual machine exists:
         final VM vm = getVm();
         if (vm == null) {
-            addValidationMessage(EngineMessage.ACTION_TYPE_FAILED_VM_NOT_FOUND);
-
-            return false;
+            return failValidation(EngineMessage.ACTION_TYPE_FAILED_VM_NOT_FOUND);
         }
 
         if (!canRunActionOnNonManagedVm()) {
@@ -142,16 +142,12 @@ protected void perform() {
             ticket = Ticketing.generateOTP();
         }
 
+        final DbUser user = getCurrentUser();
+
         VmManager vmManager = resourceManager.getVmManager(getParameters().getVmId());
         vmManager.lockVm();
-
         try {
-            // Update the dynamic information of the virtual machine in memory (we need it
-            // to update the database later):
-            final VM vm = getVm();
-            final DbUser user = getCurrentUser();
-            vm.setConsoleUserId(user.getId());
-            vm.setConsoleCurrentUserName(getConsoleUserName());
+            var vm = vmDynamicDao.get(getVm().getId());
 
             // If the virtual machine has the allow reconnect flag or the user
             // needed additional permissions to connect to the console then we just
@@ -166,12 +162,16 @@ protected void perform() {
             // and proceed only if the previous user in the database is null. This
             // is needed to prevent races between different users trying to access
             // the console of the same virtual machine simultaneously.
-            if (vm.getAllowConsoleReconnect() || neededPermissions) {
-                vmManager.update(vm.getDynamicData());
+            if (getVm().getAllowConsoleReconnect() || neededPermissions) {
+                if (!Objects.equals(user.getId(), vm.getConsoleUserId())) {
+                    updateConsoleUser(user, vmManager, vm);
+                }
                 sendTicket();
             } else {
-                final boolean saved = vmDynamicDao.updateConsoleUserWithOptimisticLocking(vm.getDynamicData());
-                if (saved) {
+                if (vm.getConsoleUserId() == null || vm.getConsoleUserId().equals(user.getId())) {
+                    if (vm.getConsoleUserId() == null) {
+                        updateConsoleUser(user, vmManager, vm);
+                    }
                     sendTicket();
                 } else {
                     dontSendTicket();
@@ -182,6 +182,12 @@ protected void perform() {
         }
     }
 
+    private void updateConsoleUser(final DbUser user, VmManager vmManager, VmDynamic vm) {
+        vm.setConsoleUserId(user.getId());
+        vm.setConsoleCurrentUserName(getConsoleUserName());
+        vmManager.update(vm);
+    }
+
     /**
      * Don't send the ticket to the virtual machine and send a warning indicating
      * that two users are trying to connect to the console of the virtual machine

diff --git a/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VmDynamicDao.java b/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VmDynamicDao.java
@@ -67,16 +67,6 @@ public interface VmDynamicDao extends GenericDao<VmDynamic, Guid>, StatusAwareDa
     @Override
     void save(VmDynamic vm);
 
-    /**
-     * Update the console user name and id, but only if it was empty before. This
-     * method is needed in order to implement optimistic locking for the functionality
-     * that allows reconnection to the console without rebooting the virtual machine.
-     *
-     * @param vm the dynamic data of the virtual machine
-     * @return {@code true} if at least one row was updated, {@code false} otherwise
-     */
-    boolean updateConsoleUserWithOptimisticLocking(VmDynamic vm);
-
     void clearMigratingToVds(Guid id);
 
     void clearMigratingToVdsAndSetDynamicPinning(Guid id, String cpuPinning, String numaPinning);

diff --git a/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VmDynamicDaoImpl.java b/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VmDynamicDaoImpl.java
@@ -2,7 +2,6 @@
 
 import java.sql.ResultSet;
 import java.sql.SQLException;
-import java.sql.Types;
 import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.List;
@@ -92,22 +91,6 @@ public void clearMigratingToVdsAndSetDynamicPinning(Guid id, String currentCpuPi
         getCallsHandler().executeModification("ClearMigratingToVdsAndSetDynamicPinning", parameterSource);
     }
 
-    @Override
-    public boolean updateConsoleUserWithOptimisticLocking(VmDynamic vm) {
-        MapSqlParameterSource parameterSource = getCustomMapSqlParameterSource()
-                .addValue("vm_guid", vm.getId())
-                .addValue("console_user_id", vm.getConsoleUserId())
-                .addValue("guest_cur_user_name", vm.getGuestCurrentUserName())
-                .addValue("console_cur_user_name", vm.getConsoleCurrentUserName());
-
-        Map<String, Object> results = getCallsHandler().executeModification("UpdateConsoleUserWithOptimisticLocking",
-                parameterSource,
-                "updated",
-                Types.BIT);
-
-        return (Boolean) results.get("updated");
-    }
-
     @Override
     public List<VmDynamic> getAll() {
         return getCallsHandler().executeReadList("GetAllFromVmDynamic",

diff --git a/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/VmDynamicDaoTest.java b/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/VmDynamicDaoTest.java
@@ -97,34 +97,6 @@ public void testUpdateAll() {
         assertEquals(existingVm2, dao.get(existingVm2.getId()));
     }
 
-    /**
-     * Make sure that saving a new console user id and console user name to a virtual machine
-     * without a previous console user succeeds and returns {@code true}.
-     */
-    @Test
-    public void testUpdateConsoleUserWithOptimisticLockingSuccess() {
-        VmDynamic vmWithoutConsoleUser = dao.get(FixturesTool.VM_RHEL5_POOL_51);
-        vmWithoutConsoleUser.setConsoleUserId(new Guid("9bf7c640-b620-456f-a550-0348f366544b"));
-
-        boolean result = dao.updateConsoleUserWithOptimisticLocking(vmWithoutConsoleUser);
-
-        assertTrue(result);
-    }
-
-    /**
-     * Make sure that saving a new console user id and console user name to a virtual machine
-     * that already as a previous console user fails and returns {@code false}.
-     */
-    @Test
-    public void testUpdateConsoleUserWithOptimisticLockingFailure() {
-        VmDynamic vmWithoutConsoleUser = dao.get(FixturesTool.VM_RHEL5_POOL_57);
-        vmWithoutConsoleUser.setConsoleUserId(new Guid("9bf7c640-b620-456f-a550-0348f366544b"));
-
-        boolean result = dao.updateConsoleUserWithOptimisticLocking(vmWithoutConsoleUser);
-
-        assertFalse(result);
-    }
-
     @Test
     public void testClearMigratingToVds() {
         VmDynamic vmDynamic = dao.get(FixturesTool.VM_RHEL5_POOL_51);

diff --git a/packaging/dbscripts/vms_sp.sql b/packaging/dbscripts/vms_sp.sql
@@ -576,28 +576,6 @@ BEGIN
 END;$PROCEDURE$
 LANGUAGE plpgsql;
 
-CREATE OR REPLACE FUNCTION UpdateConsoleUserWithOptimisticLocking (
-    v_vm_guid UUID,
-    v_console_user_id UUID,
-    v_guest_cur_user_name TEXT,
-    v_console_cur_user_name VARCHAR(255),
-    OUT v_updated BOOLEAN
-    ) AS $PROCEDURE$
-BEGIN
-    UPDATE vm_dynamic
-    SET console_user_id = v_console_user_id,
-        guest_cur_user_name = v_guest_cur_user_name,
-        console_cur_user_name = v_console_cur_user_name
-    WHERE vm_guid = v_vm_guid
-        AND (
-            console_user_id = v_console_user_id
-            OR console_user_id IS NULL
-            );
-
-    v_updated := FOUND;
-END;$PROCEDURE$
-LANGUAGE plpgsql;
-
 CREATE OR REPLACE FUNCTION UpdateVmDynamicStatus (
     v_vm_guid UUID,
     v_status INT