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

vSphere: Support mounting ISO images to virtual cdrom drives. #4243

Merged
merged 1 commit into from
Apr 21, 2016

Conversation

kristinn
Copy link
Contributor

@kristinn kristinn commented Dec 9, 2015

It can come in handy to be able to mount ISOs programmatically.
For instance if you're developing a custom appliance (that automatically installs itself on the hard drive volume)
that you want to automatically test on every successful build (given the ISO is uploaded to the vmware datastore).

There are probably lots of other reasons for using this functionality.

@kristinn
Copy link
Contributor Author

kristinn commented Dec 9, 2015

Please verify the new acceptance test (the one I just created) works for you as well. I have verified it works on my vmware setup.

@tkak
Copy link
Contributor

tkak commented Dec 14, 2015

I'll try to check the acceptance test. 👍

@kristinn
Copy link
Contributor Author

Ok great! 😄

@tkak
Copy link
Contributor

tkak commented Dec 18, 2015

@kristinn I tried to check the acceptance test. But, I got the following error messages.

TF_ACC=1 go test ./builtin/providers/vsphere -v -run=VirtualMachine_createWithCdrom -timeout 90m
=== RUN   TestAccVSphereVirtualMachine_createWithCdrom
--- FAIL: TestAccVSphereVirtualMachine_createWithCdrom (113.61s)
    testing.go:144: Step 0 error: Error applying: 1 error(s) occurred:

        * vsphere_virtual_machine.with_cdrom: Invalid configuration for device '0'.
FAIL
exit status 1
FAIL    github.com/hashicorp/terraform/builtin/providers/vsphere    113.639s
Makefile:37: recipe for target 'testacc' failed
make: *** [testacc] Error 1

This is a hardware information of the new VM.

☁  ~  govc device.ls -vm.ipath=/xxx/vm/terraform-test-with-cdrom                                                                                                         [~]
ide-200       VirtualIDEController       IDE 0
ide-201       VirtualIDEController       IDE 1
ps2-300       VirtualPS2Controller       PS2 controller 0
pci-100       VirtualPCIController       PCI controller 0
sio-400       VirtualSIOController       SIO controller 0
keyboard-600  VirtualKeyboard            Keyboard
pointing-700  VirtualPointingDevice      Pointing device; Device
video-500     VirtualMachineVideoCard    Video card
vmci-12000    VirtualMachineVMCIDevice   Device on the virtual machine PCI bus that provides support for the virtual machine communication interface
pvscsi-1000   ParaVirtualSCSIController  VMware paravirtual SCSI
cdrom-3002    VirtualCdrom               Remote device CD/DVD drive 1
disk-1000-0   VirtualDisk                31,457,280 KB
ethernet-0    VirtualVmxnet3             DVSwitch: ee c6 25 50 c0 e8 4f 08-c9 31 b4 f8 fb a3 3b 98
☁  ~

@kristinn
Copy link
Contributor Author

@tkak Ok I see. I will take a look at this. Thank you for running the test.

@kristinn
Copy link
Contributor Author

@tkak Did you export all the required environment variables?

cdromDatastore := os.Getenv("VSPHERE_CDROM_DATASTORE")
cdromPath := os.Getenv("VSPHERE_CDROM_PATH")

These ones are new ones that I introduced in my acceptance test.

The cdromDatastore needs to be the name of your datastore as it shows up in your vmware console. The cdromPath needs to be an absolute path to an ISO within the cdromDatastore.

I can't see the ISO being mounted in the information you provided about the virtual machine.

I was running the test and it works for me. The ISO you point to in the environment variables must exist in the datastore.

@tkak
Copy link
Contributor

tkak commented Dec 22, 2015

@kristinn Yes, I set the environment variables. But I got the error message.

Anyway, I resolved the issue by moving a location to call createCdroms(newVM, vm.cdroms) to before newVM.Customize(context.TODO(), customSpec). And the acceptance test works in our environment. So, could you fix that?

==> Checking that code complies with gofmt requirements...
go generate ./...
TF_ACC=1 go test ./builtin/providers/vsphere -v -run=VirtualMachine_createWithCdrom -timeout 90m
=== RUN   TestAccVSphereVirtualMachine_createWithCdrom
--- PASS: TestAccVSphereVirtualMachine_createWithCdrom (334.15s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/vsphere    334.188s

@@ -1231,6 +1325,12 @@ func (vm *virtualMachine) deployVirtualMachine(c *govmomi.Client) error {
return err
}
}

// Create the cdroms if needed.
if err := createCdroms(newVM, vm.cdroms); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you move this to L1310 (before the newVM.Customize(context.TODO(), customSpec))?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, ok so that's the reason.

I will do that. Thanks a lot!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One question. Shouldn't it call createCdroms() from both createVirtualMachine() and from deployVirtualMachine()?

Isn't deployVirtualMachine() used for cloning but createVirtualMachine() when creating a brand new vm?

@kristinn kristinn force-pushed the vsphere-cdrom-support branch 2 times, most recently from 201953e to 9fa7e5e Compare December 22, 2015 10:17
@kristinn
Copy link
Contributor Author

Now I've updated the code so the cdrom creation is before the customization call. 😄

@kristinn
Copy link
Contributor Author

I only moved the createCdroms() call in deployVirtualMachine(). I didn't move it in the createVirtualMachine() function since there is no customization call there. I did reply to your comment on the code where you ask me to move createCdroms() around. There you can see my explanation and a question.

@tkak
Copy link
Contributor

tkak commented Jan 7, 2016

@kristinn Thank you for your modification. 😄 I think it looks good. There isn't the error message in case of createVirtualMachine() as you say. Thanks!

@kristinn
Copy link
Contributor Author

kristinn commented Jan 7, 2016

Awesome! 😄

@kristinn
Copy link
Contributor Author

@chrislovecnm it's actually this one we should merge since it provides support for mounting isos from different datastores than the vm is hosted on.

@chrislovecnm
Copy link
Contributor

@jen20 @phinze please merge ;)

@kristinn
Copy link
Contributor Author

I will rebase this to fix the conflicts.

@chrislovecnm
Copy link
Contributor

@kristinn thanks

It can come in handy to be able to mount ISOs programmatically.
For instance if you're developing a custom appliance (that automatically installs itself on the hard drive volume)
that you want to automatically test on every successful build (given the ISO is uploaded to the vmware datastore).

There are probably lots of other reasons for using this functionality.
@@ -71,6 +72,9 @@ The `windows_opt_config` block supports:
* `domain_user` - (Optional) User that is a member of the specified domain.
* `domain_user_password` - (Optional) Password for domain user, in plain text.

<a id="disks"></a>
## Disks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I decided to add this since it was missing (it's being linked to in line 48, but clicking that link wouldn't do much).

@kristinn
Copy link
Contributor Author

I added information about this change to the website documentation (the markdown files).

This is squashed, I've executed the acceptance test and it seems to be ready for merge - after Travis has built it successfully.

@kristinn
Copy link
Contributor Author

@chrislovecnm Just in case you didn't get a notification about my last comment. 😄

This can be merged now.

@jen20
Copy link
Contributor

jen20 commented Apr 21, 2016

LGTM! Thanks to all involved.

@jen20 jen20 merged commit 5b1b49e into hashicorp:master Apr 21, 2016
@ghost
Copy link

ghost commented Apr 26, 2020

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.

@ghost ghost locked and limited conversation to collaborators Apr 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants