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.

Sign up for GitHub

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

Jump to bottom

[BUG] simulator has data race when customize is run while a client is reading properties #3533

Closed
prziborowski opened this issue Aug 26, 2024 · 1 comment · Fixed by #3538
Closed

[BUG] simulator has data race when customize is run while a client is reading properties #3533

prziborowski opened this issue Aug 26, 2024 · 1 comment · Fixed by #3538

Comments

@prziborowski
Copy link

Describe the bug

My team has been hitting this problem with a larger test environment where one client will be querying VMs to get their properties and another client will be making requests to customize and power on VMs.
When we run tests for this with -race option, it occasionally fails mentioning a data race with one thread in the customize function of PowerOn and another thread serializing an object (I'm guessing the VM).

To Reproduce
Steps to reproduce the behavior:

  1. I added the following test to govmomi/simulator/virtual_machine_test.go
func TestVmCustomizeStress(t *testing.T) {
	ctx := context.Background()

	m := VPX()
	defer m.Remove()
	err := m.Create()
	if err != nil {
		t.Fatal(err)
	}

	s := m.Service.NewServer()
	defer s.Close()

	c, err := govmomi.NewClient(ctx, s.URL, true)
	if err != nil {
		t.Fatal(err)
	}

	spec := types.VirtualMachineConfigSpec{
		Name:    "foo",
		GuestId: string(types.VirtualMachineGuestOsIdentifierOtherGuest),
		Files: &types.VirtualMachineFileInfo{
			VmPathName: "[LocalDS_0]",
		},
		DeviceChange: []types.BaseVirtualDeviceConfigSpec{
			&types.VirtualDeviceConfigSpec{
				Operation: types.VirtualDeviceConfigSpecOperationAdd,
				Device: &types.VirtualVmxnet3{VirtualVmxnet: types.VirtualVmxnet{
					VirtualEthernetCard: types.VirtualEthernetCard{
						VirtualDevice: types.VirtualDevice{
							Key: -1,
							DeviceInfo: &types.Description{
								Label:   "NIC 1",
								Summary: "VM Network",
							},
							Backing: &types.VirtualEthernetCardNetworkBackingInfo{
								VirtualDeviceDeviceBackingInfo: types.VirtualDeviceDeviceBackingInfo{
									DeviceName: "VM Network",
								},
							},
						},
					},
				},
				},
			},
		},
	}

	finder := find.NewFinder(c.Client, false)
	dc, err := finder.DefaultDatacenter(ctx)
	if err != nil {
		t.Fatal(err)
	}

	finder.SetDatacenter(dc)
	folders, err := dc.Folders(ctx)
	if err != nil {
		t.Fatal(err)
	}
	vmFolder := folders.VmFolder

	hosts, err := finder.HostSystemList(ctx, "*/*")
	if err != nil {
		t.Fatal(err)
	}

	nhosts := len(hosts)
	host := hosts[rand.Intn(nhosts)]
	pool, err := host.ResourcePool(ctx)
	if err != nil {
		t.Fatal(err)
	}

	ctask, _ := vmFolder.CreateVM(ctx, spec, pool, nil)
	info, err := ctask.WaitForResult(ctx, nil)
	if err != nil {
		t.Fatal(err)
	}
	vmm := Map.Get(info.Result.(types.ManagedObjectReference)).(*VirtualMachine)
	ref := vmm.Reference()
	vm := object.NewVirtualMachine(c.Client, ref)
	ch := make(chan bool)

	go func() {
		var moVM mo.VirtualMachine
		for {
			insideVm := object.NewVirtualMachine(c.Client, ref)
			select {
			case <-ch:
				return
			default:
				if err = insideVm.Properties(
					ctx,
					insideVm.Reference(),
					[]string{"config"},
					&moVM); err != nil {
					t.Fatal(err)
				}
			}
		}
	}()

	for i := 0; i < 10; i++ {
		customizationTask, err := vm.Customize(ctx, types.CustomizationSpec{
			NicSettingMap: []types.CustomizationAdapterMapping{
				{
					Adapter: types.CustomizationIPSettings{
						Ip: &types.CustomizationFixedIp{
							IpAddress: "192.168.1.1",
						},
					},
				},
			},
		})
		if err != nil {
			t.Error(err)
		}
		err = customizationTask.Wait(ctx)
		if err != nil {
			t.Error(err)
		}

		powerOnTask, err := vm.PowerOn(ctx)
		if err != nil {
			t.Fatal(err)
		}
		powerOnTask.Wait(ctx)

		powerOffTask, err := vm.PowerOff(ctx)
		if err != nil {
			t.Fatal(err)
		}
		powerOffTask.Wait(ctx)
	}
	ch <- true
}
  1. From govmomi directory I run go test -v -count=1 --race -run TestVmCustomizeStress github.com/vmware/govmomi/simulator.
  2. The output I got from the latest run was:
=== RUN   TestVmCustomizeStress
==================
WARNING: DATA RACE
Write at 0x00c0005d66d0 by goroutine 242:
  reflect.typedmemmove()
      /usr/local/go/src/runtime/mbarrier.go:203 +0x0
  reflect.Value.Set()
      /usr/local/go/src/reflect/value.go:2330 +0x184
  github.com/vmware/govmomi/vim25/mo.assignValue()
      /Users/nathanp/govmomi/vim25/mo/type_info.go:262 +0xc30
  github.com/vmware/govmomi/vim25/mo.assignValue()
      /Users/nathanp/govmomi/vim25/mo/type_info.go:309 +0x122c
  github.com/vmware/govmomi/vim25/mo.assignValue()
      /Users/nathanp/govmomi/vim25/mo/type_info.go:309 +0x122c
  github.com/vmware/govmomi/vim25/mo.ApplyPropertyChange()
      /Users/nathanp/govmomi/vim25/mo/retrieve.go:89 +0x330
  github.com/vmware/govmomi/simulator.(*Registry).Update()
      /Users/nathanp/govmomi/simulator/registry.go:321 +0x2e4
  github.com/vmware/govmomi/simulator.(*VirtualMachine).customize()
      /Users/nathanp/govmomi/simulator/virtual_machine.go:2458 +0x17c6
  github.com/vmware/govmomi/simulator.(*powerVMTask).Run()
      /Users/nathanp/govmomi/simulator/virtual_machine.go:1758 +0x950
  github.com/vmware/govmomi/simulator.(*powerVMTask).Run-fm()
      <autogenerated>:1 +0x3d
  github.com/vmware/govmomi/simulator.(*Task).Run.func1()
      /Users/nathanp/govmomi/simulator/task.go:130 +0x13b

Previous read at 0x00c0005d66d0 by goroutine 49:
  reflect.Value.String()
      /usr/local/go/src/reflect/value.go:2654 +0x7b3
  github.com/vmware/govmomi/vim25/xml.(*printer).marshalSimple()
      /Users/nathanp/govmomi/vim25/xml/marshal.go:771 +0x7d0
  github.com/vmware/govmomi/vim25/xml.(*printer).marshalValue()
      /Users/nathanp/govmomi/vim25/xml/marshal.go:534 +0x1506
  github.com/vmware/govmomi/vim25/xml.(*printer).marshalStruct()
      /Users/nathanp/govmomi/vim25/xml/marshal.go:960 +0x1485
  github.com/vmware/govmomi/vim25/xml.(*printer).marshalValue()
      /Users/nathanp/govmomi/vim25/xml/marshal.go:532 +0x14cf
  github.com/vmware/govmomi/vim25/xml.(*printer).marshalStruct()
      /Users/nathanp/govmomi/vim25/xml/marshal.go:960 +0x1485
  github.com/vmware/govmomi/vim25/xml.(*printer).marshalValue()
      /Users/nathanp/govmomi/vim25/xml/marshal.go:532 +0x14cf
  github.com/vmware/govmomi/vim25/xml.(*printer).marshalStruct()
      /Users/nathanp/govmomi/vim25/xml/marshal.go:960 +0x1485
  github.com/vmware/govmomi/vim25/xml.(*printer).marshalValue()
      /Users/nathanp/govmomi/vim25/xml/marshal.go:532 +0x14cf
  github.com/vmware/govmomi/vim25/xml.(*printer).marshalValue()
      /Users/nathanp/govmomi/vim25/xml/marshal.go:455 +0x16d6
  github.com/vmware/govmomi/vim25/xml.(*printer).marshalStruct()
      /Users/nathanp/govmomi/vim25/xml/marshal.go:960 +0x1485
  github.com/vmware/govmomi/vim25/xml.(*printer).marshalValue()
      /Users/nathanp/govmomi/vim25/xml/marshal.go:532 +0x14cf
  github.com/vmware/govmomi/vim25/xml.(*printer).marshalValue()
      /Users/nathanp/govmomi/vim25/xml/marshal.go:455 +0x16d6
  github.com/vmware/govmomi/vim25/xml.(*printer).marshalStruct()
      /Users/nathanp/govmomi/vim25/xml/marshal.go:960 +0x1485
  github.com/vmware/govmomi/vim25/xml.(*printer).marshalValue()
      /Users/nathanp/govmomi/vim25/xml/marshal.go:532 +0x14cf
  github.com/vmware/govmomi/vim25/xml.(*printer).marshalStruct()
      /Users/nathanp/govmomi/vim25/xml/marshal.go:960 +0x1485
  github.com/vmware/govmomi/vim25/xml.(*printer).marshalValue()
      /Users/nathanp/govmomi/vim25/xml/marshal.go:532 +0x14cf
  github.com/vmware/govmomi/vim25/xml.(*Encoder).EncodeElement()
      /Users/nathanp/govmomi/vim25/xml/marshal.go:177 +0x7b
  github.com/vmware/govmomi/simulator.(*response).MarshalXML()
      /Users/nathanp/govmomi/simulator/simulator.go:334 +0x4bd
  github.com/vmware/govmomi/vim25/xml.(*printer).marshalInterface()
      /Users/nathanp/govmomi/vim25/xml/marshal.go:673 +0x25e
  github.com/vmware/govmomi/vim25/xml.(*printer).marshalValue()
      /Users/nathanp/govmomi/vim25/xml/marshal.go:437 +0x11ac
  github.com/vmware/govmomi/vim25/xml.(*printer).marshalStruct()
      /Users/nathanp/govmomi/vim25/xml/marshal.go:960 +0x1485
  github.com/vmware/govmomi/vim25/xml.(*printer).marshalValue()
      /Users/nathanp/govmomi/vim25/xml/marshal.go:532 +0x14cf
  github.com/vmware/govmomi/vim25/xml.(*Encoder).Encode()
      /Users/nathanp/govmomi/vim25/xml/marshal.go:162 +0x64
  github.com/vmware/govmomi/simulator.(*Service).ServeSDK()
      /Users/nathanp/govmomi/simulator/simulator.go:530 +0x163b
  github.com/vmware/govmomi/simulator.(*Service).ServeSDK-fm()
      <autogenerated>:1 +0x51
  net/http.HandlerFunc.ServeHTTP()
      /usr/local/go/src/net/http/server.go:2166 +0x47
  net/http.(*ServeMux).ServeHTTP()
      /usr/local/go/src/net/http/server.go:2683 +0x1ef
  net/http.serverHandler.ServeHTTP()
      /usr/local/go/src/net/http/server.go:3137 +0x2a1
  net/http.(*conn).serve()
      /usr/local/go/src/net/http/server.go:2039 +0x13c4
  net/http.(*Server).Serve.gowrap3()
      /usr/local/go/src/net/http/server.go:3285 +0x4f

Goroutine 242 (running) created at:
  github.com/vmware/govmomi/simulator.(*Task).Run()
      /Users/nathanp/govmomi/simulator/task.go:125 +0x724
  github.com/vmware/govmomi/simulator.(*VirtualMachine).PowerOnVMTask()
      /Users/nathanp/govmomi/simulator/virtual_machine.go:1818 +0x4f1
  runtime.call32()
      /usr/local/go/src/runtime/asm_amd64.s:771 +0x42
  reflect.Value.Call()
      /usr/local/go/src/reflect/value.go:380 +0xb5
  github.com/vmware/govmomi/simulator.(*Service).call.func1()
      /Users/nathanp/govmomi/simulator/simulator.go:217 +0x87
  github.com/vmware/govmomi/simulator.(*Registry).WithLock()
      /Users/nathanp/govmomi/simulator/registry.go:658 +0x58
  github.com/vmware/govmomi/simulator.(*Service).call()
      /Users/nathanp/govmomi/simulator/simulator.go:216 +0x1bf2
  github.com/vmware/govmomi/simulator.(*Service).ServeSDK()
      /Users/nathanp/govmomi/simulator/simulator.go:501 +0xe4a
  github.com/vmware/govmomi/simulator.(*Service).ServeSDK-fm()
      <autogenerated>:1 +0x51
  net/http.HandlerFunc.ServeHTTP()
      /usr/local/go/src/net/http/server.go:2166 +0x47
  net/http.(*ServeMux).ServeHTTP()
      /usr/local/go/src/net/http/server.go:2683 +0x1ef
  net/http.serverHandler.ServeHTTP()
      /usr/local/go/src/net/http/server.go:3137 +0x2a1
  net/http.(*conn).serve()
      /usr/local/go/src/net/http/server.go:2039 +0x13c4
  net/http.(*Server).Serve.gowrap3()
      /usr/local/go/src/net/http/server.go:3285 +0x4f

Goroutine 49 (running) created at:
  net/http.(*Server).Serve()
      /usr/local/go/src/net/http/server.go:3285 +0x8ec
  github.com/vmware/govmomi/simulator/internal.(*Server).goServe.func1()
      /Users/nathanp/govmomi/simulator/internal/server.go:278 +0xbb
==================
    testing.go:1398: race detected during execution of test
--- FAIL: TestVmCustomizeStress (1.11s)
FAIL
FAIL	github.com/vmware/govmomi/simulator	1.917s
FAIL

Expected behavior
I either expect locks to prevent the data race as it appears to be server-side (simulator) code, so a client against non-simulator wouldn't be expected to serialize their calls.

Affected version
Using commit 7be4a88.

Additional context
Sorry if this seems like a contrived case, but I tried to simplify a product use-case to something I can copy/paste.
@github-actions GitHub Actions
Copy link
Contributor

Howdy 🖐   prziborowski ! Thank you for your interest in this project. We value your feedback and will respond soon.

If you want to contribute to this project, please make yourself familiar with the CONTRIBUTION guidelines.

@dougm dougm mentioned this issue Sep 3, 2024
Merged
@dougm dougm closed this as completed in 97aa721 Sep 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: vcsim: avoid possible race when encoding PropertyCollector response dougm/govmomi
1 participant
@prziborowski