diff options
author | Steve Azzopardi <sazzopardi@gitlab.com> | 2023-01-02 15:50:27 +0100 |
---|---|---|
committer | Ash McKenzie <amckenzie@gitlab.com> | 2023-01-12 02:56:43 +0000 |
commit | a093c9d3cfc1ee18368ebbf828dc61c15b74540c (patch) | |
tree | 5e10805b8ecd9d582ef27766f5374e8150a16aeb /internal | |
parent | c5b3accf0a8b2008137a90d40cfd65ba3ef2c99a (diff) | |
download | gitlab-shell-a093c9d3cfc1ee18368ebbf828dc61c15b74540c.tar.gz |
feat: retry on error
What
---
Change the default `HTTP.Client` to
`github.com/hashicorp/go-retryablehttp.Client` to get automatic retries
and exponential backoff.
We retry the request 2 times resulting in 3 attempts of sending the
request, the min retry wait is 1 second, and the maximum is 15
seconds.
Hide the retry logic behind a temporary feature flag
`FF_GITLAB_SHELL_RETRYABLE_HTTP` to easily roll this out in GitLab.com.
When we verify that this works as expected we will remove
`FF_GITLAB_SHELL_RETRYABLE_HTTP` and have the retry logic as the default
logic.
Why
---
In https://gitlab.com/gitlab-com/gl-infra/production/-/issues/7979 users
end up seeing the following errors when trying to `git-clone(1)` a
repository locally on in CI.
```shell
remote: ===============================
remote:
remote: ERROR: Internal API unreachable
remote:
remote: ================================
```
When we look at the application logs we see the following error:
```json
{ "err": "http://gitlab-webservice-git.gitlab.svc:8181/api/v4/internal/allowed":
dial tcp 10.69.184.120:8181: connect: connection refused", "msg":
"Internal API unreachable"}
```
In
https://gitlab.com/gitlab-com/gl-infra/production/-/issues/7979#note_1222670120
we've correlated these `connection refused` errors with infrastructure
events that remove the git pods that are hosting
`gitlab-webservice-git` service. We could try to make the underlying
infrastructure more reactive to these changes as suggested in
https://gitlab.com/gitlab-com/gl-infra/production/-/issues/7979#note_1225164944
but we can still end up serving bad requests.
Implementing retry logic for 5xx or other errors would allow users to
still be able to `git-clone(1)` reposirories, although it being slower.
This is espically important during CI runs so users don't have to retry
jobs themselves.
Reference: https://gitlab.com/gitlab-com/gl-infra/production/-/issues/7979
Closes: https://gitlab.com/gitlab-org/gitlab-shell/-/issues/604
Signed-off-by: Steve Azzopardi <sazzopardi@gitlab.com>
Diffstat (limited to 'internal')
7 files changed, 33 insertions, 39 deletions
diff --git a/internal/command/discover/discover_test.go b/internal/command/discover/discover_test.go index ccbfa7e..df9ca47 100644 --- a/internal/command/discover/discover_test.go +++ b/internal/command/discover/discover_test.go @@ -16,33 +16,31 @@ import ( "gitlab.com/gitlab-org/gitlab-shell/v14/internal/config" ) -var ( - requests = []testserver.TestRequestHandler{ - { - Path: "/api/v4/internal/discover", - Handler: func(w http.ResponseWriter, r *http.Request) { - if r.URL.Query().Get("key_id") == "1" || r.URL.Query().Get("username") == "alex-doe" { - body := map[string]interface{}{ - "id": 2, - "username": "alex-doe", - "name": "Alex Doe", - } - json.NewEncoder(w).Encode(body) - } else if r.URL.Query().Get("username") == "broken_message" { - body := map[string]string{ - "message": "Forbidden!", - } - w.WriteHeader(http.StatusForbidden) - json.NewEncoder(w).Encode(body) - } else if r.URL.Query().Get("username") == "broken" { - w.WriteHeader(http.StatusInternalServerError) - } else { - fmt.Fprint(w, "null") +var requests = []testserver.TestRequestHandler{ + { + Path: "/api/v4/internal/discover", + Handler: func(w http.ResponseWriter, r *http.Request) { + if r.URL.Query().Get("key_id") == "1" || r.URL.Query().Get("username") == "alex-doe" { + body := map[string]interface{}{ + "id": 2, + "username": "alex-doe", + "name": "Alex Doe", + } + json.NewEncoder(w).Encode(body) + } else if r.URL.Query().Get("username") == "broken_message" { + body := map[string]string{ + "message": "Forbidden!", } - }, + w.WriteHeader(http.StatusForbidden) + json.NewEncoder(w).Encode(body) + } else if r.URL.Query().Get("username") == "broken" { + w.WriteHeader(http.StatusInternalServerError) + } else { + fmt.Fprint(w, "null") + } }, - } -) + }, +} func TestExecute(t *testing.T) { url := testserver.StartSocketHttpServer(t, requests) @@ -112,7 +110,7 @@ func TestFailingExecute(t *testing.T) { { desc: "When the API fails", arguments: &commandargs.Shell{GitlabUsername: "broken"}, - expectedError: "Failed to get username: Internal API error (500)", + expectedError: "Failed to get username: Internal API unreachable", }, } diff --git a/internal/command/healthcheck/healthcheck_test.go b/internal/command/healthcheck/healthcheck_test.go index f642226..12a8444 100644 --- a/internal/command/healthcheck/healthcheck_test.go +++ b/internal/command/healthcheck/healthcheck_test.go @@ -84,5 +84,5 @@ func TestFailingAPIExecute(t *testing.T) { err := cmd.Execute(context.Background()) require.Empty(t, buffer.String()) - require.EqualError(t, err, "Internal API available: FAILED - Internal API error (500)") + require.EqualError(t, err, "Internal API available: FAILED - Internal API unreachable") } diff --git a/internal/command/personalaccesstoken/personalaccesstoken_test.go b/internal/command/personalaccesstoken/personalaccesstoken_test.go index daa5834..492f745 100644 --- a/internal/command/personalaccesstoken/personalaccesstoken_test.go +++ b/internal/command/personalaccesstoken/personalaccesstoken_test.go @@ -17,9 +17,7 @@ import ( "gitlab.com/gitlab-org/gitlab-shell/v14/internal/gitlabnet/personalaccesstoken" ) -var ( - requests []testserver.TestRequestHandler -) +var requests []testserver.TestRequestHandler func setup(t *testing.T) { requests = []testserver.TestRequestHandler{ @@ -147,7 +145,7 @@ func TestExecute(t *testing.T) { GitlabKeyId: "broken", SshArgs: []string{cmdname, "newtoken", "read_api,read_repository"}, }, - expectedError: "Internal API error (500)", + expectedError: "Internal API unreachable", }, { desc: "Without KeyID or User", diff --git a/internal/command/twofactorrecover/twofactorrecover_test.go b/internal/command/twofactorrecover/twofactorrecover_test.go index 81b4dff..7e20a06 100644 --- a/internal/command/twofactorrecover/twofactorrecover_test.go +++ b/internal/command/twofactorrecover/twofactorrecover_test.go @@ -18,9 +18,7 @@ import ( "gitlab.com/gitlab-org/gitlab-shell/v14/internal/gitlabnet/twofactorrecover" ) -var ( - requests []testserver.TestRequestHandler -) +var requests []testserver.TestRequestHandler func setup(t *testing.T) { requests = []testserver.TestRequestHandler{ @@ -99,7 +97,7 @@ func TestExecute(t *testing.T) { desc: "With API fails", arguments: &commandargs.Shell{GitlabKeyId: "broken"}, answer: "yes\n", - expectedOutput: question + errorHeader + "Internal API error (500)\n", + expectedOutput: question + errorHeader + "Internal API unreachable\n", }, { desc: "With missing arguments", diff --git a/internal/command/twofactorverify/twofactorverify_test.go b/internal/command/twofactorverify/twofactorverify_test.go index 640d3ed..213c025 100644 --- a/internal/command/twofactorverify/twofactorverify_test.go +++ b/internal/command/twofactorverify/twofactorverify_test.go @@ -136,7 +136,7 @@ func TestExecute(t *testing.T) { { desc: "With API fails", arguments: &commandargs.Shell{GitlabKeyId: "broken"}, - expectedOutput: errorHeader + "Internal API error (500)\n", + expectedOutput: errorHeader + "Internal API unreachable\n", }, { desc: "With missing arguments", diff --git a/internal/config/config.go b/internal/config/config.go index 35d8e74..7f473c5 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -134,8 +134,8 @@ func (c *Config) HttpClient() (*client.HttpClient, error) { return } - tr := client.Transport - client.Transport = metrics.NewRoundTripper(tr) + tr := client.HTTPClient.Transport + client.HTTPClient.Transport = metrics.NewRoundTripper(tr) c.httpClient = client }) diff --git a/internal/gitlabnet/lfsauthenticate/client_test.go b/internal/gitlabnet/lfsauthenticate/client_test.go index f62d8e1..01b3f84 100644 --- a/internal/gitlabnet/lfsauthenticate/client_test.go +++ b/internal/gitlabnet/lfsauthenticate/client_test.go @@ -74,7 +74,7 @@ func TestFailedRequests(t *testing.T) { { desc: "With API fails", args: &commandargs.Shell{GitlabKeyId: "broken", CommandType: commandargs.LfsAuthenticate, SshArgs: []string{"git-lfs-authenticate", repo, "download"}}, - expectedOutput: "Internal API error (500)", + expectedOutput: "Internal API unreachable", }, } |