Copilot commented on code in PR #13105:
URL: https://github.com/apache/cloudstack/pull/13105#discussion_r3187573817
##########
server/src/main/java/com/cloud/server/StatsCollector.java:
##########
@@ -1582,8 +1590,11 @@ public void
doInTransactionWithoutResult(TransactionStatus status) {
sc_nic.addAnd("macAddress",
SearchCriteria.Op.EQ, vmNetworkStatEntry.getMacAddress());
NicVO nic = _nicDao.search(sc_nic,
null).get(0);
List<VlanVO> vlan =
_vlanDao.listVlansByNetworkId(nic.getNetworkId());
- if (vlan == null || vlan.size() == 0 ||
vlan.get(0).getVlanType() != VlanType.DirectAttached)
- continue; // only get network
statistics for DirectAttached network (shared networks in Basic zone and
Advanced zone with/without SG)
+ NetworkVO networkVO =
networkDao.findById(nic.getNetworkId());
+ if (CollectionUtils.isEmpty(vlan) ||
(vlan.get(0).getVlanType() != VlanType.DirectAttached
+ && (networkVO == null ||
!routedIpv4Manager.isRoutedNetwork(networkVO)))) {
Review Comment:
The new filter still skips statistics collection when
`listVlansByNetworkId(...)` returns empty, even if the NIC’s network is a
routed network. Routed guest networks get their CIDR/subnet via
`RoutedIpv4Manager`/`Ipv4GuestSubnetNetworkMap` (not via `vlan` table), so this
`CollectionUtils.isEmpty(vlan)` guard can prevent routed networks from ever
reaching the `isRoutedNetwork` check. Consider restructuring the condition to
treat routed networks as eligible regardless of VLAN list presence (e.g., check
`isRoutedNetwork` first, and only enforce the VLAN/DirectAttached requirement
for non-routed networks).
##########
server/src/main/java/com/cloud/server/StatsCollector.java:
##########
@@ -1582,8 +1590,11 @@ public void
doInTransactionWithoutResult(TransactionStatus status) {
sc_nic.addAnd("macAddress",
SearchCriteria.Op.EQ, vmNetworkStatEntry.getMacAddress());
NicVO nic = _nicDao.search(sc_nic,
null).get(0);
List<VlanVO> vlan =
_vlanDao.listVlansByNetworkId(nic.getNetworkId());
- if (vlan == null || vlan.size() == 0 ||
vlan.get(0).getVlanType() != VlanType.DirectAttached)
- continue; // only get network
statistics for DirectAttached network (shared networks in Basic zone and
Advanced zone with/without SG)
+ NetworkVO networkVO =
networkDao.findById(nic.getNetworkId());
+ if (CollectionUtils.isEmpty(vlan) ||
(vlan.get(0).getVlanType() != VlanType.DirectAttached
+ && (networkVO == null ||
!routedIpv4Manager.isRoutedNetwork(networkVO)))) {
Review Comment:
This change introduces new behavior around routed networks in the VM network
stats collection path, but there are no unit tests covering the
acceptance/rejection logic for routed vs shared networks. Since
`StatsCollectorTest` exists for this class, it would be good to add tests that
exercise the VLAN-type/routed-network branching to prevent regressions
(including the case where routed networks have no VLAN records).
--
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]
