bhouse-nexthop commented on code in PR #14378:
URL: https://github.com/apache/cloudstack/pull/14378#discussion_r4273922750
##########
engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java:
##########
@@ -4614,18 +4668,56 @@ protected NicProfile getNicProfileForVm(final Network
network, final NicProfile
final NicVO nicVO =
_nicDao.findByNetworkIdInstanceIdAndBroadcastUri(network.getId(), vm.getId(),
broadcastUri);
if (nicVO != null) {
if (ipAddress == null ||
nicVO.getIPv4Address().equals(ipAddress)) {
- nic = _networkModel.getNicProfile(vm, network.getId(),
broadcastUri);
+ nic = _networkModel.getNicProfile(vm, network.getId(),
broadcastUri, null);
}
}
} else {
final NicVO nicVO =
_nicDao.findByNtwkIdAndInstanceId(network.getId(), vm.getId());
if (nicVO != null) {
- nic = _networkModel.getNicProfile(vm, network.getId(), null);
+ nic = _networkModel.getNicProfile(vm, network.getId(), null,
null);
}
}
return nic;
}
+ private Integer getVpcVrNicMultiqueueNumber(Network network,
VirtualMachine vm) {
+ if
(ExternalGuestNetworkGuru.NETWORK_GURU_NAME.equals(network.getGuruName())) {
+ NetworkOfferingVO networkOffering =
_entityMgr.findByIdIncludingRemoved(NetworkOfferingVO.class,
network.getNetworkOfferingId());
+ if (networkOffering == null) {
+ logger.info("Unable to retrieve the multiqueue value for
network [{}], as it was not possible to find its corresponding network
offering.", network.getUuid());
+ return null;
+ }
+
+ if (!networkOffering.isForVpc()) {
+ logger.info("Network offering [{}] is not a tier offering.
Skipping the retrieval of the new NIC's multiqueue number.",
networkOffering.getUuid());
+ return null;
+ }
+
+ logger.debug("Network [{}] is a VPC tier; therefore, the private
multiqueue number defined in its corresponding offering will be used.",
network.getUuid());
+ return networkOffering.getPrivateMultiqueueNumber();
+ }
+
+ DomainRouterVO router = routerDao.findById(vm.getId());
+ if (router.getVpcId() == null) {
+ logger.info("The VR [{}] does not belong to a VPC. Skipping the
retrieval of the new NIC's multiqueue number.", vm.getInstanceName());
+ return null;
+ }
+
+ VpcVO vpc = _entityMgr.findById(VpcVO.class, router.getVpcId());
+ if (vpc == null) {
+ logger.info("Unable to find the VPC that the VR [{}] belongs to.
Skipping the retrieval of the new NIC's multiqueue number.",
vm.getInstanceName());
+ return null;
+ }
+
+ VpcOfferingVO vpcOffering =
_entityMgr.findByIdIncludingRemoved(VpcOfferingVO.class,
vpc.getVpcOfferingId());
+ if (vpcOffering == null) {
+ logger.info("Unable to retrieve the multiqueue value for the
private gateway's VR NIC, as it was not possible to find its VPC offering.");
+ return null;
+ }
+
+ return vpcOffering.getPrivateGatewayMultiqueueNumber();
Review Comment:
This fallback also catches public NICs. When a running VPC VR gets an IP
from a new public VLAN, `NicPlugInOutRules` goes `addVmToNetwork` →
`createNicForVm`. The guru there is `PublicNetworkGuru`, so the NIC gets the
private gateway value. After a VR restart, `finalizeCommandsOnStart` gives the
same NIC the public value.
VPC tiers on VXLAN (or any guru other than `ExternalGuestNetworkGuru`) end
up here too, so a tier added at runtime gets the private gateway value, but the
tier value after a restart.
Picking the value by traffic type, the way `finalizeCommandsOnStart` does,
covers both cases:
```java
if (network.getTrafficType() == TrafficType.Public) {
return vpcOffering.getPublicMultiqueueNumber();
}
if (_networkModel.isPrivateGateway(network.getId())) {
return vpcOffering.getPrivateGatewayMultiqueueNumber();
}
NetworkOfferingVO tierOffering =
_entityMgr.findByIdIncludingRemoved(NetworkOfferingVO.class,
network.getNetworkOfferingId());
return tierOffering == null ? null :
tierOffering.getPrivateMultiqueueNumber();
```
##########
engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java:
##########
@@ -2216,6 +2219,56 @@ public void doInTransactionWithoutResult(final
TransactionStatus status) {
});
}
+ private void setGuestNetworkVrNicMultiqueueNumber(Pair<NetworkVO, VpcVO>
networks, NetworkVO implementedNetwork, NicProfile nicProfile, List<NicVO>
nics) {
+ if (networks == null) {
+ logger.debug("Skipping the assignment of multiqueue for VR because
it is not related with any network.");
+ return;
+ }
+
+ VpcVO vpcVO = networks.second();
+ boolean isVpcVr = vpcVO != null;
+ if (isVpcVr) {
+ logger.trace("The multiqueue number for NICs of VPCs VRs is not
defined at this step, skipping it.");
+ return;
+ }
+
+ if (implementedNetwork.getTrafficType() == TrafficType.Control) {
+ logger.trace("The multiqueue number is not applied for NICs of
control networks, skipping it.");
+ return;
+ }
+
+ Integer multiqueueNumber =
getGuestNetworkVrNicMultiqueueNumber(implementedNetwork.getTrafficType(), nics,
nicProfile);
+ if (multiqueueNumber == null) {
+ logger.info("Multiqueue for NIC [{}] will not be defined because
it was not possible to identify its corresponding network " +
+ "offering or the corresponding multiqueue attribute has
not been defined in the offering.");
+ } else {
+ logger.info("Setting [{}] as the multiqueue for NIC [{}].",
multiqueueNumber, nicProfile.getUuid());
+ nicProfile.setMultiqueueNumber(multiqueueNumber);
+ }
+ }
+
+
+ private Integer getGuestNetworkVrNicMultiqueueNumber(TrafficType
trafficType, List<NicVO> nics, NicProfile nicProfile) {
+ logger.debug("Fetching the network offering of the guest network
associated with the VR with ID [{}].", nicProfile.getVirtualMachineId());
+ NetworkOffering networkOfferingOfVrIsolatedNetwork = nics.stream()
+ .filter(nic ->
ExternalGuestNetworkGuru.NETWORK_GURU_NAME.equals(nic.getReserver()) ||
DirectNetworkGuru.NETWORK_GURU_NAME.equals(nic.getReserver()))
Review Comment:
Matching on the reserver name only covers VLAN isolation
(`ExternalGuestNetworkGuru`) and shared networks (`DirectNetworkGuru`). On an
isolated network using VXLAN (`VxlanGuestNetworkGuru`), OVS, or any other guest
guru, nothing matches. Neither the guest NIC nor the public NIC gets queues,
and nothing reports it.
`networks.first()` from `getGuestNetworkRouterAndVpcDetails` is already the
VR's guest network, so you can pass it in and skip the scan:
```java
NetworkOfferingVO offering =
_entityMgr.findByIdIncludingRemoved(NetworkOfferingVO.class,
networks.first().getNetworkOfferingId());
```
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java:
##########
@@ -6839,22 +6837,45 @@ protected void deleteIfExists(String snapshotPath) {
}
}
- public void setInterfaceDefQueueSettings(Map<String, String> details,
Integer cpus, InterfaceDef interfaceDef) {
+ private Integer getMultiqueueFromVmDetails(Map<String, String> details) {
String nicMultiqueueNumber =
details.get(VmDetailConstants.NIC_MULTIQUEUE_NUMBER);
if (nicMultiqueueNumber != null) {
try {
- Integer nicMultiqueueNumberInteger =
Integer.valueOf(nicMultiqueueNumber);
- if (nicMultiqueueNumberInteger ==
InterfaceDef.MULTI_QUEUE_NUMBER_MEANS_CPU_CORES) {
- if (cpus != null) {
- interfaceDef.setMultiQueueNumber(cpus);
- }
- } else {
-
interfaceDef.setMultiQueueNumber(nicMultiqueueNumberInteger);
- }
+ return Integer.valueOf(nicMultiqueueNumber);
} catch (NumberFormatException ex) {
- LOGGER.warn(String.format("VM details %s is not a valid
integer value %s", VmDetailConstants.NIC_MULTIQUEUE_NUMBER,
nicMultiqueueNumber));
+ logger.warn("VM detail [{}] is not a valid integer value {}.",
VmDetailConstants.NIC_MULTIQUEUE_NUMBER, nicMultiqueueNumber);
}
}
+
+ return null;
+ }
+
+ public void setInterfaceDefQueueSettings(Map<String, String> details,
Integer cpus, InterfaceDef interfaceDef, Integer multiqueueForVrs, boolean
isVr) {
+ if (details == null) {
+ details = new HashMap<>();
+ }
+
+ Integer nicMultiqueueNumber = getMultiqueueFromVmDetails(details);
+ if (nicMultiqueueNumber != null) {
+ logger.info("VM detail [{}] is equal to [{}]; therefore, it will
be used as the multiqueue value.", VmDetailConstants.NIC_MULTIQUEUE_NUMBER,
nicMultiqueueNumber);
+ } else if (isVr && multiqueueForVrs != null) {
Review Comment:
Here the VM detail takes priority over the offering value. #14136 sets
`nicMultiqueueNumber` in the TO details of every system VM that has 2 or more
vCPUs. Once both PRs merge, the offering values are ignored on any VR with more
than one vCPU, which is when they matter.
The per-NIC value is the more specific one, so I'd let it win for VRs:
```java
Integer nicMultiqueueNumber = isVr && multiqueueForVrs != null
? multiqueueForVrs
: getMultiqueueFromVmDetails(details);
```
If you'd rather keep this order, I can change #14136 so it skips routers.
##########
server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java:
##########
@@ -8610,6 +8646,8 @@ private void
applyResolvedValuesToCommand(CloneNetworkOfferingCmd cmd, NetworkOf
applyIfNotProvided(cmd, requestParams, "serviceOfferingId",
ApiConstants.SERVICE_OFFERING_ID, cmd.getServiceOfferingId(),
sourceOffering.getServiceOfferingId());
applyIfNotProvided(cmd, requestParams, "guestIptype",
ApiConstants.GUEST_IP_TYPE, cmd.getGuestIpType(),
sourceOffering.getGuestType().toString());
applyIfNotProvided(cmd, requestParams, "maxConnections",
ApiConstants.MAX_CONNECTIONS, cmd.getMaxconnections(),
sourceOffering.getConcurrentConnections());
+ applyIfNotProvided(cmd, requestParams, "publicMultiqueueNumber",
ApiConstants.PUBLIC_MULTIQUEUE_NUMBER, cmd.getPublicMultiqueueNumber(),
sourceOffering.getPublicMultiqueueNumber());
Review Comment:
Copying these from the source breaks a clone that changes the type. Take an
isolated offering with `publicmultiqueuenumber=4` and clone it with
`forvpc=true` (or `guestiptype=L2`). The copied value fails
`validateNetworkOfferingMultiqueueParameters`, so the clone errors out on a
parameter the caller never sent.
Move these lines below the `forVpc` line and skip them when the target can't
use them:
```java
boolean targetIsL2 =
GuestType.L2.name().equalsIgnoreCase(cmd.getGuestIpType());
if (!targetIsL2 && !Boolean.TRUE.equals(cmd.getForVpc())) {
applyIfNotProvided(cmd, requestParams, "publicMultiqueueNumber", ...);
}
if (!targetIsL2) {
applyIfNotProvided(cmd, requestParams, "privateMultiqueueNumber", ...);
}
```
##########
ui/src/views/offering/AddNetworkOffering.vue:
##########
@@ -352,6 +352,32 @@
</a-select-option>
</a-select>
</a-form-item>
+ <a-row :guttter="12" v-if="guestType !== 'l2'">
+ <a-col :md="12" :lg="12">
+ <a-form-item name="privatemultiqueuenumber"
ref="privatemultiqueuenumber">
+ <template #label>
+ <tooltip-label :title="$t('label.private.multiqueue.number')"
:tooltip="apiParams.privatemultiqueuenumber.description"/>
+ </template>
+ <a-input
+ v-model:value="form.privatemultiqueuenumber"
+ :placeholder="apiParams.privatemultiqueuenumber.description"
+ style="width: 75%;"
+ />
+ </a-form-item>
+ </a-col>
+ <a-col :md="12" :lg="12">
+ <a-form-item name="publicmultiqueuenumber"
ref="publicmultiqueuenumber" v-if="!forVpc">
Review Comment:
The `v-if` hides this field, but `form.publicmultiqueuenumber` keeps its
value and `handleSubmit` sends every form key. To reproduce: enter 4, then tick
"For VPC" (or switch to L2). The API rejects the request with an error about a
field the user can't see.
Clear the value when the field is hidden, or skip the key when `forVpc` or
L2 is set. CloneNetworkOffering.vue has the same problem, and there the value
is also prefilled from the source offering.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtReplugNicCommandWrapper.java:
##########
@@ -64,9 +65,7 @@ public Answer execute(final ReplugNicCommand command, final
LibvirtComputingReso
final VifDriver newVifDriver =
libvirtComputingResource.getVifDriver(nic.getType(), nic.getName());
final InterfaceDef interfaceDef = newVifDriver.plug(nic, "Other
PV", oldPluggedNic.getModel().toString(), null);
- if (command.getDetails() != null) {
-
libvirtComputingResource.setInterfaceDefQueueSettings(command.getDetails(),
null, interfaceDef);
- }
+
libvirtComputingResource.setInterfaceDefQueueSettings(command.getDetails(),
null, interfaceDef, nic.getMultiqueueNumber(), command.getVMType() ==
VirtualMachine.Type.DomainRouter);
Review Comment:
On replug, VR NICs lose their queues. Both callers
(`NetworkMigrationManagerImpl` and `NetworkServiceImpl.updateNetwork`) build
the NicTO from a profile that has no `multiqueueNumber`. `cpus` is also null
here, so `-1` gets skipped. After a network migration, the VR's replugged NIC
is single-queue until the VR restarts.
Pass the CPU count through `ReplugNicCommand` the same way `PlugNicCommand`
now does, and set the multiqueue number on the profile at both callers.
##########
engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java:
##########
@@ -2216,6 +2219,56 @@ public void doInTransactionWithoutResult(final
TransactionStatus status) {
});
}
+ private void setGuestNetworkVrNicMultiqueueNumber(Pair<NetworkVO, VpcVO>
networks, NetworkVO implementedNetwork, NicProfile nicProfile, List<NicVO>
nics) {
+ if (networks == null) {
+ logger.debug("Skipping the assignment of multiqueue for VR because
it is not related with any network.");
+ return;
+ }
+
+ VpcVO vpcVO = networks.second();
+ boolean isVpcVr = vpcVO != null;
+ if (isVpcVr) {
+ logger.trace("The multiqueue number for NICs of VPCs VRs is not
defined at this step, skipping it.");
+ return;
+ }
+
+ if (implementedNetwork.getTrafficType() == TrafficType.Control) {
+ logger.trace("The multiqueue number is not applied for NICs of
control networks, skipping it.");
+ return;
+ }
+
+ Integer multiqueueNumber =
getGuestNetworkVrNicMultiqueueNumber(implementedNetwork.getTrafficType(), nics,
nicProfile);
+ if (multiqueueNumber == null) {
+ logger.info("Multiqueue for NIC [{}] will not be defined because
it was not possible to identify its corresponding network " +
Review Comment:
This message has two `{}` placeholders but no arguments, so the braces print
as-is. It also logs at INFO for every VR NIC on every start whenever the
offering leaves the field unset, and unset is the default.
```java
logger.debug("Not setting multiqueue for NIC [{}]: no offering found or no
value set.", nicProfile.getUuid());
```
##########
server/src/main/java/com/cloud/network/router/VpcVirtualNetworkApplianceManagerImpl.java:
##########
@@ -382,12 +384,12 @@ private void appendSourceNatIpToBootArgs(final
VirtualMachineProfile profile) {
public boolean finalizeCommandsOnStart(final Commands cmds, final
VirtualMachineProfile profile) {
final DomainRouterVO domainRouterVO =
_routerDao.findById(profile.getId());
- Map<String, String> details = new HashMap<String, String>();
-
- if(profile.getHypervisorType() == Hypervisor.HypervisorType.VMware){
+ Map<String, String> details = new HashMap<>();
+ VirtualMachineTO vmTO = null;
+ if (profile.getHypervisorType() == Hypervisor.HypervisorType.VMware ||
profile.getHypervisorType() == Hypervisor.HypervisorType.KVM) {
HypervisorGuru hvGuru =
_hvGuruMgr.getGuru(profile.getHypervisorType());
- VirtualMachineTO vmTO = hvGuru.implement(profile);
- if(vmTO.getDetails() != null){
+ vmTO = hvGuru.implement(profile);
Review Comment:
With this change, `hvGuru.implement()` runs on every KVM VR start, non-VPC
routers included. Those routers hit the `!isVpc` return a few lines down and
throw the result away. Moving the `isVpc` check above this block avoids the
extra work. If the CPU count is all you need,
`profile.getServiceOffering().getCpu()` gives it without building a full TO.
--
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]
