Make disks optional in VM resource

[4censord]

This fixes #194

[ddelnano]

@4censord apologies for the late reply. This code change definitely accomplishes the use case mentioned in #194, however, I want to make sure that we only allow this in the right context.

I assume that cdrom needs to be a required field when the disk argument is omitted. Are there any other cases you can think of aside from that?

In addition ensuring that this new functionality is used properly, we need to add an acceptance test for this. This new test case will be very similar to this. I'll add that on top of your PR once we've determined what the behavior should be. Please let me know if you'd like to attempt that yourself.

[4censord]

@4censord apologies for the late reply.

No need to apologize.

This code change definitely accomplishes the use case mentioned in #194,
however, I want to make sure that we only allow this in the right context.

Sadly it only looks that easy.
Because of special logic to handle the first disk¹ in this doesn't work.

This entails changes to the client code, so we problably want to wait until #176 is merged.

I assume that cdrom needs to be a required field when the disk
argument is omitted. Are there any other cases you can think of aside from that?

If a vm boots from the network, a cdrom would not be required.

In addition ensuring that this new functionality is used properly, we
need to add an acceptance test for this. This new test case will be
very similar to ². I'll add that on top of your PR once we've determined
what the behavior should be. Please let me know if you'd like to attempt
that yourself.

I would like to try this myself, but i would need a bit of guidance

• I do not know how the acceptance tests work
• I do not know what infrastructure is required for running the tests

For me, this is not a time critical problem, so i would like to use this as an oportunity to learn.
The workaround of simply creating a small disk is perfectly workable.

Footnotes

1. https://github.com/terra-farm/terraform-provider-xenorchestra/blob/e2bdec094fdc7b43bce4f7ba173c7d9c82bf0f2f/client/vm.go#L187-L205 ↩

2. https://github.com/terra-farm/terraform-provider-xenorchestra/blob/e2bdec094fdc7b43bce4f7ba173c7d9c82bf0f2f/xoa/resource_xenorchestra_vm_test.go#L558 ↩

[ddelnano]

This entails changes to the client code, so we problably want to wait until #176 is merged.

I've just closed #176. See my final comment for more details, but I'm not sure that the xo-sdk-go will see new development. It should be trivial to upstream any changes from this repo there and I was planning to redesign the API in #189 so until that is done I don't think there is a rush to move forward with xo-sdk-go.

If a vm boots from the network, a cdrom would not be required.

Good point, I had not considered PXE booting.

I do not know how the acceptance tests work
I do not know what infrastructure is required for running the tests

The CONTRIBUTING.md documentation gives an overview of the tests, how to run them and what needs to exist in order for the tests to succeed. Please give that a read and let me know if you have any questions or run into problems.

I can get back to you quicker over Discord as you attempt to run the test suite, so don't hesitate to ask questions there.

[ddelnano]

I ran the test suite for this branch and there is a compilation issue.

diff --git a/xoa/resource_xenorchestra_vm_test.go b/xoa/resource_xenorchestra_vm_test.go
index 2bf506b..343a979 100644
--- a/xoa/resource_xenorchestra_vm_test.go
+++ b/xoa/resource_xenorchestra_vm_test.go
@@ -1590,7 +1590,7 @@ resource "xenorchestra_vm" "bar" {
       id = data.xenorchestra_vdi.iso.id
     }
 }
