Skip to content

CLOUDSTACK-9432: cluster/host dedicated to a domain is owned by the root domain #2124

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
GabrielBrascher merged 1 commit into from
Nov 28, 2017
+77 −39
Merged

CLOUDSTACK-9432: cluster/host dedicated to a domain is owned by the root domain #2124

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
73 changes: 45 additions & 28 deletions server/src/org/apache/cloudstack/affinity/AffinityGroupServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ public AffinityGroup createAffinityGroup(CreateAffinityGroupCmd createAffinityGr
@DB
@Override
public AffinityGroup createAffinityGroup(final String accountName, final Long projectId, final Long domainId, final String affinityGroupName, final String affinityGroupType,
final String description) {
final String description) {

// validate the affinityGroupType
Map<String, AffinityGroupProcessor> typeProcessorMap = getAffinityTypeToProcessorMap();
Expand Down Expand Up @@ -158,7 +158,7 @@ public AffinityGroup createAffinityGroup(final String accountName, final Long pr
verifyAffinityGroupNameInUse(owner.getAccountId(), owner.getDomainId(), affinityGroupName);
verifyDomainLevelAffinityGroupName(domainLevel, owner.getDomainId(), affinityGroupName);

AffinityGroupVO group = createAffinityGroup(processor, owner, aclType, affinityGroupName, affinityGroupType, description);
AffinityGroupVO group = createAffinityGroup(processor, owner, aclType, affinityGroupName, affinityGroupType, description, domainLevel, domainId);

if (s_logger.isDebugEnabled()) {
s_logger.debug("Created affinity group =" + affinityGroupName);
Expand All @@ -176,25 +176,27 @@ private void verifyAccessToDomainWideProcessor(Account caller, AffinityGroupProc
}
}

private AffinityGroupVO createAffinityGroup(final AffinityGroupProcessor processor, final Account owner, final ACLType aclType, final String affinityGroupName, final String affinityGroupType, final String description) {
private AffinityGroupVO createAffinityGroup(final AffinityGroupProcessor processor, final Account owner, final ACLType aclType, final String affinityGroupName, final String affinityGroupType, final String description, boolean domainLevel, Long domainId) {

final Long affinityGroupDomainId = getDomainIdBasedOnDomainLevel(owner, domainLevel, domainId);

return Transaction.execute(new TransactionCallback<AffinityGroupVO>() {
@Override
public AffinityGroupVO doInTransaction(TransactionStatus status) {
AffinityGroupVO group =
new AffinityGroupVO(affinityGroupName, affinityGroupType, description, owner.getDomainId(), owner.getId(), aclType);
AffinityGroupVO group = new AffinityGroupVO(affinityGroupName, affinityGroupType, description, affinityGroupDomainId, owner.getId(), aclType);
_affinityGroupDao.persist(group);

if (aclType == ACLType.Domain) {
boolean subDomainAccess = false;
subDomainAccess = processor.subDomainAccess();
AffinityGroupDomainMapVO domainMap = new AffinityGroupDomainMapVO(group.getId(), owner.getDomainId(),
AffinityGroupDomainMapVO domainMap = new AffinityGroupDomainMapVO(group.getId(), affinityGroupDomainId,
subDomainAccess);
_affinityGroupDomainMapDao.persist(domainMap);
//send event for storing the domain wide resource access
Map<String, Object> params = new HashMap<String, Object>();
params.put(ApiConstants.ENTITY_TYPE, AffinityGroup.class);
params.put(ApiConstants.ENTITY_ID, group.getId());
params.put(ApiConstants.DOMAIN_ID, owner.getDomainId());
params.put(ApiConstants.DOMAIN_ID, affinityGroupDomainId);
params.put(ApiConstants.SUBDOMAIN_ACCESS, subDomainAccess);
_messageBus.publish(_name, EntityManager.MESSAGE_ADD_DOMAIN_WIDE_ENTITY_EVENT, PublishScope.LOCAL,
params);
Expand All @@ -205,6 +207,20 @@ public AffinityGroupVO doInTransaction(TransactionStatus status) {
});
}

/**
* If the account is null (domainLevel is true), then returns the domain id passed as a
* parameter; otherwise (domainLevel is false) it returns the domain id from the owner account.
*
* @note: this method fixes a critical bug. More details in JIRA ticket CLOUDSTACK-9432.
*/
protected Long getDomainIdBasedOnDomainLevel(final Account owner, boolean domainLevel, Long domainId) {
Long domainIdBasedOnDomainLevel = owner.getDomainId();
if (domainLevel) {
domainIdBasedOnDomainLevel = domainId;
}
return domainIdBasedOnDomainLevel;
}

@GabrielBrascher GabrielBrascher Jun 21, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@rafaelweingartner thanks for the review. The code now is in a method with tests and documentation.

private DomainVO getDomain(Long domainId) {
DomainVO domain = _domainDao.findById(domainId);
if (domain == null) {
Expand All @@ -225,6 +241,7 @@ private void verifyDomainLevelAffinityGroupName(boolean domainLevel, long domain
}
}

@Override
@DB
@ActionEvent(eventType = EventTypes.EVENT_AFFINITY_GROUP_DELETE, eventDescription = "Deleting affinity group")
public boolean deleteAffinityGroup(Long affinityGroupId, String account, Long projectId, Long domainId, String affinityGroupName) {
Expand Down Expand Up @@ -258,7 +275,7 @@ private AffinityGroupVO getAffinityGroup(Long affinityGroupId, String account, L
throw new InvalidParameterValueException("Either the affinity group Id or group name must be specified to delete the group");
}
if (group == null) {
throw new InvalidParameterValueException("Unable to find affinity group " + (affinityGroupId == null ? affinityGroupName : affinityGroupId));
throw new InvalidParameterValueException("Unable to find affinity group " + (affinityGroupId == null ? affinityGroupName : affinityGroupId));
}
return group;
}
Expand Down Expand Up @@ -292,10 +309,10 @@ public void doInTransactionWithoutResult(TransactionStatus status) {
List<AffinityGroupVMMapVO> affinityGroupVmMap = _affinityGroupVMMapDao.listByAffinityGroup(affinityGroupId);
if (!affinityGroupVmMap.isEmpty()) {
SearchBuilder<AffinityGroupVMMapVO> listByAffinityGroup = _affinityGroupVMMapDao.createSearchBuilder();
listByAffinityGroup.and("affinityGroupId", listByAffinityGroup.entity().getAffinityGroupId(), SearchCriteria.Op.EQ);
listByAffinityGroup.and("affinityGroupId", listByAffinityGroup.entity().getAffinityGroupId(), SearchCriteria.Op.EQ);
listByAffinityGroup.done();
SearchCriteria<AffinityGroupVMMapVO> sc = listByAffinityGroup.create();
sc.setParameters("affinityGroupId", affinityGroupId);
sc.setParameters("affinityGroupId", affinityGroupId);

_affinityGroupVMMapDao.lockRows(sc, null, true);
_affinityGroupVMMapDao.remove(sc);
Expand Down Expand Up @@ -323,12 +340,12 @@ public List<String> listAffinityGroupTypes() {
List<String> types = new ArrayList<String>();

for (AffinityGroupProcessor processor : _affinityProcessors) {
if (processor.isAdminControlledGroup()) {
continue; // we dont list the type if this group can be
// created only as an admin/system operation.
}
types.add(processor.getType());
if (processor.isAdminControlledGroup()) {

@rafaelweingartner rafaelweingartner May 26, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Are these formatting changes needed?
Are you using the CloudStack code formatter style? Or was the code not formatted properly?

@GabrielBrascher GabrielBrascher May 26, 2017 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

At first I tried to submit without those changes in lines which I did not touch; however, it failed to pass in check style validation. When my environment formatted the code (with the CloudStack style), all checks have passed. I can review those changes and see if I can minimize them. But some will be necessary.

@rafaelweingartner rafaelweingartner May 27, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah, ok.
So, let the changes there

continue; // we dont list the type if this group can be
// created only as an admin/system operation.
}
types.add(processor.getType());
}

return types;
}
Expand Down Expand Up @@ -394,20 +411,20 @@ public boolean preStateTransitionEvent(State oldState, Event event, State newSta

@Override
public boolean postStateTransitionEvent(StateMachine2.Transition<State, Event> transition, VirtualMachine vo, boolean status, Object opaque) {
if (!status) {
return false;
}
State newState = transition.getToState();
if ((newState == State.Expunging) || (newState == State.Error)) {
// cleanup all affinity groups associations of the Expunged VM
SearchCriteria<AffinityGroupVMMapVO> sc = _affinityGroupVMMapDao.createSearchCriteria();
sc.addAnd("instanceId", SearchCriteria.Op.EQ, vo.getId());
_affinityGroupVMMapDao.expunge(sc);
}
return true;
if (!status) {
return false;
}
State newState = transition.getToState();
if ((newState == State.Expunging) || (newState == State.Error)) {
// cleanup all affinity groups associations of the Expunged VM
SearchCriteria<AffinityGroupVMMapVO> sc = _affinityGroupVMMapDao.createSearchCriteria();
sc.addAnd("instanceId", SearchCriteria.Op.EQ, vo.getId());
_affinityGroupVMMapDao.expunge(sc);
}
return true;
}

@Override
@Override
public UserVm updateVMAffinityGroups(Long vmId, List<Long> affinityGroupIds) {
// Verify input parameters
UserVmVO vmInstance = _userVmDao.findById(vmId);
Expand All @@ -419,7 +436,7 @@ public UserVm updateVMAffinityGroups(Long vmId, List<Long> affinityGroupIds) {
if (!vmInstance.getState().equals(State.Stopped)) {
s_logger.warn("Unable to update affinity groups of the virtual machine " + vmInstance.toString() + " in state " + vmInstance.getState());
throw new InvalidParameterValueException("Unable update affinity groups of the virtual machine " + vmInstance.toString() + " " + "in state " +
vmInstance.getState() + "; make sure the virtual machine is stopped and not in an error state before updating.");
vmInstance.getState() + "; make sure the virtual machine is stopped and not in an error state before updating.");
}

Account caller = CallContext.current().getCallingAccount();
Expand Down
43 changes: 32 additions & 11 deletions server/test/org/apache/cloudstack/affinity/AffinityGroupServiceImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,14 @@
import javax.inject.Inject;
import javax.naming.ConfigurationException;

import com.cloud.utils.db.EntityManager;
import com.cloud.event.ActionEventUtils;
import com.cloud.user.User;

import org.apache.cloudstack.acl.ControlledEntity;
import org.apache.cloudstack.affinity.dao.AffinityGroupDao;
import org.apache.cloudstack.affinity.dao.AffinityGroupDomainMapDao;
import org.apache.cloudstack.affinity.dao.AffinityGroupVMMapDao;
import org.apache.cloudstack.api.command.user.affinitygroup.CreateAffinityGroupCmd;
import org.apache.cloudstack.context.CallContext;
import org.apache.cloudstack.framework.config.dao.ConfigurationDao;
import org.apache.cloudstack.framework.messagebus.MessageBus;
import org.apache.cloudstack.test.utils.SpringUtils;
import org.junit.After;
import org.junit.Before;
Expand All @@ -57,33 +59,32 @@
import org.springframework.test.context.ContextConfiguration;
import org.springframework.test.context.junit4.SpringJUnit4ClassRunner;
import org.springframework.test.context.support.AnnotationConfigContextLoader;
import org.apache.cloudstack.acl.ControlledEntity;
import org.apache.cloudstack.affinity.dao.AffinityGroupDomainMapDao;
import org.apache.cloudstack.api.command.user.affinitygroup.CreateAffinityGroupCmd;
import org.apache.cloudstack.context.CallContext;
import org.apache.cloudstack.framework.config.dao.ConfigurationDao;
import org.apache.cloudstack.framework.messagebus.MessageBus;

import com.cloud.dc.dao.DedicatedResourceDao;
import com.cloud.domain.dao.DomainDao;
import com.cloud.event.ActionEventUtils;
import com.cloud.event.EventVO;
import com.cloud.event.dao.EventDao;
import com.cloud.exception.InvalidParameterValueException;
import com.cloud.exception.ResourceInUseException;
import com.cloud.hypervisor.Hypervisor.HypervisorType;
import com.cloud.projects.dao.ProjectDao;
import com.cloud.user.Account;
import com.cloud.user.AccountManager;
import com.cloud.user.AccountService;
import com.cloud.user.AccountVO;
import com.cloud.user.DomainManager;
import com.cloud.user.User;
import com.cloud.user.UserVO;
import com.cloud.user.dao.AccountDao;
import com.cloud.user.dao.UserDao;
import com.cloud.utils.component.ComponentContext;
import com.cloud.utils.db.EntityManager;
import com.cloud.vm.UserVmVO;
import com.cloud.vm.VirtualMachine;
import com.cloud.vm.dao.UserVmDao;
import com.cloud.projects.dao.ProjectDao;

import org.junit.Assert;

@RunWith(SpringJUnit4ClassRunner.class)
@ContextConfiguration(loader = AnnotationConfigContextLoader.class)
Expand Down Expand Up @@ -191,6 +192,26 @@ public void createAffinityGroupTest() {

}

private AccountVO mockOwnerForTestGetDomainIdBasedOnDomainLevel() {
AccountVO mockOwner = Mockito.mock(AccountVO.class);
when(mockOwner.getDomainId()).thenReturn(0l);
return mockOwner;
}

@Test
public void getDomainIdBasedOnDomainLevelTestDomainLevelTrue() {
AccountVO owner = mockOwnerForTestGetDomainIdBasedOnDomainLevel();
Long domainIdBasedOnDomainLevel = _affinityService.getDomainIdBasedOnDomainLevel(owner, true, 1l);
Assert.assertEquals(new Long(1), domainIdBasedOnDomainLevel);
}

@Test
public void getDomainIdBasedOnDomainLevelTestDomainLevelFalse() {
AccountVO owner = mockOwnerForTestGetDomainIdBasedOnDomainLevel();
Long domainIdBasedOnDomainLevel = _affinityService.getDomainIdBasedOnDomainLevel(owner, false, 1l);
Assert.assertEquals(new Long(0), domainIdBasedOnDomainLevel);
}

@Test
public void shouldDeleteDomainLevelAffinityGroup() {
AffinityGroupVO mockGroup = Mockito.mock(AffinityGroupVO.class);
Expand Down