[GitHub] [cloudstack] nvazquez commented on a change in pull request #5181: vmware, ui: update portgroup on network offering change

[GitBox <gi...@apache.org>]

nvazquez commented on a change in pull request #5181:
URL: https://github.com/apache/cloudstack/pull/5181#discussion_r683465834



##########
File path: vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/HypervisorHostHelper.java
##########
@@ -956,6 +952,11 @@ public static boolean isSpecMatch(DVPortgroupConfigInfo currentDvPortgroupInfo,
                 specMatches = false;
             }
         }
+        if (currentPortSetting.getMacManagementPolicy() != null && newPortSetting.getMacManagementPolicy() != null &&
+                currentPortSetting.getMacManagementPolicy().getMacLearningPolicy() != null && newPortSetting.getMacManagementPolicy().getMacLearningPolicy() != null &&
+                currentPortSetting.getMacManagementPolicy().getMacLearningPolicy().isEnabled() != newPortSetting.getMacManagementPolicy().getMacLearningPolicy().isEnabled()) {

Review comment:
       Can you extract this big `if` clause into a method to improve readability? Then this could be like:
   ````
   if (hasMacLearningPolicyChanged(currentPortSetting, newPortSetting) {
      specMatches = false;
   }
   ```` 

##########
File path: server/src/main/java/com/cloud/network/NetworkServiceImpl.java
##########
@@ -2144,6 +2152,38 @@ private boolean checkForNonStoppedVmInNetwork(long networkId) {
         return vms.isEmpty();
     }
 
+    private void replugNicsForUpdatedNetwork(NetworkVO network) throws ResourceUnavailableException, InsufficientCapacityException {
+        List<NicVO> nics = _nicDao.listByNetworkId(network.getId());
+        Network updatedNetwork = getNetwork(network.getId());
+        for (NicVO nic : nics) {
+            long vmId = nic.getInstanceId();
+            VMInstanceVO vm = _vmDao.findById(vmId);
+            if (vm == null) {
+                s_logger.error("VM for nic " + nic.getId() + " not found with Vm Id:" + vmId);
+                continue;
+            }
+            if (!Hypervisor.HypervisorType.VMware.equals(vm.getHypervisorType())) {
+                s_logger.debug("VM is not on VMware");

Review comment:
       Maybe add more information to the following log lines? Something like: `Cannot replug nic <ID> as: VM ...`

##########
File path: vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/HypervisorHostHelper.java
##########
@@ -1215,6 +1218,35 @@ public static DVSSecurityPolicy createDVSSecurityPolicy(Map<NetworkOffering.Deta
         return secPolicy;
     }
 
+    public static DVSMacManagementPolicy createDVSMacManagementPolicy(Map<NetworkOffering.Detail, String> nicDetails) {
+        DVSMacManagementPolicy macManagementPolicy = new DVSMacManagementPolicy();
+        macManagementPolicy.setAllowPromiscuous(false);

Review comment:
       Minor improvement to reduce lines of code:
   ````
   macManagementPolicy.setAllowPromiscuous(
      nicDetails.containsKey(NetworkOffering.Detail.PromiscuousMode) ? 
      Boolean.valueOf(nicDetails.get(NetworkOffering.Detail.PromiscuousMode)) : false);
   ````
   Same for the other properties. Then you won't need the if blocks below.




-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org