vrouter in redundant mode acquire guest ips from first ip of the tier by ArthurHlt · Pull Request #3587 · apache/cloudstack

ArthurHlt · 2019-09-09T09:37:35Z

Description

Vrouter in redundant mode actually acquire guest ips in a tier in a random mode.
This is painful when you use terraform or other systems to acquire specific IP because vrouter can take the one selected.

It looks like a bug more than an intended behaviour.

With this PR vrouter in redundant mode will now take first available ips in the tier which can be predicted

Types of changes

How Has This Been Tested?

This is tested against production which works great.
There is no unit test as it looks like there is no in this part of the code, it also not breaking anything and can be considered as a bug fix.

add patch to stop random attribution of ip in a guest network for vrouter

svenvogel · 2019-09-09T20:42:49Z

@ArthurHlt you use redundant vrouter? which hypervisor?

ArthurHlt · 2019-09-10T18:40:01Z

@svenvogel thanks for reaching me. yes we use redundant vrouter on xenserver hypervisors.

andrijapanicsb · 2020-01-03T21:52:57Z

@blueorangutan package

blueorangutan · 2020-01-03T21:53:27Z

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

blueorangutan · 2020-01-03T22:32:13Z

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

yadvr · 2020-01-28T05:12:07Z

@blueorangutan package

blueorangutan · 2020-01-28T05:12:14Z

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

blueorangutan · 2020-01-28T06:14:12Z

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

weizhouapache · 2020-01-28T09:41:07Z

@ArthurHlt @rhtyd
can we have a global settings for it ?
some companies use the first 3 ips as gateway, some companies use the last 3 ips as gateway.
maybe we can specify it in global setting (options: first, last, random)

yadvr · 2020-02-06T06:05:06Z

@blueorangutan package

blueorangutan · 2020-02-06T06:05:56Z

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

blueorangutan · 2020-02-06T06:45:20Z

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

andrijapanicsb · 2020-02-07T12:44:48Z

@blueorangutan test

blueorangutan · 2020-02-07T12:45:02Z

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

ArthurHlt · 2020-02-07T13:25:42Z

@weizhouapache it's actually ip inside vpc tiers created by cloudstack when you set redundant mode.
There is nothing about vip or vlan context, it's only inside vpc.
By the way, when you are not in redundant mode, vrouter take the first ip in tiers.

But maybe I missed something here

weizhouapache · 2020-02-07T14:30:40Z

@weizhouapache it's actually ip inside vpc tiers created by cloudstack when you set redundant mode.
There is nothing about vip or vlan context, it's only inside vpc.
By the way, when you are not in redundant mode, vrouter take the first ip in tiers.

But maybe I missed something here

@ArthurHlt when you create a vpc tier, the gateway ip can be any ip in the cidr (.1, .254, .XX).
if vr is not redundant, the ip of guest nic in VR will be set to gateway ip.

DaanHoogland

@ArthurHlt I can see how and why you want to change this behaviour but it is actually by design. A @weizhouapache describes we'll need a setting to steer the behaviour and prevent your patch for breaking other people's clouds. sorry, your code looks fine.

ArthurHlt · 2020-02-07T15:33:25Z

@DaanHoogland @weizhouapache That's all make sense to me.
Well i want to pursue the PR for adressing these changes you have requested. I'm just wondering how to add parameters like this ? If a gentleman can give me some code to look at for adding this, i will be very pleased

DaanHoogland · 2020-02-07T15:53:56Z

@ArthurHlt if you promise never to call me a gentleman again; in the IpAddressManager that you added your new method in, you can see an example of a ConfigKey in the beginning of the interface. in IpAddressManagerImpl search for it being used (and further advertised on runtime). GuestNetworkGuru has similar config items, you can choose to put them in any Configurable.

blueorangutan · 2020-02-08T00:13:56Z

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

Test	Result	Time (s)	Test File

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

@blueorangutan package

blueorangutan · 2020-02-12T08:05:09Z

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

blueorangutan · 2020-03-07T08:46:18Z

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

blueorangutan · 2020-03-07T09:11:34Z

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

DaanHoogland · 2020-03-07T21:25:53Z

@blueorangutan test

blueorangutan · 2020-03-07T21:26:19Z

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

blueorangutan · 2020-03-08T08:10:57Z

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

Test	Result	Time (s)	Test File

DaanHoogland · 2020-03-08T08:14:52Z

@weizhouapache can you give this a second look, please?

weizhouapache · 2020-03-08T08:54:35Z


        if (vpcRouterDeploymentDefinition.isRedundant()) {
-            guestNic.setIPv4Address(_ipAddrMgr.acquireGuestIpAddress(guestNetwork, null));
+            guestNic.setIPv4Address(_ipAddrMgr.acquireFirstGuestIpAddress(guestNetwork));


@ArthurHlt should acquireGuestIpAddressForVrouterRedundant be used here as well?

weizhouapache · 2020-03-08T08:54:38Z

            if (routerDeploymentDefinition.isPublicNetwork()) {
                if (routerDeploymentDefinition.isRedundant()) {
-                    gatewayNic.setIPv4Address(_ipAddrMgr.acquireGuestIpAddress(guestNetwork, null));
+                    gatewayNic.setIPv4Address(_ipAddrMgr.acquireFirstGuestIpAddress(guestNetwork));


@ArthurHlt should acquireGuestIpAddressForVrouterRedundant be used here ?

ArthurHlt · 2020-03-08T16:37:10Z

@weizhouapache thanks for the catch, i've totally forget it.

I've reworked all enum and config that i've made to be able to retrieve configkey value in router package. This actually now look more clean inside but I'm stilling feeling bad to do this in router package: https://github.com/apache/cloudstack/pull/3587/files#diff-2df46c6f8af49cedaa2a089ace159b03R890

ArthurHlt · 2020-03-12T01:44:47Z

@weizhouapache thanks for the hint, this is a better way to handle this. What's your opinion now ?

andrijapanicsb · 2020-03-13T09:59:07Z

ping @weizhouapache - any chance to get it in today?

weizhouapache · 2020-03-13T10:04:18Z

@andrijapanicsb I will test it.

weizhouapache · 2020-03-13T10:46:43Z

@ArthurHlt can you remove some unused import ?

ArthurHlt · 2020-03-13T13:34:10Z

@weizhouapache done ;)

weizhouapache · 2020-03-13T15:56:09Z

@andrijapanicsb tested with isolated networks and vpc with redundant VRs. both work fine.
It is ready for merge I think.

andrijapanicsb · 2020-03-13T16:06:51Z

@blueorangutan package

blueorangutan · 2020-03-13T16:07:46Z

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

blueorangutan · 2020-03-13T16:32:13Z

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

DaanHoogland · 2020-03-13T16:41:10Z

@blueorangutan test

blueorangutan · 2020-03-13T16:41:45Z

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

DaanHoogland · 2020-03-13T19:08:22Z

nothing outstanding, LGTM*2 and tested. let's merge when the monkey agrees.

andrijapanicsb · 2020-03-13T19:21:19Z

Will merge if tests are finished with no errors, despite the freeze date, as everything else is OK atm (manul test, approvals, no conflicts)

blueorangutan · 2020-03-14T02:21:15Z

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

Test	Result	Time (s)	Test File

ArthurHlt added 2 commits November 8, 2018 19:35

remove random chose of ip in guest network for vrouter

171aa26

add patch to stop random attribution of ip in a guest network for vrouter

Merge branch 'master' into order-ip-red-router

7b49d46

svenvogel added component:virtual-router type:bug type:enhancement labels Sep 9, 2019

yadvr added this to the 4.14.0.0 milestone Dec 7, 2019

andrijapanicsb requested a review from DaanHoogland January 4, 2020 14:24

yadvr requested a review from weizhouapache January 28, 2020 05:12

DaanHoogland requested changes Feb 7, 2020

View reviewed changes

ArthurHlt added 2 commits March 7, 2020 23:35

rework style code

b5c7e5e

fix mvn check style ?

5489539

weizhouapache reviewed Mar 8, 2020

View reviewed changes

refactor placement and rework acquiring ip in router

eb54d74

weizhouapache reviewed Mar 11, 2020

View reviewed changes

Comment thread engine/components-api/src/main/java/com/cloud/network/IpAddressManager.java Outdated

place vrouter placement concept in addr manager

f89a564

rm unused imports

c6e315f

weizhouapache approved these changes Mar 13, 2020

View reviewed changes

DaanHoogland merged commit 3575f5e into apache:master Mar 14, 2020

Conversation

ArthurHlt commented Sep 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Types of changes

How Has This Been Tested?

Uh oh!

svenvogel commented Sep 9, 2019

Uh oh!

ArthurHlt commented Sep 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andrijapanicsb commented Jan 3, 2020

Uh oh!

blueorangutan commented Jan 3, 2020

Uh oh!

blueorangutan commented Jan 3, 2020

Uh oh!

yadvr commented Jan 28, 2020

Uh oh!

blueorangutan commented Jan 28, 2020

Uh oh!

blueorangutan commented Jan 28, 2020

Uh oh!

weizhouapache commented Jan 28, 2020

Uh oh!

yadvr commented Feb 6, 2020

Uh oh!

blueorangutan commented Feb 6, 2020

Uh oh!

blueorangutan commented Feb 6, 2020

Uh oh!

andrijapanicsb commented Feb 7, 2020

Uh oh!

blueorangutan commented Feb 7, 2020

Uh oh!

ArthurHlt commented Feb 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

weizhouapache commented Feb 7, 2020

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

ArthurHlt commented Feb 7, 2020

Uh oh!

DaanHoogland commented Feb 7, 2020

Uh oh!

blueorangutan commented Feb 8, 2020

Uh oh!

yadvr commented Feb 12, 2020

Uh oh!

blueorangutan commented Feb 12, 2020

Uh oh!

blueorangutan commented Mar 7, 2020

Uh oh!

blueorangutan commented Mar 7, 2020

Uh oh!

DaanHoogland commented Mar 7, 2020

Uh oh!

blueorangutan commented Mar 7, 2020

Uh oh!

blueorangutan commented Mar 8, 2020

Uh oh!

DaanHoogland commented Mar 8, 2020

Uh oh!

weizhouapache Mar 8, 2020

Choose a reason for hiding this comment

Uh oh!

weizhouapache Mar 8, 2020

Choose a reason for hiding this comment

Uh oh!

ArthurHlt commented Mar 8, 2020

Uh oh!

Uh oh!

ArthurHlt commented Mar 12, 2020

Uh oh!

andrijapanicsb commented Mar 13, 2020

Uh oh!

weizhouapache commented Mar 13, 2020

ArthurHlt commented Sep 9, 2019 •

edited

Loading

ArthurHlt commented Sep 10, 2019 •

edited

Loading

ArthurHlt commented Feb 7, 2020 •

edited

Loading

ArthurHlt commented Mar 13, 2020 •

edited

Loading