Multiple networks support for vms in advanced zone with security group (and kvm support) by ustcweizhou · Pull Request #3639 · apache/cloudstack · GitHub

Description

In advanced zone with security groups, there can be multiple shared networks with security groups enabled. However, it is only possible to create vm with only one network. When we try to create vm with multiple networks, it gives error "Only support one network per VM if security group enabled".

Operations succeed if we create a vm with one network and add other networks to the vm. However, the new nics do not work as there are no network rules for them applied on hypervisor.

This PR will enable the functionality on KVM hypervisors, including the support on
(1) API to support vm with multiple networks for KVM
(2) UI to create vm with multiple networks for KVM
(3) network rules applied on all nics (when create a vm, and add/remove nics)
(4) network rules for all secondary ips on all nics

This PR includes the other two bug fixes: #3635 and #3636
This PR also included a script file for integration test.

Fixes: #3045 #3568

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)

Screenshots (if appropriate):

How Has This Been Tested?

environment:
ubuntu 18.04 / python2
advanced zone with security groups

check network rules for vm (eg i-10-20-VM)

ipset list i-10-20-VM
ipset list i-10-20-VM-6
iptables-save | grep i-10-20
ebtables-save | grep i-10-20

vm actions:

create a vm with multiple networks
add new nic to vm
remove nic from vm
add secondary ips to vm
remove secondary ips to vm
reboot vm
migrate vm
stop vm
start vm

@rakgenius @wido @GabrielBrascher @rhtyd
Please review it !

We have tested with ipv4 addresses.

We have also some other codes in integration test to test if the primary and secondary ips are reachable from virtual router. However, it requires sshpass and a script in vm to configure the ips, so I removed them from integration test.

Looking good! Left a few comments

@wido Thanks for review !
It would be nice if you can test it in PCExtreme

Hi guys @wido @weizhouapache , any change to progress on this one for the 4.14 (and solve the conflict) ?

@ustcweizhou can you fix the merge conflict?

@rhtyd rebased with latest master

@blueorangutan package

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

Packaging result: ✖centos6 ✔centos7 ✔debian. JID-711

@blueorangutan test

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

Trillian test result (tid-855)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 27897 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr3639-t855-kvm-centos7.zip
Smoke tests completed. 78 look OK, 0 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File

@wido Are all your concerns met? Will you guys test this?

@GabrielBrascher interested to test this one perhaps?

@andrijapanicsb sure, going to test it soon!

@ustcweizhou can you address merge conflict?

…p (and kvm support)

@blueorangutan package

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

Packaging result: ✖centos6 ✔centos7 ✔debian. JID-871

sure, going to test it soon!

@GabrielBrascher kindly ping if any updates on testing. thx!

@blueorangutan package

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

Packaging result: ✖centos6 ✔centos7 ✔debian. JID-886

@blueorangutan test

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

Trillian test result (tid-1014)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 33763 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr3639-t1014-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_privategw_acl.py
Smoke tests completed. 78 look OK, 1 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_02_vpc_privategw_static_routes	Failure	223.58	test_privategw_acl.py
test_03_vpc_privategw_restart_vpc_cleanup	Failure	221.55	test_privategw_acl.py
test_04_rvpc_privategw_static_routes	Failure	334.08	test_privategw_acl.py

@andrijapanicsb got delayed with other tasks, just finishing to build it for a staging environment. I will be able to test it today.

Tested and it works 👍
Overall code looks good, just added a few minor observations.

Test env: KVM on Ubuntu 18.04, advanced network with security groups.

I see that RULE_TARGET_SEPARATOR is not being used outside this class. Is that correct? If it is indeed the case, this should be set back to private.

Thanks for adding documentation.

Just a note: I don't think that @param adds much info when it holds only the parameter name, normally I would vote to remove them, or add some description on each param.

Java Naming Conventions does not recommend using _ (underscore) before variable name. Can you please remove it?

code lgtm, not tested

Based on code review and tests, it LGTM.
Regression tests failed on :
test_02_vpc_privategw_static_routes Failure 223.58 test_privategw_acl.py
test_03_vpc_privategw_restart_vpc_cleanup Failure 221.55 test_privategw_acl.py
test_04_rvpc_privategw_static_routes Failure 334.08 test_privategw_acl.py

But not necessarily related with this PR (afaik). Issue #3859 was created to tackle it.

…che#3639)

svenvogel added type:new-feature component:security-group component:kvm labels Oct 17, 2019

yadvr added this to the 4.14.0.0 milestone Oct 22, 2019

wido requested changes Oct 26, 2019

View reviewed changes

ustcweizhou force-pushed the 4.14.0.0-multipleips branch from 9c64276 to 7294729 Compare January 28, 2020 09:28

yadvr closed this Jan 29, 2020

yadvr reopened this Jan 29, 2020

apache deleted a comment from yadvr Jan 30, 2020

apache deleted a comment from blueorangutan Jan 30, 2020

andrijapanicsb requested a review from wido February 7, 2020 12:08

ustcweizhou and others added 3 commits February 12, 2020 20:06

server: remove a nic in db instead of expunging it when removeNicFromVm

e7f7af6

Multiple networks support for vms in advanced zone with security grou…

3b4721d

…p (and kvm support)

Add integration test

5434dbd

DaanHoogland reopened this Feb 13, 2020

apache deleted a comment from blueorangutan Feb 13, 2020

