CLOUDSTACK-10107: For VMware VMs add devices without unit number #2288
Conversation
|
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1142 |
|
@blueorangutan test centos7 vmware-55u3 |
|
@rhtyd a Trillian-Jenkins test job (centos7 mgmt + vmware-55u3) has been kicked to run smoke tests |
dbebbd3 to
a177f09
Compare
|
@blueorangutan package |
|
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1144 |
|
LGTM based on the code |
|
@blueorangutan test centos7 vmware-55u3 |
|
@rhtyd a Trillian-Jenkins test job (centos7 mgmt + vmware-55u3) has been kicked to run smoke tests |
|
LGTM |
| connectInfo.setStartConnected(connectOnStart); | ||
| nic.setAddressType("Manual"); | ||
| nic.setConnectable(connectInfo); | ||
| nic.setMacAddress(macAddress); | ||
|
|
||
| nic.setUnitNumber(deviceNumber); | ||
| nic.setKey(-contextNumber); |
There was a problem hiding this comment.
@rhtyd I know this is not your code, but there is something that called my attention (maybe you know why this is the way it is). At first I thought it was some dusty on my laptop screen... When the method nic.setKey is called, the code does -contextNumber, converting the positive integer to a negative one. I looked at the docs and the key is a unique ID of the NIC, but I reading the docs I did not see any reasons to use negative value as IDs.
Do you understand why we are using negative values there?
There was a problem hiding this comment.
@rafaelweingartner when I was trying to fix the bug, I thought too why we're using negative values however, I could not find a reason. For backward compatibilities sake and to reduce our test scopes, I chose to keep it that way. IMO it's not a problem as long as the key is unique, being negative/positive is not a problem.
There was a problem hiding this comment.
I agree with you, I only asked to see if there were some other hidden reasons.
Thanks for the explanations.
|
@rhtyd may I ask you something? I was looking at the code, and I see that you removed a variable called BTW: long time I am not able to review or work much with you guys ;) |
|
@rafaelweingartner thanks, good to see you reviewing. The issue is that during starting of a VM, we used hard coded nic unit numbers that caused the backend (vcenter side) to fail sometimes (well most the times, when number of nics > 7). I looked at the docs, and the unit number is not a mandatory attribute to use when adding new (nic) devices, which is the fix for the bug. In the specific method, we're still passing |
When VMs are deployed or nics are plugged, using a static unit number may cause device configuration errors. This fixes a previous limitation that more than 7 nics/networks could not be added to a VM. Per the API docs, `unitNumber` need not be set: https://www.vmware.com/support/developer/converter-sdk/conv55_apireference/vim.vm.device.VirtualDevice.html Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>
a177f09 to
953aba1
Compare
|
@blueorangutan package |
|
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1149 |
|
@blueorangutan test centos7 vmware-55u3 |
|
@rhtyd a Trillian-Jenkins test job (centos7 mgmt + vmware-55u3) has been kicked to run smoke tests |
|
Trillian test result (tid-1577)
|
|
Newly added test is passing |
When VMs are deployed or nics are plugged, using a static unit number
may cause device configuration errors. This fixes a previous limitation
that more than 7 nics/networks could not be added to a VM.
Pinging for review @sureshanaparti @resmo @GabrielBrascher @PaulAngus @nvazquez @DaanHoogland @borisstoyanov and others
Per the API docs,
unitNumberneed not be set: https://www.vmware.com/support/developer/converter-sdk/conv55_apireference/vim.vm.device.VirtualDevice.html@blueorangutan package