You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by Nitin Mehta <Ni...@citrix.com> on 2013/03/18 17:10:25 UTC
[MERGE] scaleupvm functionality branch to master
I would like to merge scale up vm functionality branch to master
Spec :
https://cwiki.apache.org/confluence/display/CLOUDSTACK/Dynamic+scaling+of+CPU+and+RAM
JIRA ticket :
https://issues.apache.org/jira/browse/CLOUDSTACK-658
Branch:
Scaleupvm
Notes:-
1. I have removed the vm state change in the code and so there is no vm state machine change. I am planning to achieve the synchronization using async job framework. I will make this change once I get a reply from Kelvn (minor change)
2. With the changes in #1 this feature is independent and should not be intrusive to any functionality.
Thanks,
-Nitin
RE: [MERGE] scaleupvm functionality branch to master
Posted by Sudha Ponnaganti <su...@citrix.com>.
Thanks Nitin for Marvin Tests. Girish is going to extend the initial set of Marvin tests in due course.
Prashanth - pl do check the test results in subsequent automation runs.
I have crated Automation task and assigned it to Girish.
Thanks
/Sudha
-----Original Message-----
From: Nitin Mehta [mailto:Nitin.Mehta@citrix.com]
Sent: Thursday, March 28, 2013 8:03 AM
To: dev@cloudstack.apache.org
Subject: Re: [MERGE] scaleupvm functionality branch to master
I have merged the feature with commit below.
commit 3e4430d811d5e148c4fef1900bdd8dd07a38bdeb
Author: Nitin Mehta <ni...@citrix.com>
Date: Thu Mar 28 16:43:37 2013 +0530
CLOUDSTACK-658 - Scale vm support for Xenserver
Added the framweork so that it can be extended for vmware and kvm as well.
Added unitests and marvin tests.
On 23/03/13 1:06 AM, "Chip Childers" <ch...@sungard.com> wrote:
>On Fri, Mar 22, 2013 at 06:35:02AM +0000, Nitin Mehta wrote:
>> Alright Chip, I have pushed in the unit tests for CitrixResourceBase
>> and will work with Prasanna to learn writing the marvin tests and
>> will push them shortly as well.
>
>Great!
>
>> Lets please merge the feature as I think the unit tests should be
>>good as first line of defense. I assure you that I will push the
>>marvin tests soon as well.
>
>I'm OK with that, if others are.
Re: [MERGE] scaleupvm functionality branch to master
Posted by Nitin Mehta <Ni...@citrix.com>.
I have merged the feature with commit below.
commit 3e4430d811d5e148c4fef1900bdd8dd07a38bdeb
Author: Nitin Mehta <ni...@citrix.com>
Date: Thu Mar 28 16:43:37 2013 +0530
CLOUDSTACK-658 - Scale vm support for Xenserver
Added the framweork so that it can be extended for vmware and kvm as
well.
Added unitests and marvin tests.
On 23/03/13 1:06 AM, "Chip Childers" <ch...@sungard.com> wrote:
>On Fri, Mar 22, 2013 at 06:35:02AM +0000, Nitin Mehta wrote:
>> Alright Chip, I have pushed in the unit tests for CitrixResourceBase and
>> will work with Prasanna to learn writing the marvin tests and will push
>> them shortly as well.
>
>Great!
>
>> Lets please merge the feature as I think the unit tests should be good
>>as
>> first line of defense. I assure you that I will push the marvin tests
>>soon
>> as well.
>
>I'm OK with that, if others are.
Re: [MERGE] scaleupvm functionality branch to master
Posted by Chip Childers <ch...@sungard.com>.
On Fri, Mar 22, 2013 at 06:35:02AM +0000, Nitin Mehta wrote:
> Alright Chip, I have pushed in the unit tests for CitrixResourceBase and
> will work with Prasanna to learn writing the marvin tests and will push
> them shortly as well.
Great!
> Lets please merge the feature as I think the unit tests should be good as
> first line of defense. I assure you that I will push the marvin tests soon
> as well.
I'm OK with that, if others are.
Re: [MERGE] scaleupvm functionality branch to master
Posted by Nitin Mehta <Ni...@citrix.com>.
Alright Chip, I have pushed in the unit tests for CitrixResourceBase and
will work with Prasanna to learn writing the marvin tests and will push
them shortly as well.
Lets please merge the feature as I think the unit tests should be good as
first line of defense. I assure you that I will push the marvin tests soon
as well.
Thanks,
-Nitin
On 19/03/13 8:37 PM, "Chip Childers" <ch...@sungard.com> wrote:
>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" <ch...@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
Re: [MERGE] scaleupvm functionality branch to master
Posted by Chip Childers <ch...@sungard.com>.
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" <ch...@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
Re: [MERGE] scaleupvm functionality branch to master
Posted by Nitin Mehta <Ni...@citrix.com>.
Chip - Thanks for looking into this. Please find my answers inline.
On 18/03/13 10:34 PM, "Chip Childers" <ch...@sungard.com> wrote:
>On Mon, Mar 18, 2013 at 04:10:25PM +0000, Nitin Mehta wrote:
>> I would like to merge scale up vm functionality branch to master
>>
>> Spec :
>>
>>https://cwiki.apache.org/confluence/display/CLOUDSTACK/Dynamic+scaling+of
>>+CPU+and+RAM
>>
>> JIRA ticket :
>> https://issues.apache.org/jira/browse/CLOUDSTACK-658
>>
>> Branch:
>> Scaleupvm
>>
>> Notes:-
>>
>> 1. I have removed the vm state change in the code and so there is no
>>vm state machine change. I am planning to achieve the synchronization
>>using async job framework. I will make this change once I get a reply
>>from Kelvn (minor change)
>> 2. With the changes in #1 this feature is independent and should not
>>be intrusive to any functionality.
>>
>> Thanks,
>> -Nitin
>
>Nitin,
>
>A couple of notes:
>
>1 - The license header is misplaced in ScaleVMCmd.java
Corrected thanks.
>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.
>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.
>
Re: [MERGE] scaleupvm functionality branch to master
Posted by Chip Childers <ch...@sungard.com>.
On Mon, Mar 18, 2013 at 04:10:25PM +0000, Nitin Mehta wrote:
> I would like to merge scale up vm functionality branch to master
>
> Spec :
> https://cwiki.apache.org/confluence/display/CLOUDSTACK/Dynamic+scaling+of+CPU+and+RAM
>
> JIRA ticket :
> https://issues.apache.org/jira/browse/CLOUDSTACK-658
>
> Branch:
> Scaleupvm
>
> Notes:-
>
> 1. I have removed the vm state change in the code and so there is no vm state machine change. I am planning to achieve the synchronization using async job framework. I will make this change once I get a reply from Kelvn (minor change)
> 2. With the changes in #1 this feature is independent and should not be intrusive to any functionality.
>
> Thanks,
> -Nitin
Nitin,
A couple of notes:
1 - The license header is misplaced in ScaleVMCmd.java
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
4 - Should we have some marvin tests for this?
RE: [MERGE] scaleupvm functionality branch to master
Posted by Edison Su <Ed...@citrix.com>.
Happened to take a look at virtualmachinemanagerimpl on master, during reviewing storage migration code. Just want to know, what's the difference between migrate and migrateforscale in VirtualmachineManagerImpl? Is it necessary to duplicate code?
> -----Original Message-----
> From: Nitin Mehta [mailto:Nitin.Mehta@citrix.com]
> Sent: Monday, March 18, 2013 9:10 AM
> To: cloudstack-dev@incubator.apache.org
> Subject: [MERGE] scaleupvm functionality branch to master
>
> I would like to merge scale up vm functionality branch to master
>
> Spec :
> https://cwiki.apache.org/confluence/display/CLOUDSTACK/Dynamic+scaling
> +of+CPU+and+RAM
>
> JIRA ticket :
> https://issues.apache.org/jira/browse/CLOUDSTACK-658
>
> Branch:
> Scaleupvm
>
> Notes:-
>
> 1. I have removed the vm state change in the code and so there is no vm
> state machine change. I am planning to achieve the synchronization using
> async job framework. I will make this change once I get a reply from Kelvn
> (minor change)
> 2. With the changes in #1 this feature is independent and should not be
> intrusive to any functionality.
>
> Thanks,
> -Nitin