Adding volume resource creation to vSphere provider #6273

dkalleg · 2016-04-21T00:32:13Z

Allows the user to create a vmdk in vSphere to the given path. Update
not implemented, further research required. In the future this could
support updates for rename and move.

chrislovecnm · 2016-04-22T15:35:38Z

@dkalleg is this fixing an open issue? A bit brain dead this morning ;) I am thinking that this is a new feature?

dkalleg · 2016-04-22T19:46:02Z

New feature

chrislovecnm · 2016-04-24T00:29:18Z

builtin/providers/vsphere/resource_vsphere_virtual_disk.go

+		log.Printf("[DEBUG] resourceVSphereVirtualDiskRead - stat failed on: %v", vDisk.vmdkPath)
+		d.SetId("")
+		return err
+	} else {


Don't think we need this else

chrislovecnm · 2016-04-24T00:37:24Z

Appreciate the work. Looking forward to getting this in. Got a bunch of comments for yah ;)

dkalleg · 2016-04-26T23:18:28Z

@chrislovecnm @kristinn @phinze @tkak Please give this a review at your earliest convenience.

kristinn · 2016-04-27T18:42:53Z

builtin/providers/vsphere/resource_vsphere_virtual_disk_test.go

+		}
+
+		fileInfo, err := ds.Stat(context.TODO(), rs.Primary.Attributes["vmdk_path"])
+		_ = fileInfo


What's the reason for this?

This is a go-ism I wasn't sure the best way to work around, I still have much to learn :) As I started explaining myself here I realized my mistake! I'll push up the change!

Basically, I wanted to say _, err := ds.Stat(... but was getting the error of 'no new vars on left of :=', so I gave it fileInfo. But with that, fileInfo has to be used once or the go compiler will get angry, and this is essentially a noop to work around that. All this goes away if i take out the colon :) Doh!

@stack72 @phinze @jen20 any opinion on this? Your GoFu is strong...

you can do it as follows:

_, err = ds.Stat(context.TODO(), rs.Primary.Attributes["vmdk_path"])

have already declared err above AFAICT. You were trying to redeclare the same variable again

Sorry, didn't mean to pose that as a question. Thats the answer that came to me after reading kristinn's question :)

Updating the PR shortly

same as above

@dagnello thanks for the help!

kristinn · 2016-04-27T18:44:32Z

This is a very good PR 👍 Just missing some function signatures, that's all.

dagnello · 2016-04-27T22:36:36Z

builtin/providers/vsphere/resource_vsphere_virtual_disk_test.go

+		}
+
+		fileInfo, err := ds.Stat(context.TODO(), rs.Primary.Attributes["vmdk_path"])
+		_ = fileInfo


if you don't need fileInfo, you can replace it with:
_, err = ds.Stat(context.TODO(), vDisk.vmdkPath)

dagnello · 2016-04-28T18:56:10Z

website/source/docs/providers/vsphere/r/virtual_disk.html.markdown

+* `size` - (Required) Size of the disk (in GB).
+* `vmdk_path` - (Required) The path, including filename, of the virtual disk to be created.  This should end with '.vmdk'.
+* `type` - (Optional) 'eagerZeroedThick' (the default), or 'thin' are supported options.
+* `datacenter` - (Required) The name of a Datacenter in which to create the disk.


I think this should be optional, similar to folder or virtual machine resources

dkalleg · 2016-04-28T21:33:39Z

@kristinn Could you point out which function signature you want to see updated? It isn't obvious for me. Thx.

dagnello · 2016-04-28T23:01:22Z

builtin/providers/vsphere/resource_vsphere_virtual_disk.go

+			"size": &schema.Schema{
+				Type:     schema.TypeInt,
+				Required: true,
+				ForceNew: true, //TODO Can this be optional (resize)?


it looks like these can be false if an upgrade supports the change

That would be changed in a PR that also adds vm Update functionality. For now it needs to stay this way, else TF would go looking for a resourceVSphereVirtualMachineUpdate that isn't there.

dkalleg · 2016-05-02T19:33:54Z

@chrislovecnm @phinze @tkak Any more feedback?

chrislovecnm · 2016-05-02T19:59:47Z

LGTM!

stack72 · 2016-05-02T20:25:06Z

The main point I have here is that we are not setting the state of any values on the read. Its a normal practice to use the d.Set("param", value) notation

dkalleg · 2016-05-02T20:34:11Z

@stack72 Ah yes, let me add that and I'll push an update to this PR hopefully sometime today or tomorrow. Since we aren't getting any specific vmdk info* on the read, I won't be able to fetch every value. But should be able to do a d.Set for size, path, datacenter?, datastore. But not type and adapter type.

*I didn't see any way in govmomi apis to drill down and get vmdk-level data on a file, like init_type and adapter_type. If anyone knows how to do this I'd like to be able to add that functionality! Maybe it required changes to govmomi.

chrislovecnm · 2016-05-02T20:35:10Z

builtin/providers/vsphere/resource_vsphere_virtual_disk.go

+	}
+
+	if v, ok := d.GetOk("vmdk_path"); ok {
+		vDisk.vmdkPath = v.(string)


@stack72 you would expect the set usage here? More info please.

@chrislovecnm the setting of the state usually happens I'm the read func so we are sure that we expected to happen has happened.

chrislovecnm · 2016-05-02T20:40:13Z

@stack72 you mind a concrete example? What line in the read func would you like to be modified. I think I understand, but I don't want to assume 😄

dkalleg · 2016-05-03T20:30:47Z

@stack72 I believe I've addressed your comment. As of right now, I don't know how to query vSphere vdisks for more info other than simply stat'ing them (even the stat call doesn't seem to be returning the file size properly), so I couldn't add writes for things like size, type, ect. But I've added a todo there to mark further investigation on the govmomi side.

Keep those reviews coming :)

chrislovecnm · 2016-05-03T21:01:13Z

@dkalleg should we open a ticket with govmomi peeps asking the question about vdisk attributes?

dkalleg · 2016-05-03T21:05:13Z

That's probably a good idea. Even if there is some wacky way to accomplish it today, there isn't support in the object/datastore api, which I would think should have it.

chrislovecnm · 2016-05-03T21:08:52Z

@dkalleg make it so .. and please cc: @markpeek - he works in the same group as the govmomi guys

dougm · 2016-05-04T00:16:15Z

Datastore.Stat returns an interface type (types.BaseFileInfo), in the case of .vmdk files will be *types.VmDiskFileInfo with the fields it sounds like you're looking for:

res, err := Datastore.Stat(ctx, vmdkPath)
if err != nil {...}
info, ok := res.(*types.VmDiskFileInfo)
if !ok { panic("unexpected type") }
// make use of info.{DiskType,CapacityKb,etc}

chrislovecnm · 2016-05-04T00:42:55Z

@dougm much thanks

dkalleg · 2016-05-09T23:37:11Z

@dougm Thanks for the tip. When I store the result of the assertion / type-cast (I recently learned the proper term for it is a assertion, not a type-cast.. another weird go-ism) and print out its values, they're mostly default / zero'd values.

    fileInfo, err := ds.Stat(context.TODO(), vDisk.vmdkPath)
    if err != nil {
        log.Printf("[DEBUG] resourceVSphereVirtualDiskRead - stat failed on: %v", vDisk.vmdkPath)
        d.SetId("")
        return err
    }
    log.Printf("[DEBUG] resourceVSphereVirtualDiskRead - fileinfo: %#v", fileInfo.GetFileInfo())

    diskInfo := fileInfo.(*types.VmDiskFileInfo)
    log.Printf("[DEBUG] resourceVSphereVirtualDiskRead - disk info: %#v", diskInfo)

