Skip to content
Snippets Groups Projects

fix: read error from upload artifacts execution.

Merged Paul 🐻 requested to merge paulrbr/gitlab-runner:fix-artifact-upload-errors into master
All threads resolved!

What does this MR do?

Fixes a bug when an error occurs during the "artifacts upload" stage.

Why was this MR needed?

During a failure in the upload artifacts step of a build jobs were not failing. This is a problem.

I have tested my patch with a local build and it makes the job fail if an error occurs during artifact uploading execution:

FATAL: invalid argument                            
Job succeeded
FATAL: invalid argument                            
ERROR: Job failed: exit code 1

Are there points in the code the reviewer needs to double check?

Help would be appreciated to create a test case for failed uploads builds.

Does this MR meet the acceptance criteria?

  • Documentation created/updated
  • Tests
    • Added for this feature/bug
    • All builds are passing
  • Branch has no merge conflicts with master (if you do - rebase it please)

What are the relevant issue numbers?

Fixes #2584 (closed)

Edited by Paul 🐻

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Paul 🐻 added 3 commits

    added 3 commits

    Compare with previous version

  • Paul 🐻 added 1 commit

    added 1 commit

    Compare with previous version

  • Paul 🐻 added 2 commits

    added 2 commits

    Compare with previous version

  • Paul 🐻 added 2 commits

    added 2 commits

    Compare with previous version

  • Paul 🐻 marked the checklist item All builds are passing as completed

    marked the checklist item All builds are passing as completed

  • Paul 🐻 marked the checklist item Added for this feature/bug as completed

    marked the checklist item Added for this feature/bug as completed

  • Paul 🐻 resolved all discussions

    resolved all discussions

  • Paul 🐻 marked the checklist item Branch has no merge conflicts with master (if you do - rebase it please) as completed

    marked the checklist item Branch has no merge conflicts with master (if you do - rebase it please) as completed

  • Thanks @paulrbr, looks good to me. The test is especially appreciated!

    The method still feels too complicated - perhaps the when logic could be extracted into a method of type Artifact - but that can be deferred until a later MR.

    @tmaczukin can you review as maintainer?

    Edited by Nick Thomas
  • assigned to @tmaczukin

  • I agree with @nick.thomas - we should simplify the when part and let's do this now.

    I propose to add this in common/network.go:

    func (aw ArtifactWhen) OnSuccess() bool {
    	return aw == "" || aw == ArtifactWhenOnSuccess || aw == ArtifactWhenAlways
    }
    
    func (aw ArtifactWhen) OnFailure() bool {
    	return aw == ArtifactWhenOnFailure || aw == ArtifactWhenAlways
    }
    
    func (a Artifact) ShouldUpload(state error) bool {
    	return (state == nil && a.When.OnSuccess()) || (state != nil && a.When.OnFailure())
    }

    Then we can change the executeUploadArtifacts() function to:

    func (b *Build) executeUploadArtifacts(ctx context.Context, state error, executor Executor) (uploadError error) {
    	for _, artifact := range b.Artifacts {
    		if artifact.ShouldUpload(state) {
    			uploadError = b.executeStage(ctx, BuildStageUploadArtifacts, executor)
    		}
    	}
    
    	return
    }

    Notice that here we're also declaring uploadError in function header and skipping the if uploadError != nil { err = uploadError } part at the end of the function.

    Rest looks good. After adding changes described above we can merge it :)

  • Tomasz Maczukin changed milestone to %9.5

    changed milestone to %9.5

  • assigned to @paulrbr

  • @tmaczukin thanks for the review.

    Agreed with simplifying the when part. :thumbsup:

    However for the uploadError variable I feel we'd make the method difficult to read. Of course I mentioned there's always only a size 1 slice, for now so it doesn't matter, but looking at the code I'm afraid someone will still think as @nick.thomas thought:

    there's another problem - say artifact 1 fails to upload, and we then upload artifact 2 successfully. In this case, err is overwritten with nil in the second iteration, and the job is marked as a success.

    If you prefer to keep the variable as in header + remove condition at the end, just say the word :).

    Edited by Paul 🐻
  • Paul 🐻 resolved all discussions

    resolved all discussions

  • Paul 🐻 added 5 commits

    added 5 commits

    Compare with previous version

  • Paul 🐻 added 1 commit

    added 1 commit

    Compare with previous version

  • @tmaczukin changes updated with your comment and rebased on master.

  • Tomasz Maczukin
  • Paul 🐻 resolved all discussions

    resolved all discussions

  • Paul 🐻 added 6 commits

    added 6 commits

    Compare with previous version

  • Hi @nick.thomas and @tmaczukin do you think we will be able yo integrate this fix into 9.5 still? I think we've gone through all discussions now. Thanks!

  • assigned to @tmaczukin

  • Tomasz Maczukin resolved all discussions

    resolved all discussions

  • @paulrbr One more thing. Let's add a test in common/network_test.go to check if ShouldUpload works properly:

    func doTestArtifactShouldUpload(t *testing.T, when ArtifactWhen, stateOK, expected bool) {
    	artifact := Artifact{When: when}
    
    	var state error
    	if !stateOK {
    		state = errors.New("Test error")
    	}
    
    	result := artifact.ShouldUpload(state)
    	if expected {
    		assert.True(t, result, "ShouldUpload should return true for when=%v and state=%v", when, state)
    	} else {
    		assert.False(t, result, "ShouldUpload should return false for when=%v and state=%v", when, state)
    	}
    }
    
    func TestArtifact_ShouldUpload(t *testing.T) {
    	examples := []struct {
    		when     ArtifactWhen
    		stateOK  bool
    		expected bool
    	}{
    		{when: "", stateOK: true, expected: true},
    		{when: ArtifactWhenOnSuccess, stateOK: true, expected: true},
    		{when: ArtifactWhenOnFailure, stateOK: true, expected: false},
    		{when: ArtifactWhenAlways, stateOK: true, expected: true},
    		{when: "", stateOK: false, expected: false},
    		{when: ArtifactWhenOnSuccess, stateOK: false, expected: false},
    		{when: ArtifactWhenOnFailure, stateOK: false, expected: true},
    		{when: ArtifactWhenAlways, stateOK: false, expected: true},
    	}
    
    	for _, example := range examples {
    		doTestArtifactShouldUpload(t, example.when, example.stateOK, example.expected)
    	}
    }

    If you will not be able to add this until tomorrow, I'll do it myself and merge this into 9.5 :)

  • Paul 🐻 added 1 commit

    added 1 commit

    • 0447319d - test: add a test for new ShouldUpload function

    Compare with previous version

  • Paul 🐻 added 5 commits

    added 5 commits

    Compare with previous version

  • Paul 🐻 added 1 commit

    added 1 commit

    • 031f7982 - test: add a test for new ShouldUpload function

    Compare with previous version

  • Paul 🐻 added 1 commit

    added 1 commit

    • 97c1bef9 - test: add a test for new ShouldUpload function

    Compare with previous version

  • Tomasz Maczukin
  • Paul 🐻 added 1 commit

    added 1 commit

    • d318e691 - test: add a test for new ShouldUpload function

    Compare with previous version

  • Paul 🐻 resolved all discussions

    resolved all discussions

  • Paul 🐻 added 1 commit

    added 1 commit

    • d318e691 - test: add a test for new ShouldUpload function

    Compare with previous version

  • Thanks for the extra test case @tmaczukin I've added a commit with it.

  • Paul 🐻 added 1 commit

    added 1 commit

    • d318e691 - test: add a test for new ShouldUpload function

    Compare with previous version

  • Paul 🐻 added 1 commit

    added 1 commit

    • d318e691 - test: add a test for new ShouldUpload function

    Compare with previous version

  • Paul 🐻 added 1 commit

    added 1 commit

    • fc554083 - test: add a test for new ShouldUpload function

    Compare with previous version

  • Paul 🐻 added 1 commit

    added 1 commit

    • fc554083 - test: add a test for new ShouldUpload function

    Compare with previous version

  • Hi @tmaczukin, MR is all set now.

  • Tomasz Maczukin approved this merge request

    approved this merge request

  • Tomasz Maczukin mentioned in commit 19ab26c3

    mentioned in commit 19ab26c3

  • mentioned in issue gitlab#22711

  • Please register or sign in to reply
    Loading