CLOUDSTACK-9114: Reduce VR downtime during network restart#2508
Conversation
|
goooood @rhtyd |
|
looks promising! Can not wait to test it, 👍 |
|
looks good @rhtyd |
DaanHoogland
left a comment
There was a problem hiding this comment.
good so far. Are we going to allow for more then 2 routers in redundancy sometime soon?
|
|
||
| int routerCounts = 1; | ||
| if (offering.getRedundantRouter()) { | ||
| if (offering.getRedundantRouter() || network.isRollingRestart()) { |
There was a problem hiding this comment.
Unlikely we'll have more than 2 routers for redundancy. However, the code has an internal limit of 5 (after which it logs warning/errors that a network has more than 5 routers, which can happen in some edge cases).
|
Never mind. It was a problem with my hosts. Everything was working just fine with this PR and the new system vms 👍 |
|
@rafaelweingartner great, looking forward to your review and test results. |
|
Trillian test result (tid-2636)
|
|
@blueorangutan test |
|
@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
|
The last test results (xenserver) shows no new errors caused by this PR, and other tests are incoming soon. Given we've two LGTMs on it from @DaanHoogland and @GabrielBrascher , we may merge this but this PR as soon as test pass BUT since this adds a new operational enhancement I would like to request - @rafaelweingartner @resmo @nvazquez @wido @ustcweizhou @mike-tutkowski @fmaximus @jayapalu @marcaurele @PaulAngus @sureshanaparti @terbolous @NuxRo for their feedback, and I'm happy to move this to next 4.11.2.0 milestone if we've consensus aorund this. Please respond by the end of this week, next week will focus only on blockers and start work towards pre-RC1 testing. Thanks. (Please follow links for packages and systemvmtemplate if you're not looking for building it yourself: |
| } | ||
|
|
||
| public Boolean getMakeredundant() { | ||
| if (makeredundant != null) { |
There was a problem hiding this comment.
Is there is a default value, why not use a primitive value with a default of false? Then, you do not need these logics in a POJO get method.
There was a problem hiding this comment.
@rafaelweingartner when the option is not passed, the value should be false which is what the fix it.
There was a problem hiding this comment.
I see what you want, to set a default 'false' value and simply do a return.
There was a problem hiding this comment.
That is it :)
It is not great change, but it achieves the same result with less logic (conditionals) in the code.
| return cleanup; | ||
| } | ||
| return true; | ||
| return false; |
There was a problem hiding this comment.
The same is valid here
There was a problem hiding this comment.
@rafaelweingartner when the option is not passed, the value should be false which is what the fix it.
There was a problem hiding this comment.
This means that the default is false, right?
Can't you declare the variable as private boolean cleanup = false?
| VirtualRouter backupRouter = null; | ||
| for (final VirtualRouter router : remainingRouters) { | ||
| if (router.getRedundantState() == VirtualRouter.RedundantState.BACKUP) { | ||
| backupRouter = router; |
There was a problem hiding this comment.
Are these routers ordered some how? I mean, reading your next discussion with @DaanHoogland, it seems that the router we do not need will always be the last one (the third or fourth one) when this code is executed. Therefore, there must be some way to order this list before you reach here, right?
| backupRouter = routers.get(routers.size() - 1); | ||
| } | ||
| if (backupRouter != null) { | ||
| _routerService.destroyRouter(backupRouter.getId(), context.getAccount(), context.getCaller().getId()); |
There was a problem hiding this comment.
Javadocs are useful to keep the history behind these things. Keep in mind that the method is only public (java delimiter access); this means accessible to programmers to re-use its code. It is not something that end-users will have access to.
| } | ||
|
|
||
| @Override | ||
| public boolean validateNewRouters(final List<? extends VirtualRouter> routers) { |
There was a problem hiding this comment.
Instead of validate, what about areNewRoutersrunning?
It is not a validation method per se. It does not throw exceptions. This method is only checking if all routers of the giving list are in running state.
There was a problem hiding this comment.
This method is used to validate the restart operation, if false is returned you should see error in API/UI. Will change the name.
| @@ -227,7 +227,7 @@ public boolean implement(final Network network, final NetworkOffering offering, | |||
| final List<DomainRouterVO> routers = routerDeploymentDefinition.deployVirtualRouter(); | |||
|
|
|||
| int routerCounts = 1; | |||
There was a problem hiding this comment.
Instead of routerCounts, what about expectedNumberRouter or something similar. This is the number of VRs you expect to have in a network, right?
There was a problem hiding this comment.
@rafaelweingartner good idea, maybe for a next/different refactoring PR.
| vpc.setRollingRestart(true); | ||
| } | ||
| startVpc(vpc, dest, context); | ||
| if (oldRouters.size() > 0) { |
There was a problem hiding this comment.
Can you explain this logic here? I mean, it is the same as the one at line 2456, but when it solves to true there, we set vpc.setRollingRestart(true); and then we set vpc.setRollingRestart(false); here. Is this something that was done only to satisfy some conditional at startVpc?
There was a problem hiding this comment.
@rafaelweingartner if there are any existing old routers, we do rolling restart which is to deploy a new VR and then destroy old VRs. The setRollingRestart is a way to tell lower facade to treat deployment like for redundant routers, i.e. expect upto 2 VRs at a given point which is seen in changes in VirtualRouterElement.java. The startVpc will call the facade to deploy suitable number of routers. If there is only one old VPC VR, and if rolling restart is true this will cause the lower layers to deploy a new VPC VR. After that, we set the rolling restart to false which works great both in case of rVR (expected VRs is 2) and non-rVRs (expected VRs becomes 1), and finally we destroy old VR. If the VPC has rVRs, a new backup router is created when startVpc is called otherwise this will simply reconfigure existing VRs.
There was a problem hiding this comment.
Thanks! This is a good piece of documentation.
|
@rhtyd I have just reviewed the code. I tested it yesterday and everything was fine. I had only one problem, but that was an environment one. One of my hosts was not working properly. |
|
Thanks @rafaelweingartner and @DaanHoogland I've tried to address all the review comments, let me know if I've missed anything. Wrt to testing, existing lifecycle tests in VPC, network and routers related smoketests validate the restart operation. |
|
@blueorangutan package |
|
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Trillian test result (tid-2635)
|
This introduces a rolling restart of VRs when networks are restarted with cleanup option. Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>
Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>
Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>
Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>
Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>
Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>
Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>
|
XenServer and Vmware tests are LGTM, 1-2 failure seen in intermittent/env related. I've rebase and fixed conflicts against latest 4.11, will kick test against KVM after packaging. |
|
@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-2036 |
|
@blueorangutan test |
|
@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
|
Trillian test result (tid-2651)
|
|
Tests LGTM, we've enough reviews to merge this. If there are no objections, this will be merged by end of the day tomorrow. |
|
@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-2043 |
Description
Every time there is a major CloudStack version that requires a new systemvmtemplate, admins need to restart VRs which can take few (sometimes several) minutes. The aim is to make it easier to perform a rolling restart (or blue-green deployment) of the network with acceptable downtime. In this strategy, a new VR is provisioned before the old VR is destroyed/cleaned for non-redundant isolated VRs. For redundant networks, a rolling replacement of routers is performed.
The network downtime is usually the short-time (less than 10s) after the new VR starts (nics are up) and old VR is destroyed. The downtime may be affected by several factors as the new VR has a different mac address, stale ARP cache may take upto 30seconds (default
gc_timeouton Linux) to renew in guests making egress traffic downtime slightly higher than ingress traffic (ingress, i.e. public network requesting access to VR's public ip). As an illustration, ingress traffic such as a http request or ssh request to the VR's public IP that port-forwards to guest VM may face lower downtime than egress traffic (say a ping or curl from guest VM to public Internet). Among several environment factors, the following interval/timeouts affect how the old VR's ip-mac address is cached:To make things faster, a gratituous ARP notification is sent by the new VR (again) after the old VR is successfully destroyed.
Project Goals:
Future:
Types of changes
Screenshots (if appropriate):
Non-Redundant Isolated Network VRs
Rolling restart in action, a new VR is deployed lifing the VR as a redundant VR:

The old VR is stopped and destroyed:

The new VR's redundancy is removed and is reprogrammed:

Meanwhile, the following screenshot shows ping drops from a guest VM, in a non-redundant isolated network: (usually 2-10 ping drops observed)

Redundant Isolated Network VR
Rolling restart starts by stopping old backup VR:

New backup VR deployed:

Old master stopped, new backup becomes new master:

Old master destroyed and another backup VR deployed:

Redundant VRs after rollling reboot:

In the meanwhile, usually 0-4 ping drops observed in guest VM:

How Has This Been Tested?
Tested with following, benchmarks show rounded off avg/max downtime seconds for restarting non-redundant isolated VRs when
cleanup=true:** The downtime seconds may be higher in real-world environment under heavy load. Redundant routers show a downtime of 0-4 seconds.
*** All used a CentOS7 based management server, tested in environments deployed by Trillian
Ingress traffic was tested using ssh and http services hosted on the guest VM. Example, sshping like utility I wrote for calculating ingress traffic downtime: (number of fail count = downtime seconds)
Checklist:
@blueorangutan package