-`, testIsoName, accTestPool.Id, accDefaultNetwork.NameLabel, accTestPool.Id, vmName, accDefaultSr.Id)
+`, testIsoName, accTestPool.Id, accDefaultNetwork.NameLabel, accTestPool.Id, vmName)
 }

 func testAccVmConfigWithoutISO(vmName string) string {

After fixing that the test fails on the vm.create rpc call. I'm going to update the test and see if I can get it passing, however, I have some questions about how this will be used.

For these 'diskless' VMs will you be installing an OS after it boots (either from cdrom or PXE)? I'm just trying to make sure I understand the use case fully. So if you can share more information on the intended workflow that would be great.

[ddelnano]

After debugging the test more, the error is originating from the fact that the XO api iterates over the VBDs present to identify what SR to use for the cloud config drive (source). Since we are using a diskless template the sr id will be undefined and cause the following error:

2022/12/12 15:47:31 [DEBUG] VM params for vm.create map[string]interface {}{"CPUs":1, "VDIs":[]interface {}{}, "VIFs":[]map[string]string{map[string]string{"mac":"", "network":"6c4e1cdc-9fe0-0603-e53d-4790d1fce8dd"}}, "affinityHost":"", "bootAfterCreate":true, "cloudConfig":"template", "coreOs":false, "cpuCap":interface {}(nil), "cpuWeight":interface {}(nil), "existingDisks":map[string]interface {}{}, "expNestedHvm":false, "hvmBootFirmware":"bios", "installation":map[string]string{"method":"cdrom", "repository":"2c90f6b4-b8e7-4850-b1e9-504baee682e1"}, "memoryMax":4295000000, "name_description":"description", "name_label":"terraform-acc - TestAccXenorchestraVm_createWithoutDisks", "tags":[]string{}, "template":"355ee47d-ff4c-4924-3db2-fd86ae629676-bac315b4-66fd-4488-82f5-9808e927b4b9", "vga":"std", "videoram":8}
2022/12/12 15:47:33 [TRACE] Made rpc call `vm.create` with params: map[CPUs:1 VDIs:[] VIFs:[map[mac: network:6c4e1cdc-9fe0-0603-e53d-4790d1fce8dd]] affinityHost: bootAfterCreate:true cloudConfig:template coreOs:false cpuCap:<nil> cpuWeight:<nil> existingDisks:map[] expNestedHvm:false hvmBootFirmware:bios installation:map[method:cdrom repository:2c90f6b4-b8e7-4850-b1e9-504baee682e1] memoryMax:4295000000 name_description:description name_label:terraform-acc - TestAccXenorchestraVm_createWithoutDisks tags:[] template:355ee47d-ff4c-4924-3db2-fd86ae629676-bac315b4-66fd-4488-82f5-9808e927b4b9 vga:std videoram:8] and received : result with error: jsonrpc2: code -32000 message: no object with UUID or opaque ref: undefined
2022/12/12 15:47:33 [WARN] Got error running Terraform: exit status 1

Error: jsonrpc2: code -32000 message: no object with UUID or opaque ref: undefined: {"message":"no object with UUID or opaque ref: undefined","name":"Error","stack":"Error: no object with UUID or opaque ref: undefined\n    at Xapi.apply (/usr/local/lib/node_modules/xo-server/node_modules/xen-api/src/index.js:632:11)\n    at Xapi.getObject (file:///usr/local/lib/node_modules/xo-server/src/xapi/index.mjs:102:26)\n    at Xapi.apply (file:///usr/local/lib/node_modules/xo-server/src/xapi/index.mjs:1307:21)\n    at Xapi.createCloudInitConfigDrive (/usr/local/lib/node_modules/xo-server/node_modules/golike-defer/src/index.js:85:19)\n    at Xo.<anonymous> (file:///usr/local/lib/node_modules/xo-server/src/api/vm.mjs:209:22)\n    at Api.#callApiMethod (file:///usr/local/lib/node_modules/xo-server/src/xo-mixins/api.mjs:394:20)"}


    resource_xenorchestra_vm_test.go:582: Step 1/1 error: Error running apply: exit status 1

        Error: jsonrpc2: code -32000 message: no object with UUID or opaque ref: undefined: {"message":"no object with UUID or opaque ref: undefined","name":"Error","stack":"Error: no object with UUID or opaque ref: undefined\n    at Xapi.apply (/usr/local/lib/node_modules/xo-server/node_modules/xen-api/src/index.js:632:11)\n    at Xapi.getObject (file:///usr/local/lib/node_modules/xo-server/src/xapi/index.mjs:102:26)\n    at Xapi.apply (file:///usr/local/lib/node_modules/xo-server/src/xapi/index.mjs:1307:21)\n    at Xapi.createCloudInitConfigDrive (/usr/local/lib/node_modules/xo-server/node_modules/golike-defer/src/index.js:85:19)\n    at Xo.<anonymous> (file:///usr/local/lib/node_modules/xo-server/src/api/vm.mjs:209:22)\n    at Api.#callApiMethod (file:///usr/local/lib/node_modules/xo-server/src/xo-mixins/api.mjs:394:20)"}

I updated the test to use a template with a disk and the test fails because there is a diff after the plan is applied. This is expected since the resulting VM does have a disk while the config is diskless.

    resource_xenorchestra_vm_test.go:582: Step 1/1 error: After applying this test step, the plan was not empty.
        stdout:


        An execution plan has been generated and is shown below.
        Resource actions are indicated with the following symbols:
          ~ update in-place

        Terraform will perform the following actions:

          # xenorchestra_vm.bar will be updated in-place
          ~ resource "xenorchestra_vm" "bar" {
                auto_poweron      = false
                cloud_config      = "template"
                core_os           = false
                cpu_cap           = 0
                cpu_weight        = 0
                cpus              = 1
                exp_nested_hvm    = false
                hvm_boot_firmware = "bios"
                id                = "760b133e-1d9a-0057-877d-c76409fd7050"
                ipv4_addresses    = []
                ipv6_addresses    = []
                memory_max        = 4295000000
                name_description  = "description"
                name_label        = "terraform-acc - TestAccXenorchestraVm_createWithoutDisks"
                power_state       = "Running"
                start_delay       = 0
                template          = "9b0fd2ac-c89a-93b4-5103-8506391395cd"
                vga               = "std"
                videoram          = 8
                wait_for_ip       = false

                cdrom {
                    id = "2c90f6b4-b8e7-4850-b1e9-504baee682e1"
                }

              - disk {
                  - attached   = true -> null
                  - name_label = "debian root" -> null
                  - position   = "0" -> null
                  - size       = 4294967296 -> null
                  - sr_id      = "86a9757d-9c05-9fe0-e79a-8243cb1f37f3" -> null
                  - vbd_id     = "76222b48-996e-a4da-d18e-471adbc87213" -> null
                  - vdi_id     = "0c625bdd-c28e-4b6b-818f-756bef18e989" -> null
                }

                network {
                    attached       = true
                    device         = "0"
                    ipv4_addresses = []
                    ipv6_addresses = []
                    mac_address    = "9a:c8:0a:91:c2:b1"
                    network_id     = "6c4e1cdc-9fe0-0603-e53d-4790d1fce8dd"
                }
            }

        Plan: 0 to add, 1 to change, 0 to destroy.

Understanding your workflow for this 'diskless' VM will be important to determine how to resolve these issues. Maybe the second option can work with the ignore_changes lifecycle meta argument on the disk attribute.

[4censord]

For these 'diskless' VMs will you be installing an OS after it boots
(either from cdrom or PXE)? I'm just trying to make sure I understand
the use case fully. So if you can share more information on the
intended workflow that would be great.

Yes, I am live booting my VMs.

I am running some stateless services like (DNS-servers, reverse/caching proxys,
the like) in VMs that don't need disks.

The OS boots fully life from an ISO (or PXE), and gets some
configuration via cloud-init.

This workflow is working already very nicely, with every vm having a
small (1GiB) disk that isn't used at all.

I can't seem to run the acceptance tests myself right now:

=== RUN   TestAccXenorchestraVm_createWithoutDisks
=== PAUSE TestAccXenorchestraVm_createWithoutDisks
=== CONT  TestAccXenorchestraVm_createWithoutDisks
    resource_xenorchestra_vm_test.go:582: Step 1/1 error: unsupported state format version: expected "0.1", got "1.0"
    testing_new.go:56: Error retrieving state, there may be dangling resources: unsupported state format version: expected "0.1", got "1.0"
--- FAIL: TestAccXenorchestraVm_createWithoutDisks (0.12s)
FAIL
[DEBUG] Running sweeper
FAIL	github.com/ddelnano/terraform-provider-xenorchestra/xoa	3.615s
testing: warning: no tests to run
PASS
ok  	github.com/ddelnano/terraform-provider-xenorchestra/xoa/internal	0.008s [no tests to run]
FAIL
make: *** [Makefile:40: testacc] Error 1

[ddelnano]

@4censord thanks for clarifying. So based on my current understanding of the XO api, we still need to provide a disk because that's how the cloudinit integration works.

I need to investigate this a bit more before I know the options that are available.

As for your issues with the acceptance tests, I haven't seen that error before. Can you share the output when the following environment variable is set: TF_LOG=trace. I would try to run a single test since that's going to cause a significant amount of output.