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 <rgo...@redhat.com> --- M backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/ResourceManager.java 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 4 files changed, 164 insertions(+), 36 deletions(-) git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/99/25599/1 diff --git a/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/ResourceManager.java b/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/ResourceManager.java index 802f99a..a925334 100644 --- a/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/ResourceManager.java +++ b/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/ResourceManager.java @@ -51,7 +51,6 @@ new ConcurrentHashMap<Guid, Boolean>(); private static final String VDSCommandPrefix = "VDSCommand"; - private static Log log = LogFactory.getLog(ResourceManager.class); private ResourceManager() { 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 bbb2639..7b0b7f4 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 b1be8a9..ebe742a 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 @@ -902,20 +902,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<VM> running = checkVmsStatusChanged(); proceedWatchdogEvents(); @@ -945,8 +937,23 @@ prepareGuestAgentNetworkDevicesForUpdate(); updateLunDisks(); + } + } - } 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) { @@ -963,6 +970,8 @@ log.errorFormat("{0} failed with no exception!", commandType.name()); } } + + return vdsReturnValue.getSucceeded(); } /** @@ -1257,7 +1266,7 @@ } // if not statistics check if status changed and add to running list - private List<VM> checkVmsStatusChanged() { + protected List<VM> checkVmsStatusChanged() { List<VM> running = new ArrayList<VM>(); if (!_vdsManager.getRefreshStatistics()) { List<VmDynamic> tempRunningList = new ArrayList<VmDynamic>(); @@ -1267,18 +1276,25 @@ for (VmDynamic runningVm : tempRunningList) { VM vmToUpdate = _vmDict.get(runningVm.getId()); + boolean statusNotChanged = 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()); } else { - _runningVms.remove(runningVm.getId()); + log.error("failed to fetch vm stats. status remain unchanged"); + statusNotChanged = true; } } else { + statusNotChanged = true; + } + + if (statusNotChanged) { // status not changed move to next vm running.add(vmToUpdate); _runningVms.remove(vmToUpdate.getId()); @@ -1573,7 +1589,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()) { 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 a4b0f4e..c97677a 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,11 +12,14 @@ 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; @@ -26,10 +29,16 @@ 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; @@ -37,6 +46,8 @@ 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; @@ -44,13 +55,27 @@ @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; - Map<Guid, VmInternalData> runningVms; VdsUpdateRunTimeInfo updater; @@ -72,13 +97,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() { @@ -101,13 +141,7 @@ protected List<VmDynamic> getPoweringUpVms() { return poweringUpVms; } - - @Override - protected Map<Guid, VmInternalData> getRunningVms() { - return runningVms; - } - - }; + }); } @Test @@ -183,9 +217,8 @@ VmInternalData vmInternalData = new VmInternalData(vmDynamic, null, null, lunsMapFromVmStats); when(diskDAO.getAllForVm(any(Guid.class), any(Boolean.class))).thenReturn(vmLunDisksFromDb); - + when(updater.getRunningVms()).thenReturn(Collections.singletonMap(vmId, vmInternalData)); poweringUpVms = Collections.singletonList(vmDynamic); - runningVms = Collections.singletonMap(vmId, vmInternalData); updater.updateLunDisks(); } @@ -195,15 +228,18 @@ when(dbFacade.getVmDao()).thenReturn(vmDAO); when(dbFacade.getAuditLogDao()).thenReturn(mockAuditLogDao); when(dbFacade.getVmDeviceDao()).thenReturn(vmDeviceDAO); + when(dbFacade.getVmDynamicDao()).thenReturn(vmDynamicDao); when(dbFacade.getDiskDao()).thenReturn(diskDAO); 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_4); } @Test @@ -221,4 +257,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<VM> runningAndUnchanged = updater.checkVmsStatusChanged(); + + Assert.assertTrue(updater.getRunningVms().containsKey(VM_1)); + Assert.assertFalse(runningAndUnchanged.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<VM> runningAndUnchanged = updater.checkVmsStatusChanged(); + + Assert.assertFalse(updater.getRunningVms().containsKey(VM_1)); + Assert.assertTrue(runningAndUnchanged.contains(vm_1_db)); + } + + 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, null)); + return vms; + } } -- To view, visit http://gerrit.ovirt.org/25599 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.4 Gerrit-Owner: Roy Golan <rgo...@redhat.com> _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
