On Tue, Mar 19, 2013 at 02:33:08PM +0000, Nitin Mehta wrote:
> Chip - Thanks for looking into this. Please find my answers inline.
>
> On 18/03/13 10:34 PM, "Chip Childers" <chip.child...@sungard.com> wrote:
>
> >Nitin,
> >
> >A couple of notes:
> >
> >1 - The license header is misplaced in ScaleVMCmd.java
>
> Corrected thanks.
>
No problem!
> >2 - Thanks for adding unit tests for ScaleVMCmd and UserVMManagerImpl
> >3 - I would have expected to see unit tests for the new methods in
> >CitrixResourceBase and VirtualMachineManagerImpl
>
> I have added unit tests for VirtualMachineManagerImpl but in
> CitrixResourceBase its just a resource call (here the Xenserver) and so I
> don't see
> benefit in writing a unit test. I am not adding unit test here.
>
I strongly disagree. I think Alex did a good job in his email about
unit testing [1] in describing why this matters. Take a specific method
that you added to CltrixResourceBase as an example:
protected void scaleVM(Connection conn, VM vm, VirtualMachineTO vmSpec,
Host host) throws XenAPIException, XmlRpcException {
vm.setMemoryDynamicRange(conn, vmSpec.getMinRam() * 1024 * 1024,
vmSpec.getMaxRam() * 1024 * 1024);
vm.setVCPUsNumberLive(conn, (long)vmSpec.getCpus());
Integer speed = vmSpec.getSpeed();
if (speed != null) {
int cpuWeight = _maxWeight; //cpu_weight
// weight based allocation
cpuWeight = (int)((speed*0.99) / _host.speed * _maxWeight);
if (cpuWeight > _maxWeight) {
cpuWeight = _maxWeight;
}
if (vmSpec.getLimitCpuUse()) {
long utilization = 0; // max CPU cap, default is unlimited
utilization = ((long)speed * 100 * vmSpec.getCpus()) /
_host.speed ;
vm.addToVCPUsParamsLive(conn, "cap",
Long.toString(utilization));
}
//vm.addToVCPUsParamsLive(conn, "weight",
Integer.toString(cpuWeight));
callHostPlugin(conn, "vmops", "add_to_VCPUs_params_live", "key",
"weight", "value", Integer.toString(cpuWeight), "vmname", vmSpec.getName() );
}
}
In this method alone, you are performing multiple calculations, you are
checking different aspects of the vmSpec to take different actions, and
you are then calling the remote resource to take action.
I'd agree that no tests were needed *if* this was only a proxy to call
into the remote resource, but it's not. You clearly have defined
expectations for results, depending on the vmSpec and and host
capabilities. This is a primary example of when I believe we need unit
tests.
[1] http://markmail.org/thread/rybpfx54ydybm7kx
> >4 - Should we have some marvin tests for this?
>
> We can but this is a fairly isolated functionality and I don't see much
> use in writing this. Nevertheless I would love to get my hands dirty for
> writing one. ATM though I would rather get the code out in the community
> and then do this work rather than holding the feature push. I am also a
> Python newbie so it will take some time.
I appreciate the desire to get code visible early and often. However, the
code *is* in the hands of the community, since it's available for all of
us to review. So no worries there!
Now as for the function itself, I don't understand your point about it
being "isolated functionality". Specifically, what does that have to do
with a need to add automated tests for the feature? Doesn't that
actually make it easier to implement the tests (and therefore more
likely for us to do it)?
I understand not knowing Python (and frankly, I'd revert back and say
that I'm much stronger at Python than Java), but now's the time to
learn. We've adopted the Marvin framework for testing, so let's all get
familiar with using it.
-chip
