Unverified Commit 0b4538cc authored by Steve Azzopardi's avatar Steve Azzopardi
Browse files

Revert 9e1d0676

After discussing this a bit more with fabiopitino there seems like
there was a bit of miscommunication. We want to have the 422 error not
retry the job but still consider it a valid response that the artifact
was uploaded. From an API perspective, this is not the best solution
since a 4xx status code is an error and should be treated as such.

In light of recent events such as
https://gitlab.com/gitlab-org/gitlab/issues/199764 we've seen spikes of
similar behavior as https://gitlab.com/gitlab-org/gitlab/issues/36516
(which was what this issue was trying to solve). We decided that the
following should happen:

1. Implement https://gitlab.com/gitlab-org/gitlab/-/merge_requests/24165
   which will handle the scenario where rails actually upload the artifact
   but fail to save the metadata or any other part. If there is a failure
   Rails would respond with a 4xx/5xx error accordingly and the Runner will
   retry to upload (this is already done) on the second try of uploading
   the artifact rails will check if artifact is already uploaded if it is,
   it will pick up from where it fails (for example storing the metadata)
1. Revert
https://gitlab.com/gitlab-org/gitlab-runner/-/merge_requests/1794
because of two things
    - It does not implement the original behavior we wanted. We wanted
      to have 422 act as a valid response and not to fail the build.
    - We don't want to have any special behavior for 422, if rails
      return 422 we still want to retry the upload.

This is not a full revert of 9e1d0676
because it included some refactoring. This just removes the knowledge of
the `422` error.
parent 41e15687
......@@ -111,8 +111,6 @@ func (c *ArtifactsUploaderCommand) createAndUpload() error {
return os.ErrPermission
case common.UploadTooLarge:
return errors.New("too large")
case common.UploadUnprocessableEntity:
return errors.New("unprocessable entity")
case common.UploadFailed:
return retryableErr{err: os.ErrInvalid}
default:
......
......@@ -76,31 +76,6 @@ func TestArtifactsUploaderForbidden(t *testing.T) {
assert.Equal(t, 1, network.uploadCalled)
}
func TestArtifactsUploaderUnprocessableEntityNoRetry(t *testing.T) {
network := &testNetwork{
uploadState: common.UploadUnprocessableEntity,
}
cmd := ArtifactsUploaderCommand{
JobCredentials: UploaderCredentials,
network: network,
fileArchiver: fileArchiver{
Paths: []string{artifactsTestArchivedFile},
},
}
writeTestFile(t, artifactsTestArchivedFile)
defer os.Remove(artifactsTestArchivedFile)
removeHook := helpers.MakeFatalToPanic()
defer removeHook()
assert.Panics(t, func() {
cmd.Execute(nil)
})
assert.Equal(t, 1, network.uploadCalled)
}
func TestArtifactsUploaderRetry(t *testing.T) {
network := &testNetwork{
uploadState: common.UploadFailed,
......
......@@ -41,7 +41,6 @@ const (
UploadTooLarge
UploadForbidden
UploadFailed
UploadUnprocessableEntity
)
const (
......
......@@ -492,9 +492,6 @@ func (n *GitLabClient) UploadRawArtifacts(config common.JobCredentials, reader i
case http.StatusRequestEntityTooLarge:
log.WithField("status", res.Status).Errorln("Uploading artifacts to coordinator...", "too large archive")
return common.UploadTooLarge
case http.StatusUnprocessableEntity:
log.WithField("status", res.Status).Errorln("Uploading artifacts to coordinator...", "unprocessable entity")
return common.UploadUnprocessableEntity
default:
log.WithField("status", res.Status).Warningln("Uploading artifacts to coordinator...", "failed")
return common.UploadFailed
......
......@@ -990,9 +990,6 @@ func checkTestArtifactsUploadHandlerContent(w http.ResponseWriter, r *http.Reque
statusCode: http.StatusCreated,
formValueKey: "artifact_type",
},
"unprocessable-entity": {
statusCode: http.StatusUnprocessableEntity,
},
}
testCase, ok := cases[body]
......@@ -1111,10 +1108,6 @@ func TestArtifactsUpload(t *testing.T) {
state = uploadArtifacts(c, invalidToken, tempFile.Name(), "", ArtifactFormatDefault)
assert.Equal(t, UploadForbidden, state, "Artifacts should be rejected if invalid token")
ioutil.WriteFile(tempFile.Name(), []byte("unprocessable-entity"), 0600)
state = uploadArtifacts(c, config, tempFile.Name(), "", ArtifactFormatDefault)
assert.Equal(t, UploadUnprocessableEntity, state, "Artifacts should be not uploaded, because of unprocessable entity response")
}
func testArtifactsDownloadHandler(w http.ResponseWriter, r *http.Request, t *testing.T) {
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment