Copilot commented on code in PR #13325:
URL: https://github.com/apache/cloudstack/pull/13325#discussion_r4118917287
##########
engine/schema/src/main/resources/META-INF/db/schema-42300to2400.sql:
##########
@@ -18,3 +18,7 @@
--;
-- Schema upgrade from 4.23.0.0 to 24.0.0
--;
+
+ALTER TABLE `cloud`.`nics` ADD COLUMN `network_rate` int DEFAULT NULL COMMENT
'effective network rate in Mb/s for this NIC, -1 means unlimited';
+
+ALTER TABLE `cloud`.`vpc_offerings` ADD COLUMN `public_nw_rate` smallint
unsigned DEFAULT NULL COMMENT 'public gateway (internet-facing) network rate
throttle mbits/s';
Review Comment:
`public_nw_rate` is declared as `smallint unsigned` (maximum 65,535), but
the create/update validation only rejects negative values. A larger `Integer`
is therefore accepted and will either fail under strict MySQL or be clamped,
unlike the existing rate columns that were widened to `int unsigned` in
`schema-42210to42300.sql:486-493`. Use `int unsigned` here or enforce the
column's upper bound before persisting.
##########
engine/schema/src/main/resources/META-INF/db/schema-42300to2400.sql:
##########
@@ -18,3 +18,7 @@
--;
-- Schema upgrade from 4.23.0.0 to 24.0.0
--;
+
+ALTER TABLE `cloud`.`nics` ADD COLUMN `network_rate` int DEFAULT NULL COMMENT
'effective network rate in Mb/s for this NIC, -1 means unlimited';
+
+ALTER TABLE `cloud`.`vpc_offerings` ADD COLUMN `public_nw_rate` smallint
unsigned DEFAULT NULL COMMENT 'public gateway (internet-facing) network rate
throttle mbits/s';
Review Comment:
This upgrade script adds the new columns only for existing databases, but
the canonical fresh-install schema still defines `nics` without `network_rate`
and `vpc_offerings` without `public_nw_rate`
(`setup/db/create-schema.sql:291-320` and `2340-2354`). A fresh deployment will
therefore fail when the ORM/API tries to persist or query these fields. Add
both columns to the fresh-install schema as well as this upgrade path.
##########
server/src/main/java/com/cloud/api/query/dao/VpcOfferingJoinDaoImpl.java:
##########
@@ -78,6 +78,8 @@ public VpcOfferingResponse newVpcOfferingResponse(VpcOffering
offering) {
offeringResponse.setSpecifyAsNumber(offering.isSpecifyAsNumber());
}
offeringResponse.setConserveMode(offering.isConserveMode());
+ Integer pubNetworkRate = offering.getPublicNetworkRate();
+ offeringResponse.setPublicNetworkRate((pubNetworkRate == null ||
pubNetworkRate <= 0 ) ? -1 : pubNetworkRate);
Review Comment:
This converts an unset offering value to `-1`, but `UpdateVPCOfferingCmd`
rejects negative rates and the UI includes `publicnetworkrate` in edit
requests. As a result, editing any offering whose rate is unset/defaulted can
fail with “specify the public network rate value as 0 or more”; it also
contradicts the response contract that says unset should remain null and fall
back to the zone/global setting. Preserve null here (and only normalize an
explicitly configured 0 if that is the intended display value).
##########
server/src/main/java/com/cloud/network/vpc/VpcManagerImpl.java:
##########
@@ -2810,6 +2839,7 @@ public boolean restartVpc(Long vpcId, boolean cleanUp,
boolean makeRedundant, bo
// the restart procedure.
if (vpcDao.update(vpc.getId(), entity)) {
vpc = entity;
+ saveVpcNetworkRateInDetails(vpc);
Review Comment:
This refresh is inside the `makeRedundant` branch, so an ordinary
`restartVpc` never rewrites the `vpc_details.publicnetworkrate` snapshot. After
a zone-default or offering-rate change and a normal restart, the router can use
the recomputed cap while `VpcResponse` continues to expose the old value.
Refresh this detail on every restart that reapplies the gateway configuration.
--
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]
