Github user wilderrodrigues commented on the pull request:
https://github.com/apache/cloudstack/pull/22#issuecomment-58073492
Hi @bhaisaab,
This is a new Pull Request in order to fix the conflicts we found in the
old pull request #19 .
Travis-CI is 100% green and this PR has been extensively tested.
I will put the most update version of the description here:
Pull request of changes in the "cloud-server" module
In the last 14 weeks we have worked 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 executed many Marvin tests that we got from ACS and made compliant
with our domain:
o test_01_create_account
o test_01_add_vm_to_subdomain
o test_DeleteDomain
o test_forceDeleteDomain
o test_updateAdminDetails
o test_updateDomainAdminDetails
o test_updateUserDetails
o test_LoginApiDomain
o test_LoginApiUuidResponse
o test_privategw_acl
o test_01_reset_vm_on_reboot
o test_03_restart_network_cleanup
o test_05_router_basic
o test_06_router_advanced
o test_07_stop_router
o test_08_start_router
o test_09_reboot_router
o test_01_create_service_offering
o test_02_edit_service_offering
o test_03_delete_service_offering
o test_01_start_stop_router_after_addition_of_one_guest_network
o test_02_reboot_router_after_addition_of_one_guest_network
o test_04_chg_srv_off_router_after_addition_of_one_guest_network
o test_05_destroy_router_after_addition_of_one_guest_network
o test_01_stop_start_router_after_creating_vpc
o test_02_reboot_router_after_creating_vpc
o test_04_change_service_offerring_vpc
o test_05_destroy_router_after_creating_vpc
o test_vpc_remote_access_vpn
o test_vpc_site2site_vpn
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 VpcR
outer.
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 Rodrigues
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
