brooklyn-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Martin Harris <martin.har...@cloudsoftcorp.com>
Subject Re: Refactoring JcloudsLocation
Date Wed, 10 Jun 2015 02:20:17 GMT
+1
On 10 Jun 2015 05:10, "Aled Sage" <aled.sage@gmail.com> wrote:

> Hi all,
>
> I'd like to do a major refactor of our class
> *brooklyn.location.jclouds.JcloudsLocation*. However, I suggest we try to
> get a 0.7.0 release soon, and then do this refactoring.
>
> _*Current issues*_
> The class has grown very big (2642 lines, including comments and white
> space).
>
> The class has many responsibilities, which include:
>
>  * VM creation
>  * jclouds Template / TemplateOptions creation, for tailoring what VM
>    to create
>  * Retries and concurrency control, for VM creation
>  * Post-processing of VM (e.g. extracting most appropriate ip/port)
>  * VM customisation - for user creation
>  * VM customisation - other, e.g. generating/setting hostname, open
>    iptables, populating ~/.ssh/authorizedKeys, etc.
>  * JcloudsMachineLocation construction (to represent the created VM)
>
> The class is hard to customise. There is a
> brooklyn.location.jclouds.JcloudsLocationCustomizer, which provides hooks
> for customising the provisioning process - it has six callbacks, i.e. six
> interception points where the behaviour can be customised. These cover many
> use-cases, but there are other use-cases that require intercepting at other
> points.
>
> Sub-classing is poorly documented and potentially brittle. For example, in
> advanced-networking [1], it sub-classes JcloudsLocation to intercept
> behaviour at appropriate points to set up private networks /
> port-forwarding. We don't want to permanently support every protected
> method as part of a "public" API. We want a more thought-out approach to
> intercepting / augmenting behaviour. We want a clear statement about the
> backwards compatibility guarantees we'll give for sub-classing.
>
> There is quite complicated if-else behaviour. That would be very hard to
> remove entirely (e.g. user creation + credentials is controlled by several
> config keys, which thus requires if-else). However it could likely be
> simplified. For example, windows versus linux differences could be
> extracted into separate classes - the decision is made once, and then the
> behaviour is different in each class.
>
> _*Proposal*_
> We should separate out responsibilities into separate classes. We should
> use patterns, such as the "strategy", "command" and "factory" patterns, to
> inject behaviour.
>
> The injection should be customisable, so that power-users can customise
> the behaviour (e.g. by replacing a given strategy class).
>
> Of the "responsibilities" listed above, some of those fit nicely into the
> refactoring (e.g. extract the "user creation" code into its own class).
> Others map nicely to a cleaner separation of methods (e.g. concurrency
> control not embedded in the middle of the method).
> Others fit into a general "post-provisioning customisation" phase, which
> could be its own class.
>
> _*Next steps*_
> Let's get 0.7.0 GA out of the door first.
>
> I'm concious that the proposal above is vague, and a cynic could play
> buzz-word bingo with pattern names.
>
> However, if there's general agreement that this refactoring is worth
> while, then let's do some incremental improvements (e.g. starting with
> extracting into separate classes), and do a detailed review in the pull
> requests.
>
> Aled
>
> [1] https://github.com/brooklyncentral/advanced-networking
>
>
>

-- 
Cloudsoft Corporation Limited, Registered in Scotland No: SC349230. 
 Registered Office: 13 Dryden Place, Edinburgh, EH9 1RP
 
This e-mail message is confidential and for use by the addressee only. If 
the message is received by anyone other than the addressee, please return 
the message to the sender by replying to it and then delete the message 
from your computer. Internet e-mails are not necessarily secure. Cloudsoft 
Corporation Limited does not accept responsibility for changes made to this 
message after it was sent.

Whilst all reasonable care has been taken to avoid the transmission of 
viruses, it is the responsibility of the recipient to ensure that the 
onward transmission, opening or use of this message and any attachments 
will not adversely affect its systems or data. No responsibility is 
accepted by Cloudsoft Corporation Limited in this regard and the recipient 
should carry out such virus and other checks as it considers appropriate.

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message