Skip to content

Commit

Permalink
pkg/cvo/sync_worker: Generalize CancelError to ContextError
Browse files Browse the repository at this point in the history
With this commit, I drop contextIsCancelled in favor of Context.Err().
From the docs [1]:

  If Done is not yet closed, Err returns nil.  If Done is closed, Err
  returns a non-nil error explaining why: Canceled if the context
  was canceled or DeadlineExceeded if the context's deadline
  passed.  After Err returns a non-nil error, successive calls to
  Err return the same error.

I dunno why we'd been checking Done() instead, but contextIsCancelled
dates back to 961873d (sync: Do config syncing in the background,
2019-01-11, openshift#82).

I've also generalized a number of *Cancel* helpers to be *Context* to
remind folks that Context.Err() can be DeadlineExceeded as well as
Canceled, and the CVO uses both WithCancel and WithTimeout.  The new
error messages will be either:

  update context deadline exceeded at 1 of 2

or:

  update context canceled at 1 of 2

Instead of always claiming:

  update was cancelled at 1 of 2

Cherry-picked from eea2092 (pkg/cvo/sync_worker: Generalize
CancelError to ContextError, 2020-05-28, openshift#378) and edited to resolve
context conflicts because release-4.4 lacks 2a469e3 (cvo: When
installing or upgrading, fast-fill cluster-operators, 2020-02-07, openshift#318).

[1]: https://golang.org/pkg/context/#Context
  • Loading branch information
wking committed Jun 3, 2020
1 parent 18b995d commit f8cd27b
Showing 1 changed file with 14 additions and 23 deletions.
37 changes: 14 additions & 23 deletions pkg/cvo/sync_worker.go
Original file line number Diff line number Diff line change
Expand Up @@ -613,8 +613,8 @@ func (w *SyncWorker) apply(ctx context.Context, payloadUpdate *payload.Update, w
// update each object
errs := payload.RunGraph(ctx, graph, maxWorkers, func(ctx context.Context, tasks []*payload.Task) error {
for _, task := range tasks {
if contextIsCancelled(ctx) {
return cr.CancelError()
if err := ctx.Err(); err != nil {
return cr.ContextError(err)
}
cr.Update()

Expand Down Expand Up @@ -658,11 +658,11 @@ func init() {
)
}

type errCanceled struct {
type errContext struct {
err error
}

func (e errCanceled) Error() string { return e.err.Error() }
func (e errContext) Error() string { return e.err.Error() }

// consistentReporter hides the details of calculating the status based on the progress
// of the graph runner.
Expand Down Expand Up @@ -699,7 +699,7 @@ func (r *consistentReporter) Error(err error) {
copied := r.status
copied.Step = "ApplyResources"
copied.Fraction = float32(r.done) / float32(r.total)
if !isCancelledError(err) {
if !isContextError(err) {
copied.Failure = err
}
r.reporter.Report(copied)
Expand All @@ -720,10 +720,10 @@ func (r *consistentReporter) Errors(errs []error) error {
return err
}

func (r *consistentReporter) CancelError() error {
func (r *consistentReporter) ContextError(err error) error {
r.lock.Lock()
defer r.lock.Unlock()
return errCanceled{fmt.Errorf("update was cancelled at %d of %d", r.done, r.total)}
return errContext{fmt.Errorf("update %s at %d of %d", err, r.done, r.total)}
}

func (r *consistentReporter) Complete() {
Expand All @@ -739,11 +739,11 @@ func (r *consistentReporter) Complete() {
r.reporter.Report(copied)
}

func isCancelledError(err error) bool {
func isContextError(err error) bool {
if err == nil {
return false
}
_, ok := err.(errCanceled)
_, ok := err.(errContext)
return ok
}

Expand All @@ -764,11 +764,12 @@ func isImageVerificationError(err error) bool {
// not truly an error (cancellation).
// TODO: take into account install vs upgrade
func summarizeTaskGraphErrors(errs []error) error {
// we ignore cancellation errors since they don't provide good feedback to users and are an internal
// detail of the server
err := errors.FilterOut(errors.NewAggregate(errs), isCancelledError)
// we ignore context errors (canceled or timed out) since they don't
// provide good feedback to users and are an internal detail of the
// server
err := errors.FilterOut(errors.NewAggregate(errs), isContextError)
if err == nil {
klog.V(4).Infof("All errors were cancellation errors: %v", errs)
klog.V(4).Infof("All errors were context errors: %v", errs)
return nil
}
agg, ok := err.(errors.Aggregate)
Expand Down Expand Up @@ -939,16 +940,6 @@ func ownerRefModifier(config *configv1.ClusterVersion) resourcebuilder.MetaV1Obj
}
}

// contextIsCancelled returns true if the provided context is cancelled.
func contextIsCancelled(ctx context.Context) bool {
select {
case <-ctx.Done():
return true
default:
return false
}
}

// runThrottledStatusNotifier invokes fn every time ch is updated, but no more often than once
// every interval. If bucket is non-zero then the channel is throttled like a rate limiter bucket.
func runThrottledStatusNotifier(stopCh <-chan struct{}, interval time.Duration, bucket int, ch <-chan SyncWorkerStatus, fn func()) {
Expand Down

0 comments on commit f8cd27b

Please sign in to comment.