Skip to content

Commit

Permalink
Fixed issues from CLOUDSTACK-8800 that were introduced in PR 780
Browse files Browse the repository at this point in the history 
 
It was worked around some possible runtime exceptions introduced by the
changes that were added by the PR 780. Basically, the points in which a
null pointer exception could happen, we added safety checks to avoid
them. It was create a specific method do that, all together test cases
were created for this newly method that was added.
  • Loading branch information
@rafaelweingartner
rafaelweingartner committed May 3, 2016
1 parent 7ad3c5e commit 866ac7b
Show file tree
Hide file tree
Showing 3 changed files with 84 additions and 44 deletions.
43 changes: 24 additions & 19 deletions plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,15 +52,17 @@
import org.apache.cloudstack.utils.qemu.QemuImg.PhysicalDiskFormat;
import org.apache.commons.io.FileUtils;
import org.apache.commons.io.IOUtils;
import org.apache.commons.lang.ArrayUtils;
import org.apache.commons.lang.math.NumberUtils;
import org.apache.log4j.Logger;
import org.libvirt.Connect;
import org.libvirt.Domain;
import org.libvirt.DomainBlockStats;
import org.libvirt.DomainInfo;
import org.libvirt.MemoryStatistic;
import org.libvirt.DomainInfo.DomainState;
import org.libvirt.DomainInterfaceStats;
import org.libvirt.LibvirtException;
import org.libvirt.MemoryStatistic;
import org.libvirt.NodeInfo;

import com.cloud.agent.api.Answer;
Expand Down Expand Up @@ -189,7 +191,6 @@ public class LibvirtComputingResource extends ServerResourceBase implements Serv
private String _clusterId;

private long _hvVersion;
private long _kernelVersion;
private int _timeout;
private static final int NUMMEMSTATS =2;

Expand Down Expand Up @@ -958,13 +959,6 @@ public boolean configure(final String name, final Map<String, Object> params) th
storageProcessor.configure(name, params);
storageHandler = new StorageSubsystemCommandHandlerBase(storageProcessor);

final String unameKernelVersion = Script.runSimpleBashScript("uname -r");
final String[] kernelVersions = unameKernelVersion.split("[\\.\\-]");
_kernelVersion = Integer.parseInt(kernelVersions[0]) * 1000 * 1000 + (long)Integer.parseInt(kernelVersions[1]) * 1000 + Integer.parseInt(kernelVersions[2]);

/* Disable this, the code using this is pretty bad and non portable
* getOsVersion();
*/
return true;
}

Expand Down Expand Up @@ -3020,27 +3014,26 @@ private class VmStats {
long _ioWrote;
long _bytesRead;
long _bytesWrote;
long _intmemfree;
long _memory;
long _maxmemory;
Calendar _timestamp;
}

public VmStatsEntry getVmStat(final Connect conn, final String vmName) throws LibvirtException {
Domain dm = null;
try {
dm = getDomain(conn, vmName);
final DomainInfo info = dm.getInfo();
final MemoryStatistic[] mems = dm.memoryStats(NUMMEMSTATS); //number of memory statistics required.

if (dm == null) {
return null;
}
DomainInfo info = dm.getInfo();
final VmStatsEntry stats = new VmStatsEntry();

stats.setNumCPUs(info.nrVirtCpu);
stats.setEntityType("vm");

stats.setMemoryKBs(info.maxMem);
stats.setTargetMemoryKBs(info.memory);
stats.setIntFreeMemoryKBs((double) mems[0].getValue());
stats.setIntFreeMemoryKBs(getMemoryFreeInKBs(dm));

/* get cpu utilization */
VmStats oldStats = null;

Expand Down Expand Up @@ -3125,9 +3118,6 @@ public VmStatsEntry getVmStat(final Connect conn, final String vmName) throws Li
newStat._bytesRead = bytes_rd;
newStat._bytesWrote = bytes_wr;
newStat._timestamp = now;
newStat._intmemfree = mems[0].getValue();
newStat._memory = info.memory;
newStat._maxmemory = info.maxMem;
_vmStats.put(vmName, newStat);
return stats;
} finally {
Expand All @@ -3137,6 +3127,21 @@ public VmStatsEntry getVmStat(final Connect conn, final String vmName) throws Li
}
}

