[GitHub] [cloudstack] GabrielBrascher commented on a change in pull request #3186: Add possibility to set KVM MTU size for all NIC

2019-11-14 Thread GitBox 
GabrielBrascher commented on a change in pull request #3186: Add possibility to 
set KVM MTU size for all NIC
URL: https://github.com/apache/cloudstack/pull/3186#discussion_r346473236
 
 

 ##
 File path: 
engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java
 ##
 @@ -247,6 +262,10 @@
 @Inject
 ConfigurationManager _configMgr;
 @Inject
+ClusterDao _clusterDao;
+@Inject
+ClusterDetailsDao _clusterDetailsDao;
 
 Review comment:
   I really don't want to sound picky or negative here, just adding a few minor 
comments :-)
   - those variables could be set to private;
   - additionally, could you please remove the underscore ('_') at the 
beginning of variables names?
   
   I know that we see this a lot around the CloudStack codebase but this is not 
really the best approach. While it's technically legal to begin your variable's 
name with "_" or "$", this practice is discouraged by the Java variable naming 
convention.


This is an automated message from the Apache Git Service.
To respond to the message, please log on to 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] [cloudstack] GabrielBrascher commented on a change in pull request #3186: Add possibility to set KVM MTU size for all NIC

2019-11-14 Thread GitBox 
GabrielBrascher commented on a change in pull request #3186: Add possibility to 
set KVM MTU size for all NIC
URL: https://github.com/apache/cloudstack/pull/3186#discussion_r346597181
 
 

 ##
 File path: 
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java
 ##
 @@ -1019,21 +1021,26 @@ public String toString() {
 private String _dpdkExtraLines;
 private String _interfaceMode;
 
-public void defBridgeNet(String brName, String targetBrName, String 
macAddr, NicModel model) {
-defBridgeNet(brName, targetBrName, macAddr, model, 0);
+/**
+ * Bridge Net
 
 Review comment:
   Javadoc is always very welcome, I am always in favor; however, this one is 
not aggregating too much info. Could you please provide a bit more details? 
Thanks!


This is an automated message from the Apache Git Service.
To respond to the message, please log on to 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] [cloudstack] GabrielBrascher commented on a change in pull request #3186: Add possibility to set KVM MTU size for all NIC

2019-11-14 Thread GitBox 
GabrielBrascher commented on a change in pull request #3186: Add possibility to 
set KVM MTU size for all NIC
URL: https://github.com/apache/cloudstack/pull/3186#discussion_r346622474
 
 

 ##
 File path: 
engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java
 ##
 @@ -578,7 +583,17 @@ public void doInTransactionWithoutResult(final 
TransactionStatus status) {
 
 _networkOfferingDao.persistDefaultL2NetworkOfferings();
 
-_kvmMtuSize = 
NumbersUtil.parseInt(_configDao.getValue(KvmMtuSize.key()), 0);
+final List clusterVOs = 
_clusterDao.listByHypervisorAndCluster(HypervisorType.KVM, 
Cluster.ClusterType.CloudManaged);
+
+if ((clusterVOs != null) && !clusterVOs.isEmpty()) {
 
 Review comment:
   Can you please update this if conditional to `if 
(!CollectionUtils.isEmpty(clusterVOs)) { ... }`?
   The "isEmpty" is a Null-safe check; thus, it is a great option for checking 
if a collection is empty (returning true if null or empty).


This is an automated message from the Apache Git Service.
To respond to the message, please log on to 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] [cloudstack] GabrielBrascher commented on a change in pull request #3186: Add possibility to set KVM MTU size for all NIC

2019-11-14 Thread GitBox 
GabrielBrascher commented on a change in pull request #3186: Add possibility to 
set KVM MTU size for all NIC
URL: https://github.com/apache/cloudstack/pull/3186#discussion_r346624532
 
 

 ##
 File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
 ##
 @@ -1054,7 +1061,7 @@ public void validateCustomParameters(ServiceOfferingVO 
serviceOffering, Map

[GitHub] [cloudstack] GabrielBrascher commented on a change in pull request #3186: Add possibility to set KVM MTU size for all NIC

2019-11-14 Thread GitBox 
GabrielBrascher commented on a change in pull request #3186: Add possibility to 
set KVM MTU size for all NIC
URL: https://github.com/apache/cloudstack/pull/3186#discussion_r346625196
 
 

 ##
 File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
 ##
 @@ -2083,31 +2090,41 @@ public boolean configure(String name, Map params) throws Configu
 
 Map configs = 
_configDao.getConfiguration("AgentManager", params);
 
-_kvmMtuSize = NumbersUtil.parseInt(_configDao.getValue(KVM_MTU_KEY), 
0);
+final List clusterVOs = 
_clusterDao.listByHypervisorAndCluster(HypervisorType.KVM, 
Cluster.ClusterType.CloudManaged);
+
+if ((clusterVOs != null) && !clusterVOs.isEmpty()) {
 
 Review comment:
   CollectionUtils.isEmpty(clusterVOs) would be nice here 
:slightly_smiling_face:  :+1: 


This is an automated message from the Apache Git Service.
To respond to the message, please log on to 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] [cloudstack] GabrielBrascher commented on a change in pull request #3186: Add possibility to set KVM MTU size for all NIC

2019-11-14 Thread GitBox 
GabrielBrascher commented on a change in pull request #3186: Add possibility to 
set KVM MTU size for all NIC
URL: https://github.com/apache/cloudstack/pull/3186#discussion_r346473824
 
 

 ##
 File path: api/src/main/java/com/cloud/vm/NicProfile.java
 ##
 @@ -49,6 +48,7 @@
 URI broadcastUri;
 ReservationStrategy strategy;
 boolean defaultNic;
+Integer mtu;
 
 Review comment:
   If I am not mistaken, this variable can be set to private. Could you please 
set this to private?


This is an automated message from the Apache Git Service.
To respond to the message, please log on to 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] [cloudstack] GabrielBrascher commented on a change in pull request #3186: Add possibility to set KVM MTU size for all NIC

2019-11-14 Thread GitBox 
GabrielBrascher commented on a change in pull request #3186: Add possibility to 
set KVM MTU size for all NIC
URL: https://github.com/apache/cloudstack/pull/3186#discussion_r346597181
 
 

 ##
 File path: 
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java
 ##
 @@ -1019,21 +1021,26 @@ public String toString() {
 private String _dpdkExtraLines;
 private String _interfaceMode;
 
-public void defBridgeNet(String brName, String targetBrName, String 
macAddr, NicModel model) {
-defBridgeNet(brName, targetBrName, macAddr, model, 0);
+/**
+ * Bridge Net
 
 Review comment:
   Javadoc is always very welcome; however, this one is not aggregating too 
much info. Could you please provide a bit more details? Thanks!


This is an automated message from the Apache Git Service.
To respond to the message, please log on to 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] [cloudstack] GabrielBrascher commented on a change in pull request #3186: Add possibility to set KVM MTU size for all NIC

2019-11-14 Thread GitBox 
GabrielBrascher commented on a change in pull request #3186: Add possibility to 
set KVM MTU size for all NIC
URL: https://github.com/apache/cloudstack/pull/3186#discussion_r346628250
 
 

 ##
 File path: 
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java
 ##
 @@ -1019,21 +1021,26 @@ public String toString() {
 private String _dpdkExtraLines;
 private String _interfaceMode;
 
-public void defBridgeNet(String brName, String targetBrName, String 
macAddr, NicModel model) {
-defBridgeNet(brName, targetBrName, macAddr, model, 0);
+/**
+ * Bridge Net
+ */
+public void defBridgeNet(String brName, String targetBrName, String 
macAddr, NicModel model, Integer mtu) {
+defBridgeNet(brName, targetBrName, macAddr, model, 0, mtu);
 }
 
-public void defBridgeNet(String brName, String targetBrName, String 
macAddr, NicModel model, Integer networkRateKBps) {
+public void defBridgeNet(String brName, String targetBrName, String 
macAddr, NicModel model, Integer networkRateKBps, Integer mtu) {
 _netType = GuestNetType.BRIDGE;
 _sourceName = brName;
 _networkName = targetBrName;
 _macAddr = macAddr;
 _model = model;
 _networkRateKBps = networkRateKBps;
+_mtu = mtu;
 }
-
-public void defDpdkNet(String dpdkSourcePath, String dpdkPort, String 
macAddress, NicModel model,
-   Integer networkRateKBps, String extra, String 
interfaceMode) {
+   /**
+   * Dpdk Net
 
 Review comment:
   Same here, a bit more details would be nice. Otherwise, I think that the 
Javadoc block could be removed.
   PS: thumbs up for the Javadoc initiative it is always a plus :+1:


This is an automated message from the Apache Git Service.
To respond to the message, please log on to 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