Copilot commented on code in PR #14286:
URL: https://github.com/apache/cloudstack/pull/14286#discussion_r4246246653
##########
engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java:
##########
@@ -636,6 +636,22 @@ public void doInTransactionWithoutResult(final
TransactionStatus status) {
true, false, false, false, false, null, null,
null, true, null, null, false);
}
+ //#4-2 - default isolated offering with Source nat service and
egress traffic allowed by default
+ if
(_networkOfferingDao.findByUniqueName(NetworkOffering.DefaultIsolatedNetworkOfferingWithSourceNatServiceEgressAllowed)
== null) {
+ offering =
_configMgr.createNetworkOffering(NetworkOffering.DefaultIsolatedNetworkOfferingWithSourceNatServiceEgressAllowed,
+ "Offering for Isolated networks with Source Nat
service enabled and egress traffic allowed by default", TrafficType.Guest,
null, false, Availability.Optional, null,
+
defaultIsolatedSourceNatEnabledNetworkOfferingProviders, true,
Network.GuestType.Isolated, false, null, true, null, false, false, null, true,
null,
+ true, false, false, false, false, null, null,
null, true, null, null, false);
+ // keep consistent with the offering created on fresh
installation (ConfigurationServerImpl)
+ offering.setMulticastRateMbps(null);
+ offering.setDedicatedLB(true);
+ offering.setEipAssociatePublicIp(true);
+ offering.setSupportsVmAutoScaling(true);
+ offering.setEgressDefaultPolicy(true);
+ offering.setKeepAliveEnabled(false);
Review Comment:
The new offering created here is updated with several flags, but its `state`
is never explicitly set to `Enabled`, while the fresh-install path does set
`Enabled` before persisting. If `createNetworkOffering` defaults to a
non-Enabled state, this offering may not be usable without a manual enable. Set
the offering state to `Enabled` here as well (or ensure `createNetworkOffering`
returns an Enabled offering for this code path) to match the intended default
behavior.
##########
server/src/main/java/com/cloud/server/ConfigurationServerImpl.java:
##########
@@ -1070,12 +1070,29 @@ public void
doInTransactionWithoutResult(TransactionStatus status) {
defaultIsolatedSourceNatEnabledNetworkOffering.setSupportsVmAutoScaling(true);
defaultIsolatedSourceNatEnabledNetworkOffering =
_networkOfferingDao.persistDefaultNetworkOffering(defaultIsolatedSourceNatEnabledNetworkOffering);
+ // Offering #3-2
+ NetworkOfferingVO
defaultIsolatedSourceNatEnabledNetworkOfferingWithDefaultEgressAllowed =
+ new
NetworkOfferingVO(NetworkOffering.DefaultIsolatedNetworkOfferingWithSourceNatServiceEgressAllowed,
+ "Offering for Isolated networks with Source
Nat service enabled and egress traffic allowed by default", TrafficType.Guest,
false, false, null, null, true, Availability.Optional, null,
+ Network.GuestType.Isolated, true, false,
false, false, true, false);
+
+
defaultIsolatedSourceNatEnabledNetworkOfferingWithDefaultEgressAllowed.setState(NetworkOffering.State.Enabled);
+
defaultIsolatedSourceNatEnabledNetworkOfferingWithDefaultEgressAllowed.setSupportsVmAutoScaling(true);
+
defaultIsolatedSourceNatEnabledNetworkOfferingWithDefaultEgressAllowed.setEgressDefaultPolicy(true);
+
defaultIsolatedSourceNatEnabledNetworkOfferingWithDefaultEgressAllowed =
_networkOfferingDao.persistDefaultNetworkOffering(defaultIsolatedSourceNatEnabledNetworkOfferingWithDefaultEgressAllowed);
Review Comment:
The upgrade-time creation path (NetworkOrchestrator) sets additional
properties on this offering (e.g., `dedicatedLB`, `eipAssociatePublicIp`,
`keepAliveEnabled`, `multicastRateMbps`), but the fresh-install path here only
sets `state`, `supportsVmAutoScaling`, and `egressDefaultPolicy`. This will
produce different defaults depending on whether the offering is created during
fresh install vs. added later. To avoid configuration drift, align the two
creation paths by applying the same set of fields in both places (or refactor
into a shared helper that configures the offering consistently before
persisting/updating).
##########
server/src/main/java/com/cloud/server/ConfigurationServerImpl.java:
##########
@@ -1070,12 +1070,29 @@ public void
doInTransactionWithoutResult(TransactionStatus status) {
defaultIsolatedSourceNatEnabledNetworkOffering.setSupportsVmAutoScaling(true);
defaultIsolatedSourceNatEnabledNetworkOffering =
_networkOfferingDao.persistDefaultNetworkOffering(defaultIsolatedSourceNatEnabledNetworkOffering);
+ // Offering #3-2
+ NetworkOfferingVO
defaultIsolatedSourceNatEnabledNetworkOfferingWithDefaultEgressAllowed =
+ new
NetworkOfferingVO(NetworkOffering.DefaultIsolatedNetworkOfferingWithSourceNatServiceEgressAllowed,
+ "Offering for Isolated networks with Source
Nat service enabled and egress traffic allowed by default", TrafficType.Guest,
false, false, null, null, true, Availability.Optional, null,
+ Network.GuestType.Isolated, true, false,
false, false, true, false);
+
+
defaultIsolatedSourceNatEnabledNetworkOfferingWithDefaultEgressAllowed.setState(NetworkOffering.State.Enabled);
+
defaultIsolatedSourceNatEnabledNetworkOfferingWithDefaultEgressAllowed.setSupportsVmAutoScaling(true);
+
defaultIsolatedSourceNatEnabledNetworkOfferingWithDefaultEgressAllowed.setEgressDefaultPolicy(true);
+
defaultIsolatedSourceNatEnabledNetworkOfferingWithDefaultEgressAllowed =
_networkOfferingDao.persistDefaultNetworkOffering(defaultIsolatedSourceNatEnabledNetworkOfferingWithDefaultEgressAllowed);
+
for (Service service :
defaultIsolatedSourceNatEnabledNetworkOfferingProviders.keySet()) {
NetworkOfferingServiceMapVO offService =
new
NetworkOfferingServiceMapVO(defaultIsolatedSourceNatEnabledNetworkOffering.getId(),
service,
defaultIsolatedSourceNatEnabledNetworkOfferingProviders.get(service));
_ntwkOfferingServiceMapDao.persist(offService);
- logger.trace("Added service for the network offering: " +
offService);
+ logger.trace("Added service {} for the network offering:
{}", offService,
defaultIsolatedSourceNatEnabledNetworkOffering.getUniqueName());
+
+ offService =
+ new
NetworkOfferingServiceMapVO(defaultIsolatedSourceNatEnabledNetworkOfferingWithDefaultEgressAllowed.getId(),
service,
+
defaultIsolatedSourceNatEnabledNetworkOfferingProviders.get(service));
+ _ntwkOfferingServiceMapDao.persist(offService);
+ logger.trace("Added service {} for the network offering:
{}", offService,
defaultIsolatedSourceNatEnabledNetworkOfferingWithDefaultEgressAllowed.getUniqueName());
Review Comment:
These trace logs say \"Added service {}\" but the placeholder is populated
with the full `NetworkOfferingServiceMapVO` (`offService`) rather than the
`service` being iterated. This is misleading for log readers and makes grepping
for a specific service harder. Consider logging `service` (and optionally
provider) explicitly, e.g., log the `service` variable plus the offering unique
name, and keep `offService` only if you specifically want to dump the whole
mapping.
--
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]
