Review Request 12720: CLOUDSTACK: 3382 Unable to Migrate VM's If the hosts are implicitly or explicitly dedicated.

[Saksham Srivastava]

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

Review request for cloudstack and Devdeep Singh.


Bugs: 3382


Repository: cloudstack-git


Description
---

Allow root admin to migrate VMs across dedicated hosts.
Generate alerts for each migration performed between dedicated hosts.
Alerts are generated for VM migration between:
1) Source host is dedicated and destination host is not.
2) Source host is not dedicated and destination host is dedicated.
3) Both hosts are dedicated.
Alerts are now generated for both explicit and implicit dedication.


Diffs
-

  server/src/com/cloud/deploy/dao/PlannerHostReservationDao.java 69118f1 
  server/src/com/cloud/deploy/dao/PlannerHostReservationDaoImpl.java 41e0964 
  server/src/com/cloud/vm/UserVmManagerImpl.java 9968690 

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


Testing
---

Migrated VMs across explicitly dedicated hosts and alerts are generated.
Migrated VMs across implicitly dedicated hosts and alerts are generated.
 


Thanks,

Saksham Srivastava

Re: Review Request 12720: CLOUDSTACK: 3382 Unable to Migrate VM's If the hosts are implicitly or explicitly dedicated.

[Prasanna Santhanam]

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


Do you think you can add a integration test to test_explicit_dedication.py for 
this failure? That would be really useful to catch this part of tricky code

- Prasanna Santhanam


On July 18, 2013, 11:06 a.m., Saksham Srivastava wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12720/
> ---
> 
> (Updated July 18, 2013, 11:06 a.m.)
> 
> 
> Review request for cloudstack and Devdeep Singh.
> 
> 
> Bugs: 3382
> 
> 
> Repository: cloudstack-git
> 
> 
> Description
> ---
> 
> Allow root admin to migrate VMs across dedicated hosts.
> Generate alerts for each migration performed between dedicated hosts.
> Alerts are generated for VM migration between:
> 1) Source host is dedicated and destination host is not.
> 2) Source host is not dedicated and destination host is dedicated.
> 3) Both hosts are dedicated.
> Alerts are now generated for both explicit and implicit dedication.
> 
> 
> Diffs
> -
> 
>   server/src/com/cloud/deploy/dao/PlannerHostReservationDao.java 69118f1 
>   server/src/com/cloud/deploy/dao/PlannerHostReservationDaoImpl.java 41e0964 
>   server/src/com/cloud/vm/UserVmManagerImpl.java 9968690 
> 
> Diff: https://reviews.apache.org/r/12720/diff/
> 
> 
> Testing
> ---
> 
> Migrated VMs across explicitly dedicated hosts and alerts are generated.
> Migrated VMs across implicitly dedicated hosts and alerts are generated.
>  
> 
> 
> Thanks,
> 
> Saksham Srivastava
> 
>

Re: Review Request 12720: CLOUDSTACK: 3382 Unable to Migrate VM's If the hosts are implicitly or explicitly dedicated.

[Devdeep Singh]

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


Few comments:
1. You need to handle when migration with storage is taking place. That isn't 
handled right now.
2. The check to raise alerts should be abstracted into a function.
3. In case of implicit the scenario when a vm is migrated from a dedicated host 
of one account to dedicated host of another account isn't handled.

- Devdeep Singh


On July 18, 2013, 11:06 a.m., Saksham Srivastava wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12720/
> ---
> 
> (Updated July 18, 2013, 11:06 a.m.)
> 
> 
> Review request for cloudstack and Devdeep Singh.
> 
> 
> Bugs: 3382
> 
> 
> Repository: cloudstack-git
> 
> 
> Description
> ---
> 
> Allow root admin to migrate VMs across dedicated hosts.
> Generate alerts for each migration performed between dedicated hosts.
> Alerts are generated for VM migration between:
> 1) Source host is dedicated and destination host is not.
> 2) Source host is not dedicated and destination host is dedicated.
> 3) Both hosts are dedicated.
> Alerts are now generated for both explicit and implicit dedication.
> 
> 
> Diffs
> -
> 
>   server/src/com/cloud/deploy/dao/PlannerHostReservationDao.java 69118f1 
>   server/src/com/cloud/deploy/dao/PlannerHostReservationDaoImpl.java 41e0964 
>   server/src/com/cloud/vm/UserVmManagerImpl.java 9968690 
> 
> Diff: https://reviews.apache.org/r/12720/diff/
> 
> 
> Testing
> ---
> 
> Migrated VMs across explicitly dedicated hosts and alerts are generated.
> Migrated VMs across implicitly dedicated hosts and alerts are generated.
>  
> 
> 
> Thanks,
> 
> Saksham Srivastava
> 
>

