Commit 61f9a320 authored by Kamil Trzciński's avatar Kamil Trzciński
Browse files

Merge branch 'revert-411b98f3' into 'master'

Revert "Merge branch 'ajwalker/after_script-kamil' into 'master'"

See merge request gitlab-org/gitlab-runner!2469
parents fb8bfebb c8fc8aca
......@@ -74,7 +74,7 @@ func TestBuildsHelperCollect(t *testing.T) {
}
err = <-done
expected := &common.BuildError{FailureReason: common.JobAborted}
expected := &common.BuildError{FailureReason: common.JobCanceled}
assert.True(t, errors.Is(err, expected), "expected: %[1]T (%[1]v), got: %[2]T (%[2]v)", expected, err)
}
......
......@@ -56,15 +56,11 @@ const (
type BuildRuntimeState string
const (
BuildRunStatePending BuildRuntimeState = "pending"
BuildRunRuntimeRunning BuildRuntimeState = "running"
BuildRunRuntimeSuccess BuildRuntimeState = "success"
BuildRunRuntimeFailed BuildRuntimeState = "failed"
// This runtime state indicates that operation did finish
// which is not exactly true, canceled means that we can still
// be canceling job (running after_script)
BuildRunStatePending BuildRuntimeState = "pending"
BuildRunRuntimeRunning BuildRuntimeState = "running"
BuildRunRuntimeSuccess BuildRuntimeState = "success"
BuildRunRuntimeFailed BuildRuntimeState = "failed"
BuildRunRuntimeCanceled BuildRuntimeState = "canceled"
BuildRunRuntimeAborted BuildRuntimeState = "aborted"
BuildRunRuntimeTerminated BuildRuntimeState = "terminated"
BuildRunRuntimeTimedout BuildRuntimeState = "timedout"
)
......@@ -109,18 +105,6 @@ const (
// build stage.
var ErrSkipBuildStage = errors.New("skip build stage")
var (
errCanceledBuildError = &BuildError{
Inner: errors.New("canceled"),
FailureReason: JobCanceled,
}
errAbortedBuildError = &BuildError{
Inner: errors.New("aborted"),
FailureReason: JobAborted,
}
)
type invalidAttemptError struct {
key string
}
......@@ -447,30 +431,11 @@ func (b *Build) executeArchiveCache(ctx context.Context, state error, executor E
return b.executeStage(ctx, BuildStageArchiveOnFailureCache, executor)
}
func (b *Build) executeSteps(ctx context.Context, executor Executor) error {
for _, s := range b.Steps {
// after_script has a separate BuildStage. See common.BuildStageAfterScript
if s.Name == StepNameAfterScript {
continue
}
err := b.executeStage(ctx, StepToBuildStage(s), executor)
if err != nil {
return err
}
}
return nil
}
func (b *Build) executeScript(abortCtx context.Context, trace JobTrace, executor Executor) error {
func (b *Build) executeScript(ctx context.Context, executor Executor) error {
// track job start and create referees
startTime := time.Now()
b.createReferees(executor)
ctx, cancel := context.WithCancel(abortCtx)
defer cancel()
trace.SetCancelFunc(cancel)
// Prepare stage
err := b.executeStage(ctx, BuildStagePrepare, executor)
if err != nil {
......@@ -491,15 +456,18 @@ func (b *Build) executeScript(abortCtx context.Context, trace JobTrace, executor
}
if err == nil {
err = b.executeSteps(ctx, executor)
// This does indicate that build got canceled, instead of aborted
if ctx.Err() != nil && abortCtx.Err() == nil {
err = errCanceledBuildError
for _, s := range b.Steps {
// after_script has a separate BuildStage. See common.BuildStageAfterScript
if s.Name == StepNameAfterScript {
continue
}
err = b.executeStage(ctx, StepToBuildStage(s), executor)
if err != nil {
break
}
}
// After script should be executed always regardless of `ctx` being canceled
b.executeAfterScript(abortCtx, err, executor)
b.executeAfterScript(ctx, err, executor)
}
archiveCacheErr := b.executeArchiveCache(ctx, err, executor)
......@@ -507,7 +475,8 @@ func (b *Build) executeScript(abortCtx context.Context, trace JobTrace, executor
artifactUploadErr := b.executeUploadArtifacts(ctx, err, executor)
// track job end and execute referees
b.executeUploadReferees(ctx, startTime, time.Now())
endTime := time.Now()
b.executeUploadReferees(ctx, startTime, endTime)
b.removeFileBasedVariables(ctx, executor)
......@@ -624,14 +593,11 @@ func (b *Build) handleError(err error) error {
func (b *Build) runtimeStateAndError(err error) (BuildRuntimeState, error) {
switch err {
case errCanceledBuildError:
return BuildRunRuntimeCanceled, err
case context.Canceled:
// This is not obvious:
// it tries to discover a `abortCtx` being canceled,
// thus having an abort outcome
return BuildRunRuntimeAborted, errAbortedBuildError
return BuildRunRuntimeCanceled, &BuildError{
Inner: errors.New("canceled"),
FailureReason: JobCanceled,
}
case context.DeadlineExceeded:
return BuildRunRuntimeTimedout, &BuildError{
......@@ -647,7 +613,7 @@ func (b *Build) runtimeStateAndError(err error) (BuildRuntimeState, error) {
}
}
func (b *Build) run(ctx context.Context, trace JobTrace, executor Executor) (err error) {
func (b *Build) run(ctx context.Context, executor Executor) (err error) {
b.setCurrentState(BuildRunRuntimeRunning)
buildFinish := make(chan error, 1)
......@@ -672,7 +638,7 @@ func (b *Build) run(ctx context.Context, trace JobTrace, executor Executor) (err
}
}()
buildFinish <- b.executeScript(runContext, trace, executor)
buildFinish <- b.executeScript(runContext, executor)
}()
// Wait for signals: cancel, timeout, abort or finish
......@@ -890,7 +856,6 @@ func (b *Build) Run(globalConfig *Config, trace JobTrace) (err error) {
ctx, cancel := context.WithTimeout(context.Background(), b.GetBuildTimeout())
defer cancel()
// In early phases of build preparation a user-requested cancel is treated as an abort
trace.SetCancelFunc(cancel)
trace.SetAbortFunc(cancel)
trace.SetMasked(b.GetAllVariables().Masked())
......@@ -909,7 +874,7 @@ func (b *Build) Run(globalConfig *Config, trace JobTrace) (err error) {
executor, err = b.executeBuildSection(executor, options, provider)
if err == nil {
err = b.run(ctx, trace, executor)
err = b.run(ctx, executor)
if errWait := b.waitForTerminal(ctx, globalConfig.SessionServer.GetSessionTimeout()); errWait != nil {
b.Log().WithError(errWait).Debug("Stopped waiting for terminal")
}
......
......@@ -382,7 +382,7 @@ func TestJobFailure(t *testing.T) {
defer trace.AssertExpectations(t)
trace.On("Write", mock.Anything).Return(0, nil)
trace.On("IsStdout").Return(true)
trace.On("SetCancelFunc", mock.Anything).Twice()
trace.On("SetCancelFunc", mock.Anything).Once()
trace.On("SetAbortFunc", mock.Anything).Once()
trace.On("SetMasked", mock.Anything).Once()
trace.On("Fail", thrownErr, ScriptFailure).Once()
......@@ -417,7 +417,7 @@ func TestJobFailureOnExecutionTimeout(t *testing.T) {
defer trace.AssertExpectations(t)
trace.On("Write", mock.Anything).Return(0, nil)
trace.On("IsStdout").Return(true)
trace.On("SetCancelFunc", mock.Anything).Twice()
trace.On("SetCancelFunc", mock.Anything).Once()
trace.On("SetAbortFunc", mock.Anything).Once()
trace.On("SetMasked", mock.Anything).Once()
trace.On("Fail", mock.Anything, JobExecutionTimeout).Run(func(arguments mock.Arguments) {
......
......@@ -14,11 +14,11 @@ import (
//nolint:funlen
func RunBuildWithCancel(t *testing.T, config *common.RunnerConfig, setup func(build *common.Build)) {
abortIncludeStages := []common.BuildStage{
cancelIncludeStages := []common.BuildStage{
common.BuildStagePrepare,
common.BuildStageGetSources,
}
abortExcludeStages := []common.BuildStage{
cancelExcludeStages := []common.BuildStage{
common.BuildStageRestoreCache,
common.BuildStageDownloadArtifacts,
common.BuildStageAfterScript,
......@@ -38,34 +38,25 @@ func RunBuildWithCancel(t *testing.T, config *common.RunnerConfig, setup func(bu
onUserStep: func(build *common.Build, _ common.JobTrace) {
build.SystemInterrupt <- os.Interrupt
},
includesStage: abortIncludeStages,
excludesStage: abortExcludeStages,
includesStage: cancelIncludeStages,
excludesStage: cancelExcludeStages,
expectedErr: &common.BuildError{FailureReason: common.RunnerSystemFailure},
},
"job is aborted": {
onUserStep: func(_ *common.Build, trace common.JobTrace) {
trace.Abort()
},
includesStage: abortIncludeStages,
excludesStage: abortExcludeStages,
expectedErr: &common.BuildError{FailureReason: common.JobAborted},
includesStage: cancelIncludeStages,
excludesStage: cancelExcludeStages,
expectedErr: &common.BuildError{FailureReason: common.JobCanceled},
},
"job is canceling": {
onUserStep: func(_ *common.Build, trace common.JobTrace) {
trace.Cancel()
},
includesStage: []common.BuildStage{
common.BuildStagePrepare,
common.BuildStageGetSources,
common.BuildStageAfterScript,
},
excludesStage: []common.BuildStage{
common.BuildStageRestoreCache,
common.BuildStageDownloadArtifacts,
common.BuildStageUploadOnFailureArtifacts,
common.BuildStageUploadOnSuccessArtifacts,
},
expectedErr: &common.BuildError{FailureReason: common.JobCanceled},
includesStage: cancelIncludeStages,
excludesStage: cancelExcludeStages,
expectedErr: &common.BuildError{FailureReason: common.JobCanceled},
},
}
......
......@@ -30,8 +30,6 @@ const (
JobExecutionTimeout JobFailureReason = "job_execution_timeout"
// JobCanceled is only internal to runner, and not used inside of rails.
JobCanceled JobFailureReason = "job_canceled"
// JobAborted is only internal to runner, and not used inside of rails.
JobAborted JobFailureReason = "job_aborted"
)
const (
......
......@@ -854,7 +854,7 @@ func (e *executor) startAndWatchContainer(ctx context.Context, id string, input
// Kill and wait for exit.
// Containers are stopped so that they can be reused by the job.
return e.waiter.KillWait(e.Context, id)
return e.waiter.KillWait(ctx, id)
}
func (e *executor) removeContainer(ctx context.Context, id string) error {
......
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