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

[AngieCris]

[specific ci=Group6-VIC-Machine]

Fixes #7016
(not necessarily fixes the root cause - just an improvement on a more reasonable behavior rather than core dump)

During validator session.Populate, under some extreme circumstances session.Datacenter.Folders(op) could return nil folder. In this case, we should terminate session populate, make validator return an error and quit vic-machine process.

[zjs]

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.)

[AngieCris]

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

[zjs]

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.

[AngieCris]

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.

[hickeng]

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.

[AngieCris]

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

[AngieCris]

Status: further work on this PR is delayed until changes on VCH inventory folder support (#773) have delivered.
Leave the PR as it is for now.