Skip to content

[WIP] KVM: Support multi vlan trunk NICs - #14166

Draft
Pearl1594 wants to merge 22 commits into
mainfrom
support-multi-vlan-guestnet
Draft

Pearl1594 wants to merge 22 commits into
mainfrom
support-multi-vlan-guestnet

Conversation

@Pearl1594

Copy link
Copy Markdown
Contributor

Description

This PR adds support for multi-vlan trunk NICs
// TODO: details to be added

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (improves an existing feature and functionality)
  • Cleanup (Code refactoring and cleanup, that may add test cases)
  • Build/CI
  • Test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

  • Major
  • Minor

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

shwstppr and others added 2 commits September 14, 2026 15:29
Adds a 4.23.0 to 24.0.0 upgrade path (squashed from sb/upgradepath-424:
engine-schema: upgrade path for 24.0.0, fix CS version, fix upgrade
unit tests for cutover, fix imports).
@github-actions

Copy link
Copy Markdown

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

@codecov

codecov Bot commented Sep 14, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 3.71%. Comparing base (1a48a87) to head (b4c18d6).

❗ There is a different number of reports uploaded between BASE (1a48a87) and HEAD (b4c18d6). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (1a48a87) HEAD (b4c18d6)
unittests 1 0
Additional details and impacted files
@@              Coverage Diff              @@
##               main   #14166       +/-   ##
=============================================
- Coverage     19.91%    3.71%   -16.21%     
=============================================
  Files          6373      487     -5886     
  Lines        577230    41992   -535238     
  Branches      70696     7942    -62754     
=============================================
- Hits         114950     1558   -113392     
+ Misses       449713    40208   -409505     
+ Partials      12567      226    -12341     
Flag Coverage Δ
uitests 3.71% <ø> (ø)
unittests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions

github-actions Bot commented Sep 14, 2026 •

Copy link
Copy Markdown

🔴 Test Coverage Grade: D — Marginal

Metric Value
Line coverage 24.83%
Branch coverage 19.06%

Grade Scale

Grade Line Coverage Meaning
🟢 A ≥ 80% Excellent - this code sleeps well at night 😴
🟡 B 60-79% Good - almost there, don't stop now 😉
🟠 C 40-59% Acceptable - your code is wearing a seatbelt, but no airbags 😬
🔴 D 20-39% Marginal - boldly shipping where no test has gone before 🖖
⛔ F < 20% Failing - tests? what tests? 🔥

Branch coverage is shown as a secondary signal. Grade is determined by line coverage.
View full Actions run

@sonarqubecloud

sonarqubecloud Bot commented Sep 16, 2026 •

Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
37.8% Coverage on New Code (required ≥ 40%)

See analysis details on SonarQube Cloud

Comment thread engine/schema/src/main/java/com/cloud/upgrade/DatabaseUpgradeChecker.java Dismissed
Comment thread engine/schema/src/main/java/com/cloud/upgrade/dao/Upgrade42300to2400.java Dismissed
@github-actions

Copy link
Copy Markdown

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

The (nic_id, network_id) unique key rejected re-associating a network
after a prior disassociate, since the old row's uniqueness survives as
a soft-deleted (removed) row. Switch to a plain index so re-association
after disassociation works, relying on application-level checks (not
the DB) to reject a true duplicate live association.
…fallback

Trunk NIC VLAN membership is delivered by reusing the existing guest
bridge with vlan_filtering=1: the primary VLAN gets native/untagged
membership, associated VLANs get tagged-only membership, via libvirt's
<vlan trunk='yes'> element where supported, or manual `bridge vlan add`
on older libvirt. Adds a live membership update path
(UpdateNicVlanMembershipCommand/Answer + its KVM wrapper) so an
association change on a running VM is pushed to the host without a
restart, and a non-blocking diagnostic warning when the physical
uplink is missing a VLAN a trunk NIC needs (CloudStack no longer
manages the uplink's own VLAN membership - that is the operator's
responsibility).
- New UsageEventUtils.publishNicNetworkOfferingUsageEvents() emits one
  usage event per network a nic bills against (primary + any trunk
  associations), replacing the old single-event publishUsageEvent at
  every NIC-billing call site
