Merge pull request #1624 from greenqloud/pr-volume-usage-events-fixes-4.8

Fixes regarding VOLUME_DELETE events resulting from account deletionNew version of #1491.

**Original Description**
New version of #1373, but updated for the 4.7 branch with another fix that allows it to properly find expunged root volumes. This is a bug fix, which is why we target the 4.7 branch.

Original pull request:
Fixes regarding usage event emission.

UsageEventUtils was previously not checking deleted accounts, which meant that if an account was deleted that had some resources running on it, those resources would get destroyed without emitting any events.

Furthermore, the VOLUME_DELETE event of ROOT volumes is the responsibility of the UserVmManager, which gets circumvented when expunging resources following the account deletion. Added a check to the AccountManager which catches the ROOT volumes that need to be deleted and emits events for them.

To test this: Create a new user. As that user, create and destroy an instance. This should cause the VM_CREATE, VM_START, VM_STOP, VM_DESTROY, VOLUME_CREATE, and VOLUME_DELETE events to be emitted.
Create a new instance as the same user. Log in as admin, and delete the user. The same set of events should be emitted, and there should be no duplicate DELETE events for the ROOT volume of the previous instance.

* pr/1624:
  Emit a VOLUME_DELETE usage event when account deletion destroys an instance.

Signed-off-by: Rajani Karuturi <[email protected]>

Author: karuturi

server/src/com/cloud/user/AccountManagerImpl.java

@@ -159,6 +159,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;
@@ -755,8 +756,17 @@ protected boolean cleanupAccount(AccountVO account, long callerUserId, Account c
                 s_logger.debug("Expunging # of vms (accountId=" + accountId + "): " + vms.size());
             }
 
