Copilot commented on code in PR #12811:
URL: https://github.com/apache/cloudstack/pull/12811#discussion_r2930448486
##########
server/src/main/java/org/apache/cloudstack/network/RoutedIpv4ManagerImpl.java:
##########
@@ -1013,8 +1013,9 @@ public boolean isDynamicRoutedNetwork(Network network) {
@Override
public boolean isDynamicRoutedNetwork(NetworkOffering networkOffering) {
- return
NetworkOffering.NetworkMode.ROUTED.equals(networkOffering.getNetworkMode())
- &&
NetworkOffering.RoutingMode.Dynamic.equals(networkOffering.getRoutingMode());
+ return
NetworkOffering.RoutingMode.Dynamic.equals(networkOffering.getRoutingMode()) &&
+
(NetworkOffering.NetworkMode.ROUTED.equals(networkOffering.getNetworkMode()) ||
+
NetUtils.InternetProtocol.DualStack.equals(networkOfferingDao.getNetworkOfferingInternetProtocol(networkOffering.getId())));
Review Comment:
`isDynamicRoutedNetwork(NetworkOffering)` now calls
`networkOfferingDao.getNetworkOfferingInternetProtocol(...)`, which performs a
details DAO lookup (DB hit) for non-ROUTED offerings. If this check is used
frequently (e.g. during network create/validate flows), it can add avoidable
query overhead. Consider passing the internet protocol into this method,
caching it on the offering VO, or restructuring so the protocol is fetched once
per offering/network instead of per check.
##########
server/src/main/java/com/cloud/network/router/CommandSetupHelper.java:
##########
@@ -1467,14 +1467,29 @@ public void createBgpPeersCommands(final List<? extends
BgpPeer> bgpPeers, final
} else {
guestNetworks.add(network);
}
+ Map<Long, NetworkOfferingVO> guestNetworkOfferings = new HashMap<>();
+ for (Network guestNetwork : guestNetworks) {
+ final NetworkOfferingVO offering =
_networkOfferingDao.findByIdIncludingRemoved(guestNetwork.getNetworkOfferingId());
+ guestNetworkOfferings.put(guestNetwork.getId(), offering);
+ }
for (BgpPeer bgpPeer: bgpPeers) {
Map<BgpPeer.Detail, String> bgpPeerDetails =
bgpPeerDetailsDao.getBgpPeerDetails(bgpPeer.getId());
for (Network guestNetwork : guestNetworks) {
- bgpPeerTOs.add(new BgpPeerTO(bgpPeer.getId(),
bgpPeer.getIp4Address(), bgpPeer.getIp6Address(), bgpPeer.getAsNumber(),
bgpPeer.getPassword(),
- guestNetwork.getId(), asNumberVO.getAsNumber(),
guestNetwork.getCidr(), guestNetwork.getIp6Cidr(), bgpPeerDetails));
+ final NetworkOfferingVO offering =
guestNetworkOfferings.get(guestNetwork.getId());
+ if
(NetworkOffering.NetworkMode.ROUTED.equals(offering.getNetworkMode())) {
+ bgpPeerTOs.add(new BgpPeerTO(bgpPeer.getId(),
bgpPeer.getIp4Address(), bgpPeer.getIp6Address(), bgpPeer.getAsNumber(),
bgpPeer.getPassword(),
+ guestNetwork.getId(), asNumberVO.getAsNumber(),
guestNetwork.getCidr(), guestNetwork.getIp6Cidr(), bgpPeerDetails));
+ } else if (guestNetwork.getIp6Cidr() != null &&
bgpPeer.getIp6Address() != null) {
+ bgpPeerTOs.add(new BgpPeerTO(bgpPeer.getId(), null,
bgpPeer.getIp6Address(), bgpPeer.getAsNumber(), bgpPeer.getPassword(),
+ guestNetwork.getId(), asNumberVO.getAsNumber(),
null, guestNetwork.getIp6Cidr(), bgpPeerDetails));
+ }
}
}
+ if (bgpPeerTOs.isEmpty()) {
+ logger.debug("No BGP peers to configure for the guest network or
VPC, skipping.");
+ return;
Review Comment:
`createBgpPeersCommands` now returns early when `bgpPeerTOs` is empty. This
prevents sending an empty `SetBgpPeersCommand`, which is needed to clear any
previously configured BGP peers on the VR when the peer list becomes empty (the
SystemVM merge logic clears the databag only when it receives an empty peers
list). Please still send the command (with an empty peers array) when an AS
number exists, so stale FRR/BGP config is removed.
##########
server/src/main/java/com/cloud/api/ApiResponseHelper.java:
##########
@@ -3578,12 +3582,22 @@ public VpcResponse createVpcResponse(ResponseView view,
Vpc vpc) {
response.addIpv4Route(route);
}
}
- if (view == ResponseView.Full) {
- List<BgpPeerVO> bgpPeerVOS =
bgpPeerDao.listNonRevokeByVpcId(vpc.getId());
- for (BgpPeerVO bgpPeerVO : bgpPeerVOS) {
- BgpPeerResponse bgpPeerResponse =
routedIpv4Manager.createBgpPeerResponse(bgpPeerVO);
- response.addBgpPeer(bgpPeerResponse);
- }
+ }
+
+ // add IPv6 routes
+ ipv6Service.updateIpv6RoutesForVpcResponse(vpc, response);
+ if (Objects.nonNull(asNumberVO)) {
+ response.setIpv6Routing(Network.Routing.Dynamic.name());
+ } else {
+ response.setIpv6Routing(Network.Routing.Static.name());
Review Comment:
`createVpcResponse` sets `ipv6Routing` to Static/Dynamic purely based on
`asNumberVO` presence, without checking whether IPv6 is actually supported/used
by any VPC tier. This can report IPv6 routing as enabled on IPv4-only VPCs and
is inconsistent with `createNetworkResponse`, which only sets IPv6 routing when
the offering supports IPv6. Consider setting `ipv6Routing` only when IPv6 is
supported (e.g., when any tier has IPv6 enabled / when `ipv6Routes` is
populated) and leaving it unset otherwise.
##########
server/src/main/java/com/cloud/api/ApiResponseHelper.java:
##########
@@ -2843,6 +2839,15 @@ public NetworkResponse
createNetworkResponse(ResponseView view, Network network)
}
}
+ // Add BGP peer information for full view
+ if (view == ResponseView.Full) {
+ List<BgpPeerVO> bgpPeerVOS =
bgpPeerDao.listNonRevokeByNetworkId(network.getId());
+ for (BgpPeerVO bgpPeerVO : bgpPeerVOS) {
+ BgpPeerResponse bgpPeerResponse =
routedIpv4Manager.createBgpPeerResponse(bgpPeerVO);
+ response.addBgpPeer(bgpPeerResponse);
+ }
+ }
Review Comment:
`createNetworkResponse` now fetches BGP peers on every `ResponseView.Full`
response (`listNonRevokeByNetworkId`), even when the network has no AS number /
dynamic routing enabled. This introduces an extra DB query per network in full
views (e.g. admin list APIs). Consider gating this query on whether the network
is actually using dynamic routing (or at least has an AS number set in the
response) to avoid unnecessary per-network lookups.
##########
server/src/main/java/com/cloud/bgp/BGPServiceImpl.java:
##########
@@ -413,9 +417,12 @@ public boolean applyBgpPeers(Vpc vpc, boolean
continueOnError) throws ResourceUn
if (!routedIpv4Manager.isDynamicRoutedVpc(vpc)) {
return true;
}
- final String gatewayProviderStr =
vpcServiceMapDao.getProviderForServiceInVpc(vpc.getId(),
Network.Service.Gateway);
- if (gatewayProviderStr != null) {
- NetworkElement provider =
networkModel.getElementImplementingProvider(gatewayProviderStr);
+ VpcOffering vpcOffering =
vpcOfferingDao.findById(vpc.getVpcOfferingId());
Review Comment:
`applyBgpPeers(Vpc, ...)` dereferences `vpcOfferingDao.findById(...)` result
without checking for null (`vpcOffering.getNetworkMode()`). If the VPC offering
is removed/invalid, this will NPE and prevent BGP application. Please add a
null-check (and consider `findByIdIncludingRemoved` if appropriate) and handle
the error path explicitly.
##########
server/src/main/java/com/cloud/bgp/BGPServiceImpl.java:
##########
@@ -396,9 +397,12 @@ public boolean applyBgpPeers(Network network, boolean
continueOnError) throws Re
if (!routedIpv4Manager.isDynamicRoutedNetwork(network)) {
return true;
}
- final String gatewayProviderStr =
ntwkSrvcDao.getProviderForServiceInNetwork(network.getId(),
Network.Service.Gateway);
- if (gatewayProviderStr != null) {
- NetworkElement provider =
networkModel.getElementImplementingProvider(gatewayProviderStr);
+ NetworkOffering networkOffering =
networkOfferingDao.findById(network.getNetworkOfferingId());
+ final String bgpServiceProvider =
NetworkOffering.NetworkMode.ROUTED.equals(networkOffering.getNetworkMode()) ?
+ ntwkSrvcDao.getProviderForServiceInNetwork(network.getId(),
Network.Service.Gateway):
+ ntwkSrvcDao.getProviderForServiceInNetwork(network.getId(),
Network.Service.SourceNat);
Review Comment:
`applyBgpPeers(Network, ...)` dereferences
`networkOfferingDao.findById(...)` result without checking for null
(`networkOffering.getNetworkMode()`). If the offering is removed/invalid, this
will NPE and prevent BGP application. Please add a null-check (and ideally use
`findByIdIncludingRemoved` for consistency with other paths) and handle the
error by logging/returning a failure result.
##########
server/src/main/java/com/cloud/network/router/CommandSetupHelper.java:
##########
@@ -1467,14 +1467,29 @@ public void createBgpPeersCommands(final List<? extends
BgpPeer> bgpPeers, final
} else {
guestNetworks.add(network);
}
+ Map<Long, NetworkOfferingVO> guestNetworkOfferings = new HashMap<>();
+ for (Network guestNetwork : guestNetworks) {
+ final NetworkOfferingVO offering =
_networkOfferingDao.findByIdIncludingRemoved(guestNetwork.getNetworkOfferingId());
+ guestNetworkOfferings.put(guestNetwork.getId(), offering);
+ }
for (BgpPeer bgpPeer: bgpPeers) {
Map<BgpPeer.Detail, String> bgpPeerDetails =
bgpPeerDetailsDao.getBgpPeerDetails(bgpPeer.getId());
for (Network guestNetwork : guestNetworks) {
- bgpPeerTOs.add(new BgpPeerTO(bgpPeer.getId(),
bgpPeer.getIp4Address(), bgpPeer.getIp6Address(), bgpPeer.getAsNumber(),
bgpPeer.getPassword(),
- guestNetwork.getId(), asNumberVO.getAsNumber(),
guestNetwork.getCidr(), guestNetwork.getIp6Cidr(), bgpPeerDetails));
+ final NetworkOfferingVO offering =
guestNetworkOfferings.get(guestNetwork.getId());
+ if
(NetworkOffering.NetworkMode.ROUTED.equals(offering.getNetworkMode())) {
+ bgpPeerTOs.add(new BgpPeerTO(bgpPeer.getId(),
bgpPeer.getIp4Address(), bgpPeer.getIp6Address(), bgpPeer.getAsNumber(),
bgpPeer.getPassword(),
+ guestNetwork.getId(), asNumberVO.getAsNumber(),
guestNetwork.getCidr(), guestNetwork.getIp6Cidr(), bgpPeerDetails));
+ } else if (guestNetwork.getIp6Cidr() != null &&
bgpPeer.getIp6Address() != null) {
+ bgpPeerTOs.add(new BgpPeerTO(bgpPeer.getId(), null,
bgpPeer.getIp6Address(), bgpPeer.getAsNumber(), bgpPeer.getPassword(),
+ guestNetwork.getId(), asNumberVO.getAsNumber(),
null, guestNetwork.getIp6Cidr(), bgpPeerDetails));
Review Comment:
`offering` is taken from `findByIdIncludingRemoved(...)` and then
dereferenced (`offering.getNetworkMode()`) without a null-check. If the network
offering row is missing (or cannot be loaded), this will throw an NPE and abort
router configuration. Consider validating `offering != null` when building
`guestNetworkOfferings` (and/or when reading from the map) and either skip that
network with a clear log message or fail fast with a descriptive exception.
##########
server/src/main/java/org/apache/cloudstack/network/RoutedIpv4ManagerImpl.java:
##########
@@ -1030,8 +1031,9 @@ public boolean isDynamicRoutedVpc(Vpc vpc) {
@Override
public boolean isDynamicRoutedVpc(VpcOffering vpcOffering) {
- return
NetworkOffering.NetworkMode.ROUTED.equals(vpcOffering.getNetworkMode())
- &&
NetworkOffering.RoutingMode.Dynamic.equals(vpcOffering.getRoutingMode());
+ return
NetworkOffering.RoutingMode.Dynamic.equals(vpcOffering.getRoutingMode()) &&
+
(NetworkOffering.NetworkMode.ROUTED.equals(vpcOffering.getNetworkMode()) ||
+
NetUtils.InternetProtocol.DualStack.equals(vpcOfferingDao.getVpcOfferingInternetProtocol(vpcOffering.getId())));
Review Comment:
`isDynamicRoutedVpc(VpcOffering)` now calls
`vpcOfferingDao.getVpcOfferingInternetProtocol(...)`, which performs a details
DAO lookup (DB hit) for non-ROUTED offerings. If this check is called often, it
can add query overhead. Consider fetching/caching the internet protocol once
per offering/VPC and reusing it instead of looking it up inside this predicate.
--
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]
