From 8b2374143d0169bc22a533c026bf23334ec91f41 Mon Sep 17 00:00:00 2001 From: Pontus Rydin Date: Mon, 19 Aug 2019 21:17:27 -0400 Subject: [PATCH] Fix finder inconsistencies in vsphere input (#6245) --- Gopkg.lock | 1 - plugins/inputs/vsphere/finder.go | 118 +++++++++++---------- plugins/inputs/vsphere/vsphere_test.go | 135 ++++++++++++++----------- 3 files changed, 140 insertions(+), 114 deletions(-) diff --git a/Gopkg.lock b/Gopkg.lock index 7ad06dccd..b884eb9b9 100644 --- a/Gopkg.lock +++ b/Gopkg.lock @@ -1177,7 +1177,6 @@ name = "github.com/vmware/govmomi" packages = [ ".", - "find", "list", "nfc", "object", diff --git a/plugins/inputs/vsphere/finder.go b/plugins/inputs/vsphere/finder.go index 228a942d9..24427b205 100644 --- a/plugins/inputs/vsphere/finder.go +++ b/plugins/inputs/vsphere/finder.go @@ -16,6 +16,8 @@ var childTypes map[string][]string var addFields map[string][]string +var containers map[string]interface{} + // Finder allows callers to find resources in vCenter given a query string. type Finder struct { client *Client @@ -29,11 +31,6 @@ type ResourceFilter struct { paths []string } -type nameAndRef struct { - name string - ref types.ManagedObjectReference -} - // FindAll returns the union of resources found given the supplied resource type and paths. func (f *Finder) FindAll(ctx context.Context, resType string, paths []string, dst interface{}) error { for _, p := range paths { @@ -87,15 +84,18 @@ func (f *Finder) descend(ctx context.Context, root types.ManagedObjectReference, var content []types.ObjectContent fields := []string{"name"} + recurse := tokens[pos]["name"] == "**" + + types := ct if isLeaf { - // Special case: The last token is a recursive wildcard, so we can grab everything - // recursively in a single call. - if tokens[pos]["name"] == "**" { + if af, ok := addFields[resType]; ok { + fields = append(fields, af...) + } + if recurse { + // Special case: The last token is a recursive wildcard, so we can grab everything + // recursively in a single call. v2, err := m.CreateContainerView(ctx, root, []string{resType}, true) defer v2.Destroy(ctx) - if af, ok := addFields[resType]; ok { - fields = append(fields, af...) - } err = v2.Retrieve(ctx, []string{resType}, fields, &content) if err != nil { return err @@ -105,23 +105,16 @@ func (f *Finder) descend(ctx context.Context, root types.ManagedObjectReference, } return nil } - - if af, ok := addFields[resType]; ok { - fields = append(fields, af...) - } - err = v.Retrieve(ctx, []string{resType}, fields, &content) - if err != nil { - return err - } - } else { - err = v.Retrieve(ctx, ct, fields, &content) - if err != nil { - return err - } + types = []string{resType} // Only load wanted object type at leaf level + } + err = v.Retrieve(ctx, types, fields, &content) + if err != nil { + return err } + rerunAsLeaf := false for _, c := range content { - if !tokens[pos].MatchPropertyList(c.PropSet[:1]) { + if !matchName(tokens[pos], c.PropSet) { continue } @@ -137,44 +130,45 @@ func (f *Finder) descend(ctx context.Context, root types.ManagedObjectReference, } // Deal with recursive wildcards (**) - inc := 1 // Normally we advance one token. - if tokens[pos]["name"] == "**" { - if isLeaf { - inc = 0 // Can't advance past last token, so keep descending the tree - } else { - // Lookahead to next token. If it matches this child, we are out of - // the recursive wildcard handling and we can advance TWO tokens ahead, since - // the token that ended the recursive wildcard mode is now consumed. - if tokens[pos+1].MatchPropertyList(c.PropSet) { - if pos < len(tokens)-2 { + var inc int + if recurse { + inc = 0 // By default, we stay on this token + if !isLeaf { + // Lookahead to next token. + if matchName(tokens[pos+1], c.PropSet) { + // Are we looking ahead at a leaf node that has the wanted type? + // Rerun the entire level as a leaf. This is needed since all properties aren't loaded + // when we're processing non-leaf nodes. + if pos == len(tokens)-2 { + if c.Obj.Type == resType { + rerunAsLeaf = true + continue + } + } else if _, ok := containers[c.Obj.Type]; ok { + // Tokens match and we're looking ahead at a container type that's not a leaf + // Consume this token and the next. inc = 2 - } else { - // We found match and it's at a leaf! Grab it! - objs[c.Obj.String()] = c - continue } - } else { - // We didn't break out of recursicve wildcard mode yet, so stay on this token. - inc = 0 - } } + } else { + // The normal case: Advance to next token before descending + inc = 1 } err := f.descend(ctx, c.Obj, resType, tokens, pos+inc, objs) if err != nil { return err } } - return nil -} -func nameFromObjectContent(o types.ObjectContent) string { - for _, p := range o.PropSet { - if p.Name == "name" { - return p.Val.(string) - } + if rerunAsLeaf { + // We're at a "pseudo leaf", i.e. we looked ahead a token and found that this level contains leaf nodes. + // Rerun the entire level as a leaf to get those nodes. This will only be executed when pos is one token + // before the last, to pos+1 will always point to a leaf token. + return f.descend(ctx, root, resType, tokens, pos+1, objs) } - return "" + + return nil } func objectContentToTypedArray(objs map[string]types.ObjectContent, dst interface{}) error { @@ -214,11 +208,20 @@ func (r *ResourceFilter) FindAll(ctx context.Context, dst interface{}) error { return r.finder.FindAll(ctx, r.resType, r.paths, dst) } +func matchName(f property.Filter, props []types.DynamicProperty) bool { + for _, prop := range props { + if prop.Name == "name" { + return f.MatchProperty(prop) + } + } + return false +} + func init() { childTypes = map[string][]string{ "HostSystem": {"VirtualMachine"}, - "ComputeResource": {"HostSystem", "ResourcePool"}, - "ClusterComputeResource": {"HostSystem", "ResourcePool"}, + "ComputeResource": {"HostSystem", "ResourcePool", "VirtualApp"}, + "ClusterComputeResource": {"HostSystem", "ResourcePool", "VirtualApp"}, "Datacenter": {"Folder"}, "Folder": { "Folder", @@ -238,4 +241,13 @@ func init() { "ClusterComputeResource": {"parent"}, "Datacenter": {"parent"}, } + + containers = map[string]interface{}{ + "HostSystem": nil, + "ComputeResource": nil, + "Datacenter": nil, + "ResourcePool": nil, + "Folder": nil, + "VirtualApp": nil, + } } diff --git a/plugins/inputs/vsphere/vsphere_test.go b/plugins/inputs/vsphere/vsphere_test.go index 08e4405b3..28c2c7934 100644 --- a/plugins/inputs/vsphere/vsphere_test.go +++ b/plugins/inputs/vsphere/vsphere_test.go @@ -155,9 +155,13 @@ func defaultVSphere() *VSphere { } } -func createSim() (*simulator.Model, *simulator.Server, error) { +func createSim(folders int) (*simulator.Model, *simulator.Server, error) { model := simulator.VPX() + model.Folder = folders + model.Datacenter = 2 + //model.App = 1 + err := model.Create() if err != nil { return nil, nil, err @@ -262,7 +266,7 @@ func TestMaxQuery(t *testing.T) { if unsafe.Sizeof(i) < 8 { return } - m, s, err := createSim() + m, s, err := createSim(0) if err != nil { t.Fatal(err) } @@ -298,6 +302,20 @@ func TestMaxQuery(t *testing.T) { c2.close() } +func testLookupVM(ctx context.Context, t *testing.T, f *Finder, path string, expected int, expectedName string) { + poweredOn := types.VirtualMachinePowerState("poweredOn") + var vm []mo.VirtualMachine + err := f.Find(ctx, "VirtualMachine", path, &vm) + require.NoError(t, err) + require.Equal(t, expected, len(vm)) + if expectedName != "" { + require.Equal(t, expectedName, vm[0].Name) + } + for _, v := range vm { + require.Equal(t, poweredOn, v.Runtime.PowerState) + } +} + func TestFinder(t *testing.T) { // Don't run test on 32-bit machines due to bug in simulator. // https://github.com/vmware/govmomi/issues/1330 @@ -306,7 +324,7 @@ func TestFinder(t *testing.T) { return } - m, s, err := createSim() + m, s, err := createSim(0) if err != nil { t.Fatal(err) } @@ -320,13 +338,13 @@ func TestFinder(t *testing.T) { f := Finder{c} - dc := []mo.Datacenter{} + var dc []mo.Datacenter err = f.Find(ctx, "Datacenter", "/DC0", &dc) require.NoError(t, err) require.Equal(t, 1, len(dc)) require.Equal(t, "DC0", dc[0].Name) - host := []mo.HostSystem{} + var host []mo.HostSystem err = f.Find(ctx, "HostSystem", "/DC0/host/DC0_H0/DC0_H0", &host) require.NoError(t, err) require.Equal(t, 1, len(host)) @@ -343,67 +361,64 @@ func TestFinder(t *testing.T) { require.NoError(t, err) require.Equal(t, 3, len(host)) - vm := []mo.VirtualMachine{} - err = f.Find(ctx, "VirtualMachine", "/DC0/vm/DC0_H0_VM0", &vm) - require.NoError(t, err) - require.Equal(t, 1, len(dc)) - require.Equal(t, "DC0_H0_VM0", vm[0].Name) - - vm = []mo.VirtualMachine{} - err = f.Find(ctx, "VirtualMachine", "/DC0/vm/DC0_C0*", &vm) - require.NoError(t, err) - require.Equal(t, 1, len(dc)) - - vm = []mo.VirtualMachine{} - err = f.Find(ctx, "VirtualMachine", "/DC0/*/DC0_H0_VM0", &vm) - require.NoError(t, err) - require.Equal(t, 1, len(dc)) - require.Equal(t, "DC0_H0_VM0", vm[0].Name) - - vm = []mo.VirtualMachine{} - err = f.Find(ctx, "VirtualMachine", "/DC0/*/DC0_H0_*", &vm) - require.NoError(t, err) - require.Equal(t, 2, len(vm)) - - vm = []mo.VirtualMachine{} - err = f.Find(ctx, "VirtualMachine", "/DC0/**/DC0_H0_VM*", &vm) - require.NoError(t, err) - require.Equal(t, 2, len(vm)) - - vm = []mo.VirtualMachine{} - err = f.Find(ctx, "VirtualMachine", "/DC0/**", &vm) - require.NoError(t, err) - require.Equal(t, 4, len(vm)) - - vm = []mo.VirtualMachine{} - err = f.Find(ctx, "VirtualMachine", "/**", &vm) - require.NoError(t, err) - require.Equal(t, 4, len(vm)) - - vm = []mo.VirtualMachine{} - err = f.Find(ctx, "VirtualMachine", "/**/DC0_H0_VM*", &vm) - require.NoError(t, err) - require.Equal(t, 2, len(vm)) - - vm = []mo.VirtualMachine{} - err = f.Find(ctx, "VirtualMachine", "/DC0/**/DC0_H0_VM*", &vm) - require.NoError(t, err) - require.Equal(t, 2, len(vm)) - - vm = []mo.VirtualMachine{} - err = f.Find(ctx, "VirtualMachine", "/**/vm/**", &vm) - require.NoError(t, err) - require.Equal(t, 4, len(vm)) + var vm []mo.VirtualMachine + testLookupVM(ctx, t, &f, "/DC0/vm/DC0_H0_VM0", 1, "") + testLookupVM(ctx, t, &f, "/DC0/vm/DC0_C0*", 2, "") + testLookupVM(ctx, t, &f, "/DC0/*/DC0_H0_VM0", 1, "DC0_H0_VM0") + testLookupVM(ctx, t, &f, "/DC0/*/DC0_H0_*", 2, "") + testLookupVM(ctx, t, &f, "/DC0/**/DC0_H0_VM*", 2, "") + testLookupVM(ctx, t, &f, "/DC0/**", 4, "") + testLookupVM(ctx, t, &f, "/DC1/**", 4, "") + testLookupVM(ctx, t, &f, "/**", 8, "") + testLookupVM(ctx, t, &f, "/**/vm/**", 8, "") + testLookupVM(ctx, t, &f, "/*/host/**/*DC*", 8, "") + testLookupVM(ctx, t, &f, "/*/host/**/*DC*VM*", 8, "") + testLookupVM(ctx, t, &f, "/*/host/**/*DC*/*/*DC*", 4, "") vm = []mo.VirtualMachine{} err = f.FindAll(ctx, "VirtualMachine", []string{"/DC0/vm/DC0_H0*", "/DC0/vm/DC0_C0*"}, &vm) require.NoError(t, err) require.Equal(t, 4, len(vm)) - vm = []mo.VirtualMachine{} - err = f.FindAll(ctx, "VirtualMachine", []string{"/**"}, &vm) +} + +func TestFolders(t *testing.T) { + // Don't run test on 32-bit machines due to bug in simulator. + // https://github.com/vmware/govmomi/issues/1330 + var i int + if unsafe.Sizeof(i) < 8 { + return + } + + m, s, err := createSim(1) + if err != nil { + t.Fatal(err) + } + defer m.Remove() + defer s.Close() + + v := defaultVSphere() + ctx := context.Background() + + c, err := NewClient(ctx, s.URL, v) + + f := Finder{c} + + var folder []mo.Folder + err = f.Find(ctx, "Folder", "/F0", &folder) require.NoError(t, err) - require.Equal(t, 4, len(vm)) + require.Equal(t, 1, len(folder)) + require.Equal(t, "F0", folder[0].Name) + + var dc []mo.Datacenter + err = f.Find(ctx, "Datacenter", "/F0/DC1", &dc) + require.NoError(t, err) + require.Equal(t, 1, len(dc)) + require.Equal(t, "DC1", dc[0].Name) + + testLookupVM(ctx, t, &f, "/F0/DC0/vm/**/F*", 0, "") + testLookupVM(ctx, t, &f, "/F0/DC1/vm/**/F*/*VM*", 4, "") + testLookupVM(ctx, t, &f, "/F0/DC1/vm/**/F*/**", 4, "") } func TestAll(t *testing.T) { @@ -414,7 +429,7 @@ func TestAll(t *testing.T) { return } - m, s, err := createSim() + m, s, err := createSim(0) if err != nil { t.Fatal(err) }