- Ordinary non-trunk nics still produce exactly one event, unchanged
- New nullable usage_network_offering.network_id column disambiguates
  rows when a trunk nic's networks share one network offering
- Backfills network_id on a nic's own still-open usage row(s) the
  moment it first converts to a trunk nic; historic rows otherwise
  left alone
- New NicNetworkMapResponse + trunked/associatednetworks fields on
  NicResponse
- Populated in listNics, listVirtualMachines, and the
  associate/disassociate/change-primary command responses
- Trunk status derived from nic_network_map rows rather than a new
  column on the wide UserVmJoinVO view
- ASSOCIATED_NETWORKS/TRUNKED, needed by the previous commit's
  NicResponse fields
Core capability:
- New associateNetworkToNic/disassociateNetworkFromNic/changeNicPrimaryNetwork
  admin APIs and associateNetworksToNic (programmatic, used by deploy)
- A nic keeps one primary network plus any number of additional
  associated networks (nic_network_map), delivered as a VLAN trunk
- changeNicPrimaryNetwork only allowed while the Instance is stopped
- Rejects associating a network already reachable via another of the
  VM's nics, or with an overlapping CIDR to an existing association
- Disassociating a network is blocked while an active PF/Static
  NAT/LB rule targets its allocated IP

PF/Static NAT/LB support for associated networks:
- NetworkModel.getNicAndIpInNetwork resolves a nic/guest-ip pair via
  the nic's primary match or a trunk association, reused by
  RulesManagerImpl and LoadBalancingRulesManagerImpl so these rules
  can target a trunk nic's associated networks, not just its primary

Deployment planning:
- Hosts without VLAN filtering enabled are excluded up front for any
  VM with a multi-VLAN trunk nic, with a reactive fallback exclusion
  during start as a backstop
- Removing a nic cleans up any trunk associations it held as primary;
  deleting a network is blocked while still associated as a secondary

Metadata:
- A trunk nic's associated-network VLAN tags are exposed to the guest
  via the metadata service (nic-vlan-mapping file), gated by a new
  account-scoped config key, off by default
- EVENT_NIC_NETWORK_ASSOCIATE/DISASSOCIATE/PRIMARY_NETWORK_CHANGE,
  needed by the previous commit's @actionevent annotations
- New nicnetworkslist deploy param (nicnetworkslist[N].networkids,
  first id primary) lets a VM be deployed with trunk nics directly,
  mutually exclusive with networkids/iptonetworklist/vApp nicnetworklist
- Also carries per-entry ip4address/ip6address for the primary and
  ip4addresses/ip6addresses (comma-separated) for its associated
  networks - a network with no requested IP auto-allocates
- VNF: rejects a management-device nic from being requested as a
  trunk, both at deploy time and via the live associate API
- checkNoActiveRulesOnAssociation used findByIpAndNetworkId, which
  matches on a public IP's own address - the association's IP is a
  guest IP, so this never matched and the guard silently let
  disassociation through even with an active static NAT rule
- Use findByAssociatedVmIdAndVmIp instead, which matches the actual
  static NAT target mapping
- validateVnfApplianceTrunkNics only ran at deploy time
  (nicnetworkslist) - the live associateNetworkToNic/associateNetworksToNic
  path had no equivalent check, so a VNF's management nic could be
  trunked after deploy
- New single-nic validateVnfApplianceTrunkNic, called from
  associateNetworksInternal so both entry points are covered
- No-op for any non-VNF template
- changeNicPrimaryNetwork updated networkId and the IPv4/IPv6 address
  but left gateway, netmask, broadcast/isolation uri, and IPv6
  gateway/cidr pointing at the old primary network - wrong
  indefinitely, since nothing else ever recomputes them
