Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
24 changes: 24 additions & 0 deletions server/src/com/cloud/user/AccountManagerImpl.java
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@
import com.cloud.event.ActionEventUtils;
import com.cloud.event.ActionEvents;
import com.cloud.event.EventTypes;
import com.cloud.event.UsageEventUtils;
import com.cloud.exception.AgentUnavailableException;
import com.cloud.exception.CloudAuthenticationException;
import com.cloud.exception.ConcurrentOperationException;
Expand Down Expand Up @@ -159,6 +160,7 @@
import com.cloud.vm.UserVmManager;
import com.cloud.vm.UserVmVO;
import com.cloud.vm.VMInstanceVO;
import com.cloud.vm.VirtualMachine;
import com.cloud.vm.VirtualMachineManager;
import com.cloud.vm.dao.InstanceGroupDao;
import com.cloud.vm.dao.UserVmDao;
Expand Down Expand Up @@ -677,6 +679,17 @@ public boolean deleteAccount(AccountVO account, long callerUserId, Account calle
return cleanupAccount(account, callerUserId, caller);
}

protected List<VolumeVO> getExpungedInstanceRootVolume(long instanceId) {
SearchBuilder<VolumeVO> sb = _volumeDao.createSearchBuilder();
sb.and("instanceId", sb.entity().getInstanceId(), SearchCriteria.Op.EQ);
sb.and("vType", sb.entity().getVolumeType(), SearchCriteria.Op.EQ);
sb.done();
SearchCriteria<VolumeVO> c = sb.create();
c.setParameters("instanceId", instanceId);
c.setParameters("vType", Volume.Type.ROOT);
return _volumeDao.customSearchIncludingRemoved(c, null);
}

protected boolean cleanupAccount(AccountVO account, long callerUserId, Account caller) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is missing a simple unit tests as a test if the method expungedInstaceRootVolume is called when the cleanupAccount is invoked and there is a vm in destroyed state. That and the guarantee that the method getExpungedInstanceRootVolume works, added to the tests you made would complete it.

long accountId = account.getId();
boolean accountCleanupNeeded = false;
Expand Down Expand Up @@ -761,6 +774,17 @@ protected boolean cleanupAccount(AccountVO account, long callerUserId, Account c
s_logger.error("Unable to expunge vm: " + vm.getId());
accountCleanupNeeded = true;
}
else if (!vm.getState().equals(VirtualMachine.State.Destroyed)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

In what case would the vm's state be destroyed, when the vm was expunged above?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, do you know if the expunge thread propagates the usage event correctly?

Copy link
Contributor

Choose a reason for hiding this comment

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

The check on line 777 makes sure that the volume wasn't already destroyed before expunging. In that case, the delete event has already been emitted somewhere else.

The expunge thread does emit other usage events correctly (we tested deleting an account having active VMs, datadisks, allocated IP's, templates, and snapshots, and upon deleting the account, "delete" usage events were emitted for each of them). The emission of the ROOT volume's "delete" event is handled on a more case-by-case basis (In UserVmManager for regular VM destroys, in VolumeOrchestrator for failed vm creation, etc), and the case where the root volume gets deleted as part of account deletion was not being handled.

// We have to emit the event here because the state listener is ignoring root volume deletions,
// assuming that the UserVMManager is responsible for emitting the usage event for them when
// the vm delete command is processed
List<VolumeVO> volumes = getExpungedInstanceRootVolume(vm.getId());
for (VolumeVO volume : volumes) {
UsageEventUtils.publishUsageEvent(EventTypes.EVENT_VOLUME_DELETE, volume.getAccountId(), volume.getDataCenterId(), volume.getId(), volume.getName(),
Volume.class.getName(), volume.getUuid(), volume.isDisplayVolume());
}

}
}

// Mark the account's volumes as destroyed
Expand Down
154 changes: 153 additions & 1 deletion server/test/com/cloud/user/AccountManagerImplTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,21 @@
package com.cloud.user;

import java.lang.reflect.Field;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
import java.net.InetAddress;
import java.net.UnknownHostException;
import java.util.Arrays;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.HashMap;
import java.util.List;
import java.util.Map;

import javax.inject.Inject;

import com.cloud.server.auth.UserAuthenticator;
import com.cloud.utils.Pair;

