This Confluence has been LDAP enabled, if you are an ASF Committer, please use your LDAP Credentials to login. Any problems file an INFRA jira ticket please.

Child pages
  • Refactoring Redundant Virtual Router Implementation
Skip to end of metadata
Go to start of metadata

Motivation

In order to start the implementation of Redundant Virtual Routers for VPC - Virtual Private Cloud - we have decided to refactor the main pillars under the Virtual Network Appliance Manager and its VPC variant. Both classes together had more than ~7 thousand lines. Although number of lines doesn't necessarily mean bad quality, the way the code was structured would make understanding, maintenance and extensions slightly difficult.

Since we would work on the Redundant VPC, we took the opportunity to refactor the code, adding some design pattern implementations and more unit tests. So far, our work has been applied to the following classes/packages:

  • 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
    • About 1700 lines of unit tests

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]VirtualNetworkApplianceManager.

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 used 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 sendCommandsToRouter() method. The latter has also been extracted from the VirtualNetworkApplianceManagerImpl and is now in a new helper class: NetworkHelperImpl.

Why the Visitor Pattern?

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; and 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 - either Basic or Advanced.



Helper Classes

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

Router Deployment Definitions

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 logic 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 implementations, 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 preconditions 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.

Conclusion

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

Test Environment

  • CloudStack Simulator
    • Basic and Advanced Zones deployed
      • Advanced Zone: All 69 smoke tests executed
      • Basic Zone: Test Accounts; Test Affinity Groups; Test Service Offerings; Test ISO; Test Volumes; Test VM Life Cycle; Test Reset VM On Reboot
  • Xen Server 6.2
    • Basic and Advanced Zones deployed
      • Advanced Zone: Test VPC VPN; Test VPC Routers; Test Volumes; Test Service Offerings; Test Routers; Test Reset VM On Reboot; Test Private GW ACL; Test Accounts; Test VM Life Cycle
      • Basic Zone: Test Accounts; Test Affinity Groups; Test Service Offerings; Test VM Life Cycle; Test Reset VM On Reboot
  • Travis-CI

For the Xen Server 6.2 environment, we have reused and adapted few of the Marvin tests in order to comply with our IP ranges. That's why we do not have all 69 smoke tests in place yet.

Pull Request

The changes have been submitted to Apache CloudStack repository under the following pull request:

  • No labels