CLOUDSTACK-9287 - Fix unique mac address per rVPC router #1483

remibergsma · 2016-04-09T19:21:34Z

This is work by @wilderrodrigues, see PR #1413 It contains important fixes and I think it needs to be included so I send the PR again.

… nic profile exists

…ace/mac address issues

…ult gw

…o master

…rom a certain router - In case of redundant VPCs, the ACL items are revoked in the first iteration. Since the econd iteration is needed in order to remove the private network, we have to check if the nic profile is gone before trying to revoke the ACL items again, which would throw a NPE. - Some variable extraction in order to ease debugging.

- This also refactors the CsAddress in order to offer better readability in a couple of methods.

…elated changes

alexandrelimassantana · 2016-04-09T20:38:30Z

server/src/com/cloud/network/element/VpcVirtualRouterElement.java

@@ -466,7 +466,7 @@ public boolean deletePrivateGateway(final PrivateGateway gateway) throws Concurr
 }
 }

- return result > 0 ? true : false;
+ return result == routers.size() ? true : false;
 }


This line is equivallent to

return result == routers.size();

please take the ternary operator away

@alexandrelimassantana This code is not very new, please write a little pull request to that extend. (also the issue that you quite justly pointed out is there in the old code as well and bears no weight on the validity of this PR)

DaanHoogland · 2016-04-10T13:53:59Z

@alexandrelimassantana please make a pull request to the branch that the PR is created from, Remi can include you change in this one by pulling it to his branch.

kiwiflyer · 2016-04-11T19:42:04Z

@remibergsma

Is this exactly the same PR as the one Wilder closed last week, or does it have some changes in it?

Also, there are some comments from @dsclose added after the old PR was closed regarding some potential gotchas. See #1413 (comment)

I agree that this (along with the additional fixes proposed above) need to get in to the next release.

Si

QuiteClose · 2016-04-12T07:35:21Z

@kiwiflyer

I'm currently working on CLOUDSTACK-9339 to resolve the issues that I mentioned in #1413 - I don't think this PR breaks anything that currently works so CLOUDSTACK-9339 probably shouldn't hold this PR up.

Dean

…ix typos

remibergsma · 2016-04-12T08:47:23Z

@swill force pushed

rohityadavcloud · 2016-05-02T12:56:39Z

@remibergsma please squash changes to a single commit, and push -f

tag:needlove

swill · 2016-05-07T04:12:37Z

CI RESULTS

Tests Run: 85
  Skipped: 0
   Failed: 1
   Errors: 1
 Duration: 10h 26m 38s

Summary of the problem(s):

FAIL: Test destroy(expunge) Virtual Machine
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/data/git/cs1/cloudstack/test/integration/smoke/test_vm_life_cycle.py", line 646, in test_09_expunge_vm
    self.assertEqual(list_vm_response,None,"Check Expunged virtual machine is in listVirtualMachines response")
AssertionError: Check Expunged virtual machine is in listVirtualMachines response
----------------------------------------------------------------------
Additional details in: /tmp/MarvinLogs/test_vpc_routers_LJEVIW/results.txt

ERROR: Test to verify access to loadbalancer haproxy admin stats page
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/data/git/cs1/cloudstack/test/integration/smoke/test_internal_lb.py", line 854, in tearDown
    raise Exception("Cleanup failed with %s" % e)
Exception: Cleanup failed with Job failed: {jobprocstatus : 0, created : u'2016-05-07T01:17:22+0200', jobresult : {errorcode : 530, errortext : u'Failed to delete network'}, cmd : u'org.apache.cloudstack.api.command.user.network.DeleteNetworkCmd', userid : u'ecdfa0ce-13af-11e6-929e-5254001daa61', jobstatus : 2, jobid : u'7ba837e5-36f2-4467-ab1b-69305fe3c897', jobresultcode : 530, jobresulttype : u'object', jobinstancetype : u'Network', accountid : u'ecdf8a0d-13af-11e6-929e-5254001daa61'}
----------------------------------------------------------------------
Additional details in: /tmp/MarvinLogs/test_network_4LL71G/results.txt

Associated Uploads

/tmp/MarvinLogs/DeployDataCenter__May_06_2016_19_30_38_6T9JP6:

/tmp/MarvinLogs/test_network_4LL71G:

/tmp/MarvinLogs/test_vpc_routers_LJEVIW:

Uploads will be available until 2016-07-07 02:00:00 +0200 CEST

Comment created by upr comment.

swill · 2016-05-12T03:21:41Z

I need some code review on this one. Thanks...

DaanHoogland · 2016-05-12T09:31:31Z

LGTM did a code walk through and I know of a production install this code is running in.

swill · 2016-05-12T12:36:37Z

Can I get one more review on this one?
@remibergsma are you choosing not to squash because you are not the original author of some of the commits? If that is the case, I understand and am fine with it.

dmabry · 2016-05-12T13:59:56Z

I tested this in our 4.8 HW Lab and it worked as expected. I found the MASTER VR for a TEST VPC with a private gateway configured. I started a ping to the private gateway from an instance. Finally I ran virsh destroy against r-XX-VM and only dropped a few pings before the private gateway was moved to the new VR.

LGTM

@wilderrodrigues

CLOUDSTACK-9287 - Fix unique mac address per rVPC routerThis is work by @wilderrodrigues, see PR #1413 It contains important fixes and I think it needs to be included so I send the PR again. * pr/1483: CLOUDSTACK-9287 - Improve test by checking if pvt gw is removed and fix typos CLOUDSTACK-9287 - Fix RVR public interface CLOUDSTACK-9287 - Add integration test to cover the private gateway related changes CLOUDSTACK-9287 - Refactor the interface state configuration CLOUDSTACK-9287 - Check if the nic profile has already been removed from a certain router CLOUDSTACK-9287 - Bring up the private gw interface on state change to master CLOUDSTACK-9287 - Make sure private gw interface is not used for default gw CLOUDSTACK-9287 - Add integration test to cover the private gw interface/mac address issues CLOUDSTACK-9287 - Put private gateway interface down on backup router CLOUDSTACK-9287 - Generate new mac address if router is redundant and nic profile exists Signed-off-by: Will Stevens <[email protected]>

@wilderrodrigues

CLOUDSTACK-9287 - Fix unique mac address per rVPC routerThis is work by @wilderrodrigues, see PR apache#1413 It contains important fixes and I think it needs to be included so I send the PR again. * pr/1483: CLOUDSTACK-9287 - Improve test by checking if pvt gw is removed and fix typos CLOUDSTACK-9287 - Fix RVR public interface CLOUDSTACK-9287 - Add integration test to cover the private gateway related changes CLOUDSTACK-9287 - Refactor the interface state configuration CLOUDSTACK-9287 - Check if the nic profile has already been removed from a certain router CLOUDSTACK-9287 - Bring up the private gw interface on state change to master CLOUDSTACK-9287 - Make sure private gw interface is not used for default gw CLOUDSTACK-9287 - Add integration test to cover the private gw interface/mac address issues CLOUDSTACK-9287 - Put private gateway interface down on backup router CLOUDSTACK-9287 - Generate new mac address if router is redundant and nic profile exists Signed-off-by: Will Stevens <[email protected]> Conflicts: server/src/com/cloud/network/vpc/NetworkACLManagerImpl.java

wilderrodrigues and others added 9 commits April 9, 2016 21:13

CLOUDSTACK-9287 - Generate new mac address if router is redundant and…

f921ec6

… nic profile exists

CLOUDSTACK-9287 - Put private gateway interface down on backup router

d93b008

CLOUDSTACK-9287 - Add integration test to cover the private gw interf…

250be37

…ace/mac address issues

CLOUDSTACK-9287 - Make sure private gw interface is not used for defa…

057b54a

…ult gw

CLOUDSTACK-9287 - Bring up the private gw interface on state change t…

6a76773

…o master

CLOUDSTACK-9287 - Refactor the interface state configuration

c41edc1

- This also refactors the CsAddress in order to offer better readability in a couple of methods.

CLOUDSTACK-9287 - Add integration test to cover the private gateway r…

0e91468

…elated changes

CLOUDSTACK-9287 - Fix RVR public interface

78bbd49

alexandrelimassantana reviewed Apr 9, 2016
View reviewed changes

CLOUDSTACK-9287 - Improve test by checking if pvt gw is removed and f…

799b9f2

…ix typos

remibergsma force-pushed the pr1413-wilder-47 branch from ffd8fdb to 799b9f2 Compare April 12, 2016 08:47

asfgit merged commit 799b9f2 into apache:4.7 May 12, 2016

remibergsma mentioned this pull request Jan 21, 2017

Updated StrongSwan VPN Implementation #1741

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CLOUDSTACK-9287 - Fix unique mac address per rVPC router #1483

CLOUDSTACK-9287 - Fix unique mac address per rVPC router #1483

remibergsma commented Apr 9, 2016

alexandrelimassantana Apr 9, 2016

DaanHoogland May 12, 2016

DaanHoogland commented Apr 10, 2016

kiwiflyer commented Apr 11, 2016

QuiteClose commented Apr 12, 2016

remibergsma commented Apr 12, 2016

rohityadavcloud commented May 2, 2016

swill commented May 7, 2016

swill commented May 12, 2016

DaanHoogland commented May 12, 2016

swill commented May 12, 2016

dmabry commented May 12, 2016

CLOUDSTACK-9287 - Fix unique mac address per rVPC router #1483

CLOUDSTACK-9287 - Fix unique mac address per rVPC router #1483

Conversation

remibergsma commented Apr 9, 2016

alexandrelimassantana Apr 9, 2016

Choose a reason for hiding this comment

DaanHoogland May 12, 2016

Choose a reason for hiding this comment

DaanHoogland commented Apr 10, 2016

kiwiflyer commented Apr 11, 2016

QuiteClose commented Apr 12, 2016

remibergsma commented Apr 12, 2016

rohityadavcloud commented May 2, 2016

swill commented May 7, 2016

CI RESULTS

swill commented May 12, 2016

DaanHoogland commented May 12, 2016

swill commented May 12, 2016

dmabry commented May 12, 2016