You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by Rohit Yadav <ro...@shapeblue.com> on 2015/07/10 21:22:52 UTC
Re: [DISCUSS] Moving to Java 8
Ping Wilder - any progress/plan on moving forward (perhaps after 4.6?).
On 01-May-2015, at 4:07 pm, Wilder Rodrigues <WR...@schubergphilis.com>> wrote:
Hi Marcus,
Sorry for the push, I think after doing the whole CitrixResourceBase refactor I also got a bit attached to the whole thing/solution. ;)
Thanks for the input you gave. I will finish the refactor and apply it to both implementations.
Cheers,
Wilder
On 01 May 2015, at 09:06, Marcus <sh...@gmail.com>> wrote:
Oh, and of course the annotation added to the wrapper looks like:
...
@ResourceWrapper(handles = CheckHealthCommand.class)
public final class LibvirtCheckHealthCommandWrapper
...
maybe 'wraps' or 'wrapperfor' would be better than 'handles' in your naming scheme. You get the idea.
On Thu, Apr 30, 2015 at 11:59 PM, Marcus <sh...@gmail.com>> wrote:
I agree, this wrapper is a good step forward. It's totally fine to continue on that path because it is obviously better and makes it easy to switch to autodetection anytime later by simply adding the annotation. Sorry if I got a bit passionate about that, but as you mention I also get tired of adding things in multiple places, and the annotations have worked well in the API and provide a good model to emulate for consistency.
I can't share code, because these extensions to LibvirtComputingResource that I've provided for other companies have not been open sourced. I can speak more generically though about methods.
To answer question "a", reflection allows you to do something like:
Reflections reflections = new Reflections("com.cloud.hypervisor.kvm.resource.wrapper");
Set<Class<? extends CommandWrapper>> wrappers = reflections.getSubTypesOf(CommandWrapper.class);
So here in "new Reflections" we are automatically filtering for just the wrappers that would apply to the KVM plugin.
Then to finish it off, you iterate through the wrappers and do:
ResourceWrapper annotation = wrapper.getAnnotation(ResourceWrapper.class);
citrixCommands.put(annotation.handles(), wrapper.newInstance());
Sorry, I guess that's four lines, plus the relevant for loop. And probably a null check or something for the annotation. You also have to add the annotation class itself, and add a line for the annotation in each wrapper, but in the end when we add new Commands, we won't have to touch anything but the new class that handles the command.
public @interface ResourceWrapper {
Class<? extends Command> handles();
}
There's an example of something similar to this in KVMStoragePoolManager.java (annotation is StoragePoolInfo.java). This example has actually been adapted from that. Also to a lesser extent in the API server, but it is spread across a bunch of classes.
On Thu, Apr 30, 2015 at 10:41 PM, Wilder Rodrigues <WR...@schubergphilis.com>> wrote:
Hi Marcus,
Thanks for the email… I’m always in for improvements. But why can’t you share the code?
Few points below:
1. I added an subclassing example of LibvirtComputingResource because you mentioned it in a previous email:
On 23 Apr 2015, at 17:26, Marcus <sh...@gmail.com>> wrote:
I mentioned the reflection model because that's how I tend to handle
the commands when subclassing LibvirtComputingResource.
2. Current situation with LibvirtComputingResource on Master is:
a. 67 IFs
b. 67 private/protected methods that are used only there
c. If a new Command is added it means we will have a new IF and a new private method
e. Maintenance is hell, test is close to zero and code quality is below expectations
That being said, the main idea with the refactor is to change structure only, not behaviour. So what I’m doing is to simply move the code out the LibvirtCompRes and write tests for it, keeping the behaviour the same - to be done in a next phase.
If you look at the changes you will see that some wrappers are already 100% covered. However, some others have 4% or 8% (not that much though). I would like to refactor that as well, but that could change behaviour (mentioned above) which I don’t want to touch now.
3. With the new situation:
a. No IFs
b. All methods wrapped by other classes (command wrappers) - loosely coupled, easier to test and maintain
c. If a new Command is added we would have to add a command wrapper and 1 line in the request wrapper implementation ( I know, it hurts you a bit) - but please bear with me for the good news.
4. the warnings are due to that:
Hashtable<Class<? extends Command>, CommandWrapper>()
No big deal.
As I understood from your first paragraph we would have to annotated the commands classes, right? I mean, all of them.
That’s something I wouldn’t do in this phase, to be honest. It might seem harmless to do, but I like to break things down a bit and have more isolation in my changes.
What’s next: I will finish the refactor with the request wrapper as it is. For me it is no problem do add the lines now and remove them in 1 week. Most of the work is concentrated in the tests, which I’m trying as hard as I can to get them in the best way possible. Once it’s done and pushed to master, I will analyse what we would need to apply the annotation.
But before I go to bring the kids to school, just one question:
a. The “handle” value, in the annotation, would have the wrapper class that would be used for that command, right? Now let’s get 1 command as example: CheckHealthCommand. Its wrapper implementation differs per hypervisor (just like all the other wrapper commands do). I’m not taking the time to really think about it now, but how would we annotated the different wrappers per command?
Thanks again for your time.
Cheers,
Wilder
On 30 Apr 2015, at 22:52, Marcus <sh...@gmail.com>> wrote:
Ok. I wish I could share some code, because it isn't really as big of
a deal as it sounds from your reasoning. It is literally just 3 lines
on startup that fetch anything with the '@AgentExecutor' annotation
and stores it in a hash whose key is the value from @AgentExecutor's
'handles' property. Then when a *Command comes it it is passed to the
appropriate Executor class.
Looking at CitrixRequestWrapper, the 3 lines I mention are almost
identical in function to your init method, just that it uses the
annotation to find all of the commands, rather than hardcoding them.
We use the same annotation design for the api side of the code on the
management server, which allows the api commands to be easier to write
and self-contained (you don't have to update other code to add a new
api call). It makes things easier for novice developers.
This implementation is no less typesafe than the previous design (the
one with all of the instanceof). It didn't require any casting or
warning suppression, either, as the wrapper does.
Extending LibvirtComputingResource is not ideal, and doesn't work if
multiple third parties are involved. Granted, there hasn't been a lot
of demand for this, nevertheless it's particularly important for KVM,
where the Command classes are executed on the hypervisor it's not
really feasible to just dump the code in your management server-side
plugin like some plugins do.
In reviewing the code, the two implementations are really very close.
If you just updated init to fetch the wrappers based on either an
annotation or the class they extend, or something along those lines so
this method doesn't have to be edited every time a command is added,
that would be more or less the same thing. The the KVM agent would be
pluggable like the management server side is.
On Thu, Apr 30, 2015 at 12:55 PM, Wilder Rodrigues
<WR...@schubergphilis.com>> wrote:
Hi Marcus,
Apologies for taking so much time to reply to your email, but was, and still
am, quite busy. :)
I would only use reflection if that was the only way to do it. The use of
reflection usually makes the code more complex, which is not good when we
have java developers in all different levels (from jr. do sr) working with
cloudstack. It also makes us lose the type safety, which might also harm the
exception handling if not done well. In addition, if we need to refactor
something, the IDE is no longer going to do few things because the refection
code cannot be found.
If someone will need to extend the LibvirtComputingResource that would be no
problem with the approach I’m using. The CitrixResourceBase also has quite
few sub-classes and it works just fine.
I will document on the wiki page how it should be done when sub-classing the
LibvirtComputingResource class.
In a quick note/snippet, one would do:
public class EkhoComputingResource extends LibvirtComputingResource {
@Override
public Answer executeRequest(final Command cmd) {
final LibvirtRequestWrapper wrapper =
LibvirtRequestWrapper.getInstance();
try {
return wrapper.execute(cmd, this);
} catch (final Exception e) {
return Answer.createUnsupportedCommandAnswer(cmd);
}
}
}
In the flyweight where I keep the wrapper we could have ():
final Hashtable<Class<? extends Command>, CommandWrapper>
linbvirtCommands = new Hashtable<Class<? extends Command>,
CommandWrapper>();
linbvirtCommands.put(StopCommand.class, new
LibvirtStopCommandWrapper());
final Hashtable<Class<? extends Command>, CommandWrapper>
ekhoCommands = new Hashtable<Class<? extends Command>, CommandWrapper>();
linbvirtCommands.put(StopCommand.class, new
EkhoStopCommandWrapper());
resources.put(LibvirtComputingResource.class, linbvirtCommands);
resources.put(EkhoComputingResource.class, ekhoCommands);
But that is needed only if the StopCommand has a different behaviour for the
EkhoComputingResource.
Once a better version of the documentation is on the wiki, I will let you
know.
On other matters, I’m also adding unit tests for all the changes. We already
went from 4% to 13.6% coverage in the KVM hypervisor plugin. The code I
already refactored has 56% of coverage.
You can see all the commits here:
https://github.com/schubergphilis/cloudstack/tree/refactor/libvirt_resource
Cheers,
Wilder
On 23 Apr 2015, at 17:26, Marcus <sh...@gmail.com>> wrote:
Great to see someone working on it. What sorts of roadblocks came out
of reflection? How does the wrapper design solve the pluggability
issue? This is pretty important to me, since I've worked with several
companies now that end up subclassing LibvirtComputingResource in
order to handle their own Commands on the hypervisor from their
server-side plugins, and changing their 'resource' to that in
agent.properties. Since the main agent class needs to be set at agent
join, this is harder to manage than it should be.
I mentioned the reflection model because that's how I tend to handle
the commands when subclassing LibvirtComputingResource. I haven't had
any problems with it, but then again I haven't tried to refactor 5500
lines into that model, either.
On Thu, Apr 23, 2015 at 1:17 AM, Wilder Rodrigues
<WR...@schubergphilis.com>> wrote:
Hi Marcus,
I like the annotation idea, but reflection is trick because it hides some
information about the code.
Please, have a look at the CitrixResourceBase after the refactor I did. It
became quite smaller and test coverage was improved.
URL:
https://cwiki.apache.org/confluence/display/CLOUDSTACK/Refactoring+XenServer+Hypervisor+Plugin
The same patter is being about to Libvirt stuff. The coverage on the KVM
hypervisor plugin already went from 4 to 10.5% after refactoring 6 commands
Cheers,
Wilder
On 22 Apr 2015, at 23:06, Marcus <sh...@gmail.com>> wrote:
Kind of a tangent, but I'd actually like to see some work done to
clean up LibvirtComputing resource. One model I've prototyped that
seems to work is to create an annotation, such as
'KVMCommandExecutor', with a 'handles' property. With this annotation,
you implement a class that handles, e.g. StartCommand, etc. Then in
LibvirtComputingResource, the 'configure' method fetches all of these
executors via reflection and stores them in an object. Then, instead
of having all of the 'instanceof' lines in LibvirtComputingResource,
the executeRequest method fetches the executor that handles the
incoming command and runs it.
I think this would break up LibvirtComputingResource into smaller,
more testable and manageable chunks, and force things like config and
utility methods to move to a more sane location, as well. As a bonus,
this model makes things pluggable. Someone could ship KVM plugin code
containing standalone command executors that are discovered at runtime
for things they need to run at the hypervisor level.
On Tue, Apr 21, 2015 at 6:27 AM, Wilder Rodrigues
<WR...@schubergphilis.com>> wrote:
Hi all,
Yesterday I started working on the LibvirtComputingResource class in order
to apply the same patterns I used in the CitrixResourceBase + add more unit
tests to it After 10 hours of work I got a bit stuck with the 1st test,
which would cover the refactored LibvirtStopCommandWrapper. Why did I get
stuck? The class used a few static methods that call native libraries, which
I would like to mock. However, when writing the tests I faced problems with
the current Mockito/PowerMock we are using: they are simply not enough for
the task.
What did I do then? I added a dependency to EasyMock and PowerMock-EasyMock
API. It worked almost fine, but I had to add a “-noverify” to both my
Eclipse Runtime configuration and also to the
cloud-plugin-hypervisor-kvm/pom.xml file. I agree that’s not nice, but was
my first attempt of getting it to work. After trying to first full build I
faced more problems related to ClassDefNotFoundExpcetion which were
complaining about Mockito classes. I then found out that adding the
PowerMockRunner to all the tests classes was going to be a heavy burden and
would also mess up future changes (e.g. the -noverify flag was removed from
Java 8, thus adding it now would be a problem soon).
Now that the first 2 paragraphs explain a bit about the problem, let’s get
to the solution: Java 8
The VerifyError that I was getting was due to the use of the latest EasyMock
release (3.3.1). I tried to downgrade it to 3.1/3.2 but it also did not
work. My decision: do not refactor if the proper tests cannot be added. This
left me with one action: migrate to Java 8.
There were mentions about Java 8 in february[1] and now I will put some
energy in making it happen.
What is your opinion on it?
Thanks in advance.
Cheers,
Wilder
http://mail-archives.apache.org/mod_mbox/cloudstack-dev/201502.mbox/%3C54EEF6BE.5040401@shapeblue.com%3E<ht...@shapeblue.com>>>
Regards,
Rohit Yadav
Software Architect, ShapeBlue
[cid:9DD97B41-04C5-45F0-92A7-951F3E962F7A]
M. +91 88 262 30892 | rohit.yadav@shapeblue.com<ma...@shapeblue.com>
Blog: bhaisaab.org<http://bhaisaab.org> | Twitter: @_bhaisaab
Find out more about ShapeBlue and our range of CloudStack related services
IaaS Cloud Design & Build<http://shapeblue.com/iaas-cloud-design-and-build//>
CSForge – rapid IaaS deployment framework<http://shapeblue.com/csforge/>
CloudStack Consulting<http://shapeblue.com/cloudstack-consultancy/>
CloudStack Software Engineering<http://shapeblue.com/cloudstack-software-engineering/>
CloudStack Infrastructure Support<http://shapeblue.com/cloudstack-infrastructure-support/>
CloudStack Bootcamp Training Courses<http://shapeblue.com/cloudstack-training/>
This email and any attachments to it may be confidential and are intended solely for the use of the individual to whom it is addressed. Any views or opinions expressed are solely those of the author and do not necessarily represent those of Shape Blue Ltd or related companies. If you are not the intended recipient of this email, you must neither take any action based upon its contents, nor copy or show it to anyone. Please contact the sender if you believe you have received this email in error. Shape Blue Ltd is a company incorporated in England & Wales. ShapeBlue Services India LLP is a company incorporated in India and is operated under license from Shape Blue Ltd. Shape Blue Brasil Consultoria Ltda is a company incorporated in Brasil and is operated under license from Shape Blue Ltd. ShapeBlue SA Pty Ltd is a company registered by The Republic of South Africa and is traded under license from Shape Blue Ltd. ShapeBlue is a registered trademark.
Re: [DISCUSS] Moving to Java 8
Posted by Wido den Hollander <wi...@widodh.nl>.
On 16-07-15 17:58, John Burwell wrote:
> Wido,
>
> We have an acute problem — Oracle will be issuing no further security patches for Java7 which is a significant opsec risk. Put simply, we can’t leave our users exposed to such a risk because Ubuntu won’t ship a release for an non-EOL’ed Java version for another year. Personally, I think it is a tremendous oversight that Java7 has been EOL’ed for over three (3) months [1], and we have not addressed the issue for our users. CloudStack should have officially supported Java8 before Java7 reached EOL.
>
True, true. I'm not against going to Java 8, I just don't want to
abandon the Ubuntu users.
The PPA however is a good way to go, that could go into the Release
Notes and Upgrade docs.
> To minimize the impact of the change and smooth the transition, I suggest continuing to build with source and runtime compatibility set to 1.7 and test builds on both Java7 and Java8 until CloudStack 5. This approach will allow users to gradually transition from Java7 to Java8 with support for Java7 being eliminated at the next major release.
>
Yes, that would be best. But don't we have a Java 8 Jenkins slave already?
Wido
> Thanks,
> -John
>
> [1]: http://www.oracle.com/technetwork/java/eol-135779.html
>
> ---
> John Burwell (@john_burwell)
> VP of Software Engineering, ShapeBlue
> (571) 403-2411 | +44 20 3603 0542
> http://www.shapeblue.com
> Please join us at CloudStack Collab EU — http://events.linuxfoundation.org/events/cloudstack-collaboration-conference-europe
>
>
>
>
>> On Jul 15, 2015, at 7:57 AM, Wido den Hollander <wi...@widodh.nl> wrote:
>>
>> Signed PGP part
>>
>>
>> On 15-07-15 09:20, Wilder Rodrigues wrote:
>>> If there would be dependencies on some other things, that in no way
>>> could be fixed now, we could wait for 4.7 (5.0). However, if we
>>> could give it a go, I would be able to tackle this in our next
>>> Sprint (within 1 1/2 week from now) and still get it into 4.6.
>>>
>>> What would be the main considerations?
>>>
>>
>> Well, on Ubuntu systems we simply we will require a additional
>> software repo. The good thing is that OpenJDK 8 is available for 12.04
>> and 14.04
>>
>> I don't feel like we should do something like this in a 4.X release,
>> but that's me.
>>
>> Wido
>>
>>> Cheers, Wilder
>>>
>>>
>>> On 14 Jul 2015, at 22:25, Wido den Hollander
>>> <wi...@widodh.nl>> wrote:
>>>
>>>
>>>
>>> On 07/14/2015 10:18 PM, John Burwell wrote: Wido,
>>>
>>> Is the OpenJDK PPA [1] not acceptable? Since Java7 is no longer
>>> supported, we run the risk of an Java security issue affecting the
>>> project that won’t be fixed.
>>>
>>>
>>> I didn't know that a PPA with OpenJDk existed. Looking at it I see
>>> that the maintainer Matthias Klose works for Canonical, so it
>>> seems like an official PPA.
>>>
>>> Still, having users adding PPAs is something we want to prevent as
>>> much as possible, but I do recognize the problem here.
>>>
>>> Ubuntu 16.04 is less then a year away and will have Java 8 support,
>>> so that will be resolved by then, but for now it is a problem.
>>>
>>> I think that CloudStack 4.6 is to early, but maybe 4.7 (called
>>> 5.0?) is a good time to make the move?
>>>
>>> Wido
>>>
>>> Thanks, -John
>>>
>>> [1]: https://launchpad.net/~openjdk-r/+archive/ubuntu/ppa
>>>
>>> --- John Burwell (@john_burwell) VP of Software Engineering,
>>> ShapeBlue (571) 403-2411 | +44 20 3603 0542
>>> http://www.shapeblue.com Please join us at CloudStack Collab EU —
>>> http://events.linuxfoundation.org/events/cloudstack-collaboration-conf
>>>
>>>
>> erence-europe
>>>
>>>
>>>
>>>
>>>
>>> On Jul 10, 2015, at 5:47 PM, Wido den Hollander
>>> <wi...@widodh.nl>> wrote:
>>>
>>> Signed PGP part
>>>
>>>
>>> On 07/10/2015 09:22 PM, Rohit Yadav wrote: Ping Wilder - any
>>> progress/plan on moving forward (perhaps after 4.6?).
>>>
>>>
>>> I don't think there is? Since Ubuntu 14.04 doesn't support Java 8
>>> in any package form we can't really continue.
>>>
>>> Ubuntu 16.04 will ship with Java 8 and that will be the next LTS.
>>>
>>> Wido
>>>
>>> On 01-May-2015, at 4:07 pm, Wilder Rodrigues
>>> <WR...@schubergphilis.com><m
>> ailto:WRodrigues@schubergphilis.com>
>>>
>>>
>>>
>>>
>>> wrote:
>>>
>>> Hi Marcus,
>>>
>>> Sorry for the push, I think after doing the whole
>>> CitrixResourceBase refactor I also got a bit attached to the whole
>>> thing/solution. ;)
>>>
>>> Thanks for the input you gave. I will finish the refactor and apply
>>> it to both implementations.
>>>
>>> Cheers, Wilder
>>>
>>>
>>> On 01 May 2015, at 09:06, Marcus
>>> <sh...@gmail.com><mailto:shadowsor@gmai
>> l.com><mailto:shadowsor@gm
>>>
>>>
>> ai
>>>
>>>
>>> l.com<http://l.com>>> wrote:
>>>
>>> Oh, and of course the annotation added to the wrapper looks like:
>>>
>>>
>>> ...
>>>
>>> @ResourceWrapper(handles = CheckHealthCommand.class)
>>>
>>> public final class LibvirtCheckHealthCommandWrapper
>>>
>>> ...
>>>
>>>
>>> maybe 'wraps' or 'wrapperfor' would be better than 'handles' in
>>> your naming scheme. You get the idea.
>>>
>>> On Thu, Apr 30, 2015 at 11:59 PM, Marcus
>>> <sh...@gmail.com><mailto:shadowsor@gmai
>> l.com>>
>>> wrote: I agree, this wrapper is a good step forward. It's totally
>>> fine to continue on that path because it is obviously better and
>>> makes it easy to switch to autodetection anytime later by simply
>>> adding the annotation. Sorry if I got a bit passionate about that,
>>> but as you mention I also get tired of adding things in multiple
>>> places, and the annotations have worked well in the API and provide
>>> a good model to emulate for consistency.
>>>
>>> I can't share code, because these extensions to
>>> LibvirtComputingResource that I've provided for other companies
>>> have not been open sourced. I can speak more generically though
>>> about methods.
>>>
>>> To answer question "a", reflection allows you to do something
>>> like:
>>>
>>> Reflections reflections = new
>>> Reflections("com.cloud.hypervisor.kvm.resource.wrapper");
>>> Set<Class<? extends CommandWrapper>> wrappers =
>>> reflections.getSubTypesOf(CommandWrapper.class);
>>>
>>> So here in "new Reflections" we are automatically filtering for
>>> just the wrappers that would apply to the KVM plugin. Then to
>>> finish it off, you iterate through the wrappers and do:
>>>
>>> ResourceWrapper annotation =
>>> wrapper.getAnnotation(ResourceWrapper.class);
>>> citrixCommands.put(annotation.handles(), wrapper.newInstance());
>>>
>>> Sorry, I guess that's four lines, plus the relevant for loop. And
>>> probably a null check or something for the annotation. You also
>>> have to add the annotation class itself, and add a line for the
>>> annotation in each wrapper, but in the end when we add new
>>> Commands, we won't have to touch anything but the new class that
>>> handles the command.
>>>
>>>
>>> public @interface ResourceWrapper {
>>>
>>> Class<? extends Command> handles();
>>>
>>> }
>>>
>>> There's an example of something similar to this in
>>> KVMStoragePoolManager.java (annotation is StoragePoolInfo.java).
>>> This example has actually been adapted from that. Also to a lesser
>>> extent in the API server, but it is spread across a bunch of
>>> classes.
>>>
>>> On Thu, Apr 30, 2015 at 10:41 PM, Wilder Rodrigues
>>> <WR...@schubergphilis.com><m
>> ailto:WRodrigues@schubergphilis.com>
>>>
>>>
>>>
>>>
>>> wrote: Hi Marcus,
>>>
>>> Thanks for the email… I’m always in for improvements. But why can’t
>>> you share the code?
>>>
>>> Few points below:
>>>
>>> 1. I added an subclassing example of LibvirtComputingResource
>>> because you mentioned it in a previous email:
>>>
>>> On 23 Apr 2015, at 17:26, Marcus
>>> <sh...@gmail.com><mailto:shadowsor@gmai
>> l.com>>
>>> wrote:
>>>
>>> I mentioned the reflection model because that's how I tend to
>>> handle the commands when subclassing LibvirtComputingResource.
>>>
>>> 2. Current situation with LibvirtComputingResource on Master is:
>>>
>>> a. 67 IFs b. 67 private/protected methods that are used only there
>>> c. If a new Command is added it means we will have a new IF and a
>>> new private method e. Maintenance is hell, test is close to zero
>>> and code quality is below expectations
>>>
>>> That being said, the main idea with the refactor is to change
>>> structure only, not behaviour. So what I’m doing is to simply move
>>> the code out the LibvirtCompRes and write tests for it, keeping the
>>> behaviour the same - to be done in a next phase. If you look at the
>>> changes you will see that some wrappers are already 100% covered.
>>> However, some others have 4% or 8% (not that much though). I would
>>> like to refactor that as well, but that could change behaviour
>>> (mentioned above) which I don’t want to touch now.
>>>
>>> 3. With the new situation:
>>>
>>> a. No IFs b. All methods wrapped by other classes (command
>>> wrappers) - loosely coupled, easier to test and maintain c. If a
>>> new Command is added we would have to add a command wrapper and 1
>>> line in the request wrapper implementation ( I know, it hurts you a
>>> bit) - but please bear with me for the good news.
>>>
>>> 4. the warnings are due to that: Hashtable<Class<? extends
>>> Command>, CommandWrapper>()
>>>
>>> No big deal.
>>>
>>> As I understood from your first paragraph we would have to
>>> annotated the commands classes, right? I mean, all of them.
>>>
>>> That’s something I wouldn’t do in this phase, to be honest. It
>>> might seem harmless to do, but I like to break things down a bit
>>> and have more isolation in my changes.
>>>
>>> What’s next: I will finish the refactor with the request wrapper as
>>> it is. For me it is no problem do add the lines now and remove them
>>> in 1 week. Most of the work is concentrated in the tests, which I’m
>>> trying as hard as I can to get them in the best way possible. Once
>>> it’s done and pushed to master, I will analyse what we would need
>>> to apply the annotation.
>>>
>>> But before I go to bring the kids to school, just one question:
>>>
>>> a. The “handle” value, in the annotation, would have the wrapper
>>> class that would be used for that command, right? Now let’s get 1
>>> command as example: CheckHealthCommand. Its wrapper implementation
>>> differs per hypervisor (just like all the other wrapper commands
>>> do). I’m not taking the time to really think about it now, but how
>>> would we annotated the different wrappers per command?
>>>
>>> Thanks again for your time.
>>>
>>> Cheers, Wilder
>>>
>>>
>>> On 30 Apr 2015, at 22:52, Marcus
>>> <sh...@gmail.com><mailto:shadowsor@gmai
>> l.com>>
>>> wrote:
>>>
>>> Ok. I wish I could share some code, because it isn't really as big
>>> of a deal as it sounds from your reasoning. It is literally just 3
>>> lines on startup that fetch anything with the '@AgentExecutor'
>>> annotation and stores it in a hash whose key is the value from
>>> @AgentExecutor's 'handles' property. Then when a *Command comes it
>>> it is passed to the appropriate Executor class.
>>>
>>> Looking at CitrixRequestWrapper, the 3 lines I mention are almost
>>> identical in function to your init method, just that it uses the
>>> annotation to find all of the commands, rather than hardcoding
>>> them. We use the same annotation design for the api side of the
>>> code on the management server, which allows the api commands to be
>>> easier to write and self-contained (you don't have to update other
>>> code to add a new api call). It makes things easier for novice
>>> developers.
>>>
>>> This implementation is no less typesafe than the previous design
>>> (the one with all of the instanceof). It didn't require any casting
>>> or warning suppression, either, as the wrapper does.
>>>
>>> Extending LibvirtComputingResource is not ideal, and doesn't work
>>> if multiple third parties are involved. Granted, there hasn't been
>>> a lot of demand for this, nevertheless it's particularly important
>>> for KVM, where the Command classes are executed on the hypervisor
>>> it's not really feasible to just dump the code in your management
>>> server-side plugin like some plugins do.
>>>
>>> In reviewing the code, the two implementations are really very
>>> close. If you just updated init to fetch the wrappers based on
>>> either an annotation or the class they extend, or something along
>>> those lines so this method doesn't have to be edited every time a
>>> command is added, that would be more or less the same thing. The
>>> the KVM agent would be pluggable like the management server side
>>> is.
>>>
>>> On Thu, Apr 30, 2015 at 12:55 PM, Wilder Rodrigues
>>> <WR...@schubergphilis.com><m
>> ailto:WRodrigues@schubergphilis.com>
>>>
>>>
>>>
>>>
>>> wrote: Hi Marcus,
>>>
>>> Apologies for taking so much time to reply to your email, but was,
>>> and still am, quite busy. :)
>>>
>>> I would only use reflection if that was the only way to do it. The
>>> use of reflection usually makes the code more complex, which is not
>>> good when we have java developers in all different levels (from jr.
>>> do sr) working with cloudstack. It also makes us lose the type
>>> safety, which might also harm the exception handling if not done
>>> well. In addition, if we need to refactor something, the IDE is no
>>> longer going to do few things because the refection code cannot be
>>> found.
>>>
>>> If someone will need to extend the LibvirtComputingResource that
>>> would be no problem with the approach I’m using. The
>>> CitrixResourceBase also has quite few sub-classes and it works just
>>> fine.
>>>
>>> I will document on the wiki page how it should be done when
>>> sub-classing the LibvirtComputingResource class.
>>>
>>> In a quick note/snippet, one would do:
>>>
>>> public class EkhoComputingResource extends LibvirtComputingResource
>>> {
>>>
>>> @Override public Answer executeRequest(final Command cmd) {
>>>
>>> final LibvirtRequestWrapper wrapper =
>>> LibvirtRequestWrapper.getInstance(); try { return
>>> wrapper.execute(cmd, this); } catch (final Exception e) { return
>>> Answer.createUnsupportedCommandAnswer(cmd); } } }
>>>
>>>
>>> In the flyweight where I keep the wrapper we could have ():
>>>
>>> final Hashtable<Class<? extends Command>, CommandWrapper>
>>> linbvirtCommands = new Hashtable<Class<? extends Command>,
>>> CommandWrapper>(); linbvirtCommands.put(StopCommand.class, new
>>> LibvirtStopCommandWrapper());
>>>
>>> final Hashtable<Class<? extends Command>, CommandWrapper>
>>> ekhoCommands = new Hashtable<Class<? extends Command>,
>>> CommandWrapper>(); linbvirtCommands.put(StopCommand.class, new
>>> EkhoStopCommandWrapper());
>>>
>>> resources.put(LibvirtComputingResource.class, linbvirtCommands);
>>> resources.put(EkhoComputingResource.class, ekhoCommands);
>>>
>>> But that is needed only if the StopCommand has a different
>>> behaviour for the EkhoComputingResource.
>>>
>>> Once a better version of the documentation is on the wiki, I will
>>> let you know.
>>>
>>> On other matters, I’m also adding unit tests for all the changes.
>>> We already went from 4% to 13.6% coverage in the KVM hypervisor
>>> plugin. The code I already refactored has 56% of coverage.
>>>
>>> You can see all the commits here:
>>> https://github.com/schubergphilis/cloudstack/tree/refactor/libvirt_r
>>>
>>>
>> es
>>>
>>>
>>> ource
>>>
>>> Cheers, Wilder
>>>
>>> On 23 Apr 2015, at 17:26, Marcus
>>> <sh...@gmail.com><mailto:shadowsor@gmai
>> l.com>>
>>> wrote:
>>>
>>> Great to see someone working on it. What sorts of roadblocks came
>>> out of reflection? How does the wrapper design solve the
>>> pluggability issue? This is pretty important to me, since I've
>>> worked with several companies now that end up subclassing
>>> LibvirtComputingResource in order to handle their own Commands on
>>> the hypervisor from their server-side plugins, and changing their
>>> 'resource' to that in agent.properties. Since the main agent class
>>> needs to be set at agent join, this is harder to manage than it
>>> should be.
>>>
>>> I mentioned the reflection model because that's how I tend to
>>> handle the commands when subclassing LibvirtComputingResource. I
>>> haven't had any problems with it, but then again I haven't tried to
>>> refactor 5500 lines into that model, either.
>>>
>>> On Thu, Apr 23, 2015 at 1:17 AM, Wilder Rodrigues
>>> <WR...@schubergphilis.com><m
>> ailto:WRodrigues@schubergphilis.com>
>>>
>>>
>>>
>>>
>>> wrote:
>>>
>>> Hi Marcus,
>>>
>>> I like the annotation idea, but reflection is trick because it
>>> hides some information about the code.
>>>
>>> Please, have a look at the CitrixResourceBase after the refactor I
>>> did. It became quite smaller and test coverage was improved.
>>>
>>> URL:
>>> https://cwiki.apache.org/confluence/display/CLOUDSTACK/Refactoring+X
>>>
>>>
>> en
>>>
>>>
>>> Server+Hypervisor+Plugin
>>>
>>> The same patter is being about to Libvirt stuff. The coverage on
>>> the KVM hypervisor plugin already went from 4 to 10.5% after
>>> refactoring 6 commands
>>>
>>> Cheers, Wilder
>>>
>>> On 22 Apr 2015, at 23:06, Marcus
>>> <sh...@gmail.com><mailto:shadowsor@gmai
>> l.com>>
>>> wrote:
>>>
>>> Kind of a tangent, but I'd actually like to see some work done to
>>> clean up LibvirtComputing resource. One model I've prototyped that
>>> seems to work is to create an annotation, such as
>>> 'KVMCommandExecutor', with a 'handles' property. With this
>>> annotation, you implement a class that handles, e.g. StartCommand,
>>> etc. Then in LibvirtComputingResource, the 'configure' method
>>> fetches all of these executors via reflection and stores them in an
>>> object. Then, instead of having all of the 'instanceof' lines in
>>> LibvirtComputingResource, the executeRequest method fetches the
>>> executor that handles the incoming command and runs it.
>>>
>>> I think this would break up LibvirtComputingResource into smaller,
>>> more testable and manageable chunks, and force things like config
>>> and utility methods to move to a more sane location, as well. As a
>>> bonus, this model makes things pluggable. Someone could ship KVM
>>> plugin code containing standalone command executors that are
>>> discovered at runtime for things they need to run at the hypervisor
>>> level.
>>>
>>> On Tue, Apr 21, 2015 at 6:27 AM, Wilder Rodrigues
>>> <WR...@schubergphilis.com><m
>> ailto:WRodrigues@schubergphilis.com>
>>>
>>>
>>>
>>>
>>> wrote:
>>>
>>> Hi all,
>>>
>>> Yesterday I started working on the LibvirtComputingResource class
>>> in order to apply the same patterns I used in the
>>> CitrixResourceBase + add more unit tests to it After 10 hours of
>>> work I got a bit stuck with the 1st test, which would cover the
>>> refactored LibvirtStopCommandWrapper. Why did I get stuck? The
>>> class used a few static methods that call native libraries, which I
>>> would like to mock. However, when writing the tests I faced
>>> problems with the current Mockito/PowerMock we are using: they are
>>> simply not enough for the task.
>>>
>>> What did I do then? I added a dependency to EasyMock and
>>> PowerMock-EasyMock API. It worked almost fine, but I had to add a
>>> “-noverify” to both my Eclipse Runtime configuration and also to
>>> the cloud-plugin-hypervisor-kvm/pom.xml file. I agree that’s not
>>> nice, but was my first attempt of getting it to work. After trying
>>> to first full build I faced more problems related to
>>> ClassDefNotFoundExpcetion which were complaining about Mockito
>>> classes. I then found out that adding the PowerMockRunner to all
>>> the tests classes was going to be a heavy burden and would also
>>> mess up future changes (e.g. the -noverify flag was removed from
>>> Java 8, thus adding it now would be a problem soon).
>>>
>>> Now that the first 2 paragraphs explain a bit about the problem,
>>> let’s get to the solution: Java 8
>>>
>>> The VerifyError that I was getting was due to the use of the latest
>>> EasyMock release (3.3.1). I tried to downgrade it to 3.1/3.2 but it
>>> also did not work. My decision: do not refactor if the proper tests
>>> cannot be added. This left me with one action: migrate to Java 8.
>>>
>>> There were mentions about Java 8 in february[1] and now I will put
>>> some energy in making it happen.
>>>
>>> What is your opinion on it?
>>>
>>> Thanks in advance.
>>>
>>> Cheers, Wilder
>>>
>>> http://mail-archives.apache.org/mod_mbox/cloudstack-dev/201502.mbox/
>>>
>>>
>> %3
>>>
>>>
>>> C54EEF6BE.5040401@shapeblue.com<mailto:C54EEF6BE.5040401@shapeblue.com
>>> %3E<http://mail-archives.apache.org/mod_m
>>>
>>>
>> box/cloudstack-dev/201502.mbox/<54EEF6BE.5040401@shapeblue.com<mailto:54
>> EEF6BE.5040401@shapeblue.com><mailto
>>> :54
>>>
>>>
>>> EEF6BE.5040401@shapeblue.com<ma...@shapeblue.com>>>>
>>>
>>> Regards, Rohit Yadav Software Architect, ShapeBlue
>>>
>>>
>>> [cid:9DD97B41-04C5-45F0-92A7-951F3E962F7A]
>>>
>>>
>>> M. +91 88 262 30892 |
>>> rohit.yadav@shapeblue.com<ma...@shapeblue.com><mailto:roh
>> it.yadav@shapeblue.com>
>>>
>>>
>> Blog: bhaisaab.org<http://bhaisaab.org><http://bhaisaab.org> | Twitter:
>> @_bhaisaab
>>>
>>>
>>>
>>>
>>> Find out more about ShapeBlue and our range of CloudStack related
>>> services
>>>
>>> IaaS Cloud Design &
>>> Build<http://shapeblue.com/iaas-cloud-design-and-build//> CSForge –
>>> rapid IaaS deployment framework<http://shapeblue.com/csforge/>
>>> CloudStack
>>> Consulting<http://shapeblue.com/cloudstack-consultancy/> CloudStack
>>> Software
>>> Engineering<http://shapeblue.com/cloudstack-software-engineering/>
>>>
>>>
>>> CloudStack Infrastructure
>>> Support<http://shapeblue.com/cloudstack-infrastructure-support/>
>>>
>>>
>>> CloudStack Bootcamp Training
>>> Courses<http://shapeblue.com/cloudstack-training/>
>>>
>>> This email and any attachments to it may be confidential and are
>>> intended solely for the use of the individual to whom it is
>>> addressed. Any views or opinions expressed are solely those of the
>>> author and do not necessarily represent those of Shape Blue Ltd or
>>> related companies. If you are not the intended recipient of this
>>> email, you must neither take any action based upon its contents,
>>> nor copy or show it to anyone. Please contact the sender if you
>>> believe you have received this email in error. Shape Blue Ltd is a
>>> company incorporated in England & Wales. ShapeBlue Services India
>>> LLP is a company incorporated in India and is operated under
>>> license from Shape Blue Ltd. Shape Blue Brasil Consultoria Ltda is
>>> a company incorporated in Brasil and is operated under license from
>>> Shape Blue Ltd. ShapeBlue SA Pty Ltd is a company registered by The
>>> Republic of South Africa and is traded under license from Shape
>>> Blue Ltd. ShapeBlue is a registered trademark.
>>>
>>>
>>>
>>> Find out more about ShapeBlue and our range of CloudStack related
>>> services
>>>
>>> IaaS Cloud Design &
>>> Build<http://shapeblue.com/iaas-cloud-design-and-build//> CSForge
>>> – rapid IaaS deployment framework<http://shapeblue.com/csforge/>
>>> CloudStack Consulting<http://shapeblue.com/cloudstack-consultancy/>
>>> CloudStack Software
>>> Engineering<http://shapeblue.com/cloudstack-software-engineering/>
>>> CloudStack Infrastructure
>>> Support<http://shapeblue.com/cloudstack-infrastructure-support/>
>>> CloudStack Bootcamp Training
>>> Courses<http://shapeblue.com/cloudstack-training/>
>>>
>>> This email and any attachments to it may be confidential and are
>>> intended solely for the use of the individual to whom it is
>>> addressed. Any views or opinions expressed are solely those of the
>>> author and do not necessarily represent those of Shape Blue Ltd or
>>> related companies. If you are not the intended recipient of this
>>> email, you must neither take any action based upon its contents,
>>> nor copy or show it to anyone. Please contact the sender if you
>>> believe you have received this email in error. Shape Blue Ltd is a
>>> company incorporated in England & Wales. ShapeBlue Services India
>>> LLP is a company incorporated in India and is operated under
>>> license from Shape Blue Ltd. Shape Blue Brasil Consultoria Ltda is
>>> a company incorporated in Brasil and is operated under license
>>> from Shape Blue Ltd. ShapeBlue SA Pty Ltd is a company registered
>>> by The Republic of South Africa and is traded under license from
>>> Shape Blue Ltd. ShapeBlue is a registered trademark.
>>>
>>>
>>>
>
> Find out more about ShapeBlue and our range of CloudStack related services
>
> IaaS Cloud Design & Build<http://shapeblue.com/iaas-cloud-design-and-build//>
> CSForge – rapid IaaS deployment framework<http://shapeblue.com/csforge/>
> CloudStack Consulting<http://shapeblue.com/cloudstack-consultancy/>
> CloudStack Software Engineering<http://shapeblue.com/cloudstack-software-engineering/>
> CloudStack Infrastructure Support<http://shapeblue.com/cloudstack-infrastructure-support/>
> CloudStack Bootcamp Training Courses<http://shapeblue.com/cloudstack-training/>
>
> This email and any attachments to it may be confidential and are intended solely for the use of the individual to whom it is addressed. Any views or opinions expressed are solely those of the author and do not necessarily represent those of Shape Blue Ltd or related companies. If you are not the intended recipient of this email, you must neither take any action based upon its contents, nor copy or show it to anyone. Please contact the sender if you believe you have received this email in error. Shape Blue Ltd is a company incorporated in England & Wales. ShapeBlue Services India LLP is a company incorporated in India and is operated under license from Shape Blue Ltd. Shape Blue Brasil Consultoria Ltda is a company incorporated in Brasil and is operated under license from Shape Blue Ltd. ShapeBlue SA Pty Ltd is a company registered by The Republic of South Africa and is traded under license from Shape Blue Ltd. ShapeBlue is a registered trademark.
>
Re: [DISCUSS] Moving to Java 8
Posted by John Burwell <jo...@shapeblue.com>.
Wido,
We have an acute problem — Oracle will be issuing no further security patches for Java7 which is a significant opsec risk. Put simply, we can’t leave our users exposed to such a risk because Ubuntu won’t ship a release for an non-EOL’ed Java version for another year. Personally, I think it is a tremendous oversight that Java7 has been EOL’ed for over three (3) months [1], and we have not addressed the issue for our users. CloudStack should have officially supported Java8 before Java7 reached EOL.
To minimize the impact of the change and smooth the transition, I suggest continuing to build with source and runtime compatibility set to 1.7 and test builds on both Java7 and Java8 until CloudStack 5. This approach will allow users to gradually transition from Java7 to Java8 with support for Java7 being eliminated at the next major release.
Thanks,
-John
[1]: http://www.oracle.com/technetwork/java/eol-135779.html
---
John Burwell (@john_burwell)
VP of Software Engineering, ShapeBlue
(571) 403-2411 | +44 20 3603 0542
http://www.shapeblue.com
Please join us at CloudStack Collab EU — http://events.linuxfoundation.org/events/cloudstack-collaboration-conference-europe
> On Jul 15, 2015, at 7:57 AM, Wido den Hollander <wi...@widodh.nl> wrote:
>
> Signed PGP part
>
>
> On 15-07-15 09:20, Wilder Rodrigues wrote:
> > If there would be dependencies on some other things, that in no way
> > could be fixed now, we could wait for 4.7 (5.0). However, if we
> > could give it a go, I would be able to tackle this in our next
> > Sprint (within 1 1/2 week from now) and still get it into 4.6.
> >
> > What would be the main considerations?
> >
>
> Well, on Ubuntu systems we simply we will require a additional
> software repo. The good thing is that OpenJDK 8 is available for 12.04
> and 14.04
>
> I don't feel like we should do something like this in a 4.X release,
> but that's me.
>
> Wido
>
> > Cheers, Wilder
> >
> >
> > On 14 Jul 2015, at 22:25, Wido den Hollander
> > <wi...@widodh.nl>> wrote:
> >
> >
> >
> > On 07/14/2015 10:18 PM, John Burwell wrote: Wido,
> >
> > Is the OpenJDK PPA [1] not acceptable? Since Java7 is no longer
> > supported, we run the risk of an Java security issue affecting the
> > project that won’t be fixed.
> >
> >
> > I didn't know that a PPA with OpenJDk existed. Looking at it I see
> > that the maintainer Matthias Klose works for Canonical, so it
> > seems like an official PPA.
> >
> > Still, having users adding PPAs is something we want to prevent as
> > much as possible, but I do recognize the problem here.
> >
> > Ubuntu 16.04 is less then a year away and will have Java 8 support,
> > so that will be resolved by then, but for now it is a problem.
> >
> > I think that CloudStack 4.6 is to early, but maybe 4.7 (called
> > 5.0?) is a good time to make the move?
> >
> > Wido
> >
> > Thanks, -John
> >
> > [1]: https://launchpad.net/~openjdk-r/+archive/ubuntu/ppa
> >
> > --- John Burwell (@john_burwell) VP of Software Engineering,
> > ShapeBlue (571) 403-2411 | +44 20 3603 0542
> > http://www.shapeblue.com Please join us at CloudStack Collab EU —
> > http://events.linuxfoundation.org/events/cloudstack-collaboration-conf
> >
> >
> erence-europe
> >
> >
> >
> >
> >
> > On Jul 10, 2015, at 5:47 PM, Wido den Hollander
> > <wi...@widodh.nl>> wrote:
> >
> > Signed PGP part
> >
> >
> > On 07/10/2015 09:22 PM, Rohit Yadav wrote: Ping Wilder - any
> > progress/plan on moving forward (perhaps after 4.6?).
> >
> >
> > I don't think there is? Since Ubuntu 14.04 doesn't support Java 8
> > in any package form we can't really continue.
> >
> > Ubuntu 16.04 will ship with Java 8 and that will be the next LTS.
> >
> > Wido
> >
> > On 01-May-2015, at 4:07 pm, Wilder Rodrigues
> > <WR...@schubergphilis.com><m
> ailto:WRodrigues@schubergphilis.com>
> >
> >
> >
> >
> > wrote:
> >
> > Hi Marcus,
> >
> > Sorry for the push, I think after doing the whole
> > CitrixResourceBase refactor I also got a bit attached to the whole
> > thing/solution. ;)
> >
> > Thanks for the input you gave. I will finish the refactor and apply
> > it to both implementations.
> >
> > Cheers, Wilder
> >
> >
> > On 01 May 2015, at 09:06, Marcus
> > <sh...@gmail.com><mailto:shadowsor@gmai
> l.com><mailto:shadowsor@gm
> >
> >
> ai
> >
> >
> > l.com<http://l.com>>> wrote:
> >
> > Oh, and of course the annotation added to the wrapper looks like:
> >
> >
> > ...
> >
> > @ResourceWrapper(handles = CheckHealthCommand.class)
> >
> > public final class LibvirtCheckHealthCommandWrapper
> >
> > ...
> >
> >
> > maybe 'wraps' or 'wrapperfor' would be better than 'handles' in
> > your naming scheme. You get the idea.
> >
> > On Thu, Apr 30, 2015 at 11:59 PM, Marcus
> > <sh...@gmail.com><mailto:shadowsor@gmai
> l.com>>
> > wrote: I agree, this wrapper is a good step forward. It's totally
> > fine to continue on that path because it is obviously better and
> > makes it easy to switch to autodetection anytime later by simply
> > adding the annotation. Sorry if I got a bit passionate about that,
> > but as you mention I also get tired of adding things in multiple
> > places, and the annotations have worked well in the API and provide
> > a good model to emulate for consistency.
> >
> > I can't share code, because these extensions to
> > LibvirtComputingResource that I've provided for other companies
> > have not been open sourced. I can speak more generically though
> > about methods.
> >
> > To answer question "a", reflection allows you to do something
> > like:
> >
> > Reflections reflections = new
> > Reflections("com.cloud.hypervisor.kvm.resource.wrapper");
> > Set<Class<? extends CommandWrapper>> wrappers =
> > reflections.getSubTypesOf(CommandWrapper.class);
> >
> > So here in "new Reflections" we are automatically filtering for
> > just the wrappers that would apply to the KVM plugin. Then to
> > finish it off, you iterate through the wrappers and do:
> >
> > ResourceWrapper annotation =
> > wrapper.getAnnotation(ResourceWrapper.class);
> > citrixCommands.put(annotation.handles(), wrapper.newInstance());
> >
> > Sorry, I guess that's four lines, plus the relevant for loop. And
> > probably a null check or something for the annotation. You also
> > have to add the annotation class itself, and add a line for the
> > annotation in each wrapper, but in the end when we add new
> > Commands, we won't have to touch anything but the new class that
> > handles the command.
> >
> >
> > public @interface ResourceWrapper {
> >
> > Class<? extends Command> handles();
> >
> > }
> >
> > There's an example of something similar to this in
> > KVMStoragePoolManager.java (annotation is StoragePoolInfo.java).
> > This example has actually been adapted from that. Also to a lesser
> > extent in the API server, but it is spread across a bunch of
> > classes.
> >
> > On Thu, Apr 30, 2015 at 10:41 PM, Wilder Rodrigues
> > <WR...@schubergphilis.com><m
> ailto:WRodrigues@schubergphilis.com>
> >
> >
> >
> >
> > wrote: Hi Marcus,
> >
> > Thanks for the email… I’m always in for improvements. But why can’t
> > you share the code?
> >
> > Few points below:
> >
> > 1. I added an subclassing example of LibvirtComputingResource
> > because you mentioned it in a previous email:
> >
> > On 23 Apr 2015, at 17:26, Marcus
> > <sh...@gmail.com><mailto:shadowsor@gmai
> l.com>>
> > wrote:
> >
> > I mentioned the reflection model because that's how I tend to
> > handle the commands when subclassing LibvirtComputingResource.
> >
> > 2. Current situation with LibvirtComputingResource on Master is:
> >
> > a. 67 IFs b. 67 private/protected methods that are used only there
> > c. If a new Command is added it means we will have a new IF and a
> > new private method e. Maintenance is hell, test is close to zero
> > and code quality is below expectations
> >
> > That being said, the main idea with the refactor is to change
> > structure only, not behaviour. So what I’m doing is to simply move
> > the code out the LibvirtCompRes and write tests for it, keeping the
> > behaviour the same - to be done in a next phase. If you look at the
> > changes you will see that some wrappers are already 100% covered.
> > However, some others have 4% or 8% (not that much though). I would
> > like to refactor that as well, but that could change behaviour
> > (mentioned above) which I don’t want to touch now.
> >
> > 3. With the new situation:
> >
> > a. No IFs b. All methods wrapped by other classes (command
> > wrappers) - loosely coupled, easier to test and maintain c. If a
> > new Command is added we would have to add a command wrapper and 1
> > line in the request wrapper implementation ( I know, it hurts you a
> > bit) - but please bear with me for the good news.
> >
> > 4. the warnings are due to that: Hashtable<Class<? extends
> > Command>, CommandWrapper>()
> >
> > No big deal.
> >
> > As I understood from your first paragraph we would have to
> > annotated the commands classes, right? I mean, all of them.
> >
> > That’s something I wouldn’t do in this phase, to be honest. It
> > might seem harmless to do, but I like to break things down a bit
> > and have more isolation in my changes.
> >
> > What’s next: I will finish the refactor with the request wrapper as
> > it is. For me it is no problem do add the lines now and remove them
> > in 1 week. Most of the work is concentrated in the tests, which I’m
> > trying as hard as I can to get them in the best way possible. Once
> > it’s done and pushed to master, I will analyse what we would need
> > to apply the annotation.
> >
> > But before I go to bring the kids to school, just one question:
> >
> > a. The “handle” value, in the annotation, would have the wrapper
> > class that would be used for that command, right? Now let’s get 1
> > command as example: CheckHealthCommand. Its wrapper implementation
> > differs per hypervisor (just like all the other wrapper commands
> > do). I’m not taking the time to really think about it now, but how
> > would we annotated the different wrappers per command?
> >
> > Thanks again for your time.
> >
> > Cheers, Wilder
> >
> >
> > On 30 Apr 2015, at 22:52, Marcus
> > <sh...@gmail.com><mailto:shadowsor@gmai
> l.com>>
> > wrote:
> >
> > Ok. I wish I could share some code, because it isn't really as big
> > of a deal as it sounds from your reasoning. It is literally just 3
> > lines on startup that fetch anything with the '@AgentExecutor'
> > annotation and stores it in a hash whose key is the value from
> > @AgentExecutor's 'handles' property. Then when a *Command comes it
> > it is passed to the appropriate Executor class.
> >
> > Looking at CitrixRequestWrapper, the 3 lines I mention are almost
> > identical in function to your init method, just that it uses the
> > annotation to find all of the commands, rather than hardcoding
> > them. We use the same annotation design for the api side of the
> > code on the management server, which allows the api commands to be
> > easier to write and self-contained (you don't have to update other
> > code to add a new api call). It makes things easier for novice
> > developers.
> >
> > This implementation is no less typesafe than the previous design
> > (the one with all of the instanceof). It didn't require any casting
> > or warning suppression, either, as the wrapper does.
> >
> > Extending LibvirtComputingResource is not ideal, and doesn't work
> > if multiple third parties are involved. Granted, there hasn't been
> > a lot of demand for this, nevertheless it's particularly important
> > for KVM, where the Command classes are executed on the hypervisor
> > it's not really feasible to just dump the code in your management
> > server-side plugin like some plugins do.
> >
> > In reviewing the code, the two implementations are really very
> > close. If you just updated init to fetch the wrappers based on
> > either an annotation or the class they extend, or something along
> > those lines so this method doesn't have to be edited every time a
> > command is added, that would be more or less the same thing. The
> > the KVM agent would be pluggable like the management server side
> > is.
> >
> > On Thu, Apr 30, 2015 at 12:55 PM, Wilder Rodrigues
> > <WR...@schubergphilis.com><m
> ailto:WRodrigues@schubergphilis.com>
> >
> >
> >
> >
> > wrote: Hi Marcus,
> >
> > Apologies for taking so much time to reply to your email, but was,
> > and still am, quite busy. :)
> >
> > I would only use reflection if that was the only way to do it. The
> > use of reflection usually makes the code more complex, which is not
> > good when we have java developers in all different levels (from jr.
> > do sr) working with cloudstack. It also makes us lose the type
> > safety, which might also harm the exception handling if not done
> > well. In addition, if we need to refactor something, the IDE is no
> > longer going to do few things because the refection code cannot be
> > found.
> >
> > If someone will need to extend the LibvirtComputingResource that
> > would be no problem with the approach I’m using. The
> > CitrixResourceBase also has quite few sub-classes and it works just
> > fine.
> >
> > I will document on the wiki page how it should be done when
> > sub-classing the LibvirtComputingResource class.
> >
> > In a quick note/snippet, one would do:
> >
> > public class EkhoComputingResource extends LibvirtComputingResource
> > {
> >
> > @Override public Answer executeRequest(final Command cmd) {
> >
> > final LibvirtRequestWrapper wrapper =
> > LibvirtRequestWrapper.getInstance(); try { return
> > wrapper.execute(cmd, this); } catch (final Exception e) { return
> > Answer.createUnsupportedCommandAnswer(cmd); } } }
> >
> >
> > In the flyweight where I keep the wrapper we could have ():
> >
> > final Hashtable<Class<? extends Command>, CommandWrapper>
> > linbvirtCommands = new Hashtable<Class<? extends Command>,
> > CommandWrapper>(); linbvirtCommands.put(StopCommand.class, new
> > LibvirtStopCommandWrapper());
> >
> > final Hashtable<Class<? extends Command>, CommandWrapper>
> > ekhoCommands = new Hashtable<Class<? extends Command>,
> > CommandWrapper>(); linbvirtCommands.put(StopCommand.class, new
> > EkhoStopCommandWrapper());
> >
> > resources.put(LibvirtComputingResource.class, linbvirtCommands);
> > resources.put(EkhoComputingResource.class, ekhoCommands);
> >
> > But that is needed only if the StopCommand has a different
> > behaviour for the EkhoComputingResource.
> >
> > Once a better version of the documentation is on the wiki, I will
> > let you know.
> >
> > On other matters, I’m also adding unit tests for all the changes.
> > We already went from 4% to 13.6% coverage in the KVM hypervisor
> > plugin. The code I already refactored has 56% of coverage.
> >
> > You can see all the commits here:
> > https://github.com/schubergphilis/cloudstack/tree/refactor/libvirt_r
> >
> >
> es
> >
> >
> > ource
> >
> > Cheers, Wilder
> >
> > On 23 Apr 2015, at 17:26, Marcus
> > <sh...@gmail.com><mailto:shadowsor@gmai
> l.com>>
> > wrote:
> >
> > Great to see someone working on it. What sorts of roadblocks came
> > out of reflection? How does the wrapper design solve the
> > pluggability issue? This is pretty important to me, since I've
> > worked with several companies now that end up subclassing
> > LibvirtComputingResource in order to handle their own Commands on
> > the hypervisor from their server-side plugins, and changing their
> > 'resource' to that in agent.properties. Since the main agent class
> > needs to be set at agent join, this is harder to manage than it
> > should be.
> >
> > I mentioned the reflection model because that's how I tend to
> > handle the commands when subclassing LibvirtComputingResource. I
> > haven't had any problems with it, but then again I haven't tried to
> > refactor 5500 lines into that model, either.
> >
> > On Thu, Apr 23, 2015 at 1:17 AM, Wilder Rodrigues
> > <WR...@schubergphilis.com><m
> ailto:WRodrigues@schubergphilis.com>
> >
> >
> >
> >
> > wrote:
> >
> > Hi Marcus,
> >
> > I like the annotation idea, but reflection is trick because it
> > hides some information about the code.
> >
> > Please, have a look at the CitrixResourceBase after the refactor I
> > did. It became quite smaller and test coverage was improved.
> >
> > URL:
> > https://cwiki.apache.org/confluence/display/CLOUDSTACK/Refactoring+X
> >
> >
> en
> >
> >
> > Server+Hypervisor+Plugin
> >
> > The same patter is being about to Libvirt stuff. The coverage on
> > the KVM hypervisor plugin already went from 4 to 10.5% after
> > refactoring 6 commands
> >
> > Cheers, Wilder
> >
> > On 22 Apr 2015, at 23:06, Marcus
> > <sh...@gmail.com><mailto:shadowsor@gmai
> l.com>>
> > wrote:
> >
> > Kind of a tangent, but I'd actually like to see some work done to
> > clean up LibvirtComputing resource. One model I've prototyped that
> > seems to work is to create an annotation, such as
> > 'KVMCommandExecutor', with a 'handles' property. With this
> > annotation, you implement a class that handles, e.g. StartCommand,
> > etc. Then in LibvirtComputingResource, the 'configure' method
> > fetches all of these executors via reflection and stores them in an
> > object. Then, instead of having all of the 'instanceof' lines in
> > LibvirtComputingResource, the executeRequest method fetches the
> > executor that handles the incoming command and runs it.
> >
> > I think this would break up LibvirtComputingResource into smaller,
> > more testable and manageable chunks, and force things like config
> > and utility methods to move to a more sane location, as well. As a
> > bonus, this model makes things pluggable. Someone could ship KVM
> > plugin code containing standalone command executors that are
> > discovered at runtime for things they need to run at the hypervisor
> > level.
> >
> > On Tue, Apr 21, 2015 at 6:27 AM, Wilder Rodrigues
> > <WR...@schubergphilis.com><m
> ailto:WRodrigues@schubergphilis.com>
> >
> >
> >
> >
> > wrote:
> >
> > Hi all,
> >
> > Yesterday I started working on the LibvirtComputingResource class
> > in order to apply the same patterns I used in the
> > CitrixResourceBase + add more unit tests to it After 10 hours of
> > work I got a bit stuck with the 1st test, which would cover the
> > refactored LibvirtStopCommandWrapper. Why did I get stuck? The
> > class used a few static methods that call native libraries, which I
> > would like to mock. However, when writing the tests I faced
> > problems with the current Mockito/PowerMock we are using: they are
> > simply not enough for the task.
> >
> > What did I do then? I added a dependency to EasyMock and
> > PowerMock-EasyMock API. It worked almost fine, but I had to add a
> > “-noverify” to both my Eclipse Runtime configuration and also to
> > the cloud-plugin-hypervisor-kvm/pom.xml file. I agree that’s not
> > nice, but was my first attempt of getting it to work. After trying
> > to first full build I faced more problems related to
> > ClassDefNotFoundExpcetion which were complaining about Mockito
> > classes. I then found out that adding the PowerMockRunner to all
> > the tests classes was going to be a heavy burden and would also
> > mess up future changes (e.g. the -noverify flag was removed from
> > Java 8, thus adding it now would be a problem soon).
> >
> > Now that the first 2 paragraphs explain a bit about the problem,
> > let’s get to the solution: Java 8
> >
> > The VerifyError that I was getting was due to the use of the latest
> > EasyMock release (3.3.1). I tried to downgrade it to 3.1/3.2 but it
> > also did not work. My decision: do not refactor if the proper tests
> > cannot be added. This left me with one action: migrate to Java 8.
> >
> > There were mentions about Java 8 in february[1] and now I will put
> > some energy in making it happen.
> >
> > What is your opinion on it?
> >
> > Thanks in advance.
> >
> > Cheers, Wilder
> >
> > http://mail-archives.apache.org/mod_mbox/cloudstack-dev/201502.mbox/
> >
> >
> %3
> >
> >
> > C54EEF6BE.5040401@shapeblue.com<mailto:C54EEF6BE.5040401@shapeblue.com
> >%3E<http://mail-archives.apache.org/mod_m
> >
> >
> box/cloudstack-dev/201502.mbox/<54EEF6BE.5040401@shapeblue.com<mailto:54
> EEF6BE.5040401@shapeblue.com><mailto
> > :54
> >
> >
> > EEF6BE.5040401@shapeblue.com<ma...@shapeblue.com>>>>
> >
> > Regards, Rohit Yadav Software Architect, ShapeBlue
> >
> >
> > [cid:9DD97B41-04C5-45F0-92A7-951F3E962F7A]
> >
> >
> > M. +91 88 262 30892 |
> > rohit.yadav@shapeblue.com<ma...@shapeblue.com><mailto:roh
> it.yadav@shapeblue.com>
> >
> >
> Blog: bhaisaab.org<http://bhaisaab.org><http://bhaisaab.org> | Twitter:
> @_bhaisaab
> >
> >
> >
> >
> > Find out more about ShapeBlue and our range of CloudStack related
> > services
> >
> > IaaS Cloud Design &
> > Build<http://shapeblue.com/iaas-cloud-design-and-build//> CSForge –
> > rapid IaaS deployment framework<http://shapeblue.com/csforge/>
> > CloudStack
> > Consulting<http://shapeblue.com/cloudstack-consultancy/> CloudStack
> > Software
> > Engineering<http://shapeblue.com/cloudstack-software-engineering/>
> >
> >
> > CloudStack Infrastructure
> > Support<http://shapeblue.com/cloudstack-infrastructure-support/>
> >
> >
> > CloudStack Bootcamp Training
> > Courses<http://shapeblue.com/cloudstack-training/>
> >
> > This email and any attachments to it may be confidential and are
> > intended solely for the use of the individual to whom it is
> > addressed. Any views or opinions expressed are solely those of the
> > author and do not necessarily represent those of Shape Blue Ltd or
> > related companies. If you are not the intended recipient of this
> > email, you must neither take any action based upon its contents,
> > nor copy or show it to anyone. Please contact the sender if you
> > believe you have received this email in error. Shape Blue Ltd is a
> > company incorporated in England & Wales. ShapeBlue Services India
> > LLP is a company incorporated in India and is operated under
> > license from Shape Blue Ltd. Shape Blue Brasil Consultoria Ltda is
> > a company incorporated in Brasil and is operated under license from
> > Shape Blue Ltd. ShapeBlue SA Pty Ltd is a company registered by The
> > Republic of South Africa and is traded under license from Shape
> > Blue Ltd. ShapeBlue is a registered trademark.
> >
> >
> >
> > Find out more about ShapeBlue and our range of CloudStack related
> > services
> >
> > IaaS Cloud Design &
> > Build<http://shapeblue.com/iaas-cloud-design-and-build//> CSForge
> > – rapid IaaS deployment framework<http://shapeblue.com/csforge/>
> > CloudStack Consulting<http://shapeblue.com/cloudstack-consultancy/>
> > CloudStack Software
> > Engineering<http://shapeblue.com/cloudstack-software-engineering/>
> > CloudStack Infrastructure
> > Support<http://shapeblue.com/cloudstack-infrastructure-support/>
> > CloudStack Bootcamp Training
> > Courses<http://shapeblue.com/cloudstack-training/>
> >
> > This email and any attachments to it may be confidential and are
> > intended solely for the use of the individual to whom it is
> > addressed. Any views or opinions expressed are solely those of the
> > author and do not necessarily represent those of Shape Blue Ltd or
> > related companies. If you are not the intended recipient of this
> > email, you must neither take any action based upon its contents,
> > nor copy or show it to anyone. Please contact the sender if you
> > believe you have received this email in error. Shape Blue Ltd is a
> > company incorporated in England & Wales. ShapeBlue Services India
> > LLP is a company incorporated in India and is operated under
> > license from Shape Blue Ltd. Shape Blue Brasil Consultoria Ltda is
> > a company incorporated in Brasil and is operated under license
> > from Shape Blue Ltd. ShapeBlue SA Pty Ltd is a company registered
> > by The Republic of South Africa and is traded under license from
> > Shape Blue Ltd. ShapeBlue is a registered trademark.
> >
> >
> >
Find out more about ShapeBlue and our range of CloudStack related services
IaaS Cloud Design & Build<http://shapeblue.com/iaas-cloud-design-and-build//>
CSForge – rapid IaaS deployment framework<http://shapeblue.com/csforge/>
CloudStack Consulting<http://shapeblue.com/cloudstack-consultancy/>
CloudStack Software Engineering<http://shapeblue.com/cloudstack-software-engineering/>
CloudStack Infrastructure Support<http://shapeblue.com/cloudstack-infrastructure-support/>
CloudStack Bootcamp Training Courses<http://shapeblue.com/cloudstack-training/>
This email and any attachments to it may be confidential and are intended solely for the use of the individual to whom it is addressed. Any views or opinions expressed are solely those of the author and do not necessarily represent those of Shape Blue Ltd or related companies. If you are not the intended recipient of this email, you must neither take any action based upon its contents, nor copy or show it to anyone. Please contact the sender if you believe you have received this email in error. Shape Blue Ltd is a company incorporated in England & Wales. ShapeBlue Services India LLP is a company incorporated in India and is operated under license from Shape Blue Ltd. Shape Blue Brasil Consultoria Ltda is a company incorporated in Brasil and is operated under license from Shape Blue Ltd. ShapeBlue SA Pty Ltd is a company registered by The Republic of South Africa and is traded under license from Shape Blue Ltd. ShapeBlue is a registered trademark.
Re: [DISCUSS] Moving to Java 8
Posted by Wido den Hollander <wi...@widodh.nl>.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 15-07-15 09:20, Wilder Rodrigues wrote:
> If there would be dependencies on some other things, that in no way
> could be fixed now, we could wait for 4.7 (5.0). However, if we
> could give it a go, I would be able to tackle this in our next
> Sprint (within 1 1/2 week from now) and still get it into 4.6.
>
> What would be the main considerations?
>
Well, on Ubuntu systems we simply we will require a additional
software repo. The good thing is that OpenJDK 8 is available for 12.04
and 14.04
I don't feel like we should do something like this in a 4.X release,
but that's me.
Wido
> Cheers, Wilder
>
>
> On 14 Jul 2015, at 22:25, Wido den Hollander
> <wi...@widodh.nl>> wrote:
>
>
>
> On 07/14/2015 10:18 PM, John Burwell wrote: Wido,
>
> Is the OpenJDK PPA [1] not acceptable? Since Java7 is no longer
> supported, we run the risk of an Java security issue affecting the
> project that won’t be fixed.
>
>
> I didn't know that a PPA with OpenJDk existed. Looking at it I see
> that the maintainer Matthias Klose works for Canonical, so it
> seems like an official PPA.
>
> Still, having users adding PPAs is something we want to prevent as
> much as possible, but I do recognize the problem here.
>
> Ubuntu 16.04 is less then a year away and will have Java 8 support,
> so that will be resolved by then, but for now it is a problem.
>
> I think that CloudStack 4.6 is to early, but maybe 4.7 (called
> 5.0?) is a good time to make the move?
>
> Wido
>
> Thanks, -John
>
> [1]: https://launchpad.net/~openjdk-r/+archive/ubuntu/ppa
>
> --- John Burwell (@john_burwell) VP of Software Engineering,
> ShapeBlue (571) 403-2411 | +44 20 3603 0542
> http://www.shapeblue.com Please join us at CloudStack Collab EU —
> http://events.linuxfoundation.org/events/cloudstack-collaboration-conf
>
>
erence-europe
>
>
>
>
>
> On Jul 10, 2015, at 5:47 PM, Wido den Hollander
> <wi...@widodh.nl>> wrote:
>
> Signed PGP part
>
>
> On 07/10/2015 09:22 PM, Rohit Yadav wrote: Ping Wilder - any
> progress/plan on moving forward (perhaps after 4.6?).
>
>
> I don't think there is? Since Ubuntu 14.04 doesn't support Java 8
> in any package form we can't really continue.
>
> Ubuntu 16.04 will ship with Java 8 and that will be the next LTS.
>
> Wido
>
> On 01-May-2015, at 4:07 pm, Wilder Rodrigues
> <WR...@schubergphilis.com><m
ailto:WRodrigues@schubergphilis.com>
>
>
>
>
> wrote:
>
> Hi Marcus,
>
> Sorry for the push, I think after doing the whole
> CitrixResourceBase refactor I also got a bit attached to the whole
> thing/solution. ;)
>
> Thanks for the input you gave. I will finish the refactor and apply
> it to both implementations.
>
> Cheers, Wilder
>
>
> On 01 May 2015, at 09:06, Marcus
> <sh...@gmail.com><mailto:shadowsor@gmai
l.com><mailto:shadowsor@gm
>
>
ai
>
>
> l.com<http://l.com>>> wrote:
>
> Oh, and of course the annotation added to the wrapper looks like:
>
>
> ...
>
> @ResourceWrapper(handles = CheckHealthCommand.class)
>
> public final class LibvirtCheckHealthCommandWrapper
>
> ...
>
>
> maybe 'wraps' or 'wrapperfor' would be better than 'handles' in
> your naming scheme. You get the idea.
>
> On Thu, Apr 30, 2015 at 11:59 PM, Marcus
> <sh...@gmail.com><mailto:shadowsor@gmai
l.com>>
> wrote: I agree, this wrapper is a good step forward. It's totally
> fine to continue on that path because it is obviously better and
> makes it easy to switch to autodetection anytime later by simply
> adding the annotation. Sorry if I got a bit passionate about that,
> but as you mention I also get tired of adding things in multiple
> places, and the annotations have worked well in the API and provide
> a good model to emulate for consistency.
>
> I can't share code, because these extensions to
> LibvirtComputingResource that I've provided for other companies
> have not been open sourced. I can speak more generically though
> about methods.
>
> To answer question "a", reflection allows you to do something
> like:
>
> Reflections reflections = new
> Reflections("com.cloud.hypervisor.kvm.resource.wrapper");
> Set<Class<? extends CommandWrapper>> wrappers =
> reflections.getSubTypesOf(CommandWrapper.class);
>
> So here in "new Reflections" we are automatically filtering for
> just the wrappers that would apply to the KVM plugin. Then to
> finish it off, you iterate through the wrappers and do:
>
> ResourceWrapper annotation =
> wrapper.getAnnotation(ResourceWrapper.class);
> citrixCommands.put(annotation.handles(), wrapper.newInstance());
>
> Sorry, I guess that's four lines, plus the relevant for loop. And
> probably a null check or something for the annotation. You also
> have to add the annotation class itself, and add a line for the
> annotation in each wrapper, but in the end when we add new
> Commands, we won't have to touch anything but the new class that
> handles the command.
>
>
> public @interface ResourceWrapper {
>
> Class<? extends Command> handles();
>
> }
>
> There's an example of something similar to this in
> KVMStoragePoolManager.java (annotation is StoragePoolInfo.java).
> This example has actually been adapted from that. Also to a lesser
> extent in the API server, but it is spread across a bunch of
> classes.
>
> On Thu, Apr 30, 2015 at 10:41 PM, Wilder Rodrigues
> <WR...@schubergphilis.com><m
ailto:WRodrigues@schubergphilis.com>
>
>
>
>
> wrote: Hi Marcus,
>
> Thanks for the email… I’m always in for improvements. But why can’t
> you share the code?
>
> Few points below:
>
> 1. I added an subclassing example of LibvirtComputingResource
> because you mentioned it in a previous email:
>
> On 23 Apr 2015, at 17:26, Marcus
> <sh...@gmail.com><mailto:shadowsor@gmai
l.com>>
> wrote:
>
> I mentioned the reflection model because that's how I tend to
> handle the commands when subclassing LibvirtComputingResource.
>
> 2. Current situation with LibvirtComputingResource on Master is:
>
> a. 67 IFs b. 67 private/protected methods that are used only there
> c. If a new Command is added it means we will have a new IF and a
> new private method e. Maintenance is hell, test is close to zero
> and code quality is below expectations
>
> That being said, the main idea with the refactor is to change
> structure only, not behaviour. So what I’m doing is to simply move
> the code out the LibvirtCompRes and write tests for it, keeping the
> behaviour the same - to be done in a next phase. If you look at the
> changes you will see that some wrappers are already 100% covered.
> However, some others have 4% or 8% (not that much though). I would
> like to refactor that as well, but that could change behaviour
> (mentioned above) which I don’t want to touch now.
>
> 3. With the new situation:
>
> a. No IFs b. All methods wrapped by other classes (command
> wrappers) - loosely coupled, easier to test and maintain c. If a
> new Command is added we would have to add a command wrapper and 1
> line in the request wrapper implementation ( I know, it hurts you a
> bit) - but please bear with me for the good news.
>
> 4. the warnings are due to that: Hashtable<Class<? extends
> Command>, CommandWrapper>()
>
> No big deal.
>
> As I understood from your first paragraph we would have to
> annotated the commands classes, right? I mean, all of them.
>
> That’s something I wouldn’t do in this phase, to be honest. It
> might seem harmless to do, but I like to break things down a bit
> and have more isolation in my changes.
>
> What’s next: I will finish the refactor with the request wrapper as
> it is. For me it is no problem do add the lines now and remove them
> in 1 week. Most of the work is concentrated in the tests, which I’m
> trying as hard as I can to get them in the best way possible. Once
> it’s done and pushed to master, I will analyse what we would need
> to apply the annotation.
>
> But before I go to bring the kids to school, just one question:
>
> a. The “handle” value, in the annotation, would have the wrapper
> class that would be used for that command, right? Now let’s get 1
> command as example: CheckHealthCommand. Its wrapper implementation
> differs per hypervisor (just like all the other wrapper commands
> do). I’m not taking the time to really think about it now, but how
> would we annotated the different wrappers per command?
>
> Thanks again for your time.
>
> Cheers, Wilder
>
>
> On 30 Apr 2015, at 22:52, Marcus
> <sh...@gmail.com><mailto:shadowsor@gmai
l.com>>
> wrote:
>
> Ok. I wish I could share some code, because it isn't really as big
> of a deal as it sounds from your reasoning. It is literally just 3
> lines on startup that fetch anything with the '@AgentExecutor'
> annotation and stores it in a hash whose key is the value from
> @AgentExecutor's 'handles' property. Then when a *Command comes it
> it is passed to the appropriate Executor class.
>
> Looking at CitrixRequestWrapper, the 3 lines I mention are almost
> identical in function to your init method, just that it uses the
> annotation to find all of the commands, rather than hardcoding
> them. We use the same annotation design for the api side of the
> code on the management server, which allows the api commands to be
> easier to write and self-contained (you don't have to update other
> code to add a new api call). It makes things easier for novice
> developers.
>
> This implementation is no less typesafe than the previous design
> (the one with all of the instanceof). It didn't require any casting
> or warning suppression, either, as the wrapper does.
>
> Extending LibvirtComputingResource is not ideal, and doesn't work
> if multiple third parties are involved. Granted, there hasn't been
> a lot of demand for this, nevertheless it's particularly important
> for KVM, where the Command classes are executed on the hypervisor
> it's not really feasible to just dump the code in your management
> server-side plugin like some plugins do.
>
> In reviewing the code, the two implementations are really very
> close. If you just updated init to fetch the wrappers based on
> either an annotation or the class they extend, or something along
> those lines so this method doesn't have to be edited every time a
> command is added, that would be more or less the same thing. The
> the KVM agent would be pluggable like the management server side
> is.
>
> On Thu, Apr 30, 2015 at 12:55 PM, Wilder Rodrigues
> <WR...@schubergphilis.com><m
ailto:WRodrigues@schubergphilis.com>
>
>
>
>
> wrote: Hi Marcus,
>
> Apologies for taking so much time to reply to your email, but was,
> and still am, quite busy. :)
>
> I would only use reflection if that was the only way to do it. The
> use of reflection usually makes the code more complex, which is not
> good when we have java developers in all different levels (from jr.
> do sr) working with cloudstack. It also makes us lose the type
> safety, which might also harm the exception handling if not done
> well. In addition, if we need to refactor something, the IDE is no
> longer going to do few things because the refection code cannot be
> found.
>
> If someone will need to extend the LibvirtComputingResource that
> would be no problem with the approach I’m using. The
> CitrixResourceBase also has quite few sub-classes and it works just
> fine.
>
> I will document on the wiki page how it should be done when
> sub-classing the LibvirtComputingResource class.
>
> In a quick note/snippet, one would do:
>
> public class EkhoComputingResource extends LibvirtComputingResource
> {
>
> @Override public Answer executeRequest(final Command cmd) {
>
> final LibvirtRequestWrapper wrapper =
> LibvirtRequestWrapper.getInstance(); try { return
> wrapper.execute(cmd, this); } catch (final Exception e) { return
> Answer.createUnsupportedCommandAnswer(cmd); } } }
>
>
> In the flyweight where I keep the wrapper we could have ():
>
> final Hashtable<Class<? extends Command>, CommandWrapper>
> linbvirtCommands = new Hashtable<Class<? extends Command>,
> CommandWrapper>(); linbvirtCommands.put(StopCommand.class, new
> LibvirtStopCommandWrapper());
>
> final Hashtable<Class<? extends Command>, CommandWrapper>
> ekhoCommands = new Hashtable<Class<? extends Command>,
> CommandWrapper>(); linbvirtCommands.put(StopCommand.class, new
> EkhoStopCommandWrapper());
>
> resources.put(LibvirtComputingResource.class, linbvirtCommands);
> resources.put(EkhoComputingResource.class, ekhoCommands);
>
> But that is needed only if the StopCommand has a different
> behaviour for the EkhoComputingResource.
>
> Once a better version of the documentation is on the wiki, I will
> let you know.
>
> On other matters, I’m also adding unit tests for all the changes.
> We already went from 4% to 13.6% coverage in the KVM hypervisor
> plugin. The code I already refactored has 56% of coverage.
>
> You can see all the commits here:
> https://github.com/schubergphilis/cloudstack/tree/refactor/libvirt_r
>
>
es
>
>
> ource
>
> Cheers, Wilder
>
> On 23 Apr 2015, at 17:26, Marcus
> <sh...@gmail.com><mailto:shadowsor@gmai
l.com>>
> wrote:
>
> Great to see someone working on it. What sorts of roadblocks came
> out of reflection? How does the wrapper design solve the
> pluggability issue? This is pretty important to me, since I've
> worked with several companies now that end up subclassing
> LibvirtComputingResource in order to handle their own Commands on
> the hypervisor from their server-side plugins, and changing their
> 'resource' to that in agent.properties. Since the main agent class
> needs to be set at agent join, this is harder to manage than it
> should be.
>
> I mentioned the reflection model because that's how I tend to
> handle the commands when subclassing LibvirtComputingResource. I
> haven't had any problems with it, but then again I haven't tried to
> refactor 5500 lines into that model, either.
>
> On Thu, Apr 23, 2015 at 1:17 AM, Wilder Rodrigues
> <WR...@schubergphilis.com><m
ailto:WRodrigues@schubergphilis.com>
>
>
>
>
> wrote:
>
> Hi Marcus,
>
> I like the annotation idea, but reflection is trick because it
> hides some information about the code.
>
> Please, have a look at the CitrixResourceBase after the refactor I
> did. It became quite smaller and test coverage was improved.
>
> URL:
> https://cwiki.apache.org/confluence/display/CLOUDSTACK/Refactoring+X
>
>
en
>
>
> Server+Hypervisor+Plugin
>
> The same patter is being about to Libvirt stuff. The coverage on
> the KVM hypervisor plugin already went from 4 to 10.5% after
> refactoring 6 commands
>
> Cheers, Wilder
>
> On 22 Apr 2015, at 23:06, Marcus
> <sh...@gmail.com><mailto:shadowsor@gmai
l.com>>
> wrote:
>
> Kind of a tangent, but I'd actually like to see some work done to
> clean up LibvirtComputing resource. One model I've prototyped that
> seems to work is to create an annotation, such as
> 'KVMCommandExecutor', with a 'handles' property. With this
> annotation, you implement a class that handles, e.g. StartCommand,
> etc. Then in LibvirtComputingResource, the 'configure' method
> fetches all of these executors via reflection and stores them in an
> object. Then, instead of having all of the 'instanceof' lines in
> LibvirtComputingResource, the executeRequest method fetches the
> executor that handles the incoming command and runs it.
>
> I think this would break up LibvirtComputingResource into smaller,
> more testable and manageable chunks, and force things like config
> and utility methods to move to a more sane location, as well. As a
> bonus, this model makes things pluggable. Someone could ship KVM
> plugin code containing standalone command executors that are
> discovered at runtime for things they need to run at the hypervisor
> level.
>
> On Tue, Apr 21, 2015 at 6:27 AM, Wilder Rodrigues
> <WR...@schubergphilis.com><m
ailto:WRodrigues@schubergphilis.com>
>
>
>
>
> wrote:
>
> Hi all,
>
> Yesterday I started working on the LibvirtComputingResource class
> in order to apply the same patterns I used in the
> CitrixResourceBase + add more unit tests to it After 10 hours of
> work I got a bit stuck with the 1st test, which would cover the
> refactored LibvirtStopCommandWrapper. Why did I get stuck? The
> class used a few static methods that call native libraries, which I
> would like to mock. However, when writing the tests I faced
> problems with the current Mockito/PowerMock we are using: they are
> simply not enough for the task.
>
> What did I do then? I added a dependency to EasyMock and
> PowerMock-EasyMock API. It worked almost fine, but I had to add a
> “-noverify” to both my Eclipse Runtime configuration and also to
> the cloud-plugin-hypervisor-kvm/pom.xml file. I agree that’s not
> nice, but was my first attempt of getting it to work. After trying
> to first full build I faced more problems related to
> ClassDefNotFoundExpcetion which were complaining about Mockito
> classes. I then found out that adding the PowerMockRunner to all
> the tests classes was going to be a heavy burden and would also
> mess up future changes (e.g. the -noverify flag was removed from
> Java 8, thus adding it now would be a problem soon).
>
> Now that the first 2 paragraphs explain a bit about the problem,
> let’s get to the solution: Java 8
>
> The VerifyError that I was getting was due to the use of the latest
> EasyMock release (3.3.1). I tried to downgrade it to 3.1/3.2 but it
> also did not work. My decision: do not refactor if the proper tests
> cannot be added. This left me with one action: migrate to Java 8.
>
> There were mentions about Java 8 in february[1] and now I will put
> some energy in making it happen.
>
> What is your opinion on it?
>
> Thanks in advance.
>
> Cheers, Wilder
>
> http://mail-archives.apache.org/mod_mbox/cloudstack-dev/201502.mbox/
>
>
%3
>
>
> C54EEF6BE.5040401@shapeblue.com<mailto:C54EEF6BE.5040401@shapeblue.com
>%3E<http://mail-archives.apache.org/mod_m
>
>
box/cloudstack-dev/201502.mbox/<54EEF6BE.5040401@shapeblue.com<mailto:54
EEF6BE.5040401@shapeblue.com><mailto
> :54
>
>
> EEF6BE.5040401@shapeblue.com<ma...@shapeblue.com>>>>
>
> Regards, Rohit Yadav Software Architect, ShapeBlue
>
>
> [cid:9DD97B41-04C5-45F0-92A7-951F3E962F7A]
>
>
> M. +91 88 262 30892 |
> rohit.yadav@shapeblue.com<ma...@shapeblue.com><mailto:roh
it.yadav@shapeblue.com>
>
>
Blog: bhaisaab.org<http://bhaisaab.org><http://bhaisaab.org> | Twitter:
@_bhaisaab
>
>
>
>
> Find out more about ShapeBlue and our range of CloudStack related
> services
>
> IaaS Cloud Design &
> Build<http://shapeblue.com/iaas-cloud-design-and-build//> CSForge –
> rapid IaaS deployment framework<http://shapeblue.com/csforge/>
> CloudStack
> Consulting<http://shapeblue.com/cloudstack-consultancy/> CloudStack
> Software
> Engineering<http://shapeblue.com/cloudstack-software-engineering/>
>
>
> CloudStack Infrastructure
> Support<http://shapeblue.com/cloudstack-infrastructure-support/>
>
>
> CloudStack Bootcamp Training
> Courses<http://shapeblue.com/cloudstack-training/>
>
> This email and any attachments to it may be confidential and are
> intended solely for the use of the individual to whom it is
> addressed. Any views or opinions expressed are solely those of the
> author and do not necessarily represent those of Shape Blue Ltd or
> related companies. If you are not the intended recipient of this
> email, you must neither take any action based upon its contents,
> nor copy or show it to anyone. Please contact the sender if you
> believe you have received this email in error. Shape Blue Ltd is a
> company incorporated in England & Wales. ShapeBlue Services India
> LLP is a company incorporated in India and is operated under
> license from Shape Blue Ltd. Shape Blue Brasil Consultoria Ltda is
> a company incorporated in Brasil and is operated under license from
> Shape Blue Ltd. ShapeBlue SA Pty Ltd is a company registered by The
> Republic of South Africa and is traded under license from Shape
> Blue Ltd. ShapeBlue is a registered trademark.
>
>
>
> Find out more about ShapeBlue and our range of CloudStack related
> services
>
> IaaS Cloud Design &
> Build<http://shapeblue.com/iaas-cloud-design-and-build//> CSForge
> – rapid IaaS deployment framework<http://shapeblue.com/csforge/>
> CloudStack Consulting<http://shapeblue.com/cloudstack-consultancy/>
> CloudStack Software
> Engineering<http://shapeblue.com/cloudstack-software-engineering/>
> CloudStack Infrastructure
> Support<http://shapeblue.com/cloudstack-infrastructure-support/>
> CloudStack Bootcamp Training
> Courses<http://shapeblue.com/cloudstack-training/>
>
> This email and any attachments to it may be confidential and are
> intended solely for the use of the individual to whom it is
> addressed. Any views or opinions expressed are solely those of the
> author and do not necessarily represent those of Shape Blue Ltd or
> related companies. If you are not the intended recipient of this
> email, you must neither take any action based upon its contents,
> nor copy or show it to anyone. Please contact the sender if you
> believe you have received this email in error. Shape Blue Ltd is a
> company incorporated in England & Wales. ShapeBlue Services India
> LLP is a company incorporated in India and is operated under
> license from Shape Blue Ltd. Shape Blue Brasil Consultoria Ltda is
> a company incorporated in Brasil and is operated under license
> from Shape Blue Ltd. ShapeBlue SA Pty Ltd is a company registered
> by The Republic of South Africa and is traded under license from
> Shape Blue Ltd. ShapeBlue is a registered trademark.
>
>
>
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1
iQIcBAEBAgAGBQJVpkq9AAoJEAGbWC3bPspCG8cP/1LBR6uTNXhCzgpFH3BsGLRc
VO/T/2Co1hj2WTpQTdf2dCubYSHKghlnktkjeDYU1U0G5XNvgryaHPl5uIpKrQiS
Eb4k2nQzmB55/O50Gbs5HiCsnCJ6t62eDUZs7LMDj5sQaOy4A/7V+sc4Kozt2bAi
GKbDRxAL294A49BHXH4w58KthN2LJ1P7O9584rB/G1HP9bGn24kxHy3e0OGKmA+2
dtR5apGYKjv0DlqMX2gwJqFxidWko17u9ievddCgZmJLVgx1wnmTPh04ctEq4/Du
EnTF4fmc8pDIPX5YSGbz3SdY0uGHxTPrYzdeITlNVQSLaAJZ4rYQm1olaJ+ptkiN
hDH45J9vZfw2bkIFIk3rf/dlKDgrvgqFhWXsL2SmSg8EYWD4/bEVWhLZP2V0qZDk
YGxBMmkbN1Y+m5U3uO8WnpkjuUstvOtZMixChK/8PaPlp8c4Ay1uE5Z2FlPafshg
UmXALmxI8l6iVxhYeH1ZMCQ908qYnzPWac4EkK7pdy+mAxvvtXUzuyc+/sn6UaYb
cwTKYWoDo727UrUZUFnZN8qjmpmfLf8mUDX3AxC8KjevKIkLeo5R1+yo2TjmbeY5
jQR/kq3Hgf4R51iKsWE8q6US1AFuB72FPrnpy7aklmzKp+Tw5LOCkB2esLlRT44y
pCL4oUi71+SiLmuE8q4W
=Ufo9
-----END PGP SIGNATURE-----
Re: [DISCUSS] Moving to Java 8
Posted by Wilder Rodrigues <WR...@schubergphilis.com>.
If there would be dependencies on some other things, that in no way could be fixed now, we could wait for 4.7 (5.0). However, if we could give it a go, I would be able to tackle this in our next Sprint (within 1 1/2 week from now) and still get it into 4.6.
What would be the main considerations?
Cheers,
Wilder
On 14 Jul 2015, at 22:25, Wido den Hollander <wi...@widodh.nl>> wrote:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 07/14/2015 10:18 PM, John Burwell wrote:
Wido,
Is the OpenJDK PPA [1] not acceptable? Since Java7 is no longer
supported, we run the risk of an Java security issue affecting the
project that won’t be fixed.
I didn't know that a PPA with OpenJDk existed. Looking at it I see
that the maintainer Matthias Klose works for Canonical, so it seems
like an official PPA.
Still, having users adding PPAs is something we want to prevent as
much as possible, but I do recognize the problem here.
Ubuntu 16.04 is less then a year away and will have Java 8 support, so
that will be resolved by then, but for now it is a problem.
I think that CloudStack 4.6 is to early, but maybe 4.7 (called 5.0?)
is a good time to make the move?
Wido
Thanks, -John
[1]: https://launchpad.net/~openjdk-r/+archive/ubuntu/ppa
--- John Burwell (@john_burwell) VP of Software Engineering,
ShapeBlue (571) 403-2411 | +44 20 3603 0542
http://www.shapeblue.com Please join us at CloudStack Collab EU —
http://events.linuxfoundation.org/events/cloudstack-collaboration-conf
erence-europe
On Jul 10, 2015, at 5:47 PM, Wido den Hollander <wi...@widodh.nl>>
wrote:
Signed PGP part
On 07/10/2015 09:22 PM, Rohit Yadav wrote:
Ping Wilder - any progress/plan on moving forward (perhaps
after 4.6?).
I don't think there is? Since Ubuntu 14.04 doesn't support Java 8
in any package form we can't really continue.
Ubuntu 16.04 will ship with Java 8 and that will be the next
LTS.
Wido
On 01-May-2015, at 4:07 pm, Wilder Rodrigues
<WR...@schubergphilis.com>
wrote:
Hi Marcus,
Sorry for the push, I think after doing the whole
CitrixResourceBase refactor I also got a bit attached to the
whole thing/solution. ;)
Thanks for the input you gave. I will finish the refactor and
apply it to both implementations.
Cheers, Wilder
On 01 May 2015, at 09:06, Marcus
<sh...@gmail.com><mailto:shadowsor@gm
ai
l.com<http://l.com>>>
wrote:
Oh, and of course the annotation added to the wrapper looks
like:
...
@ResourceWrapper(handles = CheckHealthCommand.class)
public final class LibvirtCheckHealthCommandWrapper
...
maybe 'wraps' or 'wrapperfor' would be better than 'handles'
in your naming scheme. You get the idea.
On Thu, Apr 30, 2015 at 11:59 PM, Marcus
<sh...@gmail.com>> wrote: I
agree, this wrapper is a good step forward. It's totally fine
to continue on that path because it is obviously better and
makes it easy to switch to autodetection anytime later by
simply adding the annotation. Sorry if I got a bit passionate
about that, but as you mention I also get tired of adding
things in multiple places, and the annotations have worked well
in the API and provide a good model to emulate for
consistency.
I can't share code, because these extensions to
LibvirtComputingResource that I've provided for other
companies have not been open sourced. I can speak more
generically though about methods.
To answer question "a", reflection allows you to do something
like:
Reflections reflections = new
Reflections("com.cloud.hypervisor.kvm.resource.wrapper");
Set<Class<? extends CommandWrapper>> wrappers =
reflections.getSubTypesOf(CommandWrapper.class);
So here in "new Reflections" we are automatically filtering
for just the wrappers that would apply to the KVM plugin. Then
to finish it off, you iterate through the wrappers and do:
ResourceWrapper annotation =
wrapper.getAnnotation(ResourceWrapper.class);
citrixCommands.put(annotation.handles(),
wrapper.newInstance());
Sorry, I guess that's four lines, plus the relevant for loop.
And probably a null check or something for the annotation. You
also have to add the annotation class itself, and add a line
for the annotation in each wrapper, but in the end when we add
new Commands, we won't have to touch anything but the new class
that handles the command.
public @interface ResourceWrapper {
Class<? extends Command> handles();
}
There's an example of something similar to this in
KVMStoragePoolManager.java (annotation is
StoragePoolInfo.java). This example has actually been adapted
from that. Also to a lesser extent in the API server, but it is
spread across a bunch of classes.
On Thu, Apr 30, 2015 at 10:41 PM, Wilder Rodrigues
<WR...@schubergphilis.com>
wrote: Hi Marcus,
Thanks for the email… I’m always in for improvements. But why
can’t you share the code?
Few points below:
1. I added an subclassing example of LibvirtComputingResource
because you mentioned it in a previous email:
On 23 Apr 2015, at 17:26, Marcus
<sh...@gmail.com>> wrote:
I mentioned the reflection model because that's how I tend to
handle the commands when subclassing LibvirtComputingResource.
2. Current situation with LibvirtComputingResource on Master
is:
a. 67 IFs b. 67 private/protected methods that are used only
there c. If a new Command is added it means we will have a new
IF and a new private method e. Maintenance is hell, test is
close to zero and code quality is below expectations
That being said, the main idea with the refactor is to change
structure only, not behaviour. So what I’m doing is to simply
move the code out the LibvirtCompRes and write tests for it,
keeping the behaviour the same - to be done in a next phase. If
you look at the changes you will see that some wrappers are
already 100% covered. However, some others have 4% or 8% (not
that much though). I would like to refactor that as well, but
that could change behaviour (mentioned above) which I don’t
want to touch now.
3. With the new situation:
a. No IFs b. All methods wrapped by other classes (command
wrappers) - loosely coupled, easier to test and maintain c. If
a new Command is added we would have to add a command wrapper
and 1 line in the request wrapper implementation ( I know, it
hurts you a bit) - but please bear with me for the good news.
4. the warnings are due to that: Hashtable<Class<? extends
Command>, CommandWrapper>()
No big deal.
As I understood from your first paragraph we would have to
annotated the commands classes, right? I mean, all of them.
That’s something I wouldn’t do in this phase, to be honest. It
might seem harmless to do, but I like to break things down a
bit and have more isolation in my changes.
What’s next: I will finish the refactor with the request
wrapper as it is. For me it is no problem do add the lines now
and remove them in 1 week. Most of the work is concentrated in
the tests, which I’m trying as hard as I can to get them in the
best way possible. Once it’s done and pushed to master, I will
analyse what we would need to apply the annotation.
But before I go to bring the kids to school, just one
question:
a. The “handle” value, in the annotation, would have the
wrapper class that would be used for that command, right? Now
let’s get 1 command as example: CheckHealthCommand. Its wrapper
implementation differs per hypervisor (just like all the other
wrapper commands do). I’m not taking the time to really think
about it now, but how would we annotated the different wrappers
per command?
Thanks again for your time.
Cheers, Wilder
On 30 Apr 2015, at 22:52, Marcus
<sh...@gmail.com>> wrote:
Ok. I wish I could share some code, because it isn't really as
big of a deal as it sounds from your reasoning. It is literally
just 3 lines on startup that fetch anything with the
'@AgentExecutor' annotation and stores it in a hash whose key
is the value from @AgentExecutor's 'handles' property. Then
when a *Command comes it it is passed to the appropriate
Executor class.
Looking at CitrixRequestWrapper, the 3 lines I mention are
almost identical in function to your init method, just that it
uses the annotation to find all of the commands, rather than
hardcoding them. We use the same annotation design for the api
side of the code on the management server, which allows the api
commands to be easier to write and self-contained (you don't
have to update other code to add a new api call). It makes
things easier for novice developers.
This implementation is no less typesafe than the previous
design (the one with all of the instanceof). It didn't require
any casting or warning suppression, either, as the wrapper
does.
Extending LibvirtComputingResource is not ideal, and doesn't
work if multiple third parties are involved. Granted, there
hasn't been a lot of demand for this, nevertheless it's
particularly important for KVM, where the Command classes are
executed on the hypervisor it's not really feasible to just
dump the code in your management server-side plugin like some
plugins do.
In reviewing the code, the two implementations are really very
close. If you just updated init to fetch the wrappers based on
either an annotation or the class they extend, or something
along those lines so this method doesn't have to be edited
every time a command is added, that would be more or less the
same thing. The the KVM agent would be pluggable like the
management server side is.
On Thu, Apr 30, 2015 at 12:55 PM, Wilder Rodrigues
<WR...@schubergphilis.com>
wrote: Hi Marcus,
Apologies for taking so much time to reply to your email, but
was, and still am, quite busy. :)
I would only use reflection if that was the only way to do it.
The use of reflection usually makes the code more complex,
which is not good when we have java developers in all different
levels (from jr. do sr) working with cloudstack. It also makes
us lose the type safety, which might also harm the exception
handling if not done well. In addition, if we need to refactor
something, the IDE is no longer going to do few things because
the refection code cannot be found.
If someone will need to extend the LibvirtComputingResource
that would be no problem with the approach I’m using. The
CitrixResourceBase also has quite few sub-classes and it works
just fine.
I will document on the wiki page how it should be done when
sub-classing the LibvirtComputingResource class.
In a quick note/snippet, one would do:
public class EkhoComputingResource extends
LibvirtComputingResource {
@Override public Answer executeRequest(final Command cmd) {
final LibvirtRequestWrapper wrapper =
LibvirtRequestWrapper.getInstance(); try { return
wrapper.execute(cmd, this); } catch (final Exception e) {
return Answer.createUnsupportedCommandAnswer(cmd); } } }
In the flyweight where I keep the wrapper we could have ():
final Hashtable<Class<? extends Command>, CommandWrapper>
linbvirtCommands = new Hashtable<Class<? extends Command>,
CommandWrapper>(); linbvirtCommands.put(StopCommand.class, new
LibvirtStopCommandWrapper());
final Hashtable<Class<? extends Command>, CommandWrapper>
ekhoCommands = new Hashtable<Class<? extends Command>,
CommandWrapper>(); linbvirtCommands.put(StopCommand.class, new
EkhoStopCommandWrapper());
resources.put(LibvirtComputingResource.class,
linbvirtCommands); resources.put(EkhoComputingResource.class,
ekhoCommands);
But that is needed only if the StopCommand has a different
behaviour for the EkhoComputingResource.
Once a better version of the documentation is on the wiki, I
will let you know.
On other matters, I’m also adding unit tests for all the
changes. We already went from 4% to 13.6% coverage in the KVM
hypervisor plugin. The code I already refactored has 56% of
coverage.
You can see all the commits here:
https://github.com/schubergphilis/cloudstack/tree/refactor/libvirt_r
es
ource
Cheers, Wilder
On 23 Apr 2015, at 17:26, Marcus
<sh...@gmail.com>> wrote:
Great to see someone working on it. What sorts of roadblocks
came out of reflection? How does the wrapper design solve the
pluggability issue? This is pretty important to me, since I've
worked with several companies now that end up subclassing
LibvirtComputingResource in order to handle their own Commands
on the hypervisor from their server-side plugins, and changing
their 'resource' to that in agent.properties. Since the main
agent class needs to be set at agent join, this is harder to
manage than it should be.
I mentioned the reflection model because that's how I tend to
handle the commands when subclassing LibvirtComputingResource.
I haven't had any problems with it, but then again I haven't
tried to refactor 5500 lines into that model, either.
On Thu, Apr 23, 2015 at 1:17 AM, Wilder Rodrigues
<WR...@schubergphilis.com>
wrote:
Hi Marcus,
I like the annotation idea, but reflection is trick because it
hides some information about the code.
Please, have a look at the CitrixResourceBase after the
refactor I did. It became quite smaller and test coverage was
improved.
URL:
https://cwiki.apache.org/confluence/display/CLOUDSTACK/Refactoring+X
en
Server+Hypervisor+Plugin
The same patter is being about to Libvirt stuff. The coverage
on the KVM hypervisor plugin already went from 4 to 10.5%
after refactoring 6 commands
Cheers, Wilder
On 22 Apr 2015, at 23:06, Marcus
<sh...@gmail.com>> wrote:
Kind of a tangent, but I'd actually like to see some work done
to clean up LibvirtComputing resource. One model I've
prototyped that seems to work is to create an annotation, such
as 'KVMCommandExecutor', with a 'handles' property. With this
annotation, you implement a class that handles, e.g.
StartCommand, etc. Then in LibvirtComputingResource, the
'configure' method fetches all of these executors via
reflection and stores them in an object. Then, instead of
having all of the 'instanceof' lines in
LibvirtComputingResource, the executeRequest method fetches
the executor that handles the incoming command and runs it.
I think this would break up LibvirtComputingResource into
smaller, more testable and manageable chunks, and force things
like config and utility methods to move to a more sane
location, as well. As a bonus, this model makes things
pluggable. Someone could ship KVM plugin code containing
standalone command executors that are discovered at runtime for
things they need to run at the hypervisor level.
On Tue, Apr 21, 2015 at 6:27 AM, Wilder Rodrigues
<WR...@schubergphilis.com>
wrote:
Hi all,
Yesterday I started working on the LibvirtComputingResource
class in order to apply the same patterns I used in the
CitrixResourceBase + add more unit tests to it After 10 hours
of work I got a bit stuck with the 1st test, which would cover
the refactored LibvirtStopCommandWrapper. Why did I get stuck?
The class used a few static methods that call native libraries,
which I would like to mock. However, when writing the tests I
faced problems with the current Mockito/PowerMock we are using:
they are simply not enough for the task.
What did I do then? I added a dependency to EasyMock and
PowerMock-EasyMock API. It worked almost fine, but I had to add
a “-noverify” to both my Eclipse Runtime configuration and also
to the cloud-plugin-hypervisor-kvm/pom.xml file. I agree that’s
not nice, but was my first attempt of getting it to work. After
trying to first full build I faced more problems related to
ClassDefNotFoundExpcetion which were complaining about Mockito
classes. I then found out that adding the PowerMockRunner to
all the tests classes was going to be a heavy burden and would
also mess up future changes (e.g. the -noverify flag was
removed from Java 8, thus adding it now would be a problem
soon).
Now that the first 2 paragraphs explain a bit about the
problem, let’s get to the solution: Java 8
The VerifyError that I was getting was due to the use of the
latest EasyMock release (3.3.1). I tried to downgrade it to
3.1/3.2 but it also did not work. My decision: do not refactor
if the proper tests cannot be added. This left me with one
action: migrate to Java 8.
There were mentions about Java 8 in february[1] and now I will
put some energy in making it happen.
What is your opinion on it?
Thanks in advance.
Cheers, Wilder
http://mail-archives.apache.org/mod_mbox/cloudstack-dev/201502.mbox/
%3
C54EEF6BE.5040401@shapeblue.com<ma...@shapeblue.com>%3E<http://mail-archives.apache.org/mod_m
box/cloudstack-dev/201502.mbox/<54...@shapeblue.com><mailto
:54
EEF6BE.5040401@shapeblue.com<ma...@shapeblue.com>>>>
Regards, Rohit Yadav Software Architect, ShapeBlue
[cid:9DD97B41-04C5-45F0-92A7-951F3E962F7A]
M. +91 88 262 30892 |
rohit.yadav@shapeblue.com<ma...@shapeblue.com>
Blog: bhaisaab.org<http://bhaisaab.org><http://bhaisaab.org> | Twitter: @_bhaisaab
Find out more about ShapeBlue and our range of CloudStack
related services
IaaS Cloud Design &
Build<http://shapeblue.com/iaas-cloud-design-and-build//>
CSForge – rapid IaaS deployment
framework<http://shapeblue.com/csforge/> CloudStack
Consulting<http://shapeblue.com/cloudstack-consultancy/>
CloudStack Software
Engineering<http://shapeblue.com/cloudstack-software-engineering/>
CloudStack Infrastructure
Support<http://shapeblue.com/cloudstack-infrastructure-support/>
CloudStack Bootcamp Training
Courses<http://shapeblue.com/cloudstack-training/>
This email and any attachments to it may be confidential and
are intended solely for the use of the individual to whom it
is addressed. Any views or opinions expressed are solely those
of the author and do not necessarily represent those of Shape
Blue Ltd or related companies. If you are not the intended
recipient of this email, you must neither take any action based
upon its contents, nor copy or show it to anyone. Please
contact the sender if you believe you have received this email
in error. Shape Blue Ltd is a company incorporated in England &
Wales. ShapeBlue Services India LLP is a company incorporated
in India and is operated under license from Shape Blue Ltd.
Shape Blue Brasil Consultoria Ltda is a company incorporated in
Brasil and is operated under license from Shape Blue Ltd.
ShapeBlue SA Pty Ltd is a company registered by The Republic of
South Africa and is traded under license from Shape Blue Ltd.
ShapeBlue is a registered trademark.
Find out more about ShapeBlue and our range of CloudStack related
services
IaaS Cloud Design &
Build<http://shapeblue.com/iaas-cloud-design-and-build//> CSForge –
rapid IaaS deployment framework<http://shapeblue.com/csforge/>
CloudStack
Consulting<http://shapeblue.com/cloudstack-consultancy/> CloudStack
Software
Engineering<http://shapeblue.com/cloudstack-software-engineering/>
CloudStack Infrastructure
Support<http://shapeblue.com/cloudstack-infrastructure-support/>
CloudStack Bootcamp Training
Courses<http://shapeblue.com/cloudstack-training/>
This email and any attachments to it may be confidential and are
intended solely for the use of the individual to whom it is
addressed. Any views or opinions expressed are solely those of the
author and do not necessarily represent those of Shape Blue Ltd or
related companies. If you are not the intended recipient of this
email, you must neither take any action based upon its contents,
nor copy or show it to anyone. Please contact the sender if you
believe you have received this email in error. Shape Blue Ltd is a
company incorporated in England & Wales. ShapeBlue Services India
LLP is a company incorporated in India and is operated under
license from Shape Blue Ltd. Shape Blue Brasil Consultoria Ltda is
a company incorporated in Brasil and is operated under license from
Shape Blue Ltd. ShapeBlue SA Pty Ltd is a company registered by The
Republic of South Africa and is traded under license from Shape
Blue Ltd. ShapeBlue is a registered trademark.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1
iQIcBAEBAgAGBQJVpXBRAAoJEAGbWC3bPspCsEMP/0q8vUvG89JAtrOqBRF3s6Jh
T7pSSFGds+D3/WLKI5/qwt9ep92CmFbTOLoF6J5+33kFgePUTWtCHsXJJSH00Iyc
XbYG6YYYSxFa2sj208hkMlResKzLUMUAGTdSosy8S2sdV6Bv3CY6eoo1XrnmOPIP
gi7VbXauVRPQNoqoTKhwVWYRt2aH3cOxnfyojYcBmc9wPq2DjsZbwcasBDLilyF/
ezAoLB4PO7/Nf1iKl/oIakjUep7xgba7TQNdP/i5ALeLRkC4fRHDCFRrw3FqZkhX
i7oPND9yZ8KrEJi1i/hvhTgFQm/r+gktu12ydfbGl0C3auaBgYsOVS92EIRTtFae
Q1zh9E51mVUoKfSUp92rff0d7wrHI7HEJ1qeciuc4TVGkvgShWnJzGTft2RiVRuI
ps1jQof8K3ae4tnjbxz4tfMIn7/ZTlZcklsSisGTrzhGzF802368TWL/ePR51s56
VLCEJZsHyF9PUUVf/3eRLkqk/6bABRfjqvjS8VHHbfVp0QGnYaqe4621FUnTC+lB
P077F/bvXhNlOHiPMGs4etcM+xq7aXXMPdXeLxdCsjmsENrvStApUQ07uRneX5a9
eU6BAXH0kEBLJzhXmOjbsCk378c/d4FxslgHySiYrt/C9NUpY0bakZpjYFemw12E
+PFfUwbg/Lq8UvaS8fWq
=nJKK
-----END PGP SIGNATURE-----
Re: [DISCUSS] Moving to Java 8
Posted by Wido den Hollander <wi...@widodh.nl>.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 07/14/2015 10:18 PM, John Burwell wrote:
> Wido,
>
> Is the OpenJDK PPA [1] not acceptable? Since Java7 is no longer
> supported, we run the risk of an Java security issue affecting the
> project that won’t be fixed.
>
I didn't know that a PPA with OpenJDk existed. Looking at it I see
that the maintainer Matthias Klose works for Canonical, so it seems
like an official PPA.
Still, having users adding PPAs is something we want to prevent as
much as possible, but I do recognize the problem here.
Ubuntu 16.04 is less then a year away and will have Java 8 support, so
that will be resolved by then, but for now it is a problem.
I think that CloudStack 4.6 is to early, but maybe 4.7 (called 5.0?)
is a good time to make the move?
Wido
> Thanks, -John
>
> [1]: https://launchpad.net/~openjdk-r/+archive/ubuntu/ppa
>
> --- John Burwell (@john_burwell) VP of Software Engineering,
> ShapeBlue (571) 403-2411 | +44 20 3603 0542
> http://www.shapeblue.com Please join us at CloudStack Collab EU —
> http://events.linuxfoundation.org/events/cloudstack-collaboration-conf
erence-europe
>
>
>
>
>
>> On Jul 10, 2015, at 5:47 PM, Wido den Hollander <wi...@widodh.nl>
>> wrote:
>>
>> Signed PGP part
>>
>>
>> On 07/10/2015 09:22 PM, Rohit Yadav wrote:
>>> Ping Wilder - any progress/plan on moving forward (perhaps
>>> after 4.6?).
>>>
>>
>> I don't think there is? Since Ubuntu 14.04 doesn't support Java 8
>> in any package form we can't really continue.
>>
>> Ubuntu 16.04 will ship with Java 8 and that will be the next
>> LTS.
>>
>> Wido
>>
>>> On 01-May-2015, at 4:07 pm, Wilder Rodrigues
>>> <WR...@schubergphilis.com>
>
>>>
>>>
wrote:
>>>
>>> Hi Marcus,
>>>
>>> Sorry for the push, I think after doing the whole
>>> CitrixResourceBase refactor I also got a bit attached to the
>>> whole thing/solution. ;)
>>>
>>> Thanks for the input you gave. I will finish the refactor and
>>> apply it to both implementations.
>>>
>>> Cheers, Wilder
>>>
>>>
>>> On 01 May 2015, at 09:06, Marcus
>>> <sh...@gmail.com><mailto:shadowsor@gm
ai
>>
>>>
l.com>>
>>> wrote:
>>>
>>> Oh, and of course the annotation added to the wrapper looks
>>> like:
>>>
>>>
>>> ...
>>>
>>> @ResourceWrapper(handles = CheckHealthCommand.class)
>>>
>>> public final class LibvirtCheckHealthCommandWrapper
>>>
>>> ...
>>>
>>>
>>> maybe 'wraps' or 'wrapperfor' would be better than 'handles'
>>> in your naming scheme. You get the idea.
>>>
>>> On Thu, Apr 30, 2015 at 11:59 PM, Marcus
>>> <sh...@gmail.com>> wrote: I
>>> agree, this wrapper is a good step forward. It's totally fine
>>> to continue on that path because it is obviously better and
>>> makes it easy to switch to autodetection anytime later by
>>> simply adding the annotation. Sorry if I got a bit passionate
>>> about that, but as you mention I also get tired of adding
>>> things in multiple places, and the annotations have worked well
>>> in the API and provide a good model to emulate for
>>> consistency.
>>>
>>> I can't share code, because these extensions to
>>> LibvirtComputingResource that I've provided for other
>>> companies have not been open sourced. I can speak more
>>> generically though about methods.
>>>
>>> To answer question "a", reflection allows you to do something
>>> like:
>>>
>>> Reflections reflections = new
>>> Reflections("com.cloud.hypervisor.kvm.resource.wrapper");
>>> Set<Class<? extends CommandWrapper>> wrappers =
>>> reflections.getSubTypesOf(CommandWrapper.class);
>>>
>>> So here in "new Reflections" we are automatically filtering
>>> for just the wrappers that would apply to the KVM plugin. Then
>>> to finish it off, you iterate through the wrappers and do:
>>>
>>> ResourceWrapper annotation =
>>> wrapper.getAnnotation(ResourceWrapper.class);
>>> citrixCommands.put(annotation.handles(),
>>> wrapper.newInstance());
>>>
>>> Sorry, I guess that's four lines, plus the relevant for loop.
>>> And probably a null check or something for the annotation. You
>>> also have to add the annotation class itself, and add a line
>>> for the annotation in each wrapper, but in the end when we add
>>> new Commands, we won't have to touch anything but the new class
>>> that handles the command.
>>>
>>>
>>> public @interface ResourceWrapper {
>>>
>>> Class<? extends Command> handles();
>>>
>>> }
>>>
>>> There's an example of something similar to this in
>>> KVMStoragePoolManager.java (annotation is
>>> StoragePoolInfo.java). This example has actually been adapted
>>> from that. Also to a lesser extent in the API server, but it is
>>> spread across a bunch of classes.
>>>
>>> On Thu, Apr 30, 2015 at 10:41 PM, Wilder Rodrigues
>>> <WR...@schubergphilis.com>
>
>>>
>>>
wrote: Hi Marcus,
>>>
>>> Thanks for the email… I’m always in for improvements. But why
>>> can’t you share the code?
>>>
>>> Few points below:
>>>
>>> 1. I added an subclassing example of LibvirtComputingResource
>>> because you mentioned it in a previous email:
>>>
>>> On 23 Apr 2015, at 17:26, Marcus
>>> <sh...@gmail.com>> wrote:
>>>
>>> I mentioned the reflection model because that's how I tend to
>>> handle the commands when subclassing LibvirtComputingResource.
>>>
>>> 2. Current situation with LibvirtComputingResource on Master
>>> is:
>>>
>>> a. 67 IFs b. 67 private/protected methods that are used only
>>> there c. If a new Command is added it means we will have a new
>>> IF and a new private method e. Maintenance is hell, test is
>>> close to zero and code quality is below expectations
>>>
>>> That being said, the main idea with the refactor is to change
>>> structure only, not behaviour. So what I’m doing is to simply
>>> move the code out the LibvirtCompRes and write tests for it,
>>> keeping the behaviour the same - to be done in a next phase. If
>>> you look at the changes you will see that some wrappers are
>>> already 100% covered. However, some others have 4% or 8% (not
>>> that much though). I would like to refactor that as well, but
>>> that could change behaviour (mentioned above) which I don’t
>>> want to touch now.
>>>
>>> 3. With the new situation:
>>>
>>> a. No IFs b. All methods wrapped by other classes (command
>>> wrappers) - loosely coupled, easier to test and maintain c. If
>>> a new Command is added we would have to add a command wrapper
>>> and 1 line in the request wrapper implementation ( I know, it
>>> hurts you a bit) - but please bear with me for the good news.
>>>
>>> 4. the warnings are due to that: Hashtable<Class<? extends
>>> Command>, CommandWrapper>()
>>>
>>> No big deal.
>>>
>>> As I understood from your first paragraph we would have to
>>> annotated the commands classes, right? I mean, all of them.
>>>
>>> That’s something I wouldn’t do in this phase, to be honest. It
>>> might seem harmless to do, but I like to break things down a
>>> bit and have more isolation in my changes.
>>>
>>> What’s next: I will finish the refactor with the request
>>> wrapper as it is. For me it is no problem do add the lines now
>>> and remove them in 1 week. Most of the work is concentrated in
>>> the tests, which I’m trying as hard as I can to get them in the
>>> best way possible. Once it’s done and pushed to master, I will
>>> analyse what we would need to apply the annotation.
>>>
>>> But before I go to bring the kids to school, just one
>>> question:
>>>
>>> a. The “handle” value, in the annotation, would have the
>>> wrapper class that would be used for that command, right? Now
>>> let’s get 1 command as example: CheckHealthCommand. Its wrapper
>>> implementation differs per hypervisor (just like all the other
>>> wrapper commands do). I’m not taking the time to really think
>>> about it now, but how would we annotated the different wrappers
>>> per command?
>>>
>>> Thanks again for your time.
>>>
>>> Cheers, Wilder
>>>
>>>
>>> On 30 Apr 2015, at 22:52, Marcus
>>> <sh...@gmail.com>> wrote:
>>>
>>> Ok. I wish I could share some code, because it isn't really as
>>> big of a deal as it sounds from your reasoning. It is literally
>>> just 3 lines on startup that fetch anything with the
>>> '@AgentExecutor' annotation and stores it in a hash whose key
>>> is the value from @AgentExecutor's 'handles' property. Then
>>> when a *Command comes it it is passed to the appropriate
>>> Executor class.
>>>
>>> Looking at CitrixRequestWrapper, the 3 lines I mention are
>>> almost identical in function to your init method, just that it
>>> uses the annotation to find all of the commands, rather than
>>> hardcoding them. We use the same annotation design for the api
>>> side of the code on the management server, which allows the api
>>> commands to be easier to write and self-contained (you don't
>>> have to update other code to add a new api call). It makes
>>> things easier for novice developers.
>>>
>>> This implementation is no less typesafe than the previous
>>> design (the one with all of the instanceof). It didn't require
>>> any casting or warning suppression, either, as the wrapper
>>> does.
>>>
>>> Extending LibvirtComputingResource is not ideal, and doesn't
>>> work if multiple third parties are involved. Granted, there
>>> hasn't been a lot of demand for this, nevertheless it's
>>> particularly important for KVM, where the Command classes are
>>> executed on the hypervisor it's not really feasible to just
>>> dump the code in your management server-side plugin like some
>>> plugins do.
>>>
>>> In reviewing the code, the two implementations are really very
>>> close. If you just updated init to fetch the wrappers based on
>>> either an annotation or the class they extend, or something
>>> along those lines so this method doesn't have to be edited
>>> every time a command is added, that would be more or less the
>>> same thing. The the KVM agent would be pluggable like the
>>> management server side is.
>>>
>>> On Thu, Apr 30, 2015 at 12:55 PM, Wilder Rodrigues
>>> <WR...@schubergphilis.com>
>
>>>
>>>
wrote: Hi Marcus,
>>>
>>> Apologies for taking so much time to reply to your email, but
>>> was, and still am, quite busy. :)
>>>
>>> I would only use reflection if that was the only way to do it.
>>> The use of reflection usually makes the code more complex,
>>> which is not good when we have java developers in all different
>>> levels (from jr. do sr) working with cloudstack. It also makes
>>> us lose the type safety, which might also harm the exception
>>> handling if not done well. In addition, if we need to refactor
>>> something, the IDE is no longer going to do few things because
>>> the refection code cannot be found.
>>>
>>> If someone will need to extend the LibvirtComputingResource
>>> that would be no problem with the approach I’m using. The
>>> CitrixResourceBase also has quite few sub-classes and it works
>>> just fine.
>>>
>>> I will document on the wiki page how it should be done when
>>> sub-classing the LibvirtComputingResource class.
>>>
>>> In a quick note/snippet, one would do:
>>>
>>> public class EkhoComputingResource extends
>>> LibvirtComputingResource {
>>>
>>> @Override public Answer executeRequest(final Command cmd) {
>>>
>>> final LibvirtRequestWrapper wrapper =
>>> LibvirtRequestWrapper.getInstance(); try { return
>>> wrapper.execute(cmd, this); } catch (final Exception e) {
>>> return Answer.createUnsupportedCommandAnswer(cmd); } } }
>>>
>>>
>>> In the flyweight where I keep the wrapper we could have ():
>>>
>>> final Hashtable<Class<? extends Command>, CommandWrapper>
>>> linbvirtCommands = new Hashtable<Class<? extends Command>,
>>> CommandWrapper>(); linbvirtCommands.put(StopCommand.class, new
>>> LibvirtStopCommandWrapper());
>>>
>>> final Hashtable<Class<? extends Command>, CommandWrapper>
>>> ekhoCommands = new Hashtable<Class<? extends Command>,
>>> CommandWrapper>(); linbvirtCommands.put(StopCommand.class, new
>>> EkhoStopCommandWrapper());
>>>
>>> resources.put(LibvirtComputingResource.class,
>>> linbvirtCommands); resources.put(EkhoComputingResource.class,
>>> ekhoCommands);
>>>
>>> But that is needed only if the StopCommand has a different
>>> behaviour for the EkhoComputingResource.
>>>
>>> Once a better version of the documentation is on the wiki, I
>>> will let you know.
>>>
>>> On other matters, I’m also adding unit tests for all the
>>> changes. We already went from 4% to 13.6% coverage in the KVM
>>> hypervisor plugin. The code I already refactored has 56% of
>>> coverage.
>>>
>>> You can see all the commits here:
>>> https://github.com/schubergphilis/cloudstack/tree/refactor/libvirt_r
es
>>
>>>
ource
>>>
>>> Cheers, Wilder
>>>
>>> On 23 Apr 2015, at 17:26, Marcus
>>> <sh...@gmail.com>> wrote:
>>>
>>> Great to see someone working on it. What sorts of roadblocks
>>> came out of reflection? How does the wrapper design solve the
>>> pluggability issue? This is pretty important to me, since I've
>>> worked with several companies now that end up subclassing
>>> LibvirtComputingResource in order to handle their own Commands
>>> on the hypervisor from their server-side plugins, and changing
>>> their 'resource' to that in agent.properties. Since the main
>>> agent class needs to be set at agent join, this is harder to
>>> manage than it should be.
>>>
>>> I mentioned the reflection model because that's how I tend to
>>> handle the commands when subclassing LibvirtComputingResource.
>>> I haven't had any problems with it, but then again I haven't
>>> tried to refactor 5500 lines into that model, either.
>>>
>>> On Thu, Apr 23, 2015 at 1:17 AM, Wilder Rodrigues
>>> <WR...@schubergphilis.com>
>
>>>
>>>
wrote:
>>>
>>> Hi Marcus,
>>>
>>> I like the annotation idea, but reflection is trick because it
>>> hides some information about the code.
>>>
>>> Please, have a look at the CitrixResourceBase after the
>>> refactor I did. It became quite smaller and test coverage was
>>> improved.
>>>
>>> URL:
>>> https://cwiki.apache.org/confluence/display/CLOUDSTACK/Refactoring+X
en
>>
>>>
Server+Hypervisor+Plugin
>>>
>>> The same patter is being about to Libvirt stuff. The coverage
>>> on the KVM hypervisor plugin already went from 4 to 10.5%
>>> after refactoring 6 commands
>>>
>>> Cheers, Wilder
>>>
>>> On 22 Apr 2015, at 23:06, Marcus
>>> <sh...@gmail.com>> wrote:
>>>
>>> Kind of a tangent, but I'd actually like to see some work done
>>> to clean up LibvirtComputing resource. One model I've
>>> prototyped that seems to work is to create an annotation, such
>>> as 'KVMCommandExecutor', with a 'handles' property. With this
>>> annotation, you implement a class that handles, e.g.
>>> StartCommand, etc. Then in LibvirtComputingResource, the
>>> 'configure' method fetches all of these executors via
>>> reflection and stores them in an object. Then, instead of
>>> having all of the 'instanceof' lines in
>>> LibvirtComputingResource, the executeRequest method fetches
>>> the executor that handles the incoming command and runs it.
>>>
>>> I think this would break up LibvirtComputingResource into
>>> smaller, more testable and manageable chunks, and force things
>>> like config and utility methods to move to a more sane
>>> location, as well. As a bonus, this model makes things
>>> pluggable. Someone could ship KVM plugin code containing
>>> standalone command executors that are discovered at runtime for
>>> things they need to run at the hypervisor level.
>>>
>>> On Tue, Apr 21, 2015 at 6:27 AM, Wilder Rodrigues
>>> <WR...@schubergphilis.com>
>
>>>
>>>
wrote:
>>>
>>> Hi all,
>>>
>>> Yesterday I started working on the LibvirtComputingResource
>>> class in order to apply the same patterns I used in the
>>> CitrixResourceBase + add more unit tests to it After 10 hours
>>> of work I got a bit stuck with the 1st test, which would cover
>>> the refactored LibvirtStopCommandWrapper. Why did I get stuck?
>>> The class used a few static methods that call native libraries,
>>> which I would like to mock. However, when writing the tests I
>>> faced problems with the current Mockito/PowerMock we are using:
>>> they are simply not enough for the task.
>>>
>>> What did I do then? I added a dependency to EasyMock and
>>> PowerMock-EasyMock API. It worked almost fine, but I had to add
>>> a “-noverify” to both my Eclipse Runtime configuration and also
>>> to the cloud-plugin-hypervisor-kvm/pom.xml file. I agree that’s
>>> not nice, but was my first attempt of getting it to work. After
>>> trying to first full build I faced more problems related to
>>> ClassDefNotFoundExpcetion which were complaining about Mockito
>>> classes. I then found out that adding the PowerMockRunner to
>>> all the tests classes was going to be a heavy burden and would
>>> also mess up future changes (e.g. the -noverify flag was
>>> removed from Java 8, thus adding it now would be a problem
>>> soon).
>>>
>>> Now that the first 2 paragraphs explain a bit about the
>>> problem, let’s get to the solution: Java 8
>>>
>>> The VerifyError that I was getting was due to the use of the
>>> latest EasyMock release (3.3.1). I tried to downgrade it to
>>> 3.1/3.2 but it also did not work. My decision: do not refactor
>>> if the proper tests cannot be added. This left me with one
>>> action: migrate to Java 8.
>>>
>>> There were mentions about Java 8 in february[1] and now I will
>>> put some energy in making it happen.
>>>
>>> What is your opinion on it?
>>>
>>> Thanks in advance.
>>>
>>> Cheers, Wilder
>>>
>>> http://mail-archives.apache.org/mod_mbox/cloudstack-dev/201502.mbox/
%3
>>
>>>
C54EEF6BE.5040401@shapeblue.com%3E<http://mail-archives.apache.org/mod_m
>> box/cloudstack-dev/201502.mbox/<54EEF6BE.5040401@shapeblue.com<mailto
:54
>>
>>
EEF6BE.5040401@shapeblue.com>>>
>>>
>>> Regards, Rohit Yadav Software Architect, ShapeBlue
>>>
>>>
>>> [cid:9DD97B41-04C5-45F0-92A7-951F3E962F7A]
>>>
>>>
>>> M. +91 88 262 30892 |
>>> rohit.yadav@shapeblue.com<ma...@shapeblue.com>
>>> Blog: bhaisaab.org<http://bhaisaab.org> | Twitter: @_bhaisaab
>>>
>>>
>>>
>>>
>>> Find out more about ShapeBlue and our range of CloudStack
>>> related services
>>>
>>> IaaS Cloud Design &
>>> Build<http://shapeblue.com/iaas-cloud-design-and-build//>
>>> CSForge – rapid IaaS deployment
>>> framework<http://shapeblue.com/csforge/> CloudStack
>>> Consulting<http://shapeblue.com/cloudstack-consultancy/>
>>> CloudStack Software
>>> Engineering<http://shapeblue.com/cloudstack-software-engineering/>
>>>
>>>
CloudStack Infrastructure
>>> Support<http://shapeblue.com/cloudstack-infrastructure-support/>
>>>
>>>
CloudStack Bootcamp Training
>>> Courses<http://shapeblue.com/cloudstack-training/>
>>>
>>> This email and any attachments to it may be confidential and
>>> are intended solely for the use of the individual to whom it
>>> is addressed. Any views or opinions expressed are solely those
>>> of the author and do not necessarily represent those of Shape
>>> Blue Ltd or related companies. If you are not the intended
>>> recipient of this email, you must neither take any action based
>>> upon its contents, nor copy or show it to anyone. Please
>>> contact the sender if you believe you have received this email
>>> in error. Shape Blue Ltd is a company incorporated in England &
>>> Wales. ShapeBlue Services India LLP is a company incorporated
>>> in India and is operated under license from Shape Blue Ltd.
>>> Shape Blue Brasil Consultoria Ltda is a company incorporated in
>>> Brasil and is operated under license from Shape Blue Ltd.
>>> ShapeBlue SA Pty Ltd is a company registered by The Republic of
>>> South Africa and is traded under license from Shape Blue Ltd.
>>> ShapeBlue is a registered trademark.
>>>
>>
>
> Find out more about ShapeBlue and our range of CloudStack related
> services
>
> IaaS Cloud Design &
> Build<http://shapeblue.com/iaas-cloud-design-and-build//> CSForge –
> rapid IaaS deployment framework<http://shapeblue.com/csforge/>
> CloudStack
> Consulting<http://shapeblue.com/cloudstack-consultancy/> CloudStack
> Software
> Engineering<http://shapeblue.com/cloudstack-software-engineering/>
> CloudStack Infrastructure
> Support<http://shapeblue.com/cloudstack-infrastructure-support/>
> CloudStack Bootcamp Training
> Courses<http://shapeblue.com/cloudstack-training/>
>
> This email and any attachments to it may be confidential and are
> intended solely for the use of the individual to whom it is
> addressed. Any views or opinions expressed are solely those of the
> author and do not necessarily represent those of Shape Blue Ltd or
> related companies. If you are not the intended recipient of this
> email, you must neither take any action based upon its contents,
> nor copy or show it to anyone. Please contact the sender if you
> believe you have received this email in error. Shape Blue Ltd is a
> company incorporated in England & Wales. ShapeBlue Services India
> LLP is a company incorporated in India and is operated under
> license from Shape Blue Ltd. Shape Blue Brasil Consultoria Ltda is
> a company incorporated in Brasil and is operated under license from
> Shape Blue Ltd. ShapeBlue SA Pty Ltd is a company registered by The
> Republic of South Africa and is traded under license from Shape
> Blue Ltd. ShapeBlue is a registered trademark.
>
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1
iQIcBAEBAgAGBQJVpXBRAAoJEAGbWC3bPspCsEMP/0q8vUvG89JAtrOqBRF3s6Jh
T7pSSFGds+D3/WLKI5/qwt9ep92CmFbTOLoF6J5+33kFgePUTWtCHsXJJSH00Iyc
XbYG6YYYSxFa2sj208hkMlResKzLUMUAGTdSosy8S2sdV6Bv3CY6eoo1XrnmOPIP
gi7VbXauVRPQNoqoTKhwVWYRt2aH3cOxnfyojYcBmc9wPq2DjsZbwcasBDLilyF/
ezAoLB4PO7/Nf1iKl/oIakjUep7xgba7TQNdP/i5ALeLRkC4fRHDCFRrw3FqZkhX
i7oPND9yZ8KrEJi1i/hvhTgFQm/r+gktu12ydfbGl0C3auaBgYsOVS92EIRTtFae
Q1zh9E51mVUoKfSUp92rff0d7wrHI7HEJ1qeciuc4TVGkvgShWnJzGTft2RiVRuI
ps1jQof8K3ae4tnjbxz4tfMIn7/ZTlZcklsSisGTrzhGzF802368TWL/ePR51s56
VLCEJZsHyF9PUUVf/3eRLkqk/6bABRfjqvjS8VHHbfVp0QGnYaqe4621FUnTC+lB
P077F/bvXhNlOHiPMGs4etcM+xq7aXXMPdXeLxdCsjmsENrvStApUQ07uRneX5a9
eU6BAXH0kEBLJzhXmOjbsCk378c/d4FxslgHySiYrt/C9NUpY0bakZpjYFemw12E
+PFfUwbg/Lq8UvaS8fWq
=nJKK
-----END PGP SIGNATURE-----
Re: [DISCUSS] Moving to Java 8
Posted by John Burwell <jo...@shapeblue.com>.
Wido,
Is the OpenJDK PPA [1] not acceptable? Since Java7 is no longer supported, we run the risk of an Java security issue affecting the project that won’t be fixed.
Thanks,
-John
[1]: https://launchpad.net/~openjdk-r/+archive/ubuntu/ppa
---
John Burwell (@john_burwell)
VP of Software Engineering, ShapeBlue
(571) 403-2411 | +44 20 3603 0542
http://www.shapeblue.com
Please join us at CloudStack Collab EU — http://events.linuxfoundation.org/events/cloudstack-collaboration-conference-europe
> On Jul 10, 2015, at 5:47 PM, Wido den Hollander <wi...@widodh.nl> wrote:
>
> Signed PGP part
>
>
> On 07/10/2015 09:22 PM, Rohit Yadav wrote:
> > Ping Wilder - any progress/plan on moving forward (perhaps after
> > 4.6?).
> >
>
> I don't think there is? Since Ubuntu 14.04 doesn't support Java 8 in
> any package form we can't really continue.
>
> Ubuntu 16.04 will ship with Java 8 and that will be the next LTS.
>
> Wido
>
> > On 01-May-2015, at 4:07 pm, Wilder Rodrigues
> > <WR...@schubergphilis.com>>
> > wrote:
> >
> > Hi Marcus,
> >
> > Sorry for the push, I think after doing the whole
> > CitrixResourceBase refactor I also got a bit attached to the whole
> > thing/solution. ;)
> >
> > Thanks for the input you gave. I will finish the refactor and apply
> > it to both implementations.
> >
> > Cheers, Wilder
> >
> >
> > On 01 May 2015, at 09:06, Marcus
> > <sh...@gmail.com><mailto:shadowsor@gmai
> l.com>>
> > wrote:
> >
> > Oh, and of course the annotation added to the wrapper looks like:
> >
> >
> > ...
> >
> > @ResourceWrapper(handles = CheckHealthCommand.class)
> >
> > public final class LibvirtCheckHealthCommandWrapper
> >
> > ...
> >
> >
> > maybe 'wraps' or 'wrapperfor' would be better than 'handles' in
> > your naming scheme. You get the idea.
> >
> > On Thu, Apr 30, 2015 at 11:59 PM, Marcus
> > <sh...@gmail.com>> wrote: I agree,
> > this wrapper is a good step forward. It's totally fine to continue
> > on that path because it is obviously better and makes it easy to
> > switch to autodetection anytime later by simply adding the
> > annotation. Sorry if I got a bit passionate about that, but as you
> > mention I also get tired of adding things in multiple places, and
> > the annotations have worked well in the API and provide a good
> > model to emulate for consistency.
> >
> > I can't share code, because these extensions to
> > LibvirtComputingResource that I've provided for other companies
> > have not been open sourced. I can speak more generically though
> > about methods.
> >
> > To answer question "a", reflection allows you to do something
> > like:
> >
> > Reflections reflections = new
> > Reflections("com.cloud.hypervisor.kvm.resource.wrapper");
> > Set<Class<? extends CommandWrapper>> wrappers =
> > reflections.getSubTypesOf(CommandWrapper.class);
> >
> > So here in "new Reflections" we are automatically filtering for
> > just the wrappers that would apply to the KVM plugin. Then to
> > finish it off, you iterate through the wrappers and do:
> >
> > ResourceWrapper annotation =
> > wrapper.getAnnotation(ResourceWrapper.class);
> > citrixCommands.put(annotation.handles(), wrapper.newInstance());
> >
> > Sorry, I guess that's four lines, plus the relevant for loop. And
> > probably a null check or something for the annotation. You also
> > have to add the annotation class itself, and add a line for the
> > annotation in each wrapper, but in the end when we add new
> > Commands, we won't have to touch anything but the new class that
> > handles the command.
> >
> >
> > public @interface ResourceWrapper {
> >
> > Class<? extends Command> handles();
> >
> > }
> >
> > There's an example of something similar to this in
> > KVMStoragePoolManager.java (annotation is StoragePoolInfo.java).
> > This example has actually been adapted from that. Also to a lesser
> > extent in the API server, but it is spread across a bunch of
> > classes.
> >
> > On Thu, Apr 30, 2015 at 10:41 PM, Wilder Rodrigues
> > <WR...@schubergphilis.com>>
> > wrote: Hi Marcus,
> >
> > Thanks for the email… I’m always in for improvements. But why can’t
> > you share the code?
> >
> > Few points below:
> >
> > 1. I added an subclassing example of LibvirtComputingResource
> > because you mentioned it in a previous email:
> >
> > On 23 Apr 2015, at 17:26, Marcus
> > <sh...@gmail.com>> wrote:
> >
> > I mentioned the reflection model because that's how I tend to
> > handle the commands when subclassing LibvirtComputingResource.
> >
> > 2. Current situation with LibvirtComputingResource on Master is:
> >
> > a. 67 IFs b. 67 private/protected methods that are used only there
> > c. If a new Command is added it means we will have a new IF and a
> > new private method e. Maintenance is hell, test is close to zero
> > and code quality is below expectations
> >
> > That being said, the main idea with the refactor is to change
> > structure only, not behaviour. So what I’m doing is to simply move
> > the code out the LibvirtCompRes and write tests for it, keeping the
> > behaviour the same - to be done in a next phase. If you look at the
> > changes you will see that some wrappers are already 100% covered.
> > However, some others have 4% or 8% (not that much though). I would
> > like to refactor that as well, but that could change behaviour
> > (mentioned above) which I don’t want to touch now.
> >
> > 3. With the new situation:
> >
> > a. No IFs b. All methods wrapped by other classes (command
> > wrappers) - loosely coupled, easier to test and maintain c. If a
> > new Command is added we would have to add a command wrapper and 1
> > line in the request wrapper implementation ( I know, it hurts you a
> > bit) - but please bear with me for the good news.
> >
> > 4. the warnings are due to that: Hashtable<Class<? extends
> > Command>, CommandWrapper>()
> >
> > No big deal.
> >
> > As I understood from your first paragraph we would have to
> > annotated the commands classes, right? I mean, all of them.
> >
> > That’s something I wouldn’t do in this phase, to be honest. It
> > might seem harmless to do, but I like to break things down a bit
> > and have more isolation in my changes.
> >
> > What’s next: I will finish the refactor with the request wrapper as
> > it is. For me it is no problem do add the lines now and remove them
> > in 1 week. Most of the work is concentrated in the tests, which I’m
> > trying as hard as I can to get them in the best way possible. Once
> > it’s done and pushed to master, I will analyse what we would need
> > to apply the annotation.
> >
> > But before I go to bring the kids to school, just one question:
> >
> > a. The “handle” value, in the annotation, would have the wrapper
> > class that would be used for that command, right? Now let’s get 1
> > command as example: CheckHealthCommand. Its wrapper implementation
> > differs per hypervisor (just like all the other wrapper commands
> > do). I’m not taking the time to really think about it now, but how
> > would we annotated the different wrappers per command?
> >
> > Thanks again for your time.
> >
> > Cheers, Wilder
> >
> >
> > On 30 Apr 2015, at 22:52, Marcus
> > <sh...@gmail.com>> wrote:
> >
> > Ok. I wish I could share some code, because it isn't really as big
> > of a deal as it sounds from your reasoning. It is literally just 3
> > lines on startup that fetch anything with the '@AgentExecutor'
> > annotation and stores it in a hash whose key is the value from
> > @AgentExecutor's 'handles' property. Then when a *Command comes it
> > it is passed to the appropriate Executor class.
> >
> > Looking at CitrixRequestWrapper, the 3 lines I mention are almost
> > identical in function to your init method, just that it uses the
> > annotation to find all of the commands, rather than hardcoding
> > them. We use the same annotation design for the api side of the
> > code on the management server, which allows the api commands to be
> > easier to write and self-contained (you don't have to update other
> > code to add a new api call). It makes things easier for novice
> > developers.
> >
> > This implementation is no less typesafe than the previous design
> > (the one with all of the instanceof). It didn't require any casting
> > or warning suppression, either, as the wrapper does.
> >
> > Extending LibvirtComputingResource is not ideal, and doesn't work
> > if multiple third parties are involved. Granted, there hasn't been
> > a lot of demand for this, nevertheless it's particularly important
> > for KVM, where the Command classes are executed on the hypervisor
> > it's not really feasible to just dump the code in your management
> > server-side plugin like some plugins do.
> >
> > In reviewing the code, the two implementations are really very
> > close. If you just updated init to fetch the wrappers based on
> > either an annotation or the class they extend, or something along
> > those lines so this method doesn't have to be edited every time a
> > command is added, that would be more or less the same thing. The
> > the KVM agent would be pluggable like the management server side
> > is.
> >
> > On Thu, Apr 30, 2015 at 12:55 PM, Wilder Rodrigues
> > <WR...@schubergphilis.com>>
> > wrote: Hi Marcus,
> >
> > Apologies for taking so much time to reply to your email, but was,
> > and still am, quite busy. :)
> >
> > I would only use reflection if that was the only way to do it. The
> > use of reflection usually makes the code more complex, which is not
> > good when we have java developers in all different levels (from jr.
> > do sr) working with cloudstack. It also makes us lose the type
> > safety, which might also harm the exception handling if not done
> > well. In addition, if we need to refactor something, the IDE is no
> > longer going to do few things because the refection code cannot be
> > found.
> >
> > If someone will need to extend the LibvirtComputingResource that
> > would be no problem with the approach I’m using. The
> > CitrixResourceBase also has quite few sub-classes and it works just
> > fine.
> >
> > I will document on the wiki page how it should be done when
> > sub-classing the LibvirtComputingResource class.
> >
> > In a quick note/snippet, one would do:
> >
> > public class EkhoComputingResource extends LibvirtComputingResource
> > {
> >
> > @Override public Answer executeRequest(final Command cmd) {
> >
> > final LibvirtRequestWrapper wrapper =
> > LibvirtRequestWrapper.getInstance(); try { return
> > wrapper.execute(cmd, this); } catch (final Exception e) { return
> > Answer.createUnsupportedCommandAnswer(cmd); } } }
> >
> >
> > In the flyweight where I keep the wrapper we could have ():
> >
> > final Hashtable<Class<? extends Command>, CommandWrapper>
> > linbvirtCommands = new Hashtable<Class<? extends Command>,
> > CommandWrapper>(); linbvirtCommands.put(StopCommand.class, new
> > LibvirtStopCommandWrapper());
> >
> > final Hashtable<Class<? extends Command>, CommandWrapper>
> > ekhoCommands = new Hashtable<Class<? extends Command>,
> > CommandWrapper>(); linbvirtCommands.put(StopCommand.class, new
> > EkhoStopCommandWrapper());
> >
> > resources.put(LibvirtComputingResource.class, linbvirtCommands);
> > resources.put(EkhoComputingResource.class, ekhoCommands);
> >
> > But that is needed only if the StopCommand has a different
> > behaviour for the EkhoComputingResource.
> >
> > Once a better version of the documentation is on the wiki, I will
> > let you know.
> >
> > On other matters, I’m also adding unit tests for all the changes.
> > We already went from 4% to 13.6% coverage in the KVM hypervisor
> > plugin. The code I already refactored has 56% of coverage.
> >
> > You can see all the commits here:
> > https://github.com/schubergphilis/cloudstack/tree/refactor/libvirt_res
> ource
> >
> > Cheers, Wilder
> >
> > On 23 Apr 2015, at 17:26, Marcus
> > <sh...@gmail.com>> wrote:
> >
> > Great to see someone working on it. What sorts of roadblocks came
> > out of reflection? How does the wrapper design solve the
> > pluggability issue? This is pretty important to me, since I've
> > worked with several companies now that end up subclassing
> > LibvirtComputingResource in order to handle their own Commands on
> > the hypervisor from their server-side plugins, and changing their
> > 'resource' to that in agent.properties. Since the main agent class
> > needs to be set at agent join, this is harder to manage than it
> > should be.
> >
> > I mentioned the reflection model because that's how I tend to
> > handle the commands when subclassing LibvirtComputingResource. I
> > haven't had any problems with it, but then again I haven't tried to
> > refactor 5500 lines into that model, either.
> >
> > On Thu, Apr 23, 2015 at 1:17 AM, Wilder Rodrigues
> > <WR...@schubergphilis.com>>
> > wrote:
> >
> > Hi Marcus,
> >
> > I like the annotation idea, but reflection is trick because it
> > hides some information about the code.
> >
> > Please, have a look at the CitrixResourceBase after the refactor I
> > did. It became quite smaller and test coverage was improved.
> >
> > URL:
> > https://cwiki.apache.org/confluence/display/CLOUDSTACK/Refactoring+Xen
> Server+Hypervisor+Plugin
> >
> > The same patter is being about to Libvirt stuff. The coverage on
> > the KVM hypervisor plugin already went from 4 to 10.5% after
> > refactoring 6 commands
> >
> > Cheers, Wilder
> >
> > On 22 Apr 2015, at 23:06, Marcus
> > <sh...@gmail.com>> wrote:
> >
> > Kind of a tangent, but I'd actually like to see some work done to
> > clean up LibvirtComputing resource. One model I've prototyped that
> > seems to work is to create an annotation, such as
> > 'KVMCommandExecutor', with a 'handles' property. With this
> > annotation, you implement a class that handles, e.g. StartCommand,
> > etc. Then in LibvirtComputingResource, the 'configure' method
> > fetches all of these executors via reflection and stores them in an
> > object. Then, instead of having all of the 'instanceof' lines in
> > LibvirtComputingResource, the executeRequest method fetches the
> > executor that handles the incoming command and runs it.
> >
> > I think this would break up LibvirtComputingResource into smaller,
> > more testable and manageable chunks, and force things like config
> > and utility methods to move to a more sane location, as well. As a
> > bonus, this model makes things pluggable. Someone could ship KVM
> > plugin code containing standalone command executors that are
> > discovered at runtime for things they need to run at the hypervisor
> > level.
> >
> > On Tue, Apr 21, 2015 at 6:27 AM, Wilder Rodrigues
> > <WR...@schubergphilis.com>>
> > wrote:
> >
> > Hi all,
> >
> > Yesterday I started working on the LibvirtComputingResource class
> > in order to apply the same patterns I used in the
> > CitrixResourceBase + add more unit tests to it After 10 hours of
> > work I got a bit stuck with the 1st test, which would cover the
> > refactored LibvirtStopCommandWrapper. Why did I get stuck? The
> > class used a few static methods that call native libraries, which I
> > would like to mock. However, when writing the tests I faced
> > problems with the current Mockito/PowerMock we are using: they are
> > simply not enough for the task.
> >
> > What did I do then? I added a dependency to EasyMock and
> > PowerMock-EasyMock API. It worked almost fine, but I had to add a
> > “-noverify” to both my Eclipse Runtime configuration and also to
> > the cloud-plugin-hypervisor-kvm/pom.xml file. I agree that’s not
> > nice, but was my first attempt of getting it to work. After trying
> > to first full build I faced more problems related to
> > ClassDefNotFoundExpcetion which were complaining about Mockito
> > classes. I then found out that adding the PowerMockRunner to all
> > the tests classes was going to be a heavy burden and would also
> > mess up future changes (e.g. the -noverify flag was removed from
> > Java 8, thus adding it now would be a problem soon).
> >
> > Now that the first 2 paragraphs explain a bit about the problem,
> > let’s get to the solution: Java 8
> >
> > The VerifyError that I was getting was due to the use of the latest
> > EasyMock release (3.3.1). I tried to downgrade it to 3.1/3.2 but it
> > also did not work. My decision: do not refactor if the proper tests
> > cannot be added. This left me with one action: migrate to Java 8.
> >
> > There were mentions about Java 8 in february[1] and now I will put
> > some energy in making it happen.
> >
> > What is your opinion on it?
> >
> > Thanks in advance.
> >
> > Cheers, Wilder
> >
> > http://mail-archives.apache.org/mod_mbox/cloudstack-dev/201502.mbox/%3
> C54EEF6BE.5040401@shapeblue.com%3E<http://mail-archives.apache.org/mod_m
> box/cloudstack-dev/201502.mbox/<54EEF6BE.5040401@shapeblue.com<mailto:54
> EEF6BE.5040401@shapeblue.com>>>
> >
> > Regards, Rohit Yadav Software Architect, ShapeBlue
> >
> >
> > [cid:9DD97B41-04C5-45F0-92A7-951F3E962F7A]
> >
> >
> > M. +91 88 262 30892 |
> > rohit.yadav@shapeblue.com<ma...@shapeblue.com> Blog:
> > bhaisaab.org<http://bhaisaab.org> | Twitter: @_bhaisaab
> >
> >
> >
> >
> > Find out more about ShapeBlue and our range of CloudStack related
> > services
> >
> > IaaS Cloud Design &
> > Build<http://shapeblue.com/iaas-cloud-design-and-build//> CSForge –
> > rapid IaaS deployment framework<http://shapeblue.com/csforge/>
> > CloudStack
> > Consulting<http://shapeblue.com/cloudstack-consultancy/> CloudStack
> > Software
> > Engineering<http://shapeblue.com/cloudstack-software-engineering/>
> > CloudStack Infrastructure
> > Support<http://shapeblue.com/cloudstack-infrastructure-support/>
> > CloudStack Bootcamp Training
> > Courses<http://shapeblue.com/cloudstack-training/>
> >
> > This email and any attachments to it may be confidential and are
> > intended solely for the use of the individual to whom it is
> > addressed. Any views or opinions expressed are solely those of the
> > author and do not necessarily represent those of Shape Blue Ltd or
> > related companies. If you are not the intended recipient of this
> > email, you must neither take any action based upon its contents,
> > nor copy or show it to anyone. Please contact the sender if you
> > believe you have received this email in error. Shape Blue Ltd is a
> > company incorporated in England & Wales. ShapeBlue Services India
> > LLP is a company incorporated in India and is operated under
> > license from Shape Blue Ltd. Shape Blue Brasil Consultoria Ltda is
> > a company incorporated in Brasil and is operated under license from
> > Shape Blue Ltd. ShapeBlue SA Pty Ltd is a company registered by The
> > Republic of South Africa and is traded under license from Shape
> > Blue Ltd. ShapeBlue is a registered trademark.
> >
>
Find out more about ShapeBlue and our range of CloudStack related services
IaaS Cloud Design & Build<http://shapeblue.com/iaas-cloud-design-and-build//>
CSForge – rapid IaaS deployment framework<http://shapeblue.com/csforge/>
CloudStack Consulting<http://shapeblue.com/cloudstack-consultancy/>
CloudStack Software Engineering<http://shapeblue.com/cloudstack-software-engineering/>
CloudStack Infrastructure Support<http://shapeblue.com/cloudstack-infrastructure-support/>
CloudStack Bootcamp Training Courses<http://shapeblue.com/cloudstack-training/>
This email and any attachments to it may be confidential and are intended solely for the use of the individual to whom it is addressed. Any views or opinions expressed are solely those of the author and do not necessarily represent those of Shape Blue Ltd or related companies. If you are not the intended recipient of this email, you must neither take any action based upon its contents, nor copy or show it to anyone. Please contact the sender if you believe you have received this email in error. Shape Blue Ltd is a company incorporated in England & Wales. ShapeBlue Services India LLP is a company incorporated in India and is operated under license from Shape Blue Ltd. Shape Blue Brasil Consultoria Ltda is a company incorporated in Brasil and is operated under license from Shape Blue Ltd. ShapeBlue SA Pty Ltd is a company registered by The Republic of South Africa and is traded under license from Shape Blue Ltd. ShapeBlue is a registered trademark.
Re: [DISCUSS] Moving to Java 8
Posted by Wido den Hollander <wi...@widodh.nl>.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 07/10/2015 09:22 PM, Rohit Yadav wrote:
> Ping Wilder - any progress/plan on moving forward (perhaps after
> 4.6?).
>
I don't think there is? Since Ubuntu 14.04 doesn't support Java 8 in
any package form we can't really continue.
Ubuntu 16.04 will ship with Java 8 and that will be the next LTS.
Wido
> On 01-May-2015, at 4:07 pm, Wilder Rodrigues
> <WR...@schubergphilis.com>>
> wrote:
>
> Hi Marcus,
>
> Sorry for the push, I think after doing the whole
> CitrixResourceBase refactor I also got a bit attached to the whole
> thing/solution. ;)
>
> Thanks for the input you gave. I will finish the refactor and apply
> it to both implementations.
>
> Cheers, Wilder
>
>
> On 01 May 2015, at 09:06, Marcus
> <sh...@gmail.com><mailto:shadowsor@gmai
l.com>>
> wrote:
>
> Oh, and of course the annotation added to the wrapper looks like:
>
>
> ...
>
> @ResourceWrapper(handles = CheckHealthCommand.class)
>
> public final class LibvirtCheckHealthCommandWrapper
>
> ...
>
>
> maybe 'wraps' or 'wrapperfor' would be better than 'handles' in
> your naming scheme. You get the idea.
>
> On Thu, Apr 30, 2015 at 11:59 PM, Marcus
> <sh...@gmail.com>> wrote: I agree,
> this wrapper is a good step forward. It's totally fine to continue
> on that path because it is obviously better and makes it easy to
> switch to autodetection anytime later by simply adding the
> annotation. Sorry if I got a bit passionate about that, but as you
> mention I also get tired of adding things in multiple places, and
> the annotations have worked well in the API and provide a good
> model to emulate for consistency.
>
> I can't share code, because these extensions to
> LibvirtComputingResource that I've provided for other companies
> have not been open sourced. I can speak more generically though
> about methods.
>
> To answer question "a", reflection allows you to do something
> like:
>
> Reflections reflections = new
> Reflections("com.cloud.hypervisor.kvm.resource.wrapper");
> Set<Class<? extends CommandWrapper>> wrappers =
> reflections.getSubTypesOf(CommandWrapper.class);
>
> So here in "new Reflections" we are automatically filtering for
> just the wrappers that would apply to the KVM plugin. Then to
> finish it off, you iterate through the wrappers and do:
>
> ResourceWrapper annotation =
> wrapper.getAnnotation(ResourceWrapper.class);
> citrixCommands.put(annotation.handles(), wrapper.newInstance());
>
> Sorry, I guess that's four lines, plus the relevant for loop. And
> probably a null check or something for the annotation. You also
> have to add the annotation class itself, and add a line for the
> annotation in each wrapper, but in the end when we add new
> Commands, we won't have to touch anything but the new class that
> handles the command.
>
>
> public @interface ResourceWrapper {
>
> Class<? extends Command> handles();
>
> }
>
> There's an example of something similar to this in
> KVMStoragePoolManager.java (annotation is StoragePoolInfo.java).
> This example has actually been adapted from that. Also to a lesser
> extent in the API server, but it is spread across a bunch of
> classes.
>
> On Thu, Apr 30, 2015 at 10:41 PM, Wilder Rodrigues
> <WR...@schubergphilis.com>>
> wrote: Hi Marcus,
>
> Thanks for the email… I’m always in for improvements. But why can’t
> you share the code?
>
> Few points below:
>
> 1. I added an subclassing example of LibvirtComputingResource
> because you mentioned it in a previous email:
>
> On 23 Apr 2015, at 17:26, Marcus
> <sh...@gmail.com>> wrote:
>
> I mentioned the reflection model because that's how I tend to
> handle the commands when subclassing LibvirtComputingResource.
>
> 2. Current situation with LibvirtComputingResource on Master is:
>
> a. 67 IFs b. 67 private/protected methods that are used only there
> c. If a new Command is added it means we will have a new IF and a
> new private method e. Maintenance is hell, test is close to zero
> and code quality is below expectations
>
> That being said, the main idea with the refactor is to change
> structure only, not behaviour. So what I’m doing is to simply move
> the code out the LibvirtCompRes and write tests for it, keeping the
> behaviour the same - to be done in a next phase. If you look at the
> changes you will see that some wrappers are already 100% covered.
> However, some others have 4% or 8% (not that much though). I would
> like to refactor that as well, but that could change behaviour
> (mentioned above) which I don’t want to touch now.
>
> 3. With the new situation:
>
> a. No IFs b. All methods wrapped by other classes (command
> wrappers) - loosely coupled, easier to test and maintain c. If a
> new Command is added we would have to add a command wrapper and 1
> line in the request wrapper implementation ( I know, it hurts you a
> bit) - but please bear with me for the good news.
>
> 4. the warnings are due to that: Hashtable<Class<? extends
> Command>, CommandWrapper>()
>
> No big deal.
>
> As I understood from your first paragraph we would have to
> annotated the commands classes, right? I mean, all of them.
>
> That’s something I wouldn’t do in this phase, to be honest. It
> might seem harmless to do, but I like to break things down a bit
> and have more isolation in my changes.
>
> What’s next: I will finish the refactor with the request wrapper as
> it is. For me it is no problem do add the lines now and remove them
> in 1 week. Most of the work is concentrated in the tests, which I’m
> trying as hard as I can to get them in the best way possible. Once
> it’s done and pushed to master, I will analyse what we would need
> to apply the annotation.
>
> But before I go to bring the kids to school, just one question:
>
> a. The “handle” value, in the annotation, would have the wrapper
> class that would be used for that command, right? Now let’s get 1
> command as example: CheckHealthCommand. Its wrapper implementation
> differs per hypervisor (just like all the other wrapper commands
> do). I’m not taking the time to really think about it now, but how
> would we annotated the different wrappers per command?
>
> Thanks again for your time.
>
> Cheers, Wilder
>
>
> On 30 Apr 2015, at 22:52, Marcus
> <sh...@gmail.com>> wrote:
>
> Ok. I wish I could share some code, because it isn't really as big
> of a deal as it sounds from your reasoning. It is literally just 3
> lines on startup that fetch anything with the '@AgentExecutor'
> annotation and stores it in a hash whose key is the value from
> @AgentExecutor's 'handles' property. Then when a *Command comes it
> it is passed to the appropriate Executor class.
>
> Looking at CitrixRequestWrapper, the 3 lines I mention are almost
> identical in function to your init method, just that it uses the
> annotation to find all of the commands, rather than hardcoding
> them. We use the same annotation design for the api side of the
> code on the management server, which allows the api commands to be
> easier to write and self-contained (you don't have to update other
> code to add a new api call). It makes things easier for novice
> developers.
>
> This implementation is no less typesafe than the previous design
> (the one with all of the instanceof). It didn't require any casting
> or warning suppression, either, as the wrapper does.
>
> Extending LibvirtComputingResource is not ideal, and doesn't work
> if multiple third parties are involved. Granted, there hasn't been
> a lot of demand for this, nevertheless it's particularly important
> for KVM, where the Command classes are executed on the hypervisor
> it's not really feasible to just dump the code in your management
> server-side plugin like some plugins do.
>
> In reviewing the code, the two implementations are really very
> close. If you just updated init to fetch the wrappers based on
> either an annotation or the class they extend, or something along
> those lines so this method doesn't have to be edited every time a
> command is added, that would be more or less the same thing. The
> the KVM agent would be pluggable like the management server side
> is.
>
> On Thu, Apr 30, 2015 at 12:55 PM, Wilder Rodrigues
> <WR...@schubergphilis.com>>
> wrote: Hi Marcus,
>
> Apologies for taking so much time to reply to your email, but was,
> and still am, quite busy. :)
>
> I would only use reflection if that was the only way to do it. The
> use of reflection usually makes the code more complex, which is not
> good when we have java developers in all different levels (from jr.
> do sr) working with cloudstack. It also makes us lose the type
> safety, which might also harm the exception handling if not done
> well. In addition, if we need to refactor something, the IDE is no
> longer going to do few things because the refection code cannot be
> found.
>
> If someone will need to extend the LibvirtComputingResource that
> would be no problem with the approach I’m using. The
> CitrixResourceBase also has quite few sub-classes and it works just
> fine.
>
> I will document on the wiki page how it should be done when
> sub-classing the LibvirtComputingResource class.
>
> In a quick note/snippet, one would do:
>
> public class EkhoComputingResource extends LibvirtComputingResource
> {
>
> @Override public Answer executeRequest(final Command cmd) {
>
> final LibvirtRequestWrapper wrapper =
> LibvirtRequestWrapper.getInstance(); try { return
> wrapper.execute(cmd, this); } catch (final Exception e) { return
> Answer.createUnsupportedCommandAnswer(cmd); } } }
>
>
> In the flyweight where I keep the wrapper we could have ():
>
> final Hashtable<Class<? extends Command>, CommandWrapper>
> linbvirtCommands = new Hashtable<Class<? extends Command>,
> CommandWrapper>(); linbvirtCommands.put(StopCommand.class, new
> LibvirtStopCommandWrapper());
>
> final Hashtable<Class<? extends Command>, CommandWrapper>
> ekhoCommands = new Hashtable<Class<? extends Command>,
> CommandWrapper>(); linbvirtCommands.put(StopCommand.class, new
> EkhoStopCommandWrapper());
>
> resources.put(LibvirtComputingResource.class, linbvirtCommands);
> resources.put(EkhoComputingResource.class, ekhoCommands);
>
> But that is needed only if the StopCommand has a different
> behaviour for the EkhoComputingResource.
>
> Once a better version of the documentation is on the wiki, I will
> let you know.
>
> On other matters, I’m also adding unit tests for all the changes.
> We already went from 4% to 13.6% coverage in the KVM hypervisor
> plugin. The code I already refactored has 56% of coverage.
>
> You can see all the commits here:
> https://github.com/schubergphilis/cloudstack/tree/refactor/libvirt_res
ource
>
> Cheers, Wilder
>
> On 23 Apr 2015, at 17:26, Marcus
> <sh...@gmail.com>> wrote:
>
> Great to see someone working on it. What sorts of roadblocks came
> out of reflection? How does the wrapper design solve the
> pluggability issue? This is pretty important to me, since I've
> worked with several companies now that end up subclassing
> LibvirtComputingResource in order to handle their own Commands on
> the hypervisor from their server-side plugins, and changing their
> 'resource' to that in agent.properties. Since the main agent class
> needs to be set at agent join, this is harder to manage than it
> should be.
>
> I mentioned the reflection model because that's how I tend to
> handle the commands when subclassing LibvirtComputingResource. I
> haven't had any problems with it, but then again I haven't tried to
> refactor 5500 lines into that model, either.
>
> On Thu, Apr 23, 2015 at 1:17 AM, Wilder Rodrigues
> <WR...@schubergphilis.com>>
> wrote:
>
> Hi Marcus,
>
> I like the annotation idea, but reflection is trick because it
> hides some information about the code.
>
> Please, have a look at the CitrixResourceBase after the refactor I
> did. It became quite smaller and test coverage was improved.
>
> URL:
> https://cwiki.apache.org/confluence/display/CLOUDSTACK/Refactoring+Xen
Server+Hypervisor+Plugin
>
> The same patter is being about to Libvirt stuff. The coverage on
> the KVM hypervisor plugin already went from 4 to 10.5% after
> refactoring 6 commands
>
> Cheers, Wilder
>
> On 22 Apr 2015, at 23:06, Marcus
> <sh...@gmail.com>> wrote:
>
> Kind of a tangent, but I'd actually like to see some work done to
> clean up LibvirtComputing resource. One model I've prototyped that
> seems to work is to create an annotation, such as
> 'KVMCommandExecutor', with a 'handles' property. With this
> annotation, you implement a class that handles, e.g. StartCommand,
> etc. Then in LibvirtComputingResource, the 'configure' method
> fetches all of these executors via reflection and stores them in an
> object. Then, instead of having all of the 'instanceof' lines in
> LibvirtComputingResource, the executeRequest method fetches the
> executor that handles the incoming command and runs it.
>
> I think this would break up LibvirtComputingResource into smaller,
> more testable and manageable chunks, and force things like config
> and utility methods to move to a more sane location, as well. As a
> bonus, this model makes things pluggable. Someone could ship KVM
> plugin code containing standalone command executors that are
> discovered at runtime for things they need to run at the hypervisor
> level.
>
> On Tue, Apr 21, 2015 at 6:27 AM, Wilder Rodrigues
> <WR...@schubergphilis.com>>
> wrote:
>
> Hi all,
>
> Yesterday I started working on the LibvirtComputingResource class
> in order to apply the same patterns I used in the
> CitrixResourceBase + add more unit tests to it After 10 hours of
> work I got a bit stuck with the 1st test, which would cover the
> refactored LibvirtStopCommandWrapper. Why did I get stuck? The
> class used a few static methods that call native libraries, which I
> would like to mock. However, when writing the tests I faced
> problems with the current Mockito/PowerMock we are using: they are
> simply not enough for the task.
>
> What did I do then? I added a dependency to EasyMock and
> PowerMock-EasyMock API. It worked almost fine, but I had to add a
> “-noverify” to both my Eclipse Runtime configuration and also to
> the cloud-plugin-hypervisor-kvm/pom.xml file. I agree that’s not
> nice, but was my first attempt of getting it to work. After trying
> to first full build I faced more problems related to
> ClassDefNotFoundExpcetion which were complaining about Mockito
> classes. I then found out that adding the PowerMockRunner to all
> the tests classes was going to be a heavy burden and would also
> mess up future changes (e.g. the -noverify flag was removed from
> Java 8, thus adding it now would be a problem soon).
>
> Now that the first 2 paragraphs explain a bit about the problem,
> let’s get to the solution: Java 8
>
> The VerifyError that I was getting was due to the use of the latest
> EasyMock release (3.3.1). I tried to downgrade it to 3.1/3.2 but it
> also did not work. My decision: do not refactor if the proper tests
> cannot be added. This left me with one action: migrate to Java 8.
>
> There were mentions about Java 8 in february[1] and now I will put
> some energy in making it happen.
>
> What is your opinion on it?
>
> Thanks in advance.
>
> Cheers, Wilder
>
> http://mail-archives.apache.org/mod_mbox/cloudstack-dev/201502.mbox/%3
C54EEF6BE.5040401@shapeblue.com%3E<http://mail-archives.apache.org/mod_m
box/cloudstack-dev/201502.mbox/<54EEF6BE.5040401@shapeblue.com<mailto:54
EEF6BE.5040401@shapeblue.com>>>
>
> Regards, Rohit Yadav Software Architect, ShapeBlue
>
>
> [cid:9DD97B41-04C5-45F0-92A7-951F3E962F7A]
>
>
> M. +91 88 262 30892 |
> rohit.yadav@shapeblue.com<ma...@shapeblue.com> Blog:
> bhaisaab.org<http://bhaisaab.org> | Twitter: @_bhaisaab
>
>
>
>
> Find out more about ShapeBlue and our range of CloudStack related
> services
>
> IaaS Cloud Design &
> Build<http://shapeblue.com/iaas-cloud-design-and-build//> CSForge –
> rapid IaaS deployment framework<http://shapeblue.com/csforge/>
> CloudStack
> Consulting<http://shapeblue.com/cloudstack-consultancy/> CloudStack
> Software
> Engineering<http://shapeblue.com/cloudstack-software-engineering/>
> CloudStack Infrastructure
> Support<http://shapeblue.com/cloudstack-infrastructure-support/>
> CloudStack Bootcamp Training
> Courses<http://shapeblue.com/cloudstack-training/>
>
> This email and any attachments to it may be confidential and are
> intended solely for the use of the individual to whom it is
> addressed. Any views or opinions expressed are solely those of the
> author and do not necessarily represent those of Shape Blue Ltd or
> related companies. If you are not the intended recipient of this
> email, you must neither take any action based upon its contents,
> nor copy or show it to anyone. Please contact the sender if you
> believe you have received this email in error. Shape Blue Ltd is a
> company incorporated in England & Wales. ShapeBlue Services India
> LLP is a company incorporated in India and is operated under
> license from Shape Blue Ltd. Shape Blue Brasil Consultoria Ltda is
> a company incorporated in Brasil and is operated under license from
> Shape Blue Ltd. ShapeBlue SA Pty Ltd is a company registered by The
> Republic of South Africa and is traded under license from Shape
> Blue Ltd. ShapeBlue is a registered trademark.
>
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1
iQIcBAEBAgAGBQJVoD1hAAoJEAGbWC3bPspCft0P/jA0VgMdGxdWGYCaJxJWEQcn
u+9nXyQfrLOmejiYmjVuY0o9rX31iGGJDqEEnokebh0Mq6JVMnLzQ4TCOHx2sDPg
/aKKFg6QOq3fiR38/l/8QCvYGnyEz+IxATqcaTd/821h+3EEeKpj6UJLbjmABa9F
canevcCSUPrWUwpTrhMncWZs416fzR+2cYjblGlBIwWi6n1d8kIvGyaIALvOhOvl
/sl0o8/635Uy7LywdgGE/Hg+EcAmyRH6W0LhNaZNjrc/ioj7hIIWja0L+HGjcCc7
nEtA1T4mJkXfMSjFYP6LcVMKcrXDwfTHEBo1rL+xtXVrtrUARg8VDdrxSkezc68X
fMaFFoXmjCjGRzF7xqzK02Mn2kuK/owlqvCXwmfp5PglW4kP97m6vnq3h1cRFHxl
fpANd1ZYSSWamKsHEDYM7U28aopdYI2jnQavlWlwKXNQjc+8G5ZkgbQyFQSPFlkA
+DaYA0LFqx71PRbwwRo/+DxR31XB1ZjfJqtSO49e87YdiAfUowyGET7fxGLlq18I
FU5MVCO9gwXVV/1x88PWmjxmmyFvPXOPb30ftStHimSeGajAms4aZPOo/aaBe55H
aMzfTeNpcajJ6HAT9hETMqKkAVuFe4zHu8GG3bO8f4jN5eu4vk2ACy+E7iVoZove
rILQJfo0P3fsA7MyLZPG
=bcDV
-----END PGP SIGNATURE-----