fix(backend): fixes useless error message when visualization-server i…

…s not accessible. Fixes kubeflow#4157 (kubeflow#4201) Add another unit test to handle ServiceHostNotExistError
Jeffwan · Dec 9, 2020 · 7a4990a · 7a4990a
1 parent e08ea08
commit 7a4990a
Show file tree

Hide file tree

Showing 2 changed files with 38 additions and 10 deletions.
diff --git a/backend/src/apiserver/server/visualization_server.go b/backend/src/apiserver/server/visualization_server.go
@@ -14,6 +14,7 @@ import (
 	"github.com/kubeflow/pipelines/backend/src/apiserver/common"
 	"github.com/kubeflow/pipelines/backend/src/apiserver/resource"
 	"github.com/kubeflow/pipelines/backend/src/common/util"
+	"github.com/pkg/errors"
 )
 
 const (
@@ -78,11 +79,8 @@ func (s *VisualizationServer) validateCreateVisualizationRequest(request *go_cli
 // It returns the generated HTML as a string and any error that is encountered.
 func (s *VisualizationServer) generateVisualizationFromRequest(request *go_client.CreateVisualizationRequest) ([]byte, error) {
 	serviceURL := s.getVisualizationServiceURL(request)
-	if !isVisualizationServiceAlive(serviceURL) {
-		return nil, util.NewInternalServerError(
-			fmt.Errorf("service not available"),
-			"Service not available",
-		)
+	if err := isVisualizationServiceAlive(serviceURL); err != nil {
+		return nil, util.Wrap(err, "Cannot generate visualization.")
 	}
 	visualizationType := strings.ToLower(go_client.Visualization_Type_name[int32(request.Visualization.Type)])
 	urlValues := url.Values{
@@ -115,13 +113,19 @@ func (s *VisualizationServer) getVisualizationServiceURL(request *go_client.Crea
 	return s.serviceURL
 }
 
-func isVisualizationServiceAlive(serviceURL string) bool {
+func isVisualizationServiceAlive(serviceURL string) error {
 	resp, err := http.Get(serviceURL)
+
 	if err != nil {
-		glog.Error("Unable to verify visualization service is alive!", err)
-		return false
+		wrappedErr := util.Wrap(err, fmt.Sprintf("Unable to verify visualization service aliveness by sending request to %s", serviceURL))
+		glog.Error(wrappedErr)
+		return wrappedErr
+	} else if resp.StatusCode != http.StatusOK {
+		wrappedErr := errors.New(fmt.Sprintf("Unable to verify visualization service aliveness by sending request to %s and get response code: %s !", serviceURL, resp.Status))
+		glog.Error(wrappedErr)
+		return wrappedErr
 	}
-	return resp.StatusCode == http.StatusOK
+	return nil
 }
 
 func NewVisualizationServer(resourceManager *resource.ResourceManager, serviceHost string, servicePort string) *VisualizationServer {

diff --git a/backend/src/apiserver/server/visualization_server_test.go b/backend/src/apiserver/server/visualization_server_test.go
@@ -2,6 +2,7 @@ package server
 
 import (
 	"context"
+	"fmt"
 	"net/http"
 	"net/http/httptest"
 	"testing"
@@ -155,7 +156,30 @@ func TestGenerateVisualization_ServiceNotAvailableError(t *testing.T) {
 	}
 	body, err := server.generateVisualizationFromRequest(request)
 	assert.Nil(t, body)
-	assert.Equal(t, "InternalServerError: Service not available: service not available", err.Error())
+	assert.Contains(t, err.Error(), "500 Internal Server Error")
+}
+
+func TestGenerateVisualization_ServiceHostNotExistError(t *testing.T) {
+	clients, manager, _ := initWithExperiment(t)
+	defer clients.Close()
+	nonExistingServerURL := "http://127.0.0.2:53484"
+	server := &VisualizationServer{
+		resourceManager: manager,
+		serviceURL:      nonExistingServerURL,
+	}
+	visualization := &go_client.Visualization{
+		Type:      go_client.Visualization_ROC_CURVE,
+		Source:    "gs://ml-pipeline/roc/data.csv",
+		Arguments: "{}",
+	}
+	request := &go_client.CreateVisualizationRequest{
+		Visualization: visualization,
+	}
+	body, err := server.generateVisualizationFromRequest(request)
+	assert.Nil(t, body)
+	errMsg := err.Error()
+	assert.Contains(t, errMsg, "Unable to verify visualization service aliveness")
+	assert.Contains(t, err.Error(), fmt.Sprintf("dial tcp %s", nonExistingServerURL[7:]))
 }
 
 func TestGenerateVisualization_ServerError(t *testing.T) {