Kelven,
(I apologize in advance for the length of my reply.)
I agree completely that we need to consider adopting a third-party dependency
injection framework. I propose that we give strong consideration to Spring
because of the following additional capabilities it provides:
Authentication and Authorization: Spring Security provides robust integration
with SSO facilities (CAS by default) to support enterprise authentication
scenarios (including LDAP and AD). It also provides a powerful authorization
capability to implement RBAC controls.
Configuration: Resolves configuration options from a hierarchy of sources and
injects their values through Spring DI. It also provides the means to bind
dependencies in code at runtime -- preventing a proliferation of annotations
across the codebase.
Transaction Management and JDBC Framework: A powerful, annotation based
transaction management facility with a nice set of abstractions for using JDBC
in a robust manner.
Scheduling: Spring provides a nice set of abstractions around various Java
scheduling mechanisms including Quartz.
Currently, CloudStack only authorizes users at the endpoint. Furthermore, the
authentication sessions are not shared in a manner that would permit
integration with other enterprise facilities (e.g. storage, LDAP, management
tools, etc). Shifting to Spring Security would permit fine-grained
authorization at the service boundary, and allow CloudStack to share
authentication session information in a trusted manner via third-party SSO
services. Finally, CloudStack would be able to defer LDAP/AD integration and
password hashing strategies to the SSO services while allowing more advanced
security features such as multi-factor authentication.
As discussed elsewhere on the list, the myriad of technologies integrated by
CloudStack introduces many opportunities for conflicting dependencies (i.e. JAR
hell), as well as, a requirement to isolate components. As such, CloudStack
seems like a good candidate for a component framework such as OSGi. While the
transition would require a lot of work (e.g. making the CloudStack artifacts
and third-party dependencies compliant, enhancing the build process, etc),
Spring was one of the original adopters of OSGi, and the dependency injection
framework works well with it.
I find the Spring XML configuration to be a horrendous blight on a codebase
followed closely by the proliferation of @Inject/@Component annotations
everywhere. In addition to tightly coupling the codebase to a dependency
injection framework (JSR-333 annotations do little to help matters, IMHO), they
require test frameworks on top of frameworks to perform unit testing. The most
valuable unit test suites are well-isolated and simple. The additional
overhead of these test frameworks adds unnecessary complexity to test case
authoring and debugging activities. Thankfully, Spring Configuration provides
the means to implement dependency injection without XML or annotations
everywhere. Borrowing the UsageManager example below, every subsystem (e.g.
usage) provides a single class annotated with the @Configuration annotation
such as the following:
com.cloud.usage;
@Configuration
public class UsageSubsystem {
@Component
private AccoutDao m_accountDao
// Inject other dependencies required by services exposed by this subsystem
// Inject configuration options required by services exposed by this subsystem
@Component
public UsageManager usageManager() {
return new UsageManagerImpl(m_accountDao, /* other dependencies and
configuration options */
}
// Method per service implementation binding
}
Since the dependency injection framework is isolated in this Subsystem class,
UsageManagerImpl is simplified to the following:
com.cloud.usage;
final class UsageManagerImpl implements UsageManager {
private final AccountDao m_accountDao;
// Other dependencies …
UsageManagerImpl(AccountDao accountDao, /* other dependencies and configuration
options */) {
super();
this.m_accountDao = accountDao;
}
// UsageManager implementation
}
In this model, unit tests do not need to concern themselves with DI framework.
They create mock implementations of each UsageManagerImpl dependency and create
an instance using the new operator. Also, Spring provides a well-defined
lifecycle for configuration classes, including startup and shutdown hook
methods when the Lifecycle interface is implemented, and dependency graphs
(e.g. Subsystem C depends on the A and B). Finally, the dependencies on the DI
framework are isolated to a handful "subsystem" classes.
Currently, CloudStack maintains its own infrastructure for transaction
management and JDBC infrastructure code. Spring provides a robust transaction
management mechanism exposed via annotations and JDBC templates that would us
to remove code under maintenance while adopting facilities that are well
understood and widely used across the industry.
Finally, there are number of places in the CloudStack code based with scheduled
tasks occur. Each capability is implemented independently (usually using a
ScheduledTaskExecutor) with no coordination. Furthermore, none of these
scheduled tasks a standard approach for crash recovery or retry. Adopting a
centralized scheduling framework would not only establish consistency for these
types of operations, but also allow CloudStack to leverage error recovery and
retry provided by those facilities.
I am sure I have left out a myriad of details. I believe that we can simplify
the codebase while increasing capability through a framework such as Spring. I
also believe that adopting mechanisms for these needs that are widely used
through our industry will make the codebase more approachable to potential
contributors while leaving the project to focus on its core cloud management
capabilities.
Thank you for sticking with my long stream of consciousness reply,
-John
On Nov 8, 2012, at 7:19 PM, Kelven Yang <kelven.y...@citrix.com> wrote:
> As part of architecture refactoring work, we are exploring some
> initiatives that would help CloudStack move toward more component-based
> architecture, one of such efforts is to standardize the way on how a
> single individual java component to be integrated into CloudStack system.
>
> CloudStack currently uses ComponentLocator to bootstrap components, it has
> done a great job while in the mean time, there are a couple of issues with
> this customized component injection framework (in my view).
>
> 1) ComponentLocator can not fully perform dependency injection, it can
> inject independent components without a problem, however, when things come
> with dependency, it will become tricky, although most of time auto-wiring
> with @Inject annotation works, but in some other time, developers will
> have to use following explicit wiring logic to make things work.
>
> @Local(value={UsageManager.class})
> public class UsageManagerImpl implements UsageManager, Runnable {
> public static final Logger s_logger =
> Logger.getLogger(UsageManagerImpl.class.getName());
>
> protected static final String DAILY = "DAILY";
> protected static final String WEEKLY = "WEEKLY";
> protected static final String MONTHLY = "MONTHLY";
>
> private static final int HOURLY_TIME = 60;
> private static final int DAILY_TIME = 60 * 24;
> private static final int THREE_DAYS_IN_MINUTES = 60 * 24 * 3;
> private static final int USAGE_AGGREGATION_RANGE_MIN = 10;
>
> --> hard-wiring logic goes here...
> private final ComponentLocator _locator =
> ComponentLocator.getLocator(UsageServer.Name, "usage-components.xml",
> "log4j-cloud_usage");
> private final AccountDao m_accountDao =
> _locator.getDao(AccountDao.class);
> private final UserStatisticsDao m_userStatsDao =
> _locator.getDao(UserStatisticsDao.class);
> private final UsageDao m_usageDao = _locator.getDao(UsageDao.class);
> private final UsageVMInstanceDao m_usageInstanceDao =
> _locator.getDao(UsageVMInstanceDao.class);
> private final UsageIPAddressDao m_usageIPAddressDao =
> _locator.getDao(UsageIPAddressDao.class);
> private final UsageNetworkDao m_usageNetworkDao =
> _locator.getDao(UsageNetworkDao.class);
> private final UsageVolumeDao m_usageVolumeDao =
> _locator.getDao(UsageVolumeDao.class);
> private final UsageStorageDao m_usageStorageDao =
> _locator.getDao(UsageStorageDao.class);
> private final UsageLoadBalancerPolicyDao m_usageLoadBalancerPolicyDao
> = _locator.getDao(UsageLoadBalancerPolicyDao.class);
> private final UsagePortForwardingRuleDao m_usagePortForwardingRuleDao
> = _locator.getDao(UsagePortForwardingRuleDao.class);
> private final UsageNetworkOfferingDao m_usageNetworkOfferingDao =
> _locator.getDao(UsageNetworkOfferingDao.class);
> private final UsageVPNUserDao m_usageVPNUserDao =
> _locator.getDao(UsageVPNUserDao.class);
> private final UsageSecurityGroupDao m_usageSecurityGroupDao =
> _locator.getDao(UsageSecurityGroupDao.class);
> private final UsageJobDao m_usageJobDao =
> _locator.getDao(UsageJobDao.class);
>
> The problem of doing so is that we create a cross-reference between
> component and the component container, it breaks the principle of being
> component-ized. In the above case, the component shouldn't really make any
> assumption of its container, and the other problem is that developers will
> sometimes be confused at when, under what situation, they can use @Inject
> to wire a component or they have to use run-time wiring logic like above
> to make it work.
>
>
> 2) AOP pattern is implicitly buried with current injection framework.
>
> AOP(Aspect Oriented Pattern) is very powerful concept, and CloudStack uses
> it a lot, @DB annotation is a very typical example. However, wiring an
> aspect is not that explicit, it buries inside those interceptors that work
> together with ComponentLocator during injection phase. Although AOP
> concept is powerful, but it better to be explicit, we need to encourage to
> component-ize the AOP aspect itself as component, although
> ComponentLocator has no problem to offer AOP support, it lacks of letting
> people do it in a more explicit way.
>
> 3) Unit test integration
> In the example in 1), to perform a unit test on the component, it involves
> with having to construct a mocked ComponentLocator, these little pieces
> add up to make unit-testing a little challenge in CloudStack.
>
> 4) CloudStack specific logic
>
> ComponentLocator also has CloudStack specific assumptions on Adapters,
> Managers, PluggableService etc, this has to move up to the business layer
> and leave this underlying layer generic and clean, if in the future people
> want to add some other cool stuff like CoolServiceGates etc, it requires
> the developer to modify ComponentLocator, this kind of possibility alone
> makes ComponentLocator itself not really a component, we should not
> encourage that.
>
> As a solution, we are exploring the possibility to adopt Spring Framework
> into CloudStack, it has the ability to address all above issues, the goal
> here is to leave all these generic concerns away from CloudStack core
> business logic, and standardize the way we program in CloudStack. The goal
> is
>
> 1) Individual component should be clean, no hard-coded assumption of any
> component container framework
> 2) System to in layered with clean boundary. No business logic leaked into
> generic plumbing layer.
> 3) AOP pattern to be explicit, represented through aspect component and
> explicit wiring of the AOP
> 4) Make unit test simple, simple and simple (Junit Spring integration has
> already done that)
>
> Since 4.0 is already out of the door, I re-raise the discussion here
> again, please feel free to share your opinions
>
> Kelven
>
>
> On 10/19/12 6:14 PM, "Kelven Yang" <kelven.y...@citrix.com> wrote:
>
>> David,
>>
>> Thanks for the remind. As we are working on a feature branch(Javelin) and
>> thought not to heat-up the mailing list while everyone is still busy on
>> getting the 4.0 release out of the door.
>>
>> We are evaluating the possibility to leverage Spring Framework to
>> standardize Dependency injection/AOP pattern usages in CloudStack. Spring
>> framework is very popular to Java community and under Apache license. By
>> switching the underlying architecture plumbing to a de-facto standardized
>> framework would help attract developers to focus more on CloudStack core
>> business logic with already familiar coding practices.
>>
>> We will not extensively use full featured Spring Framework, may primarily
>> on core DI/AOP features since these are commonly used in CloudStack, we
>> also haven't decided to go with Spring annotation based approach or XML
>> based approach yet. With currently check-ins, they are annotation based
>> which I personally prefer, however, this may change, depends on how to get
>> our old code loaded nicely under Spring.
>>
>> If anyone does have a strong opinion on this, please feel free to share
>> with us.
>>
>>
>> Kelven
>>
>> On 10/19/12 3:47 PM, "David Nalley" <da...@gnsa.us> wrote:
>>
>>> This adds a number of new dependencies.
>>>
>>> This needs to explicitly be discussed before it gets committed.
>>> See below for reference:
>>> http://markmail.org/message/huevw4ur73a64b5c
>>>
>>> On Fri, Oct 19, 2012 at 6:25 PM, <kelv...@apache.org> wrote:
>>>> Updated Branches:
>>>> refs/heads/javelin a75916d45 -> 8ef9e32cf
>>>>
>>>>
>>>> Architecture refactoring - Stateless management server - Spring
>>>> Framework initiatives
>>>>
>>>>
>>>> Project:
>>>> http://git-wip-us.apache.org/repos/asf/incubator-cloudstack/repo
>>>> Commit:
>>>> http://git-wip-us.apache.org/repos/asf/incubator-cloudstack/commit/8ef9e
>>>> 3
>>>> 2c
>>>> Tree:
>>>> http://git-wip-us.apache.org/repos/asf/incubator-cloudstack/tree/8ef9e32
>>>> c
>>>> Diff:
>>>> http://git-wip-us.apache.org/repos/asf/incubator-cloudstack/diff/8ef9e32
>>>> c
>>>>
>>>> Branch: refs/heads/javelin
>>>> Commit: 8ef9e32cfda4b1ce138ac167caa26521bd7a3508
>>>> Parents: a75916d
>>>> Author: Kelven Yang <kelven.y...@citrix.com>
>>>> Authored: Fri Oct 19 15:24:01 2012 -0700
>>>> Committer: Kelven Yang <kelven.y...@citrix.com>
>>>> Committed: Fri Oct 19 15:24:15 2012 -0700
>>>>
>>>> ----------------------------------------------------------------------
>>>> client/WEB-INF/web.xml | 9 ++-
>>>> pom.xml | 81
>>>> +++++++++++++++
>>>> utils/conf/db.properties | 1 +
>>>> utils/src/com/cloud/utils/events/EventBus.java | 4 +-
>>>> utils/src/com/cloud/utils/events/EventBusBase.java | 5 +-
>>>> utils/src/com/cloud/utils/events/Subscriber.java | 4 +-
>>>> 6 files changed, 99 insertions(+), 5 deletions(-)
>>>> ----------------------------------------------------------------------
>>>>
>>>>
>>>>
>>>> http://git-wip-us.apache.org/repos/asf/incubator-cloudstack/blob/8ef9e32
>>>> c
>>>> /client/WEB-INF/web.xml
>>>> ----------------------------------------------------------------------
>>>> diff --git a/client/WEB-INF/web.xml b/client/WEB-INF/web.xml
>>>> index 50f2455..c6fd30f 100644
>>>> --- a/client/WEB-INF/web.xml
>>>> +++ b/client/WEB-INF/web.xml
>>>> @@ -20,7 +20,14 @@
>>>> xsi:schemaLocation="http://java.sun.com/xml/ns/javaee
>>>> http://java.sun.com/xml/ns/javaee/web-app_2_5.xsd";
>>>> version="2.5">
>>>>
>>>> -
>>>> + <listener>
>>>> +
>>>> <listener-class>org.springframework.web.context.ContextLoaderListener</l
>>>> i
>>>> stener-class>
>>>> + </listener>
>>>> + <context-param>
>>>> + <param-name>contextConfigLocation</param-name>
>>>> + <param-value>classpath:applicationContext.xml</param-value>
>>>> + </context-param>
>>>> +
>>>> <servlet>
>>>> <servlet-name>cloudStartupServlet</servlet-name>
>>>>
>>>> <servlet-class>com.cloud.servlet.CloudStartupServlet</servlet-class>
>>>>
>>>>
>>>> http://git-wip-us.apache.org/repos/asf/incubator-cloudstack/blob/8ef9e32
>>>> c
>>>> /pom.xml
>>>> ----------------------------------------------------------------------
>>>> diff --git a/pom.xml b/pom.xml
>>>> index 3c14d4e..5f63ce6 100644
>>>> --- a/pom.xml
>>>> +++ b/pom.xml
>>>> @@ -79,6 +79,7 @@
>>>> <cs.servlet.version>2.4</cs.servlet.version>
>>>> <cs.jstl.version>1.2</cs.jstl.version>
>>>>
>>>> <cs.selenium.server.version>1.0-20081010.060147</cs.selenium.server.vers
>>>> i
>>>> on>
>>>> +
>>>> <org.springframework.version>3.0.5.RELEASE</org.springframework.version>
>>>> <skipTests>true</skipTests>
>>>>
>>>> </properties>
>>>> @@ -169,6 +170,86 @@
>>>> <version>${cs.junit.version}</version>
>>>> <scope>test</scope>
>>>> </dependency>
>>>> +
>>>> + <dependency>
>>>> + <groupId>org.springframework</groupId>
>>>> + <artifactId>spring-core</artifactId>
>>>> + <version>${org.springframework.version}</version>
>>>> + </dependency>
>>>> +
>>>> + <dependency>
>>>> + <groupId>org.springframework</groupId>
>>>> + <artifactId>spring-expression</artifactId>
>>>> + <version>${org.springframework.version}</version>
>>>> + </dependency>
>>>> +
>>>> + <dependency>
>>>> + <groupId>org.springframework</groupId>
>>>> + <artifactId>spring-beans</artifactId>
>>>> + <version>${org.springframework.version}</version>
>>>> + </dependency>
>>>> +
>>>> + <dependency>
>>>> + <groupId>org.springframework</groupId>
>>>> + <artifactId>spring-aop</artifactId>
>>>> + <version>${org.springframework.version}</version>
>>>> + </dependency>
>>>> +
>>>> + <dependency>
>>>> + <groupId>org.springframework</groupId>
>>>> + <artifactId>spring-context</artifactId>
>>>> + <version>${org.springframework.version}</version>
>>>> + </dependency>
>>>> +
>>>> + <dependency>
>>>> + <groupId>org.springframework</groupId>
>>>> + <artifactId>spring-context-support</artifactId>
>>>> + <version>${org.springframework.version}</version>
>>>> + </dependency>
>>>> +
>>>> + <dependency>
>>>> + <groupId>org.springframework</groupId>
>>>> + <artifactId>spring-tx</artifactId>
>>>> + <version>${org.springframework.version}</version>
>>>> + </dependency>
>>>> +
>>>> + <dependency>
>>>> + <groupId>org.springframework</groupId>
>>>> + <artifactId>spring-jdbc</artifactId>
>>>> + <version>${org.springframework.version}</version>
>>>> + </dependency>
>>>> +
>>>> + <dependency>
>>>> + <groupId>org.springframework</groupId>
>>>> + <artifactId>spring-orm</artifactId>
>>>> + <version>${org.springframework.version}</version>
>>>> + </dependency>
>>>> +
>>>> + <dependency>
>>>> + <groupId>org.springframework</groupId>
>>>> + <artifactId>spring-oxm</artifactId>
>>>> + <version>${org.springframework.version}</version>
>>>> + </dependency>
>>>> +
>>>> + <dependency>
>>>> + <groupId>org.springframework</groupId>
>>>> + <artifactId>spring-web</artifactId>
>>>> + <version>${org.springframework.version}</version>
>>>> + </dependency>
>>>> +
>>>> + <dependency>
>>>> + <groupId>org.springframework</groupId>
>>>> + <artifactId>spring-webmvc</artifactId>
>>>> + <version>${org.springframework.version}</version>
>>>> + </dependency>
>>>> +
>>>> + <dependency>
>>>> + <groupId>org.springframework</groupId>
>>>> + <artifactId>spring-test</artifactId>
>>>> + <version>${org.springframework.version}</version>
>>>> + <scope>test</scope>
>>>> + </dependency>
>>>> +
>>>> </dependencies>
>>>>
>>>> <build>
>>>>
>>>>
>>>> http://git-wip-us.apache.org/repos/asf/incubator-cloudstack/blob/8ef9e32
>>>> c
>>>> /utils/conf/db.properties
>>>> ----------------------------------------------------------------------
>>>> diff --git a/utils/conf/db.properties b/utils/conf/db.properties
>>>> index 6bdb6d6..8d98119 100644
>>>> --- a/utils/conf/db.properties
>>>> +++ b/utils/conf/db.properties
>>>> @@ -24,6 +24,7 @@ cluster.servlet.port=9090
>>>> # CloudStack database settings
>>>> db.cloud.username=cloud
>>>> db.cloud.password=cloud
>>>> +db.root.password=
>>>> db.cloud.host=localhost
>>>> db.cloud.port=3306
>>>> db.cloud.name=cloud
>>>>
>>>>
>>>> http://git-wip-us.apache.org/repos/asf/incubator-cloudstack/blob/8ef9e32
>>>> c
>>>> /utils/src/com/cloud/utils/events/EventBus.java
>>>> ----------------------------------------------------------------------
>>>> diff --git a/utils/src/com/cloud/utils/events/EventBus.java
>>>> b/utils/src/com/cloud/utils/events/EventBus.java
>>>> index 4195acd..c1b6f70 100644
>>>> --- a/utils/src/com/cloud/utils/events/EventBus.java
>>>> +++ b/utils/src/com/cloud/utils/events/EventBus.java
>>>> @@ -17,9 +17,11 @@
>>>>
>>>> package com.cloud.utils.events;
>>>>
>>>> +import java.io.Serializable;
>>>> +
>>>> public interface EventBus {
>>>> void subscribe(String subject, Subscriber subscriber);
>>>> void unsubscribe(String subject, Subscriber subscriber);
>>>>
>>>> - void publish(String subject, PublishScope scope, Object sender,
>>>> String args);
>>>> + void publish(String subject, PublishScope scope, Object sender,
>>>> Serializable args);
>>>> }
>>>>
>>>>
>>>> http://git-wip-us.apache.org/repos/asf/incubator-cloudstack/blob/8ef9e32
>>>> c
>>>> /utils/src/com/cloud/utils/events/EventBusBase.java
>>>> ----------------------------------------------------------------------
>>>> diff --git a/utils/src/com/cloud/utils/events/EventBusBase.java
>>>> b/utils/src/com/cloud/utils/events/EventBusBase.java
>>>> index 0c135db..cd10f1d 100644
>>>> --- a/utils/src/com/cloud/utils/events/EventBusBase.java
>>>> +++ b/utils/src/com/cloud/utils/events/EventBusBase.java
>>>> @@ -17,6 +17,7 @@
>>>>
>>>> package com.cloud.utils.events;
>>>>
>>>> +import java.io.Serializable;
>>>> import java.util.ArrayList;
>>>> import java.util.HashMap;
>>>> import java.util.List;
>>>> @@ -72,7 +73,7 @@ public class EventBusBase implements EventBus {
>>>>
>>>> @Override
>>>> public void publish(String subject, PublishScope scope, Object
>>>> sender,
>>>> - String args) {
>>>> + Serializable args) {
>>>>
>>>> if(_gate.enter(true)) {
>>>>
>>>> @@ -283,7 +284,7 @@ public class EventBusBase implements EventBus {
>>>> _children.put(key, childNode);
>>>> }
>>>>
>>>> - public void notifySubscribers(String subject, Object
>>>> sender, String args) {
>>>> + public void notifySubscribers(String subject, Object
>>>> sender, Serializable args) {
>>>> for(Subscriber subscriber : _subscribers) {
>>>> subscriber.onPublishEvent(subject,
>>>> sender, args);
>>>> }
>>>>
>>>>
>>>> http://git-wip-us.apache.org/repos/asf/incubator-cloudstack/blob/8ef9e32
>>>> c
>>>> /utils/src/com/cloud/utils/events/Subscriber.java
>>>> ----------------------------------------------------------------------
>>>> diff --git a/utils/src/com/cloud/utils/events/Subscriber.java
>>>> b/utils/src/com/cloud/utils/events/Subscriber.java
>>>> index 7af283b..c3baa6f 100644
>>>> --- a/utils/src/com/cloud/utils/events/Subscriber.java
>>>> +++ b/utils/src/com/cloud/utils/events/Subscriber.java
>>>> @@ -17,6 +17,8 @@
>>>>
>>>> package com.cloud.utils.events;
>>>>
>>>> +import java.io.Serializable;
>>>> +
>>>> public interface Subscriber {
>>>> - void onPublishEvent(String subject, Object sender, String
>>>> args);
>>>> + void onPublishEvent(String subject, Object sender, Serializable
>>>> args);
>>>> }
>>>>
>>
>
