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 2013/08/22 15:03:07 UTC
Review Request 13736: Resolved Cloudstack: 4452 - fixed test cases in
test_snapshots.py
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13736/
-----------------------------------------------------------
Review request for cloudstack and Prasanna Santhanam.
Repository: cloudstack-git
Description
-------
Resolved cloudstack: 4452
Changes in function is_snapshot_on_nfs to check if the snapshot is present in the secondary storage.
Earlier it was trying to match the name of the snapshot present on the storage with the UUID of the snapshot which is incorrect way.
There's apparently no relation between the snapshot name on the storage and the UUID of the snapshot.
Changed the function to check if the snapshotPath is valid and is indeed a "File".
Also removed the VM creation (virtual_machine_without_disk) step in setupClass of class TestSnapshots. It is not used anywhere and was eating up the running time of test case and resources.
Diffs
-----
test/integration/component/test_snapshots.py cc2e604
Diff: https://reviews.apache.org/r/13736/diff/
Testing
-------
Thanks,
Gaurav Aradhye
Re: Review Request 13736: Resolved Cloudstack: 4452 - fixed test cases in
test_snapshots.py
Posted by Prasanna Santhanam <ts...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13736/#review25459
-----------------------------------------------------------
Gaurav, there's more occurrences of is_snapshot_on_nfs in the same suite and in two other snapshot related suites. Can you please fix those too? If it makes sense make this a utility method. I can see that we have to revisit this test when run with an object store for secondary storage. So it would be wise to generalize it at this point.
- Prasanna Santhanam
On Aug. 22, 2013, 1:03 p.m., Gaurav Aradhye wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13736/
> -----------------------------------------------------------
>
> (Updated Aug. 22, 2013, 1:03 p.m.)
>
>
> Review request for cloudstack and Prasanna Santhanam.
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> Resolved cloudstack: 4452
> Changes in function is_snapshot_on_nfs to check if the snapshot is present in the secondary storage.
>
> Earlier it was trying to match the name of the snapshot present on the storage with the UUID of the snapshot which is incorrect way.
> There's apparently no relation between the snapshot name on the storage and the UUID of the snapshot.
>
> Changed the function to check if the snapshotPath is valid and is indeed a "File".
>
> Also removed the VM creation (virtual_machine_without_disk) step in setupClass of class TestSnapshots. It is not used anywhere and was eating up the running time of test case and resources.
>
>
> Diffs
> -----
>
> test/integration/component/test_snapshots.py cc2e604
>
> Diff: https://reviews.apache.org/r/13736/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Gaurav Aradhye
>
>
Re: Review Request 13736: Resolved Cloudstack: 4452 - fixed test cases in
test_snapshots.py
Posted by Prasanna Santhanam <ts...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13736/#review25538
-----------------------------------------------------------
Ship it!
Applied to master and 4.2-forward but with additional changes that I had requested in the review.
- Prasanna Santhanam
On Aug. 23, 2013, 11:52 a.m., Gaurav Aradhye wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13736/
> -----------------------------------------------------------
>
> (Updated Aug. 23, 2013, 11:52 a.m.)
>
>
> Review request for cloudstack and Prasanna Santhanam.
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> Resolved cloudstack: 4452
> Changes in function is_snapshot_on_nfs to check if the snapshot is present in the secondary storage.
>
> Earlier it was trying to match the name of the snapshot present on the storage with the UUID of the snapshot which is incorrect way.
> There's apparently no relation between the snapshot name on the storage and the UUID of the snapshot.
>
> Changed the function to check if the snapshotPath is valid and is indeed a "File".
>
> Also removed the VM creation (virtual_machine_without_disk) step in setupClass of class TestSnapshots. It is not used anywhere and was eating up the running time of test case and resources.
>
>
> Diffs
> -----
>
> test/integration/component/test_snapshot_gc.py 3cd4194
> test/integration/component/test_snapshot_limits.py 1bc2798
> test/integration/component/test_snapshots.py cc2e604
> tools/marvin/marvin/integration/lib/utils.py 7863bfb
>
> Diff: https://reviews.apache.org/r/13736/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Gaurav Aradhye
>
>
Re: Review Request 13736: Resolved Cloudstack: 4452 - fixed test cases in
test_snapshots.py
Posted by Gaurav Aradhye <ga...@clogeny.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13736/
-----------------------------------------------------------
(Updated Aug. 23, 2013, 11:52 a.m.)
Review request for cloudstack and Prasanna Santhanam.
Changes
-------
Removed print statements.
This also includes the fix for 4472. Will discard separate patch added for 4472.
Repository: cloudstack-git
Description
-------
Resolved cloudstack: 4452
Changes in function is_snapshot_on_nfs to check if the snapshot is present in the secondary storage.
Earlier it was trying to match the name of the snapshot present on the storage with the UUID of the snapshot which is incorrect way.
There's apparently no relation between the snapshot name on the storage and the UUID of the snapshot.
Changed the function to check if the snapshotPath is valid and is indeed a "File".
Also removed the VM creation (virtual_machine_without_disk) step in setupClass of class TestSnapshots. It is not used anywhere and was eating up the running time of test case and resources.
Diffs (updated)
-----
test/integration/component/test_snapshot_gc.py 3cd4194
test/integration/component/test_snapshot_limits.py 1bc2798
test/integration/component/test_snapshots.py cc2e604
tools/marvin/marvin/integration/lib/utils.py 7863bfb
Diff: https://reviews.apache.org/r/13736/diff/
Testing
-------
Thanks,
Gaurav Aradhye
Re: Review Request 13736: Resolved Cloudstack: 4452 - fixed test cases in
test_snapshots.py
Posted by Prasanna Santhanam <ts...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13736/#review25465
-----------------------------------------------------------
dir_paths should not be a required argument. You can simply mount the secondary nfs path on /mnt/tmp after you've created that directory and unmount at the end of the test. Also please document the additional arguments in the method's docstring. Rest of the change looks good. thanks
- Prasanna Santhanam
On Aug. 23, 2013, 10:03 a.m., Gaurav Aradhye wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13736/
> -----------------------------------------------------------
>
> (Updated Aug. 23, 2013, 10:03 a.m.)
>
>
> Review request for cloudstack and Prasanna Santhanam.
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> Resolved cloudstack: 4452
> Changes in function is_snapshot_on_nfs to check if the snapshot is present in the secondary storage.
>
> Earlier it was trying to match the name of the snapshot present on the storage with the UUID of the snapshot which is incorrect way.
> There's apparently no relation between the snapshot name on the storage and the UUID of the snapshot.
>
> Changed the function to check if the snapshotPath is valid and is indeed a "File".
>
> Also removed the VM creation (virtual_machine_without_disk) step in setupClass of class TestSnapshots. It is not used anywhere and was eating up the running time of test case and resources.
>
>
> Diffs
> -----
>
> test/integration/component/test_snapshot_gc.py 3cd4194
> test/integration/component/test_snapshot_limits.py 1bc2798
> test/integration/component/test_snapshots.py cc2e604
> tools/marvin/marvin/integration/lib/utils.py 7863bfb
>
> Diff: https://reviews.apache.org/r/13736/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Gaurav Aradhye
>
>
Re: Review Request 13736: Resolved Cloudstack: 4452 - fixed test cases in
test_snapshots.py
Posted by Prasanna Santhanam <ts...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13736/#review25466
-----------------------------------------------------------
Also please remove the 'print' statements as they won't show in the testrunner
- Prasanna Santhanam
On Aug. 23, 2013, 10:03 a.m., Gaurav Aradhye wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13736/
> -----------------------------------------------------------
>
> (Updated Aug. 23, 2013, 10:03 a.m.)
>
>
> Review request for cloudstack and Prasanna Santhanam.
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> Resolved cloudstack: 4452
> Changes in function is_snapshot_on_nfs to check if the snapshot is present in the secondary storage.
>
> Earlier it was trying to match the name of the snapshot present on the storage with the UUID of the snapshot which is incorrect way.
> There's apparently no relation between the snapshot name on the storage and the UUID of the snapshot.
>
> Changed the function to check if the snapshotPath is valid and is indeed a "File".
>
> Also removed the VM creation (virtual_machine_without_disk) step in setupClass of class TestSnapshots. It is not used anywhere and was eating up the running time of test case and resources.
>
>
> Diffs
> -----
>
> test/integration/component/test_snapshot_gc.py 3cd4194
> test/integration/component/test_snapshot_limits.py 1bc2798
> test/integration/component/test_snapshots.py cc2e604
> tools/marvin/marvin/integration/lib/utils.py 7863bfb
>
> Diff: https://reviews.apache.org/r/13736/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Gaurav Aradhye
>
>
Re: Review Request 13736: Resolved Cloudstack: 4452 - fixed test cases in
test_snapshots.py
Posted by Gaurav Aradhye <ga...@clogeny.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13736/
-----------------------------------------------------------
(Updated Aug. 23, 2013, 10:03 a.m.)
Review request for cloudstack and Prasanna Santhanam.
Changes
-------
Updated the patch. Added the function to utility and made respective changes in all snapshot related test suites
Repository: cloudstack-git
Description
-------
Resolved cloudstack: 4452
Changes in function is_snapshot_on_nfs to check if the snapshot is present in the secondary storage.
Earlier it was trying to match the name of the snapshot present on the storage with the UUID of the snapshot which is incorrect way.
There's apparently no relation between the snapshot name on the storage and the UUID of the snapshot.
Changed the function to check if the snapshotPath is valid and is indeed a "File".
Also removed the VM creation (virtual_machine_without_disk) step in setupClass of class TestSnapshots. It is not used anywhere and was eating up the running time of test case and resources.
Diffs (updated)
-----
test/integration/component/test_snapshot_gc.py 3cd4194
test/integration/component/test_snapshot_limits.py 1bc2798
test/integration/component/test_snapshots.py cc2e604
tools/marvin/marvin/integration/lib/utils.py 7863bfb
Diff: https://reviews.apache.org/r/13736/diff/
Testing
-------
Thanks,
Gaurav Aradhye