kvm/bridge: Allow Link Local Cidr (cloud0 interface) to be configured by wido · Pull Request #3500 · apache/cloudstack · GitHub

Description

There are certain scenarios where the 169.254.0.0/16 subnet is used for different
purposes then CloudStack on a hypervisor.

Once of such scenarios is a BGP+EVPN+VXLAN setup using BGP Unnumbered where the
169.254.0.1 address is used by Frr/Zebra BGP routing to send traffic to the
neighboring router.

The following settings can be changed in the agent.properties (default values added):

control.cidr=169.254.0.0/16

Make sure the global setting 'control.cidr' matches the values defined in the agent.propeties!

In the future the mgmt server can send this parameter to a KVM Agent on startup, but at the moment
this framework is not in place and thus these values can't be send to the Agent in a proper manner.

Fixes: #3488

Types of changes

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)

How Has This Been Tested?

Ran local test on our cloud environment and verified cloud0 now has a different address.

root@hv-138-a05-23:~# ip addr show cloud0
58: cloud0:  mtu 1500 qdisc noqueue state UP group default qlen 1000
    link/ether fe:00:a9:fe:f0:c9 brd ff:ff:ff:ff:ff:ff
    inet 169.254.240.1/20 scope global cloud0
       valid_lft forever preferred_lft forever
    inet6 fe80::8c3c:82ff:fe5a:77c/64 scope link 
       valid_lft forever preferred_lft forever
root@hv-138-a05-23:~#

In addition, also tested and verified: systemvms do get their ips from within the control.cidr and they are fully functional.

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

@blueorangutan test

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

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

Test	Result	Time (s)	Test File
test_04_rvpc_internallb_haproxy_stats_on_all_interfaces	Error	201.73	test_internal_lb.py

Thanks for the PR @wido, I left some comments after reviewing it

How about keeping NetUtils.getLinkLocalGateway() but extending it to accept a CIDR parameter? As internally it invokes getLinkLocalFirstAddressFromCIDR as well

Could be, but there aren't many methods calling it. I see what you mean, I don't have a try preference for it though.

Extra line :)

Argh!

Same as the comment above, instead of returning a hardcoded value, this method can return the configured CIDR and can make getLinkLocalGateway() above to be consistent with it.

Do we have access to the configuration here? I don't think so. Therefor I changed it this way.

Looks good

@wido cc @GabrielBrascher - can you review the oustanding comments. Tests LGTM.
@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-168

@blueorangutan test

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

Left some remark, some changes I'll need to re-review, meanwhile I'll wait for regression tests to come back.

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

Test	Result	Time (s)	Test File
test_05_rvpc_multi_tiers	Failure	417.48	test_vpc_redundant.py
test_05_rvpc_multi_tiers	Error	446.76	test_vpc_redundant.py

I think I addressed the comments above. Some seem to be a coding style thing but not really a technical change.

Let me know if this PR is LGTM

@wido can you comment on the IvsVifDriver removal?

@rhtyd I did? The file is not referenced anywhere. I also think that it was a typo when the file was created.

@wido nope, that's not a typo it's a driver for the indigo virtual switch introduced in #151 and it's not used anywhere because this is something one configures for the libvirt.vif.driver= in the agent.properties file. Kindly revert that change.

Changes requested.

There are certain scenarios where the 169.254.0.0/16 subnet is used for different purposes then CloudStack on a hypervisor. Once of such scenarios is a BGP+EVPN+VXLAN setup using BGP Unnumbered where the 169.254.0.1 address is used by Frr/Zebra BGP routing to send traffic to the neighboring router. The following settings can be changed in the agent.properties (default values added): control.cidr=169.254.0.0/16 Make sure the global setting 'control.cidr' matches the values defined in the agent.propeties! In the future the mgmt server can send this parameter to a KVM Agent on startup, but at the moment this framework is not in place and thus these values can't be send to the Agent in a proper manner. This commit also: - Adds Unit Tests - Removes the dead IvsVifDriver Signed-off-by: Wido den Hollander <wido@widodh.nl>

@rhtyd I see! The file is back with a few modifications.

@nvazquez I also took another look at your comments and they should be adressed now.

Code LGTM

LGTM, thanks @wido

Looks like libvirtVersion was not being used on this class, is that right?

Not being used indeed. Dead variable which was never called

Looks good

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

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

@blueorangutan test

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

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

