Add ForcedOperation.FatalError() and use to replace oft replicated

error checking sequence where possible.
hatched · Jul 10, 2019 · 93cdfaf · 93cdfaf
1 parent b3486ad
commit 93cdfaf
Show file tree

Hide file tree

Showing 7 changed files with 115 additions and 178 deletions.
diff --git a/state/application.go b/state/application.go
@@ -315,11 +315,8 @@ func (op *DestroyApplicationOperation) Done(err error) error {
 // constructed up until the error will be discarded and the error will be returned.
 func (op *DestroyApplicationOperation) destroyOps() ([]txn.Op, error) {
 	rels, err := op.app.Relations()
-	if err != nil {
-		if !op.Force {
-			return nil, err
-		}
-		op.AddError(err)
+	if op.FatalError(err) {
+		return nil, err
 	}
 	if len(rels) != op.app.doc.RelationCount {
 		// This is just an early bail out. The relations obtained may still
@@ -358,11 +355,8 @@ func (op *DestroyApplicationOperation) destroyOps() ([]txn.Op, error) {
 		return nil, op.LastError()
 	}
 	resOps, err := removeResourcesOps(op.app.st, op.app.doc.Name)
-	if err != nil {
-		if !op.Force {
-			return nil, errors.Trace(err)
-		}
-		op.AddError(err)
+	if op.FatalError(err) {
+		return nil, errors.Trace(err)
 	}
 	ops = append(ops, resOps...)
 
@@ -508,11 +502,8 @@ func (a *Application) removeOps(asserts bson.D, op *ForcedOperation) ([]txn.Op,
 
 	// Remove application offers.
 	removeOfferOps, err := removeApplicationOffersOps(a.st, a.doc.Name)
-	if err != nil {
-		if !op.Force {
-			return nil, errors.Trace(err)
-		}
-		op.AddError(err)
+	if op.FatalError(err) {
+		return nil, errors.Trace(err)
 	}
 	ops = append(ops, removeOfferOps...)
 
@@ -532,10 +523,9 @@ func (a *Application) removeOps(asserts bson.D, op *ForcedOperation) ([]txn.Op,
 			// the application is already removed, reload yourself and try again
 			return nil, errRefresh
 		}
-		if !op.Force {
+		if op.FatalError(err) {
 			return nil, errors.Trace(err)
 		}
-		op.AddError(err)
 	}
 	ops = append(ops, charmOps...)
 
@@ -2009,25 +1999,16 @@ func (a *Application) AddUnit(args AddUnitParams) (unit *Unit, err error) {
 // If the 'force' is not set, any error will be fatal and no operations will be returned.
 func (a *Application) removeUnitOps(u *Unit, asserts bson.D, op *ForcedOperation, destroyStorage bool) ([]txn.Op, error) {
 	hostOps, err := u.destroyHostOps(a, op)
-	if err != nil {
-		if !op.Force {
-			return nil, errors.Trace(err)
-		}
-		op.AddError(err)
+	if op.FatalError(err) {
+		return nil, errors.Trace(err)
 	}
 	portsOps, err := removePortsForUnitOps(a.st, u)
-	if err != nil {
-		if !op.Force {
-			return nil, errors.Trace(err)
-		}
-		op.AddError(err)
+	if op.FatalError(err) {
+		return nil, errors.Trace(err)
 	}
 	resOps, err := removeUnitResourcesOps(a.st, u.doc.Name)
-	if err != nil {
-		if !op.Force {
-			return nil, errors.Trace(err)
-		}
-		op.AddError(err)
+	if op.FatalError(err) {
+		return nil, errors.Trace(err)
 	}
 
 	observedFieldsMatch := bson.D{
@@ -2054,30 +2035,21 @@ func (a *Application) removeUnitOps(u *Unit, asserts bson.D, op *ForcedOperation
 	ops = append(ops, hostOps...)
 
 	m, err := a.st.Model()
-	if err != nil {
-		if !op.Force {
-			return nil, errors.Trace(err)
-		}
-		op.AddError(err)
+	if op.FatalError(err) {
+		return nil, errors.Trace(err)
 	} else {
 		if m.Type() == ModelTypeCAAS {
 			ops = append(ops, u.removeCloudContainerOps()...)
 		}
 	}
 
 	sb, err := NewStorageBackend(a.st)
-	if err != nil {
-		if !op.Force {
-			return nil, errors.Trace(err)
-		}
-		op.AddError(err)
+	if op.FatalError(err) {
+		return nil, errors.Trace(err)
 	} else {
 		storageInstanceOps, err := removeStorageInstancesOps(sb, u.Tag(), op.Force)
-		if err != nil {
-			if !op.Force {
-				return nil, errors.Trace(err)
-			}
-			op.AddError(err)
+		if op.FatalError(err) {
+			return nil, errors.Trace(err)
 		}
 		ops = append(ops, storageInstanceOps...)
 	}
@@ -2092,11 +2064,8 @@ func (a *Application) removeUnitOps(u *Unit, asserts bson.D, op *ForcedOperation
 		decOps, err := appCharmDecRefOps(a.st, a.doc.Name, u.doc.CharmURL, maybeDoFinal, op)
 		if errors.IsNotFound(err) {
 			return nil, errRefresh
-		} else if err != nil {
-			if !op.Force {
-				return nil, errors.Trace(err)
-			}
-			op.AddError(err)
+		} else if op.FatalError(err) {
+			return nil, errors.Trace(err)
 		}
 		ops = append(ops, decOps...)
 	}

diff --git a/state/charmref.go b/state/charmref.go
@@ -79,37 +79,28 @@ func appCharmDecRefOps(st modelBackend, appName string, curl *charm.URL, maybeDo
 	ops := []txn.Op{}
 	charmKey := charmGlobalKey(curl)
 	charmOp, err := nsRefcounts.AliveDecRefOp(refcounts, charmKey)
-	if err != nil {
-		err = errors.Annotate(err, "charm reference")
-		if !op.Force {
-			return fail(err)
-		}
-		op.AddError(err)
-	} else {
+	if op.FatalError(err) {
+		return fail(errors.Annotate(err, "charm reference"))
+	}
+	if err == nil {
 		ops = append(ops, charmOp)
 	}
 
 	settingsKey := applicationCharmConfigKey(appName, curl)
 	settingsOp, isFinal, err := nsRefcounts.DyingDecRefOp(refcounts, settingsKey)
-	if err != nil {
-		err = errors.Annotatef(err, "settings reference %s", settingsKey)
-		if !op.Force {
-			return fail(err)
-		}
-		op.AddError(err)
-	} else {
+	if op.FatalError(err) {
+		return fail(errors.Annotatef(err, "settings reference %s", settingsKey))
+	}
+	if err == nil {
 		ops = append(ops, settingsOp)
 	}
 
 	storageConstraintsKey := applicationStorageConstraintsKey(appName, curl)
 	storageConstraintsOp, _, err := nsRefcounts.DyingDecRefOp(refcounts, storageConstraintsKey)
-	if err != nil {
-		err = errors.Annotatef(err, "storage constraints reference %s", storageConstraintsKey)
-		if !op.Force {
-			return fail(err)
-		}
-		op.AddError(err)
-	} else {
+	if op.FatalError(err) {
+		return fail(errors.Annotatef(err, "storage constraints reference %s", storageConstraintsKey))
+	}
+	if err == nil {
 		ops = append(ops, storageConstraintsOp)
 	}
 

diff --git a/state/forcedoperation.go b/state/forcedoperation.go
@@ -0,0 +1,51 @@
+// Copyright 2019 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package state
+
+import "time"
+
+// ForcedOperation that allows accumulation of operational errors and
+// can be forced.
+type ForcedOperation struct {
+	// Force controls whether or not the removal of a unit
+	// will be forced, i.e. ignore operational errors.
+	Force bool
+
+	// Errors contains errors encountered while applying this operation.
+	// Generally, these are non-fatal errors that have been encountered
+	// during, say, force. They may not have prevented the operation from being
+	// aborted but the user might still want to know about them.
+	Errors []error
+
+	// MaxWait specifies the amount of time that each step in relation destroy process
+	// will wait before forcing the next step to kick-off. This parameter
+	// only makes sense in combination with 'force' set to 'true'.
+	MaxWait time.Duration
+}
+
+// AddError adds an error to the collection of errors for this operation.
+func (op *ForcedOperation) AddError(one ...error) {
+	op.Errors = append(op.Errors, one...)
+}
+
+// FatalError returns true if the err is not nil and Force is false.
+// If the error is not nil, it's added to the slice of errors for the
+// operation.
+func (op *ForcedOperation) FatalError(err error) bool {
+	if err != nil {
+		if !op.Force {
+			return true
+		}
+		op.Errors = append(op.Errors, err)
+	}
+	return false
+}
+
+// LastError returns last added error for this operation.
+func (op *ForcedOperation) LastError() error {
+	if len(op.Errors) == 0 {
+		return nil
+	}
+	return op.Errors[len(op.Errors)-1]
+}
diff --git a/state/relation.go b/state/relation.go
@@ -342,30 +342,21 @@ func (op *DestroyRelationOperation) internalDestroy() (ops []txn.Op, err error)
 	rel := &Relation{op.r.st, op.r.doc}
 
 	remoteApp, isCrossModel, err := op.r.RemoteApplication()
-	if err != nil {
-		if !op.Force {
-			return nil, errors.Trace(err)
-		}
-		op.AddError(err)
+	if op.FatalError(err) {
+		return nil, errors.Trace(err)
 	} else {
 		// If the status of the consumed app is terminated, we will never
 		// get an orderly exit of units from scope so force the issue.
 		if isCrossModel {
 			statusInfo, err := remoteApp.Status()
-			if err != nil && !errors.IsNotFound(err) {
-				if !op.Force {
-					return nil, errors.Trace(err)
-				}
-				op.AddError(err)
+			if op.FatalError(err) && !errors.IsNotFound(err) {
+				return nil, errors.Trace(err)
 			}
 			if err == nil && statusInfo.Status == status.Terminated {
 				logger.Debugf("forcing cleanup of units for %v", remoteApp.Name())
 				remoteUnits, err := rel.AllRemoteUnits(remoteApp.Name())
-				if err != nil {
-					if !op.Force {
-						return nil, errors.Trace(err)
-					}
-					op.AddError(err)
+				if op.FatalError(err) {
+					return nil, errors.Trace(err)
 				}
 				logger.Debugf("got %v relation units to clean", len(remoteUnits))
 				failRemoteUnits := false
@@ -394,11 +385,8 @@ func (op *DestroyRelationOperation) internalDestroy() (ops []txn.Op, err error)
 	destroyOps, _, err := rel.destroyOps("", &op.ForcedOperation)
 	if err == errAlreadyDying {
 		return nil, jujutxn.ErrNoOperations
-	} else if err != nil {
-		if !op.Force {
-			return nil, err
-		}
-		op.AddError(err)
+	} else if op.FatalError(err) {
+		return nil, err
 	}
 	return append(ops, destroyOps...), nil
 }

diff --git a/state/relationunit.go b/state/relationunit.go
@@ -404,12 +404,8 @@ func (op *LeaveScopeOperation) internalLeaveScope() ([]txn.Op, error) {
 	// the database is actually changed).
 	logger.Debugf("%v leaving scope", op.Description())
 	count, err := relationScopes.FindId(key).Count()
-	if err != nil {
-		err := fmt.Errorf("cannot examine scope for %s: %v", op.Description(), err)
-		if !op.Force {
-			return nil, err
-		}
-		op.AddError(err)
+	if op.FatalError(errors.Annotatef(err, "cannot examine scope for %s", op.Description())) {
+		return nil, err
 	} else if count == 0 {
 		return nil, jujutxn.ErrNoOperations
 	}
@@ -438,11 +434,8 @@ func (op *LeaveScopeOperation) internalLeaveScope() ([]txn.Op, error) {
 		// and accumulate all operational errors encountered in the operation.
 		// If the 'force' is not set, any error will be fatal and no operations will be returned.
 		relOps, err := op.ru.relation.removeOps("", op.ru.unitName, &op.ForcedOperation)
-		if err != nil {
-			if !op.Force {
-				return nil, err
-			}
-			op.AddError(err)
+		if op.FatalError(err) {
+			return nil, err
 		}
 		ops = append(ops, relOps...)
 	}

diff --git a/state/remoteapplication.go b/state/remoteapplication.go
@@ -346,11 +346,10 @@ func (op *DestroyRemoteApplicationOperation) destroyOps() (ops []txn.Op, err err
 	}
 	haveRels := true
 	rels, err := op.app.Relations()
+	if op.FatalError(err) {
+		return nil, errors.Trace(err)
+	}
 	if err != nil {
-		if !op.Force {
-			return nil, errors.Trace(err)
-		}
-		op.AddError(err)
 		haveRels = false
 	}
 
@@ -363,11 +362,8 @@ func (op *DestroyRemoteApplicationOperation) destroyOps() (ops []txn.Op, err err
 
 	// We'll need status below when processing relations.
 	statusInfo, statusErr := op.app.Status()
-	if statusErr != nil && !errors.IsNotFound(statusErr) {
-		if !op.Force {
-			return nil, statusErr
-		}
-		op.AddError(statusErr)
+	if op.FatalError(statusErr) && !errors.IsNotFound(statusErr) {
+		return nil, statusErr
 	}
 
 	removeCount := 0
@@ -426,11 +422,8 @@ func (op *DestroyRemoteApplicationOperation) destroyOps() (ops []txn.Op, err err
 	if op.app.doc.RelationCount == removeCount {
 		hasLastRefs := bson.D{{"life", Alive}, {"relationcount", removeCount}}
 		removeOps, err := op.app.removeOps(hasLastRefs)
-		if err != nil {
-			if !op.Force {
-				return nil, errors.Trace(err)
-			}
-			op.AddError(err)
+		if op.FatalError(err) {
+			return nil, errors.Trace(err)
 		}
 		ops = append(ops, removeOps...)
 		return ops, nil