refactor: simplify negotiateONTAPAPI

cmd/collectors/ontap.go

@@ -13,23 +13,26 @@ import (
 
 func GatherClusterInfo(pollerName string, cred *auth.Credentials) (conf.Remote, error) {
 
-	var (
-		err error
-	)
+	remoteZapi, errZapi := checkZapi(pollerName, cred)
+	remoteRest, errRest := checkRest(pollerName, cred)
 
-	remoteZapi, err := checkZapi(pollerName, cred)
-	if err != nil {
-		return conf.Remote{}, err
+	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)
+
+	remoteRest.ZAPIsExist = remoteZapi.ZAPIsExist
+
+	if errZapi != nil {
+		return remoteRest, err
 	}
 
-	remoteRest, err := checkRest(pollerName, cred)
-	if err != nil {
+	if errRest != nil {
 		return remoteZapi, err
 	}
 
-	remoteRest.ZAPIsExist = remoteZapi.ZAPIsExist
-
-	return remoteRest, nil
+	return remoteRest, err
 }
 
 func checkRest(pollerName string, cred *auth.Credentials) (conf.Remote, error) {
@@ -92,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
 		}
 	}
 

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

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,74 @@ 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 Collector",
-			collectors: []conf.Collector{
-				{Name: "StorageGrid"},
-			},
-			mockReturn:     conf.Remote{},
-			mockError:      nil,
-			expectedRemote: conf.Remote{},
+			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: "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 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 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:       "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:       "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,
+		},
+		{
+			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 {
 		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)
 			}
 		})
 	}