yadvr closed this Feb 14, 2020

yadvr reopened this Feb 14, 2020

GabrielBrascher reviewed Feb 19, 2020

View reviewed changes

DaanHoogland approved these changes Feb 19, 2020

View reviewed changes

GabrielBrascher approved these changes Feb 19, 2020

View reviewed changes

DaanHoogland added the status:ready-for-merge label Feb 19, 2020

DaanHoogland merged commit 458d3b5 into apache:master Feb 19, 2020

nvazquez pushed a commit to shapeblue/cloudstack that referenced this pull request Feb 21, 2020

Multiple networks support for vms in advanced zone with securit… (apa…

b1b7dde

…che#3639)

nvazquez pushed a commit to shapeblue/cloudstack that referenced this pull request Feb 21, 2020

Multiple networks support for vms in advanced zone with securit… (apa…

914ecbe

…che#3639)

ggoodrich-ipp pushed a commit to ippathways/cloudstack that referenced this pull request Feb 24, 2020

Multiple networks support for vms in advanced zone with securit… (apa…

629af84

…che#3639)

ustcweizhou added a commit to ustcweizhou/cloudstack that referenced this pull request Feb 28, 2020

Multiple networks support for vms in advanced zone with securit… (apa…

73717b5

…che#3639)

DaanHoogland mentioned this pull request Oct 28, 2020

Security Groups are not properly applied on KVM with multiple NICs #3568

Closed

weizhouapache mentioned this pull request May 28, 2021

FIX Network with SG Disabled still has security group script adding rules on KVM #5049

Merged

2 of 10 tasks

weizhouapache mentioned this pull request Feb 27, 2023

replace vm.getPrivateMacAddress() with nic.getMacAddress() #7293

Merged

2 of 10 tasks

DaanHoogland removed the status:ready-for-merge label Dec 4, 2024

weizhouapache deleted the 4.14.0.0-multipleips branch March 21, 2025 08:47

                   private static final String CIDR_LENGTH_SEPARATOR = "/";
-                  private static final char RULE_TARGET_SEPARATOR = ',';
-                  private static final char RULE_COMMAND_SEPARATOR = ';';
+                  public static final char RULE_TARGET_SEPARATOR = ',';

+                   * @param conn
+                   * @param vm
+                   * @param checkBeforeApply
+                   * @return

                   @Inject
                   private NetworkDetailsDao networkDetailsDao;
+                  @Inject
+                  private SecurityGroupManager _securityGroupManager;

Multiple networks support for vms in advanced zone with security group (and kvm support) by ustcweizhou · Pull Request #3639 · apache/cloudstack · GitHub

Repository navigation

Multiple networks support for vms in advanced zone with security group (and kvm support) - #3639

Multiple networks support for vms in advanced zone with security group (and kvm support)#3639DaanHoogland merged 4 commits intoapache:masterapache/cloudstack:masterfrom ustcweizhou:4.14.0.0-multipleipsustcweizhou/cloudstack:4.14.0.0-multipleipsCopy head branch name to clipboard

Conversation

ustcweizhou commented Oct 17, 2019

Description

Types of changes

Screenshots (if appropriate):

How Has This Been Tested?

Uh oh!

ustcweizhou commented Oct 17, 2019

Uh oh!

wido left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ustcweizhou commented Oct 28, 2019

Uh oh!

andrijapanicsb commented Jan 13, 2020

Uh oh!

yadvr commented Jan 28, 2020

Uh oh!

weizhouapache commented Jan 28, 2020

Uh oh!

weizhouapache commented Jan 30, 2020

Uh oh!

blueorangutan commented Jan 30, 2020

Uh oh!

blueorangutan commented Jan 30, 2020

Uh oh!

DaanHoogland commented Jan 30, 2020

Uh oh!

blueorangutan commented Jan 30, 2020

Uh oh!

blueorangutan commented Jan 30, 2020

Uh oh!

DaanHoogland commented Jan 30, 2020

Uh oh!

andrijapanicsb commented Feb 7, 2020

Uh oh!

GabrielBrascher commented Feb 7, 2020

Uh oh!

yadvr commented Feb 12, 2020

Uh oh!

yadvr commented Feb 14, 2020

Uh oh!

blueorangutan commented Feb 14, 2020

Uh oh!

blueorangutan commented Feb 14, 2020

Uh oh!

andrijapanicsb commented Feb 14, 2020

Uh oh!

yadvr commented Feb 17, 2020

Uh oh!

blueorangutan commented Feb 17, 2020

Uh oh!

blueorangutan commented Feb 17, 2020

Uh oh!

DaanHoogland commented Feb 17, 2020

Uh oh!

blueorangutan commented Feb 17, 2020

Uh oh!

blueorangutan commented Feb 18, 2020

Uh oh!

GabrielBrascher commented Feb 18, 2020

Uh oh!

GabrielBrascher left a comment

Choose a reason for hiding this comment

Uh oh!

GabrielBrascher Feb 19, 2020 • 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!

GabrielBrascher Feb 19, 2020

Choose a reason for hiding this comment

Uh oh!

Multiple networks support for vms in advanced zone with security group (and kvm support)#3639
DaanHoogland merged 4 commits into
apache:masterfrom
ustcweizhou:4.14.0.0-multipleips

GabrielBrascher Feb 19, 2020 •
edited
Loading