Copilot commented on code in PR #13105:
URL: https://github.com/apache/cloudstack/pull/13105#discussion_r3195394940
##########
server/src/main/java/com/cloud/server/StatsCollector.java:
##########
@@ -1582,8 +1590,13 @@ 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());
+ boolean isRoutedNetwork = networkVO !=
null && routedIpv4Manager.isRoutedNetwork(networkVO);
+ boolean isDirectAttachedNetwork =
CollectionUtils.isNotEmpty(vlan)
+ && vlan.get(0).getVlanType() ==
VlanType.DirectAttached;
Review Comment:
This loop now does multiple DB lookups per NIC/stat entry
(listVlansByNetworkId + networkDao.findById + isRoutedNetwork() which itself
consults the offering DAO). In large deployments this can become a hot path
during periodic stats collection. Consider caching per-networkId (e.g.,
Map<Long,Boolean> routed/directAttached) within the task run/transaction and
short-circuiting the VLAN query for routed networks (check routed first; only
query VLANs when not routed).
##########
server/src/main/java/com/cloud/server/StatsCollector.java:
##########
@@ -1582,8 +1590,13 @@ 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());
+ boolean isRoutedNetwork = networkVO !=
null && routedIpv4Manager.isRoutedNetwork(networkVO);
+ boolean isDirectAttachedNetwork =
CollectionUtils.isNotEmpty(vlan)
+ && vlan.get(0).getVlanType() ==
VlanType.DirectAttached;
+ if (!isRoutedNetwork &&
!isDirectAttachedNetwork) {
+ continue; // only get network
statistics for Shared or Routed network
+ }
Review Comment:
The routed-network filtering logic is newly introduced here but doesn’t
appear to be covered by existing unit tests for StatsCollector. Please add a
focused test (mocking NetworkDao/RoutedIpv4Manager/VlanDao and a NIC) to verify
that routed networks are included, and that non-direct-attached, non-routed
networks are skipped.
--
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]
