-
Notifications
You must be signed in to change notification settings - Fork 1.3k
MAC address assignment improvements #12349
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 4.20
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -53,6 +53,8 @@ public class PodBasedNetworkGuru extends AdapterBase implements NetworkGuru { | |
| DataCenterDao _dcDao; | ||
| @Inject | ||
| StorageNetworkManager _sNwMgr; | ||
| @Inject | ||
| NetworkModel _networkModel; | ||
|
|
||
| Random _rand = new Random(System.currentTimeMillis()); | ||
|
|
||
|
|
@@ -131,7 +133,11 @@ public void reserve(NicProfile nic, Network config, VirtualMachineProfile vm, De | |
| Integer vlan = result.getVlan(); | ||
|
|
||
| nic.setIPv4Address(result.getIpAddress()); | ||
| nic.setMacAddress(NetUtils.long2Mac(NetUtils.createSequenceBasedMacAddress(result.getMacAddress(), NetworkModel.MACIdentifier.value()))); | ||
| 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); | ||
|
Comment on lines
+136
to
+140
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This pattern seems to be recurring (also in
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, it's repeating. will check. |
||
| nic.setIPv4Gateway(pod.getGateway()); | ||
| nic.setFormat(AddressFormat.Ip4); | ||
| String netmask = NetUtils.getCidrNetmask(pod.getCidrSize()); | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -131,17 +131,17 @@ public static boolean isIpv6EnabledProtocol(InternetProtocol protocol) { | |||||
| } | ||||||
| } | ||||||
|
|
||||||
| public static long createSequenceBasedMacAddress(final long macAddress, long globalConfig) { | ||||||
| public static long createSequenceBasedMacAddress(final long macAddress, long macIdentifier) { | ||||||
| /* | ||||||
| Logic for generating MAC address: | ||||||
| Mac = B1:B2:B3:B4:B5:B6 (Bx is a byte). | ||||||
| B1 -> Presently controlled by prefix variable. The value should be such that the MAC is local and unicast. | ||||||
| B2 -> This will be configurable for each deployment/installation. Controlled by the global config MACIdentifier | ||||||
| B2 -> This will be configurable for each deployment/installation. Controlled by the 'mac.identifier' zone-level config | ||||||
| B3 -> A randomly generated number between 0 - 255 | ||||||
| B4,5,6 -> These bytes are based on the unique DB identifier associated with the IP address for which MAC is generated (refer to mac_address field in user_ip_address table). | ||||||
| */ | ||||||
|
|
||||||
| 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; | ||||||
|
||||||
| 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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@weizhouapache @DaanHoogland can update this as per the comment here? previously, it used to generate till 254 only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this default implementation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DaanHoogland there are some mock classes implementing it, not implemented it there (I think, we can return default true for them)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.