Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
refactor: cleanup context usage in Worker.exec
- pass in the global context
- derive contexts from global context
- rename various ctx usages to clarify purpose
  • Loading branch information
cognifloyd committed Apr 28, 2022
commit 976cf795b8ce3ee57b6225421e5d551b11bfb4d4
21 changes: 11 additions & 10 deletions cmd/vela-worker/exec.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,14 @@ import (
// exec is a helper function to poll the queue
// and execute Vela pipelines for the Worker.
// nolint: nilerr // ignore returning nil - don't want to crash worker
func (w *Worker) exec(index int) error {
func (w *Worker) exec(ctx context.Context, index int) error {
var err error

// setup the version
v := version.New()

// capture an item from the queue
item, err := w.Queue.Pop(context.Background())
item, err := w.Queue.Pop(ctx)
if err != nil {
return err
}
Expand Down Expand Up @@ -93,19 +93,20 @@ func (w *Worker) exec(index int) error {
t = time.Duration(item.Repo.GetTimeout()) * time.Minute
}

// create a background context
buildCtx, done := context.WithCancel(context.Background())
// create a build context
buildCtx, done := context.WithCancel(ctx)
defer done()

// add to the background context with a timeout
// built in for ensuring a build doesn't run forever
ctx, timeout := context.WithTimeout(buildCtx, t)
timeoutCtx, timeout := context.WithTimeout(buildCtx, t)
defer timeout()

defer func() {
logger.Info("destroying build")
// destroy the build with the executor
err = _executor.DestroyBuild(context.Background())
// using buildCtx instead of timeoutCtx so that it happens after the timeout
err = _executor.DestroyBuild(buildCtx)
if err != nil {
logger.Errorf("unable to destroy build: %v", err)
}
Expand All @@ -115,15 +116,15 @@ func (w *Worker) exec(index int) error {

logger.Info("creating build")
// create the build with the executor
err = _executor.CreateBuild(ctx)
err = _executor.CreateBuild(timeoutCtx)
if err != nil {
logger.Errorf("unable to create build: %v", err)
return nil
}

logger.Info("planning build")
// plan the build with the executor
err = _executor.PlanBuild(ctx)
err = _executor.PlanBuild(timeoutCtx)
if err != nil {
logger.Errorf("unable to plan build: %v", err)
return nil
Expand All @@ -141,15 +142,15 @@ func (w *Worker) exec(index int) error {

logger.Info("assembling build")
// assemble the build with the executor
err = _executor.AssembleBuild(ctx)
err = _executor.AssembleBuild(timeoutCtx)
if err != nil {
logger.Errorf("unable to assemble build: %v", err)
return nil
}

logger.Info("executing build")
// execute the build with the executor
err = _executor.ExecBuild(ctx)
err = _executor.ExecBuild(timeoutCtx)
if err != nil {
logger.Errorf("unable to execute build: %v", err)
return nil
Expand Down
3 changes: 1 addition & 2 deletions cmd/vela-worker/operate.go
Original file line number Diff line number Diff line change
Expand Up @@ -105,8 +105,7 @@ func (w *Worker) operate(ctx context.Context) error {
return nil
default:
// exec operator subprocess to poll and execute builds
// nolint: contextcheck // ignore passing context
err = w.exec(id)
err = w.exec(gctx, id)
Copy link
Contributor

@jbrockopp jbrockopp Apr 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure we want this change?

I'm worried this will have unintended consequences when running concurrent builds on a worker.

For some background, we setup one executor thread per build on a worker.

Reviewing the docs from errgroup.WithContext:

https://pkg.go.dev/golang.org/x/sync/errgroup#WithContext

WithContext returns a new Group and an associated Context derived from ctx.

The derived Context is canceled the first time a function passed to Go returns a non-nil error or the first time Wait returns, whichever occurs first.

So I believe if w.exec() returns an error, gctx will be canceled which would stop all executor threads.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. In that case, whoever reviews this in the future (probably me once I've forgotten) needs a more descriptive comment to explain why we're not passing the context.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. I reverted this change. Now this PR only adds a few comments and renames one of the contexts for clarity.

if err != nil {
// log the error received from the executor
//
Expand Down