From c336902573cb3dd99a296270d7936611a7571e96 Mon Sep 17 00:00:00 2001 From: Chris Grindstaff Date: Wed, 13 Nov 2024 14:24:29 -0500 Subject: [PATCH 1/3] refactor: simplify negotiateONTAPAPI --- cmd/collectors/ontap.go | 17 +++-------------- 1 file changed, 3 insertions(+), 14 deletions(-) diff --git a/cmd/collectors/ontap.go b/cmd/collectors/ontap.go index 05c313f61..cd8ea9520 100644 --- a/cmd/collectors/ontap.go +++ b/cmd/collectors/ontap.go @@ -13,23 +13,12 @@ import ( func GatherClusterInfo(pollerName string, cred *auth.Credentials) (conf.Remote, error) { - var ( - err error - ) - - remoteZapi, err := checkZapi(pollerName, cred) - if err != nil { - return conf.Remote{}, err - } - - remoteRest, err := checkRest(pollerName, cred) - if err != nil { - return remoteZapi, err - } + remoteZapi, errZapi := checkZapi(pollerName, cred) + remoteRest, errRest := checkRest(pollerName, cred) remoteRest.ZAPIsExist = remoteZapi.ZAPIsExist - return remoteRest, nil + return remoteRest, errors.Join(errZapi, errRest) } func checkRest(pollerName string, cred *auth.Credentials) (conf.Remote, error) { From 4ef26dc790ce190847f07ed4eee44b17e14dbaa1 Mon Sep 17 00:00:00 2001 From: Chris Grindstaff Date: Thu, 14 Nov 2024 09:56:35 -0500 Subject: [PATCH 2/3] refactor: simplify negotiateONTAPAPI test: cover merging remotes --- cmd/collectors/ontap.go | 16 ++++++- cmd/poller/poller.go | 14 +++--- cmd/poller/poller_test.go | 92 ++++++++++++++++++++++----------------- 3 files changed, 75 insertions(+), 47 deletions(-) diff --git a/cmd/collectors/ontap.go b/cmd/collectors/ontap.go index cd8ea9520..69799c617 100644 --- a/cmd/collectors/ontap.go +++ b/cmd/collectors/ontap.go @@ -16,9 +16,23 @@ func GatherClusterInfo(pollerName string, cred *auth.Credentials) (conf.Remote, remoteZapi, errZapi := checkZapi(pollerName, cred) remoteRest, errRest := checkRest(pollerName, cred) + return MergeRemotes(remoteZapi, remoteRest, errZapi, errRest) +} + +func MergeRemotes(remoteZapi conf.Remote, remoteRest conf.Remote, errZapi error, errRest error) (conf.Remote, error) { + err := errors.Join(errZapi, errRest) + + if errZapi != nil { + return remoteRest, err + } + + if errRest != nil { + return remoteZapi, err + } + remoteRest.ZAPIsExist = remoteZapi.ZAPIsExist - return remoteRest, errors.Join(errZapi, errRest) + return remoteRest, err } func checkRest(pollerName string, cred *auth.Credentials) (conf.Remote, error) { diff --git a/cmd/poller/poller.go b/cmd/poller/poller.go index a4ec90903..5d3731db5 100644 --- a/cmd/poller/poller.go +++ b/cmd/poller/poller.go @@ -301,7 +301,7 @@ func (p *Poller) Init() error { return errs.New(errs.ErrNoCollector, "no collectors") } - p.negotiateONTAPAPI(filteredCollectors, collectors.GatherClusterInfo) + p.negotiateONTAPAPI(filteredCollectors) objectsToCollectors := make(map[string][]objectCollector) for _, c := range filteredCollectors { @@ -711,7 +711,11 @@ func (p *Poller) readObjects(c conf.Collector) ([]objectCollector, error) { template, subTemplate *node.Node ) - c = p.upgradeCollector(c, p.remote) + newC := p.upgradeCollector(c, p.remote) + if newC.Name != c.Name { + logger.Info("upgraded collector", slog.String("old", c.Name), slog.String("new", newC.Name)) + } + c = newC class = c.Name // throw warning for deprecated collectors if r, d := deprecatedCollectors[strings.ToLower(class)]; d { @@ -1491,7 +1495,7 @@ func (p *Poller) sendHarvestVersion() error { return nil } -func (p *Poller) negotiateONTAPAPI(cols []conf.Collector, gatherClusterInfo func(pollerName string, cred *auth.Credentials) (conf.Remote, error)) { +func (p *Poller) negotiateONTAPAPI(cols []conf.Collector) { anyONTAP := false for _, c := range cols { if _, ok := util.IsONTAPCollector[c.Name]; ok { @@ -1504,9 +1508,9 @@ func (p *Poller) negotiateONTAPAPI(cols []conf.Collector, gatherClusterInfo func return } - remote, err := gatherClusterInfo(opts.Poller, p.auth) + remote, err := collectors.GatherClusterInfo(opts.Poller, p.auth) if err != nil { - logger.Error("Failed to gather cluster info", slogx.Err(err)) + logger.Warn("gather cluster info", slog.Any("remote", remote), slog.Any("remoteErr", err)) } p.remote = remote diff --git a/cmd/poller/poller_test.go b/cmd/poller/poller_test.go index a531b31ac..2104d2875 100644 --- a/cmd/poller/poller_test.go +++ b/cmd/poller/poller_test.go @@ -4,7 +4,7 @@ import ( "errors" "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" - "github.com/netapp/harvest/v2/pkg/auth" + "github.com/netapp/harvest/v2/cmd/collectors" "github.com/netapp/harvest/v2/pkg/conf" "github.com/netapp/harvest/v2/pkg/tree/node" "strings" @@ -212,11 +212,11 @@ func Test_nonOverlappingCollectors(t *testing.T) { } func ocs(names ...string) []objectCollector { - collectors := make([]objectCollector, 0, len(names)) + objectCollectors := make([]objectCollector, 0, len(names)) for _, n := range names { - collectors = append(collectors, objectCollector{class: n}) + objectCollectors = append(objectCollectors, objectCollector{class: n}) } - return collectors + return objectCollectors } func Test_uniquifyObjectCollectors(t *testing.T) { @@ -272,55 +272,65 @@ func objectCollectorMap(constructors ...string) map[string][]objectCollector { return objectsToCollectors } -func TestNegotiateONTAPAPI(t *testing.T) { +func TestMergeRemotes(t *testing.T) { - tests := []struct { - name string - collectors []conf.Collector - mockReturn conf.Remote - mockError error - expectedRemote conf.Remote - }{ + type test struct { + name string + remoteZapi conf.Remote + remoteRest conf.Remote + errZapi error + errRest error + want conf.Remote + wantErr bool + } + + tests := []test{ + { + name: "No ONTAP", + remoteZapi: conf.Remote{}, + remoteRest: conf.Remote{}, + errZapi: errors.New("no ZAPI"), + errRest: errors.New("no REST"), + want: conf.Remote{}, + wantErr: true, + }, { - name: "No ONTAP Collector", - collectors: []conf.Collector{ - {Name: "StorageGrid"}, - }, - mockReturn: conf.Remote{}, - mockError: nil, - expectedRemote: conf.Remote{}, + name: "No REST", + remoteZapi: conf.Remote{UUID: "abc", Version: "9.11.1", ZAPIsExist: true}, + remoteRest: conf.Remote{}, + errZapi: nil, + errRest: errors.New("no REST"), + want: conf.Remote{UUID: "abc", Version: "9.11.1", ZAPIsExist: true}, + wantErr: true, }, { - name: "ONTAP Collector with Success", - collectors: []conf.Collector{ - {Name: "Zapi"}, - }, - mockReturn: conf.Remote{Version: "9.11.1"}, - mockError: nil, - expectedRemote: conf.Remote{Version: "9.11.1"}, + name: "No ZAPIs", + remoteZapi: conf.Remote{}, + remoteRest: conf.Remote{UUID: "abc", Version: "9.17.1", HasREST: true}, + errZapi: errors.New("no ZAPI"), + errRest: nil, + want: conf.Remote{UUID: "abc", Version: "9.17.1", HasREST: true}, + wantErr: true, }, { - name: "ONTAP Collector with Error", - collectors: []conf.Collector{ - {Name: "Zapi"}, - }, - mockReturn: conf.Remote{Version: "9.11.1"}, - mockError: errors.New("failed to gather cluster info"), - expectedRemote: conf.Remote{Version: "9.11.1"}, + name: "Both", + remoteZapi: conf.Remote{UUID: "abc", Version: "9.17.1", ZAPIsExist: true, HasREST: true}, + remoteRest: conf.Remote{UUID: "abc", Version: "9.17.1", ZAPIsExist: true, HasREST: true}, + errZapi: nil, + errRest: nil, + want: conf.Remote{UUID: "abc", Version: "9.17.1", ZAPIsExist: true, HasREST: true}, + wantErr: false, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - mockGatherClusterInfo := func(_ string, _ *auth.Credentials) (conf.Remote, error) { - return tt.mockReturn, tt.mockError + gotRemote, err := collectors.MergeRemotes(tt.remoteZapi, tt.remoteRest, tt.errZapi, tt.errRest) + if (err != nil) != tt.wantErr { + t.Errorf("MergeRemotes() error = %v, wantErr %v", err, tt.wantErr) } - poller := Poller{} - - poller.negotiateONTAPAPI(tt.collectors, mockGatherClusterInfo) - - if diff := cmp.Diff(poller.remote, tt.expectedRemote); diff != "" { - t.Errorf("negotiateONTAPAPI() mismatch (-got +want):\n%s", diff) + if diff := cmp.Diff(gotRemote, tt.want); diff != "" { + t.Errorf("MergeRemotes() mismatch (-gotRemote +want):\n%s", diff) } }) } From 53477cce74b483bd4d58c46b390fbf6e2dea1114 Mon Sep 17 00:00:00 2001 From: Chris Grindstaff Date: Fri, 15 Nov 2024 08:37:28 -0500 Subject: [PATCH 3/3] refactor: simplify negotiateONTAPAPI test: cover merging remotes --- cmd/collectors/ontap.go | 7 ++++--- cmd/poller/poller_test.go | 9 +++++++++ 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/cmd/collectors/ontap.go b/cmd/collectors/ontap.go index 69799c617..7d592d942 100644 --- a/cmd/collectors/ontap.go +++ b/cmd/collectors/ontap.go @@ -22,6 +22,8 @@ func GatherClusterInfo(pollerName string, cred *auth.Credentials) (conf.Remote, func MergeRemotes(remoteZapi conf.Remote, remoteRest conf.Remote, errZapi error, errRest error) (conf.Remote, error) { err := errors.Join(errZapi, errRest) + remoteRest.ZAPIsExist = remoteZapi.ZAPIsExist + if errZapi != nil { return remoteRest, err } @@ -29,8 +31,6 @@ func MergeRemotes(remoteZapi conf.Remote, remoteRest conf.Remote, errZapi error, if errRest != nil { return remoteZapi, err } - - remoteRest.ZAPIsExist = remoteZapi.ZAPIsExist return remoteRest, err } @@ -95,7 +95,8 @@ func checkZapi(pollerName string, cred *auth.Credentials) (conf.Remote, error) { } if returnErr { - return conf.Remote{}, err + // Assume that ZAPIs exist so we don't upgrade ZAPI to REST when there is an error + return conf.Remote{ZAPIsExist: true}, err } } diff --git a/cmd/poller/poller_test.go b/cmd/poller/poller_test.go index 2104d2875..999e312ad 100644 --- a/cmd/poller/poller_test.go +++ b/cmd/poller/poller_test.go @@ -321,6 +321,15 @@ func TestMergeRemotes(t *testing.T) { want: conf.Remote{UUID: "abc", Version: "9.17.1", ZAPIsExist: true, HasREST: true}, wantErr: false, }, + { + name: "Both Fail", + remoteZapi: conf.Remote{ZAPIsExist: true}, + remoteRest: conf.Remote{}, + errZapi: errors.New("auth error"), + errRest: errors.New("auth error"), + want: conf.Remote{ZAPIsExist: true}, + wantErr: true, + }, } for _, tt := range tests {