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.

Sign up for GitHub

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

Jump to bottom

vic-machine validator bail out if session populate doesn't find a vm folder #7491

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

vic-machine validator bail out if session populate doesn't find a vm folder #7491

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5a2b9f4
vic-machine validator bail out if session doesn't find a vm folder
AngieCris Mar 9, 2018
9124874
Merge branch 'master' of github.com:vmware/vic into vic/7016
AngieCris Mar 9, 2018
95cfa72
Merge branch 'master' of github.com:vmware/vic into vic/7016
AngieCris Mar 13, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions lib/install/validate/validator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,15 @@ func NewValidator(ctx context.Context, input *data.Data) (*Validator, error) {
op.Debugf("new validator Session.Populate: %s", err)
}

if v.Session.VMFolder == nil {
Copy link
Member

Choose a reason for hiding this comment

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

I think this may cause issues when doing a vic-machine ls with multiple datacenters.

I suggest delaying any further work on this until @matthewavery has delivered the inventory folder support (#773) and then revisting it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes sense. I will then leave the PR as it is for now, and move the ticket back to To-Do. Thanks!

op.Debugf("Failed to set validator session VM folder")
// it's possible that VMFolder is not set, but session.Populate doesn't return any error
if err == nil {
err = errors.New("validator Session.Populate: no datacenter folder (nil) is found")
}
return nil, err
}

if strings.Contains(sessionconfig.DatacenterPath, "/") {
detail := "--target should only specify datacenter in the path (e.g. https://addr/datacenter) - specify cluster, resource pool, or folder with --compute-resource"
op.Error(detail)
Expand Down
7 changes: 7 additions & 0 deletions pkg/vsphere/session/session.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -359,6 +359,13 @@ func (s *Session) Populate(ctx context.Context) (*Session, error) {
} else {
op.Debugf("Cached folders: %s", s.DatacenterPath)
}
// There could be cases where no error from Datacenter.Folders, but nil folder is returned. In this case we should bail out.
// It's also possible that there's an error, but a valid vm folder is returned
if folders == nil {
errs = append(errs, fmt.Sprintf("Nil folder returned when finding folders (%s)", s.DatacenterPath))
return nil, errors.New(strings.Join(errs, "\n"))
Copy link
Member

Choose a reason for hiding this comment

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

Can we just omit this here, and rely on the if len(errs) > 0 { block below? (Which would require putting the s.VMFolder = folders.VmFolder line in an else block, of course.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

under some circumstances (tho very rare), s.Datacenter.Folders(op) returns no error (err == nil) but the folders return is nil. (for example, some reference handling race condition in vSphere or sth)
this is included in the comment a little above it

Also errs include errors from other stuff, checking for a specific one requires iterating and it doesn't feel very neat

Copy link
Member

@zjs zjs Mar 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think I'm suggesting anything that'd require iterating.

Right now, you have:

	if s.Datacenter != nil {
		folders, err := s.Datacenter.Folders(op)
		if err != nil {
			errs = append(errs, fmt.Sprintf("Failure finding folders (%s): %s", s.DatacenterPath, err.Error()))
		} else {
			op.Debugf("Cached folders: %s", s.DatacenterPath)
		}
		// There could be cases where no error from Datacenter.Folders, but nil folder is returned. In this case we should bail out.
		// It's also possible that there's an error, but a valid vm folder is returned
		if folders == nil {
			errs = append(errs, fmt.Sprintf("Nil folder returned when finding folders (%s)", s.DatacenterPath))
			return nil, errors.New(strings.Join(errs, "\n"))
		}

		s.VMFolder = folders.VmFolder
	}

	if len(errs) > 0 {
		op.Debugf("Error count populating vSphere cache: (%d)", len(errs))
		return nil, errors.New(strings.Join(errs, "\n"))
	}
	op.Debug("vSphere resource cache populated...")
	return s, nil

I think you could just remove the return you added without changing the behavior:

	if s.Datacenter != nil {
		folders, err := s.Datacenter.Folders(op)
		if err != nil {
			errs = append(errs, fmt.Sprintf("Failure finding folders (%s): %s", s.DatacenterPath, err.Error()))
		} else {
			op.Debugf("Cached folders: %s", s.DatacenterPath)
		}
		// There could be cases where no error from Datacenter.Folders, but nil folder is returned. In this case we should bail out.
		// It's also possible that there's an error, but a valid vm folder is returned
		if folders == nil {
			errs = append(errs, fmt.Sprintf("Nil folder returned when finding folders (%s)", s.DatacenterPath))
-			return nil, errors.New(strings.Join(errs, "\n"))
-		}
-		
+		} else {
			s.VMFolder = folders.VmFolder
+		}
	}

	if len(errs) > 0 {
		op.Debugf("Error count populating vSphere cache: (%d)", len(errs))
		return nil, errors.New(strings.Join(errs, "\n"))
	}
	op.Debug("vSphere resource cache populated...")
	return s, nil

The behavior should still be the same; since you're appending to errs in the folder == nil block, len(errs) will be > 0, and you'll return the same thing at the end:

	if s.Datacenter != nil {
		folders, err := s.Datacenter.Folders(op)
		if err != nil {
			errs = append(errs, fmt.Sprintf("Failure finding folders (%s): %s", s.DatacenterPath, err.Error()))
		} else {
			op.Debugf("Cached folders: %s", s.DatacenterPath)
		}
		// There could be cases where no error from Datacenter.Folders, but nil folder is returned. In this case we should bail out.
		// It's also possible that there's an error, but a valid vm folder is returned
		if folders == nil {
			errs = append(errs, fmt.Sprintf("Nil folder returned when finding folders (%s)", s.DatacenterPath))
		} else {
			s.VMFolder = folders.VmFolder
		}
	}

	if len(errs) > 0 {
		op.Debugf("Error count populating vSphere cache: (%d)", len(errs))
		return nil, errors.New(strings.Join(errs, "\n"))
	}
	op.Debug("vSphere resource cache populated...")
	return s, nil

This essentially just preserves the pattern the code was already using: accumulate a collection of errors and then return all of them. This is important because if someone adds another check after the if s.Datacenter != nil block, you'd want to include any errors from that too.

Copy link
Contributor Author

@AngieCris AngieCris Mar 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I think what I wanted was to make the session.Populate early abort when folders == nil, without doing s.VMFolder = folders.VmFolder step that dumps core when folders == nil.

Preserving the error accumulation pattern makes perfect sense. To make it cleaner, I think I could do sth like this:

		if folders == nil {
			errs = append(errs, fmt.Sprintf("Nil folder returned when finding folders (%s)", s.DatacenterPath))
		} else {
                      s.VMFolder = folders.VmFolder 
                }

And then it's the validator's job to check session.VMFolder and quit if it's empty.

}

s.VMFolder = folders.VmFolder
}

Expand Down