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

Add vsan and disk commands / helpers #672

Merged
merged 1 commit into from
Feb 27, 2017
Merged

Add vsan and disk commands / helpers #672

merged 1 commit into from
Feb 27, 2017

Conversation

dougm
Copy link
Member

@dougm dougm commented Feb 25, 2017

  • Add DatastoreFileManager API wrapper

  • Add HostVsanInternalSystem API wrappers

  • Add govc datastore.disk.create command

  • Add govc datastore.vsan.ls and datastore.vsan.rm commands

  • Use DatastoreFileManager in govc datastore.rm command

Copy link

@emlin emlin left a comment

Choose a reason for hiding this comment

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

lgtm

return `Create VMDK on DS.

Examples:
govc datastore.disk.create -size 24G disks/disk1.vmdk`
Copy link
Member

Choose a reason for hiding this comment

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

Is disks the datastore or is this on datastore by default?

Copy link
Member Author

Choose a reason for hiding this comment

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

Need to datastore.mkdir first if it doesn't already exist, I'll add that to the example.

@@ -43,6 +43,7 @@ func (cmd *rm) Register(ctx context.Context, f *flag.FlagSet) {
cmd.DatastoreFlag, ctx = flags.NewDatastoreFlag(ctx)
cmd.DatastoreFlag.Register(ctx, f)

f.BoolVar(&cmd.kind, "t", true, "Use file type to choose disk or file manager")
Copy link
Member

Choose a reason for hiding this comment

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

might find out further down, but not sure what will be non-functional if this is set to false.

Copy link
Member Author

Choose a reason for hiding this comment

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

We're using it in the datastore_file_manager.sh to test the vsan.dom commands

}

if mds.Summary.Type != "vsan" {
return flag.ErrHelp
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps include the fact that the target is not a vSAN datastore.

func (m *DatastoreFileManager) markDiskAsDeletable(ctx context.Context, path *DatastorePath) {
r, n, err := m.Datastore.Download(ctx, path.Path, &soap.DefaultDownload)
if err != nil {
return
Copy link
Member

Choose a reason for hiding this comment

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

not returning a cause here may make it hard to differentiate between errors:

  • disk in use
  • missing Host.Config.SystemManagement privilege
  • connection error
  • other

Copy link
Member Author

Choose a reason for hiding this comment

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

We discussed in person, I'll add comment/log.

defer r.Close()

if n > 2048 {
return // sanity check before reading; should be only a few hundred bytes
Copy link
Member

Choose a reason for hiding this comment

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

This is true for the moment but we are discussing using the ddb to store the layer metadata. Regardless, if we've somehow accidentally downloaded the data file instead of the descriptor this will be Gigabytes too late - we could download only the first 512 bytes and check for known keys (for example) - not really thought about edge cases with this suggestion however.

Copy link
Member Author

Choose a reason for hiding this comment

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

Regarding ddb for metadata, hopefully we won't still be using this workaround by then :)

Regarding the download, we've only read the HTTP headers at this point, 'n' is the value of the Content-Length header. Nothing reads the HTTP body until we call s.Scan() below.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed to use https://golang.org/pkg/io/#LimitedReader instead of just checking the Content-Length header.

* Add DatastoreFileManager API wrapper

* Add HostVsanInternalSystem API wrappers

* Add govc datastore.disk.create command

* Add govc datastore.vsan.dom.ls and datastore.vsan.dom.rm commands

* Use DatastoreFileManager in govc datastore.rm command
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.

None yet

5 participants