Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

r/virtual_machine: Correct present NIC device calculation #280

Merged
merged 1 commit into from
Dec 6, 2017

Conversation

vancluever
Copy link
Contributor

This corrects an issue with the way NIC devices were checked to see if
the slots they are supposed to occupy on the PCI bus were already used
or not. An offset that was used to populate the current device range was
not being used when the devices were actually checked, creating invalid
results and out of range errors.

Fixes #279.

@vancluever
Copy link
Contributor Author

Test passes:

=== RUN   TestAccResourceVSphereVirtualMachine
=== RUN   TestAccResourceVSphereVirtualMachine/maximum_number_of_nics
--- PASS: TestAccResourceVSphereVirtualMachine (187.24s)
    --- PASS: TestAccResourceVSphereVirtualMachine/maximum_number_of_nics (187.24s)

This corrects an issue with the way NIC devices were checked to see if
the slots they are supposed to occupy on the PCI bus were already used
or not. An offset that was used to populate the current device range was
not being used when the devices were actually checked, creating invalid
results and out of range errors.

Fixes #279.
Copy link
Member

@mbfrahry mbfrahry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a minor thing

@@ -870,7 +870,7 @@ func (r *NetworkInterfaceSubresource) assignEthernetCard(l object.VirtualDeviceL

// Now that we know which units are used, we can pick one
newUnit := int32(r.Index) + pciDeviceOffset
if units[newUnit] {
if units[newUnit-pciDeviceOffset] {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to add pciDeviceOffset if we are just subtracting it 2 lines later? Line 872 for reference

Copy link
Contributor Author

@vancluever vancluever Dec 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, units is a []bool of length 10 that we use to get a "map" of what PCI devices in the available network device PCI range (7-16) are currently taken to make sure we are not going to add/assign a device in an already occupied slot. This obviously has an index range of 0-9, and is not reflective of the final unit number. This was actually an error as the slice is being correctly populated here, but not being subsequently referenced correctly.

This is why only 3 devices can be added right now as per the referenced issue:

network_interface.0 7
network_interface.1 8
network_interface.2 9
network_interface.3 10 (out of range here)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah. Yep! Makes sense now. Thank you

Copy link
Member

@mbfrahry mbfrahry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@vancluever
Copy link
Contributor Author

Thanks @mbfrahry!

@vancluever vancluever merged commit 144ca6a into master Dec 6, 2017
@vancluever vancluever deleted the GH-279 branch December 6, 2017 22:36
@ghost ghost locked and limited conversation to collaborators Apr 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Type: Bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Not possible to create more than 3 network interfaces!
2 participants