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

Allows to create empty VM #484

Merged
merged 78 commits into from
Apr 28, 2020
Merged

Allows to create empty VM #484

merged 78 commits into from
Apr 28, 2020

Conversation

vbauzys
Copy link
Contributor

@vbauzys vbauzys commented Apr 1, 2020

Ref: https://github.com/terraform-providers/terraform-provider-vcd/issues/85
Depends on: vmware/go-vcloud-director#296

This allows users to create VM without template.

  • Made existing template_name/catalog_name optional
  • Additionally empty VM allows use/update description
  • New fields added boot_image, hardware_version, os_type
  • Power off on delete handling improved

vbauzys added 30 commits July 15, 2019 15:39
Signed-off-by: Vaidotas Bauzys <[email protected]>
Merge branch 'master' of github.com:terraform-providers/terraform-provider-vcd
Merge branch 'master' of github.com:terraform-providers/terraform-provider-vcd
Merge branch 'master' of github.com:terraform-providers/terraform-provider-vcd
vbauzys and others added 8 commits April 16, 2020 08:10
Signed-off-by: Vaidotas Bauzys <[email protected]>

# Conflicts:
#	go.sum
#	vcd/resource_vcd_vapp_vm.go
#	vendor/modules.txt
Merge branch 'master' of github.com:terraform-providers/terraform-provider-vcd
Merge branch 'master' of github.com:terraform-providers/terraform-provider-vcd
Signed-off-by: Vaidotas Bauzys <[email protected]>

# Conflicts:
#	CHANGELOG.md
#	go.sum
#	vendor/modules.txt
Signed-off-by: Vaidotas Bauzys <[email protected]>
…provider-vcd into create-empty-vm

Signed-off-by: Vaidotas Bauzys <[email protected]>

# Conflicts:
#	CHANGELOG.md
Signed-off-by: Vaidotas Bauzys <[email protected]>
Copy link
Collaborator

@Didainius Didainius left a comment

Choose a reason for hiding this comment

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

Before I forget I must ask to update the message in "index" page:
NOTE: Version 2.8 does not support NSX-T yet. -> NOTE: Version 2.9 does not support NSX-T yet.

One more thought about real life example and wondering if it works well. Imagine - I create a VM and specify a boot image. At the same time it needs to have some disks defined, but disk definition occurs after I have created a VM and specified boot media already. There is some sort of chicken and egg problem.

resource "vcd_vapp" "empty" {
    name = "testing-empty-vm"
}

resource "vcd_vapp_vm" "emptyVM" {
  vapp_name     = vcd_vapp.empty.name
  name          = "VmWithoutTemplate"
  computer_name = "empty-computer"
  memory        = 2048
  cpus          = 2
  cpu_cores     = 1

  os_type          = "sles10_64Guest"
  hardware_version = "vmx-14"
  boot_image       = "dsl-4.11.rc2.iso"

}


resource "vcd_vm_internal_disk" "disk1" {
  vapp_name       = vcd_vapp.empty.name
  vm_name         = vcd_vapp_vm.emptyVM.name
  bus_type        = "sata"
  size_in_mb      = "13333"
  bus_number      = 0
  unit_number     = 0
  storage_profile = "Development"
  allow_vm_reboot = true
}

@vbauzys
Copy link
Contributor Author

vbauzys commented Apr 24, 2020

Before I forget I must ask to update the message in "index" page:
NOTE: Version 2.8 does not support NSX-T yet. -> NOTE: Version 2.9 does not support NSX-T yet.

One more thought about real life example and wondering if it works well. Imagine - I create a VM and specify a boot image. At the same time it needs to have some disks defined, but disk definition occurs after I have created a VM and specified boot media already. There is some sort of chicken and egg problem.

resource "vcd_vapp" "empty" {
    name = "testing-empty-vm"
}

resource "vcd_vapp_vm" "emptyVM" {
  vapp_name     = vcd_vapp.empty.name
  name          = "VmWithoutTemplate"
  computer_name = "empty-computer"
  memory        = 2048
  cpus          = 2
  cpu_cores     = 1

  os_type          = "sles10_64Guest"
  hardware_version = "vmx-14"
  boot_image       = "dsl-4.11.rc2.iso"

}


resource "vcd_vm_internal_disk" "disk1" {
  vapp_name       = vcd_vapp.empty.name
  vm_name         = vcd_vapp_vm.emptyVM.name
  bus_type        = "sata"
  size_in_mb      = "13333"
  bus_number      = 0
  unit_number     = 0
  storage_profile = "Development"
  allow_vm_reboot = true
}

Well we can't manage disks in VM resource. Overall it's not 100% nice, but boot image is mounted to cd-rom. You can handle VM with power_on when you assing internal disk.

@Didainius
Copy link
Collaborator

Well we can't manage disks in VM resource. Overall it's not 100% nice, but boot image is mounted to cd-rom. You can handle VM with power_on when you assing internal disk.

I was able to install ubuntu semi-manually. Of course it is based on the fact that installer took some time and did not scan for disks until the actual disk was added. Other way to work on it is to setup the scene with power_on=false as you mentioned.

