Copilot commented on code in PR #13791:
URL: https://github.com/apache/cloudstack/pull/13791#discussion_r3718892747
##########
ui/src/views/offering/AddNetworkOffering.vue:
##########
@@ -160,6 +160,38 @@
</a-form-item>
</a-col>
</a-row>
+ <a-row :gutter="12" v-if="form.provider === 'NSX'">
+ <a-col :md="8" :lg="8">
+ <a-form-item name="nsxipdiscoveryprofileid"
ref="nsxipdiscoveryprofileid">
+ <template #label>
+ <tooltip-label
+ :title="$t('label.nsx.ip.discovery.profile.id')"
+
:tooltip="$t('message.network.offering.nsx.ip.discovery.profile.id')"/>
+ </template>
+ <a-input v-model:value="form.nsxipdiscoveryprofileid"
:maxlength="255" />
+ </a-form-item>
+ </a-col>
+ <a-col :md="8" :lg="8">
+ <a-form-item name="nsxmacdiscoveryprofileid"
ref="nsxmacdiscoveryprofileid">
+ <template #label>
+ <tooltip-label
+ :title="$t('label.nsx.mac.discovery.profile.id')"
+
:tooltip="$t('message.network.offering.nsx.mac.discovery.profile.id')"/>
+ </template>
+ <a-input v-model:value="form.nsxmacdiscoveryprofileid"
:maxlength="255" />
+ </a-form-item>
+ </a-col>
+ <a-col :md="8" :lg="8">
+ <a-form-item name="nsxsegmentsecurityprofileid"
ref="nsxsegmentsecurityprofileid">
+ <template #label>
+ <tooltip-label
+ :title="$t('label.nsx.segment.security.profile.id')"
+
:tooltip="$t('message.network.offering.nsx.segment.security.profile.id')"/>
+ </template>
+ <a-input v-model:value="form.nsxsegmentsecurityprofileid"
:maxlength="255" />
+ </a-form-item>
+ </a-col>
+ </a-row>
Review Comment:
This NSX profile block is duplicated in both AddNetworkOffering.vue and
CloneNetworkOffering.vue. Consider extracting it into a shared component (or a
shared render/helper) to reduce duplication and prevent future drift (e.g.,
labels, max length, validation rules).
##########
ui/src/views/offering/CloneNetworkOffering.vue:
##########
@@ -1392,6 +1430,20 @@ export default {
params['details[' + detailsIndex + '].value'] = values.maclearning
detailsIndex++
}
+ if (values.nsxipdiscoveryprofileid) {
+ params['details[' + detailsIndex + '].key'] =
'nsxipdiscoveryprofileid'
+ params['details[' + detailsIndex + '].value'] =
values.nsxipdiscoveryprofileid
+ detailsIndex++
+ }
+ if (values.nsxmacdiscoveryprofileid) {
+ params['details[' + detailsIndex + '].key'] =
'nsxmacdiscoveryprofileid'
+ params['details[' + detailsIndex + '].value'] =
values.nsxmacdiscoveryprofileid
+ detailsIndex++
+ }
+ if (values.nsxsegmentsecurityprofileid) {
+ params['details[' + detailsIndex + '].key'] =
'nsxsegmentsecurityprofileid'
+ params['details[' + detailsIndex + '].value'] =
values.nsxsegmentsecurityprofileid
+ }
Review Comment:
The `detailsIndex` counter isn’t incremented after adding
`nsxsegmentsecurityprofileid`. It works today because no further details are
appended below, but it’s fragile and can lead to overwriting if additional
details are added later. Increment `detailsIndex` after setting this value for
consistency with the other fields.
##########
server/src/main/java/com/cloud/network/NetworkServiceImpl.java:
##########
@@ -4230,6 +4236,20 @@ protected boolean canUpgrade(Network network, long
oldNetworkOfferingId, long ne
return canMoveToPhysicalNetwork(network, oldNetworkOfferingId,
newNetworkOfferingId);
}
+ protected boolean haveMatchingNsxSegmentProfiles(long
oldNetworkOfferingId, long newNetworkOfferingId) {
+ Map<NetworkOffering.Detail, String> oldDetails =
_networkModel.getNtwkOffDetails(oldNetworkOfferingId);
+ Map<NetworkOffering.Detail, String> newDetails =
_networkModel.getNtwkOffDetails(newNetworkOfferingId);
+ for (NetworkOffering.Detail detail :
List.of(NetworkOffering.Detail.NsxIpDiscoveryProfileId,
+ NetworkOffering.Detail.NsxMacDiscoveryProfileId,
NetworkOffering.Detail.NsxSegmentSecurityProfileId)) {
Review Comment:
This allocates a new `List` on every call to
`haveMatchingNsxSegmentProfiles`. Consider using a static constant collection
(or a small fixed array) to avoid repeated allocations on a potentially hot
path (network offering upgrades/checks).
##########
plugins/network-elements/nsx/src/main/java/org/apache/cloudstack/service/NsxApiClient.java:
##########
@@ -489,6 +532,76 @@ public void createSegment(String segmentName, String
tier1GatewayName, String ga
}
}
+ protected String getIpDiscoveryProfilePath(String profileId) {
+ if (StringUtils.isBlank(profileId)) {
+ return null;
+ }
+ IpDiscoveryProfiles profiles = (IpDiscoveryProfiles)
nsxService.apply(IpDiscoveryProfiles.class);
+ IPDiscoveryProfile profile = profiles.get(profileId);
+ return validateProfile(profileId, profile.getId(), profile.getPath(),
"/infra/ip-discovery-profiles/", profile.getMarkedForDelete());
+ }
Review Comment:
There is test coverage for invalid returned profile data (bad path,
different ID, markedForDelete), but there’s no test for the real ‘profile does
not exist’ path where `profiles.get(profileId)` fails with an NSX NotFound/404
(vAPI error). Add a unit test that mocks `IpDiscoveryProfiles.get(\"...\")` to
throw the NotFound-equivalent error and assert `createSegment()` throws a clean
CloudRuntimeException and does not call `Segments.patch()` nor `Infra.patch()`.
##########
plugins/network-elements/nsx/src/test/java/org/apache/cloudstack/service/NsxApiClientTest.java:
##########
@@ -22,22 +22,43 @@
import com.vmware.nsx.cluster.Status;
import com.vmware.nsx.model.ClusterStatus;
import com.vmware.nsx.model.ControllerClusterStatus;
+import com.vmware.nsx_policy.Infra;
+import com.vmware.nsx_policy.infra.IpDiscoveryProfiles;
import com.vmware.nsx_policy.infra.LbAppProfiles;
import com.vmware.nsx_policy.infra.LbMonitorProfiles;
import com.vmware.nsx_policy.infra.LbPools;
import com.vmware.nsx_policy.infra.LbServices;
import com.vmware.nsx_policy.infra.LbVirtualServers;
+import com.vmware.nsx_policy.infra.MacDiscoveryProfiles;
+import com.vmware.nsx_policy.infra.SegmentSecurityProfiles;
+import com.vmware.nsx_policy.infra.Segments;
import com.vmware.nsx_policy.infra.domains.Groups;
+import com.vmware.nsx_policy.infra.tier_1s.ipsec_vpn_services.Sessions;
+import com.vmware.nsx_policy.infra.segments.SegmentDiscoveryProfileBindingMaps;
+import com.vmware.nsx_policy.infra.segments.SegmentSecurityProfileBindingMaps;
Review Comment:
These imports appear to be unused in the updated test file. Removing unused
imports will reduce noise and avoid potential failures if the project enforces
import-usage via static analysis (e.g., Checkstyle/Spotless).
##########
plugins/network-elements/nsx/src/test/java/org/apache/cloudstack/service/NsxApiClientTest.java:
##########
@@ -22,22 +22,43 @@
import com.vmware.nsx.cluster.Status;
import com.vmware.nsx.model.ClusterStatus;
import com.vmware.nsx.model.ControllerClusterStatus;
+import com.vmware.nsx_policy.Infra;
+import com.vmware.nsx_policy.infra.IpDiscoveryProfiles;
import com.vmware.nsx_policy.infra.LbAppProfiles;
import com.vmware.nsx_policy.infra.LbMonitorProfiles;
import com.vmware.nsx_policy.infra.LbPools;
import com.vmware.nsx_policy.infra.LbServices;
import com.vmware.nsx_policy.infra.LbVirtualServers;
+import com.vmware.nsx_policy.infra.MacDiscoveryProfiles;
+import com.vmware.nsx_policy.infra.SegmentSecurityProfiles;
+import com.vmware.nsx_policy.infra.Segments;
import com.vmware.nsx_policy.infra.domains.Groups;
+import com.vmware.nsx_policy.infra.tier_1s.ipsec_vpn_services.Sessions;
+import com.vmware.nsx_policy.infra.segments.SegmentDiscoveryProfileBindingMaps;
+import com.vmware.nsx_policy.infra.segments.SegmentSecurityProfileBindingMaps;
import com.vmware.nsx_policy.model.ApiError;
+import com.vmware.nsx_policy.model.ChildSegment;
+import com.vmware.nsx_policy.model.ChildSegmentDiscoveryProfileBindingMap;
+import com.vmware.nsx_policy.model.ChildSegmentSecurityProfileBindingMap;
import com.vmware.nsx_policy.model.Group;
+import com.vmware.nsx_policy.model.IPDiscoveryProfile;
import com.vmware.nsx_policy.model.LBAppProfileListResult;
import com.vmware.nsx_policy.model.LBIcmpMonitorProfile;
import com.vmware.nsx_policy.model.LBService;
import com.vmware.nsx_policy.model.LBTcpMonitorProfile;
import com.vmware.nsx_policy.model.LBPool;
import com.vmware.nsx_policy.model.LBPoolMember;
import com.vmware.nsx_policy.model.LBVirtualServer;
+import com.vmware.nsx_policy.model.MacDiscoveryProfile;
import com.vmware.nsx_policy.model.PathExpression;
+import com.vmware.nsx_policy.model.Segment;
+import com.vmware.nsx_policy.model.SegmentDiscoveryProfileBindingMap;
+import com.vmware.nsx_policy.model.SegmentSecurityProfile;
+import com.vmware.nsx_policy.model.StaticRoutesListResult;
+import com.vmware.nsx_policy.model.Tag;
+import com.vmware.nsx_policy.model.Tier1;
+import com.vmware.nsx_policy.model.TunnelInterfaceIPSubnet;
Review Comment:
These imports appear to be unused in the updated test file. Removing unused
imports will reduce noise and avoid potential failures if the project enforces
import-usage via static analysis (e.g., Checkstyle/Spotless).
--
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]
