Skip to content

Fixup vm powerstate update #8545

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
DaanHoogland merged 7 commits into from
Feb 19, 2024
Merged

Fixup vm powerstate update #8545

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c9ef2a5
Update VM's state if powerstate & state are not in sync
vishesh92 Jan 22, 2024
2c383a6
Add unit tests
vishesh92 Jan 22, 2024
3437f5c
some code improvements for instacne / power state check
sureshanaparti Feb 1, 2024
17ea9e7
Update power state after vm stop confirmation (as power state 'PowerO…
sureshanaparti Feb 1, 2024
b3580ba
Reset the power state update counter before migrate, to allow power s…
sureshanaparti Feb 1, 2024
007b0a8
Do not consider transitional states (Starting, Stopping) to check pow…
sureshanaparti Feb 2, 2024
cc8bd7b
set powerstate to off for all vm types
vishesh92 Feb 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,8 +213,8 @@
import com.cloud.service.dao.ServiceOfferingDao;
import com.cloud.storage.DiskOfferingVO;
import com.cloud.storage.ScopeType;
import com.cloud.storage.Storage.ImageFormat;
import com.cloud.storage.Storage;
import com.cloud.storage.Storage.ImageFormat;
import com.cloud.storage.StorageManager;
import com.cloud.storage.StoragePool;
import com.cloud.storage.VMTemplateVO;
Expand Down Expand Up @@ -2208,9 +2208,11 @@ private void advanceStop(final VMInstanceVO vm, final boolean cleanUpEvenIfUnabl

boolean result = stateTransitTo(vm, Event.OperationSucceeded, null);
if (result) {
vm.setPowerState(PowerState.PowerOff);
_vmDao.update(vm.getId(), vm);
if (VirtualMachine.Type.User.equals(vm.type) && ResourceCountRunningVMsonly.value()) {
ServiceOfferingVO offering = _offeringDao.findById(vm.getId(), vm.getServiceOfferingId());
resourceCountDecrement(vm.getAccountId(),new Long(offering.getCpu()), new Long(offering.getRamSize()));
resourceCountDecrement(vm.getAccountId(), offering.getCpu().longValue(), offering.getRamSize().longValue());
Copy link
Contributor

@DaanHoogland DaanHoogland Feb 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in the database these fields are nullable. Are we sure these are not potential NPE´s?

Copy link
Member

@weizhouapache weizhouapache Feb 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code below gets cpu/ram from user_vm_details if it is null in service offering.

ServiceOfferingVO offering = _offeringDao.findById(vm.getId(), vm.getServiceOfferingId());

Copy link
Member Author

@vishesh92 vishesh92 Feb 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As Wei said, it's handled in OfferingDao's findById.

cloudstack/engine/schema/src/main/java/com/cloud/service/dao/ServiceOfferingDaoImpl.java

Lines 171 to 182 in a31449b

public ServiceOfferingVO findById(Long vmId, long serviceOfferingId) {
ServiceOfferingVO offering = super.findById(serviceOfferingId);
if (offering.isDynamic()) {
offering.setDynamicFlag(true);
if (vmId == null) {
throw new CloudRuntimeException("missing argument vmId");
}
Map<String, String> dynamicOffering = userVmDetailsDao.listDetailsKeyPairs(vmId);
return getComputeOffering(offering, dynamicOffering);
}
return offering;
}

cloudstack/engine/schema/src/main/java/com/cloud/service/dao/ServiceOfferingDaoImpl.java

Lines 205 to 219 in a31449b

public ServiceOfferingVO getComputeOffering(ServiceOfferingVO serviceOffering, Map<String, String> customParameters) {
ServiceOfferingVO dummyoffering = new ServiceOfferingVO(serviceOffering);
dummyoffering.setDynamicFlag(true);
if (customParameters.containsKey(UsageEventVO.DynamicParameters.cpuNumber.name())) {
dummyoffering.setCpu(Integer.parseInt(customParameters.get(UsageEventVO.DynamicParameters.cpuNumber.name())));
}
if (customParameters.containsKey(UsageEventVO.DynamicParameters.cpuSpeed.name())) {
dummyoffering.setSpeed(Integer.parseInt(customParameters.get(UsageEventVO.DynamicParameters.cpuSpeed.name())));
}
if (customParameters.containsKey(UsageEventVO.DynamicParameters.memory.name())) {
dummyoffering.setRamSize(Integer.parseInt(customParameters.get(UsageEventVO.DynamicParameters.memory.name())));
}
return dummyoffering;
}

Copy link
Member Author

@vishesh92 vishesh92 Feb 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new Long(null) also throws a NPE. So, this is not really relevant to this PR but a minor code improvement.

Copy link
Member

@weizhouapache weizhouapache Feb 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, the method has been used in many places, it works well.
No need to add a null check I think.

anyway , good question @DaanHoogland

Copy link
Contributor

@DaanHoogland DaanHoogland Feb 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, guys, I agree i think, if we get an NPE a check would not help us take measure against it. decreasing by 0 (zero) does not make sense either.

Any improvement on this would be out of scope for this PR and I think it might not be high prio (just good to keep in mind)

Copy link
Member

@weizhouapache weizhouapache Feb 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah
A check can be added in the code Vishesh shared above.

For example

return validateOfferingParameters(offering)

}
} else {
throw new CloudRuntimeException("unable to stop " + vm);
Expand Down Expand Up @@ -2761,6 +2763,7 @@ protected void migrate(final VMInstanceVO vm, final long srcHostId, final Deploy
}

vm.setLastHostId(srcHostId);
_vmDao.resetVmPowerStateTracking(vm.getId());
try {
if (vm.getHostId() == null || vm.getHostId() != srcHostId || !changeState(vm, Event.MigrationRequested, dstHostId, work, Step.Migrating)) {
_networkMgr.rollbackNicForMigration(vmSrc, profile);
Expand Down
23 changes: 18 additions & 5 deletions engine/schema/src/main/java/com/cloud/vm/dao/VMInstanceDaoImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@
public class VMInstanceDaoImpl extends GenericDaoBase<VMInstanceVO, Long> implements VMInstanceDao {

public static final Logger s_logger = Logger.getLogger(VMInstanceDaoImpl.class);
private static final int MAX_CONSECUTIVE_SAME_STATE_UPDATE_COUNT = 3;
static final int MAX_CONSECUTIVE_SAME_STATE_UPDATE_COUNT = 3;

protected SearchBuilder<VMInstanceVO> VMClusterSearch;
protected SearchBuilder<VMInstanceVO> LHVMClusterSearch;
Expand Down Expand Up @@ -897,17 +897,19 @@ public List<VMInstanceVO> listStartingWithNoHostId() {

@Override
public boolean updatePowerState(final long instanceId, final long powerHostId, final VirtualMachine.PowerState powerState, Date wisdomEra) {
return Transaction.execute(new TransactionCallback<Boolean>() {
return Transaction.execute(new TransactionCallback<>() {
@Override
public Boolean doInTransaction(TransactionStatus status) {
boolean needToUpdate = false;
VMInstanceVO instance = findById(instanceId);
if (instance != null
&& (null == instance.getPowerStateUpdateTime()
&& (null == instance.getPowerStateUpdateTime()
|| instance.getPowerStateUpdateTime().before(wisdomEra))) {
Long savedPowerHostId = instance.getPowerHostId();
if (instance.getPowerState() != powerState || savedPowerHostId == null
|| savedPowerHostId.longValue() != powerHostId) {
if (instance.getPowerState() != powerState
|| savedPowerHostId == null
|| savedPowerHostId != powerHostId
|| !isPowerStateInSyncWithInstanceState(powerState, powerHostId, instance)) {
instance.setPowerState(powerState);
instance.setPowerHostId(powerHostId);
instance.setPowerStateUpdateCount(1);
Expand All @@ -929,6 +931,17 @@ public Boolean doInTransaction(TransactionStatus status) {
});
}

private boolean isPowerStateInSyncWithInstanceState(final VirtualMachine.PowerState powerState, final long powerHostId, final VMInstanceVO instance) {
State instanceState = instance.getState();
if ((powerState == VirtualMachine.PowerState.PowerOff && instanceState == State.Running)
|| (powerState == VirtualMachine.PowerState.PowerOn && instanceState == State.Stopped)) {
s_logger.debug(String.format("VM id: %d on host id: %d and power host id: %d is in %s state, but power state is %s",
instance.getId(), instance.getHostId(), powerHostId, instanceState, powerState));
return false;
}
return true;
}

@Override
public boolean isPowerStateUpToDate(final long instanceId) {
VMInstanceVO instance = findById(instanceId);
Expand Down
152 changes: 138 additions & 14 deletions engine/schema/src/test/java/com/cloud/vm/dao/VMInstanceDaoImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,22 +17,32 @@

package com.cloud.vm.dao;

import com.cloud.utils.Pair;
import com.cloud.vm.VirtualMachine;
import static com.cloud.vm.VirtualMachine.State.Running;
import static com.cloud.vm.VirtualMachine.State.Stopped;
import static com.cloud.vm.dao.VMInstanceDaoImpl.MAX_CONSECUTIVE_SAME_STATE_UPDATE_COUNT;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyLong;
import static org.mockito.Mockito.doReturn;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;

import java.util.Date;

import org.joda.time.DateTime;
import org.junit.Before;
import org.junit.Test;
import org.junit.Assert;
import org.mockito.Mock;

import static com.cloud.vm.VirtualMachine.State.Running;
import static com.cloud.vm.VirtualMachine.State.Stopped;

import static org.mockito.Mockito.when;
import com.cloud.vm.VMInstanceVO;
import org.mockito.MockitoAnnotations;
import org.mockito.Spy;

import com.cloud.utils.Pair;
import com.cloud.vm.VMInstanceVO;
import com.cloud.vm.VirtualMachine;

/**
* Created by sudharma_jain on 3/2/17.
*/
Expand All @@ -55,16 +65,130 @@ public void setUp() throws Exception {
}

@Test
public void testUpdateState() throws Exception {
public void testUpdateState() {
Long destHostId = null;
Pair<Long, Long> opaqueMock = new Pair<Long, Long>(new Long(1), destHostId);
Pair<Long, Long> opaqueMock = new Pair<>(1L, destHostId);
vmInstanceDao.updateState(Stopped, VirtualMachine.Event.FollowAgentPowerOffReport, Stopped, vm , opaqueMock);
}

@Test
public void testIfStateAndHostUnchanged() throws Exception {
Assert.assertEquals(vmInstanceDao.ifStateUnchanged(Stopped, Stopped, null, null), true);
Assert.assertEquals(vmInstanceDao.ifStateUnchanged(Stopped, Running, null, null), false);
public void testIfStateAndHostUnchanged() {
assertTrue(vmInstanceDao.ifStateUnchanged(Stopped, Stopped, null, null));
assertFalse(vmInstanceDao.ifStateUnchanged(Stopped, Running, null, null));
}

@Test
public void testUpdatePowerStateDifferentPowerState() {
when(vm.getPowerStateUpdateTime()).thenReturn(null);
when(vm.getPowerHostId()).thenReturn(1L);
when(vm.getPowerState()).thenReturn(VirtualMachine.PowerState.PowerOn);
doReturn(vm).when(vmInstanceDao).findById(anyLong());
doReturn(true).when(vmInstanceDao).update(anyLong(), any());

boolean result = vmInstanceDao.updatePowerState(1L, 1L, VirtualMachine.PowerState.PowerOff, new Date());

verify(vm, times(1)).setPowerState(VirtualMachine.PowerState.PowerOff);
verify(vm, times(1)).setPowerHostId(1L);
verify(vm, times(1)).setPowerStateUpdateCount(1);
verify(vm, times(1)).setPowerStateUpdateTime(any(Date.class));

assertTrue(result);
}

@Test
public void testUpdatePowerStateVmNotFound() {
when(vm.getPowerStateUpdateTime()).thenReturn(null);
when(vm.getPowerHostId()).thenReturn(1L);
when(vm.getPowerState()).thenReturn(VirtualMachine.PowerState.PowerOn);
doReturn(null).when(vmInstanceDao).findById(anyLong());

boolean result = vmInstanceDao.updatePowerState(1L, 1L, VirtualMachine.PowerState.PowerOff, new Date());

verify(vm, never()).setPowerState(any());
verify(vm, never()).setPowerHostId(anyLong());
verify(vm, never()).setPowerStateUpdateCount(any(Integer.class));
verify(vm, never()).setPowerStateUpdateTime(any(Date.class));

assertFalse(result);
}

@Test
public void testUpdatePowerStateNoChangeFirstUpdate() {
when(vm.getPowerStateUpdateTime()).thenReturn(null);
when(vm.getPowerHostId()).thenReturn(1L);
when(vm.getPowerState()).thenReturn(VirtualMachine.PowerState.PowerOn);
when(vm.getState()).thenReturn(Running);
when(vm.getPowerStateUpdateCount()).thenReturn(1);
doReturn(vm).when(vmInstanceDao).findById(anyLong());
doReturn(true).when(vmInstanceDao).update(anyLong(), any());

boolean result = vmInstanceDao.updatePowerState(1L, 1L, VirtualMachine.PowerState.PowerOn, new Date());

verify(vm, never()).setPowerState(any());
verify(vm, never()).setPowerHostId(anyLong());
verify(vm, times(1)).setPowerStateUpdateCount(2);
verify(vm, times(1)).setPowerStateUpdateTime(any(Date.class));

assertTrue(result);
}

@Test
public void testUpdatePowerStateNoChangeMaxUpdatesValidState() {
when(vm.getPowerStateUpdateTime()).thenReturn(null);
when(vm.getPowerHostId()).thenReturn(1L);
when(vm.getPowerState()).thenReturn(VirtualMachine.PowerState.PowerOn);
when(vm.getPowerStateUpdateCount()).thenReturn(MAX_CONSECUTIVE_SAME_STATE_UPDATE_COUNT);
when(vm.getState()).thenReturn(Running);
doReturn(vm).when(vmInstanceDao).findById(anyLong());
doReturn(true).when(vmInstanceDao).update(anyLong(), any());

boolean result = vmInstanceDao.updatePowerState(1L, 1L, VirtualMachine.PowerState.PowerOn, new Date());

verify(vm, never()).setPowerState(any());
verify(vm, never()).setPowerHostId(anyLong());
verify(vm, never()).setPowerStateUpdateCount(any(Integer.class));
verify(vm, never()).setPowerStateUpdateTime(any(Date.class));

assertFalse(result);
}

@Test
public void testUpdatePowerStateNoChangeMaxUpdatesInvalidStateVmStopped() {
when(vm.getPowerStateUpdateTime()).thenReturn(null);
when(vm.getPowerHostId()).thenReturn(1L);
when(vm.getPowerState()).thenReturn(VirtualMachine.PowerState.PowerOn);
when(vm.getPowerStateUpdateCount()).thenReturn(MAX_CONSECUTIVE_SAME_STATE_UPDATE_COUNT);
when(vm.getState()).thenReturn(Stopped);
doReturn(vm).when(vmInstanceDao).findById(anyLong());
doReturn(true).when(vmInstanceDao).update(anyLong(), any());

boolean result = vmInstanceDao.updatePowerState(1L, 1L, VirtualMachine.PowerState.PowerOn, new Date());

verify(vm, times(1)).setPowerState(any());
verify(vm, times(1)).setPowerHostId(anyLong());
verify(vm, times(1)).setPowerStateUpdateCount(1);
verify(vm, times(1)).setPowerStateUpdateTime(any(Date.class));

assertTrue(result);
}

@Test
public void testUpdatePowerStateNoChangeMaxUpdatesInvalidStateVmRunning() {
when(vm.getPowerStateUpdateTime()).thenReturn(null);
when(vm.getPowerHostId()).thenReturn(1L);
when(vm.getPowerState()).thenReturn(VirtualMachine.PowerState.PowerOff);
when(vm.getPowerStateUpdateCount()).thenReturn(MAX_CONSECUTIVE_SAME_STATE_UPDATE_COUNT);
when(vm.getState()).thenReturn(Running);
doReturn(vm).when(vmInstanceDao).findById(anyLong());
doReturn(true).when(vmInstanceDao).update(anyLong(), any());

boolean result = vmInstanceDao.updatePowerState(1L, 1L, VirtualMachine.PowerState.PowerOff, new Date());

verify(vm, times(1)).setPowerState(any());
verify(vm, times(1)).setPowerHostId(anyLong());
verify(vm, times(1)).setPowerStateUpdateCount(1);
verify(vm, times(1)).setPowerStateUpdateTime(any(Date.class));

assertTrue(result);
}
}