Signed-off-by: Vaidotas Bauzys <[email protected]>
@@ -197,8 +216,8 @@ The following arguments are supported:
* `vapp_name` - (Required) The vApp this VM belongs to.
* `name` - (Required) A name for the VM, unique within the vApp
* `computer_name` - (Optional; *v2.5+*) Computer name to assign to this virtual machine.
* `catalog_name` - (Required) The catalog name in which to find the given vApp Template
* `template_name` - (Required) The name of the vApp Template to use
* `catalog_name` - (Optional; *v2.8+*) The catalog name in which to find the given vApp Template
Copy link
Collaborator

Choose a reason for hiding this comment

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

2.8 should be repaced to 2.9 across this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

Signed-off-by: Vaidotas Bauzys <[email protected]>
Copy link
Collaborator

@Didainius Didainius left a comment

Choose a reason for hiding this comment

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

I have tested the following:
vCD 10.1: upgrade, binary, acceptance
vCD 9.7: acceptance and org user acceptance tests
vcd 10.0: acceptance .

vbauzys added 2 commits April 27, 2020 17:08
Signed-off-by: Vaidotas Bauzys <[email protected]>
Signed-off-by: Vaidotas Bauzys <[email protected]>
Copy link
Collaborator

@lvirbalas lvirbalas left a comment

Choose a reason for hiding this comment

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

Really useful feature when you use it together with the new boot_image field, LGTM now!

Copy link
Contributor

@dataclouder dataclouder left a comment

Choose a reason for hiding this comment

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

I have some important questions and a few minor changes.

)

if testConfig.Media.MediaName == "" {
fmt.Print("Warning: `MediaName` isn't configured, boot image won't be tested.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
fmt.Print("Warning: `MediaName` isn't configured, boot image won't be tested.")
fmt.Println("Warning: `MediaName` is not configured: boot image won't be tested.")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

}
catalog, err := org.GetCatalogByName(catalogName, false)
if err != nil {
return fmt.Errorf("error finding catalog %s: %s", d.Get("catalog_name").(string), err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return fmt.Errorf("error finding catalog %s: %s", d.Get("catalog_name").(string), err)
return fmt.Errorf("error finding catalog %s: %s", catalogName, err)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

if err != nil {
return fmt.Errorf("[vm creation] error retrieving storage profile %s : %s", storageProfileName, err)
return fmt.Errorf("error finding catalog item: %s", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return fmt.Errorf("error finding catalog item: %s", err)
return fmt.Errorf("error finding catalog item %s: %s", templateName, err)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

@@ -188,6 +188,25 @@ resource "vcd_nsxv_ip_set" "test-ipset" {
}
```

## Example Usage (Empty VM)
This example shows how to create empty VM.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
This example shows how to create empty VM.
This example shows how to create an empty VM.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

return fmt.Errorf("[VM update] error quering boot image %s : %s", previousBootImageValue, err)
}
if len(result) > 1 {
return fmt.Errorf("[VM update] error found one than more boot image %s : %s", previousBootImageValue, err)
Copy link
Contributor

@dataclouder dataclouder Apr 27, 2020

Choose a reason for hiding this comment

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

Suggested change
return fmt.Errorf("[VM update] error found one than more boot image %s : %s", previousBootImageValue, err)
return fmt.Errorf("[VM update] error found more than one boot image %s", previousBootImageValue)

The error is nil and should not be shown here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated


* `os_type` - (Optional; *v2.9+*) Operating System type. Possible values can be found in [Os Types](#os-types). Required when creating empty VM.
* `hardware_version` - (Optional; *v2.9+*) Virtual Hardware Version (e.g.`vmx-14`, `vmx-13`, `vmx-12`, etc.). Required when creating empty VM.
* `boot_image` - (Optional; *v2.9+*) Media name to mount as boot image which can be found in any accessible catalog. Image is mounted only during VM creation. On update if value is changed to empty it will eject the mounted media. If you want to mount an image later, please use [vcd_inserted_media](/docs/providers/vcd/r/inserted_media.html).
Copy link
Contributor

Choose a reason for hiding this comment

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

I have a question about this:
If I have two boot images with the same name in two different catalogs, the query will return two items, and the creation will fail. How do I say that I want the image from catalog 1 to be used in this case? In other words, how do I avoid the error that I would not have for the vApp template, which must belong to a specific catalog? It seems to me that we have introduced a behavior inconsistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As we agreed, add catalog name handling together with boot image.

return nil, fmt.Errorf("[VM creation] error getting boot image %s : %s", bootImageName, err)
}
if len(result) > 1 {
return nil, fmt.Errorf("[VM creation] error found more than one boot image %s : %s", bootImageName, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

the error is nil and should not be shown here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

vbauzys added 6 commits April 28, 2020 10:42
Signed-off-by: Vaidotas Bauzys <[email protected]>
Signed-off-by: Vaidotas Bauzys <[email protected]>
Signed-off-by: Vaidotas Bauzys <[email protected]>
Signed-off-by: Vaidotas Bauzys <[email protected]>
Copy link
Contributor

@dataclouder dataclouder left a comment

Choose a reason for hiding this comment

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

LGTM

Signed-off-by: Vaidotas Bauzys <[email protected]>
@vbauzys vbauzys merged commit 8cad64f into vmware:master Apr 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants