You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by Gaurav Aradhye <ga...@clogeny.com> on 2014/02/12 17:09:08 UTC

Review Request 18016: CLOUDSTACK-5626: Simplifying VM Migrate code

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18016/
-----------------------------------------------------------

Review request for cloudstack, Girish Shilamkar and Santhosh Edukulla.


Bugs: CLOUDSTACK-5626
    https://issues.apache.org/jira/browse/CLOUDSTACK-5626


Repository: cloudstack-git


Description
-------

Removing different ways of obtaining suitable hosts. Used built in API "findHostsForMigration" and skipped the test if suitable host is not found instead of throwing assertion error.
Removed unnecessary common function.


Diffs
-----

  test/integration/component/test_cpu_domain_limits.py c427e4f 
  test/integration/component/test_cpu_limits.py bdf2869 
  test/integration/component/test_cpu_project_limits.py a8a1b3c 
  test/integration/component/test_memory_limits.py 7921e4b 
  test/integration/component/test_mm_domain_limits.py 68660c1 
  test/integration/component/test_mm_project_limits.py c314011 
  test/integration/component/test_vpc_vm_life_cycle.py 01373ac 
  tools/marvin/marvin/integration/lib/common.py e202391 

Diff: https://reviews.apache.org/r/18016/diff/


Testing
-------

Yes.


Thanks,

Gaurav Aradhye

Re: Review Request 18016: CLOUDSTACK-5626: Simplifying VM Migrate code

Posted by Gaurav Aradhye <ga...@clogeny.com>.

> On Feb. 12, 2014, 10:09 p.m., Santhosh Edukulla wrote:
> > test/integration/component/test_cpu_limits.py, line 580
> > <https://reviews.apache.org/r/18016/diff/1/?file=483529#file483529line580>
> >
> >     Do we need to skip the test or fail it?

It's not a failure since the setup is not equipped with required number of suitable hosts. So I think we should skip instead of failing.
And after each build we can look at the skipped tests to know why they were skipped.


> On Feb. 12, 2014, 10:09 p.m., Santhosh Edukulla wrote:
> > test/integration/component/test_cpu_project_limits.py, line 294
> > <https://reviews.apache.org/r/18016/diff/1/?file=483530#file483530line294>
> >
> >     Keep the function find_suitable_host, and use it test modules. Use the new api mentioned i.e., listForMigration there in that function and abstract that to test cases. This way, test modules dont change often.

Yes I thought so, but changed because anyway all modules needed change because the fail/skip step was not handled anywhere.
I will abstract out the function as you said, and will also add the fail/skip step in each test module according to result obtained from the function because we don't want to just raise exception from common library, that won't ensure graceful cleanup.

Your thoughts?


> On Feb. 12, 2014, 10:09 p.m., Santhosh Edukulla wrote:
> > test/integration/component/test_memory_limits.py, line 254
> > <https://reviews.apache.org/r/18016/diff/1/?file=483531#file483531line254>
> >
> >     Why modify all test suites? The better thing may be to use and abstract the listForMigration inside the common library and so with this no tests get changed?

As I said, all modules will need one line additional step, but for future test cases function abstraction will be beneficial.


- Gaurav


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18016/#review34291
-----------------------------------------------------------


On Feb. 12, 2014, 9:39 p.m., Gaurav Aradhye wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18016/
> -----------------------------------------------------------
> 
> (Updated Feb. 12, 2014, 9:39 p.m.)
> 
> 
> Review request for cloudstack, Girish Shilamkar and Santhosh Edukulla.
> 
> 
> Bugs: CLOUDSTACK-5626
>     https://issues.apache.org/jira/browse/CLOUDSTACK-5626
> 
> 
> Repository: cloudstack-git
> 
> 
> Description
> -------
> 
> Removing different ways of obtaining suitable hosts. Used built in API "findHostsForMigration" and skipped the test if suitable host is not found instead of throwing assertion error.
> Removed unnecessary common function.
> 
> 
> Diffs
> -----
> 
>   test/integration/component/test_cpu_domain_limits.py c427e4f 
>   test/integration/component/test_cpu_limits.py bdf2869 
>   test/integration/component/test_cpu_project_limits.py a8a1b3c 
>   test/integration/component/test_memory_limits.py 7921e4b 
>   test/integration/component/test_mm_domain_limits.py 68660c1 
>   test/integration/component/test_mm_project_limits.py c314011 
>   test/integration/component/test_vpc_vm_life_cycle.py 01373ac 
>   tools/marvin/marvin/integration/lib/common.py e202391 
> 
> Diff: https://reviews.apache.org/r/18016/diff/
> 
> 
> Testing
> -------
> 
> Yes.
> 
> 
> Thanks,
> 
> Gaurav Aradhye
> 
>

