Copilot commented on code in PR #14166:
URL: https://github.com/apache/cloudstack/pull/14166#discussion_r4458018368
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java:
##########
@@ -6325,6 +6337,69 @@ public boolean hostSupportsInstanceConversion() {
return exitValue == 0;
}
+ /**
+ * Static capability: whether this host's libvirt understands multi-tag
trunk vlan XML
+ * (<vlan trunk='yes'>) on a standard Linux bridge interface.
+ */
+ public boolean hostSupportsVlanTrunkXml() {
+ return hypervisorLibvirtVersion >= MIN_LIBVIRT_VERSION_FOR_VLAN_TRUNK;
+ }
+
+ /**
+ * Live state: whether vlan_filtering is currently enabled on this host's
guest bridge.
+ * A vlan_filtering=0 bridge floods every tagged frame to every port
regardless of tag,
+ * so this must be true before any multi-VLAN trunk nic can be placed on
this host.
+ */
+ public boolean hostSupportsVlanFiltering() {
+ return isBridgeVlanFilteringEnabled(getEffectiveGuestBridgeName());
Review Comment:
This checks only the last guest traffic label retained by
`LibvirtCheckNetworkCommandWrapper`, but `BridgeVifDriver.plug()` chooses a
bridge separately for each NIC using `nic.getName()`. On a host with two guest
bridges, where only that last bridge has VLAN filtering enabled, NICs on the
other bridge also pass this check and take the shared-bridge path. With the
older-libvirt fallback, their VLAN membership rules do not provide isolation
because their actual bridge still has `vlan_filtering=0`; even ordinary
single-VLAN NICs are affected. Check the NIC's resolved bridge before selecting
this path, and track readiness per guest traffic label rather than using one
bridge's capability for the whole host.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCheckNetworkCommandWrapper.java:
##########
@@ -37,8 +38,12 @@ public final class LibvirtCheckNetworkCommandWrapper extends
CommandWrapper<Chec
public Answer execute(final CheckNetworkCommand command, final
LibvirtComputingResource libvirtComputingResource) {
final List<PhysicalNetworkSetupInfo> phyNics =
command.getPhysicalNetworkInfoList();
String errMsg = null;
+ String guestNetworkLabel = null;
for (final PhysicalNetworkSetupInfo nic : phyNics) {
+ if (StringUtils.isNotBlank(nic.getGuestNetworkName())) {
+ guestNetworkLabel = nic.getGuestNetworkName();
Review Comment:
This retains only the last guest traffic label, although
`CheckNetworkCommand` contains every physical network in the zone. With
`cloudbr1` using legacy VLAN bridges and `cloudbr2` having VLAN filtering
enabled, readiness is reported solely from `cloudbr2`. `BridgeVifDriver` then
uses that host-wide result for a NIC whose actual traffic label is `cloudbr1`,
selecting the shared-bridge path on the wrong bridge (also for ordinary NICs).
The reverse label order rejects usable trunks. Please track readiness per guest
bridge and check the bridge actually selected for each NIC.
##########
server/src/main/java/com/cloud/api/query/QueryManagerImpl.java:
##########
@@ -1304,6 +1308,25 @@ private Object getObjectPossibleMethodValue(Object obj,
String methodName) {
return result;
}
+ SearchBuilder<NicVO> createNicSearchBuilder(Long vpcId, boolean
hasAssociatedNics) {
+ SearchBuilder<NicVO> nicSearch = nicDao.createSearchBuilder();
+ if (hasAssociatedNics) {
+ // trunk nics list networkId as an association, not as their
primary network
+ nicSearch.and().op("networkId", nicSearch.entity().getNetworkId(),
Op.EQ);
+ nicSearch.or("associatedNicIdIN", nicSearch.entity().getId(),
Op.IN);
+ nicSearch.cp();
+ } else {
+ nicSearch.and("networkId", nicSearch.entity().getNetworkId(),
Op.EQ);
+ }
+ nicSearch.and("removed", nicSearch.entity().getRemoved(), Op.NULL);
+ if (vpcId != null) {
+ SearchBuilder<NetworkVO> networkSearch =
networkDao.createSearchBuilder();
+ networkSearch.and("vpcId", networkSearch.entity().getVpcId(),
Op.EQ);
+ nicSearch.join("vpc", networkSearch,
networkSearch.entity().getId(), nicSearch.entity().getNetworkId(),
JoinBuilder.JoinType.INNER);
Review Comment:
The VPC join still tests only the NIC's primary network, so it filters out
the associated-NIC matches added above when they cross VPC boundaries. For
example, a NIC primary in VPC A and associated with tier B in VPC B is returned
by `listVirtualMachines?networkid=B`, but disappears when `vpcid=B` is also
supplied. `EnableStaticNat.fetchDataTiers()` sends both parameters, making that
VM unavailable for static NAT in the UI despite its valid association. Apply
the VPC predicate to the matching primary or associated network, and cover the
combined network/VPC filter in a regression test.
##########
ui/src/views/infra/Resources.vue:
##########
@@ -111,6 +141,21 @@ export default {
}).finally(() => {
this.fetchLoading = false
})
+ fetchMultiNetworkNicEnabledForZone(this.zoneId)
+ this.fetchHostReadiness(entity, params[entity])
+ },
+ fetchHostReadiness (entity, entityId) {
+ const params = { type: 'Routing' }
Review Comment:
`listHosts` is paginated even when no page parameters are supplied
(`BaseListCmd.getPageSizeVal()` uses `default.page.size`, which defaults to
500). Consequently, a zone with more hosts reports readiness and total counts
for only the first page, and can display 100% ready while later hosts are not
ready. Fetch all pages or explicitly request an unlimited result before
calculating this zone/pod/cluster-wide readiness summary.
##########
engine/components-api/src/main/java/com/cloud/event/UsageEventUtils.java:
##########
@@ -94,6 +108,52 @@ public static void publishUsageEvent(String usageType, long
accountId, long zone
}
+ // named distinctly, rather than as another publishUsageEvent(...)
overload, because its parameter shape
+ // (Map<String, String>, boolean) is only distinguishable at compile time
from the existing (Long, boolean)
+ // overload above when neither argument is a literal null - several
existing call sites pass null literals
+ // here, which would make an overload ambiguous
+ public static void publishUsageEventWithDetails(String usageType, long
accountId, long zoneId, long resourceId, String resourceName, Long offeringId,
Long templateId,
+ Long size, String entityType, String
entityUUID, Map<String, String> details, boolean displayResource) {
+ if (displayResource) {
+ saveUsageEvent(usageType, accountId, zoneId, resourceId,
resourceName, offeringId, templateId, size, details);
+ }
+ publishUsageEvent(usageType, accountId, zoneId, entityType,
entityUUID);
+ }
+
+ /**
+ * Emits one {@code usageType} usage event per network a NIC is actually
billed against - its primary
+ * network, plus any additional networks it is associated with as a
multi-VLAN trunk nic. An ordinary,
+ * non-trunk nic has exactly one such network, so this is identical to
today's single-event behaviour for
+ * every legacy nic. Each event carries the network id as a detail, so the
usage-processing side can
+ * disambiguate rows when a trunk nic's networks share the same network
offering.
+ */
+ public static void publishNicNetworkOfferingUsageEvents(String usageType,
long accountId, long zoneId, long vmId, String entityType, String entityUUID,
+ Nic nic, long isDefault, boolean displayResource) {
+ NetworkVO primaryNetwork = s_networkDao.findById(nic.getNetworkId());
+ if (primaryNetwork != null) {
+ publishNicNetworkOfferingUsageEvent(usageType, accountId, zoneId,
vmId, entityType, entityUUID, nic.getId(), primaryNetwork, isDefault,
displayResource);
+ }
+ if (nic.getMultiNetwork()) {
+ for (NicNetworkMapVO association :
s_nicNetworkMapDao.listByNicId(nic.getId())) {
+ NetworkVO associatedNetwork =
s_networkDao.findById(association.getNetworkId());
+ if (associatedNetwork != null) {
+ publishNicNetworkOfferingUsageEvent(usageType, accountId,
zoneId, vmId, entityType, entityUUID, nic.getId(), associatedNetwork, isDefault,
Review Comment:
Associated networks inherit `isDefault` from the primary NIC here. For a
default trunk NIC, a VM start therefore emits association events with size=1,
whereas `NetworkServiceImpl.publishAssociationUsageEvent()` emits size=0 for
the same networks when associated live. The usage processor turns this value
into `isDefault`, so an association's reported default status changes merely by
stopping and starting the VM. Keep the primary network's flag unchanged, but
emit 0L for associated-network events, consistently with the live-association
path.
--
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]
