-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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
[WIP] vSphere provider support for SDRS with storage pods #7031
Conversation
@markpeek @thetuxkeeper @dkalleg @dougm I am getting the following error running any test that has two disks, when I use a storagepod with DRS. The example code that I have with
The following call is failing. Ideas? return vm.AddDevice(context.TODO(), disk) |
Why is there a dot as datastore name? Is that correct? (don't know SDRS and if there's a special naming) |
@thetuxkeeper I am just using our old code with the new way to look up recommended datastores with DRS ... don't know. btw there is my TF from testing;
Error: --- FAIL: TestAccVSphereVirtualMachine_updateDRSDisks (24.34s)
testing.go:247: Step 0 error: Error applying: 1 error(s) occurred:
* vsphere_virtual_machine.foo: Invalid datastore path '[.] terraform-test/two.vmdk'.
FAIL
exit status 1
FAIL github.com/hashicorp/terraform/builtin/providers/vsphere 24.362s
make: *** [testacc] Error 1 |
@thetuxkeeper I assume it is the |
@dougm @markpeek here is what I am at The code we are using matches the code used in govc's vm.disk.create. I have not figured out how to use vm.disk.create, so I have not recreated it. Probably next on my todo ... When I am adding a new non-drs disk we call disk := devices.CreateDisk(controller, datastore.Reference(), diskPath) Where diskPath := "[BEL-5798813-EMC-VMAX1789-LUN-06] terraform-test/one" This is calculated with diskPath = fmt.Sprintf("[%v] %v", datastore.Name(), diskPath) When How do I get the correct value for the govomi api method Also this does not work either diskPath := "[BEL-SDRS-VMAX1789-POOL01] terraform-test/two.vmdk" |
41bbf0e
to
680128a
Compare
@chrislovecnm haven't had a chance to look yet, I'll try to get to it by tomorrow |
Thanks @dougm |
@dougm ping |
@dougm good news is that I figured it out :) |
great news @chrislovecnm |
b1d12a3
to
0430806
Compare
@markpeek @stack72 @jen20 @thetuxkeeper @dkalleg ~ can I get a code review? I have a couple of unit tests that are failing ... Don't think I broke the other tests, but I will get them happy regardless, but the code is at a good please to review :) Do you think we should start breaking apart the testing into separate files? For instance very few developers will be able to run the SDRS unit tests. |
Thanks Daniel, yah a lot of shops do not have enterprise installed |
@chrislovecnm is the testingg doc up to date? I'll build and run tests tomorrow, ive got 5.5u3 and 6.0 latest u environments. |
@mixacha the tests will now print to stdout if the are skipped. Let me know if you have questions about the ENV vars. Thanks Chris |
@mixacha here are my testing ENV vars https://gist.github.com/chrislovecnm/bf43c64a6e64a00cc2e533dbca5035ee - btw I have tested against 5.5 |
@jen20 @stack72 @phinze all my tests are passing, and @mixacha offered to test locally. How soon is 0.7? We have new test that I created that uncovered an existing bug.
Also tests are now skipping or failing more gracefully. For posterity https://gist.github.com/chrislovecnm/474fe0c5862386c1349739f7a7553cc3 |
@mixacha you get a chance to test? |
@chrislovecnm I'm setting up the environment at the moment, was too busy with regular schedule yesterday. |
Tests are running, I've observed that it keeps creating thick disks even in thin tests:
|
Which tests? Is this a new behavior? |
@chrislovecnm I think it's old behavior if you use different disk types in same virtual machine, terraform will apply last virtual machine disk type to all. TestAccVSphereVirtualMachine_diskInitType and pretty much any other VM related test that adds additional disks. |
I believe this is where one of problem lies, we're adding a thin disk but init_type for virtual machine resource defaults to eager_zeroed. Sorry I'm not of a big help, I need to get my gofu up to date. |
Full test output : vsphere 6.0 https://gist.github.com/mixacha/8d8d7f1bd6176a57a9969139f7a6e279 |
@mixacha you have DHCP setup by any chance? A bunch of those tests are bailing because of that. Also can you do the full debug output 😁 I want to get this stuff fixed :P |
@chrislovecnm I'll setup a small box tomorrow to enable dhcp and post debug output afterwards. |
When yah do please run full debug logs. |
@chrislovecnm gitst from my previous comment was updated with latest test results and debug.log attached. Do note that majority of tests create thick disks, that's why testing takes quite a while. |
need to verify that I have not introduced #7217 |
I can test this tomorrow sometime. |
|
||
relocateSpec := types.VirtualMachineRelocateSpec{ | ||
DeviceChange: spds.ConfigSpecsNetwork, | ||
Folder: &folderref, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where is the Folder parameter coming from, it is not defined in the VirtualMachineRelocateSpec:
https://www.vmware.com/support/developer/vc-sdk/visdk2xpubs/ReferenceGuide/vim.vm.RelocateSpec.html
Any word on this getting reviewed and added? |
This is still WIP, tryin to see if and when we can get this done :( |
I am going to close this, but it will still be on my fork. If someone wants to pick this up, please let me know. I have not had client sponsored time to pick up this work, hopefully in the new year. |
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
This is working for a couple of use cases, but getting errors with multiple disks.