2016/05/09 16:26:45 [DEBUG] terraform-provider-vsphere: 2016/05/09 16:26:45 [DEBUG] resourceVSphereVirtualDiskRead - fileinfo: &types.FileInfo{DynamicData:types.DynamicData{}, Path:"newVmdk.vmdk", FileSize:0, Modification:(*time.Time)(nil), Owner:"root"}
2016/05/09 16:26:45 [DEBUG] terraform-provider-vsphere: 2016/05/09 16:26:45 [DEBUG] resourceVSphereVirtualDiskRead - disk info: &types.VmDiskFileInfo{FileInfo:types.FileInfo{DynamicData:types.DynamicData{}, Path:"newVmdk.vmdk", FileSize:0, Modification:(*time.Time)(nil), Owner:"root"}, DiskType:"", CapacityKb:0, HardwareVersion:0, ControllerType:"", DiskExtents:[]string(nil), Thin:(*bool)(nil)}

Focusing on just one attribute, I can confirm the disk is 2GB, but the returned info suggests its 0kb. Any suggestions or ideas on why Stat is giving me this zero'd data?

dougm · 2016-05-10T00:29:31Z

@dkalleg this PR will fix that: vmware/govmomi#509

Should get that merged later or tomorrow.

dkalleg · 2016-05-10T00:44:51Z

@dougm Thanks so much for that lightning fast turn around! Does this mean I will continue to expect the remaining fields to not update? ex. ControllerType, Thin, CapacityKb, HardwareVersion, ect

type VmDiskFileInfo struct {
    FileInfo

    DiskType        string   `xml:"diskType,omitempty"`
    CapacityKb      int64    `xml:"capacityKb,omitempty"`
    HardwareVersion int32    `xml:"hardwareVersion,omitempty"`
    ControllerType  string   `xml:"controllerType,omitempty"`
    DiskExtents     []string `xml:"diskExtents,omitempty"`
    Thin            *bool    `xml:"thin"`
}

@chrislovecnm @stack72 What this means now, is:
If we want this PR to include data on size/initType that the PR trying to update our govmomi version needs to update far enough to include Doug's fix.
or
Merge what we have with the todo of updating it once govmomi v0.6.0+ lands.

dkalleg · 2016-05-11T00:25:34Z

@dougm Unrelated question.. Can you explain about types.DynamicData? I see the VirtualDevice struct extends DynamicData, which is an empty struct. What is its purpose? Is it a way for govmomi users to add in dynamic data to be associated with a VirtualDevice in vSphere? Or is it a way for vSphere to send non-api defined dynamic data back to the govmomi user? Or something else? Feel free to email me at dkalleg at vt dot edu

chrislovecnm · 2016-05-11T13:04:59Z

@dkalleg you mind putting in a issue with govmomi? ... Just for tracking.

Thanks

Chris

dkalleg · 2016-05-12T18:51:54Z

@chrislovecnm @stack72 Any further comments on this PR?

chrislovecnm · 2016-05-13T18:58:46Z

@dkalleg which version of govmomi do you need for this??

dkalleg · 2016-05-13T19:16:46Z

This was built on top of the old 0.3.0. I'll need to rebase this. Working on vm resource disk update story today so I might not get to this til next week.

chrislovecnm · 2016-05-13T19:41:38Z

@dkalleg can you mark this [WIP]. Appreciate the work! We probably going to bump the version again.

Allows the user to create a vmdk in vSphere to the given path. In the future this could support updates for rename and move.

dkalleg · 2016-05-18T22:23:35Z

@chrislovecnm @stack72 I've updated to include the new size info via our govmomi upgrade with Doug's updated Stat() info. Please review at your earliest convenience.

chrislovecnm · 2016-05-23T19:38:36Z

@dkalleg is this good to go?

dkalleg · 2016-05-23T20:01:15Z

@chrislovecnm I believe so

chrislovecnm · 2016-05-23T20:43:55Z

@catsby @jen20 @phinze @stack72 can we get a merge!

chrislovecnm · 2016-05-25T00:25:06Z

@jen20 @stack72 @phinze @catsby how do we get this merged? Help is appreciated!!

