diff options
author | Martin Polden <mpolden@mpolden.no> | 2023-04-17 10:27:31 +0200 |
---|---|---|
committer | Martin Polden <mpolden@mpolden.no> | 2023-04-17 10:31:43 +0200 |
commit | 7040b8c2c454a79316f800a5c4a9977e96905b81 (patch) | |
tree | 882f4f53e42a873e1437669c6df67a63be25d616 /client/go/internal | |
parent | 468dc5f1a47f3d7d90ae7e83476344c55c20b149 (diff) |
Never wait on 4xx for any target
Diffstat (limited to 'client/go/internal')
-rw-r--r-- | client/go/internal/cli/cmd/root.go | 1 | ||||
-rw-r--r-- | client/go/internal/vespa/deploy.go | 16 | ||||
-rw-r--r-- | client/go/internal/vespa/deploy_test.go | 2 | ||||
-rw-r--r-- | client/go/internal/vespa/target.go | 22 | ||||
-rw-r--r-- | client/go/internal/vespa/target_cloud.go | 16 | ||||
-rw-r--r-- | client/go/internal/vespa/target_custom.go | 6 | ||||
-rw-r--r-- | client/go/internal/vespa/target_test.go | 19 |
7 files changed, 55 insertions, 27 deletions
diff --git a/client/go/internal/cli/cmd/root.go b/client/go/internal/cli/cmd/root.go index 695d2eaca8f..88d43411983 100644 --- a/client/go/internal/cli/cmd/root.go +++ b/client/go/internal/cli/cmd/root.go @@ -465,7 +465,6 @@ func (c *CLI) createDeploymentOptions(pkg vespa.ApplicationPackage, target vespa ApplicationPackage: pkg, Target: target, Timeout: timeout, - HTTPClient: c.httpClient, }, nil } diff --git a/client/go/internal/vespa/deploy.go b/client/go/internal/vespa/deploy.go index 82fd014b377..f633c8ed9ee 100644 --- a/client/go/internal/vespa/deploy.go +++ b/client/go/internal/vespa/deploy.go @@ -45,7 +45,6 @@ type DeploymentOptions struct { ApplicationPackage ApplicationPackage Timeout time.Duration Version version.Version - HTTPClient util.HTTPClient } type LogLinePrepareResponse struct { @@ -130,7 +129,7 @@ func Prepare(deployment DeploymentOptions) (PrepareResult, error) { return PrepareResult{}, err } serviceDescription := "Deploy service" - response, err := deployment.HTTPClient.Do(req, time.Second*30) + response, err := deployServiceDo(req, time.Second*30, deployment) if err != nil { return PrepareResult{}, err } @@ -171,7 +170,7 @@ func Activate(sessionID int64, deployment DeploymentOptions) error { return err } serviceDescription := "Deploy service" - response, err := deployment.HTTPClient.Do(req, time.Second*30) + response, err := deployServiceDo(req, time.Second*30, deployment) if err != nil { return err } @@ -263,7 +262,7 @@ func Submit(opts DeploymentOptions) error { } request.Header.Set("Content-Type", writer.FormDataContentType()) serviceDescription := "Submit service" - response, err := opts.HTTPClient.Do(request, time.Minute*10) + response, err := deployServiceDo(request, time.Minute*10, opts) if err != nil { return err } @@ -271,6 +270,14 @@ func Submit(opts DeploymentOptions) error { return checkResponse(request, response, serviceDescription) } +func deployServiceDo(request *http.Request, timeout time.Duration, opts DeploymentOptions) (*http.Response, error) { + s, err := opts.Target.Service(DeployService, 0, 0, "") + if err != nil { + return nil, err + } + return s.Do(request, timeout) +} + func checkDeploymentOpts(opts DeploymentOptions) error { if opts.Target.Type() == TargetCloud && !opts.ApplicationPackage.HasCertificate() { return fmt.Errorf("%s: missing certificate in package", opts) @@ -330,7 +337,6 @@ func uploadApplicationPackage(url *url.URL, opts DeploymentOptions) (PrepareResu if err != nil { return PrepareResult{}, err } - response, err := service.Do(request, time.Minute*10) if err != nil { return PrepareResult{}, err diff --git a/client/go/internal/vespa/deploy_test.go b/client/go/internal/vespa/deploy_test.go index db3d17c432a..da2604282c0 100644 --- a/client/go/internal/vespa/deploy_test.go +++ b/client/go/internal/vespa/deploy_test.go @@ -24,7 +24,6 @@ func TestDeploy(t *testing.T) { opts := DeploymentOptions{ Target: target, ApplicationPackage: ApplicationPackage{Path: appDir}, - HTTPClient: &httpClient, } _, err := Deploy(opts) assert.Nil(t, err) @@ -47,7 +46,6 @@ func TestDeployCloud(t *testing.T) { opts := DeploymentOptions{ Target: target, ApplicationPackage: ApplicationPackage{Path: appDir}, - HTTPClient: &httpClient, } _, err := Deploy(opts) require.Nil(t, err) diff --git a/client/go/internal/vespa/target.go b/client/go/internal/vespa/target.go index 6d5d7efad91..9f3fd7f5c65 100644 --- a/client/go/internal/vespa/target.go +++ b/client/go/internal/vespa/target.go @@ -138,20 +138,36 @@ func (s *Service) Description() string { return fmt.Sprintf("No description of service %s", s.Name) } -func isOK(status int) bool { return status/100 == 2 } +func isOK(status int) (bool, error) { + class := status / 100 + switch class { + case 2: // success + return true, nil + case 4: // client error + return false, fmt.Errorf("request failed with status %d", status) + default: // retry + return false, nil + } +} type responseFunc func(status int, response []byte) (bool, error) type requestFunc func() *http.Request -// waitForOK queries url and returns its status code. If the url returns a non-200 status code, it is repeatedly queried +// waitForOK queries url and returns its status code. If response status is not 2xx or 4xx, it is repeatedly queried // until timeout elapses. func waitForOK(service *Service, url string, timeout time.Duration) (int, error) { req, err := http.NewRequest("GET", url, nil) if err != nil { return 0, err } - okFunc := func(status int, response []byte) (bool, error) { return isOK(status), nil } + okFunc := func(status int, response []byte) (bool, error) { + ok, err := isOK(status) + if err != nil { + return false, fmt.Errorf("failed to query %s at %s: %w", service.Description(), url, err) + } + return ok, err + } return wait(service, okFunc, func() *http.Request { return req }, timeout) } diff --git a/client/go/internal/vespa/target_cloud.go b/client/go/internal/vespa/target_cloud.go index e9dca55f654..928bb788494 100644 --- a/client/go/internal/vespa/target_cloud.go +++ b/client/go/internal/vespa/target_cloud.go @@ -123,7 +123,7 @@ func (t *cloudTarget) Service(name string, timeout time.Duration, runID int64, c if err != nil { return nil, err } - if !isOK(status) { + if ok, _ := isOK(status); !ok { return nil, fmt.Errorf("got status %d from deploy service at %s", status, service.BaseURL) } } @@ -209,7 +209,7 @@ func (t *cloudTarget) PrintLog(options LogOptions) error { return req } logFunc := func(status int, response []byte) (bool, error) { - if ok, err := isCloudOK(status); !ok { + if ok, err := isOK(status); !ok { return ok, err } logEntries, err := ReadLogEntries(bytes.NewReader(response)) @@ -272,7 +272,7 @@ func (t *cloudTarget) waitForRun(runID int64, timeout time.Duration) error { return req } jobSuccessFunc := func(status int, response []byte) (bool, error) { - if ok, err := isCloudOK(status); !ok { + if ok, err := isOK(status); !ok { return ok, err } var resp jobResponse @@ -327,7 +327,7 @@ func (t *cloudTarget) discoverEndpoints(timeout time.Duration) error { } urlsByCluster := make(map[string]string) endpointFunc := func(status int, response []byte) (bool, error) { - if ok, err := isCloudOK(status); !ok { + if ok, err := isOK(status); !ok { return ok, err } var resp deploymentResponse @@ -354,11 +354,3 @@ func (t *cloudTarget) discoverEndpoints(timeout time.Duration) error { t.deploymentOptions.ClusterURLs = urlsByCluster return nil } - -func isCloudOK(status int) (bool, error) { - if status == 401 { - // when retrying we should give up immediately if we're not authorized - return false, fmt.Errorf("status %d: invalid credentials", status) - } - return isOK(status), nil -} diff --git a/client/go/internal/vespa/target_custom.go b/client/go/internal/vespa/target_custom.go index df50e90a55b..0a3a9d48fed 100644 --- a/client/go/internal/vespa/target_custom.go +++ b/client/go/internal/vespa/target_custom.go @@ -61,7 +61,7 @@ func (t *customTarget) Service(name string, timeout time.Duration, sessionOrRunI if err != nil { return nil, err } - if !isOK(status) { + if ok, _ := isOK(status); !ok { return nil, fmt.Errorf("got status %d from deploy service at %s", status, service.BaseURL) } } else { @@ -111,8 +111,8 @@ func (t *customTarget) waitForConvergence(timeout time.Duration) error { } converged := false convergedFunc := func(status int, response []byte) (bool, error) { - if !isOK(status) { - return false, nil + if ok, err := isOK(status); !ok { + return ok, err } var resp serviceConvergeResponse if err := json.Unmarshal(response, &resp); err != nil { diff --git a/client/go/internal/vespa/target_test.go b/client/go/internal/vespa/target_test.go index d15001911d0..bf266e8f9ec 100644 --- a/client/go/internal/vespa/target_test.go +++ b/client/go/internal/vespa/target_test.go @@ -18,10 +18,16 @@ import ( type mockVespaApi struct { deploymentConverged bool + authFailure bool serverURL string } func (v *mockVespaApi) mockVespaHandler(w http.ResponseWriter, req *http.Request) { + if v.authFailure { + response := `{"message":"unauthorized"}` + w.WriteHeader(401) + w.Write([]byte(response)) + } switch req.URL.Path { case "/cli/v1/": response := `{"minVersion":"8.0.0"}` @@ -106,6 +112,9 @@ func TestCloudTargetWait(t *testing.T) { var logWriter bytes.Buffer target := createCloudTarget(t, srv.URL, &logWriter) + vc.authFailure = true + assertServiceWaitErr(t, 401, true, target, "deploy") + vc.authFailure = false assertServiceWait(t, 200, target, "deploy") _, err := target.Service("query", time.Millisecond, 42, "") @@ -188,11 +197,19 @@ func assertServiceURL(t *testing.T, url string, target Target, service string) { } func assertServiceWait(t *testing.T, expectedStatus int, target Target, service string) { + assertServiceWaitErr(t, expectedStatus, false, target, service) +} + +func assertServiceWaitErr(t *testing.T, expectedStatus int, expectErr bool, target Target, service string) { s, err := target.Service(service, 0, 42, "") assert.Nil(t, err) status, err := s.Wait(0) - assert.Nil(t, err) + if expectErr { + assert.NotNil(t, err) + } else { + assert.Nil(t, err) + } assert.Equal(t, expectedStatus, status) } |