-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Cancel taskrun using entrypoint binary #4618
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -59,6 +59,8 @@ const ( | |
DefaultSendCloudEventsForRuns = false | ||
// DefaultEmbeddedStatus is the default value for "embedded-status". | ||
DefaultEmbeddedStatus = FullEmbeddedStatus | ||
// DefaultEnableCancelUsingEntrypoint is the default value for "enable-cancel-using-entrypoint" | ||
DefaultEnableCancelUsingEntrypoint = false | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A better name for this feature flag might be something that describes the user-facing behavior changes, e.g. "stopPodOnCancel" |
||
|
||
disableAffinityAssistantKey = "disable-affinity-assistant" | ||
disableCredsInitKey = "disable-creds-init" | ||
|
@@ -70,6 +72,7 @@ const ( | |
scopeWhenExpressionsToTask = "scope-when-expressions-to-task" | ||
sendCloudEventsForRuns = "send-cloudevents-for-runs" | ||
embeddedStatus = "embedded-status" | ||
enableCancelUsingEntrypoint = "enable-cancel-using-entrypoint" | ||
) | ||
|
||
// FeatureFlags holds the features configurations | ||
|
@@ -85,6 +88,7 @@ type FeatureFlags struct { | |
EnableAPIFields string | ||
SendCloudEventsForRuns bool | ||
EmbeddedStatus string | ||
EnableCancelUsingEntrypoint bool | ||
} | ||
|
||
// GetFeatureFlagsConfigName returns the name of the configmap containing all | ||
|
@@ -136,6 +140,9 @@ func NewFeatureFlagsFromMap(cfgMap map[string]string) (*FeatureFlags, error) { | |
if err := setEmbeddedStatus(cfgMap, DefaultEmbeddedStatus, &tc.EmbeddedStatus); err != nil { | ||
return nil, err | ||
} | ||
if err := setFeature(enableCancelUsingEntrypoint, DefaultEnableCancelUsingEntrypoint, &tc.EnableCancelUsingEntrypoint); err != nil { | ||
return nil, err | ||
} | ||
|
||
// Given that they are alpha features, Tekton Bundles and Custom Tasks should be switched on if | ||
// enable-api-fields is "alpha". If enable-api-fields is not "alpha" then fall back to the value of | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -80,12 +80,15 @@ type Entrypointer struct { | |
OnError string | ||
// StepMetadataDir is the directory for a step where the step related metadata can be stored | ||
StepMetadataDir string | ||
|
||
// CancelFile is the file that causes the task to be cancelled. | ||
CancelFile string | ||
} | ||
|
||
// Waiter encapsulates waiting for files to exist. | ||
type Waiter interface { | ||
// Wait blocks until the specified file exists. | ||
Wait(file string, expectContent bool, breakpointOnFailure bool) error | ||
Wait(ctx context.Context, file string, expectContent bool, breakpointOnFailure bool) error | ||
} | ||
|
||
// Runner encapsulates running commands. | ||
|
@@ -101,7 +104,7 @@ type PostWriter interface { | |
|
||
// Go optionally waits for a file, runs the command, and writes a | ||
// post file. | ||
func (e Entrypointer) Go() error { | ||
func (e Entrypointer) Go(ctx context.Context) error { | ||
prod, _ := zap.NewProduction() | ||
logger := prod.Sugar() | ||
|
||
|
@@ -114,7 +117,7 @@ func (e Entrypointer) Go() error { | |
}() | ||
|
||
for _, f := range e.WaitFiles { | ||
if err := e.Waiter.Wait(f, e.WaitFileContent, e.BreakpointOnFailure); err != nil { | ||
if err := e.Waiter.Wait(ctx, f, e.WaitFileContent, e.BreakpointOnFailure); err != nil { | ||
// An error happened while waiting, so we bail | ||
// *but* we write postfile to make next steps bail too. | ||
// In case of breakpoint on failure do not write post file. | ||
|
@@ -142,19 +145,46 @@ func (e Entrypointer) Go() error { | |
} | ||
|
||
if err == nil { | ||
ctx := context.Background() | ||
var cancel context.CancelFunc | ||
if e.Timeout != nil && *e.Timeout != time.Duration(0) { | ||
ctx, cancel = context.WithTimeout(ctx, *e.Timeout) | ||
defer cancel() | ||
} else { | ||
ctx, cancel = context.WithCancel(ctx) | ||
} | ||
defer cancel() | ||
|
||
errChan := make(chan error, 1) | ||
go func() { | ||
errChan <- e.Runner.Run(ctx, e.Command...) | ||
cancel() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does it mean we'll run There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for the review. Yes, we will end up calling |
||
}() | ||
|
||
var cancelled bool | ||
if e.CancelFile != "" { | ||
if err := e.Waiter.Wait(ctx, e.CancelFile, true, e.BreakpointOnFailure); err != nil { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems a bit weird to reuse Waiter.Wait for a cancel file in this way; for example "breakpointOnFailure" isn't meaningful here. Just to make sure I understand what this change is doing:
I am wondering if there's some opportunity to simplify this logic? It would be helpful to at least include a comment block explaining a bit about what this does because it's a bit hard to parse. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for the review. Indeed the logic here could benefit from some simplification. I will do another review to see if it can be simplified without making too many modifications elsewhere. |
||
return err | ||
} | ||
if ctx.Err() == nil { | ||
cancel() | ||
cancelled = true | ||
} | ||
} | ||
err = e.Runner.Run(ctx, e.Command...) | ||
err = <-errChan | ||
|
||
if err == context.DeadlineExceeded { | ||
output = append(output, v1beta1.PipelineResourceResult{ | ||
Key: "Reason", | ||
Value: "TimeoutExceeded", | ||
ResultType: v1beta1.InternalTektonResultType, | ||
}) | ||
} else if cancelled { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this is the right type of error to return-- this seems like something that should be handled by the reconciler. In particular, PipelineResourceResult doesn't seem appropriate as it's not related to pipelineresources. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @lbernick I think the name is a bit infortunate as it is this struct that we use for returning |
||
// Waiter has found the cancel file: Cancel the run | ||
cancel() | ||
output = append(output, v1beta1.PipelineResourceResult{ | ||
Key: "Reason", | ||
Value: "Cancelled", | ||
ResultType: v1beta1.InternalTektonResultType, | ||
}) | ||
} | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this value of ctx be passed to
checkForBreakpointOnFailure
, instead of creating a new one in that function?