Enhancement: Added resource for snapshot and revert snapshot #107

idmsubs · 2017-07-21T08:46:47Z

Please review code changes implemented for vSphere snapshot management resources.

Resources implemented are :
a. Snapshot management (Create and delete snapshot)
b. Revert snapshot

Referred https://github.com/hashicorp/terraform/blob/master/.github/CONTRIBUTING.md for following standards for Terraform provider.

Please suggest changes if required any.

girishramnani · 2017-08-01T11:55:37Z

@idmsubs revert snapshot seems more like an action then a resource so instead of having a resource if there is a revert_to_this or similar named key on the snapshot resource. This key if is kept true then the state of the system is on that snapshot, Making it more declarative then procedural.

Any suggestions?.

idmsubs · 2017-08-09T06:41:00Z

@girishramnani that can be possible, but to avoid additional checks for mandatory and non-mandatory input parameters of create_snapshot and revert_snapshot I have created separate resource for revert_snapshot. Like for create we require "description", "memory", "quiesce", "remove_children", "consolidate", and for revert requires "suppress_power_on" - these are irrelevant in both and with suggested way use will have provide all values in any of case (revert flag = true/false).

Let me know your thoughts on this.

girishramnani · 2017-08-09T06:58:35Z

Yeah totally get that, but I am bit concerned about a state where someone removes the snapshot which is referred by snapshot_name of revert_snapshot.

There is also one more option. What if the virtual machine resource has an optional field called snapshot_name which can be interpolated as ${vsphere_snapshot.snap.id}

this can also help in cleanup process i.e. remove the snapshot if the vm is destroyed ( if the user wants that ).

NOTE - here snap is a snapshot resource that was created.

idmsubs · 2017-08-09T09:08:40Z

about a state where someone removes the snapshot which is referred by snapshot_name of revert_snapshot.
if someone removes snapshot manually and tries to remove it again with resource, apply will fail as per my implementation.

one more option.
this way is possible, but consider actual use with this option. User will create virtual machine (as its new, no snapshot name will be provided). Later whenever user wants to create snapshot, user will have to do apply for create virtual machine, with providing mandatory values of virtual machine just for snapshot create. In that we can add this option for create virtual disk also :)

remove the snapshot if the vm is destroyed
this user still can do with multiple resources in microservice, right? User can delete snapshot and then delete VM.

girishramnani · 2017-08-09T10:14:05Z

Later whenever user wants to create snapshot, user will have to do apply for create virtual machine - Correct me if I am wrong but I think this is how life cycle would look like -

If the user wants to create a snapshot then they would add a vsphere_snapshot resource which would look like this.


resource "vsphere_virtual_machine" "testing" {
     ...
}
resource "vsphere_snapshot" "Test_terraform_cases"{
       vm_name = "snapshot"
 	snapshot_name = "SnapshotForTestingTerraform"
	description = "This is snpashot created for testing and will be deleted."
 	memory = "true"
 	quiesce = "true"
 	remove_children = "false"
 	consolidate = "true"
 }

The moment user want to revert to the specific snapshot they will provide the snapshot name to the vm


resource "vsphere_virtual_machine" "testing" {
     ...
     snapshot_name ="${vsphere_snapshot.Test_terraform_cases.id}"
}
resource "vsphere_snapshot" "Test_terraform_cases"{
       vm_name = "testing"
 	snapshot_name = "SnapshotForTestingTerraform"
	description = "This is snpashot created for testing and will be deleted."
 	memory = "true"
 	quiesce = "true"
 	remove_children = "false"
 	consolidate = "true"
 }

This will trigger a update lifecycle method in the vm resource where you can find the vm and apply the snapshot ( the logic present in the create lifecycle of the revert snapshot ).

Any thoughts on this approach?

girishramnani · 2017-08-09T10:16:46Z

vsphere/resource_vsphere_snapshot_test.go