import org.junit.After;
import org.junit.Assert;
import org.junit.Before;
Expand All @@ -35,6 +41,10 @@
import org.mockito.Mockito;
import org.mockito.runners.MockitoJUnitRunner;

import com.cloud.utils.db.Filter;
import com.cloud.utils.db.SearchBuilder;
import com.cloud.utils.db.SearchCriteria;

import org.apache.cloudstack.acl.ControlledEntity;
import org.apache.cloudstack.acl.SecurityChecker;
import org.apache.cloudstack.acl.SecurityChecker.AccessType;
Expand All @@ -58,6 +68,9 @@
import com.cloud.domain.Domain;
import com.cloud.domain.DomainVO;
import com.cloud.domain.dao.DomainDao;
import com.cloud.event.UsageEventUtils;
import com.cloud.event.UsageEventVO;
import com.cloud.event.dao.UsageEventDao;
import com.cloud.exception.ConcurrentOperationException;
import com.cloud.exception.ResourceUnavailableException;
import com.cloud.network.as.AutoScaleManager;
Expand All @@ -78,6 +91,7 @@
import com.cloud.storage.dao.SnapshotDao;
import com.cloud.storage.dao.VMTemplateDao;
import com.cloud.storage.dao.VolumeDao;
import com.cloud.storage.VolumeVO;
import com.cloud.storage.snapshot.SnapshotManager;
import com.cloud.template.TemplateManager;
import com.cloud.user.Account.State;
Expand All @@ -87,6 +101,7 @@
import com.cloud.vm.UserVmManager;
import com.cloud.vm.UserVmVO;
import com.cloud.vm.VMInstanceVO;
import com.cloud.vm.VirtualMachine;
import com.cloud.vm.VirtualMachineManager;
import com.cloud.vm.dao.DomainRouterDao;
import com.cloud.vm.dao.InstanceGroupDao;
Expand Down Expand Up @@ -192,6 +207,8 @@ public class AccountManagerImplTest {
@Mock
VMSnapshotDao _vmSnapshotDao;

UsageEventDao _usageEventDao = new MockUsageEventDao();

@Mock
User callingUser;
@Mock
Expand All @@ -205,6 +222,11 @@ public class AccountManagerImplTest {
@Mock
private UserAuthenticator userAuthenticator;

//Maintain a list of old fields in the usage utils class... This
//is because of weirdness of how it uses static fields and an init
//method.
private Map<String, Object> oldFields = new HashMap<>();

@Before
public void setup() throws NoSuchFieldException, SecurityException,
IllegalArgumentException, IllegalAccessException {
Expand All @@ -231,6 +253,73 @@ public void cleanup() {
CallContext.unregister();
}

public UsageEventUtils setupUsageUtils() {
_usageEventDao = new MockUsageEventDao();
UsageEventUtils utils = new UsageEventUtils();

Map<String, String> usageUtilsFields = new HashMap<String, String>();
usageUtilsFields.put("usageEventDao", "_usageEventDao");
usageUtilsFields.put("accountDao", "_accountDao");
usageUtilsFields.put("dcDao", "_dcDao");
usageUtilsFields.put("configDao", "_configDao");

for (String fieldName : usageUtilsFields.keySet()) {
try {
Field f = UsageEventUtils.class.getDeclaredField(fieldName);
f.setAccessible(true);
//Remember the old fields for cleanup later (see cleanupUsageUtils)
Field staticField = UsageEventUtils.class.getDeclaredField("s_" + fieldName);
staticField.setAccessible(true);
oldFields.put(f.getName(), staticField.get(null));
f.set(utils, this.getClass().getDeclaredField(usageUtilsFields.get(fieldName)).get(this));
} catch (IllegalArgumentException | IllegalAccessException
| NoSuchFieldException | SecurityException e) {
// TODO Auto-generated catch block
e.printStackTrace();
}
}

Method method;
try {
method = UsageEventUtils.class.getDeclaredMethod("init");
method.setAccessible(true);
method.invoke(utils);
} catch (NoSuchMethodException | SecurityException | IllegalAccessException |
IllegalArgumentException | InvocationTargetException e) {
// TODO Auto-generated catch block
e.printStackTrace();
}

return utils;
}

public void cleanupUsageUtils() {
UsageEventUtils utils = new UsageEventUtils();

for (String fieldName : oldFields.keySet()) {
try {
Field f = UsageEventUtils.class.getDeclaredField(fieldName);
f.setAccessible(true);
f.set(utils, oldFields.get(fieldName));
} catch (IllegalArgumentException | IllegalAccessException
| NoSuchFieldException | SecurityException e) {
// TODO Auto-generated catch block
e.printStackTrace();
}

}
try {
Method method = UsageEventUtils.class.getDeclaredMethod("init");
method.setAccessible(true);
method.invoke(utils);
} catch (SecurityException | NoSuchMethodException
| IllegalAccessException | IllegalArgumentException
| InvocationTargetException e) {
// TODO Auto-generated catch block
e.printStackTrace();
}
}

@Test
public void disableAccountNotexisting()
throws ConcurrentOperationException, ResourceUnavailableException {
Expand Down Expand Up @@ -353,6 +442,69 @@ public void testAuthenticateUser() throws UnknownHostException {
Mockito.verify(userAuthenticator, Mockito.times(1)).authenticate("test", "fail", 1L, null);
Mockito.verify(userAuthenticator, Mockito.never()).authenticate("test", null, 1L, null);
Mockito.verify(userAuthenticator, Mockito.never()).authenticate("test", "", 1L, null);
}

@SuppressWarnings("unchecked")
public List<UsageEventVO> deleteUserAccountRootVolumeUsageEvents(boolean vmDestroyedPrior) {
AccountVO account = new AccountVO();
account.setId(42l);
DomainVO domain = new DomainVO();
UserVmVO vm = Mockito.mock(UserVmVO.class);
VolumeVO vol = Mockito.mock(VolumeVO.class);
Mockito.when(_accountDao.findById(42l)).thenReturn(account);
Mockito.when(
securityChecker.checkAccess(Mockito.any(Account.class),
Mockito.any(ControlledEntity.class), Mockito.any(AccessType.class),
Mockito.anyString()))
.thenReturn(true);
Mockito.when(_accountDao.remove(42l)).thenReturn(true);
Mockito.when(_userVmDao.listByAccountId(42l)).thenReturn(
Arrays.asList(vm));
Mockito.when(_userVmDao.findByUuid(Mockito.any(String.class))).thenReturn(vm);
Mockito.when(
_vmMgr.expunge(Mockito.any(UserVmVO.class), Mockito.anyLong(),
Mockito.any(Account.class))).thenReturn(true);
Mockito.when(vm.getState()).thenReturn(
vmDestroyedPrior
? VirtualMachine.State.Destroyed
: VirtualMachine.State.Running);

SearchBuilder<VolumeVO> sb = (SearchBuilder<VolumeVO>)Mockito.mock(SearchBuilder.class);

Mockito.when(_volumeDao.createSearchBuilder()).thenReturn(sb);
Mockito.when(sb.create()).thenReturn((SearchCriteria<VolumeVO>) Mockito.mock(SearchCriteria.class));
Mockito.when(sb.entity()).thenReturn(Mockito.mock(VolumeVO.class));

Mockito.when(
_volumeDao.customSearchIncludingRemoved(Mockito.any(SearchCriteria.class), Mockito.isNull(Filter.class))).thenReturn(
Arrays.asList(vol));
Mockito.when(vol.getAccountId()).thenReturn((long) 1);
Mockito.when(vol.getDataCenterId()).thenReturn((long) 1);
Mockito.when(vol.getId()).thenReturn((long) 1);
Mockito.when(vol.getName()).thenReturn("root volume");
Mockito.when(vol.getUuid()).thenReturn("vol-111111");
Mockito.when(vol.isDisplayVolume()).thenReturn(true);

Mockito.when(_domainMgr.getDomain(Mockito.anyLong()))
.thenReturn(domain);

accountManager.deleteUserAccount(42);
return _usageEventDao.listAll();
}

@Test
public void destroyedVMRootVolumeUsageEvent() {
setupUsageUtils();
List<UsageEventVO> emittedEvents = deleteUserAccountRootVolumeUsageEvents(true);
Assert.assertEquals(0, emittedEvents.size());
cleanupUsageUtils();
}

@Test
public void runningVMRootVolumeUsageEvent() {
setupUsageUtils();
List<UsageEventVO> emittedEvents = deleteUserAccountRootVolumeUsageEvents(false);
Assert.assertEquals(1, emittedEvents.size());
cleanupUsageUtils();
}
}
Loading