Re: Review Request 18016: CLOUDSTACK-5626: Simplifying VM Migrate code

Posted by Santhosh Edukulla <sa...@citrix.com>.

> On Feb. 12, 2014, 4:39 p.m., Santhosh Edukulla wrote:
> > test/integration/component/test_cpu_project_limits.py, line 294
> > <https://reviews.apache.org/r/18016/diff/1/?file=483530#file483530line294>
> >
> >     Keep the function find_suitable_host, and use it test modules. Use the new api mentioned i.e., listForMigration there in that function and abstract that to test cases. This way, test modules dont change often.
> 
> Gaurav Aradhye wrote:
>     Yes I thought so, but changed because anyway all modules needed change because the fail/skip step was not handled anywhere.
>     I will abstract out the function as you said, and will also add the fail/skip step in each test module according to result obtained from the function because we don't want to just raise exception from common library, that won't ensure graceful cleanup.
>     
>     Your thoughts?

please abstract it, that will be useful. Also, remote underscores and use suitable naming convention.


- Santhosh


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18016/#review34291
-----------------------------------------------------------


On Feb. 12, 2014, 4:09 p.m., Gaurav Aradhye wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18016/
> -----------------------------------------------------------
> 
> (Updated Feb. 12, 2014, 4:09 p.m.)
> 
> 
> Review request for cloudstack, Girish Shilamkar and Santhosh Edukulla.
> 
> 
> Bugs: CLOUDSTACK-5626
>     https://issues.apache.org/jira/browse/CLOUDSTACK-5626
> 
> 
> Repository: cloudstack-git
> 
> 
> Description
> -------
> 
> Removing different ways of obtaining suitable hosts. Used built in API "findHostsForMigration" and skipped the test if suitable host is not found instead of throwing assertion error.
> Removed unnecessary common function.
> 
> 
> Diffs
> -----
> 
>   test/integration/component/test_cpu_domain_limits.py c427e4f 
>   test/integration/component/test_cpu_limits.py bdf2869 
>   test/integration/component/test_cpu_project_limits.py a8a1b3c 
>   test/integration/component/test_memory_limits.py 7921e4b 
>   test/integration/component/test_mm_domain_limits.py 68660c1 
>   test/integration/component/test_mm_project_limits.py c314011 
>   test/integration/component/test_vpc_vm_life_cycle.py 01373ac 
>   tools/marvin/marvin/integration/lib/common.py e202391 
> 
> Diff: https://reviews.apache.org/r/18016/diff/
> 
> 
> Testing
> -------
> 
> Yes.
> 
> 
> Thanks,
> 
> Gaurav Aradhye
> 
>

Re: Review Request 18016: CLOUDSTACK-5626: Simplifying VM Migrate code

Posted by Gaurav Aradhye <ga...@clogeny.com>.

> On Feb. 12, 2014, 10:09 p.m., Santhosh Edukulla wrote:
> >

Santhosh, Can you please review this?


- Gaurav


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18016/#review34291
-----------------------------------------------------------


On Feb. 14, 2014, 1:24 p.m., Gaurav Aradhye wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18016/
> -----------------------------------------------------------
> 
> (Updated Feb. 14, 2014, 1:24 p.m.)
> 
> 
> Review request for cloudstack, Girish Shilamkar and Santhosh Edukulla.
> 
> 
> Bugs: CLOUDSTACK-5626
>     https://issues.apache.org/jira/browse/CLOUDSTACK-5626
> 
> 
> Repository: cloudstack-git
> 
> 
> Description
> -------
> 
> Removing different ways of obtaining suitable hosts. Used built in API "findHostsForMigration" and skipped the test if suitable host is not found instead of throwing assertion error.
> Removed unnecessary common function.
> 
> 
> Diffs
> -----
> 
>   test/integration/component/test_cpu_domain_limits.py c427e4f 
>   test/integration/component/test_cpu_limits.py bdf2869 
>   test/integration/component/test_cpu_project_limits.py a8a1b3c 
>   test/integration/component/test_memory_limits.py 7921e4b 
>   test/integration/component/test_mm_domain_limits.py 68660c1 
>   test/integration/component/test_mm_project_limits.py c314011 
>   test/integration/component/test_vpc_vm_life_cycle.py 01373ac 
>   tools/marvin/marvin/codes.py 74fb05d 
>   tools/marvin/marvin/integration/lib/common.py e202391 
> 
> Diff: https://reviews.apache.org/r/18016/diff/
> 
> 
> Testing
> -------
> 
> Yes.
> 
> 
> Thanks,
> 
> Gaurav Aradhye
> 
>

Re: Review Request 18016: CLOUDSTACK-5626: Simplifying VM Migrate code

