[GitHub] rafaelweingartner commented on a change in pull request #2573: Cloudstack 10356
rafaelweingartner commented on a change in pull request #2573: Cloudstack 10356
URL: https://github.com/apache/cloudstack/pull/2573#discussion_r182091684
##
File path:
server/src/main/java/com/cloud/network/router/VpcVirtualNetworkApplianceManagerImpl.java
##
@@ -740,14 +740,14 @@ public boolean startRemoteAccessVpn(final
RemoteAccessVpn vpn, final VirtualRout
throw new AgentUnavailableException("Unable to send commands to
virtual router ", router.getHostId(), e);
}
Answer answer = cmds.getAnswer("users");
-if (!answer.getResult()) {
+if (answer != null && !answer.getResult()) {
Review comment:
Sorry, I did not understand your question. Are you asking if I would suggest
to use BooleanUtils here as well? Please, go ahead and do that.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rafaelweingartner commented on a change in pull request #2573: Cloudstack 10356
rafaelweingartner commented on a change in pull request #2573: Cloudstack 10356
URL: https://github.com/apache/cloudstack/pull/2573#discussion_r181790641
##
File path:
engine/schema/src/main/java/com/cloud/configuration/dao/ResourceCountDaoImpl.java
##
@@ -126,8 +126,10 @@ public void updateDomainCount(long domainId, ResourceType
type, boolean incremen
delta = increment ? delta : delta * -1;
ResourceCountVO resourceCountVO = findByOwnerAndType(domainId,
ResourceOwnerType.Domain, type);
-resourceCountVO.setCount(resourceCountVO.getCount() + delta);
-update(resourceCountVO.getId(), resourceCountVO);
+if (resourceCountVO != null) {
Review comment:
Yes. If it is not used. So, it is better to delete it.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rafaelweingartner commented on a change in pull request #2573: Cloudstack 10356
rafaelweingartner commented on a change in pull request #2573: Cloudstack 10356
URL: https://github.com/apache/cloudstack/pull/2573#discussion_r181789714
##
File path:
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java
##
@@ -2337,8 +2337,9 @@ public int compare(final DiskTO arg0, final DiskTO arg1)
{
disk.setCacheMode(DiskDef.DiskCacheMode.valueOf(volumeObjectTO.getCacheMode().toString().toUpperCase()));
}
}
-
-vm.getDevices().addDevice(disk);
+if (vm.getDevices() != null) {
Review comment:
Actually, I am not talking about the variable `vm`. I am instead, referring
to the return of `vm.getDevices()` method.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rafaelweingartner commented on a change in pull request #2573: Cloudstack 10356
rafaelweingartner commented on a change in pull request #2573: Cloudstack 10356
URL: https://github.com/apache/cloudstack/pull/2573#discussion_r181783355
##
File path:
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java
##
@@ -2337,8 +2337,9 @@ public int compare(final DiskTO arg0, final DiskTO arg1)
{
disk.setCacheMode(DiskDef.DiskCacheMode.valueOf(volumeObjectTO.getCacheMode().toString().toUpperCase()));
}
}
-
-vm.getDevices().addDevice(disk);
+if (vm.getDevices() != null) {
Review comment:
I only mentioned an exception for a single reason. Right now the code works;
and, if we ever get a null calling `vm.getDevices() `, a run time exception
will be thrown, and we will be able to easily pin point the problem. However,
after applying your code, for sure something else will break or not work as
expected, which will make more difficult for us to pin point the problem later
(if it ever happens).
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rafaelweingartner commented on a change in pull request #2573: Cloudstack 10356
rafaelweingartner commented on a change in pull request #2573: Cloudstack 10356
URL: https://github.com/apache/cloudstack/pull/2573#discussion_r181777342
##
File path:
engine/schema/src/main/java/com/cloud/configuration/dao/ResourceCountDaoImpl.java
##
@@ -126,8 +126,10 @@ public void updateDomainCount(long domainId, ResourceType
type, boolean incremen
delta = increment ? delta : delta * -1;
ResourceCountVO resourceCountVO = findByOwnerAndType(domainId,
ResourceOwnerType.Domain, type);
-resourceCountVO.setCount(resourceCountVO.getCount() + delta);
-update(resourceCountVO.getId(), resourceCountVO);
+if (resourceCountVO != null) {
Review comment:
Then, what about if you delete it?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rafaelweingartner commented on a change in pull request #2573: Cloudstack 10356
rafaelweingartner commented on a change in pull request #2573: Cloudstack 10356
URL: https://github.com/apache/cloudstack/pull/2573#discussion_r181776039
##
File path:
engine/schema/src/main/java/com/cloud/configuration/dao/ResourceCountDaoImpl.java
##
@@ -126,8 +126,10 @@ public void updateDomainCount(long domainId, ResourceType
type, boolean incremen
delta = increment ? delta : delta * -1;
ResourceCountVO resourceCountVO = findByOwnerAndType(domainId,
ResourceOwnerType.Domain, type);
-resourceCountVO.setCount(resourceCountVO.getCount() + delta);
-update(resourceCountVO.getId(), resourceCountVO);
+if (resourceCountVO != null) {
Review comment:
I did not check the code. Is it used somewhere else?
I think we can ignore (it has been working so far...).
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rafaelweingartner commented on a change in pull request #2573: Cloudstack 10356
rafaelweingartner commented on a change in pull request #2573: Cloudstack 10356
URL: https://github.com/apache/cloudstack/pull/2573#discussion_r181775760
##
File path:
engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java
##
@@ -499,6 +499,9 @@ public VolumeInfo copyVolumeFromSecToPrimary(VolumeInfo
volume, VirtualMachine v
// Find a suitable storage to create volume on
StoragePool destPool = findStoragePool(dskCh, dc, pod, clusterId,
null, vm, avoidPools);
+if (destPool == null) {
+throw new CloudRuntimeException("Failed to find a storage pool
with enough capacity to copy the volume to.");
Review comment:
I think this one that you suggested is better.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rafaelweingartner commented on a change in pull request #2573: Cloudstack 10356
rafaelweingartner commented on a change in pull request #2573: Cloudstack 10356
URL: https://github.com/apache/cloudstack/pull/2573#discussion_r181747454
##
File path:
engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java
##
@@ -499,6 +499,9 @@ public VolumeInfo copyVolumeFromSecToPrimary(VolumeInfo
volume, VirtualMachine v
// Find a suitable storage to create volume on
StoragePool destPool = findStoragePool(dskCh, dc, pod, clusterId,
null, vm, avoidPools);
+if (destPool == null) {
+throw new CloudRuntimeException("Failed to find a storage pool
with enough capacity to copy the volume to.");
Review comment:
This message looks misleading. Will tags be considered at this point as well?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rafaelweingartner commented on a change in pull request #2573: Cloudstack 10356
rafaelweingartner commented on a change in pull request #2573: Cloudstack 10356
URL: https://github.com/apache/cloudstack/pull/2573#discussion_r181748734
##
File path:
server/src/main/java/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java
##
@@ -2139,14 +2139,14 @@ public boolean startRemoteAccessVpn(final Network
network, final RemoteAccessVpn
}
Answer answer = cmds.getAnswer("users");
-if (!answer.getResult()) {
+if (answer != null && !answer.getResult()) {
Review comment:
You can use `org.apache.commons.lang3.BooleanUtils.toBooleanObject(String,
String, String, String)` here
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rafaelweingartner commented on a change in pull request #2573: Cloudstack 10356
rafaelweingartner commented on a change in pull request #2573: Cloudstack 10356
URL: https://github.com/apache/cloudstack/pull/2573#discussion_r181748253
##
File path:
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java
##
@@ -2337,8 +2337,9 @@ public int compare(final DiskTO arg0, final DiskTO arg1)
{
disk.setCacheMode(DiskDef.DiskCacheMode.valueOf(volumeObjectTO.getCacheMode().toString().toUpperCase()));
}
}
-
-vm.getDevices().addDevice(disk);
+if (vm.getDevices() != null) {
Review comment:
Should not an exception be thrown in this case?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rafaelweingartner commented on a change in pull request #2573: Cloudstack 10356
rafaelweingartner commented on a change in pull request #2573: Cloudstack 10356
URL: https://github.com/apache/cloudstack/pull/2573#discussion_r181748304
##
File path:
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java
##
@@ -2391,8 +2392,10 @@ private void createVif(final LibvirtVMDef vm, final
NicTO nic, final String nicA
+ ") is " + nic.getType() + " traffic type. So,
vsp-vr-ip " + vrIp + " is set in the metadata");
}
}
-
-vm.getDevices().addDevice(getVifDriver(nic.getType(),
nic.getName()).plug(nic, vm.getPlatformEmulator(), nicAdapter));
+
+if (vm.getDevices() != null) {
Review comment:
The same here?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rafaelweingartner commented on a change in pull request #2573: Cloudstack 10356
rafaelweingartner commented on a change in pull request #2573: Cloudstack 10356
URL: https://github.com/apache/cloudstack/pull/2573#discussion_r181748782
##
File path:
server/src/main/java/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java
##
@@ -2139,14 +2139,14 @@ public boolean startRemoteAccessVpn(final Network
network, final RemoteAccessVpn
}
Answer answer = cmds.getAnswer("users");
-if (!answer.getResult()) {
+if (answer != null && !answer.getResult()) {
s_logger.error("Unable to start vpn: unable add users to vpn
in zone " + router.getDataCenterId() + " for account " + vpn.getAccountId() + "
on domR: "
+ router.getInstanceName() + " due to " +
answer.getDetails());
throw new ResourceUnavailableException("Unable to start vpn:
Unable to add users to vpn in zone " + router.getDataCenterId() + " for account
"
+ vpn.getAccountId() + " on domR: " +
router.getInstanceName() + " due to " + answer.getDetails(), DataCenter.class,
router.getDataCenterId());
}
answer = cmds.getAnswer("startVpn");
-if (!answer.getResult()) {
+if (answer != null && !answer.getResult()) {
Review comment:
Same here
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rafaelweingartner commented on a change in pull request #2573: Cloudstack 10356
rafaelweingartner commented on a change in pull request #2573: Cloudstack 10356
URL: https://github.com/apache/cloudstack/pull/2573#discussion_r181748841
##
File path:
server/src/main/java/com/cloud/network/router/VpcVirtualNetworkApplianceManagerImpl.java
##
@@ -740,14 +740,14 @@ public boolean startRemoteAccessVpn(final
RemoteAccessVpn vpn, final VirtualRout
throw new AgentUnavailableException("Unable to send commands to
virtual router ", router.getHostId(), e);
}
Answer answer = cmds.getAnswer("users");
-if (!answer.getResult()) {
+if (answer != null && !answer.getResult()) {
Review comment:
Here as well
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rafaelweingartner commented on a change in pull request #2573: Cloudstack 10356
rafaelweingartner commented on a change in pull request #2573: Cloudstack 10356
URL: https://github.com/apache/cloudstack/pull/2573#discussion_r181748053
##
File path:
plugins/deployment-planners/implicit-dedication/src/main/java/com/cloud/deploy/ImplicitDedicationPlanner.java
##
@@ -256,14 +256,15 @@ public PlannerResourceUsage
getResourceUsage(VirtualMachineProfile vmProfile, De
// Get the list of all the hosts in the given clusters
List allHosts = new ArrayList();
-for (Long cluster : clusterList) {
-List hostsInCluster =
resourceMgr.listAllHostsInCluster(cluster);
-for (HostVO hostVO : hostsInCluster) {
+if (clusterList != null && !clusterList.isEmpty()) {
Review comment:
You can use `CollectionUtils.isNotEmpty` here.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rafaelweingartner commented on a change in pull request #2573: Cloudstack 10356
rafaelweingartner commented on a change in pull request #2573: Cloudstack 10356
URL: https://github.com/apache/cloudstack/pull/2573#discussion_r181747775
##
File path:
engine/schema/src/main/java/com/cloud/configuration/dao/ResourceCountDaoImpl.java
##
@@ -126,8 +126,10 @@ public void updateDomainCount(long domainId, ResourceType
type, boolean incremen
delta = increment ? delta : delta * -1;
ResourceCountVO resourceCountVO = findByOwnerAndType(domainId,
ResourceOwnerType.Domain, type);
-resourceCountVO.setCount(resourceCountVO.getCount() + delta);
-update(resourceCountVO.getId(), resourceCountVO);
+if (resourceCountVO != null) {
Review comment:
If you do not find a resource to update, should not you break the execution
flow?
It seems that if the `resourceCountVO` is null, then we must have something
wrong in the database.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
