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

wido · 2019-07-17T07:32:20Z

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:~#

RPDiep · 2019-07-17T07:36:07Z

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

yadvr · 2019-07-17T10:55:57Z

@blueorangutan package

blueorangutan · 2019-07-17T10:56:41Z

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

blueorangutan · 2019-07-17T11:29:06Z

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-141

yadvr · 2019-07-17T14:38:56Z

@blueorangutan test

blueorangutan · 2019-07-17T14:39:42Z

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

blueorangutan · 2019-07-17T23:26:12Z

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

nvazquez

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

plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/IvsVifDriver.java

nvazquez · 2019-07-18T02:03:35Z

plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/BridgeVifDriver.java

-            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));


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.

nvazquez · 2019-07-18T02:04:51Z

...ins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/BridgeVifDriverTest.java

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


Extra line :)

nvazquez · 2019-07-18T02:07:20Z

utils/src/main/java/com/cloud/utils/net/NetUtils.java

    }

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


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.

yadvr · 2019-07-22T12:01:21Z

@wido cc @GabrielBrascher - can you review the oustanding comments. Tests LGTM.
@blueorangutan package

blueorangutan · 2019-07-22T12:02:06Z

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

blueorangutan · 2019-07-22T12:42:13Z

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-168

yadvr · 2019-07-22T12:42:55Z

@blueorangutan test

blueorangutan · 2019-07-22T12:44:04Z

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

yadvr · 2019-07-22T16:29:30Z

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

blueorangutan · 2019-07-22T23:07:20Z

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

wido · 2019-07-23T19:22:54Z

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

yadvr · 2019-07-24T05:48:13Z

@wido can you comment on the IvsVifDriver removal?

wido · 2019-07-24T05:57:34Z

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

yadvr · 2019-07-24T06:03:33Z

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

yadvr

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>

wido · 2019-07-24T10:01:52Z

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

GabrielBrascher

Code LGTM

nvazquez

LGTM, thanks @wido

nvazquez · 2019-07-24T13:50:27Z

plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/IvsVifDriver.java

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

-        libvirtVersion = (Long) params.get("libvirtVersion");


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

Not being used indeed. Dead variable which was never called

nvazquez · 2019-07-24T13:53:33Z

utils/src/main/java/com/cloud/utils/net/NetUtils.java

    }

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


yadvr · 2019-07-24T15:46:32Z

@blueorangutan package

blueorangutan · 2019-07-24T15:47:14Z

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

blueorangutan · 2019-07-24T16:48:46Z

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

yadvr · 2019-07-24T16:54:42Z

@blueorangutan package

blueorangutan · 2019-07-24T16:55:14Z

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

blueorangutan · 2019-07-24T17:31:12Z

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-183

yadvr · 2019-07-24T18:03:27Z

@blueorangutan test

blueorangutan · 2019-07-24T18:05:15Z

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

blueorangutan · 2019-07-25T05:34:54Z

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

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!

yadvr commented Jul 24, 2019

Uh oh!

blueorangutan commented Jul 24, 2019