[GitHub] [cloudstack] GabrielBrascher commented on a change in pull request #3186: Add possibility to set KVM MTU size for all NIC
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
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
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
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
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
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
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
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