Copilot commented on code in PR #14286:
URL: https://github.com/apache/cloudstack/pull/14286#discussion_r4324174045
##########
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:
{}", service, defaultIsolatedSourceNatEnabledNetworkOffering.getUniqueName());
+
+ offService =
+ new
NetworkOfferingServiceMapVO(defaultIsolatedSourceNatEnabledNetworkOfferingWithDefaultEgressAllowed.getId(),
service,
+
defaultIsolatedSourceNatEnabledNetworkOfferingProviders.get(service));
+ _ntwkOfferingServiceMapDao.persist(offService);
+ logger.trace("Added service {} for the network offering:
{}", service,
defaultIsolatedSourceNatEnabledNetworkOfferingWithDefaultEgressAllowed.getUniqueName());
Review Comment:
This switches logging to SLF4J-style `{}` placeholders with varargs. If this
class uses Log4j (common in CloudStack), `logger.trace(String, Object, Object)`
won’t compile / won’t log as intended. Please keep the logging call consistent
with the logger type used in this class (e.g., string concatenation for Log4j,
or ensure the logger is SLF4J if placeholders are desired).
##########
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);
+ _networkOfferingDao.update(offering.getId(), offering);
Review Comment:
Creating the offering and then immediately updating it relies on a second
write and makes it harder to reason about the effective defaults (especially
since the comment claims parity with `ConfigurationServerImpl`, but parity
currently depends on this post-update). Since you’ve added setters on
`NetworkOfferingVO` to set fields prior to persistence, consider pushing these
defaults into the creation/persistence path (e.g., extend
`createNetworkOffering` to accept egress default policy / related flags, or set
the VO fields before it’s persisted) to keep defaults centralized and avoid a
follow-up DAO update.
##########
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 properties applied to this offering on fresh install don’t match what
`NetworkOrchestrator` later enforces for existing zones (e.g., dedicated LB,
EIP associate public IP, keep-alive, multicast rate). This can lead to behavior
differences between newly installed vs upgraded environments. Please align the
two paths by setting the same fields here before persisting (or remove the
post-create adjustments in `NetworkOrchestrator` and rely on a single source of
truth).
--
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]
