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

ustcweizhou · 2019-10-17T08:58:50Z

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

ustcweizhou · 2019-10-17T09:02:25Z

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

wido

Looking good! Left a few comments

...ypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java

...vm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtPlugNicCommandWrapper.java

ustcweizhou · 2019-10-28T08:18:10Z

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

andrijapanicsb · 2020-01-13T20:18:04Z

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

yadvr · 2020-01-28T05:20:13Z

@ustcweizhou can you fix the merge conflict?

weizhouapache · 2020-01-28T09:30:37Z

@rhtyd rebased with latest master

weizhouapache · 2020-01-30T08:41:51Z

@blueorangutan package

blueorangutan · 2020-01-30T08:42:24Z

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

blueorangutan · 2020-01-30T09:13:26Z

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

DaanHoogland · 2020-01-30T10:20:57Z

@blueorangutan test

blueorangutan · 2020-01-30T10:21:24Z

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

blueorangutan · 2020-01-30T18:38:08Z

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

DaanHoogland · 2020-01-30T18:55:18Z

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

andrijapanicsb · 2020-02-07T12:59:19Z

@GabrielBrascher interested to test this one perhaps?

GabrielBrascher · 2020-02-07T14:55:44Z

@andrijapanicsb sure, going to test it soon!

yadvr · 2020-02-12T08:04:22Z

@ustcweizhou can you address merge conflict?

…p (and kvm support)

yadvr · 2020-02-14T06:37:21Z

@blueorangutan package

blueorangutan · 2020-02-14T06:38:17Z

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

blueorangutan · 2020-02-14T08:02:18Z

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

andrijapanicsb · 2020-02-14T15:28:09Z

sure, going to test it soon!

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

yadvr · 2020-02-17T15:39:40Z

@blueorangutan package

blueorangutan · 2020-02-17T15:40:36Z

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

blueorangutan · 2020-02-17T16:36:48Z

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

DaanHoogland · 2020-02-17T16:38:34Z

@blueorangutan test

blueorangutan · 2020-02-17T16:39:33Z

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

blueorangutan · 2020-02-18T02:28:13Z

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

GabrielBrascher · 2020-02-18T17:00:40Z

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

GabrielBrascher

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.

GabrielBrascher · 2020-02-19T07:20:21Z

core/src/main/java/com/cloud/agent/api/SecurityGroupRulesCmd.java

    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 = ',';


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.

GabrielBrascher · 2020-02-19T07:25:30Z

...ypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java

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


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.

GabrielBrascher · 2020-02-19T07:51:43Z

engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java

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


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

DaanHoogland

code lgtm, not tested

GabrielBrascher

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

10 tasks

weizhouapache mentioned this pull request Feb 27, 2023

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

Merged

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

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) #3639

Uh oh!

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!

GabrielBrascher Feb 19, 2020

GabrielBrascher Feb 19, 2020 •

edited

Loading