Repository: vmware/govmomi
Pull request #3162 (2023-06-27 19:39:13)

vcsim: apply PCI UnitNumber offset of 7 for ethernet
## Description

Hey there! Not totally sure if this was the best approach, but I was revisiting some behavior in vcsim, specifically the PCI offset for NIC devices discussed in #907 and #2060. The tl;dr is that when creating a VM in vcsim (either with `govc` or the Terraform provider), the NIC that gets created has the following attributes:

Created with:

```
govc vm.create -m 2048 -c 2 -g ubuntu64Guest -net.adapter e1000 -disk.controller pvscsi -pool DC0_C0/Resources -net "VM Network" test-vm
```

Creates the following NIC

```
govc device.info -vm test-vm ethernet--6
Name:               ethernet--6
  Type:             VirtualE1000
  Label:            ethernet--7
  Summary:          VM Network
  Key:              203
  Controller:       pci-100
  Unit number:      1
  Connected:        true
  Start connected:  true
  Guest control:    false
  Status:
  MAC Address:      00:0c:29:31:32:32
  Address type:
```

My understanding is that NICs _should_ have a unit number starting at 7 based on the above issues as well as [this comment](https://github.com/vmware/govmomi/blob/542be4a6b4f0d79cf3f6898faec71f2b5d650d70/object/virtual_device_list.go#L922).

This PR does a couple of things:

- It moves the logic of `newUnitNumber` to `newUnitNumberBase` to take in a base offset and `newUnitNumber` calls the new `newUnitNumberBase` function with a base of 0
- The `AssignController` function calls `newUnitNumberBase` with a base of 7 if the device is a virtual ethernet card, otherwise it calls `newUnitNumber` as it does today

With these changes, creating the same VM above creates an ethernet card with the following attributes:

```
govc device.info -vm test-vm ethernet-0
Name:               ethernet-0
  Type:             VirtualE1000
  Label:            ethernet--7
  Summary:          VM Network
  Key:              203
  Controller:       pci-100
  Unit number:      7
  Connected:        true
  Start connected:  true
  Guest control:    false
  Status:
  MAC Address:      00:0c:29:37:32:32
  Address type:
```

I'll admit that I couldn't 100% verify if this is the expected behavior, but I figured I'd put together a PR to discuss this since this seems to be the _assumed_ behavior. Also happy to modify the implementation differently if you'd like. If this not the expected behavior, feel free to close :)

## Type of change

Please mark options that are relevant:

- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to
  not work as expected)
- [ ] This change requires a documentation update
- [ ] Build related change

## How Has This Been Tested?

- Verified that this does not otherwise change the expected behavior of VMs

## Checklist:

- [x] My code follows the `CONTRIBUTION`
  [guidelines](https://github.com/vmware/govmomi/blob/main/CONTRIBUTING.md) of
  this project
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have made corresponding changes to the documentation
- [x] I have added tests that prove my fix is effective or that my feature works
- [x] New and existing unit tests pass locally with my changes
- [x] Any dependent changes have been merged

dougm: Would have merged already, but need to get CI/goreleaser fixed first (sigh), see: https://github.com/vmware/govmomi/pull/3168#pullrequestreview-1511760355
dougm: @BrianMMcClain goreleaser issue is fixed, can you rebase to include #3170 ?
BrianMMcClain: Done! 😄
dougm: Thanks @BrianMMcClain , failed again, but a timeout flake we've seen before. But also meant to ask, can you also squash: https://github.com/vmware/govmomi/blob/main/CONTRIBUTING.md#updating-pull-requests

Patch:
diff --git a/object/virtual_device_list.go b/object/virtual_device_list.go
index 37655065..92797dcd 100644
--- a/object/virtual_device_list.go
+++ b/object/virtual_device_list.go
@@ -404,9 +404,13 @@ func (l VirtualDeviceList) PickController(kind types.BaseVirtualController) type
 }
 
 // newUnitNumber returns the unit number to use for attaching a new device to the given controller.
-func (l VirtualDeviceList) newUnitNumber(c types.BaseVirtualController) int32 {
+func (l VirtualDeviceList) newUnitNumber(c types.BaseVirtualController, offset int) int32 {
 	units := make([]bool, 30)
 
+	for i := 0; i < offset; i++ {
+		units[i] = true
+	}
+
 	switch sc := c.(type) {
 	case types.BaseVirtualSCSIController:
 		//  The SCSI controller sits on its own bus
@@ -455,7 +459,14 @@ func (l VirtualDeviceList) AssignController(device types.BaseVirtualDevice, c ty
 	d := device.GetVirtualDevice()
 	d.ControllerKey = c.GetVirtualController().Key
 	d.UnitNumber = new(int32)
-	*d.UnitNumber = l.newUnitNumber(c)
+
+	offset := 0
+	switch device.(type) {
+	case types.BaseVirtualEthernetCard:
+		offset = 7
+	}
+	*d.UnitNumber = l.newUnitNumber(c, offset)
+
 	if d.Key == 0 {
 		d.Key = l.newRandomKey()
 	}


Test patch:
diff --git a/object/virtual_device_list_test.go b/object/virtual_device_list_test.go
index fef4db9e..466fb02d 100644
--- a/object/virtual_device_list_test.go
+++ b/object/virtual_device_list_test.go
@@ -698,7 +698,7 @@ func TestPickController(t *testing.T) {
 			t.Errorf("expected controller key: %d, got: %d\n", test.key, key)
 		}
 
-		unit := list.newUnitNumber(c)
+		unit := list.newUnitNumber(c, 0)
 		if unit != test.unit {
 			t.Errorf("expected unit number: %d, got: %d\n", test.unit, unit)
 		}