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 move the VM to UKNOWN so it would be ignored in this cycle. 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, 136 insertions(+), 18 deletions(-) git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/48/25548/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 f695f60..0169187 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 @@ -53,7 +53,6 @@ Collections.newSetFromMap(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 109c2f8..b7cddbb 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 6dda8bf..5da02c1 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 @@ -934,7 +934,7 @@ } } - private void refreshVmStats() { + protected void refreshVmStats() { if (Config.<Boolean> getValue(ConfigValues.DebugTimerLogging)) { log.debug("vds::refreshVmList entered"); } @@ -1295,7 +1295,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>(); @@ -1618,7 +1618,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..cb09d62 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 @@ -4,6 +4,7 @@ import static junit.framework.Assert.assertFalse; import static junit.framework.Assert.assertTrue; import static org.mockito.Matchers.any; +import static org.mockito.Mockito.doNothing; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -12,11 +13,13 @@ 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.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,87 @@ 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.refreshVmStats(); + Assert.assertTrue("expecting the VM in running VMs map", updater.getRunningVms().containsKey(VM_1)); + Assert.assertTrue( + String.format("expecting the VM to be in status %s and got %s", VMStatus.Up, vm_1_db.getStatus()), + updater.getRunningVms().get(VM_1).getVmDynamic().getStatus() == VMStatus.Up); + } + + /** + * Test that when we fail in getting a response for GetVmStats we still insert the VM to the runningVms structure,<br> + * but with status UNKNOWN + */ + @Test + public void verifyListOfRunningVmsIsSameWithFailureOnGetVmStats() { + prepareForRefreshVmStatsCall(); + mockGetVmStatsCommand(false); + // start refreshing vm data... VURTI now fetches Vms list from ResourceManager and loop through it + updater.refreshVmStats(); + Assert.assertTrue("expecting the VM in running VMs map", updater.getRunningVms().containsKey(VM_1)); + Assert.assertTrue( + String.format("expecting the VM to be in status %s and got %s", VMStatus.Unknown, vm_1_db.getStatus()), + updater.getRunningVms().get(VM_1).getVmDynamic().getStatus() == VMStatus.Unknown); + } + + private void prepareForRefreshVmStatsCall() { + initVm(); + mockUpdater(); + mockListCommand(); + muteNonRelatedMethods(); + } + + 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 void muteNonRelatedMethods() { + doNothing().when(updater).processExternallyManagedVms(); + when(updater.getPoweringUpVms()).thenReturn(Collections.<VmDynamic>emptyList()); + } + + 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/25548 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I415804d9fa1f1288423241b8547f4cab5540914a Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Roy Golan <rgo...@redhat.com> _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
