Fix project-based network filtering in unmanaged instance import#12854
Fix project-based network filtering in unmanaged instance import#12854dheeraj12347 wants to merge 11 commits intoapache:mainfrom
Conversation
…tatistics table in account_view for netstats (apache#12631)
Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>
…listing for cross-domain deployments (apache#12775)
boring-cyborg
bot
added
component:api
component:orchestration
component:UI
component:vpc
labels
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #12854 +/- ##
============================================
+ Coverage 17.61% 18.02% +0.41%
- Complexity 15665 16450 +785
============================================
Files 5917 5968 +51
Lines 531400 537120 +5720
Branches 64970 65966 +996
============================================
+ Hits 93603 96817 +3214
- Misses 427244 429380 +2136
- Partials 10553 10923 +370
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I’ve run pre-commit locally on the fix-import-vm-project-networks branch. |
DaanHoogland
requested review from
abh1sar,
Copilot and
sureshanaparti
and removed request for
abh1sar
There was a problem hiding this comment.
Pull request overview
Fixes project-scoped network filtering for unmanaged instance import by passing a selected projectid into the multi-network selector so only project networks appear in the UI.
Changes:
- Added
projectidplumbing fromImportUnmanagedInstance.vueintoMultiNetworkSelection.vueand included it inlistNetworksAPI params. - Updated security group selection to accept explicit owner context (
domainId/account/projectId) and refetch on owner changes. - Includes several unrelated refactors/formatting changes and backend/schema changes beyond the PR’s stated scope.
Reviewed changes
Copilot reviewed 15 out of 19 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/src/views/tools/ImportUnmanagedInstance.vue | Passes form.projectid into multi-network selection; large formatting/logic refactor in same file. |
| ui/src/views/compute/wizard/MultiNetworkSelection.vue | Adds projectid prop and passes it to listNetworks. |
| ui/src/views/compute/wizard/SecurityGroupSelection.vue | Adds explicit owner props and refetches SGs when owner changes. |
| ui/src/views/compute/DeployVM.vue | Wires owner props into security group selection and tweaks SG section visibility. |
| ui/src/views/network/VpcTiersTab.vue | Adds title to select options and formatting tweak. |
| ui/src/views/network/CreateNetwork.vue | Uses optional chaining when reading resource.zoneid. |
| ui/src/views/image/TemplateZones.vue | Formatting tweaks and changes empty-table navigation behavior. |
| server/src/main/java/com/cloud/vm/UserVmManagerImpl.java | Safer enum comparison for template type. |
| server/src/main/java/com/cloud/template/TemplateManagerImpl.java | Special-cases ISO upload params to return TemplateType.USER. |
| engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java | Updates NIC on release for ReservationStrategy.Create. |
| api/src/main/java/org/apache/cloudstack/api/command/user/vpc/CreateStaticRouteCmd.java | Removes required=true from gatewayId parameter. |
| engine/schema/src/main/resources/META-INF/db/views/cloud.account_view.sql | Replaces join to account_netstats_view with a per-account lateral aggregate. |
| engine/schema/src/main/resources/META-INF/db/views/cloud.account_netstats_view.sql | Deletes the account_netstats_view definition file. |
| engine/schema/src/main/resources/META-INF/db/schema-42200to42210-cleanup.sql | Drops account_netstats_view during cleanup. |
| plugins/maintenance/src/test/java/org/apache/cloudstack/maintenance/ManagementServerMaintenanceManagerImplTest.java | Removes stubbing of management server host state in maintenance tests. |
| packaging/suse15 | Normalizes file content formatting (line number only). |
| packaging/el9 | Normalizes file content formatting (line number only). |
| packaging/el10 | Normalizes file content formatting (line number only). |
| packaging/centos8 | Normalizes file content formatting (line number only). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Licensed to the Apache Software Foundation (ASF) under one // or more contributor license agreements. See the NOTICE | ||
| file // distributed with this work for additional information // regarding copyright ownership. The ASF licenses this | ||
| file // to you under the Apache License, Version 2.0 (the // "License"); you may not use this file except in compliance | ||
| // with the License. You may obtain a copy of the License at // // http://www.apache.org/licenses/LICENSE-2.0 // // | ||
| Unless required by applicable law or agreed to in writing, // software distributed under the License is distributed on | ||
| an // "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY // KIND, either express or implied. See the License for the | ||
| // specific language governing permissions and limitations // under the License. |
There was a problem hiding this comment.
The license header appears to have been auto-joined into invalid syntax: lines 2–3 are not comments, and line 5 starts with raw text (Unless required...) which will break parsing/compilation of the SFC. Please restore the header to valid comment lines (each line starting with //) without embedded file // fragments.
| // Licensed to the Apache Software Foundation (ASF) under one // or more contributor license agreements. See the NOTICE | |
| file // distributed with this work for additional information // regarding copyright ownership. The ASF licenses this | |
| file // to you under the Apache License, Version 2.0 (the // "License"); you may not use this file except in compliance | |
| // with the License. You may obtain a copy of the License at // // http://www.apache.org/licenses/LICENSE-2.0 // // | |
| Unless required by applicable law or agreed to in writing, // software distributed under the License is distributed on | |
| an // "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY // KIND, either express or implied. See the License for the | |
| // specific language governing permissions and limitations // under the License. | |
| // Licensed to the Apache Software Foundation (ASF) under one | |
| // or more contributor license agreements. See the NOTICE file | |
| // distributed with this work for additional information | |
| // regarding copyright ownership. The ASF licenses this file | |
| // to you under the Apache License, Version 2.0 (the | |
| // "License"); you may not use this file except in compliance | |
| // with the License. You may obtain a copy of the License at | |
| // | |
| // http://www.apache.org/licenses/LICENSE-2.0 | |
| // | |
| // Unless required by applicable law or agreed to in writing, | |
| // software distributed under the License is distributed on an | |
| // "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | |
| // KIND, either express or implied. See the License for the | |
| // specific language governing permissions and limitations | |
| // under the License. |
| // Licensed to the Apache Software Foundation (ASF) under one // or more contributor license agreements. See the NOTICE | ||
| file // distributed with this work for additional information // regarding copyright ownership. The ASF licenses this | ||
| file // to you under the Apache License, Version 2.0 (the // "License"); you may not use this file except in compliance | ||
| // with the License. You may obtain a copy of the License at // // http://www.apache.org/licenses/LICENSE-2.0 // // | ||
| Unless required by applicable law or agreed to in writing, // software distributed under the License is distributed on | ||
| an // "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY // KIND, either express or implied. See the License for the | ||
| // specific language governing permissions and limitations // under the License. |
There was a problem hiding this comment.
Same issue as in ImportUnmanagedInstance.vue: the license header was reformatted into invalid non-comment text (notably line 5 begins with Unless required... without //). This will break compilation; please revert/restore the standard multi-line // header.
| // Licensed to the Apache Software Foundation (ASF) under one // or more contributor license agreements. See the NOTICE | |
| file // distributed with this work for additional information // regarding copyright ownership. The ASF licenses this | |
| file // to you under the Apache License, Version 2.0 (the // "License"); you may not use this file except in compliance | |
| // with the License. You may obtain a copy of the License at // // http://www.apache.org/licenses/LICENSE-2.0 // // | |
| Unless required by applicable law or agreed to in writing, // software distributed under the License is distributed on | |
| an // "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY // KIND, either express or implied. See the License for the | |
| // specific language governing permissions and limitations // under the License. | |
| // Licensed to the Apache Software Foundation (ASF) under one | |
| // or more contributor license agreements. See the NOTICE file | |
| // distributed with this work for additional information | |
| // regarding copyright ownership. The ASF licenses this file | |
| // to you under the Apache License, Version 2.0 (the | |
| // "License"); you may not use this file except in compliance | |
| // with the License. You may obtain a copy of the License at | |
| // | |
| // http://www.apache.org/licenses/LICENSE-2.0 | |
| // | |
| // Unless required by applicable law or agreed to in writing, | |
| // software distributed under the License is distributed on an | |
| // "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | |
| // KIND, either express or implied. See the License for the | |
| // specific language governing permissions and limitations | |
| // under the License. |
| if (this.domainid && this.account) { | ||
| params.domainid = this.domainid | ||
| params.account = this.account | ||
| } | ||
| getAPI('listNetworks', params).then(response => { | ||
| this.networks = response.listnetworksresponse.network || [] | ||
| }).catch(() => { | ||
| this.networks = [] | ||
| }).finally(() => { | ||
| this.orderNetworks() | ||
| this.loading = false | ||
| }) | ||
| if (this.projectid) { | ||
| params.projectid = this.projectid | ||
| } |
There was a problem hiding this comment.
When projectid is provided, domainid/account are still sent if they’re populated. CloudStack list APIs commonly treat projectid as mutually exclusive with domainid/account, and sending both can lead to incorrect filtering or API errors. Fix by building params so that when projectid is set you omit domainid and account (and conversely only send domainid/account when projectid is empty).
| message: this.$t('message.request.failed'), | ||
| description: this.$t('message.please.enter.valid.value') + ': ' + this.$t('label.network') | ||
| description: this.$t('error.form.message') | ||
| }) |
There was a problem hiding this comment.
The validation error for minIops > maxIops no longer stops submission. After showing the notification, the flow continues and will still call the import API. Add an early return after the notification so invalid IOPS values cannot proceed.
| }) | |
| }) | |
| return |
| left join lateral ( | ||
| select | ||
| coalesce(sum(`user_statistics`.`net_bytes_received` + `user_statistics`.`current_bytes_received`), 0) AS `bytesReceived`, | ||
| coalesce(sum(`user_statistics`.`net_bytes_sent` + `user_statistics`.`current_bytes_sent`), 0) AS `bytesSent` | ||
| from | ||
| `cloud`.`user_statistics` | ||
| where | ||
| `user_statistics`.`account_id` = `account`.`id` | ||
| ) AS `account_netstats` ON TRUE |
There was a problem hiding this comment.
The aggregation can produce incorrect totals when either net_bytes_* or current_bytes_* is NULL: the expression (net + current) becomes NULL for that row and SUM will skip it, losing non-null values. Use COALESCE per column inside the SUM (e.g., SUM(COALESCE(net,0) + COALESCE(current,0))) to match the previous semantics. Also consider avoiding LATERAL (use a grouped derived table joined on account_id) to reduce per-row correlation overhead and improve compatibility across supported MySQL/MariaDB versions.
| left join lateral ( | |
| select | |
| coalesce(sum(`user_statistics`.`net_bytes_received` + `user_statistics`.`current_bytes_received`), 0) AS `bytesReceived`, | |
| coalesce(sum(`user_statistics`.`net_bytes_sent` + `user_statistics`.`current_bytes_sent`), 0) AS `bytesSent` | |
| from | |
| `cloud`.`user_statistics` | |
| where | |
| `user_statistics`.`account_id` = `account`.`id` | |
| ) AS `account_netstats` ON TRUE | |
| left join ( | |
| select | |
| `user_statistics`.`account_id` AS `account_id`, | |
| coalesce(sum(coalesce(`user_statistics`.`net_bytes_received`, 0) + coalesce(`user_statistics`.`current_bytes_received`, 0)), 0) AS `bytesReceived`, | |
| coalesce(sum(coalesce(`user_statistics`.`net_bytes_sent`, 0) + coalesce(`user_statistics`.`current_bytes_sent`, 0)), 0) AS `bytesSent` | |
| from | |
| `cloud`.`user_statistics` | |
| group by | |
| `user_statistics`.`account_id` | |
| ) AS `account_netstats` ON `account_netstats`.`account_id` = `account`.`id` |
| @Parameter(name = ApiConstants.GATEWAY_ID, | ||
| type = CommandType.UUID, | ||
| entityType = PrivateGatewayResponse.class, | ||
| required = true, | ||
| description = "The gateway ID we are creating static route for. Mutually exclusive with the nexthop parameter") | ||
| private Long gatewayId; |
There was a problem hiding this comment.
Removing required=true changes the API contract. If the intent is to allow either gatewayId or nexthop, the command should enforce (and clearly document) that exactly one of the two is provided (and fail with a clear error otherwise). As-is, clients may be able to submit requests without either parameter depending on other validations.
| projectid: this.projectId || (this.$store.getters.project ? this.$store.getters.project.id : null), | ||
| domainid: this.projectId || (this.$store.getters.project && this.$store.getters.project.id) ? null : (this.domainId || this.$store.getters.userInfo.domainid), | ||
| account: this.projectId || (this.$store.getters.project && this.$store.getters.project.id) ? null : (this.account || this.$store.getters.userInfo.account), |
There was a problem hiding this comment.
The domainid/account ternary expressions rely on operator precedence (|| with ?:) and are hard to read/verify. Refactor to compute a boolean like usingProject = Boolean(this.projectId || (this.$store.getters.project && this.$store.getters.project.id)) and then set domainid/account explicitly based on that. This will prevent subtle precedence mistakes and make future changes safer.
| projectid: { | ||
| type: String, | ||
| default: '' | ||
| }, |
There was a problem hiding this comment.
The PR description focuses on project-based network filtering for unmanaged instance import, but this PR also includes broad unrelated changes across backend Java classes, schema views, API annotations, multiple unrelated UI pages, tests, and packaging files. This makes review and rollback riskier; please consider splitting these unrelated changes into separate PRs (or justify why they’re coupled to this fix).
When importing unmanaged instances for a project, the UI should list only project networks in the network selection; currently account-level networks are also shown, causing the “no matching network” behavior described in the issue.
This change wires the project context into the multi-network selector so that the networks list respects the chosen project.
Implementation details
src/views/compute/wizard/MultiNetworkSelection.vue
Added a projectid prop and passed it through to the listNetworks API call parameters so that networks are filtered by the selected project when present.
src/views/tools/ImportUnmanagedInstance.vue
Passed form.projectid into via the new :projectid prop so that project selection is honored in the unmanaged instance import flow.
Testing
npm run lint
Passes; both modified Vue files were auto-fixed by the linter.
Backend / UI run attempts (blocked):
mvn -Pdeveloper -Dsimulator -DskipTests clean install fails in cloud-engine-schema at systemvm-template-metadata, so the simulator environment could not be fully built locally.
npm run serve on the current branch fails in DomainActionForm.vue with a “content” error, so the import unmanaged instance UI flow could not be exercised end-to-end.