/**
* This method retrieves the memory statistics from the domain given as parameters.
* If no memory statistic is found, it will return {@link NumberUtils#LONG_ZERO} as the value of free memory in the domain.
* If it can retrieve the domain memory statistics, it will return the free memory statistic; that means, it returns the value at the first position of the array returned by {@link Domain#memoryStats(int)}.
*
* @return the amount of free memory in KBs
*/
protected long getMemoryFreeInKBs(Domain dm) throws LibvirtException {
MemoryStatistic[] mems = dm.memoryStats(NUMMEMSTATS);
if (ArrayUtils.isEmpty(mems)) {
return NumberUtils.LONG_ZERO;
}
return mems[0].getValue();
}

private boolean canBridgeFirewall(final String prvNic) {
final Script cmd = new Script(_securityGroupPath, _timeout, s_logger);
cmd.add("can_bridge_firewall");
Expand Down
69 changes: 51 additions & 18 deletions .../hypervisors/kvm/test/com/cloud/hypervisor/kvm/resource/LibvirtComputingResourceTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,10 +65,10 @@
import org.libvirt.DomainInfo.DomainState;
import org.libvirt.DomainInterfaceStats;
import org.libvirt.LibvirtException;
import org.libvirt.MemoryStatistic;
import org.libvirt.NodeInfo;
import org.libvirt.StorageVol;
import org.libvirt.MemoryStatistic;

import org.libvirt.jna.virDomainMemoryStats;
import org.mockito.Matchers;
import org.mockito.Mock;
import org.mockito.Mockito;
Expand Down Expand Up @@ -182,8 +182,8 @@ public class LibvirtComputingResourceTest {
@Mock
private LibvirtComputingResource libvirtComputingResource;

String _hyperVisorType = "kvm";
Random _random = new Random();
String hyperVisorType = "kvm";
Random random = new Random();

/**
This test tests if the Agent can handle a vmSpec coming
Expand All @@ -194,10 +194,10 @@ public class LibvirtComputingResourceTest {
*/
@Test
public void testCreateVMFromSpecLegacy() {
final int id = _random.nextInt(65534);
final int id = random.nextInt(65534);
final String name = "test-instance-1";

final int cpus = _random.nextInt(2) + 1;
final int cpus = random.nextInt(2) + 1;
final int speed = 1024;
final int minRam = 256 * 1024;
final int maxRam = 512 * 1024;
Expand All @@ -213,7 +213,7 @@ public void testCreateVMFromSpecLegacy() {
to.setUuid("b0f0a72d-7efb-3cad-a8ff-70ebf30b3af9");

final LibvirtVMDef vm = lcr.createVMFromSpec(to);
vm.setHvsType(_hyperVisorType);
vm.setHvsType(hyperVisorType);

verifyVm(to, vm);
}
Expand All @@ -223,7 +223,7 @@ public void testCreateVMFromSpecLegacy() {
*/
@Test
public void testCreateVMFromSpecWithTopology6() {
final int id = _random.nextInt(65534);
final int id = random.nextInt(65534);
final String name = "test-instance-1";

final int cpus = 12;
Expand All @@ -243,7 +243,7 @@ public void testCreateVMFromSpecWithTopology6() {
to.setUuid("b0f0a72d-7efb-3cad-a8ff-70ebf30b3af9");

final LibvirtVMDef vm = lcr.createVMFromSpec(to);
vm.setHvsType(_hyperVisorType);
vm.setHvsType(hyperVisorType);

verifyVm(to, vm);
}
Expand All @@ -253,7 +253,7 @@ public void testCreateVMFromSpecWithTopology6() {
*/
@Test
public void testCreateVMFromSpecWithTopology4() {
final int id = _random.nextInt(65534);
final int id = random.nextInt(65534);
final String name = "test-instance-1";

final int cpus = 8;
Expand All @@ -273,7 +273,7 @@ public void testCreateVMFromSpecWithTopology4() {
to.setUuid("b0f0a72d-7efb-3cad-a8ff-70ebf30b3af9");

final LibvirtVMDef vm = lcr.createVMFromSpec(to);
vm.setHvsType(_hyperVisorType);
vm.setHvsType(hyperVisorType);

verifyVm(to, vm);
}
Expand All @@ -287,10 +287,10 @@ public void testCreateVMFromSpecWithTopology4() {
*/
@Test
public void testCreateVMFromSpec() {
final int id = _random.nextInt(65534);
final int id = random.nextInt(65534);
final String name = "test-instance-1";

final int cpus = _random.nextInt(2) + 1;
final int cpus = random.nextInt(2) + 1;
final int minSpeed = 1024;
final int maxSpeed = 2048;
final int minRam = 256 * 1024;
Expand All @@ -308,7 +308,7 @@ public void testCreateVMFromSpec() {
to.setUuid("b0f0a72d-7efb-3cad-a8ff-70ebf30b3af9");

final LibvirtVMDef vm = lcr.createVMFromSpec(to);
vm.setHvsType(_hyperVisorType);
vm.setHvsType(hyperVisorType);

verifyVm(to, vm);
}
Expand Down Expand Up @@ -718,10 +718,9 @@ public void testGetVmDiskStatsCommand() {
}
}

@SuppressWarnings("unchecked")
@Test
@SuppressWarnings("unchecked")
public void testGetVmDiskStatsCommandException() {
Mockito.mock(Connect.class);
final LibvirtUtilitiesHelper libvirtUtilitiesHelper = Mockito.mock(LibvirtUtilitiesHelper.class);

final String vmName = "Test";
Expand Down Expand Up @@ -941,7 +940,6 @@ public void testRebootRouterCommandConnect() {
public void testGetHostStatsCommand() {
// A bit difficult to test due to the logger being passed and the parser itself relying on the connection.
// Have to spend some more time afterwards in order to refactor the wrapper itself.
Mockito.mock(LibvirtUtilitiesHelper.class);
final CPUStat cpuStat = Mockito.mock(CPUStat.class);
final MemStat memStat = Mockito.mock(MemStat.class);

Expand Down Expand Up @@ -3531,7 +3529,6 @@ public void testNetworkUsageCommandVpcNoOption() {
verify(libvirtComputingResource, times(1)).configureVPCNetworkUsage(command.getPrivateIP(), command.getGatewayIP(), command.getOption(), command.getVpcCIDR());
}

@SuppressWarnings("unchecked")
@Test
public void testCreatePrivateTemplateFromVolumeCommand() {
//Simple test used to make sure the flow (LibvirtComputingResource => Request => CommandWrapper) is working.
Expand Down Expand Up @@ -5040,4 +5037,40 @@ public void testIsInterface () {
}
}
}

@Test
public void testMemoryFreeInKBsDomainReturningOfSomeMemoryStatistics() throws LibvirtException {
LibvirtComputingResource libvirtComputingResource = new LibvirtComputingResource();

MemoryStatistic[] mem = createMemoryStatisticFreeMemory100();
Domain domainMock = getDomainConfiguredToReturnMemoryStatistic(mem);
long memoryFreeInKBs = libvirtComputingResource.getMemoryFreeInKBs(domainMock);

Assert.assertEquals(100, memoryFreeInKBs);
}

@Test
public void testMemoryFreeInKBsDomainReturningNoMemoryStatistics() throws LibvirtException {
LibvirtComputingResource libvirtComputingResource = new LibvirtComputingResource();

Domain domainMock = getDomainConfiguredToReturnMemoryStatistic(null);
long memoryFreeInKBs = libvirtComputingResource.getMemoryFreeInKBs(domainMock);

Assert.assertEquals(0, memoryFreeInKBs);
}

private MemoryStatistic[] createMemoryStatisticFreeMemory100() {
virDomainMemoryStats stat = new virDomainMemoryStats();
stat.val = 100;

MemoryStatistic[] mem = new MemoryStatistic[2];
mem[0] = new MemoryStatistic(stat);
return mem;
}

private Domain getDomainConfiguredToReturnMemoryStatistic(MemoryStatistic[] mem) throws LibvirtException {
Domain domainMock = Mockito.mock(Domain.class);
when(domainMock.memoryStats(2)).thenReturn(mem);
return domainMock;
}
}
16 changes: 9 additions & 7 deletions ...hypervisors/xenserver/src/com/cloud/hypervisor/xenserver/resource/CitrixResourceBase.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,8 @@ public String toString() {
}
}

private final static int BASE_TO_CONVERT_BYTES_INTO_KILOBYTES = 1024;

protected static final XenServerConnectionPool ConnPool = XenServerConnectionPool.getInstance();
// static min values for guests on xenserver
private static final long mem_128m = 134217728L;
Expand Down Expand Up @@ -3323,19 +3325,19 @@ public HashMap<String, VmStatsEntry> getVmStats(final Connection conn, final Get
vmStatsAnswer.setNumCPUs(vmStatsAnswer.getNumCPUs() + 1);
vmStatsAnswer.setCPUUtilization(vmStatsAnswer.getCPUUtilization() + getDataAverage(dataNode, col, numRows));
} else if (param.matches("vif_\\d*_rx")) {
vmStatsAnswer.setNetworkReadKBs(vmStatsAnswer.getNetworkReadKBs() + getDataAverage(dataNode, col, numRows) / 1000);
vmStatsAnswer.setNetworkReadKBs(vmStatsAnswer.getNetworkReadKBs() + getDataAverage(dataNode, col, numRows) / BASE_TO_CONVERT_BYTES_INTO_KILOBYTES);
} else if (param.matches("vif_\\d*_tx")) {
vmStatsAnswer.setNetworkWriteKBs(vmStatsAnswer.getNetworkWriteKBs() + getDataAverage(dataNode, col, numRows) / 1000);
vmStatsAnswer.setNetworkWriteKBs(vmStatsAnswer.getNetworkWriteKBs() + getDataAverage(dataNode, col, numRows) / BASE_TO_CONVERT_BYTES_INTO_KILOBYTES);
} else if (param.matches("vbd_.*_read")) {
vmStatsAnswer.setDiskReadKBs(vmStatsAnswer.getDiskReadKBs() + getDataAverage(dataNode, col, numRows) / 1000);
vmStatsAnswer.setDiskReadKBs(vmStatsAnswer.getDiskReadKBs() + getDataAverage(dataNode, col, numRows) / BASE_TO_CONVERT_BYTES_INTO_KILOBYTES);
} else if (param.matches("vbd_.*_write")) {
vmStatsAnswer.setDiskWriteKBs(vmStatsAnswer.getDiskWriteKBs() + getDataAverage(dataNode, col, numRows) / 1000);
vmStatsAnswer.setDiskWriteKBs(vmStatsAnswer.getDiskWriteKBs() + getDataAverage(dataNode, col, numRows) / BASE_TO_CONVERT_BYTES_INTO_KILOBYTES);
} else if (param.contains("memory_internal_free")) {
vmStatsAnswer.setIntFreeMemoryKBs(vmStatsAnswer.getIntFreeMemoryKBs() + getDataAverage(dataNode, col, numRows) / 1024);
vmStatsAnswer.setIntFreeMemoryKBs(vmStatsAnswer.getIntFreeMemoryKBs() + getDataAverage(dataNode, col, numRows) / BASE_TO_CONVERT_BYTES_INTO_KILOBYTES);
} else if (param.contains("memory_target")) {
vmStatsAnswer.setTargetMemoryKBs(vmStatsAnswer.getTargetMemoryKBs() + getDataAverage(dataNode, col, numRows) / 1024);
vmStatsAnswer.setTargetMemoryKBs(vmStatsAnswer.getTargetMemoryKBs() + getDataAverage(dataNode, col, numRows) / BASE_TO_CONVERT_BYTES_INTO_KILOBYTES);
} else if (param.contains("memory")) {
vmStatsAnswer.setMemoryKBs(vmStatsAnswer.getMemoryKBs() + getDataAverage(dataNode, col, numRows) / 1024);
vmStatsAnswer.setMemoryKBs(vmStatsAnswer.getMemoryKBs() + getDataAverage(dataNode, col, numRows) / BASE_TO_CONVERT_BYTES_INTO_KILOBYTES);
}

}
Expand Down

0 comments on commit 866ac7b

Please sign in to comment.