From 6eb5c491621925fb0aecdfc93d79969ab48ff913 Mon Sep 17 00:00:00 2001 From: nnesic Date: Mon, 12 Oct 2015 13:15:30 +0000 Subject: [PATCH] Emit a VOLUME_DELETE usage event when account deletion destroys an instance. Currently the logic about volume deletion seems to be that an event should be emitted when the volume delete is requested, not when the deletion completes. The VolumeStateListener specifically ignores destroy events for ROOT volumes, assuming that the ROOT volume only gets deleted when the instance is destroyed and the UserVmManager should take care of it. When deleting an account, all of its resources get destroyed, but the instance expunging circumvents the UserVmManager, and thus we miss the VOLUME_DESTROY usage event. Added a check in the AccountManager to emit the deletion event for ROOT volumes belonging to instances which weren't destroyed prior to the account deletion. --- .../com/cloud/user/AccountManagerImpl.java | 24 ++ .../cloud/user/AccountManagerImplTest.java | 154 +++++++- .../com/cloud/user/MockUsageEventDao.java | 356 ++++++++++++++++++ 3 files changed, 533 insertions(+), 1 deletion(-) create mode 100644 server/test/com/cloud/user/MockUsageEventDao.java diff --git a/server/src/com/cloud/user/AccountManagerImpl.java b/server/src/com/cloud/user/AccountManagerImpl.java index d3eb40c7db69..b4bc04afcb53 100644 --- a/server/src/com/cloud/user/AccountManagerImpl.java +++ b/server/src/com/cloud/user/AccountManagerImpl.java @@ -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; @@ -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; @@ -677,6 +679,17 @@ public boolean deleteAccount(AccountVO account, long callerUserId, Account calle return cleanupAccount(account, callerUserId, caller); } + protected List getExpungedInstanceRootVolume(long instanceId) { + SearchBuilder sb = _volumeDao.createSearchBuilder(); + sb.and("instanceId", sb.entity().getInstanceId(), SearchCriteria.Op.EQ); + sb.and("vType", sb.entity().getVolumeType(), SearchCriteria.Op.EQ); + sb.done(); + SearchCriteria 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) { long accountId = account.getId(); boolean accountCleanupNeeded = false; @@ -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)) { + // 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 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 diff --git a/server/test/com/cloud/user/AccountManagerImplTest.java b/server/test/com/cloud/user/AccountManagerImplTest.java index a895ec27d778..6852432caf3e 100644 --- a/server/test/com/cloud/user/AccountManagerImplTest.java +++ b/server/test/com/cloud/user/AccountManagerImplTest.java @@ -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; @@ -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; @@ -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; @@ -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; @@ -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; @@ -192,6 +207,8 @@ public class AccountManagerImplTest { @Mock VMSnapshotDao _vmSnapshotDao; + UsageEventDao _usageEventDao = new MockUsageEventDao(); + @Mock User callingUser; @Mock @@ -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 oldFields = new HashMap<>(); + @Before public void setup() throws NoSuchFieldException, SecurityException, IllegalArgumentException, IllegalAccessException { @@ -231,6 +253,73 @@ public void cleanup() { CallContext.unregister(); } + public UsageEventUtils setupUsageUtils() { + _usageEventDao = new MockUsageEventDao(); + UsageEventUtils utils = new UsageEventUtils(); + + Map usageUtilsFields = new HashMap(); + 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 { @@ -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 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 sb = (SearchBuilder)Mockito.mock(SearchBuilder.class); + + Mockito.when(_volumeDao.createSearchBuilder()).thenReturn(sb); + Mockito.when(sb.create()).thenReturn((SearchCriteria) 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 emittedEvents = deleteUserAccountRootVolumeUsageEvents(true); + Assert.assertEquals(0, emittedEvents.size()); + cleanupUsageUtils(); + } + + @Test + public void runningVMRootVolumeUsageEvent() { + setupUsageUtils(); + List emittedEvents = deleteUserAccountRootVolumeUsageEvents(false); + Assert.assertEquals(1, emittedEvents.size()); + cleanupUsageUtils(); } } diff --git a/server/test/com/cloud/user/MockUsageEventDao.java b/server/test/com/cloud/user/MockUsageEventDao.java new file mode 100644 index 000000000000..5f9f3c9ff565 --- /dev/null +++ b/server/test/com/cloud/user/MockUsageEventDao.java @@ -0,0 +1,356 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. +package com.cloud.user; + +import java.util.ArrayList; +import java.util.Date; +import java.util.List; +import java.util.Map; + +import javax.naming.ConfigurationException; + +import com.cloud.event.UsageEventVO; +import com.cloud.event.dao.UsageEventDao; +import com.cloud.utils.Pair; +import com.cloud.utils.db.Attribute; +import com.cloud.utils.db.Filter; +import com.cloud.utils.db.GenericSearchBuilder; +import com.cloud.utils.db.SearchBuilder; +import com.cloud.utils.db.SearchCriteria; + +public class MockUsageEventDao implements UsageEventDao{ + + List persistedItems; + + public MockUsageEventDao() { + persistedItems = new ArrayList(); + } + + @Override + public UsageEventVO findById(Long id) { + // TODO Auto-generated method stub + return null; + } + + @Override + public UsageEventVO findByIdIncludingRemoved(Long id) { + // TODO Auto-generated method stub + return null; + } + + @Override + public UsageEventVO findById(Long id, boolean fresh) { + // TODO Auto-generated method stub + return null; + } + + @Override + public UsageEventVO findByUuid(String uuid) { + // TODO Auto-generated method stub + return null; + } + + @Override + public UsageEventVO findByUuidIncludingRemoved(String uuid) { + // TODO Auto-generated method stub + return null; + } + + @Override + public UsageEventVO createForUpdate() { + // TODO Auto-generated method stub + return null; + } + + @Override + public SearchBuilder createSearchBuilder() { + // TODO Auto-generated method stub + return null; + } + + @Override + public GenericSearchBuilder createSearchBuilder( + Class clazz) { + // TODO Auto-generated method stub + return null; + } + + @Override + public UsageEventVO createForUpdate(Long id) { + // TODO Auto-generated method stub + return null; + } + + @Override + public SearchCriteria createSearchCriteria() { + // TODO Auto-generated method stub + return null; + } + + @Override + public List lockRows(SearchCriteria sc, + Filter filter, boolean exclusive) { + // TODO Auto-generated method stub + return null; + } + + @Override + public UsageEventVO lockOneRandomRow(SearchCriteria sc, + boolean exclusive) { + // TODO Auto-generated method stub + return null; + } + + @Override + public UsageEventVO lockRow(Long id, Boolean exclusive) { + // TODO Auto-generated method stub + return null; + } + + @Override + public UsageEventVO acquireInLockTable(Long id) { + // TODO Auto-generated method stub + return null; + } + + @Override + public UsageEventVO acquireInLockTable(Long id, int seconds) { + // TODO Auto-generated method stub + return null; + } + + @Override + public boolean releaseFromLockTable(Long id) { + // TODO Auto-generated method stub + return false; + } + + @Override + public boolean update(Long id, UsageEventVO entity) { + // TODO Auto-generated method stub + return false; + } + + @Override + public int update(UsageEventVO entity, SearchCriteria sc) { + // TODO Auto-generated method stub + return 0; + } + + @Override + public List listAll() { + + return persistedItems; + } + + @Override + public List listAll(Filter filter) { + // TODO Auto-generated method stub + return null; + } + + @Override + public List search(SearchCriteria sc, + Filter filter) { + // TODO Auto-generated method stub + return null; + } + + @Override + public List search(SearchCriteria sc, + Filter filter, boolean enableQueryCache) { + // TODO Auto-generated method stub + return null; + } + + @Override + public List searchIncludingRemoved( + SearchCriteria sc, Filter filter, Boolean lock, + boolean cache) { + // TODO Auto-generated method stub + return null; + } + + @Override + public List searchIncludingRemoved( + SearchCriteria sc, Filter filter, Boolean lock, + boolean cache, boolean enableQueryCache) { + // TODO Auto-generated method stub + return null; + } + + @Override + public List customSearchIncludingRemoved(SearchCriteria sc, + Filter filter) { + // TODO Auto-generated method stub + return null; + } + + @Override + public List listAllIncludingRemoved() { + // TODO Auto-generated method stub + return null; + } + + @Override + public List listAllIncludingRemoved(Filter filter) { + // TODO Auto-generated method stub + return null; + } + + @Override + public UsageEventVO persist(UsageEventVO entity) { + persistedItems.add(entity); + return entity; + } + + @Override + public boolean remove(Long id) { + // TODO Auto-generated method stub + return false; + } + + @Override + public int remove(SearchCriteria sc) { + // TODO Auto-generated method stub + return 0; + } + + @Override + public boolean expunge(Long id) { + // TODO Auto-generated method stub + return false; + } + + @Override + public int expunge(SearchCriteria sc) { + // TODO Auto-generated method stub + return 0; + } + + @Override + public void expunge() { + // TODO Auto-generated method stub + + } + + @Override + public K getNextInSequence(Class clazz, String name) { + // TODO Auto-generated method stub + return null; + } + + @Override + public boolean configure(String name, Map params) + throws ConfigurationException { + // TODO Auto-generated method stub + return false; + } + + @Override + public List customSearch(SearchCriteria sc, Filter filter) { + // TODO Auto-generated method stub + return null; + } + + @Override + public boolean lockInLockTable(String id) { + // TODO Auto-generated method stub + return false; + } + + @Override + public boolean lockInLockTable(String id, int seconds) { + // TODO Auto-generated method stub + return false; + } + + @Override + public boolean unlockFromLockTable(String id) { + // TODO Auto-generated method stub + return false; + } + + @Override + public K getRandomlyIncreasingNextInSequence(Class clazz, String name) { + // TODO Auto-generated method stub + return null; + } + + @Override + public UsageEventVO findOneBy(SearchCriteria sc) { + // TODO Auto-generated method stub + return null; + } + + @Override + public Class getEntityBeanType() { + // TODO Auto-generated method stub + return null; + } + + @Override + public Pair, Integer> searchAndCount( + SearchCriteria sc, Filter filter) { + // TODO Auto-generated method stub + return null; + } + + @Override + public Pair, Integer> searchAndDistinctCount(SearchCriteria sc, Filter filter) { + //TODO Auto-generated method stub + return null; + } + + @Override + public Map getAllAttributes() { + // TODO Auto-generated method stub + return null; + } + + @Override + public List listLatestEvents(Date endDate) { + // TODO Auto-generated method stub + return null; + } + + @Override + public List getLatestEvent() { + // TODO Auto-generated method stub + return null; + } + + @Override + public List getRecentEvents(Date endDate) { + // TODO Auto-generated method stub + return null; + } + + @Override + public List listDirectIpEvents(Date startDate, Date endDate, + long zoneId) { + // TODO Auto-generated method stub + return null; + } + + @Override + public void saveDetails(long eventId, Map details) { + // TODO Auto-generated method stub + + } + +}