Posted by Santhosh Edukulla <sa...@citrix.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18016/#review34291
-----------------------------------------------------------



test/integration/component/test_cpu_limits.py
<https://reviews.apache.org/r/18016/#comment64316>

    Do we need to skip the test or fail it?



test/integration/component/test_cpu_project_limits.py
<https://reviews.apache.org/r/18016/#comment64317>

    Keep the function find_suitable_host, and use it test modules. Use the new api mentioned i.e., listForMigration there in that function and abstract that to test cases. This way, test modules dont change often.



test/integration/component/test_memory_limits.py
<https://reviews.apache.org/r/18016/#comment64318>

    Why modify all test suites? The better thing may be to use and abstract the listForMigration inside the common library and so with this no tests get changed?


- Santhosh Edukulla


On Feb. 12, 2014, 4:09 p.m., Gaurav Aradhye wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18016/
> -----------------------------------------------------------
> 
> (Updated Feb. 12, 2014, 4:09 p.m.)
> 
> 
> Review request for cloudstack, Girish Shilamkar and Santhosh Edukulla.
> 
> 
> Bugs: CLOUDSTACK-5626
>     https://issues.apache.org/jira/browse/CLOUDSTACK-5626
> 
> 
> Repository: cloudstack-git
> 
> 
> Description
> -------
> 
> Removing different ways of obtaining suitable hosts. Used built in API "findHostsForMigration" and skipped the test if suitable host is not found instead of throwing assertion error.
> Removed unnecessary common function.
> 
> 
> Diffs
> -----
> 
>   test/integration/component/test_cpu_domain_limits.py c427e4f 
>   test/integration/component/test_cpu_limits.py bdf2869 
>   test/integration/component/test_cpu_project_limits.py a8a1b3c 
>   test/integration/component/test_memory_limits.py 7921e4b 
>   test/integration/component/test_mm_domain_limits.py 68660c1 
>   test/integration/component/test_mm_project_limits.py c314011 
>   test/integration/component/test_vpc_vm_life_cycle.py 01373ac 
>   tools/marvin/marvin/integration/lib/common.py e202391 
> 
> Diff: https://reviews.apache.org/r/18016/diff/
> 
> 
> Testing
> -------
> 
> Yes.
> 
> 
> Thanks,
> 
> Gaurav Aradhye
> 
>

Re: Review Request 18016: CLOUDSTACK-5626: Simplifying VM Migrate code

Posted by Santhosh Edukulla <sa...@citrix.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18016/#review34996
-----------------------------------------------------------

Ship it!


Ship It!

- Santhosh Edukulla


On Feb. 14, 2014, 7:54 a.m., Gaurav Aradhye wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18016/
> -----------------------------------------------------------
> 
> (Updated Feb. 14, 2014, 7:54 a.m.)
> 
> 
> Review request for cloudstack, Girish Shilamkar and Santhosh Edukulla.
> 
> 
> Bugs: CLOUDSTACK-5626
>     https://issues.apache.org/jira/browse/CLOUDSTACK-5626
> 
> 
> Repository: cloudstack-git
> 
> 
> Description
> -------
> 
> Removing different ways of obtaining suitable hosts. Used built in API "findHostsForMigration" and skipped the test if suitable host is not found instead of throwing assertion error.
> Removed unnecessary common function.
> 
> 
> Diffs
> -----
> 
>   test/integration/component/test_cpu_domain_limits.py c427e4f 
>   test/integration/component/test_cpu_limits.py bdf2869 
>   test/integration/component/test_cpu_project_limits.py a8a1b3c 
>   test/integration/component/test_memory_limits.py 7921e4b 
>   test/integration/component/test_mm_domain_limits.py 68660c1 
>   test/integration/component/test_mm_project_limits.py c314011 
>   test/integration/component/test_vpc_vm_life_cycle.py 01373ac 
>   tools/marvin/marvin/codes.py 74fb05d 
>   tools/marvin/marvin/integration/lib/common.py e202391 
> 
> Diff: https://reviews.apache.org/r/18016/diff/
> 
> 
> Testing
> -------
> 
> Yes.
> 
> 
> Thanks,
> 
> Gaurav Aradhye
> 
>

Re: Review Request 18016: CLOUDSTACK-5626: Simplifying VM Migrate code

Posted by ASF Subversion and Git Services <as...@urd.zones.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18016/#review36628
-----------------------------------------------------------


Commit 3b3ae024591fae4db9a6ca991d4838d98b96154e in cloudstack's branch refs/heads/master from Gaurav Aradhye
[ https://git-wip-us.apache.org/repos/asf?p=cloudstack.git;h=3b3ae02 ]

CLOUDSTACK-5626: Simplifying VM Migrate code

Signed-off-by: SrikanteswaraRao Talluri <ta...@apache.org>


- ASF Subversion and Git Services


On March 7, 2014, 9:36 a.m., Gaurav Aradhye wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18016/
> -----------------------------------------------------------
> 
> (Updated March 7, 2014, 9:36 a.m.)
> 
> 
> Review request for cloudstack, Girish Shilamkar, Santhosh Edukulla, and SrikanteswaraRao Talluri.
> 
> 
> Bugs: CLOUDSTACK-5626
>     https://issues.apache.org/jira/browse/CLOUDSTACK-5626
> 
> 
> Repository: cloudstack-git
> 
> 
> Description
> -------
> 
> Removing different ways of obtaining suitable hosts. Used built in API "findHostsForMigration" and skipped the test if suitable host is not found instead of throwing assertion error.
> Removed unnecessary common function.
> 
> 
> Diffs
> -----
> 
>   test/integration/component/test_cpu_domain_limits.py c427e4f 
>   test/integration/component/test_cpu_limits.py bdf2869 
>   test/integration/component/test_cpu_project_limits.py a8a1b3c 
>   test/integration/component/test_memory_limits.py 7921e4b 
>   test/integration/component/test_mm_domain_limits.py 68660c1 
>   test/integration/component/test_mm_project_limits.py c314011 
>   test/integration/component/test_vpc_vm_life_cycle.py 01373ac 
>   tools/marvin/marvin/codes.py 3882f0d 
>   tools/marvin/marvin/integration/lib/common.py b2da3ff 
> 
> Diff: https://reviews.apache.org/r/18016/diff/
> 
> 
> Testing
> -------
> 
> Yes.
> 
> 
> Thanks,
> 
> Gaurav Aradhye
> 
>

Re: Review Request 18016: CLOUDSTACK-5626: Simplifying VM Migrate code

Posted by Gaurav Aradhye <ga...@clogeny.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18016/
-----------------------------------------------------------

(Updated March 7, 2014, 3:06 p.m.)


Review request for cloudstack, Girish Shilamkar, Santhosh Edukulla, and SrikanteswaraRao Talluri.


Changes
-------

Rebased and tested with latest master. Talluri, can you please commit this?


Bugs: CLOUDSTACK-5626
    https://issues.apache.org/jira/browse/CLOUDSTACK-5626


Repository: cloudstack-git


Description
-------

Removing different ways of obtaining suitable hosts. Used built in API "findHostsForMigration" and skipped the test if suitable host is not found instead of throwing assertion error.
Removed unnecessary common function.


Diffs (updated)
-----

  test/integration/component/test_cpu_domain_limits.py c427e4f 
  test/integration/component/test_cpu_limits.py bdf2869 
  test/integration/component/test_cpu_project_limits.py a8a1b3c 
  test/integration/component/test_memory_limits.py 7921e4b 
  test/integration/component/test_mm_domain_limits.py 68660c1 
  test/integration/component/test_mm_project_limits.py c314011 
  test/integration/component/test_vpc_vm_life_cycle.py 01373ac 
  tools/marvin/marvin/codes.py 3882f0d 
  tools/marvin/marvin/integration/lib/common.py b2da3ff 

Diff: https://reviews.apache.org/r/18016/diff/


Testing
-------

Yes.


Thanks,

Gaurav Aradhye

Re: Review Request 18016: CLOUDSTACK-5626: Simplifying VM Migrate code

Posted by Gaurav Aradhye <ga...@clogeny.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18016/
-----------------------------------------------------------

(Updated Feb. 14, 2014, 1:24 p.m.)


Review request for cloudstack, Girish Shilamkar and Santhosh Edukulla.


Changes
-------

Review changes as suggested.


Bugs: CLOUDSTACK-5626
    https://issues.apache.org/jira/browse/CLOUDSTACK-5626


Repository: cloudstack-git


Description
-------

Removing different ways of obtaining suitable hosts. Used built in API "findHostsForMigration" and skipped the test if suitable host is not found instead of throwing assertion error.
Removed unnecessary common function.


Diffs (updated)
-----

  test/integration/component/test_cpu_domain_limits.py c427e4f 
  test/integration/component/test_cpu_limits.py bdf2869 
  test/integration/component/test_cpu_project_limits.py a8a1b3c 
  test/integration/component/test_memory_limits.py 7921e4b 
  test/integration/component/test_mm_domain_limits.py 68660c1 
  test/integration/component/test_mm_project_limits.py c314011 
  test/integration/component/test_vpc_vm_life_cycle.py 01373ac 
  tools/marvin/marvin/codes.py 74fb05d 
  tools/marvin/marvin/integration/lib/common.py e202391 

Diff: https://reviews.apache.org/r/18016/diff/


Testing
-------

Yes.


Thanks,

Gaurav Aradhye