- New applyNetworkAddressingToNic refreshes them from the new primary
  network, mirroring the field derivation used when a nic is first
  created
- disassociateNetworkFromNic now clears nics.multi_network once
  nic_network_map has no remaining rows for that nic
- Without this, listNics/listVirtualMachines kept reporting trunked=true
  and the deployment planner kept restricting the VM to VLAN-filtering-
  capable hosts even after all associations were removed
- New CommandSetupHelper.createDhcpEntryCommand overload takes addressing
  explicitly instead of reading it off a NicVO, since a nic's own DB row
  only ever carries its primary network's IP/gateway
- NetworkServiceImpl sends the entry directly to the associated network's
  router(s) on live associate/disassociate, bypassing the
  DhcpServiceProvider pipeline (which re-resolves the nic from the DB and
  so can never see anything but the primary IP)
- UserVmManagerImpl.finalizeStart converges a nic's associated-network
  DHCP entries with its current nic_network_map rows on every VM start,
  covering associations made while the VM was stopped and cleaning up
  entries for associations removed while stopped
- New NicNetworkMapDao.listRemovedByNicId to support that cleanup
- Destination-capability-driven interface XML rewrite during migration
  (LibvirtMigrateCommandWrapper): rewrites bridge name and trunk-vlan
  XML per nic based on the destination's own reported capability and
  bridge mapping, failing closed if that capability is unknown.
- Destination reports its own per-nic bridge name and VLAN-filtering/
  trunk-XML capability back to the source via PrepareForMigrationAnswer
  (LibvirtPrepareForMigrationCommandWrapper), rather than having the
  source guess it.
- New PostMigrationCommand path applies manual VLAN trunk membership on
  destinations whose libvirt can't express it via XML
  (LibvirtPostMigrationCommandWrapper).
- Host-readiness gating for migration destinations: a VM with a
  multi-VLAN trunk nic is rejected from migrating to a host without
  VLAN filtering enabled, both for explicit-host migration
  (UserVmManagerImpl) and for host listing/planner-driven migration
  (ManagementServerImpl, DeploymentPlanningManagerImpl).
- Shared VlanTrunkMigrationHelper so capability population and
  post-migration membership application work identically whether a VM
  migrates via the plain compute-only path (VirtualMachineManagerImpl)
  or via storage-motion (StorageSystemDataMotionStrategy,
  StorPoolDataMotionStrategy, LinstorDataMotionStrategy).
- Fix VM-start/add-nic VLAN membership application
  (LibvirtStartCommandWrapper, LibvirtPlugNicCommandWrapper) to cover
  any VLAN nic on a vlan_filtering-enabled host, not just trunk nics,
  matching the already-correct post-migration behavior.
…i-vlan-guestnet

# Conflicts:
#	engine/schema/src/main/java/com/cloud/upgrade/DatabaseUpgradeChecker.java
#	engine/schema/src/main/resources/META-INF/db/schema-42300to2400.sql
#	plugins/integrations/veeam-control-service/src/main/java/org/apache/cloudstack/veeam/api/dto/Version.java
#	utils/src/main/java/org/apache/cloudstack/utils/CloudStackVersion.java
#	utils/src/test/java/org/apache/cloudstack/utils/CloudStackVersionTest.java
- Add missing VlanTrunkMigrationHelper import in VirtualMachineManagerImpl
- Fix AssignLoadBalancerTest's NetworkModelImpl spy to stub the new
  getNicAndIpInNetwork method used by LoadBalancingRulesManagerImpl,
  matching the existing getNics() stub it replaced
…ommand

replaceVlanTrunkInterfaces iterates VirtualMachineTO.getNics(), which
the test's bare mock returned null for (unstubbed), unlike every real
VirtualMachineTO built via toVmTO, which always sets nics. Stub it the
same way getDisks() already was.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants