Pull request of changes in the "cloud-server" module

[wilderrodrigues]

In the last 10 weeks we have been working in the cloud-server, focusing our time in the refactor of the [Vpc]VirtualNetworkApplianceManagerImpl. We had a mains goals increase of Maintainability, Extensibility, Readability and test coverage. That was just a first step towards the development, still in progress, of the Redundant Virtual Routers for VPC.

== What has been done so far:

• The VirtualNetworkApplianceManagerImpl class line numbers dropped from 4440 to 2558
• The VpcVirtualNetworkApplianceImpl class line numbers dropped from 1484 to 749
• We created 35 new classes in order to split the code/responsibility
• We added 97.8% unit test coverage for com.cloud.network.element/router and org.cloud.network.router.deployment packages
o The most complex classes we changed are in those packages
o About 1700 lines of unit tests
• We started doing some integration tests
o Deployment of Basic and Advanced Zones
o Test Create Account and user for that account
o Test Sub domain allowed to launch VM when a Domain level zone is created
o Test delete domain without force option
o Test delete domain with force option
o Test update admin details
o Test update domain admin details
o Test user update API
o Test login API with domain
o Test if Login API does not return UUID's
• VPC related tests were still done manually.
o Will have it automated as well.

We started the changes in the network area, trying to identify the differences in the 2 types of network we have. For that we created Basic and Advanced Network Topology classes. The network topology classes are responsible by invoking the Apply/Setup/Create/Save rules that were previously done by the [Vpc]VirtualNetworkAppliance. A topology instance is retrieved via a context object that is injected in the [Vpc]VirtualElement. The context object will return the most appropriate topology instance based on the Network Type, which is defined in the Data Centre. That was the first step towards the refactor.

From the topology class we reach the Rule Applier implementation that will be used to do all the rule setup preparation (i.e. invoke DAOs and prepare the command object). The RuleApplier interface was extracted from the VirtualNetworkApplianceManagerImpl, where it use to be an inner interface. For each anonymous implementation of the RuleApplier we created a concrete class. The rules are used as elements of a Visitor class, which will perform some extra logic, depending on the rule it's visiting, and call the send commands to router method. The latter has also been extracted from the VirtualNetworkApplianceManagerImpl and is now in a new helper class: NetworkHelperImpl.

The visitor has been used because we were aiming to split the responsibility and also because the way the RuleApplier was implemented before, it was clear that every command sent to the router was following a 2-steps approach: gather information to create the commands, apply some logic to send to the router. For those reason we implemented the visitor pattern. Since we already had the Basic/Advanced Network Topology classes, we created 2 concrete classes to visit the rules: Basic/Advanced Network Visitors. Both classes extend the abstract class NetworkTopologyVisitor, which defines all the visit methods per type of rule. By doing so, we can use the same rule and separate the logic based on the type of visitor that we have - Basic or Advanced.

Continuing on the refactor, we also added some helper classes for the "getSomething" related methods. Following this approach we ended up having the following classes:

• NetworkHelper (interface)
• NetworkHelperImpl
• VpcNetworkHelperImpl
• CommandSetupHelper
• NicProfileHelper
• RouterControlHelper

Last, but not least - and actually the most crucial part of the code - there was also a huge refactor in terms of how the routers are deployed. The previous deployeRouter and deployVpcInrouter methods do not exist any more. Instead of having the logics spread, or sometime tangled, in the [Vpc]VirtualNetworkApplianceManagerImpl, we have created a Router Deployment Definition mechanism, with classes that follow the same naming convention. The deployment definition has 2 implementations, Router and Vpc router, which are created with the aid of a Builder class. Most of the work, which is common to both implementation, is being done by the RouterDeploymentDefinition class. The specific bits are done by their implementation. for example, when findOrDeployVirtualrouter() method is called, it will make sure that precondition are checked, deployment plan is done and generated and executed. The implementation will vary according to the Deployment Definition instance we have: Router or VpcRouter.

Although it looks like a huge change in the ACS cloud-server core, we kept most of the original code. Ou mains focus in this first step was to restructure it and make it better to understand. We have excessively tested our tested via Unit Tests, integration tests and also manually in order to have the 100% confidence to push the code towards the upstream branch.

Please, if you have doubts/suggestions/change requests, do not hesitate to contact us. Also feel free to improve the code we change in any aspect you think it's necessary, but do not forget to share with the community your reasons for doing so.

The Redundant VPC subject has been discussed in a few threads in the last months:

• Working on CloudStack Jira-764:nTier Apps 2.0 : Redundant Virtual Router for VPC email 2 of 2 http://markmail.org/message/56xrscvnmdweoxf5
• redundant virtual routers for VPCs: http://markmail.org/message/w4ow3ddcpxsic7g6
• Adding Redundant Routers to VPCs: http://markmail.org/message/hcay37lvfaev6wqw

Look to hear your feedback.

With kind regards,
Wilder and Antonio

[wilderrodrigues]

Hi Rohit,

Did as you suggested and got no conflicts detected.

[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 13:13.154s
[INFO] Finished at: Tue Sep 02 15:13:50 CEST 2014
[INFO] Final Memory: 83M/211M
[INFO] ------------------------------------------------------------------------

I will now push the branch to you guys.

Cheers,
Wilder

[lsimons]

Thanks Travis! So cool that this is working...the failure is due to the change here:

schubergphilis/cloudstack@680dd7e#diff-11

Wilder will update the pull request to filter it out.

[rohityadavcloud]

Thanks to @imduffy15 we have TravisCI fire up on every commit.

[wilderrodrigues]

Fixing it right now.

Sorry for the incovenience.

Cheers,
Wilder

[wilderrodrigues]

Hi Rohit,

I changed the db.properties last night and from the 3 builds 1 was successful (job 30.1). Concerning the other 2, jobs 30.2 and 30.3, I saw problems related to some Marvin tests. .

I had a look and the logs and identify the following:

30.2
=== TestName: test_privategw_acl | Status : EXCEPTION ===
test/integration/smoke/test_privategw_acl.py
=== TestName: None | Status : EXCEPTION ===
test/integration/smoke/test_reset_vm_on_reboot.py
=== TestName: None | Status : EXCEPTION ===
test/integration/smoke/test_routers.py

30.3
=== TestName: test_01_create_service_offering | Status : EXCEPTION ===
test/integration/smoke/test_service_offerings.py
=== TestName: None | Status : EXCEPTION ===
test/integration/smoke/test_templates.py
=== TestName: None | Status : EXCEPTION ===
test/integration/smoke/test_vm_life_cycle.py
=== TestName: None | Status : EXCEPTION ===
test/integration/smoke/test_volumes.py
=== TestName: test_vpc_remote_access_vpn | Status : EXCEPTION ===
test/integration/smoke/test_vpc_vpn.py
=== TestName: test_vpc_site2site_vpn | Status : EXCEPTION ===
test/integration/smoke/test_vpc_vpn.py

Checking other builds, I saw few of the same errors. Are those things related to "work in progress" status?

I will try to get them running on my environment, although I have a simpler setup than what Trevis does.

Back to you later.

Cheers,
Wilder

[wilderrodrigues]

Sorry.... mouse went over the wrong button my mistake and I closed the PR.

Cheers,
Wilder

[wilderrodrigues]

Closing this pull request because we got a bad state on our branch due to an unnecessary merge.
We had a clean branch, which is being used to rebase from the upstream and then we will cherry pick the last changes from the visitor-rebase branch.

Once it's all done, [unit]-tested and marvin tested, I will fire another PR.