-            // no need to catch exception at this place as expunging vm should pass in order to perform further cleanup
             for (UserVmVO vm : vms) {
+                if (vm.getState() != VirtualMachine.State.Destroyed && vm.getState() != VirtualMachine.State.Expunging) {
+                    try {
+                        _vmMgr.destroyVm(vm.getId());
+                    } catch (Exception e) {
+                        e.printStackTrace();
+                        s_logger.warn("Failed destroying instance " + vm.getUuid() + " as part of account deletion.");
+                    }
+                }
+                // no need to catch exception at this place as expunging vm
+                // should pass in order to perform further cleanup
                 if (!_vmMgr.expunge(vm, callerUserId, caller)) {
                     s_logger.error("Unable to expunge vm: " + vm.getId());
                     accountCleanupNeeded = true;

server/test/com/cloud/user/AccountManagerImplTest.java

@@ -16,220 +16,35 @@
 // under the License.
 package com.cloud.user;
 
-import java.lang.reflect.Field;
 import java.net.InetAddress;
 import java.net.UnknownHostException;
-import java.util.Arrays;
 import java.util.ArrayList;
-
-import javax.inject.Inject;
-
+import java.util.Arrays;
 import com.cloud.server.auth.UserAuthenticator;
 import com.cloud.utils.Pair;
-import org.junit.After;
+
 import org.junit.Assert;
-import org.junit.Before;
 import org.junit.Test;
-import org.junit.runner.RunWith;
 import org.mockito.Mock;
 import org.mockito.Mockito;
-import org.mockito.runners.MockitoJUnitRunner;
-
 import org.apache.cloudstack.acl.ControlledEntity;
-import org.apache.cloudstack.acl.SecurityChecker;
 import org.apache.cloudstack.acl.SecurityChecker.AccessType;
-import org.apache.cloudstack.affinity.dao.AffinityGroupDao;
-import org.apache.cloudstack.context.CallContext;
-import org.apache.cloudstack.engine.orchestration.service.NetworkOrchestrationService;
-import org.apache.cloudstack.framework.config.dao.ConfigurationDao;
-import org.apache.cloudstack.framework.messagebus.MessageBus;
-import org.apache.cloudstack.region.gslb.GlobalLoadBalancerRuleDao;
-
-import com.cloud.vm.snapshot.VMSnapshotManager;
 import com.cloud.vm.snapshot.VMSnapshotVO;
-import com.cloud.vm.snapshot.dao.VMSnapshotDao;
-
-import com.cloud.configuration.ConfigurationManager;
-import com.cloud.configuration.dao.ResourceCountDao;
-import com.cloud.configuration.dao.ResourceLimitDao;
-import com.cloud.dc.dao.DataCenterDao;
-import com.cloud.dc.dao.DataCenterVnetDao;
-import com.cloud.dc.dao.DedicatedResourceDao;
 import com.cloud.domain.Domain;
 import com.cloud.domain.DomainVO;
-import com.cloud.domain.dao.DomainDao;
 import com.cloud.exception.ConcurrentOperationException;
 import com.cloud.exception.ResourceUnavailableException;
-import com.cloud.network.as.AutoScaleManager;
-import com.cloud.network.dao.AccountGuestVlanMapDao;
-import com.cloud.network.dao.IPAddressDao;
-import com.cloud.network.dao.NetworkDao;
-import com.cloud.network.dao.RemoteAccessVpnDao;
-import com.cloud.network.dao.VpnUserDao;
-import com.cloud.network.security.SecurityGroupManager;
-import com.cloud.network.security.dao.SecurityGroupDao;
-import com.cloud.network.vpc.VpcManager;
-import com.cloud.network.vpn.RemoteAccessVpnService;
-import com.cloud.network.vpn.Site2SiteVpnManager;
-import com.cloud.projects.ProjectManager;
-import com.cloud.projects.dao.ProjectAccountDao;
-import com.cloud.projects.dao.ProjectDao;
-import com.cloud.storage.VolumeApiService;
-import com.cloud.storage.dao.SnapshotDao;
-import com.cloud.storage.dao.VMTemplateDao;
-import com.cloud.storage.dao.VolumeDao;
-import com.cloud.storage.snapshot.SnapshotManager;
-import com.cloud.template.TemplateManager;
 import com.cloud.user.Account.State;
-import com.cloud.user.dao.AccountDao;
-import com.cloud.user.dao.UserAccountDao;
-import com.cloud.user.dao.UserDao;
-import com.cloud.vm.UserVmManager;
+import com.cloud.vm.UserVmManagerImpl;
 import com.cloud.vm.UserVmVO;
 import com.cloud.vm.VMInstanceVO;
-import com.cloud.vm.VirtualMachineManager;
-import com.cloud.vm.dao.DomainRouterDao;
-import com.cloud.vm.dao.InstanceGroupDao;
-import com.cloud.vm.dao.UserVmDao;
-import com.cloud.vm.dao.VMInstanceDao;
-import org.springframework.test.util.ReflectionTestUtils;
 
-@RunWith(MockitoJUnitRunner.class)
-public class AccountManagerImplTest {
-    @Mock
-    AccountDao _accountDao;
-    @Mock
-    ConfigurationDao _configDao;
-    @Mock
-    ResourceCountDao _resourceCountDao;
-    @Mock
-    UserDao _userDao;
-    @Mock
-    InstanceGroupDao _vmGroupDao;
-    @Mock
-    UserAccountDao _userAccountDao;
-    @Mock
-    VolumeDao _volumeDao;
-    @Mock
-    UserVmDao _userVmDao;
-    @Mock
-    VMTemplateDao _templateDao;
-    @Mock
-    NetworkDao _networkDao;
-    @Mock
-    SecurityGroupDao _securityGroupDao;
-    @Mock
-    VMInstanceDao _vmDao;
-    @Mock
-    protected SnapshotDao _snapshotDao;
-    @Mock
-    protected VMTemplateDao _vmTemplateDao;
-    @Mock
-    SecurityGroupManager _networkGroupMgr;
-    @Mock
-    NetworkOrchestrationService _networkMgr;
-    @Mock
-    SnapshotManager _snapMgr;
-    @Mock
-    UserVmManager _vmMgr;
-    @Mock
-    TemplateManager _tmpltMgr;
-    @Mock
-    ConfigurationManager _configMgr;
-    @Mock
-    VirtualMachineManager _itMgr;
-    @Mock
-    RemoteAccessVpnDao _remoteAccessVpnDao;
-    @Mock
-    RemoteAccessVpnService _remoteAccessVpnMgr;
-    @Mock
-    VpnUserDao _vpnUser;
-    @Mock
-    DataCenterDao _dcDao;
-    @Mock
-    DomainManager _domainMgr;
-    @Mock
-    ProjectManager _projectMgr;
-    @Mock
-    ProjectDao _projectDao;
-    @Mock
-    AccountDetailsDao _accountDetailsDao;
-    @Mock
-    DomainDao _domainDao;
-    @Mock
-    ProjectAccountDao _projectAccountDao;
-    @Mock
-    IPAddressDao _ipAddressDao;
-    @Mock
-    VpcManager _vpcMgr;
-    @Mock
-    DomainRouterDao _routerDao;
-    @Mock
-    Site2SiteVpnManager _vpnMgr;
-    @Mock
-    AutoScaleManager _autoscaleMgr;
-    @Mock
-    VolumeApiService volumeService;
-    @Mock
-    AffinityGroupDao _affinityGroupDao;
-    @Mock
-    AccountGuestVlanMapDao _accountGuestVlanMapDao;
-    @Mock
-    DataCenterVnetDao _dataCenterVnetDao;
-    @Mock
-    ResourceLimitService _resourceLimitMgr;
-    @Mock
-    ResourceLimitDao _resourceLimitDao;
-    @Mock
-    DedicatedResourceDao _dedicatedDao;
-    @Mock
-    GlobalLoadBalancerRuleDao _gslbRuleDao;
-    @Mock
-    MessageBus _messageBus;
 
-    @Mock
-    VMSnapshotManager _vmSnapshotMgr;
-    @Mock
-    VMSnapshotDao _vmSnapshotDao;
+public class AccountManagerImplTest extends AccountManagetImplTestBase {
 
-    @Mock
-    User callingUser;
-    @Mock
-    Account callingAccount;
-
-    AccountManagerImpl accountManager;
-
-    @Mock
-    SecurityChecker securityChecker;
 
     @Mock
-    private UserAuthenticator userAuthenticator;
-
-    @Before
-    public void setup() throws NoSuchFieldException, SecurityException,
-            IllegalArgumentException, IllegalAccessException {
-        accountManager = new AccountManagerImpl();
-        for (Field field : AccountManagerImpl.class.getDeclaredFields()) {
-            if (field.getAnnotation(Inject.class) != null) {
-                field.setAccessible(true);
-                try {
-                    Field mockField = this.getClass().getDeclaredField(
-                            field.getName());
-                    field.set(accountManager, mockField.get(this));
-                } catch (Exception e) {
-                    // ignore missing fields
-                }
-            }
-        }
-        ReflectionTestUtils.setField(accountManager, "_userAuthenticators", Arrays.asList(userAuthenticator));
-        accountManager.setSecurityCheckers(Arrays.asList(securityChecker));
-        CallContext.register(callingUser, callingAccount);
-    }
-
-    @After
-    public void cleanup() {
-        CallContext.unregister();
-    }
+    UserVmManagerImpl _vmMgr;
 
     @Test
     public void disableAccountNotexisting()
@@ -240,7 +55,7 @@ public void disableAccountNotexisting()
 
     @Test
     public void disableAccountDisabled() throws ConcurrentOperationException,
-            ResourceUnavailableException {
+    ResourceUnavailableException {
         AccountVO disabledAccount = new AccountVO();
         disabledAccount.setState(State.disabled);
         Mockito.when(_accountDao.findById(42l)).thenReturn(disabledAccount);
@@ -249,7 +64,7 @@ public void disableAccountDisabled() throws ConcurrentOperationException,
 
     @Test
     public void disableAccount() throws ConcurrentOperationException,
-            ResourceUnavailableException {
+    ResourceUnavailableException {
         AccountVO account = new AccountVO();
         account.setState(State.enabled);
         Mockito.when(_accountDao.findById(42l)).thenReturn(account);
@@ -272,17 +87,17 @@ public void deleteUserAccount() {
         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.any(ControlledEntity.class), Mockito.any(AccessType.class),
+                        Mockito.anyString()))
+        .thenReturn(true);
         Mockito.when(_accountDao.remove(42l)).thenReturn(true);
         Mockito.when(_configMgr.releaseAccountSpecificVirtualRanges(42l))
-                .thenReturn(true);
+        .thenReturn(true);
         Mockito.when(_domainMgr.getDomain(Mockito.anyLong())).thenReturn(domain);
         Mockito.when(
                 securityChecker.checkAccess(Mockito.any(Account.class),
                         Mockito.any(Domain.class)))
-                .thenReturn(true);
+        .thenReturn(true);
         Mockito.when(_vmSnapshotDao.listByAccountId(Mockito.anyLong())).thenReturn(new ArrayList<VMSnapshotVO>());
 
         Assert.assertTrue(accountManager.deleteUserAccount(42));
@@ -301,10 +116,10 @@ public void deleteUserAccountCleanup() {
                 securityChecker.checkAccess(Mockito.any(Account.class),
                         Mockito.any(ControlledEntity.class), Mockito.any(AccessType.class),
                         Mockito.anyString()))
-                .thenReturn(true);
+        .thenReturn(true);
         Mockito.when(_accountDao.remove(42l)).thenReturn(true);
         Mockito.when(_configMgr.releaseAccountSpecificVirtualRanges(42l))
-                .thenReturn(true);
+        .thenReturn(true);
         Mockito.when(_userVmDao.listByAccountId(42l)).thenReturn(
                 Arrays.asList(Mockito.mock(UserVmVO.class)));
         Mockito.when(
@@ -314,7 +129,7 @@ public void deleteUserAccountCleanup() {
         Mockito.when(
                 securityChecker.checkAccess(Mockito.any(Account.class),
                         Mockito.any(Domain.class)))
-                .thenReturn(true);
+        .thenReturn(true);
 
         Assert.assertTrue(accountManager.deleteUserAccount(42));
         // assert that this was NOT a clean delete
@@ -327,7 +142,7 @@ public void deleteUserAccountCleanup() {
     public void testAuthenticateUser() throws UnknownHostException {
         Pair<Boolean, UserAuthenticator.ActionOnFailedAuthentication> successAuthenticationPair = new Pair<>(true, null);
         Pair<Boolean, UserAuthenticator.ActionOnFailedAuthentication> failureAuthenticationPair = new Pair<>(false,
-                                                                                                             UserAuthenticator.ActionOnFailedAuthentication.INCREMENT_INCORRECT_LOGIN_ATTEMPT_COUNT);
+                UserAuthenticator.ActionOnFailedAuthentication.INCREMENT_INCORRECT_LOGIN_ATTEMPT_COUNT);
 
         UserAccountVO userAccountVO = new UserAccountVO();
         userAccountVO.setSource(User.Source.UNKNOWN);
@@ -353,6 +168,7 @@ 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);
-
     }
+
+
 }