You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by jayapalu <gi...@git.apache.org> on 2017/01/02 05:54:44 UTC
[GitHub] cloudstack pull request #1883: CLOUDSTACK-9723: Enable unique mac address ac...
GitHub user jayapalu opened a pull request:
https://github.com/apache/cloudstack/pull/1883
CLOUDSTACK-9723: Enable unique mac address across the zones
Enable unique mac address across different deployments and networks.
If there two networks in two different deployment zones then this feature will avoid mac address collision.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/Accelerite/cloudstack uniqueMac
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/1883.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1883
----
commit c3f1ff9533fe8d433580dc13209b7bcf4020bc3a
Author: Jayapal <ja...@accelerite.com>
Date: 2016-12-30T07:10:25Z
CLOUDSTACK-9723: Enable unique mac address across the zones
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1883: CLOUDSTACK-9723: Enable unique mac address ac...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1883#discussion_r95035623
--- Diff: engine/schema/src/com/cloud/network/dao/NetworkDaoImpl.java ---
@@ -377,11 +377,16 @@ protected void addAccountToNetwork(final long networkId, final long accountId, f
}
@Override
- public String getNextAvailableMacAddress(final long networkConfigId) {
+ public String getNextAvailableMacAddress(final long networkConfigId, Integer zoneMacIdentifier) {
--- End diff --
Please add unit test case(s) to verify the behavior of this method.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1883: CLOUDSTACK-9723: Enable unique mac address across th...
Posted by cloudmonger <gi...@git.apache.org>.
Github user cloudmonger commented on the issue:
https://github.com/apache/cloudstack/pull/1883
### ACS CI BVT Run
**Sumarry:**
Build Number 354
Hypervisor xenserver
NetworkType Advanced
Passed=103
Failed=2
Skipped=7
_Link to logs Folder (search by build_no):_ https://www.dropbox.com/sh/yj3wnzbceo9uef2/AAB6u-Iap-xztdm6jHX9SjPja?dl=0
**Failed tests:**
* test_non_contigiousvlan.py
* test_extendPhysicalNetworkVlan Failed
* test_routers_network_ops.py
* test_02_RVR_Network_FW_PF_SSH_default_routes_egress_false Failed
**Skipped tests:**
test_01_test_vm_volume_snapshot
test_vm_nic_adapter_vmxnet3
test_static_role_account_acls
test_11_ss_nfs_version_on_ssvm
test_nested_virtualization_vmware
test_3d_gpu_support
test_deploy_vgpu_enabled_vm
**Passed test suits:**
test_deploy_vm_with_userdata.py
test_affinity_groups_projects.py
test_portable_publicip.py
test_over_provisioning.py
test_global_settings.py
test_scale_vm.py
test_service_offerings.py
test_routers_iptables_default_policy.py
test_loadbalance.py
test_routers.py
test_reset_vm_on_reboot.py
test_deploy_vms_with_varied_deploymentplanners.py
test_network.py
test_router_dns.py
test_login.py
test_deploy_vm_iso.py
test_list_ids_parameter.py
test_public_ip_range.py
test_multipleips_per_nic.py
test_regions.py
test_affinity_groups.py
test_network_acl.py
test_pvlan.py
test_volumes.py
test_nic.py
test_deploy_vm_root_resize.py
test_resource_detail.py
test_secondary_storage.py
test_vm_life_cycle.py
test_disk_offerings.py
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1883: CLOUDSTACK-9723: Enable unique mac address across th...
Posted by cloudmonger <gi...@git.apache.org>.
Github user cloudmonger commented on the issue:
https://github.com/apache/cloudstack/pull/1883
### ACS CI BVT Run
**Sumarry:**
Build Number 390
Hypervisor xenserver
NetworkType Advanced
Passed=105
Failed=0
Skipped=7
_Link to logs Folder (search by build_no):_ https://www.dropbox.com/sh/yj3wnzbceo9uef2/AAB6u-Iap-xztdm6jHX9SjPja?dl=0
**Failed tests:**
**Skipped tests:**
test_01_test_vm_volume_snapshot
test_vm_nic_adapter_vmxnet3
test_static_role_account_acls
test_11_ss_nfs_version_on_ssvm
test_nested_virtualization_vmware
test_3d_gpu_support
test_deploy_vgpu_enabled_vm
**Passed test suits:**
test_deploy_vm_with_userdata.py
test_affinity_groups_projects.py
test_portable_publicip.py
test_over_provisioning.py
test_global_settings.py
test_scale_vm.py
test_service_offerings.py
test_routers_iptables_default_policy.py
test_loadbalance.py
test_routers.py
test_reset_vm_on_reboot.py
test_deploy_vms_with_varied_deploymentplanners.py
test_network.py
test_router_dns.py
test_non_contigiousvlan.py
test_login.py
test_deploy_vm_iso.py
test_list_ids_parameter.py
test_public_ip_range.py
test_multipleips_per_nic.py
test_regions.py
test_affinity_groups.py
test_network_acl.py
test_pvlan.py
test_volumes.py
test_nic.py
test_deploy_vm_root_resize.py
test_resource_detail.py
test_secondary_storage.py
test_vm_life_cycle.py
test_routers_network_ops.py
test_disk_offerings.py
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1883: CLOUDSTACK-9723: Enable unique mac address across th...
Posted by kishankavala <gi...@git.apache.org>.
Github user kishankavala commented on the issue:
https://github.com/apache/cloudstack/pull/1883
Code LGTM
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1883: CLOUDSTACK-9723: Enable unique mac address across th...
Posted by jayapalu <gi...@git.apache.org>.
Github user jayapalu commented on the issue:
https://github.com/apache/cloudstack/pull/1883
tag:mergeready
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1883: CLOUDSTACK-9723: Enable unique mac address ac...
Posted by jayapalu <gi...@git.apache.org>.
Github user jayapalu commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1883#discussion_r102651320
--- Diff: engine/schema/src/com/cloud/network/dao/NetworkDaoImpl.java ---
@@ -377,11 +377,16 @@ protected void addAccountToNetwork(final long networkId, final long accountId, f
}
@Override
- public String getNextAvailableMacAddress(final long networkConfigId) {
+ public String getNextAvailableMacAddress(final long networkConfigId, Integer zoneMacIdentifier) {
final SequenceFetcher fetch = SequenceFetcher.getInstance();
-
long seq = fetch.getNextSequence(Long.class, _tgMacAddress, networkConfigId);
- seq = seq | _prefix << 40 | _rand.nextInt(Short.MAX_VALUE) << 16 & 0x00000000ffff0000l;
+ if(zoneMacIdentifier!=0){
+ seq = seq | _prefix << 40 | (long)zoneMacIdentifier << 32 | networkConfigId << 16 & 0x00000000ffff0000l;
+ }
+ else {
+ seq = seq | _prefix << 40 | _rand.nextInt(Short.MAX_VALUE) << 16 & 0x00000000ffff0000l;
+
+ }
--- End diff --
formatted
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1883: CLOUDSTACK-9723: Enable unique mac address ac...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1883#discussion_r95034972
--- Diff: engine/schema/src/com/cloud/network/dao/NetworkDaoImpl.java ---
@@ -377,11 +377,16 @@ protected void addAccountToNetwork(final long networkId, final long accountId, f
}
@Override
- public String getNextAvailableMacAddress(final long networkConfigId) {
+ public String getNextAvailableMacAddress(final long networkConfigId, Integer zoneMacIdentifier) {
final SequenceFetcher fetch = SequenceFetcher.getInstance();
-
long seq = fetch.getNextSequence(Long.class, _tgMacAddress, networkConfigId);
- seq = seq | _prefix << 40 | _rand.nextInt(Short.MAX_VALUE) << 16 & 0x00000000ffff0000l;
+ if(zoneMacIdentifier!=0){
--- End diff --
What is `zoneMacIdentifier` is `null`? Should there be a `Preconditions.checkArgument` check added or a different behavior?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1883: CLOUDSTACK-9723: Enable unique mac address across th...
Posted by jayapalu <gi...@git.apache.org>.
Github user jayapalu commented on the issue:
https://github.com/apache/cloudstack/pull/1883
There are no marvin tests available for this. The unique mac address supposed for the two different deployments.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1883: CLOUDSTACK-9723: Enable unique mac address ac...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1883#discussion_r95035461
--- Diff: utils/src/main/java/com/cloud/utils/net/NetUtils.java ---
@@ -83,9 +83,19 @@
public final static int DEFAULT_AUTOSCALE_POLICY_INTERVAL_TIME = 30;
public final static int DEFAULT_AUTOSCALE_POLICY_QUIET_TIME = 5 * 60;
private final static Random s_rand = new Random(System.currentTimeMillis());
-
- public static long createSequenceBasedMacAddress(final long macAddress) {
- return macAddress | 0x060000000000l | (long)s_rand.nextInt(32768) << 25 & 0x00fffe000000l;
+ private final static long prefix = 0x1e;
+
+ public static long createSequenceBasedMacAddress(final long macAddress, long globalConfig) {
--- End diff --
Is it acceptable for ``macAddress`` or ``globalConfig`` to be less than zero? Is the maximum value permitted?
Also, please add unit test case(s) to verify the behavior of this method.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1883: CLOUDSTACK-9723: Enable unique mac address ac...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1883#discussion_r95035212
--- Diff: server/src/com/cloud/configuration/ConfigurationManagerImpl.java ---
@@ -790,6 +791,18 @@ private String validateConfigurationValue(final String name, String value, final
return null;
}
+ if (type.equals(Integer.class) && NetworkModel.MACIdentifier.key().equalsIgnoreCase(name)) {
+ try {
+ final int val = Integer.parseInt(value);
+ if(val <0 || val >255){
+ throw new InvalidParameterValueException(name+" value should be between 0 and 255. 0 value will disable this feature");
+ }
--- End diff --
Why are we using a magic value in the parameter to determine whether or not a feature is enabled? Why not add an explicit flag to make it clearer for the end user?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1883: CLOUDSTACK-9723: Enable unique mac address ac...
Posted by jayapalu <gi...@git.apache.org>.
Github user jayapalu commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1883#discussion_r102888327
--- Diff: engine/schema/src/com/cloud/network/dao/NetworkDaoImpl.java ---
@@ -377,11 +377,16 @@ protected void addAccountToNetwork(final long networkId, final long accountId, f
}
@Override
- public String getNextAvailableMacAddress(final long networkConfigId) {
+ public String getNextAvailableMacAddress(final long networkConfigId, Integer zoneMacIdentifier) {
--- End diff --
I tried writing unit test cases for it. I have difficulty in call static method. I tried with the PowerMockito.mockStatic but still I face NPE in fetch.getNextSequence.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1883: CLOUDSTACK-9723: Enable unique mac address ac...
Posted by jayapalu <gi...@git.apache.org>.
Github user jayapalu commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1883#discussion_r102649918
--- Diff: utils/src/main/java/com/cloud/utils/net/NetUtils.java ---
@@ -83,9 +83,19 @@
public final static int DEFAULT_AUTOSCALE_POLICY_INTERVAL_TIME = 30;
public final static int DEFAULT_AUTOSCALE_POLICY_QUIET_TIME = 5 * 60;
private final static Random s_rand = new Random(System.currentTimeMillis());
-
- public static long createSequenceBasedMacAddress(final long macAddress) {
- return macAddress | 0x060000000000l | (long)s_rand.nextInt(32768) << 25 & 0x00fffe000000l;
+ private final static long prefix = 0x1e;
+
+ public static long createSequenceBasedMacAddress(final long macAddress, long globalConfig) {
--- End diff --
It is not possible for macAddress, globalConfig to less than zero.
For globalconfig maximum value 255 is allowed.
macAddress value is coming from the DB (ipVO)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1883: CLOUDSTACK-9723: Enable unique mac address across th...
Posted by srinivas-gandikota <gi...@git.apache.org>.
Github user srinivas-gandikota commented on the issue:
https://github.com/apache/cloudstack/pull/1883
LGTM
Reviewed code -- looks good
I see even the BVTs have passed without any failures
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1883: CLOUDSTACK-9723: Enable unique mac address ac...
Posted by jayapalu <gi...@git.apache.org>.
Github user jayapalu commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1883#discussion_r102651284
--- Diff: engine/schema/src/com/cloud/network/dao/NetworkDaoImpl.java ---
@@ -377,11 +377,16 @@ protected void addAccountToNetwork(final long networkId, final long accountId, f
}
@Override
- public String getNextAvailableMacAddress(final long networkConfigId) {
+ public String getNextAvailableMacAddress(final long networkConfigId, Integer zoneMacIdentifier) {
final SequenceFetcher fetch = SequenceFetcher.getInstance();
-
long seq = fetch.getNextSequence(Long.class, _tgMacAddress, networkConfigId);
- seq = seq | _prefix << 40 | _rand.nextInt(Short.MAX_VALUE) << 16 & 0x00000000ffff0000l;
+ if(zoneMacIdentifier!=0){
--- End diff --
Added null check for it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1883: CLOUDSTACK-9723: Enable unique mac address ac...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1883#discussion_r95034912
--- Diff: engine/schema/src/com/cloud/network/dao/NetworkDaoImpl.java ---
@@ -377,11 +377,16 @@ protected void addAccountToNetwork(final long networkId, final long accountId, f
}
@Override
- public String getNextAvailableMacAddress(final long networkConfigId) {
+ public String getNextAvailableMacAddress(final long networkConfigId, Integer zoneMacIdentifier) {
final SequenceFetcher fetch = SequenceFetcher.getInstance();
-
long seq = fetch.getNextSequence(Long.class, _tgMacAddress, networkConfigId);
- seq = seq | _prefix << 40 | _rand.nextInt(Short.MAX_VALUE) << 16 & 0x00000000ffff0000l;
+ if(zoneMacIdentifier!=0){
+ seq = seq | _prefix << 40 | (long)zoneMacIdentifier << 32 | networkConfigId << 16 & 0x00000000ffff0000l;
+ }
+ else {
+ seq = seq | _prefix << 40 | _rand.nextInt(Short.MAX_VALUE) << 16 & 0x00000000ffff0000l;
+
+ }
--- End diff --
Please format per coding standards.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---