DaanHoogland commented on a change in pull request #5985:
URL: https://github.com/apache/cloudstack/pull/5985#discussion_r805800994
##########
File path:
plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java
##########
@@ -1622,6 +1589,72 @@ private ExecutionResult
prepareNetworkElementCommand(IpAssocCommand cmd) {
return new ExecutionResult(true, null);
}
+ private ExecutionResult cleanupNetworkElementCommand(IpAssocCommand cmd) {
+ VmwareContext context = getServiceContext();
+ try {
+ VmwareHypervisorHost hyperHost = getHyperHost(context);
+ IpAddressTO[] ips = cmd.getIpAddresses();
+ String routerName =
cmd.getAccessDetail(NetworkElementCommand.ROUTER_NAME);
+
+ VirtualMachineMO vmMo = hyperHost.findVmOnHyperHost(routerName);
+ // command may sometimes be redirect to a wrong host, we relax
+ // the check and will try to find it within cluster
+ if (vmMo == null) {
+ if (hyperHost instanceof HostMO) {
+ final DatacenterMO dcMo = new DatacenterMO(context,
hyperHost.getHyperHostDatacenter());
+ vmMo = dcMo.findVm(routerName);
+ }
+ }
+
+ if (vmMo == null) {
+ String msg = String.format("Router %s no longer exists to
execute IPAssoc command ", routerName);
+ s_logger.error(msg);
+ throw new Exception(msg);
+ }
Review comment:
can you factor out this bit?
##########
File path:
plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java
##########
@@ -1622,6 +1589,72 @@ private ExecutionResult
prepareNetworkElementCommand(IpAssocCommand cmd) {
return new ExecutionResult(true, null);
}
+ private ExecutionResult cleanupNetworkElementCommand(IpAssocCommand cmd) {
+ VmwareContext context = getServiceContext();
+ try {
+ VmwareHypervisorHost hyperHost = getHyperHost(context);
+ IpAddressTO[] ips = cmd.getIpAddresses();
+ String routerName =
cmd.getAccessDetail(NetworkElementCommand.ROUTER_NAME);
+
+ VirtualMachineMO vmMo = hyperHost.findVmOnHyperHost(routerName);
+ // command may sometimes be redirect to a wrong host, we relax
+ // the check and will try to find it within cluster
+ if (vmMo == null) {
+ if (hyperHost instanceof HostMO) {
+ final DatacenterMO dcMo = new DatacenterMO(context,
hyperHost.getHyperHostDatacenter());
+ vmMo = dcMo.findVm(routerName);
+ }
+ }
+
+ if (vmMo == null) {
+ String msg = String.format("Router %s no longer exists to
execute IPAssoc command ", routerName);
+ s_logger.error(msg);
+ throw new Exception(msg);
+ }
+
+ if (ips.length == 1 && !ips[0].isAdd()) {
+ IpAddressTO ip = ips[0];
+ NicTO nicTO = ip.getNicTO();
+ URI broadcastUri =
BroadcastDomainType.fromString(ip.getBroadcastUri());
+ if (BroadcastDomainType.getSchemeValue(broadcastUri) !=
BroadcastDomainType.Vlan) {
+ throw new InternalErrorException(String.format("Unable to
assign a public IP to a VIF on network %s", ip.getBroadcastUri()));
+ }
+ String vlanId = BroadcastDomainType.getValue(broadcastUri);
+
+ String publicNetworkName =
HypervisorHostHelper.getPublicNetworkNamePrefix(vlanId);
+ Pair<Integer, VirtualDevice> publicNicInfo =
vmMo.getNicDeviceIndex(publicNetworkName);
+
+ if (s_logger.isDebugEnabled()) {
+ s_logger.debug(String.format("Find public NIC index,
public network name: %s , index: %s", publicNetworkName,
publicNicInfo.first()));
+ }
+
+ VirtualDevice nic = findVirtualNicDevice(vmMo, nicTO.getMac());
+
+ if (nic == null) {
+ return new ExecutionResult(false, "Couldn't find NIC");
+ }
+ configureNicDevice(vmMo, nic,
VirtualDeviceConfigSpecOperation.REMOVE);
+ }
Review comment:
can you factor out this bit? (maybe if even smaller/nested chunks)
##########
File path:
plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java
##########
@@ -1622,6 +1589,72 @@ private ExecutionResult
prepareNetworkElementCommand(IpAssocCommand cmd) {
return new ExecutionResult(true, null);
}
+ private ExecutionResult cleanupNetworkElementCommand(IpAssocCommand cmd) {
+ VmwareContext context = getServiceContext();
+ try {
+ VmwareHypervisorHost hyperHost = getHyperHost(context);
+ IpAddressTO[] ips = cmd.getIpAddresses();
+ String routerName =
cmd.getAccessDetail(NetworkElementCommand.ROUTER_NAME);
+
+ VirtualMachineMO vmMo = hyperHost.findVmOnHyperHost(routerName);
+ // command may sometimes be redirect to a wrong host, we relax
+ // the check and will try to find it within cluster
+ if (vmMo == null) {
+ if (hyperHost instanceof HostMO) {
+ final DatacenterMO dcMo = new DatacenterMO(context,
hyperHost.getHyperHostDatacenter());
+ vmMo = dcMo.findVm(routerName);
+ }
+ }
+
+ if (vmMo == null) {
+ String msg = String.format("Router %s no longer exists to
execute IPAssoc command ", routerName);
+ s_logger.error(msg);
+ throw new Exception(msg);
+ }
+
+ if (ips.length == 1 && !ips[0].isAdd()) {
+ IpAddressTO ip = ips[0];
+ NicTO nicTO = ip.getNicTO();
+ URI broadcastUri =
BroadcastDomainType.fromString(ip.getBroadcastUri());
+ if (BroadcastDomainType.getSchemeValue(broadcastUri) !=
BroadcastDomainType.Vlan) {
+ throw new InternalErrorException(String.format("Unable to
assign a public IP to a VIF on network %s", ip.getBroadcastUri()));
+ }
+ String vlanId = BroadcastDomainType.getValue(broadcastUri);
+
+ String publicNetworkName =
HypervisorHostHelper.getPublicNetworkNamePrefix(vlanId);
+ Pair<Integer, VirtualDevice> publicNicInfo =
vmMo.getNicDeviceIndex(publicNetworkName);
+
+ if (s_logger.isDebugEnabled()) {
+ s_logger.debug(String.format("Find public NIC index,
public network name: %s , index: %s", publicNetworkName,
publicNicInfo.first()));
+ }
+
+ VirtualDevice nic = findVirtualNicDevice(vmMo, nicTO.getMac());
+
+ if (nic == null) {
+ return new ExecutionResult(false, "Couldn't find NIC");
+ }
+ configureNicDevice(vmMo, nic,
VirtualDeviceConfigSpecOperation.REMOVE);
+ }
+ } catch (Throwable e) {
+ s_logger.error("Unexpected exception: " + e.toString() + " will
shortcut rest of IPAssoc commands", e);
+ return new ExecutionResult(false, e.toString());
+ }
+ return new ExecutionResult(true, null);
+ }
+
+ private void configureNicDevice(VirtualMachineMO vmMo, VirtualDevice nic,
VirtualDeviceConfigSpecOperation operation) throws Exception {
+ VirtualMachineConfigSpec vmConfigSpec = new VirtualMachineConfigSpec();
+ VirtualDeviceConfigSpec deviceConfigSpec = new
VirtualDeviceConfigSpec();
+ deviceConfigSpec.setDevice(nic);
+ deviceConfigSpec.setOperation(operation);
+
+
+ vmConfigSpec.getDeviceChange().add(deviceConfigSpec);
+ if (!vmMo.configureVm(vmConfigSpec)) {
+ throw new Exception("Failed to configure devices when running
unplugNicCommand");
Review comment:
this is not always true I think,
```suggestion
throw new Exception("Failed to configure devices when running a
(un/re)PlugNicCommand");
```
maybe even pass it as parameter?
good extraction otherwise.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
