Skip to content

Refactor system VM default network creation #1360

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
asfgit merged 2 commits into from
May 26, 2016
Merged

Refactor system VM default network creation #1360

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3e2c18a
Refactor console proxy default network retrieval.
Jan 22, 2016
8971875
Refactor ssvm default network retrieval.
Jan 22, 2016
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
90 changes: 72 additions & 18 deletions server/src/com/cloud/consoleproxy/ConsoleProxyManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
import org.apache.cloudstack.storage.datastore.db.StoragePoolVO;
import org.apache.cloudstack.storage.datastore.db.TemplateDataStoreDao;
import org.apache.cloudstack.storage.datastore.db.TemplateDataStoreVO;
import org.apache.commons.collections.CollectionUtils;
import org.apache.log4j.Logger;

import com.cloud.agent.AgentManager;
Expand Down Expand Up @@ -666,35 +667,88 @@ public ConsoleProxyVO startNew(long dataCenterId) throws ConcurrentOperationExce
return null;
}

protected Map<String, Object> createProxyInstance(long dataCenterId, VMTemplateVO template) throws ConcurrentOperationException {

long id = _consoleProxyDao.getNextInSequence(Long.class, "id");
String name = VirtualMachineName.getConsoleProxyName(id, _instance);
DataCenterVO dc = _dcDao.findById(dataCenterId);
Account systemAcct = _accountMgr.getSystemAccount();
/**
* Get the default network for the console proxy VM, based on the zone it is in. Delegates to
* either {@link #getDefaultNetworkForZone(DataCenter)} or {@link #getDefaultNetworkForAdvancedSGZone(DataCenter)},
* depending on the zone network type and whether or not security groups are enabled in the zone.
* @param dc - The zone (DataCenter) of the console proxy VM.
* @return The default network for use with the console proxy VM.
*/
protected NetworkVO getDefaultNetworkForCreation(DataCenter dc) {
Copy link
Member

@GabrielBrascher GabrielBrascher Jan 23, 2016

Choose a reason for hiding this comment

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

@ProjectMoon Could you please create a documentation and a test case for this method ("com.cloud.consoleproxy.ConsoleProxyManagerImpl.getDefaultNetworkForCreation(DataCenter)")?

if (dc.getNetworkType() == NetworkType.Advanced) {
return getDefaultNetworkForAdvancedZone(dc);
} else {
return getDefaultNetworkForBasicZone(dc);
}
}

DataCenterDeployment plan = new DataCenterDeployment(dataCenterId);
/**
* Get default network for a console proxy VM starting up in an advanced zone. If the zone
* is security group-enabled, the first network found that supports SG services is returned.
* If the zone is not SG-enabled, the Public network is returned.
* @param dc - The zone.
* @return The selected default network.
* @throws CloudRuntimeException - If the zone is not a valid choice or a network couldn't be found.
*/
protected NetworkVO getDefaultNetworkForAdvancedZone(DataCenter dc) {
if (dc.getNetworkType() != NetworkType.Advanced) {
throw new CloudRuntimeException("Zone " + dc + " is not advanced.");
}

NetworkVO defaultNetwork = null;
if (dc.getNetworkType() == NetworkType.Advanced && dc.isSecurityGroupEnabled()) {
List<NetworkVO> networks = _networkDao.listByZoneSecurityGroup(dataCenterId);
if (networks == null || networks.size() == 0) {
if (dc.isSecurityGroupEnabled()) {
List<NetworkVO> networks = _networkDao.listByZoneSecurityGroup(dc.getId());
if (CollectionUtils.isEmpty(networks)) {
throw new CloudRuntimeException("Can not found security enabled network in SG Zone " + dc);
}
defaultNetwork = networks.get(0);
} else {

return networks.get(0);
}
else {
TrafficType defaultTrafficType = TrafficType.Public;
if (dc.getNetworkType() == NetworkType.Basic || dc.isSecurityGroupEnabled()) {
defaultTrafficType = TrafficType.Guest;
}
List<NetworkVO> defaultNetworks = _networkDao.listByZoneAndTrafficType(dataCenterId, defaultTrafficType);
List<NetworkVO> defaultNetworks = _networkDao.listByZoneAndTrafficType(dc.getId(), defaultTrafficType);

// api should never allow this situation to happen
if (defaultNetworks.size() != 1) {
throw new CloudRuntimeException("Found " + defaultNetworks.size() + " networks of type " + defaultTrafficType + " when expect to find 1");
}
defaultNetwork = defaultNetworks.get(0);

return defaultNetworks.get(0);
}
}

/**
* Get default network for console proxy VM for starting up in a basic zone. Basic zones select
* the Guest network whether or not the zone is SG-enabled.
* @param dc - The zone.
* @return The default network according to the zone's network selection rules.
* @throws CloudRuntimeException - If the zone is not a valid choice or a network couldn't be found.
*/
protected NetworkVO getDefaultNetworkForBasicZone(DataCenter dc) {
if (dc.getNetworkType() != NetworkType.Basic) {
throw new CloudRuntimeException("Zone " + dc + "is not basic.");
}

TrafficType defaultTrafficType = TrafficType.Guest;
List<NetworkVO> defaultNetworks = _networkDao.listByZoneAndTrafficType(dc.getId(), defaultTrafficType);

// api should never allow this situation to happen
if (defaultNetworks.size() != 1) {
throw new CloudRuntimeException("Found " + defaultNetworks.size() + " networks of type " + defaultTrafficType + " when expect to find 1");
}

return defaultNetworks.get(0);
}

protected Map<String, Object> createProxyInstance(long dataCenterId, VMTemplateVO template) throws ConcurrentOperationException {

long id = _consoleProxyDao.getNextInSequence(Long.class, "id");
String name = VirtualMachineName.getConsoleProxyName(id, _instance);
DataCenterVO dc = _dcDao.findById(dataCenterId);
Account systemAcct = _accountMgr.getSystemAccount();

DataCenterDeployment plan = new DataCenterDeployment(dataCenterId);

NetworkVO defaultNetwork = getDefaultNetworkForCreation(dc);

List<? extends NetworkOffering> offerings =
_networkModel.getSystemAccountNetworkOfferings(NetworkOffering.SystemControlNetwork, NetworkOffering.SystemManagementNetwork);
Expand Down
159 changes: 159 additions & 0 deletions server/test/com/cloud/consoleproxy/ConsoleProxyManagerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,33 @@

package com.cloud.consoleproxy;

import static org.mockito.AdditionalMatchers.not;
import static org.mockito.Matchers.any;
import static org.mockito.Matchers.anyLong;
import static org.mockito.Matchers.eq;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

import java.util.Collections;

import org.apache.log4j.Logger;
import org.junit.Assert;
import org.junit.Before;
import org.junit.Test;
import org.mockito.Mock;
import org.mockito.Mockito;
import org.mockito.MockitoAnnotations;
import org.springframework.test.util.ReflectionTestUtils;

import com.cloud.dc.DataCenterVO;
import com.cloud.dc.DataCenter;
import com.cloud.dc.DataCenter.NetworkType;
import com.cloud.dc.dao.DataCenterDao;
import com.cloud.network.Networks.TrafficType;
import com.cloud.network.dao.NetworkDao;
import com.cloud.network.dao.NetworkVO;
import com.cloud.utils.db.GlobalLock;
import com.cloud.utils.exception.CloudRuntimeException;
import com.cloud.vm.ConsoleProxyVO;

public class ConsoleProxyManagerTest {
Expand All @@ -37,13 +55,22 @@ public class ConsoleProxyManagerTest {
@Mock
ConsoleProxyVO proxyVO;
@Mock
DataCenterDao _dcDao;
@Mock
NetworkDao _networkDao;
@Mock
ConsoleProxyManagerImpl cpvmManager;

@Before
public void setup() throws Exception {
MockitoAnnotations.initMocks(this);
ReflectionTestUtils.setField(cpvmManager, "_allocProxyLock", globalLock);
ReflectionTestUtils.setField(cpvmManager, "_dcDao", _dcDao);
ReflectionTestUtils.setField(cpvmManager, "_networkDao", _networkDao);
Mockito.doCallRealMethod().when(cpvmManager).expandPool(Mockito.anyLong(), Mockito.anyObject());
Mockito.doCallRealMethod().when(cpvmManager).getDefaultNetworkForCreation(Mockito.any(DataCenter.class));
Mockito.doCallRealMethod().when(cpvmManager).getDefaultNetworkForAdvancedZone(Mockito.any(DataCenter.class));
Mockito.doCallRealMethod().when(cpvmManager).getDefaultNetworkForBasicZone(Mockito.any(DataCenter.class));
}

@Test
Expand Down Expand Up @@ -87,4 +114,136 @@ public void testExisingCPVMStartFailure() throws Exception {

cpvmManager.expandPool(new Long(1), new Object());
}

@Test
public void getDefaultNetworkForAdvancedNonSG() {
DataCenterVO dc = mock(DataCenterVO.class);
when(dc.getNetworkType()).thenReturn(NetworkType.Advanced);
when(dc.isSecurityGroupEnabled()).thenReturn(false);

when(_dcDao.findById(Mockito.anyLong())).thenReturn(dc);

NetworkVO network = Mockito.mock(NetworkVO.class);
NetworkVO badNetwork = Mockito.mock(NetworkVO.class);
when(_networkDao.listByZoneAndTrafficType(anyLong(), eq(TrafficType.Public)))
.thenReturn(Collections.singletonList(network));

when(_networkDao.listByZoneAndTrafficType(anyLong(), not(eq(TrafficType.Public))))
.thenReturn(Collections.singletonList(badNetwork));

when(_networkDao.listByZoneSecurityGroup(anyLong()))
.thenReturn(Collections.singletonList(badNetwork));

NetworkVO returnedNetwork = cpvmManager.getDefaultNetworkForAdvancedZone(dc);

Assert.assertNotNull(returnedNetwork);
Assert.assertEquals(network, returnedNetwork);
Assert.assertNotEquals(badNetwork, returnedNetwork);
}

@Test
public void getDefaultNetworkForAdvancedSG() {
Copy link
Contributor

@koushik-das koushik-das Apr 6, 2016

Choose a reason for hiding this comment

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

I don't see any security group related check in getDefaultNetworkForBasicZone(). Why do you need tests for SG/non-SG in case basic zone?

Copy link
Author

@ProjectMoon ProjectMoon Apr 7, 2016

Choose a reason for hiding this comment

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

To cover all scenarios. Even if there is no security group-related checks in getDefaultNetworkForBasicZone, client code doesn't necessarily know this and the unit tests should make sure all possible combinations work.

Copy link
Contributor

@koushik-das koushik-das Apr 7, 2016

Choose a reason for hiding this comment

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

@ProjectMoon The unit tests getDefaultNetworkForBasicNonSG() and getDefaultNetworkForBasicSG() are same since SG has no impact on the method being tested.
The line "when(dc.isSecurityGroupEnabled()).thenReturn(true or false);" doesn't impact test outcome. Thats what I mentioned in my initial comment.

DataCenterVO dc = Mockito.mock(DataCenterVO.class);
when(dc.getNetworkType()).thenReturn(NetworkType.Advanced);
when(dc.isSecurityGroupEnabled()).thenReturn(true);

when(_dcDao.findById(Mockito.anyLong())).thenReturn(dc);

NetworkVO network = Mockito.mock(NetworkVO.class);
NetworkVO badNetwork = Mockito.mock(NetworkVO.class);
when(_networkDao.listByZoneAndTrafficType(anyLong(), any(TrafficType.class)))
.thenReturn(Collections.singletonList(badNetwork));

when(_networkDao.listByZoneSecurityGroup(anyLong()))
.thenReturn(Collections.singletonList(network));

NetworkVO returnedNetwork = cpvmManager.getDefaultNetworkForAdvancedZone(dc);

Assert.assertEquals(network, returnedNetwork);
Assert.assertNotEquals(badNetwork, returnedNetwork);
}

@Test
public void getDefaultNetworkForBasicNonSG() {
DataCenterVO dc = Mockito.mock(DataCenterVO.class);
when(dc.getNetworkType()).thenReturn(NetworkType.Basic);
when(dc.isSecurityGroupEnabled()).thenReturn(false);

when(_dcDao.findById(Mockito.anyLong())).thenReturn(dc);

NetworkVO network = Mockito.mock(NetworkVO.class);
NetworkVO badNetwork = Mockito.mock(NetworkVO.class);
when(_networkDao.listByZoneAndTrafficType(anyLong(), eq(TrafficType.Guest)))
.thenReturn(Collections.singletonList(network));

when(_networkDao.listByZoneAndTrafficType(anyLong(), not(eq(TrafficType.Guest))))
.thenReturn(Collections.singletonList(badNetwork));

NetworkVO returnedNetwork = cpvmManager.getDefaultNetworkForBasicZone(dc);
Assert.assertNotNull(returnedNetwork);
Assert.assertEquals(network, returnedNetwork);
Assert.assertNotEquals(badNetwork, returnedNetwork);
}

@Test
public void getDefaultNetworkForBasicSG() {
DataCenterVO dc = Mockito.mock(DataCenterVO.class);
when(dc.getNetworkType()).thenReturn(NetworkType.Basic);
when(dc.isSecurityGroupEnabled()).thenReturn(true);

when(_dcDao.findById(Mockito.anyLong())).thenReturn(dc);

NetworkVO network = Mockito.mock(NetworkVO.class);
NetworkVO badNetwork = Mockito.mock(NetworkVO.class);
when(_networkDao.listByZoneAndTrafficType(anyLong(), eq(TrafficType.Guest)))
.thenReturn(Collections.singletonList(network));

when(_networkDao.listByZoneAndTrafficType(anyLong(), not(eq(TrafficType.Guest))))
.thenReturn(Collections.singletonList(badNetwork));

NetworkVO returnedNetwork = cpvmManager.getDefaultNetworkForBasicZone(dc);

Assert.assertNotNull(returnedNetwork);
Assert.assertEquals(network, returnedNetwork);
Assert.assertNotEquals(badNetwork, returnedNetwork);
}

//also test invalid input
@Test(expected=CloudRuntimeException.class)
public void getDefaultNetworkForBasicSGWrongZoneType() {
DataCenterVO dc = Mockito.mock(DataCenterVO.class);
when(dc.getNetworkType()).thenReturn(NetworkType.Advanced);
when(dc.isSecurityGroupEnabled()).thenReturn(true);

when(_dcDao.findById(Mockito.anyLong())).thenReturn(dc);

NetworkVO network = Mockito.mock(NetworkVO.class);
NetworkVO badNetwork = Mockito.mock(NetworkVO.class);
when(_networkDao.listByZoneAndTrafficType(anyLong(), eq(TrafficType.Guest)))
.thenReturn(Collections.singletonList(network));

when(_networkDao.listByZoneAndTrafficType(anyLong(), not(eq(TrafficType.Guest))))
.thenReturn(Collections.singletonList(badNetwork));

cpvmManager.getDefaultNetworkForBasicZone(dc);
}

@Test(expected=CloudRuntimeException.class)
public void getDefaultNetworkForAdvancedWrongZoneType() {
DataCenterVO dc = Mockito.mock(DataCenterVO.class);
when(dc.getNetworkType()).thenReturn(NetworkType.Basic);
when(dc.isSecurityGroupEnabled()).thenReturn(true);

when(_dcDao.findById(Mockito.anyLong())).thenReturn(dc);

NetworkVO network = Mockito.mock(NetworkVO.class);
NetworkVO badNetwork = Mockito.mock(NetworkVO.class);
when(_networkDao.listByZoneAndTrafficType(anyLong(), any(TrafficType.class)))
.thenReturn(Collections.singletonList(badNetwork));

when(_networkDao.listByZoneSecurityGroup(anyLong()))
.thenReturn(Collections.singletonList(network));

cpvmManager.getDefaultNetworkForAdvancedZone(dc);
}
}
Loading