Attach and list RDM/LUN #656

bastienbc · 2017-01-17T08:25:08Z

govc does not provide command line options to:

attach RDM using the device name.
list available devices for a specific VM, retrieving various useful information ( UIDS, Device name, Device path, ... )

vmwclabot · 2017-01-17T08:25:10Z

@bastienbc, you must sign our contributor license agreement before your changes are merged. Click here to sign the agreement. If you are a VMware employee, read this for further instruction.

vmwclabot · 2017-01-17T09:15:40Z

@bastienbc, your company's legal contact has rejected your signed contributor license agreement. The merge can not proceed until the agreement has been resigned. Click here to resign the agreement.

… request).

vmwclabot · 2017-01-17T09:24:27Z

@bastienbc, your company's legal contact has approved your signed contributor license agreement. It will also be reviewed by VMware, but the merge can proceed.

vmwclabot · 2017-01-19T21:48:42Z

@bastienbc, VMware has approved your signed contributor license agreement.

dougm

@bastienbc thanks, looks very useful. A few comments for you to consider.

dougm · 2017-01-24T21:06:02Z

govc/vm/rdm/attach.go

+	}
+	return nil
+}
+


Can you add a Description here (example from vm.clone below). This'll get included in the help output and the generated govc/USAGE.md doc.

func (cmd *clone) Description() string { return `Clone VM to NAME. Examples: govc vm.clone -vm template-vm new-vm` }

dougm · 2017-01-24T21:08:51Z

govc/vm/rdm/attach.go

+	if err != nil {
+		return err
+	}
+	var VM_withProp mo.VirtualMachine


Can rename to avoid the go-lint warning below, just 'mvm' for example would be fine.

don't use underscores in Go names; var VM_withProp should be VMWithProp

dougm · 2017-01-24T21:09:06Z

govc/vm/rdm/attach.go

+
+		err = task.Wait(ctx)
+		if err != nil {
+			return errors.New(fmt.Sprintf("Error adding device %+v \n with backing %+v \nLogged Item:  %s", device, backing, err))


from go-lint:

should replace errors.New(fmt.Sprintf(...)) with fmt.Errorf(...)

dougm · 2017-01-24T21:09:22Z

govc/vm/rdm/attach.go

+		return nil
+
+	}
+	return errors.New(fmt.Sprintf("Error: No LUN with device name containing %s found.", cmd.device))


from go-lint:

should replace errors.New(fmt.Sprintf(...)) with fmt.Errorf(...)

dougm · 2017-01-24T21:12:09Z

govc/vm/rdm/attach.go

+	//Query VM To Find Devices avilable for attaching to VM
+	var queryConfigRequest types.QueryConfigTarget
+	queryConfigRequest.This = VM_withProp.EnvironmentBrowser
+	cl, err := cmd.Client()


check err != nil here.

dougm · 2017-01-24T21:15:56Z

govc/vm/rdm/attach.go

