[GitHub] cloudstack pull request #1953: CLOUDSTACK-9794: Unable to attach more than 1...

2017-03-13 Thread asfgit 
Github user asfgit closed the pull request at:

https://github.com/apache/cloudstack/pull/1953


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1953: CLOUDSTACK-9794: Unable to attach more than 1...

2017-02-22 Thread sureshanaparti 
Github user sureshanaparti commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1953#discussion_r102539843
  
--- Diff: 
plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java 
---
@@ -584,18 +584,36 @@ public void defFileBasedDisk(String filePath, String 
diskLabel, DiskBus bus, Dis
 
 /* skip iso label */
 private String getDevLabel(int devId, DiskBus bus) {
--- End diff --

@HrWiggles Will add unit tests.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1953: CLOUDSTACK-9794: Unable to attach more than 1...

2017-02-22 Thread sureshanaparti 
Github user sureshanaparti commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1953#discussion_r102539356
  
--- Diff: server/src/com/cloud/storage/VolumeApiServiceImpl.java ---
@@ -2639,22 +2639,23 @@ private int getMaxDataVolumesSupported(UserVmVO vm) 
{
 return maxDataVolumesSupported.intValue();
 }
 
-private Long getDeviceId(long vmId, Long deviceId) {
+private Long getDeviceId(UserVmVO vm, Long deviceId) {
 // allocate deviceId
-List vols = _volsDao.findByInstance(vmId);
+int maxDataVolumesSupported = getMaxDataVolumesSupported(vm);
+List vols = _volsDao.findByInstance(vm.getId());
 if (deviceId != null) {
-if (deviceId.longValue() > 15 || deviceId.longValue() == 3) {
-throw new RuntimeException("deviceId should be 1,2,4-15");
+if (deviceId.longValue() > maxDataVolumesSupported || 
deviceId.longValue() == 3) {
+throw new RuntimeException("deviceId should be 1,2,4-" + 
maxDataVolumesSupported);
 }
 for (VolumeVO vol : vols) {
 if (vol.getDeviceId().equals(deviceId)) {
-throw new RuntimeException("deviceId " + deviceId + " 
is used by vm" + vmId);
+throw new RuntimeException("deviceId " + deviceId + " 
is used by vm" + vm.getId());
 }
 }
 } else {
 // allocate deviceId here
 List devIds = new ArrayList();
-for (int i = 1; i < 15; i++) {
+for (int i = 1; i < maxDataVolumesSupported; i++) {
--- End diff --

@HrWiggles Correct. I've updated the code. Thanks.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1953: CLOUDSTACK-9794: Unable to attach more than 1...

2017-02-21 Thread HrWiggles 
Github user HrWiggles commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1953#discussion_r102342529
  
--- Diff: 
plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java 
---
@@ -584,18 +584,36 @@ public void defFileBasedDisk(String filePath, String 
diskLabel, DiskBus bus, Dis
 
 /* skip iso label */
 private String getDevLabel(int devId, DiskBus bus) {
--- End diff --

Would be great to have unit tests for either `getDevLabel(int devId, 
DiskBus bus)` or `getDevLabelSuffix(int deviceIndex)`, especially to test for 
the expected results when `devId` (or `deviceIndex`) are high enough to return 
a double-letter device label suffix.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1953: CLOUDSTACK-9794: Unable to attach more than 1...

2017-02-21 Thread HrWiggles 
Github user HrWiggles commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1953#discussion_r102336557
  
--- Diff: server/src/com/cloud/storage/VolumeApiServiceImpl.java ---
@@ -2639,22 +2639,23 @@ private int getMaxDataVolumesSupported(UserVmVO vm) 
{
 return maxDataVolumesSupported.intValue();
 }
 
-private Long getDeviceId(long vmId, Long deviceId) {
+private Long getDeviceId(UserVmVO vm, Long deviceId) {
 // allocate deviceId
-List vols = _volsDao.findByInstance(vmId);
+int maxDataVolumesSupported = getMaxDataVolumesSupported(vm);
+List vols = _volsDao.findByInstance(vm.getId());
 if (deviceId != null) {
-if (deviceId.longValue() > 15 || deviceId.longValue() == 3) {
-throw new RuntimeException("deviceId should be 1,2,4-15");
+if (deviceId.longValue() > maxDataVolumesSupported || 
deviceId.longValue() == 3) {
+throw new RuntimeException("deviceId should be 1,2,4-" + 
maxDataVolumesSupported);
 }
 for (VolumeVO vol : vols) {
 if (vol.getDeviceId().equals(deviceId)) {
-throw new RuntimeException("deviceId " + deviceId + " 
is used by vm" + vmId);
+throw new RuntimeException("deviceId " + deviceId + " 
is used by vm" + vm.getId());
 }
 }
 } else {
 // allocate deviceId here
 List devIds = new ArrayList();
-for (int i = 1; i < 15; i++) {
+for (int i = 1; i < maxDataVolumesSupported; i++) {
--- End diff --

@sureshanaparti If the condition really should be `i < 
maxDataVolumesSupported` (which would make the maximum device id returned by 
the method be `maxDataVolumesSupported - 1`), then the check + error message 
above
```
if (deviceId.longValue() <= 0 || deviceId.longValue() > 
maxDataVolumesSupported || deviceId.longValue() == 3) {
throw new RuntimeException("deviceId should be 1,2,4-" + 
maxDataVolumesSupported);
```
need to be changed so as not to include the value of 
`maxDataVolumesSupported` itself.
Otherwise, when `maxDataVolumesSupported` has value `6` (for example), the 
method would not ever return `6` when parameter `deviceId` is specified as 
`null` but would return `6` when parameter `deviceId` is specified as `6` 
(assuming device id `6` is not already in use).  Also, the error message would 
state "deviceId should be 1,2,4-6" whenever parameter `deviceId` would be 
specified as an invalid value, which would not be correct (as `5` should be the 
highest valid device id).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1953: CLOUDSTACK-9794: Unable to attach more than 1...

2017-02-21 Thread sureshanaparti 
Github user sureshanaparti commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1953#discussion_r102311697
  
--- Diff: 
plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java 
---
@@ -716,11 +734,6 @@ public DiskFmtType getDiskFormatType() {
 return _diskFmtType;
 }
 
--- End diff --

Removed unused method _getDiskSeq()_.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1953: CLOUDSTACK-9794: Unable to attach more than 1...

2017-02-21 Thread sureshanaparti 
Github user sureshanaparti commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1953#discussion_r102309898
  
--- Diff: server/src/com/cloud/storage/VolumeApiServiceImpl.java ---
@@ -2639,22 +2639,23 @@ private int getMaxDataVolumesSupported(UserVmVO vm) 
{
 return maxDataVolumesSupported.intValue();
 }
 
-private Long getDeviceId(long vmId, Long deviceId) {
+private Long getDeviceId(UserVmVO vm, Long deviceId) {
 // allocate deviceId
-List vols = _volsDao.findByInstance(vmId);
+int maxDataVolumesSupported = getMaxDataVolumesSupported(vm);
--- End diff --

@rafaelweingartner when configuring getMaxDataVolumesSupported(vm) with 6 
for the hypervisor of the VM, the VM can have max 6 devices. 1 root (id 0), 1 
CD-ROM (id 3) and other 4 for extra disks/volumes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1953: CLOUDSTACK-9794: Unable to attach more than 1...

2017-02-21 Thread sureshanaparti 
Github user sureshanaparti commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1953#discussion_r102303566
  
--- Diff: server/src/com/cloud/storage/VolumeApiServiceImpl.java ---
@@ -2639,22 +2639,23 @@ private int getMaxDataVolumesSupported(UserVmVO vm) 
{
 return maxDataVolumesSupported.intValue();
 }
 
-private Long getDeviceId(long vmId, Long deviceId) {
+private Long getDeviceId(UserVmVO vm, Long deviceId) {
 // allocate deviceId
-List vols = _volsDao.findByInstance(vmId);
+int maxDataVolumesSupported = getMaxDataVolumesSupported(vm);
+List vols = _volsDao.findByInstance(vm.getId());
 if (deviceId != null) {
-if (deviceId.longValue() > 15 || deviceId.longValue() == 3) {
-throw new RuntimeException("deviceId should be 1,2,4-15");
+if (deviceId.longValue() > maxDataVolumesSupported || 
deviceId.longValue() == 3) {
+throw new RuntimeException("deviceId should be 1,2,4-" + 
maxDataVolumesSupported);
 }
 for (VolumeVO vol : vols) {
 if (vol.getDeviceId().equals(deviceId)) {
-throw new RuntimeException("deviceId " + deviceId + " 
is used by vm" + vmId);
+throw new RuntimeException("deviceId " + deviceId + " 
is used by vm" + vm.getId());
 }
 }
 } else {
 // allocate deviceId here
 List devIds = new ArrayList();
-for (int i = 1; i < 15; i++) {
+for (int i = 1; i < maxDataVolumesSupported; i++) {
 devIds.add(String.valueOf(i));
 }
 devIds.remove("3");
--- End diff --

Thanks. Added this.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1953: CLOUDSTACK-9794: Unable to attach more than 1...

2017-02-21 Thread sureshanaparti 
Github user sureshanaparti commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1953#discussion_r102303488
  
--- Diff: server/src/com/cloud/storage/VolumeApiServiceImpl.java ---
@@ -2639,22 +2639,23 @@ private int getMaxDataVolumesSupported(UserVmVO vm) 
{
 return maxDataVolumesSupported.intValue();
 }
 
-private Long getDeviceId(long vmId, Long deviceId) {
+private Long getDeviceId(UserVmVO vm, Long deviceId) {
 // allocate deviceId
-List vols = _volsDao.findByInstance(vmId);
+int maxDataVolumesSupported = getMaxDataVolumesSupported(vm);
+List vols = _volsDao.findByInstance(vm.getId());
 if (deviceId != null) {
-if (deviceId.longValue() > 15 || deviceId.longValue() == 3) {
-throw new RuntimeException("deviceId should be 1,2,4-15");
+if (deviceId.longValue() > maxDataVolumesSupported || 
deviceId.longValue() == 3) {
+throw new RuntimeException("deviceId should be 1,2,4-" + 
maxDataVolumesSupported);
 }
 for (VolumeVO vol : vols) {
 if (vol.getDeviceId().equals(deviceId)) {
-throw new RuntimeException("deviceId " + deviceId + " 
is used by vm" + vmId);
+throw new RuntimeException("deviceId " + deviceId + " 
is used by vm" + vm.getId());
 }
 }
 } else {
 // allocate deviceId here
 List devIds = new ArrayList();
--- End diff --

@HrWiggles. Noted, not considering it for now.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1953: CLOUDSTACK-9794: Unable to attach more than 1...

2017-02-21 Thread sureshanaparti 
Github user sureshanaparti commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1953#discussion_r102303229
  
--- Diff: server/src/com/cloud/storage/VolumeApiServiceImpl.java ---
@@ -2639,22 +2639,23 @@ private int getMaxDataVolumesSupported(UserVmVO vm) 
{
 return maxDataVolumesSupported.intValue();
 }
 
-private Long getDeviceId(long vmId, Long deviceId) {
+private Long getDeviceId(UserVmVO vm, Long deviceId) {
 // allocate deviceId
-List vols = _volsDao.findByInstance(vmId);
+int maxDataVolumesSupported = getMaxDataVolumesSupported(vm);
+List vols = _volsDao.findByInstance(vm.getId());
 if (deviceId != null) {
-if (deviceId.longValue() > 15 || deviceId.longValue() == 3) {
-throw new RuntimeException("deviceId should be 1,2,4-15");
+if (deviceId.longValue() > maxDataVolumesSupported || 
deviceId.longValue() == 3) {
+throw new RuntimeException("deviceId should be 1,2,4-" + 
maxDataVolumesSupported);
 }
 for (VolumeVO vol : vols) {
 if (vol.getDeviceId().equals(deviceId)) {
-throw new RuntimeException("deviceId " + deviceId + " 
is used by vm" + vmId);
+throw new RuntimeException("deviceId " + deviceId + " 
is used by vm" + vm.getId());
 }
 }
 } else {
 // allocate deviceId here
 List devIds = new ArrayList();
-for (int i = 1; i < 15; i++) {
+for (int i = 1; i < maxDataVolumesSupported; i++) {
--- End diff --

@HrWiggles Thanks for pointing this. Addressed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1953: CLOUDSTACK-9794: Unable to attach more than 1...

2017-02-21 Thread sureshanaparti 
Github user sureshanaparti commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1953#discussion_r102302052
  
--- Diff: server/src/com/cloud/storage/VolumeApiServiceImpl.java ---
@@ -2639,22 +2639,23 @@ private int getMaxDataVolumesSupported(UserVmVO vm) 
{
 return maxDataVolumesSupported.intValue();
 }
 
-private Long getDeviceId(long vmId, Long deviceId) {
+private Long getDeviceId(UserVmVO vm, Long deviceId) {
 // allocate deviceId
-List vols = _volsDao.findByInstance(vmId);
+int maxDataVolumesSupported = getMaxDataVolumesSupported(vm);
+List vols = _volsDao.findByInstance(vm.getId());
 if (deviceId != null) {
-if (deviceId.longValue() > 15 || deviceId.longValue() == 3) {
-throw new RuntimeException("deviceId should be 1,2,4-15");
+if (deviceId.longValue() > maxDataVolumesSupported || 
deviceId.longValue() == 3) {
--- End diff --

@HrWiggles Addressed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1953: CLOUDSTACK-9794: Unable to attach more than 1...

2017-02-21 Thread sureshanaparti 
Github user sureshanaparti commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1953#discussion_r102301869
  
--- Diff: server/src/com/cloud/storage/VolumeApiServiceImpl.java ---
@@ -2639,22 +2639,23 @@ private int getMaxDataVolumesSupported(UserVmVO vm) 
{
 return maxDataVolumesSupported.intValue();
 }
 
-private Long getDeviceId(long vmId, Long deviceId) {
+private Long getDeviceId(UserVmVO vm, Long deviceId) {
--- End diff --

@HrWiggles Will check if I can write a test for the same.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1953: CLOUDSTACK-9794: Unable to attach more than 1...

2017-02-21 Thread sureshanaparti 
Github user sureshanaparti commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1953#discussion_r102301645
  
--- Diff: server/src/com/cloud/storage/VolumeApiServiceImpl.java ---
@@ -2639,22 +2639,23 @@ private int getMaxDataVolumesSupported(UserVmVO vm) 
{
 return maxDataVolumesSupported.intValue();
 }
 
-private Long getDeviceId(long vmId, Long deviceId) {
+private Long getDeviceId(UserVmVO vm, Long deviceId) {
 // allocate deviceId
-List vols = _volsDao.findByInstance(vmId);
+int maxDataVolumesSupported = getMaxDataVolumesSupported(vm);
--- End diff --

@HrWiggles Thanks for the review. The max data volumes here is the actual 
hypervisor capability (which is posted in the db). The device id 3 is being 
reserved for something since long and I don't want that to be effected.
When _getMaxDataVolumesSupported()_ returns 6, max 5 volumes can be 
attached to the VM and one device reserved (might be for virtual tools/CDROM). 
_maxDataVolumesSupported_ specifies the data volumes limit supported by 
hypervisor, nothing related to _maxDeviceId_.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1953: CLOUDSTACK-9794: Unable to attach more than 1...

2017-02-21 Thread rafaelweingartner 
Github user rafaelweingartner commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1953#discussion_r102294407
  
--- Diff: server/src/com/cloud/storage/VolumeApiServiceImpl.java ---
@@ -2639,22 +2639,23 @@ private int getMaxDataVolumesSupported(UserVmVO vm) 
{
 return maxDataVolumesSupported.intValue();
 }
 
-private Long getDeviceId(long vmId, Long deviceId) {
+private Long getDeviceId(UserVmVO vm, Long deviceId) {
 // allocate deviceId
-List vols = _volsDao.findByInstance(vmId);
+int maxDataVolumesSupported = getMaxDataVolumesSupported(vm);
--- End diff --

I think this is a good question. I would add that the ID `0` is reserved 
for the root device.

So, I add the question if the `maxDataVolumesSupported` already accounts 
for the one already reserved for the root disk. 

For instance, when configuring `getMaxDataVolumesSupported(vm)` with `6` 
for the hypervisor of the VM. Does that mean that the VM can have up to 7 
devices? 1 root (id `0`), 1 CD-ROM (id `3`) and other 5 for extra disks/volumes.



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1953: CLOUDSTACK-9794: Unable to attach more than 1...

2017-02-21 Thread rafaelweingartner 
Github user rafaelweingartner commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1953#discussion_r102291066
  
--- Diff: server/src/com/cloud/storage/VolumeApiServiceImpl.java ---
@@ -2639,22 +2639,23 @@ private int getMaxDataVolumesSupported(UserVmVO vm) 
{
 return maxDataVolumesSupported.intValue();
 }
 
-private Long getDeviceId(long vmId, Long deviceId) {
+private Long getDeviceId(UserVmVO vm, Long deviceId) {
--- End diff --

big 👍 for this request :)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1953: CLOUDSTACK-9794: Unable to attach more than 1...

2017-02-21 Thread HrWiggles 
Github user HrWiggles commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1953#discussion_r102179249
  
--- Diff: server/src/com/cloud/storage/VolumeApiServiceImpl.java ---
@@ -2639,22 +2639,23 @@ private int getMaxDataVolumesSupported(UserVmVO vm) 
{
 return maxDataVolumesSupported.intValue();
 }
 
-private Long getDeviceId(long vmId, Long deviceId) {
+private Long getDeviceId(UserVmVO vm, Long deviceId) {
 // allocate deviceId
-List vols = _volsDao.findByInstance(vmId);
+int maxDataVolumesSupported = getMaxDataVolumesSupported(vm);
+List vols = _volsDao.findByInstance(vm.getId());
 if (deviceId != null) {
-if (deviceId.longValue() > 15 || deviceId.longValue() == 3) {
-throw new RuntimeException("deviceId should be 1,2,4-15");
+if (deviceId.longValue() > maxDataVolumesSupported || 
deviceId.longValue() == 3) {
--- End diff --

There's no check for whether `deviceId` is greater than `0`.  Should check 
for that here, as well.  Otherwise, the method will accept `0` and negative 
values as valid device ids.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1953: CLOUDSTACK-9794: Unable to attach more than 1...

2017-02-21 Thread HrWiggles 
Github user HrWiggles commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1953#discussion_r102181285
  
--- Diff: server/src/com/cloud/storage/VolumeApiServiceImpl.java ---
@@ -2639,22 +2639,23 @@ private int getMaxDataVolumesSupported(UserVmVO vm) 
{
 return maxDataVolumesSupported.intValue();
 }
 
-private Long getDeviceId(long vmId, Long deviceId) {
+private Long getDeviceId(UserVmVO vm, Long deviceId) {
 // allocate deviceId
-List vols = _volsDao.findByInstance(vmId);
+int maxDataVolumesSupported = getMaxDataVolumesSupported(vm);
+List vols = _volsDao.findByInstance(vm.getId());
 if (deviceId != null) {
-if (deviceId.longValue() > 15 || deviceId.longValue() == 3) {
-throw new RuntimeException("deviceId should be 1,2,4-15");
+if (deviceId.longValue() > maxDataVolumesSupported || 
deviceId.longValue() == 3) {
+throw new RuntimeException("deviceId should be 1,2,4-" + 
maxDataVolumesSupported);
 }
 for (VolumeVO vol : vols) {
 if (vol.getDeviceId().equals(deviceId)) {
-throw new RuntimeException("deviceId " + deviceId + " 
is used by vm" + vmId);
+throw new RuntimeException("deviceId " + deviceId + " 
is used by vm" + vm.getId());
 }
 }
 } else {
 // allocate deviceId here
 List devIds = new ArrayList();
-for (int i = 1; i < 15; i++) {
+for (int i = 1; i < maxDataVolumesSupported; i++) {
 devIds.add(String.valueOf(i));
 }
 devIds.remove("3");
--- End diff --

Not part of your changes but... there's a possible `NoSuchElementException` 
below, for line:
```
deviceId = Long.parseLong(devIds.iterator().next());
```
A check should be added for whether `devIds` is empty and, if so, throw a 
`RuntimeException` with an error message which specifies that all possible 
device ids are already in use by the vm (if only to be consistent with the rest 
of the method).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1953: CLOUDSTACK-9794: Unable to attach more than 1...

2017-02-21 Thread HrWiggles 
Github user HrWiggles commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1953#discussion_r102178630
  
--- Diff: server/src/com/cloud/storage/VolumeApiServiceImpl.java ---
@@ -2639,22 +2639,23 @@ private int getMaxDataVolumesSupported(UserVmVO vm) 
{
 return maxDataVolumesSupported.intValue();
 }
 
-private Long getDeviceId(long vmId, Long deviceId) {
+private Long getDeviceId(UserVmVO vm, Long deviceId) {
 // allocate deviceId
-List vols = _volsDao.findByInstance(vmId);
+int maxDataVolumesSupported = getMaxDataVolumesSupported(vm);
+List vols = _volsDao.findByInstance(vm.getId());
 if (deviceId != null) {
-if (deviceId.longValue() > 15 || deviceId.longValue() == 3) {
-throw new RuntimeException("deviceId should be 1,2,4-15");
+if (deviceId.longValue() > maxDataVolumesSupported || 
deviceId.longValue() == 3) {
+throw new RuntimeException("deviceId should be 1,2,4-" + 
maxDataVolumesSupported);
 }
 for (VolumeVO vol : vols) {
 if (vol.getDeviceId().equals(deviceId)) {
-throw new RuntimeException("deviceId " + deviceId + " 
is used by vm" + vmId);
+throw new RuntimeException("deviceId " + deviceId + " 
is used by vm" + vm.getId());
 }
 }
 } else {
 // allocate deviceId here
 List devIds = new ArrayList();
-for (int i = 1; i < 15; i++) {
+for (int i = 1; i < maxDataVolumesSupported; i++) {
--- End diff --

Since `maxDataVolumesSupported` is basically being used to indicate the 
"max-device-id", the conditional should be `i <= maxDataVolumesSupported`.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1953: CLOUDSTACK-9794: Unable to attach more than 1...

2017-02-21 Thread HrWiggles 
Github user HrWiggles commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1953#discussion_r102184780
  
--- Diff: server/src/com/cloud/storage/VolumeApiServiceImpl.java ---
@@ -2639,22 +2639,23 @@ private int getMaxDataVolumesSupported(UserVmVO vm) 
{
 return maxDataVolumesSupported.intValue();
 }
 
-private Long getDeviceId(long vmId, Long deviceId) {
+private Long getDeviceId(UserVmVO vm, Long deviceId) {
 // allocate deviceId
-List vols = _volsDao.findByInstance(vmId);
+int maxDataVolumesSupported = getMaxDataVolumesSupported(vm);
+List vols = _volsDao.findByInstance(vm.getId());
 if (deviceId != null) {
-if (deviceId.longValue() > 15 || deviceId.longValue() == 3) {
-throw new RuntimeException("deviceId should be 1,2,4-15");
+if (deviceId.longValue() > maxDataVolumesSupported || 
deviceId.longValue() == 3) {
+throw new RuntimeException("deviceId should be 1,2,4-" + 
maxDataVolumesSupported);
 }
 for (VolumeVO vol : vols) {
 if (vol.getDeviceId().equals(deviceId)) {
-throw new RuntimeException("deviceId " + deviceId + " 
is used by vm" + vmId);
+throw new RuntimeException("deviceId " + deviceId + " 
is used by vm" + vm.getId());
 }
 }
 } else {
 // allocate deviceId here
 List devIds = new ArrayList();
--- End diff --

Not part of your changes but... variable `devIds` should have type 
`List` instead of `List`.
All that conversion from `int` to `String` and then converting from 
`String` to `long` seems unnecessary.
Should simply be able to do:
```
List devIds = new ArrayList<>();
for (long i = 1; i <= maxDataVolumesSupported; i++) {
devIds.add(i);
}
devIds.remove(3L);
for (VolumeVO vol : vols) {
devIds.remove(vol.getDeviceId());
}
if (devIds.isEmpty()) {
throw new RuntimeException("every available deviceId already in use by 
vm " + vm.getId());
}
deviceId = devIds.iterator().next();
```
Note: my code above includes fixes to two other comments I made further 
down in the code.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1953: CLOUDSTACK-9794: Unable to attach more than 1...

2017-02-21 Thread HrWiggles 
Github user HrWiggles commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1953#discussion_r102187534
  
--- Diff: server/src/com/cloud/storage/VolumeApiServiceImpl.java ---
@@ -2639,22 +2639,23 @@ private int getMaxDataVolumesSupported(UserVmVO vm) 
{
 return maxDataVolumesSupported.intValue();
 }
 
-private Long getDeviceId(long vmId, Long deviceId) {
+private Long getDeviceId(UserVmVO vm, Long deviceId) {
--- End diff --

How about adding unit tests for the method `getDeviceId(UserVmVO vm, Long 
deviceId)`?
Things I can currently think of to test:
- `RuntimeException` if param `deviceId` is specified as a negative value
- `RuntimeException` if param `deviceId` is specified as `0L`
- `RuntimeException` if param `deviceId` is specified as a value greater 
than the "max-device-id"
- `RuntimeException` if param `deviceId` is specified as reserved id `3L`
- `RuntimeException` if param `deviceId` is specified as an id that is 
already in use
- `RuntimeException` if param `deviceId` is specified as `null` and all 
device ids are in use
- returns id specified in param `deviceId` when not `null` and the id is 
not in use
- returns lowest available id when param `deviceId` is specified as `null`

(all of the above are from my understanding of how the method should behave)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1953: CLOUDSTACK-9794: Unable to attach more than 1...

2017-02-21 Thread HrWiggles 
Github user HrWiggles commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1953#discussion_r102177792
  
--- Diff: server/src/com/cloud/storage/VolumeApiServiceImpl.java ---
@@ -2639,22 +2639,23 @@ private int getMaxDataVolumesSupported(UserVmVO vm) 
{
 return maxDataVolumesSupported.intValue();
 }
 
-private Long getDeviceId(long vmId, Long deviceId) {
+private Long getDeviceId(UserVmVO vm, Long deviceId) {
 // allocate deviceId
-List vols = _volsDao.findByInstance(vmId);
+int maxDataVolumesSupported = getMaxDataVolumesSupported(vm);
--- End diff --

Is it enough to use max-data-volumes-supported as the max device id, seeing 
as device id 3 is not used? (or is reserved)

E.g. let's assume that `getMaxDataVolumesSupported(vm)` returns `6`.  What 
device ids should be ok in that case?  Would they be `1`, `2`, `4`, `5`, and 
`6` (since `maxDataVolumesSupported` is `6`) which is a total of `5` data 
volumes, or should `maxDataVolumesSupported` be renamed as `maxDeviceId` and be 
assigned a value of `getMaxDataVolumesSupported(vm) + 1` to account for the 
unused/reserved id `3`?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1953: CLOUDSTACK-9794: Unable to attach more than 1...

2017-02-18 Thread sureshanaparti 
GitHub user sureshanaparti opened a pull request:

https://github.com/apache/cloudstack/pull/1953

CLOUDSTACK-9794: Unable to attach more than 14 devices to a VM

Updated hardcoded value with max data volumes limit from hypervisor 
capabilities.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/Accelerite/cloudstack CLOUDSTACK-9794

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/cloudstack/pull/1953.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #1953


commit cdf50c1e0f69297beb335f852ae82663bbc87fb4
Author: Suresh Kumar Anaparti 
Date:   2017-02-18T20:22:30Z

CLOUDSTACK-9794: Unable to attach more than 14 devices to a VM

Updated hardcoded value with max data volumes limit from hypervisor 
capabilities.




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---