+const testAccCheckVmSnapshotConfig_basic = `
+resource "vsphere_snapshot" "Test_terraform_cases"{
+
+	snapshot_name = "SnapshotForTestingTerraform"


Could you please add values for required parameters such as vm_name and folder.

girishramnani · 2017-08-09T10:17:53Z

vsphere/resource_vsphere_snapshot_test.go

+	}
+}
+
+func TestAccVmSnanpshot_Basic(t *testing.T) {


Could you please solve the typo here.

vancluever

Hey @idmsubs - thanks for the PR!

I have included comments inline - can you please review and action based on the feedback!

We also require documentation for a PR to be accepted - can you please review https://github.com/hashicorp/terraform-website for details on how to set up a preview version of the website so that documentation can be authored for this resource?

Also, I had some questions about the resources in general and maybe you can help me understand. Since VM snapshots in vSphere are generally short-lived resources, are not suitable by themselves for use as backups, and can lead to performance issues and data loss if not managed correctly, can you give me some background on a good use case for these resources? I'm inparticularly wondering about the revert resource, for which the lifecycle of the resource would be pretty much over upon creation (unless you wanted to revert to that snapshot every time terraform apply was run). Maybe if you could relate the individual use case that would help?

Thanks!

vancluever · 2017-08-10T21:19:06Z

vsphere/resource_vsphere_snapshot_test.go

+)
+
+func testBasicPreCheckSnapshot(t *testing.T) {
+


Can this leading space be removed?

vancluever · 2017-08-10T21:19:19Z

vsphere/resource_vsphere_snapshot_test.go

+}
+
+func testAccCheckVmSnapshotDestroy(s *terraform.State) error {
+


This one as well please!

vancluever · 2017-08-10T21:20:31Z

vsphere/resource_vsphere_snapshot_test.go

+
+const testAccCheckVmSnapshotConfig_basic = `
+resource "vsphere_snapshot" "Test_terraform_cases"{
+  	vm_name = "vmForTesting"


I notice the VM name is not parameterized here and the VM is not created with a vsphere_virtual_machine resource. Is there something I'm missing on whether or not this test will actually run if vmForTesting does not exist?

Also just noticed here - can you make sure this curly bracer is properly spaced (ie: resource "vsphere_snapshot" "Test_terraform_cases" {)? If you have a way to filter specific text through terraform fmt -, that would be a good idea.

vancluever · 2017-08-10T21:22:25Z

vsphere/resource_vsphere_revert_snapshot.go

+import (
+	"fmt"
+	"github.com/hashicorp/terraform/helper/schema"
+	"golang.org/x/net/context"


Can you change the context package used here to the standard library context package (versus golang.org/x/net/context)?

vancluever · 2017-08-10T21:22:37Z

vsphere/resource_vsphere_revert_snapshot_test.go

+	"github.com/vmware/govmomi"
+	"github.com/vmware/govmomi/find"
+	"github.com/vmware/govmomi/vim25/mo"
+	"golang.org/x/net/context"


Can you change the context package used here to the standard library context package (versus golang.org/x/net/context)?

vancluever · 2017-08-10T21:27:40Z

vsphere/resource_vsphere_snapshot.go

+				Type:        schema.TypeString,
+				Required:    true,
+				ForceNew:    true,
+				DefaultFunc: schema.EnvDefaultFunc("VSPHERE_VM_NAME", nil),


I don't think a DefaultFunc is necessary here - if anything it might lead to confusion or data loss if it's defined by mistake and not documented - can you remove it?

If this was being used for testing, it is probably better to generate your configuration on the fly with a configuration generation function in the _test.go file.

vancluever · 2017-08-10T21:27:58Z

vsphere/resource_vsphere_snapshot.go

+				Type:        schema.TypeString,
+				Required:    true,
+				ForceNew:    true,
+				DefaultFunc: schema.EnvDefaultFunc("VSPHERE_VM_FOLDER", nil),


Same with this DefaultFunc - if you can remove it and re-factor the tests it would be great!

vancluever · 2017-08-10T21:43:06Z

vsphere/resource_vsphere_revert_snapshot.go

+				ForceNew:    true,
+				DefaultFunc: schema.EnvDefaultFunc("VSPHERE_VM_FOLDER", nil),
+			},
+			"snapshot_name": {


Can this be changed to something a bit more unique? It seems like FindSnapshot takes a ManagedObjectReference.Value that might be more suitable for sourcing as a snapshot ID. This will allow for the removal of the VM and folder name too and hence should make snapshot lookup easier and less fragile.

vancluever · 2017-08-10T21:48:02Z

vsphere/provider.go

@@ -68,6 +68,8 @@ func Provider() terraform.ResourceProvider {
 			"vsphere_folder":          resourceVSphereFolder(),
 			"vsphere_virtual_disk":    resourceVSphereVirtualDisk(),
 			"vsphere_virtual_machine": resourceVSphereVirtualMachine(),
+			"vsphere_snapshot":        resourceVSphereSnapshot(),


Can we change the name of these resources? I think it would be helpful to people who are unfamiliar with how snapshots work to understand that a snapshot is not a snapshot of a disk but a VM. Hence, how about vsphere_virtual_machine_snapshot and vsphere_virtual_machine_snapshot_revert?

vancluever · 2017-08-10T21:48:47Z

vsphere/resource_vsphere_snapshot.go

+		return fmt.Errorf(" Error While waiting for the Task for Create Snapshot: %s", err)
+	}
+	log.Printf("[INFO] Create Snapshot completed %v", d.Get("snapshot_name").(string))
+	d.SetId(d.Get("snapshot_name").(string))


Can this be changed to something a bit more unique? If it's suitable, I would prefer this to be the value of the MO reference that comes back.

More info: https://pubs.vmware.com/vsphere-6-5/index.jsp#com.vmware.wssdk.apiref.doc/vim.VirtualMachine.html#createSnapshot

This could then be relayed into the vsphere_snapshot_revert resource, making it much less fragile.

vancluever · 2017-08-17T22:55:22Z

Hey @idmsubs - just noticed that there's no response yet to this one. Would you be able to follow up and address the review comments and concerns I had about the business case of these resources?

I'll keep the PR open for another week. Also, if it's not a good time, we can always just close it off and you can re-submit it at a later time.

Thanks again!

idmsubs · 2017-08-18T09:36:52Z

@vancluever we are working on making required changes for incorporating review comments, hoping to get those done and committed by early in next week. For your complete usecase understanding question, yes we will post those details too.

Sorry for delayed response.

vancluever · 2017-08-23T03:10:15Z

Hey @idmsubs, that would be helpful! Will wait to hear from you more on this one.

idmsubs · 2017-08-24T05:59:01Z

@vancluever we have fixed your review comments and also resolved conflicts. Please review and let me know if any additional comments. For detailed usecase explanation will send it today.

idmsubs

Fixed review comments. Please review again.

vancluever · 2017-08-25T15:39:33Z

Hey @idmsubs, one thing I still don't necessarily see here is the use case for this resource. Would you be able to review the concerns that I had regarding the necessity or feasibility for a VM snapshot resource in Terraform, given the concerns surrounding snapshot age and the potential for technical issues and data loss, and get back to me with your thoughts?

Again, I'm concerned around the fact that VM snapshots are not disk snapshots, are not designed to stick around for a long time, and have a lot of potential for data loss issues if not treated properly. Also, the revert resource does not really seem like it belongs in Terraform and would just be better suited for a CLI call at any in particular point in time.

PS: In the bigger picture of the HashiCorp ecosystem, I would more see snapshots being a part of Packer, where the VMware builders created a snapshot with a template that could then be used for linked clones.

Thanks!

idmsubs · 2017-08-28T09:22:38Z

@vancluever vSphere snapshots are basically restore_points created of VM which can be used later to make VM running on particular restore_point by simply reverting snapshot.

Consider deployment of server with steps as (using Terraform) :

VM deployment with vSphere provider
Addition of disk to VM
Installation & configuration of some complex software on deployed VM
Taking snapshot to create restore_point

Now, with use of deployed VM, user might require to take snapshot later to create additional restore_points. Similarly, can use revert resource to reverting back to particular snapshot, this will help in reverting back to particular point with less time along with user data being reverted.

In case use doesn't use snapshot, if something went wrong with VM user will require to destroy VM, request for new VM deployment, this will also cause user data loss at particular point to which user can revert using snapshot.

I understand with Terraform infrastructure as a service support, snapshot resources might not help during complete infrastructure creation but they will definitely help for maintaining infrastructure. Please let me know if this helps in understanding use cases I am trying to achieve.

idmsubs · 2017-09-04T06:26:28Z

@vancluever could you please help me in completing review process, so I can merge changes.

Thanks

vancluever · 2017-09-04T16:51:37Z

Hey @idmsubs, sorry for being silent for the last week, but I have been busy with trying to roll out some new resources and wanted to make sure I had the time to give this a thoughtful enough response.

I still personally have some big reservations about this resource. I understand the workflow that you have laid out above, but personally don't necessarily think it agrees with the philosophy of the Terraform workflow, as you have mentioned.

Rather than creating snapshots that, in my opinion, can be fragile over the course of a lifetime of a VM (especially when it could pan out to a long snapshot chain like you have laid out above), I would rather the engineer lean on configuration management for the scenario that you have laid out above, or backups, or a combination of both.

The ideal scenario would be using Packer to build a base virtual machine, which can abstract the complex configuration that you have mentioned above to the point where it can be generalized and turned into a template. Then, using Terraform, the VM could be either fully cloned, creating an independent copy, or a linked clone could be made, creating an "instance" off of the base image (more akin to what you would see in a modern cloud).

From there, backups (not snapshots) should be used to back up the machine, and regular restore tests should happen to ensure that the application can be restored to a reasonably recent working state.

Nonetheless, I want to recognize your hard work here and also the fact that not everyone Terraforms exactly the same - so I want to merge this, with the stipulation that the vsphere_virtual_machine_snapshot_revert resource be removed, as I really can't justfiy having this kind of resource in the provider. Instead, to support reverting to an existing snapshot, someone should be taking the ID of the snapshot produced by TF (as an output for example) and using an OOB process to revert it (ie: using govc vm.snapshot.revert). If you can do that, we can move towards merging this.

I have also left more review comments inline in the current diff.

vancluever

Hey @idmsubs, inline comments now done - if you can review and apply the necessary changes that would be great!

vancluever · 2017-09-04T16:52:11Z

vsphere/README.md

@@ -0,0 +1,62 @@
+# Terraform vSphere Provider Dev Docs


This file needs to be removed completely as it has been removed in master.

vancluever · 2017-09-04T16:52:48Z

vsphere/provider.go

+			"vsphere_virtual_disk":                    resourceVSphereVirtualDisk(),
+			"vsphere_virtual_machine":                 resourceVSphereVirtualMachine(),
+			"vsphere_virtual_machine_snapshot":        resourceVSphereSnapshot(),
+			"vsphere_virtual_machine_snapshot_revert": resourceVSphereRevertSnapshot(),


Just a note here to remove the vsphere_virtual_machine_snapshot_revert resource.

vancluever · 2017-09-04T16:53:36Z

vsphere/resource_vsphere_snapshot.go

@@ -0,0 +1,186 @@
+package vsphere


Can you change the file name here to resource_vsphere_virtual_machine_snapshot.go?

vancluever · 2017-09-04T16:53:47Z

vsphere/resource_vsphere_snapshot_test.go

@@ -0,0 +1,122 @@
+package vsphere


Can you change the file name here to resource_vsphere_virtual_machine_snapshot_test.go?

vancluever · 2017-09-04T17:13:22Z

vsphere/resource_vsphere_snapshot.go

+		Delete: resourceVSphereSnapshotDelete,
+
+		Schema: map[string]*schema.Schema{
+			"vm_id": {


Can you change this to reference the the UUID of the virtual machine versus the VM path name?

vsphere_virtual_machine supplies this via the uuid attribute, so it can be fetched in Terraform. From there, you can get the MoRef for the VM via the FindAllByUuid method.

vancluever · 2017-09-04T17:34:07Z

vsphere/resource_vsphere_snapshot.go

+	return nil
+}
+
+func findVM(d *schema.ResourceData, meta interface{}) (*object.VirtualMachine, error) {


Once this is changed to find by UUID, can you move this function to a new file called virtual_machine_helper.go? I want to try and start ensuring that all helpers are organized by their inparticular MO.

Also, make sure the function name is self-documenting - in this case, it would be virtualMachineFromUUID.

vancluever · 2017-09-04T17:34:59Z

website/docs/r/virtual_machine_snapshot.html.markdown

+## Example Usage
+
+```hcl
+resource "vsphere_virtual_machine_snapshot" "demo1"{


Can you filter this through terraform fmt so that it is correctly formatted?

vancluever · 2017-09-04T17:35:24Z

vsphere/resource_vsphere_snapshot_test.go

+
+func testAccCheckVSphereVMSnapshotConfig_basic(vmId, snapshotName, description, memory, quiesce string) string {
+	return fmt.Sprintf(`
+	resource "vsphere_virtual_machine_snapshot" "Test_terraform_cases" {


Can you filter this through terraform fmt so that it is correctly formatted?

vancluever · 2017-09-04T17:39:36Z

website/docs/index.html.markdown

@@ -1,209 +1,229 @@
---


Can you remove this file from the PR?

I will probably be revising the provider index doc soon, and at that point it will contain a more terse example, something more akin to the AWS provider's index page.

vancluever · 2017-09-04T17:40:32Z

website/docs/r/virtual_machine_snapshot.html.markdown

+* `quiesce` - (Required) If the quiesce flag set to true, and the virtual machine is powered on when the snapshot is taken, VMware Tools is used to quiesce the file system in the virtual machine.
+* `datacenter` - (Optional) The name of a Datacenter in which the virtual machine exists for which we are taking snapshot
+* `remove_children` - (Optional) Flag to specify removal of the entire snapshot subtree.
+* `consolidate` - (optional) If set to true, the virtual disk associated with this snapshot will be merged with other disk if possible. Defaults to true.


optional -> Optional

…Snapshot

vancluever · 2017-09-08T17:57:10Z

Hey @idmsubs, I think this LGTM now but there are some changes I will be making (while preserving your commits in place) pertaining to items that were missed from the review. I will be merging shortly.

Thank you very much for your work on this and your patience on the process!

vancluever · 2017-09-08T21:59:49Z

Hey @idmsubs, just wanted to let you know the post-merge edits can be seen in the commit log in master. One significant change is I have changed the vm_uuid parameter to virtual_machine_uuid, just to keep with our convention of using full-word parameters (for the most part).

Anyway, this should be in next week's release, so stay tuned and thanks again!

Enhancement: Added resource for snapshot and revert snapshot

f71c9ab

idmsubs mentioned this pull request Jul 27, 2017

Terraform vSphere Snapshot management resources by GSLab hashicorp/terraform#14660

Closed

grubernaut added enhancement Type: Enhancement new-resource Feature: New Resource labels Aug 1, 2017

girishramnani reviewed Aug 9, 2017

View reviewed changes

idmsubs added 3 commits August 10, 2017 15:17

Typo corrected

43deced

typo corrected

c9c8f9b

added parameters to testcase

4703a3c

vancluever suggested changes Aug 10, 2017

View reviewed changes

vancluever added the waiting-response Status: Waiting on a Response label Aug 17, 2017

Resource name changed

9477f63

idmsubs and others added 3 commits August 23, 2017 18:57

Fixed ManagedObjectReference, Documentation

b96aee1

Update provider.go

827577f

Terraform Merged with updated repo

36e2031

idmsubs added 2 commits August 24, 2017 11:29

removed space

28bbd45

go fmt fixed

b35714c

idmsubs commented Aug 24, 2017

View reviewed changes

Updated vsphere provider with latest

f746702

vancluever suggested changes Sep 4, 2017

View reviewed changes

Shubham Tholiya added 2 commits September 6, 2017 19:37

Enhancement: Snapshot can be created with vm_uuid and removed Revert …

db8598a

…Snapshot

Merged: merged changes

ae583a9

vancluever mentioned this pull request Sep 7, 2017

Needs clarification on the use of vsphere_virtual_disk #150

Closed

vancluever approved these changes Sep 8, 2017

View reviewed changes

vancluever merged commit ae583a9 into hashicorp:master Sep 8, 2017

vancluever removed the waiting-response Status: Waiting on a Response label Sep 8, 2017

vancluever mentioned this pull request Dec 22, 2017

vSphere provider wishlist #335

Closed

vancluever mentioned this pull request Sep 26, 2018

Question: Can "vsphere_virtual_machine_snapshot" resource used to revert a VM to a particular snapshot ? #626

Closed

ghost locked and limited conversation to collaborators Apr 19, 2020

		}

		func testAccCheckVmSnapshotDestroy(s *terraform.State) error {

Enhancement: Added resource for snapshot and revert snapshot #107

Enhancement: Added resource for snapshot and revert snapshot #107

Conversation

idmsubs commented Jul 21, 2017 • edited Loading

girishramnani commented Aug 1, 2017

idmsubs commented Aug 9, 2017

girishramnani commented Aug 9, 2017

idmsubs commented Aug 9, 2017 • edited Loading

girishramnani commented Aug 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vancluever left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vancluever commented Aug 17, 2017

idmsubs commented Aug 18, 2017

vancluever commented Aug 23, 2017

idmsubs commented Aug 24, 2017

idmsubs left a comment

Choose a reason for hiding this comment

vancluever commented Aug 25, 2017 • edited Loading

idmsubs commented Aug 28, 2017

idmsubs commented Sep 4, 2017 • edited Loading

vancluever commented Sep 4, 2017

vancluever left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vancluever commented Sep 8, 2017

vancluever commented Sep 8, 2017

idmsubs commented Jul 21, 2017 •

edited

Loading

idmsubs commented Aug 9, 2017 •

edited

Loading

vancluever commented Aug 25, 2017 •

edited

Loading

idmsubs commented Sep 4, 2017 •

edited

Loading