Re: Review Request 12720: CLOUDSTACK: 3382 Unable to Migrate VM's If the hosts are implicitly or explicitly dedicated.

[Saksham Srivastava]

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

(Updated July 19, 2013, 11:52 a.m.)


Review request for cloudstack and Devdeep Singh.


Changes
---

Updated diff.


Bugs: 3382


Repository: cloudstack-git


Description
---

Allow root admin to migrate VMs across dedicated hosts.
Generate alerts for each migration performed between dedicated hosts.
Alerts are generated for VM migration between:
1) Source host is dedicated and destination host is not.
2) Source host is not dedicated and destination host is dedicated.
3) Both hosts are dedicated.
Alerts are now generated for both explicit and implicit dedication.


Diffs (updated)
-

  server/src/com/cloud/deploy/dao/PlannerHostReservationDao.java 69118f1 
  server/src/com/cloud/deploy/dao/PlannerHostReservationDaoImpl.java 41e0964 
  server/src/com/cloud/vm/UserVmManagerImpl.java 9968690 

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


Testing
---

Migrated VMs across explicitly dedicated hosts and alerts are generated.
Migrated VMs across implicitly dedicated hosts and alerts are generated.
 


Thanks,

Saksham Srivastava

Re: Review Request 12720: CLOUDSTACK: 3382 Unable to Migrate VM's If the hosts are implicitly or explicitly dedicated.

[Saksham Srivastava]

> On July 18, 2013, 11:09 a.m., Prasanna Santhanam wrote:
> > Do you think you can add a integration test to test_explicit_dedication.py 
> > for this failure? That would be really useful to catch this part of tricky 
> > code

Prasanna the changes I have made are specifically related to alert generation, 
the original functionality is kept unchanged.
Do you suggest to additional test that checks for alert generation ?


- Saksham


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


On July 19, 2013, 11:52 a.m., Saksham Srivastava wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12720/
> ---
> 
> (Updated July 19, 2013, 11:52 a.m.)
> 
> 
> Review request for cloudstack and Devdeep Singh.
> 
> 
> Bugs: 3382
> 
> 
> Repository: cloudstack-git
> 
> 
> Description
> ---
> 
> Allow root admin to migrate VMs across dedicated hosts.
> Generate alerts for each migration performed between dedicated hosts.
> Alerts are generated for VM migration between:
> 1) Source host is dedicated and destination host is not.
> 2) Source host is not dedicated and destination host is dedicated.
> 3) Both hosts are dedicated.
> Alerts are now generated for both explicit and implicit dedication.
> 
> 
> Diffs
> -
> 
>   server/src/com/cloud/deploy/dao/PlannerHostReservationDao.java 69118f1 
>   server/src/com/cloud/deploy/dao/PlannerHostReservationDaoImpl.java 41e0964 
>   server/src/com/cloud/vm/UserVmManagerImpl.java 9968690 
> 
> Diff: https://reviews.apache.org/r/12720/diff/
> 
> 
> Testing
> ---
> 
> Migrated VMs across explicitly dedicated hosts and alerts are generated.
> Migrated VMs across implicitly dedicated hosts and alerts are generated.
>  
> 
> 
> Thanks,
> 
> Saksham Srivastava
> 
>

Re: Review Request 12720: CLOUDSTACK: 3382 Unable to Migrate VM's If the hosts are implicitly or explicitly dedicated.

[Devdeep Singh]

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



