Merge pull request #19298 from vespa-engine/mpolden/wait-deploy-service

Support waiting for deploy service without existing deployment

9 files changed, 43 insertions, 52 deletions

diff --git a/client/go/cmd/command_tester.go b/client/go/cmd/command_tester.go
index 71f821521df..6929b59decb 100644
--- a/client/go/cmd/command_tester.go
+++ b/client/go/cmd/command_tester.go
@@ -116,5 +116,3 @@ func (c *mockHttpClient) Do(request *http.Request, timeout time.Duration) (*http
 }
 
 func (c *mockHttpClient) UseCertificate(certificate tls.Certificate) {}
-
-func convergeServices(client *mockHttpClient) { client.NextResponse(200, `{"converged":true}`) }
diff --git a/client/go/cmd/curl_test.go b/client/go/cmd/curl_test.go
index e67ef560c41..d5021e19cf2 100644
--- a/client/go/cmd/curl_test.go
+++ b/client/go/cmd/curl_test.go
@@ -12,7 +12,6 @@ import (
 func TestCurl(t *testing.T) {
 	homeDir := filepath.Join(t.TempDir(), ".vespa")
 	httpClient := &mockHttpClient{}
-	convergeServices(httpClient)
 	out, _ := execute(command{homeDir: homeDir, args: []string{"curl", "-n", "-a", "t1.a1.i1", "--", "-v", "--data-urlencode", "arg=with space", "/search"}}, t, httpClient)
 
 	expected := fmt.Sprintf("curl --key %s --cert %s -v --data-urlencode 'arg=with space' https://127.0.0.1:8080/search\n",
diff --git a/client/go/cmd/document_test.go b/client/go/cmd/document_test.go
index 8aecb538f89..1f82b85f915 100644
--- a/client/go/cmd/document_test.go
+++ b/client/go/cmd/document_test.go
@@ -67,7 +67,6 @@ func TestDocumentRemoveWithoutIdArg(t *testing.T) {
 func TestDocumentSendMissingId(t *testing.T) {
 	arguments := []string{"document", "put", "testdata/A-Head-Full-of-Dreams-Without-Operation.json"}
 	client := &mockHttpClient{}
-	convergeServices(client)
 	assert.Equal(t,
 		"Error: No document id given neither as argument or as a 'put' key in the json file\n",
 		executeCommand(t, client, arguments, []string{}))
@@ -76,7 +75,6 @@ func TestDocumentSendMissingId(t *testing.T) {
 func TestDocumentSendWithDisagreeingOperations(t *testing.T) {
 	arguments := []string{"document", "update", "testdata/A-Head-Full-of-Dreams-Put.json"}
 	client := &mockHttpClient{}
-	convergeServices(client)
 	assert.Equal(t,
 		"Error: Wanted document operation is update but the JSON file specifies put\n",
 		executeCommand(t, client, arguments, []string{}))
@@ -140,7 +138,6 @@ func assertDocumentGet(arguments []string, documentId string, t *testing.T) {
 
 func assertDocumentError(t *testing.T, status int, errorMessage string) {
 	client := &mockHttpClient{}
-	convergeServices(client)
 	client.NextResponse(status, errorMessage)
 	assert.Equal(t,
 		"Error: Invalid document operation: Status "+strconv.Itoa(status)+"\n\n"+errorMessage+"\n",
@@ -151,7 +148,6 @@ func assertDocumentError(t *testing.T, status int, errorMessage string) {
 
 func assertDocumentServerError(t *testing.T, status int, errorMessage string) {
 	client := &mockHttpClient{}
-	convergeServices(client)
 	client.NextResponse(status, errorMessage)
 	assert.Equal(t,
 		"Error: Container (document API) at 127.0.0.1:8080: Status "+strconv.Itoa(status)+"\n\n"+errorMessage+"\n",
@@ -161,6 +157,5 @@ func assertDocumentServerError(t *testing.T, status int, errorMessage string) {
 }
 
 func documentServiceURL(client *mockHttpClient) string {
-	convergeServices(client)
 	return getService("document", 0).BaseURL
 }
diff --git a/client/go/cmd/helpers.go b/client/go/cmd/helpers.go
index 09eef495018..98d6814d16f 100644
--- a/client/go/cmd/helpers.go
+++ b/client/go/cmd/helpers.go
@@ -131,14 +131,11 @@ func getService(service string, sessionOrRunID int64) *vespa.Service {
 	t := getTarget()
 	timeout := time.Duration(waitSecsArg) * time.Second
 	if timeout > 0 {
-		log.Printf("Waiting up to %d %s for services to become available ...", color.Cyan(waitSecsArg), color.Cyan("seconds"))
+		log.Printf("Waiting up to %d %s for service to become available ...", color.Cyan(waitSecsArg), color.Cyan("seconds"))
 	}
-	if err := t.DiscoverServices(timeout, sessionOrRunID); err != nil {
-		fatalErr(err, "Services unavailable")
-	}
-	s, err := t.Service(service)
+	s, err := t.Service(service, timeout, sessionOrRunID)
 	if err != nil {
-		fatalErr(err, "Invalid service")
+		fatalErr(err, "Invalid service: ", service)
 	}
 	return s
 }
diff --git a/client/go/cmd/query_test.go b/client/go/cmd/query_test.go
index bd9ae91f24d..137ffa01cd5 100644
--- a/client/go/cmd/query_test.go
+++ b/client/go/cmd/query_test.go
@@ -56,7 +56,6 @@ func assertQuery(t *testing.T, expectedQuery string, query ...string) {
 
 func assertQueryError(t *testing.T, status int, errorMessage string) {
 	client := &mockHttpClient{}
-	convergeServices(client)
 	client.NextResponse(status, errorMessage)
 	assert.Equal(t,
 		"Error: Invalid query: Status "+strconv.Itoa(status)+"\n"+errorMessage+"\n",
@@ -66,7 +65,6 @@ func assertQueryError(t *testing.T, status int, errorMessage string) {
 
 func assertQueryServiceError(t *testing.T, status int, errorMessage string) {
 	client := &mockHttpClient{}
-	convergeServices(client)
 	client.NextResponse(status, errorMessage)
 	assert.Equal(t,
 		"Error: Status "+strconv.Itoa(status)+" from container at 127.0.0.1:8080\n"+errorMessage+"\n",
@@ -75,6 +73,5 @@ func assertQueryServiceError(t *testing.T, status int, errorMessage string) {
 }
 
 func queryServiceURL(client *mockHttpClient) string {
-	convergeServices(client)
 	return getService("query", 0).BaseURL
 }
diff --git a/client/go/cmd/status_test.go b/client/go/cmd/status_test.go
index 8ddca71a35b..0c1c8e4e3a7 100644
--- a/client/go/cmd/status_test.go
+++ b/client/go/cmd/status_test.go
@@ -44,7 +44,6 @@ func TestStatusErrorResponse(t *testing.T) {
 
 func assertDeployStatus(target string, args []string, t *testing.T) {
 	client := &mockHttpClient{}
-	convergeServices(client)
 	assert.Equal(t,
 		"Deploy API at "+target+" is ready\n",
 		executeCommand(t, client, []string{"status", "deploy"}, args),
@@ -54,14 +53,12 @@ func assertDeployStatus(target string, args []string, t *testing.T) {
 
 func assertQueryStatus(target string, args []string, t *testing.T) {
 	client := &mockHttpClient{}
-	convergeServices(client)
 	assert.Equal(t,
 		"Container (query API) at "+target+" is ready\n",
 		executeCommand(t, client, []string{"status", "query"}, args),
 		"vespa status container")
 	assert.Equal(t, target+"/ApplicationStatus", client.lastRequest.URL.String())
 
-	convergeServices(client)
 	assert.Equal(t,
 		"Container (query API) at "+target+" is ready\n",
 		executeCommand(t, client, []string{"status"}, args),
@@ -71,7 +68,6 @@ func assertQueryStatus(target string, args []string, t *testing.T) {
 
 func assertDocumentStatus(target string, args []string, t *testing.T) {
 	client := &mockHttpClient{}
-	convergeServices(client)
 	assert.Equal(t,
 		"Container (document API) at "+target+" is ready\n",
 		executeCommand(t, client, []string{"status", "document"}, args),
@@ -81,7 +77,6 @@ func assertDocumentStatus(target string, args []string, t *testing.T) {
 
 func assertQueryStatusError(target string, args []string, t *testing.T) {
 	client := &mockHttpClient{}
-	convergeServices(client)
 	client.NextStatus(500)
 	assert.Equal(t,
 		"Container (query API) at "+target+" is not ready\nStatus 500\n",
diff --git a/client/go/vespa/deploy.go b/client/go/vespa/deploy.go
index ece841617c0..19319724d18 100644
--- a/client/go/vespa/deploy.go
+++ b/client/go/vespa/deploy.go
@@ -71,7 +71,7 @@ func (d DeploymentOpts) String() string {
 func (d *DeploymentOpts) IsCloud() bool { return d.Target.Type() == cloudTargetType }
 
 func (d *DeploymentOpts) url(path string) (*url.URL, error) {
-	service, err := d.Target.Service("deploy")
+	service, err := d.Target.Service(deployService, 0, 0)
 	if err != nil {
 		return nil, err
 	}
diff --git a/client/go/vespa/target.go b/client/go/vespa/target.go
index 24452a96f0f..df9144cd186 100644
--- a/client/go/vespa/target.go
+++ b/client/go/vespa/target.go
@@ -39,11 +39,8 @@ type Target interface {
 	// Type returns this target's type, e.g. local or cloud.
 	Type() string
 
-	// Service returns the service for given name.
-	Service(name string) (*Service, error)
-
-	// DiscoverServices queries for services available on this target after the deployment run has completed.
-	DiscoverServices(timeout time.Duration, runID int64) error
+	// Service returns the service for given name. If timeout is non-zero, wait for the service to converge.
+	Service(name string, timeout time.Duration, sessionOrRunID int64) (*Service, error)
 }
 
 // TLSOptions configures the certificate to use for service requests.
@@ -105,7 +102,12 @@ func (s *Service) Description() string {
 
 func (t *customTarget) Type() string { return t.targetType }
 
-func (t *customTarget) Service(name string) (*Service, error) {
+func (t *customTarget) Service(name string, timeout time.Duration, sessionID int64) (*Service, error) {
+	if timeout > 0 && name != deployService {
+		if err := t.waitForConvergence(timeout); err != nil {
+			return nil, err
+		}
+	}
 	switch name {
 	case deployService, queryService, documentService:
 		url, err := t.urlWithPort(name)
@@ -137,12 +139,12 @@ func (t *customTarget) urlWithPort(serviceName string) (string, error) {
 	return u.String(), nil
 }
 
-func (t *customTarget) DiscoverServices(timeout time.Duration, runID int64) error {
-	deployService, err := t.Service("deploy")
+func (t *customTarget) waitForConvergence(timeout time.Duration) error {
+	deployer, err := t.Service(deployService, 0, 0)
 	if err != nil {
 		return err
 	}
-	url := fmt.Sprintf("%s/application/v2/tenant/default/application/default/environment/prod/region/default/instance/default/serviceconverge", deployService.BaseURL)
+	url := fmt.Sprintf("%s/application/v2/tenant/default/application/default/environment/prod/region/default/instance/default/serviceconverge", deployer.BaseURL)
 	req, err := http.NewRequest("GET", url, nil)
 	if err != nil {
 		return err
@@ -182,27 +184,30 @@ type cloudTarget struct {
 
 func (t *cloudTarget) Type() string { return t.targetType }
 
-func (t *cloudTarget) Service(name string) (*Service, error) {
+func (t *cloudTarget) Service(name string, timeout time.Duration, runID int64) (*Service, error) {
+	if timeout > 0 && name != deployService {
+		if err := t.waitForEndpoints(timeout, runID); err != nil {
+			return nil, err
+		}
+	}
 	switch name {
 	case deployService:
 		return &Service{Name: name, BaseURL: t.apiURL}, nil
 	case queryService:
 		if t.queryURL == "" {
-			return nil, fmt.Errorf("service %s not discovered", name)
+			return nil, fmt.Errorf("service %s is not discovered", name)
 		}
 		return &Service{Name: name, BaseURL: t.queryURL, TLSOptions: t.tlsOptions}, nil
 	case documentService:
 		if t.documentURL == "" {
-			return nil, fmt.Errorf("service %s not discovered", name)
+			return nil, fmt.Errorf("service %s is not discovered", name)
 		}
 		return &Service{Name: name, BaseURL: t.documentURL, TLSOptions: t.tlsOptions}, nil
 	}
 	return nil, fmt.Errorf("unknown service: %s", name)
 }
 
-// DiscoverServices waits for run identified by runID to complete and at least one endpoint is available, or timeout
-// passes.
-func (t *cloudTarget) DiscoverServices(timeout time.Duration, runID int64) error {
+func (t *cloudTarget) waitForEndpoints(timeout time.Duration, runID int64) error {
 	signer := NewRequestSigner(t.deployment.Application.SerializedForm(), t.apiKey)
 	if runID > 0 {
 		if err := t.waitForRun(signer, runID, timeout); err != nil {
@@ -232,8 +237,8 @@ func (t *cloudTarget) waitForRun(signer *RequestSigner, runID int64, timeout tim
 		return req
 	}
 	jobSuccessFunc := func(status int, response []byte) (bool, error) {
-		if status/100 != 2 {
-			return false, nil
+		if ok, err := isOK(status); !ok {
+			return ok, err
 		}
 		var resp jobResponse
 		if err := json.Unmarshal(response, &resp); err != nil {
@@ -290,8 +295,8 @@ func (t *cloudTarget) discoverEndpoints(signer *RequestSigner, timeout time.Dura
 	}
 	var endpointURL string
 	endpointFunc := func(status int, response []byte) (bool, error) {
-		if status/100 != 2 {
-			return false, nil
+		if ok, err := isOK(status); !ok {
+			return ok, err
 		}
 		var resp deploymentResponse
 		if err := json.Unmarshal(response, &resp); err != nil {
@@ -314,6 +319,13 @@ func (t *cloudTarget) discoverEndpoints(signer *RequestSigner, timeout time.Dura
 	return nil
 }
 
+func isOK(status int) (bool, error) {
+	if status == 401 {
+		return false, fmt.Errorf("status %d: invalid api key", status)
+	}
+	return status/100 == 2, nil
+}
+
 // LocalTarget creates a target for a Vespa platform running locally.
 func LocalTarget() Target {
 	return &customTarget{targetType: localTargetType, baseURL: "http://127.0.0.1"}
@@ -407,7 +419,8 @@ func wait(fn responseFunc, reqFn requestFunc, certificate *tls.Certificate, time
 				return statusCode, nil
 			}
 		}
-		if loopOnce {
+		timeLeft := deadline.Sub(time.Now())
+		if loopOnce || timeLeft < waitRetryInterval {
 			break
 		}
 		time.Sleep(waitRetryInterval)
diff --git a/client/go/vespa/target_test.go b/client/go/vespa/target_test.go
index 9e95c8c6ac2..2c90baefbbc 100644
--- a/client/go/vespa/target_test.go
+++ b/client/go/vespa/target_test.go
@@ -74,11 +74,11 @@ func TestCustomTargetWait(t *testing.T) {
 	defer srv.Close()
 	target := CustomTarget(srv.URL)
 
-	err := target.DiscoverServices(0, 42)
+	_, err := target.Service("query", time.Millisecond, 42)
 	assert.NotNil(t, err)
 
 	vc.deploymentConverged = true
-	err = target.DiscoverServices(0, 42)
+	_, err = target.Service("query", time.Millisecond, 42)
 	assert.Nil(t, err)
 
 	assertServiceWait(t, 200, target, "deploy")
@@ -117,14 +117,11 @@ func TestCloudTargetWait(t *testing.T) {
 	}
 	assertServiceWait(t, 200, target, "deploy")
 
-	_, err = target.Service("query")
-	assert.NotNil(t, err)
-
-	err = target.DiscoverServices(0, 42)
+	_, err = target.Service("query", time.Millisecond, 42)
 	assert.NotNil(t, err)
 
 	vc.deploymentConverged = true
-	err = target.DiscoverServices(0, 42)
+	_, err = target.Service("query", time.Millisecond, 42)
 	assert.Nil(t, err)
 
 	assertServiceWait(t, 500, target, "query")
@@ -137,13 +134,13 @@ func TestCloudTargetWait(t *testing.T) {
 }
 
 func assertServiceURL(t *testing.T, url string, target Target, service string) {
-	s, err := target.Service(service)
+	s, err := target.Service(service, 0, 42)
 	assert.Nil(t, err)
 	assert.Equal(t, url, s.BaseURL)
 }
 
 func assertServiceWait(t *testing.T, expectedStatus int, target Target, service string) {
-	s, err := target.Service(service)
+	s, err := target.Service(service, 0, 42)
 	assert.Nil(t, err)
 
 	status, err := s.Wait(0)