stack72 · 2016-05-25T10:21:15Z

LGTM now :) Thanks for the extra additions as requested

P.

dkalleg · 2016-05-25T17:14:47Z

Thanks @stack72 !

ghost · 2020-04-25T02:22:09Z

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.

stack72 added enhancement provider/vsphere labels Apr 21, 2016

chrislovecnm reviewed Apr 24, 2016
View reviewed changes

dkalleg mentioned this pull request Apr 25, 2016

vSphere Provider: Mapping out the Next Steps #3624

Closed

dkalleg force-pushed the create_virtual_disk branch 2 times, most recently from bbe6220 to 10c7b39 Compare April 26, 2016 23:08

kristinn reviewed Apr 27, 2016
View reviewed changes

dkalleg force-pushed the create_virtual_disk branch from 10c7b39 to b81578a Compare April 27, 2016 20:53

dagnello reviewed Apr 27, 2016
View reviewed changes

dkalleg force-pushed the create_virtual_disk branch from b81578a to 9eee372 Compare April 27, 2016 22:57

dagnello reviewed Apr 28, 2016
View reviewed changes

dkalleg force-pushed the create_virtual_disk branch from 9eee372 to 5a6a2f4 Compare April 28, 2016 19:18

dagnello reviewed Apr 28, 2016
View reviewed changes

chrislovecnm reviewed May 2, 2016
View reviewed changes

dkalleg force-pushed the create_virtual_disk branch from 5a6a2f4 to fb7e6f0 Compare May 3, 2016 18:12

dkalleg changed the title ~~Adding volume resource creation to vSphere provider~~ [WIP] - Adding volume resource creation to vSphere provider May 13, 2016

Adding volume resource creation to vSphere provider

867b6a8

Allows the user to create a vmdk in vSphere to the given path. In the future this could support updates for rename and move.

dkalleg force-pushed the create_virtual_disk branch from fb7e6f0 to 867b6a8 Compare May 18, 2016 22:20

dkalleg changed the title ~~[WIP] - Adding volume resource creation to vSphere provider~~ Adding volume resource creation to vSphere provider May 18, 2016

stack72 merged commit a47ad10 into hashicorp:master May 25, 2016

ghost locked and limited conversation to collaborators Apr 25, 2020

Adding volume resource creation to vSphere provider #6273

Adding volume resource creation to vSphere provider #6273

Conversation

dkalleg commented Apr 21, 2016

chrislovecnm commented Apr 22, 2016

dkalleg commented Apr 22, 2016

Choose a reason for hiding this comment

chrislovecnm commented Apr 24, 2016

dkalleg commented Apr 26, 2016

Choose a reason for hiding this comment

dkalleg Apr 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stack72 Apr 27, 2016 • edited Loading

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

kristinn commented Apr 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkalleg commented Apr 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkalleg commented May 2, 2016

chrislovecnm commented May 2, 2016

stack72 commented May 2, 2016

dkalleg commented May 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrislovecnm commented May 2, 2016

dkalleg commented May 3, 2016

chrislovecnm commented May 3, 2016

dkalleg commented May 3, 2016

chrislovecnm commented May 3, 2016

dougm commented May 4, 2016

chrislovecnm commented May 4, 2016

dkalleg commented May 9, 2016

dougm commented May 10, 2016

dkalleg commented May 10, 2016

dkalleg commented May 11, 2016

chrislovecnm commented May 11, 2016

dkalleg commented May 12, 2016

chrislovecnm commented May 13, 2016

dkalleg commented May 13, 2016

chrislovecnm commented May 13, 2016

dkalleg commented May 18, 2016

chrislovecnm commented May 23, 2016

dkalleg commented May 23, 2016

chrislovecnm commented May 23, 2016

chrislovecnm commented May 25, 2016

stack72 commented May 25, 2016

dkalleg commented May 25, 2016

ghost commented Apr 25, 2020

dkalleg Apr 27, 2016 •

edited

Loading

stack72 Apr 27, 2016 •

edited

Loading