shwstppr commented on code in PR #6426:
URL: https://github.com/apache/cloudstack/pull/6426#discussion_r905735670
##########
api/src/main/java/org/apache/cloudstack/api/ApiConstants.java:
##########
@@ -902,13 +902,19 @@ public class ApiConstants {
public static final String DYNAMIC_SCALING_ENABLED =
"dynamicscalingenabled";
public static final String POOL_TYPE = "pooltype";
+ public static final String REDUNDANT_STATE = "redundantstate";
public static final String ADMINS_ONLY = "adminsonly";
public static final String ANNOTATION_FILTER = "annotationfilter";
public static final String LOGIN = "login";
public static final String LOGOUT = "logout";
public static final String LIST_IDPS = "listIdps";
+ public static final String PUBLIC_MTU = "publicmtu";
+ public static final String PRIVATE_MTU = "privatemtu";
+ public static final String MTU = "mtu";
+
+ public static final Integer DEFAULT_MTU = 1500;
Review Comment:
@Pearl1594 should this a configuration or maybe defined in some
networkservice or something?
##########
api/src/main/java/org/apache/cloudstack/api/response/VpcResponse.java:
##########
@@ -136,6 +136,10 @@ public class VpcResponse extends
BaseResponseWithAnnotations implements Controll
@Param(description = "The routes for the network to ease adding route in
upstream router", since = "4.17.0")
private Set<Ipv6RouteResponse> ipv6Routes;
+ @SerializedName(ApiConstants.PUBLIC_MTU)
+ @Param(description = "MTU configured on the public interfaces of the VPC
VR")
Review Comment:
no `since` key here
##########
api/src/main/java/org/apache/cloudstack/api/command/user/network/UpdateNetworkCmd.java:
##########
@@ -84,6 +84,14 @@ public class UpdateNetworkCmd extends BaseAsyncCustomIdCmd
implements UserCmd {
@Parameter(name= ApiConstants.FORCED, type = CommandType.BOOLEAN,
description = "Setting this to true will cause a forced network update,",
authorized = {RoleType.Admin})
private Boolean forced;
+ @Parameter(name = ApiConstants.PUBLIC_MTU, type = CommandType.INTEGER,
+ description = "MTU to be configured on the network VR's public
facing interfaces")
+ private Integer publicMtu;
+
+ @Parameter(name = ApiConstants.PRIVATE_MTU, type = CommandType.INTEGER,
+ description = "MTU to be configured on the network VR's public
facing interfaces")
+ private Integer privateMtu;
Review Comment:
`since` key here
##########
engine/schema/src/main/java/com/cloud/upgrade/dao/Upgrade41700to41800.java:
##########
@@ -0,0 +1,87 @@
+// Licensed to the Apache Software Foundation (ASF) under one
Review Comment:
Why is this here? I guess Nicolas already added upgrade path in main?
##########
test/integration/smoke/test_network.py:
##########
@@ -2043,4 +2060,341 @@ def test_03_destroySharedNetwork(self):
len(nic_ip_address),
0,
"Failed to find the placeholder IP"
- )
\ No newline at end of file
+ )
+
+
+class TestNetworkWithMtuConfiguration(cloudstackTestCase):
Review Comment:
Just my opinion, this can be a class in a separate file and maybe a
component test. Smoke tests already take over 12 hours. Also in case of failure
for any test in this file all the tests will run again.
##########
engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java:
##########
@@ -993,16 +998,75 @@ public Pair<NicProfile, Integer> allocateNic(final
NicProfile requested, final N
}
deviceId = applyProfileToNic(vo, profile, deviceId);
-
+ if (vm.getType() == Type.DomainRouter) {
Review Comment:
Isn't it better to get the mtu value here and use it here and in ln
1009-1011 instead of querying router twice?
##########
api/src/main/java/org/apache/cloudstack/api/command/user/vpc/UpdateVPCCmd.java:
##########
@@ -59,6 +59,10 @@ public class UpdateVPCCmd extends BaseAsyncCustomIdCmd
implements UserCmd {
@Parameter(name = ApiConstants.FOR_DISPLAY, type = CommandType.BOOLEAN,
description = "an optional field, whether to the display the vpc to the end
user or not", since = "4.4", authorized = {RoleType.Admin})
private Boolean display;
+ @Parameter(name = ApiConstants.PUBLIC_MTU, type = CommandType.INTEGER,
+ description = "MTU to be configured on the network VR's public
facing interfaces")
+ private Integer publicMtu;
Review Comment:
Do we use this instead of network tier's value in case of VPCs? Are there
checks not allow user to input publicmtu in case of network teirs?
##########
engine/schema/src/main/java/com/cloud/vm/NicVO.java:
##########
@@ -330,7 +333,7 @@ public String toString() {
.append(reservationId)
.append("-")
.append(iPv4Address)
- .append("]")
+ .append(mtu).append("]")
Review Comment:
is it intentionally appended without a separator?
##########
core/src/main/java/com/cloud/agent/resource/virtualnetwork/VRScripts.java:
##########
@@ -78,5 +78,7 @@ public class VRScripts {
public static final String RETRIEVE_DIAGNOSTICS =
"get_diagnostics_files.py";
public static final String VR_FILE_CLEANUP = "cleanup.sh";
+ public static final String VR_UPDATE_MTU = "update_interface_config.sh";
Review Comment:
name of the script and variable doesn't complement
##########
core/src/main/java/com/cloud/agent/resource/virtualnetwork/VirtualRoutingResource.java:
##########
@@ -216,6 +225,51 @@ private Answer executeQueryCommand(NetworkElementCommand
cmd) {
}
}
+ private static String getRouterSshControlIp(NetworkElementCommand cmd) {
+ String routerIp = cmd.getAccessDetail(NetworkElementCommand.ROUTER_IP);
+ if (s_logger.isDebugEnabled())
+ s_logger.debug("Use router's private IP for SSH control. IP : " +
routerIp);
+ return routerIp;
+ }
+
+ private Answer execute(UpdateNetworkCommand cmd) {
+ IpAddressTO[] ipAddresses = cmd.getIpAddresses();
+ String routerIp = getRouterSshControlIp(cmd);
+ boolean finalResult = true;
+ for (IpAddressTO ipAddressTO : ipAddresses) {
+ try {
+ SubnetUtils util = new SubnetUtils(ipAddressTO.getPublicIp(),
ipAddressTO.getVlanNetmask());
+ String address = util.getInfo().getCidrSignature();
+ String subnet = address.split("/")[1];
+ ExecutionResult result = _vrDeployer.executeInVR(routerIp,
VRScripts.VR_UPDATE_MTU,
+ ipAddressTO.getPublicIp() + " " + subnet + " " +
ipAddressTO.getMtu() + " " + 15);
+ if (s_logger.isDebugEnabled())
+ s_logger.debug("result: " + result.isSuccess() + ",
output: " + result.getDetails());
+ if (!Boolean.TRUE.equals(result.isSuccess())) {
+ if (result.getDetails().contains(String.format("Interface
with IP %s not found", ipAddressTO.getPublicIp()))) {
+ s_logger.warn(String.format("Skipping IP: %s as it
isn't configured on router interface", ipAddressTO.getPublicIp()));
Review Comment:
It says skipping but it will still end up running code at ln258-259. Is this
okay?
##########
engine/schema/src/main/resources/META-INF/db/schema-41710to41800.sql:
##########
@@ -17,4 +17,115 @@
--;
-- Schema upgrade from 4.17.1.0 to 4.18.0.0
---;
\ No newline at end of file
+--;
+
+
+ALTER TABLE `cloud`.`networks` ADD COLUMN `public_iface_mtu` bigint unsigned
comment "MTU for VR public interface" ;
+ALTER TABLE `cloud`.`networks` ADD COLUMN `private_iface_mtu` bigint unsigned
comment "MTU for VR private interfaces" ;
+ALTER TABLE `cloud`.`vpc` ADD COLUMN `public_mtu` bigint unsigned comment "MTU
for VPC VR public interface" ;
Review Comment:
why this difference in naming convention for network vs vpc column
##########
ui/src/views/network/VpcTiersTab.vue:
##########
@@ -195,6 +195,16 @@
</a-select-option>
</a-select>
</a-form-item>
+ <a-form-item
+ ref="privatemtu"
+ name="privatemtu">
+ <template #label>
+ <tooltip-label :title="$t('label.privatemtu')"
:tooltip="$t('label.privatemtu')"/>
+ </template>
+ <a-input
Review Comment:
would it be better to use `a-input-number`?
##########
engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java:
##########
@@ -993,16 +998,75 @@ public Pair<NicProfile, Integer> allocateNic(final
NicProfile requested, final N
}
deviceId = applyProfileToNic(vo, profile, deviceId);
-
+ if (vm.getType() == Type.DomainRouter) {
+ setMtuDetailsInVRNic(vm.getId(), network, vo);
+ }
vo = _nicDao.persist(vo);
final Integer networkRate =
_networkModel.getNetworkRate(network.getId(), vm.getId());
final NicProfile vmNic = new NicProfile(vo, network,
vo.getBroadcastUri(), vo.getIsolationUri(), networkRate,
_networkModel.isSecurityGroupSupportedInNetwork(network),
_networkModel.getNetworkTag(vm.getHypervisorType(), network));
-
+ if (vm.getType() == Type.DomainRouter) {
+ setMtuInVRNicProfile(vm.getId(), network.getTrafficType(), vmNic);
+ }
return new Pair<NicProfile, Integer>(vmNic, Integer.valueOf(deviceId));
}
+ private void setMtuDetailsInVRNic(final long vmId, Network network, NicVO
vo) {
+ if (TrafficType.Public == network.getTrafficType()) {
+ Pair<NetworkVO, VpcVO> networks =
getGuestNetworkRouterAndVpcDetails(vmId);
+ if (networks == null) {
+ return;
+ }
+ NetworkVO networkVO = networks.first();
+ VpcVO vpcVO = networks.second();
+ if (vpcVO != null) {
+ vo.setMtu(vpcVO.getPublicMtu());
+ } else {
+ vo.setMtu(networkVO.getPublicIfaceMtu());
+ }
+ } else if (TrafficType.Guest == network.getTrafficType()) {
+ vo.setMtu(network.getPrivateIfaceMtu());
+ }
+ }
+
+ private void setMtuInVRNicProfile(final long vmId, TrafficType
trafficType, NicProfile vmNic) {
+ Pair<NetworkVO, VpcVO> networks =
getGuestNetworkRouterAndVpcDetails(vmId);
+ if (networks == null) {
+ return;
+ }
+ NetworkVO networkVO = networks.first();
+ VpcVO vpcVO = networks.second();
+ if (networkVO != null) {
+ if (TrafficType.Public == trafficType) {
+ if (vpcVO != null) {
+ vmNic.setMtu(vpcVO.getPublicMtu());
+ } else {
+ vmNic.setMtu(networkVO.getPublicIfaceMtu());
+ }
+ } else if (TrafficType.Guest == trafficType) {
+ vmNic.setMtu(networkVO.getPrivateIfaceMtu());
+ }
+ }
+ }
+
+ private Pair<NetworkVO, VpcVO> getGuestNetworkRouterAndVpcDetails(long
routerId) {
+ List<DomainRouterJoinVO> routerVo =
routerJoinDao.getRouterByIdAndTrafficType(routerId, TrafficType.Guest);
+ if (routerVo.isEmpty()) {
+ routerVo = routerJoinDao.getRouterByIdAndTrafficType(routerId,
TrafficType.Public);
+ if (routerVo.isEmpty()) {
+ return null;
+ }
+ }
Review Comment:
why do we need to get router with traffic type when we already have
routerId? I've no idea and just curious to know
--
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]
