Roy Golan has uploaded a new change for review. Change subject: core: VURTI - fix handle expection in GetVmStats ......................................................................
core: VURTI - fix handle expection in GetVmStats after getting all vms with List command, VURTI loops through them to update internal structure data. BUT if the call fails, VURTI removes the VM from the internal running map - this leads to a split-brains for VURTI assumes the Vm is DOWN and if it HA, its runs it. the fix is to exclude this Vm from the list of the vms that have changed this is similar to a case where Vm didn't changed its status UNIT TEST added a unit test to make sure the VM is kept in the map although the call to GetVmStats didn't made it Bug-Url: https://bugzilla.redhat.com/1072282 Change-Id: I415804d9fa1f1288423241b8547f4cab5540914a Signed-off-by: Roy Golan <[email protected]> --- M backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsManager.java M backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsUpdateRunTimeInfo.java M backend/manager/modules/vdsbroker/src/test/java/org/ovirt/engine/core/vdsbroker/VdsUpdateRunTimeInfoTest.java 3 files changed, 177 insertions(+), 31 deletions(-) git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/36/25636/1 diff --git a/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsManager.java b/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsManager.java index ad6185f..a0f7309 100644 --- a/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsManager.java +++ b/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsManager.java @@ -65,7 +65,7 @@ return (_refreshIteration == _numberRefreshesBeforeSave); } - private static final int VDS_REFRESH_RATE = Config.<Integer> GetValue(ConfigValues.VdsRefreshRate) * 1000; + private int VDS_REFRESH_RATE = Config.<Integer> GetValue(ConfigValues.VdsRefreshRate) * 1000; private String onTimerJobId; @@ -97,7 +97,7 @@ private final AtomicInteger mUnrespondedAttempts; private final AtomicBoolean sshSoftFencingExecuted; - private static final int VDS_DURING_FAILURE_TIMEOUT_IN_MINUTES = Config + private int VDS_DURING_FAILURE_TIMEOUT_IN_MINUTES = Config .<Integer> GetValue(ConfigValues.TimeToReduceFailedRunOnVdsInMinutes); private String duringFailureJobId; private boolean privateInitialized; diff --git a/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsUpdateRunTimeInfo.java b/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsUpdateRunTimeInfo.java index 3ad788d..568e6f9 100644 --- a/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsUpdateRunTimeInfo.java +++ b/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsUpdateRunTimeInfo.java @@ -898,20 +898,12 @@ auditLog(logable, AuditLogType.VDS_DETECTED); } - private void refreshVmStats() { + protected void refreshVmStats() { if (Config.<Boolean> GetValue(ConfigValues.DebugTimerLogging)) { log.debug("vds::refreshVmList entered"); } - VDSReturnValue vdsReturnValue; - VDSCommandType commandType = - _vdsManager.getRefreshStatistics() - ? VDSCommandType.GetAllVmStats - : VDSCommandType.List; - vdsReturnValue = getResourceManager().runVdsCommand(commandType, new VdsIdAndVdsVDSCommandParametersBase(_vds)); - _runningVms = (Map<Guid, VmInternalData>) vdsReturnValue.getReturnValue(); - - if (vdsReturnValue.getSucceeded()) { + if (fetchRunningVms()) { List<Guid> staleRunningVms = checkVmsStatusChanged(); proceedWatchdogEvents(); @@ -939,8 +931,23 @@ } prepareGuestAgentNetworkDevicesForUpdate(); + } + } - } else { + /** + * fetch running VMs and populate the internal structure. if we fail, handle the error + * @return true if we could get vms otherwise false + */ + protected boolean fetchRunningVms() { + VDSCommandType commandType = + _vdsManager.getRefreshStatistics() + ? VDSCommandType.GetAllVmStats + : VDSCommandType.List; + VDSReturnValue vdsReturnValue = + getResourceManager().runVdsCommand(commandType, new VdsIdAndVdsVDSCommandParametersBase(_vds)); + _runningVms = (Map<Guid, VmInternalData>) vdsReturnValue.getReturnValue(); + + if (!vdsReturnValue.getSucceeded()) { RuntimeException callException = vdsReturnValue.getExceptionObject(); if (callException != null) { if (callException instanceof VDSErrorException) { @@ -957,6 +964,8 @@ log.errorFormat("{0} failed with no exception!", commandType.name()); } } + + return vdsReturnValue.getSucceeded(); } /** @@ -1216,8 +1225,8 @@ return (String) device.get(VdsProperties.Device); } - // if not statistics check if status changed and add to running list - private List<Guid> checkVmsStatusChanged() { + // if not statistics check if status changed return a list of those + protected List<Guid> checkVmsStatusChanged() { List<Guid> staleRunningVms = new ArrayList<>(); if (!_vdsManager.getRefreshStatistics()) { List<VmDynamic> tempRunningList = new ArrayList<VmDynamic>(); @@ -1227,21 +1236,31 @@ for (VmDynamic runningVm : tempRunningList) { VM vmToUpdate = _vmDict.get(runningVm.getId()); + boolean statusChanged = false; if (vmToUpdate == null || (vmToUpdate.getStatus() != runningVm.getStatus() && - !(vmToUpdate.getStatus() == VMStatus.PreparingForHibernate && runningVm.getStatus() == VMStatus.Up))) { - VDSReturnValue vdsReturnValue = getResourceManager().runVdsCommand( - VDSCommandType.GetVmStats, - new GetVmStatsVDSCommandParameters(_vds, runningVm.getId())); - if (vdsReturnValue.getSucceeded()) { - _runningVms.put(runningVm.getId(), (VmInternalData) vdsReturnValue.getReturnValue()); + !(vmToUpdate.getStatus() == VMStatus.PreparingForHibernate && runningVm.getStatus() == VMStatus.Up)) ) { + VDSReturnValue vmStats = + getResourceManager().runVdsCommand( + VDSCommandType.GetVmStats, + new GetVmStatsVDSCommandParameters(_vds, runningVm.getId())); + if (vmStats.getSucceeded()) { + _runningVms.put(runningVm.getId(), (VmInternalData) vmStats.getReturnValue()); + statusChanged = true; } else { - _runningVms.remove(runningVm.getId()); + if (vmToUpdate != null) { + log.errorFormat( + "failed to fetch {0} stats. status remain unchanged ({1})", + vmToUpdate.getName(), + vmToUpdate.getStatus()); + } } - } else { + } + + if (!statusChanged) { // status not changed move to next vm - staleRunningVms.add(vmToUpdate.getId()); - _runningVms.remove(vmToUpdate.getId()); + staleRunningVms.add(runningVm.getId()); + _runningVms.remove(runningVm.getId()); } } } @@ -1534,7 +1553,7 @@ } } - private void processExternallyManagedVms() { + protected void processExternallyManagedVms() { List<String> vmsToQuery = new ArrayList<String>(); // Searching for External VMs that run on the host for (VmInternalData vmInternalData : _runningVms.values()) { @@ -1626,7 +1645,7 @@ if (vmToUpdate.getStatus() != VMStatus.Up && vmToUpdate.getStatus() != VMStatus.MigratingFrom && runningVm.getStatus() == VMStatus.Up) { // Vm moved to Up status - remove its record from Async - // running handling + // reportedAndUnchangedVms handling if (log.isDebugEnabled()) { log.debugFormat("removing VM {0} from successful run VMs list", vmToUpdate.getId()); } @@ -2079,6 +2098,10 @@ return Collections.unmodifiableList(removedDeviceIds); } + protected Map<Guid, VmInternalData> getRunningVms() { + return _runningVms; + } + protected void auditLog(AuditLogableBase auditLogable, AuditLogType logType) { AuditLogDirector.log(auditLogable, logType); } diff --git a/backend/manager/modules/vdsbroker/src/test/java/org/ovirt/engine/core/vdsbroker/VdsUpdateRunTimeInfoTest.java b/backend/manager/modules/vdsbroker/src/test/java/org/ovirt/engine/core/vdsbroker/VdsUpdateRunTimeInfoTest.java index 21555c1..89d4d33 100644 --- a/backend/manager/modules/vdsbroker/src/test/java/org/ovirt/engine/core/vdsbroker/VdsUpdateRunTimeInfoTest.java +++ b/backend/manager/modules/vdsbroker/src/test/java/org/ovirt/engine/core/vdsbroker/VdsUpdateRunTimeInfoTest.java @@ -12,38 +12,66 @@ import java.util.List; import java.util.Map; +import junit.framework.Assert; + import org.junit.Before; import org.junit.ClassRule; import org.junit.Test; import org.junit.runner.RunWith; import org.mockito.Mock; +import org.mockito.Mockito; import org.mockito.runners.MockitoJUnitRunner; import org.ovirt.engine.core.common.AuditLogType; import org.ovirt.engine.core.common.businessentities.AuditLog; import org.ovirt.engine.core.common.businessentities.VDS; import org.ovirt.engine.core.common.businessentities.VDSGroup; import org.ovirt.engine.core.common.businessentities.VM; +import org.ovirt.engine.core.common.businessentities.VMStatus; import org.ovirt.engine.core.common.businessentities.VmDevice; -import org.ovirt.engine.core.common.businessentities.VmDynamic; import org.ovirt.engine.core.common.businessentities.VmDeviceGeneralType; +import org.ovirt.engine.core.common.businessentities.VmDynamic; +import org.ovirt.engine.core.common.config.ConfigValues; +import org.ovirt.engine.core.common.vdscommands.VDSCommandType; +import org.ovirt.engine.core.common.vdscommands.VDSParametersBase; +import org.ovirt.engine.core.common.vdscommands.VDSReturnValue; import org.ovirt.engine.core.compat.Guid; +import org.ovirt.engine.core.compat.Version; import org.ovirt.engine.core.dal.dbbroker.DbFacade; import org.ovirt.engine.core.dal.dbbroker.auditloghandling.AuditLogableBase; import org.ovirt.engine.core.dao.AuditLogDAO; import org.ovirt.engine.core.dao.VdsGroupDAO; import org.ovirt.engine.core.dao.VmDAO; import org.ovirt.engine.core.dao.VmDeviceDAO; +import org.ovirt.engine.core.dao.VmDynamicDAO; +import org.ovirt.engine.core.utils.MockConfigRule; import org.ovirt.engine.core.utils.MockEJBStrategyRule; import org.ovirt.engine.core.vdsbroker.vdsbroker.VdsProperties; +import org.ovirt.engine.core.vdsbroker.vdsbroker.entities.VmInternalData; @RunWith(MockitoJUnitRunner.class) public class VdsUpdateRunTimeInfoTest { + private static final Guid VM_1 = Guid.createGuidFromString("7eeabc50-325f-49bb-acb6-15e786599423"); + @ClassRule public static MockEJBStrategyRule mockEjbRule = new MockEJBStrategyRule(); + @ClassRule + public static MockConfigRule mcr = new MockConfigRule( + MockConfigRule.mockConfig( + ConfigValues.DebugTimerLogging, + true), + MockConfigRule.mockConfig( + ConfigValues.VdsRefreshRate, + 3), + MockConfigRule.mockConfig( + ConfigValues.TimeToReduceFailedRunOnVdsInMinutes, + 3) + ); + private VDS vds; HashMap[] vmInfo; + List<VmDynamic> poweringUpVms; VdsUpdateRunTimeInfo updater; @@ -62,13 +90,28 @@ @Mock VmDeviceDAO vmDeviceDAO; + @Mock + VmDynamicDAO vmDynamicDao; + AuditLogDAO mockAuditLogDao = new AuditLogDaoMocker(); + + VM vm_1_db; + VM vm_1_vdsm; + + @Mock + ResourceManager resourceManager; + + @Mock + private VdsManager vdsManager; + @Before public void setup() { initVds(); initConditions(); - updater = new VdsUpdateRunTimeInfo(null, vds, mock(MonitoringStrategy.class)) { + when(vdsManager.getRefreshStatistics()).thenReturn(false); + updater = Mockito.spy( + new VdsUpdateRunTimeInfo(vdsManager, vds, mock(MonitoringStrategy.class)) { @Override public DbFacade getDbFacade() { @@ -87,7 +130,7 @@ return vmInfo; } - }; + }); } @Test @@ -127,14 +170,17 @@ when(dbFacade.getVmDao()).thenReturn(vmDAO); when(dbFacade.getAuditLogDao()).thenReturn(mockAuditLogDao); when(dbFacade.getVmDeviceDao()).thenReturn(vmDeviceDAO); + when(dbFacade.getVmDynamicDao()).thenReturn(vmDynamicDao); when(groupDAO.get((Guid) any())).thenReturn(cluster); Map<Guid, VM> emptyMap = Collections.emptyMap(); - when(vmDAO.getAllRunningByVds(vds.getId())).thenReturn(emptyMap); + initVm(); + when(vmDAO.getAllRunningByVds(vds.getId())).thenReturn(Collections.singletonMap(VM_1, vm_1_db)); } private void initVds() { vds = new VDS(); vds.setId(new Guid("00000000-0000-0000-0000-000000000012")); + vds.setVdsGroupCompatibilityVersion(Version.v3_3); } @Test @@ -152,4 +198,81 @@ dynamic.setLastWatchdogEvent(null); assertFalse(VdsUpdateRunTimeInfo.isNewWatchdogEvent(dynamic, vm)); } + + /** + * Test that when we succeed in retriving a VM stats, we insert it into the internal runningVms structure + */ + @Test + public void verifyListOfRunningVmsIsSameWithSuccessFromVdsmResponse() { + prepareForRefreshVmStatsCall(); + mockGetVmStatsCommand(true); + // start refreshing vm data... VURTI now fetches Vms list from ResourceManager and loop through it + updater.fetchRunningVms(); + List<Guid> staleRunningVms = updater.checkVmsStatusChanged(); + + Assert.assertTrue(updater.getRunningVms().containsKey(VM_1)); + Assert.assertFalse(staleRunningVms.contains(VM_1)); + } + + /** + * Test that when we fail in getting a response for GetVmStats we still insert the VM to the runningVms structure,<br> + * but not the internal runningVmStructure which is the same handling as if vm status didn't change + */ + @Test + public void verifyListOfRunningVmsIsSameWithFailureOnGetVmStats() { + prepareForRefreshVmStatsCall(); + mockGetVmStatsCommand(false); + // start refreshing vm data... VURTI now fetches Vms list from ResourceManager and loop through it + updater.fetchRunningVms(); + List<Guid> staleRunningVms= updater.checkVmsStatusChanged(); + + Assert.assertFalse(updater.getRunningVms().containsKey(VM_1)); + Assert.assertTrue(staleRunningVms.contains(VM_1)); + } + + private void prepareForRefreshVmStatsCall() { + initVm(); + mockUpdater(); + mockListCommand(); + } + + private void mockUpdater() { + when(updater.getResourceManager()).thenReturn(resourceManager); + } + + private void initVm() { + vm_1_vdsm = new VM(); + vm_1_db = new VM(); + vm_1_db.setId(VM_1); + vm_1_vdsm.setId(VM_1); + vm_1_db.setStatus(VMStatus.WaitForLaunch); + vm_1_vdsm.setStatus(VMStatus.Up); + vm_1_db.setName("vm-prod-mailserver"); + vm_1_vdsm.setName("vm-prod-mailserver"); + when(vmDynamicDao.get(VM_1)).thenReturn(vm_1_db.getDynamicData()); + } + + private void mockGetVmStatsCommand(boolean mockSuccess) { + VDSReturnValue vdsReturnValue = new VDSReturnValue(); + if (mockSuccess) { + vdsReturnValue.setSucceeded(true); + vdsReturnValue.setReturnValue(createRunningVms().get(VM_1)); + } + when(resourceManager.runVdsCommand(Mockito.eq(VDSCommandType.GetVmStats), (VDSParametersBase) Mockito.anyObject())).thenReturn(vdsReturnValue); + } + + private void mockListCommand() { + VDSReturnValue vdsReturnValue = new VDSReturnValue(); + vdsReturnValue.setSucceeded(true); + vdsReturnValue.setReturnValue(createRunningVms()); + when(resourceManager.runVdsCommand(Mockito.eq(VDSCommandType.List), (VDSParametersBase) Mockito.anyObject())).thenReturn(vdsReturnValue); + when(resourceManager.runVdsCommand(Mockito.eq(VDSCommandType.GetVmStats), (VDSParametersBase) Mockito.anyObject())).thenReturn(vdsReturnValue); + when(updater.getResourceManager()).thenReturn(resourceManager); + } + + private Map<Guid, VmInternalData> createRunningVms() { + HashMap<Guid, VmInternalData> vms = new HashMap<>(); + vms.put(VM_1, new VmInternalData(vm_1_vdsm.getDynamicData(), null, null)); + return vms; + } } -- To view, visit http://gerrit.ovirt.org/25636 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I415804d9fa1f1288423241b8547f4cab5540914a Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: ovirt-engine-3.3 Gerrit-Owner: Roy Golan <[email protected]> _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
