Repository: NetApp/harvest
Pull request #326 (2021-07-21 13:07:45)

fix: Improve ZAPI template selection
fixes#322

rahulguptajss: edge cases covered. Thanks @cgrinds for the code improvements.
cgrinds: applied the review comments and validated with `9.10.1`

Patch:
diff --git a/cmd/poller/collector/helpers.go b/cmd/poller/collector/helpers.go
index 6fd6117b..3e80a6bf 100644
--- a/cmd/poller/collector/helpers.go
+++ b/cmd/poller/collector/helpers.go
@@ -13,10 +13,12 @@ package collector
 
 import (
 	"errors"
+	"fmt"
+	"github.com/hashicorp/go-version"
 	"io/ioutil"
 	"path"
 	"regexp"
-	"strconv"
+	"sort"
 	"strings"
 
 	"goharvest2/cmd/poller/plugin"
@@ -41,27 +43,24 @@ func ImportTemplate(confPath, confFn, collectorName string) (*node.Node, error)
 // multiple objects and each object is forked as a separate collector.
 // The subtemplates are sorted in subdirectories that serve as "tag" for the
 // matching ONTAP version. ImportSubTemplate will attempt to choose the subtemplate
-// with closest matching ONTAP version.
+// with the closest matching ONTAP version.
 //
 // Arguments:
 // @model		- ONTAP model, either cdot or 7mode
 // @filename	- name of the subtemplate
 // @version		- ONTAP version triple (generation, major, minor)
 //
-// BUG: ImportSubTemplate will break if ONTAP version is higher than 9.9.0.
-func (c *AbstractCollector) ImportSubTemplate(model, filename string, version [3]int) (*node.Node, error) {
+func (c *AbstractCollector) ImportSubTemplate(model, filename string, ver [3]int) (*node.Node, error) {
 
 	var (
 		selectedVersion, pathPrefix, subTemplateFp string
-		availableVersions                          map[string]bool
-		versionDecimal                             int
+		availableVersions                          []string
 	)
 
 	pathPrefix = path.Join(c.Options.HomePath, "conf/", strings.ToLower(c.Name), model)
 	c.Logger.Debug().Msgf("Looking for best-fitting template in [%s]", pathPrefix)
 
 	// check for available versions, those are the subdirectories that include filename
-	availableVersions = make(map[string]bool)
 	if files, err := ioutil.ReadDir(pathPrefix); err == nil {
 		for _, file := range files {
 			if match, _ := regexp.MatchString(`\d+\.\d+\.\d+`, file.Name()); match == true && file.IsDir() {
@@ -69,7 +68,7 @@ func (c *AbstractCollector) ImportSubTemplate(model, filename string, version [3
 					for _, t := range templates {
 						if t.Name() == filename {
 							c.Logger.Trace().Msgf("available version dir: [%s]", file.Name())
-							availableVersions[file.Name()] = true
+							availableVersions = append(availableVersions, file.Name())
 							break
 						}
 					}
@@ -81,20 +80,29 @@ func (c *AbstractCollector) ImportSubTemplate(model, filename string, version [3
 	}
 	c.Logger.Trace().Msgf("checking for %d available versions: %v", len(availableVersions), availableVersions)
 
-	versionDecimal = version[0]*100 + version[1]*10 + version[2]
+	if len(availableVersions) > 0 {
+		versions := make([]*version.Version, len(availableVersions))
+		for i, raw := range availableVersions {
+			v, err := version.NewVersion(raw)
+			if err != nil {
+				c.Logger.Trace().Msgf("error parsing version: %s err: %s", raw, err)
+				continue
+			}
+			versions[i] = v
+		}
 
-	for max := 0; max <= 100; max++ {
-		// check older version
-		str := strings.Join(strings.Split(strconv.Itoa(versionDecimal-max), ""), ".")
-		if _, exists := availableVersions[str]; exists == true {
-			selectedVersion = str
-			break
+		sort.Sort(version.Collection(versions))
+
+		verWithDots := strings.Trim(strings.Join(strings.Fields(fmt.Sprint(ver)), "."), "[]")
+		verS, err := version.NewVersion(verWithDots)
+		if err != nil {
+			c.Logger.Trace().Msgf("error parsing ONTAP version: %s err: %s", verWithDots, err)
+			return nil, errors.New("No best-fitting subtemplate version found")
 		}
-		// check newer version
-		str = strings.Join(strings.Split(strconv.Itoa(versionDecimal+max), ""), ".")
-		if _, exists := availableVersions[str]; exists == true {
-			selectedVersion = str
-			break
+		// get closest index
+		idx := getClosestIndex(versions, verS)
+		if idx >= 0 && idx < len(versions) {
+			selectedVersion = versions[idx].String()
 		}
 	}
 
@@ -103,10 +111,29 @@ func (c *AbstractCollector) ImportSubTemplate(model, filename string, version [3
 	}
 
 	subTemplateFp = path.Join(pathPrefix, selectedVersion, filename)
-	c.Logger.Debug().Msgf("selected best-fitting subtemplate [%s]", subTemplateFp)
+	c.Logger.Info().Msgf("selected best-fitting subtemplate [%s]", subTemplateFp)
 	return tree.Import("yaml", subTemplateFp)
 }
 
+//getClosestIndex input versions should be sorted
+// returns -1 if not match else returns equals or closest match to the left
+func getClosestIndex(versions []*version.Version, version *version.Version) int {
+	var l = 0
+	var r = len(versions) - 1
+	for l <= r {
+		var m = l + ((r - l) >> 1)
+		var comp = versions[m].Compare(version)
+		if comp < 0 { // versions[m] comes before the element
+			l = m + 1
+		} else if comp > 0 { // versions[m] comes after the element
+			r = m - 1
+		} else { // versions[m] equals the element
+			return m
+		}
+	}
+	return l - 1
+}
+
 // ParseMetricName parses display name from the raw name of the metric as defined in (sub)template.
 // Users can rename a metric with "=>" (e.g. some_long_metric_name => short).
 // Trailing "^" characters are ignored/cleaned as they have special meaning in some collectors.


Test patch:
diff --git a/cmd/poller/collector/helpers_test.go b/cmd/poller/collector/helpers_test.go
new file mode 100644
index 00000000..9a22b58c
--- /dev/null
+++ b/cmd/poller/collector/helpers_test.go
@@ -0,0 +1,47 @@
+package collector
+
+import (
+	"github.com/hashicorp/go-version"
+	"sort"
+	"testing"
+)
+
+func setupVersions(availableVersions []string) []*version.Version {
+	versions := make([]*version.Version, len(availableVersions))
+	for i, raw := range availableVersions {
+		versions[i] = buildVersion(raw)
+	}
+	sort.Sort(version.Collection(versions))
+	return versions
+}
+
+func buildVersion(ver string) *version.Version {
+	v, _ := version.NewVersion(ver)
+	return v
+}
+func Test_getClosestIndex(t *testing.T) {
+	type args struct {
+		versions []*version.Version
+		version  *version.Version
+	}
+
+	type test struct {
+		name string
+		args args
+		want int
+	}
+
+	tests := []test{
+		{"MatchCase", args{setupVersions([]string{"9.8.0", "9.8.1", "9.9.0", "10.10.10"}), buildVersion("9.8.1")}, 1},
+		{"NonMatchCase", args{setupVersions([]string{"9.8.0", "9.8.1", "9.9.0", "10.10.10"}), buildVersion("9.7.1")}, -1},
+		{"ClosestMatchCase", args{setupVersions([]string{"9.8.0", "9.8.1", "9.9.0", "10.10.10"}), buildVersion("9.9.2")}, 2},
+		{"EmptyCase", args{setupVersions([]string{}), buildVersion("9.8.1")}, -1},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := getClosestIndex(tt.args.versions, tt.args.version); got != tt.want {
+				t.Errorf("getClosestIndex() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}