Skip to content

Make sure other than user VMs can have multiple NICs in a network#5896

Merged
sureshanaparti merged 2 commits intoapache:4.16from
shapeblue:allowMultipleNicsOnPublicNets
Jan 31, 2022
Merged

Make sure other than user VMs can have multiple NICs in a network#5896
sureshanaparti merged 2 commits intoapache:4.16from
shapeblue:allowMultipleNicsOnPublicNets

Conversation

@DaanHoogland
Copy link
Copy Markdown
Contributor

@DaanHoogland DaanHoogland commented Jan 25, 2022 •
edited
Loading

Description

During assignment of an IP from a secondary address range an extra NIC is created on a router. This must be allowed even though the router already has a NIC on the public network. The creation of extra NICs should only be prevented in User VMs on guest networks.

This PR...

Fixes: #5890

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?

manual and throught the existing smoke tests.

@DaanHoogland DaanHoogland added this to the 4.16.1.0 milestone Jan 25, 2022
@DaanHoogland DaanHoogland self-assigned this Jan 25, 2022
@DaanHoogland
Copy link
Copy Markdown
Contributor Author

DaanHoogland commented Jan 25, 2022

@blueorangutan package

@blueorangutan
Copy link
Copy Markdown

blueorangutan commented Jan 25, 2022

@DaanHoogland a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

@blueorangutan
Copy link
Copy Markdown

blueorangutan commented Jan 25, 2022

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 2297

weizhouapache
weizhouapache reviewed Jan 25, 2022
View reviewed changes
Comment thread engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java Outdated
@weizhouapache
Copy link
Copy Markdown
Member

weizhouapache commented Jan 25, 2022

@DaanHoogland could you please update the title and description ?

@DaanHoogland DaanHoogland changed the title only check user VMs Make sure other than user VMs can have multiple NICs in a network Jan 25, 2022
@DaanHoogland @weizhouapache
Update engine/orchestration/src/main/java/com/cloud/vm/VirtualMachine…
4426fcd 
…ManagerImpl.java

Co-authored-by: Wei Zhou <weizhou@apache.org>
@DaanHoogland
Copy link
Copy Markdown
Contributor Author

DaanHoogland commented Jan 25, 2022

@blueorangutan test keepEnv

@blueorangutan
Copy link
Copy Markdown

blueorangutan commented Jan 25, 2022

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

@blueorangutan
Copy link
Copy Markdown

blueorangutan commented Jan 26, 2022

Trillian test result (tid-2990)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 34673 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5896-t2990-kvm-centos7.zip
Smoke tests completed. 91 look OK, 1 have errors
Only failed tests results shown below:

Test Result Time (s) Test File
test_disable_oobm_ha_state_ineligible Error 1511.93 test_hostha_kvm.py

@weizhouapache
Copy link
Copy Markdown
Member

weizhouapache commented Jan 28, 2022

@DaanHoogland
is this ready for testing ?

@DaanHoogland
Copy link
Copy Markdown
Contributor Author

DaanHoogland commented Jan 28, 2022

@DaanHoogland is this ready for testing ?

yes @weizhouapache . I am pretty sure the HA failure has nothing to do with the code. I have tested locally.

@weizhouapache
Copy link
Copy Markdown
Member

weizhouapache commented Jan 28, 2022

@DaanHoogland is this ready for testing ?

yes @weizhouapache . I am pretty sure the HA failure has nothing to do with the code. I have tested locally.

great. can you make it 'ready for review' ?

@sureshanaparti sureshanaparti linked an issue Jan 28, 2022 that may be closed by this pull request
Cannot add extra public nics to VPC VR #5890
Closed
weizhouapache
weizhouapache approved these changes Jan 28, 2022
View reviewed changes
Copy link
Copy Markdown
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.

tested ok

@sureshanaparti
Copy link
Copy Markdown
Contributor

sureshanaparti commented Jan 30, 2022

@blueorangutan package

@blueorangutan
Copy link
Copy Markdown

blueorangutan commented Jan 30, 2022

@sureshanaparti a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

@blueorangutan
Copy link
Copy Markdown

blueorangutan commented Jan 30, 2022

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 2388

sureshanaparti
sureshanaparti approved these changes Jan 31, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@sureshanaparti sureshanaparti 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

@sureshanaparti
Copy link
Copy Markdown
Contributor

sureshanaparti commented Jan 31, 2022

Verified the fix with the steps mentioned in issue #5890.

borisstoyanov
borisstoyanov approved these changes Jan 31, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@borisstoyanov borisstoyanov left a comment

Choose a reason for hiding this comment

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

LGTM, manually checked

@DaanHoogland DaanHoogland marked this pull request as ready for review January 31, 2022 13:27
@sureshanaparti sureshanaparti merged commit fde34df into apache:4.16 Jan 31, 2022
@sureshanaparti sureshanaparti mentioned this pull request Jan 31, 2022
Closed
@DaanHoogland DaanHoogland removed their assignment Jun 13, 2023
@DaanHoogland DaanHoogland deleted the allowMultipleNicsOnPublicNets branch January 15, 2026 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sureshanaparti sureshanaparti sureshanaparti approved these changes

@borisstoyanov borisstoyanov borisstoyanov approved these changes

@weizhouapache weizhouapache weizhouapache approved these changes

Assignees

No one assigned

Labels

None yet

Projects

No open projects
Apache CloudStack 4.16.1
Status: Done

Milestone

4.16.1.0

Development

Successfully merging this pull request may close these issues.

Cannot add extra public nics to VPC VR

5 participants

@DaanHoogland @blueorangutan @weizhouapache @sureshanaparti @borisstoyanov