server/src/com/cloud/vm/UserVmManagerImpl.java


Why not return a Long, that we, we don't need to do a -1 check.



server/src/com/cloud/vm/UserVmManagerImpl.java


Same comment as above



server/src/com/cloud/vm/UserVmManagerImpl.java


We are calling checkIfHostIsDedicated on src and destination multiple time. 
It'll be better to call it once and save the information and to do all the 
checks.



server/src/com/cloud/vm/UserVmManagerImpl.java


If the destination has vm in strict mode for this account, then we 
shouldn't be raising an alert. This is happening now.



server/src/com/cloud/vm/UserVmManagerImpl.java


Why not just check if the given destination host is dedicated. We shouldn't 
be required to list all dedicated hosts and parse through them to check if 
destination is dedicated.


- Devdeep Singh


On July 19, 2013, 11:52 a.m., Saksham Srivastava wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12720/
> ---
> 
> (Updated July 19, 2013, 11:52 a.m.)
> 
> 
> Review request for cloudstack and Devdeep Singh.
> 
> 
> Bugs: 3382
> 
> 
> Repository: cloudstack-git
> 
> 
> Description
> ---
> 
> Allow root admin to migrate VMs across dedicated hosts.
> Generate alerts for each migration performed between dedicated hosts.
> Alerts are generated for VM migration between:
> 1) Source host is dedicated and destination host is not.
> 2) Source host is not dedicated and destination host is dedicated.
> 3) Both hosts are dedicated.
> Alerts are now generated for both explicit and implicit dedication.
> 
> 
> Diffs
> -
> 
>   server/src/com/cloud/deploy/dao/PlannerHostReservationDao.java 69118f1 
>   server/src/com/cloud/deploy/dao/PlannerHostReservationDaoImpl.java 41e0964 
>   server/src/com/cloud/vm/UserVmManagerImpl.java 9968690 
> 
> Diff: https://reviews.apache.org/r/12720/diff/
> 
> 
> Testing
> ---
> 
> Migrated VMs across explicitly dedicated hosts and alerts are generated.
> Migrated VMs across implicitly dedicated hosts and alerts are generated.
>  
> 
> 
> Thanks,
> 
> Saksham Srivastava
> 
>

Re: Review Request 12720: CLOUDSTACK: 3382 Unable to Migrate VM's If the hosts are implicitly or explicitly dedicated.

[Jenkins Cloudstack.org]

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


Review 12720 failed the build test : FAILURE
The url of build cloudstack-master-with-patch #2 is : 
http://jenkins.cloudstack.org/job/cloudstack-master-with-patch/2/

- Jenkins Cloudstack.org


On July 19, 2013, 11:52 a.m., Saksham Srivastava wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12720/
> ---
> 
> (Updated July 19, 2013, 11:52 a.m.)
> 
> 
> Review request for cloudstack and Devdeep Singh.
> 
> 
> Bugs: 3382
> 
> 
> Repository: cloudstack-git
> 
> 
> Description
> ---
> 
> Allow root admin to migrate VMs across dedicated hosts.
> Generate alerts for each migration performed between dedicated hosts.
> Alerts are generated for VM migration between:
> 1) Source host is dedicated and destination host is not.
> 2) Source host is not dedicated and destination host is dedicated.
> 3) Both hosts are dedicated.
> Alerts are now generated for both explicit and implicit dedication.
> 
> 
> Diffs
> -
> 
>   server/src/com/cloud/deploy/dao/PlannerHostReservationDao.java 69118f1 
>   server/src/com/cloud/deploy/dao/PlannerHostReservationDaoImpl.java 41e0964 
>   server/src/com/cloud/vm/UserVmManagerImpl.java 9968690 
> 
> Diff: https://reviews.apache.org/r/12720/diff/
> 
> 
> Testing
> ---
> 
> Migrated VMs across explicitly dedicated hosts and alerts are generated.
> Migrated VMs across implicitly dedicated hosts and alerts are generated.
>  
> 
> 
> Thanks,
> 
> Saksham Srivastava
> 
>

Re: Review Request 12720: CLOUDSTACK: 3382 Unable to Migrate VM's If the hosts are implicitly or explicitly dedicated.