Test	Result	Time (s)	Test File
test_05_rvpc_multi_tiers	Failure	433.07	test_vpc_redundant.py
test_05_rvpc_multi_tiers	Error	462.51	test_vpc_redundant.py

wido added component:kvm component:vxlan VXLAN labels Jul 17, 2019

wido added this to the 4.13.0.0 milestone Jul 17, 2019

wido requested review from GabrielBrascher, nvazquez and yadvr July 17, 2019 07:32

wido mentioned this pull request Jul 17, 2019

KVM: Propagating changes on host parameters to the agents #3491

Merged

5 tasks

nvazquez requested changes Jul 18, 2019

View reviewed changes

yadvr requested changes Jul 24, 2019

View reviewed changes

wido force-pushed the cloud0-cidr branch from 212647b to 2a2fed7 Compare July 24, 2019 09:02

wido force-pushed the cloud0-cidr branch from 2a2fed7 to 1c9e6d2 Compare July 24, 2019 09:51

GabrielBrascher approved these changes Jul 24, 2019

View reviewed changes

nvazquez approved these changes Jul 24, 2019

View reviewed changes

yadvr approved these changes Jul 25, 2019

View reviewed changes

yadvr merged commit e894658 into apache:master Jul 25, 2019

-                          Script.runSimpleBashScript("ip address add 169.254.0.1/16 dev " + linkLocalBr + ";" + "ip route add " + NetUtils.getLinkLocalCIDR() + " dev " + linkLocalBr + " src " +
-                                  NetUtils.getLinkLocalGateway());
+                          Script.runSimpleBashScript("ip address add " + NetUtils.getLinkLocalAddressFromCIDR(_controlCidr) + " dev " + linkLocalBr);
+                          Script.runSimpleBashScript("ip route add " + _controlCidr + " dev " + linkLocalBr + " src " + NetUtils.getLinkLocalFirstAddressFromCIDR(_controlCidr));

                       Assert.assertTrue(driver.isValidProtocolAndVnetId("123", "vlan"));
                       Assert.assertTrue(driver.isValidProtocolAndVnetId("456", "vxlan"));
                   }

                   }
                   public static String getLinkLocalCIDR() {
                       return "169.254.0.0/16";

                       }
                       _ivsIfUpPath = Script.findScript(utilScriptsDir, "qemu-ivs-ifup");
-                      libvirtVersion = (Long) params.get("libvirtVersion");

kvm/bridge: Allow Link Local Cidr (cloud0 interface) to be configured by wido · Pull Request #3500 · apache/cloudstack · GitHub

kvm/bridge: Allow Link Local Cidr (cloud0 interface) to be configured - #3500

kvm/bridge: Allow Link Local Cidr (cloud0 interface) to be configured#3500yadvr merged 1 commit intoapache:masterapache/cloudstack:masterfrom CLDIN:cloud0-cidrCLDIN/cloudstack:cloud0-cidrCopy head branch name to clipboard

Conversation

wido commented Jul 17, 2019

Description

Types of changes

How Has This Been Tested?

Uh oh!

RPDiep commented Jul 17, 2019

Uh oh!

yadvr commented Jul 17, 2019

Uh oh!

blueorangutan commented Jul 17, 2019

Uh oh!

blueorangutan commented Jul 17, 2019

Uh oh!

yadvr commented Jul 17, 2019

Uh oh!

blueorangutan commented Jul 17, 2019

Uh oh!

blueorangutan commented Jul 17, 2019

Uh oh!

nvazquez left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yadvr commented Jul 22, 2019

Uh oh!

blueorangutan commented Jul 22, 2019

Uh oh!

blueorangutan commented Jul 22, 2019

Uh oh!

yadvr commented Jul 22, 2019

Uh oh!

blueorangutan commented Jul 22, 2019

Uh oh!

yadvr commented Jul 22, 2019

Uh oh!

blueorangutan commented Jul 22, 2019

Uh oh!

wido commented Jul 23, 2019

Uh oh!

yadvr commented Jul 24, 2019

Uh oh!

wido commented Jul 24, 2019

Uh oh!

yadvr commented Jul 24, 2019

Uh oh!

yadvr left a comment

Choose a reason for hiding this comment

Uh oh!

wido commented Jul 24, 2019

Uh oh!

GabrielBrascher left a comment

Choose a reason for hiding this comment

Uh oh!

nvazquez left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kvm/bridge: Allow Link Local Cidr (cloud0 interface) to be configured#3500
yadvr merged 1 commit into
apache:masterfrom
CLDIN:cloud0-cidr