-
Notifications
You must be signed in to change notification settings - Fork 453
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
feat: add instant clone #1516
base: main
Are you sure you want to change the base?
feat: add instant clone #1516
Conversation
@redeux @markpeek @aareet @mfilipiak-ccg The Old PR CLA is resolved. Please take a look |
@Anant99-sys, taking a moment to quickly scan the pull request and noticed that the Ryan Johnson |
@tenthirtyam Can you please guide me as to what to do for updating the documentation for vsphere_virtual_machine? |
@Anant99-sys, I'll follow up over Slack next week to discuss this feature with you along with the documentation and tests. |
vsphere/internal/helper/virtualmachine/virtual_machine_helper.go
Outdated
Show resolved
Hide resolved
vsphere/internal/helper/virtualmachine/virtual_machine_helper.go
Outdated
Show resolved
Hide resolved
vsphere/internal/helper/virtualmachine/virtual_machine_helper.go
Outdated
Show resolved
Hide resolved
vsphere/internal/vmworkflow/virtual_machine_clone_subresource.go
Outdated
Show resolved
Hide resolved
vsphere/internal/vmworkflow/virtual_machine_clone_subresource.go
Outdated
Show resolved
Hide resolved
CheckDestroy: testAccResourceVSphereVirtualMachineCheckExists(false), | ||
Steps: []resource.TestStep{ | ||
{ | ||
Config: testAccResourceVSphereVirtualMachineConfigInstant(), |
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.
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.
So what change should I do regarding this?
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.
Hi @Anant99-sys,
For the updates to the documentation, update the website/docs/r/virtual_machine.html.markdown
file with the information for the instant_clone
argument.
terraform-provider-vsphere/website/docs/r/virtual_machine.html.markdown
Lines 1022 to 1031 in 3bc2ce9
The options available in the `clone` block are: | |
* `template_uuid` - (Required) The UUID of the source virtual machine or template. | |
* `linked_clone` - (Optional) Clone the virtual machine from a snapshot or a template. Default: `false`. | |
* `timeout` - (Optional) The timeout, in minutes, to wait for the cloning process to complete. Default: 30 minutes. | |
* `customize` - (Optional) The customization spec for this clone. This allows the user to configure the virtual machine post-clone. For more details, see [virtual machine customizations](#virtual-machine-customizations). | |
Ryan Johnson
Staff II Solutions Architect | VMware, Inc.
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.
Not critical, but we may need to limit to 6.7 and higher, akin to the following for instant_clone
:
version := viapi.ParseVersionFromClient(client)
if version.Newer(viapi.VSphereVersion{Product: version.Product, Major: 6, Minor: 7}) {
....
}
return obj
But something easy to remove after the end of 6.x support.
Resolved conflicts and errors; all checks have now passed. Ready for testing once again. Ryan Johnson |
Hi, any updates on this? |
Please refer to the tentative milestone assignment. |
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.
In general it looks working, except the comments which need to be addressed.
@@ -2563,6 +2581,39 @@ func testAccResourceVSphereVirtualMachinePreCheck(t *testing.T) { | |||
} | |||
} | |||
|
|||
func testAccResourceVSphereVirtualMachinePreInstantCloneCheck(t *testing.T) { |
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.
It was not necessary the entire testAccResourceVSphereVirtualMachinePreCheck
function to be copied, a single function containing just the check
if os.Getenv("TF_VAR_VSPHERE_USE_INSTANT_CLONE") == "" { t.Skip("set TF_VAR_VSPHERE_USE_INSTANT_CLONE to run vsphere_virtual_machine acceptance tests") }
could be implemeted and added in the preCheck property like for example it is done in TestAccResourceVSphereVirtualMachine_basic
|
||
clone { | ||
template_uuid = "${data.vsphere_virtual_machine.template.id}" | ||
instant_clone = "${var.instant_clone != "" ? "true" : "false" }" |
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.
instant_clone
should be explicitly set to true since the test will be skipped in every case if the TF_VAR_VSPHERE_USE_INSTANT_CLONE is set to something falsy
@@ -3418,6 +3469,16 @@ func testAccResourceVSphereVirtualMachineConfigBase() string { | |||
testhelper.ConfigDataRootPortGroup1()) | |||
} | |||
|
|||
func testAccResourceVSphereVirtualMachineInstantCloneConfigBase() string { | |||
return testhelper.CombineConfigs( |
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.
A minimal set of required resources should be specified here
// InstantClone wraps the creation of a virtual machine instantly and the subsequent waiting of | ||
// the task. A higher-level virtual machine object is returned. | ||
func InstantClone(c *govmomi.Client, src *object.VirtualMachine, f *object.Folder, name string, spec types.VirtualMachineInstantCloneSpec, timeout int) (*object.VirtualMachine, error) { | ||
log.Printf("[DEBUG] Instant Cloning virtual machine %q", fmt.Sprintf("%s/%s", f.InventoryPath, name)) |
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.
Since the Folder pointer and the source VM name are used only for logging and not for the logic inside the function the logging should be done outside the function call / the logging should be changed not to use these variables
} | ||
|
||
resource "vsphere_virtual_machine" "vm" { | ||
name = "testacc-test" |
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.
Use minumum required set of properties for the test, for example the hardware like CPU, Memory, Disk are not necessary since a vm is just being copied and those cant be changed + what will happen if the values in the HCL are different then the values from the template VM ?
2498f89
to
ad02e3b
Compare
@Anant99-sys - could you review the current comments? Perhaps we can put this one forward now... |
Adds support for instant clones.
ad02e3b
to
1eb031f
Compare
return nil, fmt.Errorf("no storage DRS recommendations were found for the requested action (type: %q)", sps.Type) | ||
} | ||
// result to pin disks to recommended datastores | ||
ds := recs[0].Action[0].(*types.StoragePlacementAction).Destination |
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.
I would suggest checking len(recs[0].Action) before accessing.
If it is always populated by design you can add a comment, otherwise it looks like a panic-prone line
"instant_clone": { | ||
Type: schema.TypeBool, | ||
Optional: true, | ||
Description: "Whether or not to create a instant clone when cloning. When this option is used, the source VM must be in a running state.", |
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.
nit: "..an instant.."
* `linked_clone` - (Optional) Clone the virtual machine from a snapshot or a template. Default: `false`. | ||
* `instant_clone` - (Optional) Clone the virtual machine from a running powered on virtual machine. Supported on vSphere 6.7 and later. Default: `false`. Only one of `instant_clone` or `linked_clone` can be set to `true`. | ||
|
||
* `linked_clone` - (Optional) Clone the virtual machine from a snapshot or a template. Default: `false`. Only one of `linked_clone` or `instant_clone` can be set to `true`. |
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.
The fact that the two clone modes are mutually exclusive is not actually enforced.
By the looks of it if both are set to true the provider will attempt to do an instant clone.
Just a suggestion - it would be great if you can figure out a neat way to validate that the user is not trying to apply an invalid configuration
Description
Implementation for instant clone functionality
Instant cloning is very convenient for large-scale application deployments because it ensures memory efficiency and allows for creating numerous virtual machines on a single host.
Acceptance tests
Output from acceptance testing:
Release Note
Release note for CHANGELOG:
References
Old PR: #1371