MAC address assignment improvements #12349

sureshanaparti · 2025-12-29T18:03:54Z

Description

This PR has the following improvements for the MAC address assignment.

MAC Identifier configuration 'mac.identifier' is first picked from the zone value as it is defined for the zone scope, if not global scope. When it is 0, DB id of the zone is considered as the identifier which is already mentioned in the config description:

cloudstack/api/src/main/java/com/cloud/network/NetworkModel.java

Lines 94 to 95 in 56a39e6

    
           static final ConfigKey<Integer> MACIdentifier = new ConfigKey<>("Advanced",Integer.class, "mac.identifier", "0", 
        
                   "This value will be used while generating the mac addresses for isolated and shared networks. The hexadecimal equivalent value will be present at the 2nd octet of the mac address. Default value is zero (0) which means that the DB id of the zone will be used.", true, ConfigKey.Scope.Zone);

MAC address uniqueness is checked within the nics of the network (not in the overall nics).
MAC address created from [1] is validated for uniqueness, and when not unique, new MAC generated from [2] to avoid duplicates.
[1]
https://github.com/apache/cloudstack/blob/4.20/utils/src/main/java/com/cloud/utils/net/NetUtils.java#L134-L145
[2]
https://github.com/apache/cloudstack/blob/4.20/engine/schema/src/main/java/com/cloud/network/dao/NetworkDaoImpl.java#L432-L439

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

Screenshots (if appropriate):

How Has This Been Tested?

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

codecov · 2025-12-29T18:11:37Z

Codecov Report

❌ Patch coverage is 8.77193% with 52 lines in your changes missing coverage. Please review.
✅ Project coverage is 16.23%. Comparing base (56a39e6) to head (7188a62).

Files with missing lines	Patch %	Lines
.../main/java/com/cloud/network/NetworkModelImpl.java	0.00%	16 Missing ⚠️
...com/cloud/network/router/NicProfileHelperImpl.java	0.00%	8 Missing ⚠️
.../src/main/java/com/cloud/network/NetworkModel.java	0.00%	6 Missing ⚠️
...va/com/cloud/network/guru/PodBasedNetworkGuru.java	0.00%	4 Missing ⚠️
...ava/com/cloud/network/guru/PrivateNetworkGuru.java	0.00%	4 Missing ⚠️
...ava/com/cloud/network/guru/StorageNetworkGuru.java	0.00%	4 Missing ⚠️
...ain/java/com/cloud/network/dao/NetworkDaoImpl.java	0.00%	2 Missing ⚠️
...ema/src/main/java/com/cloud/vm/dao/NicDaoImpl.java	0.00%	2 Missing ⚠️
...n/java/com/cloud/network/IpAddressManagerImpl.java	0.00%	2 Missing ⚠️
...src/main/java/com/cloud/network/addr/PublicIp.java	75.00%	0 Missing and 1 partial ⚠️
... and 3 more

Additional details and impacted files

@@             Coverage Diff              @@
##               4.20   #12349      +/-   ##
============================================
- Coverage     16.23%   16.23%   -0.01%     
- Complexity    13378    13379       +1     
============================================
  Files          5657     5657              
  Lines        498866   498899      +33     
  Branches      60545    60553       +8     
============================================
+ Hits          81011    81013       +2     
- Misses       408821   408851      +30     
- Partials       9034     9035       +1

Flag	Coverage Δ
uitests	`4.00% <ø> (ø)`
unittests	`17.09% <8.77%> (-0.01%)`	⬇️

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

☔ View full report in Codecov by Sentry.
📢 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.

sureshanaparti · 2025-12-29T18:24:56Z

@blueorangutan package

blueorangutan · 2025-12-29T18:26:04Z

@sureshanaparti a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

sureshanaparti · 2025-12-29T19:33:22Z

@blueorangutan package

blueorangutan · 2025-12-29T19:34:05Z

@sureshanaparti a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2025-12-29T20:44:34Z

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 16198

sureshanaparti · 2025-12-30T04:05:05Z

@blueorangutan test

blueorangutan · 2025-12-30T04:06:03Z

@sureshanaparti a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

Copilot

Pull request overview

This PR improves MAC address assignment by introducing zone-level MAC identifier configuration and scoping MAC address uniqueness checks to individual networks rather than globally.

Key changes:

Zone-level mac.identifier configuration is now retrieved from zone scope first, falling back to global scope, with the zone's database ID used when the value is 0
MAC address uniqueness is now checked within the scope of each network (not globally across all NICs)
MAC addresses generated via sequence-based logic are validated for uniqueness and regenerated using the fallback method if duplicates are detected

Reviewed changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 10 comments.

Show a summary per file

File	Description
`utils/src/main/java/com/cloud/utils/net/NetUtils.java`	Updated parameter name in `createSequenceBasedMacAddress` to `macIdentifier` and updated comment to reference zone-level config; fixed lowercase 'l' suffix to uppercase 'L'
`api/src/main/java/com/cloud/network/NetworkModel.java`	Added `isMACUnique` and `getMacIdentifier` default methods to support network-scoped MAC uniqueness checks and zone-level MAC identifier retrieval
`server/src/main/java/com/cloud/network/NetworkModelImpl.java`	Implemented `getMacIdentifier` to retrieve zone-level config and `isMACUnique` to check uniqueness within a network; updated `getNextAvailableMacAddressInNetwork` to use network-scoped uniqueness
`engine/schema/src/main/java/com/cloud/vm/dao/NicDao.java`	Updated `findByMacAddress` signature to accept networkId parameter for network-scoped lookups
`engine/schema/src/main/java/com/cloud/vm/dao/NicDaoImpl.java`	Implemented network-scoped MAC address lookup by adding networkId parameter to search criteria
`engine/schema/src/main/java/com/cloud/network/dao/NetworkDaoImpl.java`	Code style improvements: changed `.intValue()` comparison to direct integer comparison and fixed uppercase 'L' suffix
`server/src/main/java/com/cloud/network/guru/*.java`	Updated all network gurus (Storage, PodBased, Private, Direct) to use `getMacIdentifier` and validate/regenerate MAC addresses for uniqueness within the network
`server/src/main/java/com/cloud/network/router/NicProfileHelper.java`	Added `InsufficientAddressCapacityException` to method signature
`server/src/main/java/com/cloud/network/router/NicProfileHelperImpl.java`	Updated to use zone-level MAC identifier and validate/regenerate MAC addresses for private gateway NICs
`server/src/main/java/com/cloud/network/IpAddressManagerImpl.java`	Added MAC uniqueness check before assigning MAC address to NICs
`server/src/main/java/com/cloud/network/Ipv6AddressManagerImpl.java`	Added MAC uniqueness check in `setNicPropertiesFromNetwork`
`engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java`	Added MAC uniqueness check in `configureNicProfileBasedOnRequestedIp`
`engine/components-api/src/main/java/com/cloud/network/addr/PublicIp.java`	Updated `createFromAddrAndVlan` to use zone-level MAC identifier; removed trailing blank lines
`server/src/test/java/com/cloud/network/Ipv6AddressManagerTest.java`	Updated test to mock network ID and MAC uniqueness check
`engine/orchestration/src/test/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestratorTest.java`	Updated test to mock MAC uniqueness check

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

server/src/main/java/com/cloud/network/router/NicProfileHelperImpl.java

server/src/main/java/com/cloud/network/guru/PodBasedNetworkGuru.java

server/src/main/java/com/cloud/network/guru/PrivateNetworkGuru.java

Copilot · 2025-12-30T04:14:09Z

utils/src/main/java/com/cloud/utils/net/NetUtils.java

         */

-        return macAddress | prefix<<40 | globalConfig << 32 & 0x00ff00000000l | (long)s_rand.nextInt(255) << 24;
+        return macAddress | prefix << 40 | macIdentifier << 32 & 0x00ff00000000L | (long)s_rand.nextInt(255) << 24;


The random number generation uses s_rand.nextInt(255) which generates values from 0 to 254 (exclusive upper bound). According to the comment on line 140, B3 should be "A randomly generated number between 0 - 255". To include 255, this should be s_rand.nextInt(256).

Suggested change

return macAddress | prefix << 40 | macIdentifier << 32 & 0x00ff00000000L | (long)s_rand.nextInt(255) << 24;

return macAddress | prefix << 40 | macIdentifier << 32 & 0x00ff00000000L | (long)s_rand.nextInt(256) << 24;

@weizhouapache @DaanHoogland can update this as per the comment here? previously, it used to generate till 254 only.

I think the previous programming has been either overly defensive or sloppy. But in general I’d rather see us iterate than randomly go through the range. After half is full the hit-and-miss rate becomes pretty high like this. Either way, it is functional as is, so address in another PR, i’d say.

server/src/main/java/com/cloud/network/guru/StorageNetworkGuru.java

engine/schema/src/main/java/com/cloud/vm/dao/NicDaoImpl.java

engine/schema/src/main/java/com/cloud/network/dao/NetworkDaoImpl.java

server/src/main/java/com/cloud/network/NetworkModelImpl.java

server/src/main/java/com/cloud/network/router/NicProfileHelperImpl.java

sureshanaparti · 2025-12-30T06:00:35Z

@blueorangutan package

blueorangutan · 2025-12-30T06:02:05Z

@sureshanaparti a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2025-12-30T07:57:54Z

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 16203

DaanHoogland

looks generally good, two questions though.

DaanHoogland · 2025-12-30T08:17:13Z

api/src/main/java/com/cloud/network/NetworkModel.java

+    default boolean isMACUnique(String mac, long networkId) {
+        return true;
+    }


why this default implementation?

@DaanHoogland there are some mock classes implementing it, not implemented it there (I think, we can return default true for them)

yes, but it seems a logical error for anyone wanting to implement a new NetworkModel. They should be forced to implement it and not use this code by default.

DaanHoogland · 2025-12-30T08:24:17Z

server/src/main/java/com/cloud/network/guru/PodBasedNetworkGuru.java

+        String macAddress = NetUtils.long2Mac(NetUtils.createSequenceBasedMacAddress(result.getMacAddress(), _networkModel.getMacIdentifier(dest.getDataCenter().getId())));
+        if (!_networkModel.isMACUnique(macAddress, config.getId())) {
+            macAddress = _networkModel.getNextAvailableMacAddressInNetwork(config.getId());
+        }
+        nic.setMacAddress(macAddress);


This pattern seems to be recurring (also in PrivateNetworkGuru, StorageNetworkGuru and NetProfileHelperImpl). Is it worth codifying it in a method?

yes, it's repeating. will check.

boring-cyborg bot added component:api component:ipv6 component:networking component:orchestration labels Dec 29, 2025

sureshanaparti force-pushed the mac-address-improvements branch from e155985 to 3672fcd Compare December 29, 2025 18:24

MAC address assignment improvements

57da8b5

sureshanaparti force-pushed the mac-address-improvements branch from 3672fcd to 9f95521 Compare December 29, 2025 19:04

fix tests

3b53055

sureshanaparti force-pushed the mac-address-improvements branch from 9f95521 to 3b53055 Compare December 29, 2025 19:33

sureshanaparti requested review from DaanHoogland, Copilot and weizhouapache December 30, 2025 04:04

Copilot started reviewing on behalf of sureshanaparti December 30, 2025 04:04 View session

Copilot AI reviewed Dec 30, 2025

View reviewed changes

review comments

7188a62

DaanHoogland reviewed Dec 30, 2025

View reviewed changes

	static final ConfigKey<Integer> MACIdentifier = new ConfigKey<>("Advanced",Integer.class, "mac.identifier", "0",
	"This value will be used while generating the mac addresses for isolated and shared networks. The hexadecimal equivalent value will be present at the 2nd octet of the mac address. Default value is zero (0) which means that the DB id of the zone will be used.", true, ConfigKey.Scope.Zone);

	return macAddress \| prefix << 40 \| macIdentifier << 32 & 0x00ff00000000L \| (long)s_rand.nextInt(255) << 24;
	return macAddress \| prefix << 40 \| macIdentifier << 32 & 0x00ff00000000L \| (long)s_rand.nextInt(256) << 24;

MAC address assignment improvements #12349

Are you sure you want to change the base?

MAC address assignment improvements #12349

Conversation

sureshanaparti commented Dec 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

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

Uh oh!

codecov bot commented Dec 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sureshanaparti commented Dec 29, 2025

Uh oh!

blueorangutan commented Dec 29, 2025

Uh oh!

sureshanaparti commented Dec 29, 2025

Uh oh!

blueorangutan commented Dec 29, 2025

Uh oh!

blueorangutan commented Dec 29, 2025

Uh oh!

sureshanaparti commented Dec 30, 2025

Uh oh!

blueorangutan commented Dec 30, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Dec 30, 2025

Choose a reason for hiding this comment

Uh oh!

sureshanaparti Dec 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DaanHoogland Dec 30, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sureshanaparti commented Dec 30, 2025

Uh oh!

blueorangutan commented Dec 30, 2025

Uh oh!

blueorangutan commented Dec 30, 2025

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

DaanHoogland Dec 30, 2025

Choose a reason for hiding this comment

Uh oh!

sureshanaparti Dec 30, 2025

Choose a reason for hiding this comment

Uh oh!

DaanHoogland Dec 30, 2025

Choose a reason for hiding this comment

Uh oh!

DaanHoogland Dec 30, 2025

Choose a reason for hiding this comment

Uh oh!

sureshanaparti Dec 30, 2025

Choose a reason for hiding this comment

Uh oh!

sureshanaparti commented Dec 29, 2025 •

edited

Loading

codecov bot commented Dec 29, 2025 •

edited

Loading

sureshanaparti Dec 30, 2025 •

edited

Loading