Skip to content
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

guarantee MAC uniqueness #7634

Merged
merged 8 commits into from
Jul 19, 2023
Merged

Conversation

DaanHoogland
Copy link
Contributor

@DaanHoogland DaanHoogland commented Jun 15, 2023

Description

This PR makes sure a new MAC is unique by

  • using zone DB ID as default ZoneMacIdentifyer
  • checking if the mac exists in the nics table
    if the mac exists a new one will be generated untill a free one is found.

This is a fix that will not generate duplicate MAC addresses in the future and prevent duplicates if prior versions generated randon MACs before.

Fixes: #7633

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)

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?

The testing of this feature is dependend on randomly generated MAC addresses that are in the way when generating logicly deterministic. Any testing suggestions are appreciated.

@apache apache deleted a comment from blueorangutan Jun 15, 2023
@apache apache deleted a comment from blueorangutan Jun 15, 2023
@codecov
Copy link

codecov bot commented Jun 16, 2023

Codecov Report

Merging #7634 (016c550) into 4.18 (1aa4f80) will increase coverage by 0.03%.
The diff coverage is 0.00%.

@@             Coverage Diff              @@
##               4.18    #7634      +/-   ##
============================================
+ Coverage     12.98%   13.02%   +0.03%     
- Complexity     8989     9028      +39     
============================================
  Files          2716     2719       +3     
  Lines        256343   256846     +503     
  Branches      39981    40051      +70     
============================================
+ Hits          33291    33448     +157     
- Misses       218886   219211     +325     
- Partials       4166     4187      +21     
Impacted Files Coverage Δ
...ter/entity/api/db/dao/EngineDataCenterDaoImpl.java 0.00% <ø> (ø)
.../src/main/java/com/cloud/dc/dao/DataCenterDao.java 0.00% <ø> (ø)
.../main/java/com/cloud/dc/dao/DataCenterDaoImpl.java 18.81% <ø> (+0.63%) ⬆️
...ain/java/com/cloud/network/dao/NetworkDaoImpl.java 3.21% <ø> (+<0.01%) ⬆️
...ema/src/main/java/com/cloud/vm/dao/NicDaoImpl.java 23.50% <ø> (+0.11%) ⬆️
...a/com/cloud/hypervisor/hyperv/guru/HypervGuru.java 0.00% <0.00%> (ø)
...com/cloud/hypervisor/guru/VmwareVmImplementer.java 0.00% <0.00%> (ø)
...a/com/cloud/network/vm/NetScalerVMManagerImpl.java 0.00% <0.00%> (ø)
.../main/java/com/cloud/network/NetworkModelImpl.java 11.11% <0.00%> (-0.05%) ⬇️
...ava/com/cloud/network/guru/ControlNetworkGuru.java 4.81% <0.00%> (ø)

... and 41 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@apache apache deleted a comment from blueorangutan Jun 16, 2023
Copy link
Member

@weizhouapache weizhouapache left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code lgtm

@DaanHoogland
Ihink the title needs to be updated

@DaanHoogland DaanHoogland changed the title use zone DB ID as default ZoneMacIdentifyer garantee MAC uniqueness Jun 19, 2023
@DaanHoogland DaanHoogland marked this pull request as ready for review June 19, 2023 09:17
@weizhouapache weizhouapache added this to the 4.18.1.0 milestone Jun 20, 2023
Copy link
Contributor

@BryanMLima BryanMLima left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I just made some suggestions over some typos around the code.

api/src/main/java/com/cloud/network/NetworkModel.java Outdated Show resolved Hide resolved
Co-authored-by: Bryan Lima <42067040+BryanMLima@users.noreply.github.com>
@apache apache deleted a comment from blueorangutan Jun 21, 2023
@apache apache deleted a comment from blueorangutan Jun 21, 2023
@apache apache deleted a comment from blueorangutan Jun 21, 2023
@apache apache deleted a comment from blueorangutan Jun 21, 2023
@apache apache deleted a comment from blueorangutan Jun 21, 2023
@weizhouapache weizhouapache linked an issue Jun 22, 2023 that may be closed by this pull request
@apache apache deleted a comment from blueorangutan Jul 5, 2023
@apache apache deleted a comment from blueorangutan Jul 5, 2023
@apache apache deleted a comment from blueorangutan Jul 5, 2023
@DaanHoogland
Copy link
Contributor Author

@blueorangutan package

@blueorangutan
Copy link

@DaanHoogland a [SF] 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.

@apache apache deleted a comment from blueorangutan Jul 5, 2023
@apache apache deleted a comment from blueorangutan Jul 5, 2023
@blueorangutan
Copy link

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

@DaanHoogland
Copy link
Contributor Author

@blueorangutan test

@blueorangutan
Copy link

@DaanHoogland a [SF] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

@rohityadavcloud
Copy link
Member

ping @shwstppr @nvazquez for review
@blueorangutan test

@blueorangutan
Copy link

@rohityadavcloud a [SF] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

@DaanHoogland
Copy link
Contributor Author

@blueorangutan test

@blueorangutan
Copy link

@DaanHoogland a [SF] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

@DaanHoogland
Copy link
Contributor Author

@blueorangutan package

@blueorangutan
Copy link

@DaanHoogland a [SF] 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
Copy link

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

@rohityadavcloud
Copy link
Member

@blueorangutan test

@blueorangutan
Copy link

@rohityadavcloud a [SF] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

@blueorangutan
Copy link

[SF] Trillian test result (tid-7098)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 42717 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7634-t7098-kvm-centos7.zip
Smoke tests completed. 107 look OK, 1 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test Result Time (s) Test File
test_01_migrate_VM_and_root_volume Error 78.56 test_vm_life_cycle.py
test_02_migrate_VM_with_two_data_disks Error 55.37 test_vm_life_cycle.py

@DaanHoogland DaanHoogland merged commit 73a269e into apache:4.18 Jul 19, 2023
25 of 27 checks passed
DaanHoogland added a commit that referenced this pull request Jul 19, 2023
* 4.18:
  Storage and volumes statistics tasks for StorPool primary storage (#7404)
  proper storage construction (#6797)
  guarantee MAC uniqueness (#7634)
  server: allow migration of all VMs with local storage on KVM (#7656)
  Add L2 networks to Zones with SG (#7719)
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.

Duplicated mac addresses in a zone
6 participants