From: radhika <radhika@curoverse.com>
Date: Thu, 19 Nov 2015 19:54:37 +0000 (-0500)
Subject: 7490: use loggerutil to log any datamanager errors.
X-Git-Tag: 1.1.0~1240^2~4
X-Git-Url: https://git.arvados.org/arvados.git/commitdiff_plain/07f18157a5f8b72c20c0050c4617564effe91bd2?ds=sidebyside

7490: use loggerutil to log any datamanager errors.
---

diff --git a/services/datamanager/datamanager.go b/services/datamanager/datamanager.go
index f3be93c19f..50213787aa 100644
--- a/services/datamanager/datamanager.go
+++ b/services/datamanager/datamanager.go
@@ -42,21 +42,21 @@ func init() {
 func main() {
 	flag.Parse()
 	if minutesBetweenRuns == 0 {
-		arv, err := makeArvadosClient()
+		arv, err := arvadosclient.MakeArvadosClient()
 		if err != nil {
-			log.Fatalf("makeArvadosClient: %v", err)
+			loggerutil.FatalWithMessage(arvLogger, fmt.Sprintf("Error making arvados client: %v", err))
 		}
 		err = singlerun(arv)
 		if err != nil {
-			log.Fatalf("singlerun: %v", err)
+			loggerutil.FatalWithMessage(arvLogger, fmt.Sprintf("singlerun: %v", err))
 		}
 	} else {
 		waitTime := time.Minute * time.Duration(minutesBetweenRuns)
 		for {
 			log.Println("Beginning Run")
-			arv, err := makeArvadosClient()
+			arv, err := arvadosclient.MakeArvadosClient()
 			if err != nil {
-				log.Fatalf("makeArvadosClient: %v", err)
+				loggerutil.FatalWithMessage(arvLogger, fmt.Sprintf("Error making arvados client: %v", err))
 			}
 			err = singlerun(arv)
 			if err != nil {
@@ -68,9 +68,7 @@ func main() {
 	}
 }
 
-func makeArvadosClient() (arvadosclient.ArvadosClient, error) {
-	return arvadosclient.MakeArvadosClient()
-}
+var arvLogger *logger.Logger
 
 func singlerun(arv arvadosclient.ArvadosClient) error {
 	var err error
@@ -80,7 +78,6 @@ func singlerun(arv arvadosclient.ArvadosClient) error {
 		return errors.New("Current user is not an admin. Datamanager requires a privileged token.")
 	}
 
-	var arvLogger *logger.Logger
 	if logEventTypePrefix != "" {
 		arvLogger = logger.NewLogger(logger.LoggerParams{
 			Client:          arv,
@@ -111,7 +108,7 @@ func singlerun(arv arvadosclient.ArvadosClient) error {
 		return readCollections.Err
 	}
 
-	_, err = summary.MaybeWriteData(arvLogger, readCollections, keepServerInfo)
+	err = summary.MaybeWriteData(arvLogger, readCollections, keepServerInfo)
 	if err != nil {
 		return err
 	}
diff --git a/services/datamanager/datamanager_test.go b/services/datamanager/datamanager_test.go
index 4d82b4928b..53fb46b3ef 100644
--- a/services/datamanager/datamanager_test.go
+++ b/services/datamanager/datamanager_test.go
@@ -31,7 +31,7 @@ func SetupDataManagerTest(t *testing.T) {
 	arvadostest.StartKeep(2, false)
 
 	var err error
-	arv, err = makeArvadosClient()
+	arv, err = arvadosclient.MakeArvadosClient()
 	if err != nil {
 		t.Fatalf("Error making arvados client: %s", err)
 	}
diff --git a/services/datamanager/keep/keep_test.go b/services/datamanager/keep/keep_test.go
index b5091f2396..3ca7b8218a 100644
--- a/services/datamanager/keep/keep_test.go
+++ b/services/datamanager/keep/keep_test.go
@@ -3,6 +3,7 @@ package keep
 import (
 	"encoding/json"
 	"fmt"
+	"net"
 	"net/http"
 	"net/http/httptest"
 	"net/url"
@@ -152,18 +153,18 @@ type APITestData struct {
 }
 
 func (s *KeepSuite) TestGetKeepServers_UnsupportedServiceType(c *C) {
-	testGetKeepServersFromAPI(c, APITestData{1, "notadisk", 200})
+	testGetKeepServersFromAPI(c, APITestData{1, "notadisk", 200}, "Unsupported service type")
 }
 
 func (s *KeepSuite) TestGetKeepServers_ReceivedTooFewServers(c *C) {
-	testGetKeepServersFromAPI(c, APITestData{2, "disk", 200})
+	testGetKeepServersFromAPI(c, APITestData{2, "disk", 200}, "Did not receive all available keep servers")
 }
 
 func (s *KeepSuite) TestGetKeepServers_ServerError(c *C) {
-	testGetKeepServersFromAPI(c, APITestData{-1, "disk", -1})
+	testGetKeepServersFromAPI(c, APITestData{-1, "disk", -1}, "arvados API server error")
 }
 
-func testGetKeepServersFromAPI(c *C, testData APITestData) {
+func testGetKeepServersFromAPI(c *C, testData APITestData, expectedError string) {
 	keepServers := ServiceList{
 		ItemsAvailable: testData.numServers,
 		KeepServers: []ServerAddress{{
@@ -202,11 +203,8 @@ func testGetKeepServersFromAPI(c *C, testData APITestData) {
 	}
 
 	_, err := GetKeepServersAndSummarize(params)
-	if testData.numServers > 1 {
-		c.Assert(err, ErrorMatches, ".*Did not receive all available keep servers.*")
-	} else if testData.serverType != "disk" {
-		c.Assert(err, ErrorMatches, ".*Unsupported service type.*")
-	}
+	c.Assert(err, NotNil)
+	c.Assert(err, ErrorMatches, fmt.Sprintf(".*%s.*", expectedError))
 }
 
 type KeepServerTestData struct {
@@ -265,15 +263,15 @@ func testGetKeepServersAndSummarize(c *C, testData KeepServerTestData) {
 
 	ksURL, err := url.Parse(ks.URL)
 	c.Check(err, IsNil)
-	ksURLParts := strings.Split(ksURL.Host, ":")
-	ksPort, err := strconv.Atoi(ksURLParts[1])
+	ksHost, port, err := net.SplitHostPort(ksURL.Host)
+	ksPort, err := strconv.Atoi(port)
 	c.Check(err, IsNil)
 
 	servers_list := ServiceList{
 		ItemsAvailable: 1,
 		KeepServers: []ServerAddress{{
 			SSL:         false,
-			Host:        strings.Split(ksURL.Host, ":")[0],
+			Host:        ksHost,
 			Port:        ksPort,
 			UUID:        "abcdefg",
 			ServiceType: "disk",
diff --git a/services/datamanager/summary/file.go b/services/datamanager/summary/file.go
index 7ed9765c13..63c8adeb74 100644
--- a/services/datamanager/summary/file.go
+++ b/services/datamanager/summary/file.go
@@ -51,13 +51,13 @@ func init() {
 // working with stale data.
 func MaybeWriteData(arvLogger *logger.Logger,
 	readCollections collection.ReadCollections,
-	keepServerInfo keep.ReadServers) (bool, error) {
+	keepServerInfo keep.ReadServers) error {
 	if WriteDataTo == "" {
-		return false, nil
+		return nil
 	}
 	summaryFile, err := os.Create(WriteDataTo)
 	if err != nil {
-		return false, err
+		return err
 	}
 	defer summaryFile.Close()
 
@@ -67,10 +67,10 @@ func MaybeWriteData(arvLogger *logger.Logger,
 		KeepServerInfo:  keepServerInfo}
 	err = enc.Encode(data)
 	if err != nil {
-		return false, err
+		return err
 	}
 	log.Printf("Wrote summary data to: %s", WriteDataTo)
-	return true, nil
+	return nil
 }
 
 // ShouldReadData should not be used outside of development