[Saksham Srivastava]

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

(Updated July 22, 2013, 11:28 a.m.)


Review request for cloudstack and Devdeep Singh.


Bugs: 3382


Repository: cloudstack-git


Description
---

Allow root admin to migrate VMs across dedicated hosts.
Generate alerts for each migration performed between dedicated hosts.
Alerts are generated for VM migration between:
1) Source host is dedicated and destination host is not.
2) Source host is not dedicated and destination host is dedicated.
3) Both hosts are dedicated.
Alerts are now generated for both explicit and implicit dedication.


Diffs
-

  server/src/com/cloud/deploy/dao/PlannerHostReservationDao.java 69118f1 
  server/src/com/cloud/deploy/dao/PlannerHostReservationDaoImpl.java 41e0964 
  server/src/com/cloud/vm/UserVmManagerImpl.java 9968690 

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


Testing
---

Migrated VMs across explicitly dedicated hosts and alerts are generated.
Migrated VMs across implicitly dedicated hosts and alerts are generated.
 


Thanks,

Saksham Srivastava

Re: Review Request 12720: CLOUDSTACK: 3382 Unable to Migrate VM's If the hosts are implicitly or explicitly dedicated.

[Saksham Srivastava]

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

(Updated July 22, 2013, 11:28 a.m.)


Review request for cloudstack and Devdeep Singh.


Changes
---

Updated diff.


Bugs: 3382


Repository: cloudstack-git


Description
---

Allow root admin to migrate VMs across dedicated hosts.
Generate alerts for each migration performed between dedicated hosts.
Alerts are generated for VM migration between:
1) Source host is dedicated and destination host is not.
2) Source host is not dedicated and destination host is dedicated.
3) Both hosts are dedicated.
Alerts are now generated for both explicit and implicit dedication.


Diffs (updated)
-

  server/src/com/cloud/deploy/dao/PlannerHostReservationDao.java 69118f1 
  server/src/com/cloud/deploy/dao/PlannerHostReservationDaoImpl.java 41e0964 
  server/src/com/cloud/vm/UserVmManagerImpl.java 9968690 

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


Testing
---

Migrated VMs across explicitly dedicated hosts and alerts are generated.
Migrated VMs across implicitly dedicated hosts and alerts are generated.
 


Thanks,

Saksham Srivastava

Re: Review Request 12720: CLOUDSTACK: 3382 Unable to Migrate VM's If the hosts are implicitly or explicitly dedicated.

[Devdeep Singh]

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

Ship it!


Committed to 4.2 and master in commits e2f2bc5f0 and 89b94bb75 respectively.

- Devdeep Singh


On July 22, 2013, 11:28 a.m., Saksham Srivastava wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12720/
> ---
> 
> (Updated July 22, 2013, 11:28 a.m.)
> 
> 
> Review request for cloudstack and Devdeep Singh.
> 
> 
> Bugs: 3382
> 
> 
> Repository: cloudstack-git
> 
> 
> Description
> ---
> 
> Allow root admin to migrate VMs across dedicated hosts.
> Generate alerts for each migration performed between dedicated hosts.
> Alerts are generated for VM migration between:
> 1) Source host is dedicated and destination host is not.
> 2) Source host is not dedicated and destination host is dedicated.
> 3) Both hosts are dedicated.
> Alerts are now generated for both explicit and implicit dedication.
> 
> 
> Diffs
> -
> 
>   server/src/com/cloud/deploy/dao/PlannerHostReservationDao.java 69118f1 
>   server/src/com/cloud/deploy/dao/PlannerHostReservationDaoImpl.java 41e0964 
>   server/src/com/cloud/vm/UserVmManagerImpl.java 9968690 
> 
> Diff: https://reviews.apache.org/r/12720/diff/
> 
> 
> Testing
> ---
> 
> Migrated VMs across explicitly dedicated hosts and alerts are generated.
> Migrated VMs across implicitly dedicated hosts and alerts are generated.
>  
> 
> 
> Thanks,
> 
> Saksham Srivastava
> 
>