+		for u = 0; u < 16; u++ {
+			free := true
+			for _, device := range devices {
+				if device.GetVirtualDevice().ControllerKey == device.GetVirtualDevice().ControllerKey {


Looks like you need to rename 'device' in the loop (to 'd' for example), so you can compare to the device defined on line 105 above.

dougm · 2017-01-24T21:22:30Z

govc/vm/rdm/attach.go

+		backing.CompatibilityMode = "physicalMode"
+		backing.DeviceName = ScsiDisk.Disk.DeviceName
+		for _, descriptor := range ScsiDisk.Disk.Descriptor {
+			if string([]rune(descriptor.Id)[:4]) == "vml." {


strings.HasPrefix(descriptor.Id, "vml.")

dougm · 2017-01-24T21:28:06Z

govc/vm/rdm/ls.go

+	var queryConfigRequest types.QueryConfigTarget
+	queryConfigRequest.This = VM_withProp.EnvironmentBrowser
+	cl, err := cmd.Client()
+	queryConfigResp, err := methods.QueryConfigTarget(ctx, cl, &queryConfigRequest)


Maybe wrap this in a helper, since its being used in 2 places:

func (v VirtualMachine) QueryConfigTarget(ctx context.Context) (*types.ConfigTarget, error) { var vm mo.VirtualMachine err := v.Properties(ctx, v.Reference(), []string{"environmentBrowser"}, &vm) if err != nil { return nil, err } req := types.QueryConfigTarget{ This: vm.EnvironmentBrowser, } res, err := methods.QueryConfigTarget(ctx, v.Client(), &req) if err != nil { return nil, err } return res.Returnval, nil }

dougm · 2017-01-24T21:30:32Z

govc/vm/rdm/ls.go

+	tw := tabwriter.NewWriter(os.Stdout, 2, 0, 2, ' ', 0)
+	for _, disk := range r.Disks {
+		fmt.Fprintf(tw, "Name:\t%s\n", disk.Name)
+		fmt.Fprintf(tw, "	Device name:\t%s\n", disk.Disk.DeviceName)


2 space indent like the other commands, rather than tab.

bastienbc · 2017-01-25T10:23:42Z

Hi!
First thank you to have a look into my code.
I am relatively new to Go, so I am keeping any of your advises preciously.
Also I did not knew about golint, which is now installed and used :-)

About this pull request itself:
To 2014-2017?
Something else?
You mentioned it on an other pull request comment, so I was wondering what to do exactly.

…alMachine method. Also took in consideration comments made by dougm to merge this to the vmware branch.

…are branch. Added Description method in both vm.rdm.attach and vm.rdm.ls.

dougm

Looking good, just a few more minor suggestions.

dougm · 2017-01-27T19:05:05Z

govc/vm/rdm/attach.go

+	cmd.VirtualMachineFlag, ctx = flags.NewVirtualMachineFlag(ctx)
+	cmd.VirtualMachineFlag.Register(ctx, f)
+
+	f.StringVar(&cmd.device, "deviceName", "", "Device Name")


Let's name the flag "device", we use that name in other commands.

dougm · 2017-01-27T19:07:00Z

govc/vm/rdm/attach.go

+	return `Attach DEVICE to VM with RDM.
+
+Examples:
+  govc vm.rdm.attach -vm VM -deviceName DEVICE`


s/deviceName/device/ here too. Can you use a real example value for DEVICE? An example from the output of device.ls, such as "disk-200-0"

dougm · 2017-01-27T19:07:52Z

govc/vm/rdm/attach.go

+		return err
+	}
+
+	for _, ScsiDisk := range vmConfigOptions.ScsiDisk {


Go style nit: unexported vars start with lower case, 'scsiDisk' or just 'disk' here would be fine.

dougm · 2017-01-27T19:09:05Z

govc/vm/rdm/attach.go

+			continue
+		}
+		var backing types.VirtualDiskRawDiskMappingVer1BackingInfo
+		backing.CompatibilityMode = "physicalMode"


Could use the generated enum here: string(types.VirtualDiskCompatibilityModePhysicalMode)

dougm · 2017-01-27T19:10:25Z

govc/vm/rdm/attach.go

+		for u = 0; u < 16; u++ {
+			free := true
+			for _, d := range devices {
+				if d.GetVirtualDevice().ControllerKey == d.GetVirtualDevice().ControllerKey {


Still comparing the same device ('d'), shouldn't this compare the ControllerKey of 'd' and 'device' instead ?

dougm · 2017-01-27T19:11:43Z

object/virtual_machine.go

@@ -689,3 +689,24 @@ func (v VirtualMachine) Unregister(ctx context.Context) error {
 	_, err := methods.UnregisterVM(ctx, v.Client(), &req)
 	return err
 }
+
+// QueryEnvironmentBrowser is a helper to get the environmentBrowser property.
+func (v VirtualMachine) QueryEnvironmentBrowser(ctx context.Context) (*types.ConfigTarget, error) {


Let's name this method 'QueryConfigTarget', since 'EnvironmentBrowser' is just a parameter.

dougm · 2017-01-27T19:13:28Z

…o 2017, and resolve some bug

dougm · 2017-01-30T17:06:34Z

thanks @bastienbc !

bastienbc changed the title ~~Rdmattachement~~ Attach and list RDM/LUN Jan 17, 2017

bastienbc mentioned this pull request Jan 17, 2017

how to Creating/Adding a Raw Device Mapping (RDM) to a Virtual Machine by govc #453

Closed

Adding files to attach and list RDM

2d01962

vmwclabot added the cla-rejected label Jan 17, 2017

Change github path in the main.go to use the vmware one (for the pull…

4f20858

… request).

vmwclabot removed the cla-rejected label Jan 19, 2017

dougm requested changes Jan 24, 2017

View reviewed changes

Bastien Barbé Creuly added 2 commits January 25, 2017 12:28

Refactor code so that the query of environment browser is now a Virtu…

9a289a8

…alMachine method. Also took in consideration comments made by dougm to merge this to the vmware branch.

Took in consideration comments made by dougm to merge this to the vmw…

bbd2db6

…are branch. Added Description method in both vm.rdm.attach and vm.rdm.ls.

dougm requested changes Jan 27, 2017

View reviewed changes

Change QueryEnvironmentBrowser to QueryConfigTarget, copyright year t…

f5155ba

…o 2017, and resolve some bug

dougm merged commit 3e95cb1 into vmware:master Jan 30, 2017

bizarrekaze mentioned this pull request Sep 24, 2021

List alll RDM devices #2594

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Attach and list RDM/LUN #656

Attach and list RDM/LUN #656

bastienbc commented Jan 17, 2017

vmwclabot commented Jan 17, 2017

vmwclabot commented Jan 17, 2017

vmwclabot commented Jan 17, 2017

vmwclabot commented Jan 19, 2017

dougm left a comment

dougm Jan 24, 2017

dougm Jan 24, 2017

dougm Jan 24, 2017

dougm Jan 24, 2017

dougm Jan 24, 2017

dougm Jan 24, 2017

dougm Jan 24, 2017

dougm Jan 24, 2017

dougm Jan 24, 2017

bastienbc commented Jan 25, 2017 •

edited

dougm left a comment

dougm Jan 27, 2017

dougm Jan 27, 2017

dougm Jan 27, 2017

dougm Jan 27, 2017

dougm Jan 27, 2017

dougm Jan 27, 2017

dougm commented Jan 27, 2017

dougm commented Jan 30, 2017

Attach and list RDM/LUN #656

Attach and list RDM/LUN #656

Conversation

bastienbc commented Jan 17, 2017

vmwclabot commented Jan 17, 2017

vmwclabot commented Jan 17, 2017

vmwclabot commented Jan 17, 2017

vmwclabot commented Jan 19, 2017

dougm 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

bastienbc commented Jan 25, 2017 • edited

dougm 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

dougm commented Jan 27, 2017

dougm commented Jan 30, 2017

bastienbc commented Jan 25, 2017 •

edited