Added comments to help clarify where operatoinal errors ar expected v…

…s database ones.
hatched · Apr 9, 2019 · d4c6765 · d4c6765
1 parent 2e4dae6
commit d4c6765
Show file tree

Hide file tree

Showing 7 changed files with 117 additions and 0 deletions.
diff --git a/state/application.go b/state/application.go
@@ -285,6 +285,11 @@ func (op *DestroyApplicationOperation) Build(attempt int) ([]txn.Op, error) {
 			return nil, err
 		}
 	}
+	// This call returns needed operations to destroy an application.
+	// All operational errors are added to 'op' struct
+	// and may be of interest to the user. Without 'force', these errors are considered fatal.
+	// If 'force' is specified, they are treated as non-fatal - they will not prevent further
+	// processing: we'll still try to remove application.
 	ops, err := op.destroyOps()
 	switch err {
 	case errRefresh:
@@ -309,6 +314,13 @@ func (op *DestroyApplicationOperation) Done(err error) error {
 // destroyOps returns the operations required to destroy the application. If it
 // returns errRefresh, the application should be refreshed and the destruction
 // operations recalculated.
+//
+// When this operation has 'force' set, all operational errors are considered non-fatal
+// and are accumulated on the operation.
+// This method will return all operations we can construct despite errors.
+//
+// When the 'force' is not set, any operational errors will be considered fatal. All operations
+// constructed up until the error will be discarded and the error will be returned.
 func (op *DestroyApplicationOperation) destroyOps() ([]txn.Op, error) {
 	if op.app.doc.Life == Dying {
 		if !op.Force {
@@ -334,6 +346,10 @@ func (op *DestroyApplicationOperation) destroyOps() ([]txn.Op, error) {
 	removeCount := 0
 	failedRels := false
 	for _, rel := range rels {
+		// When forced, this call will return both operations to remove this
+		// relation as well as all operational errors encountered.
+		// If the 'force' is not set and the call came across some errors,
+		// these errors will be fatal and no operations will be returned.
 		relOps, isRemove, opErrs, err := rel.destroyOps(op.app.doc.Name, op.Force)
 		op.AddError(opErrs...)
 		if err == errAlreadyDying {
@@ -390,6 +406,10 @@ func (op *DestroyApplicationOperation) destroyOps() ([]txn.Op, error) {
 	// removed, the application can also be removed.
 	if op.app.doc.UnitCount == 0 && op.app.doc.RelationCount == removeCount {
 		hasLastRefs := bson.D{{"life", Alive}, {"unitcount", 0}, {"relationcount", removeCount}}
+		// When forced, this call will return both operations to remove this
+		// application as well as all operational errors encountered.
+		// If the 'force' is not set and the call came across some errors,
+		// these errors will be fatal and no operations will be returned.
 		removeOps, opErrs, err := op.app.removeOps(hasLastRefs, op.Force)
 		op.AddError(opErrs...)
 		if err != nil {
@@ -463,6 +483,9 @@ func removeResourcesOps(st *State, applicationID string) ([]txn.Op, error) {
 // When force is set, the operation will proceed regardless of the errors,
 // and if any errors are encountered, all possible accumulated operations
 // as well as all encountered errors will be returned.
+// When 'force' is set, this call will return both operations to remove this
+// application as well as all operational errors encountered.
+// If the 'force' is not set, any error will be fatal and no operations will be returned.
 func (a *Application) removeOps(asserts bson.D, force bool) ([]txn.Op, []error, error) {
 	ops := []txn.Op{{
 		C:      applicationsC,
@@ -488,6 +511,9 @@ func (a *Application) removeOps(asserts bson.D, force bool) ([]txn.Op, []error,
 	// do it explicitly below.
 	name := a.doc.Name
 	curl := a.doc.CharmURL
+	// When 'force' is set, this call will return both operations to delete application references
+	// to this charm as well as all operational errors encountered.
+	// If the 'force' is not set, any error will be fatal and no operations will be returned.
 	charmOps, opErrs, err := appCharmDecRefOps(a.st, name, curl, false, force)
 	errs = append(errs, opErrs...)
 	if err != nil {
@@ -1961,6 +1987,9 @@ func (a *Application) AddUnit(args AddUnitParams) (unit *Unit, err error) {
 
 // removeUnitOps returns the operations necessary to remove the supplied unit,
 // assuming the supplied asserts apply to the unit document.
+// When 'force' is set, this call will return both needed operations
+// as well as all operational errors encountered.
+// 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, force bool) ([]txn.Op, []error, error) {
 	errs := []error{}
 	hostOps, opErrs, err := u.destroyHostOps(a, force)
@@ -2042,6 +2071,9 @@ func (a *Application) removeUnitOps(u *Unit, asserts bson.D, force bool) ([]txn.
 		// If the unit has a different URL to the application, allow any final
 		// cleanup to happen; otherwise we just do it when the app itself is removed.
 		maybeDoFinal := u.doc.CharmURL != a.doc.CharmURL
+		// When 'force' is set, this call will return both needed operations
+		// as well as all operational errors encountered.
+		// If the 'force' is not set, any error will be fatal and no operations will be returned.
 		decOps, opErrs, err := appCharmDecRefOps(a.st, a.doc.Name, u.doc.CharmURL, maybeDoFinal, force)
 		errs = append(errs, opErrs...)
 		if errors.IsNotFound(err) {
@@ -2056,6 +2088,9 @@ func (a *Application) removeUnitOps(u *Unit, asserts bson.D, force bool) ([]txn.
 	}
 	if a.doc.Life == Dying && a.doc.RelationCount == 0 && a.doc.UnitCount == 1 {
 		hasLastRef := bson.D{{"life", Dying}, {"relationcount", 0}, {"unitcount", 1}}
+		// When 'force' is set, this call will return both needed operations
+		// as well as all operational errors encountered.
+		// If the 'force' is not set, any error will be fatal and no operations will be returned.
 		removeOps, opErrs, err := a.removeOps(hasLastRef, force)
 		errs = append(errs, opErrs...)
 		if err != nil {

diff --git a/state/applicationoffers.go b/state/applicationoffers.go
@@ -225,6 +225,9 @@ func (s *applicationOffers) Remove(offerName string, force bool) (err error) {
 					}
 				}
 
+				// When 'force' is set, this call will return both needed operations
+				// as well as all operational errors encountered.
+				// If the 'force' is not set, any error will be fatal and no operations will be returned.
 				relOps, _, opErrs, err := rel.destroyOps("", force)
 				if len(opErrs) != 0 {
 					logger.Warningf("errors while getting operations to destroy remote application relation %v: %v", remoteApp.Name(), opErrs)

diff --git a/state/charmref.go b/state/charmref.go
@@ -66,6 +66,9 @@ func appCharmIncRefOps(mb modelBackend, appName string, curl *charm.URL, canCrea
 // storage constraints documents for that pair, and schedule a cleanup
 // to see if the charm itself is now unreferenced and can be tidied
 // away itself.
+// When 'force' is set, this call will return both needed operations
+// as well as all operational errors encountered.
+// If the 'force' is not set, any error will be fatal and no operations will be returned.
 func appCharmDecRefOps(st modelBackend, appName string, curl *charm.URL, maybeDoFinal, force bool) ([]txn.Op, []error, error) {
 	refcounts, closer := st.db().GetCollection(refcountsC)
 	defer closer()

diff --git a/state/relation.go b/state/relation.go
@@ -263,6 +263,13 @@ func (r *Relation) Destroy() error {
 	return err
 }
 
+// When 'force' is set, this call will construct and apply needed operations
+// as well as accumulate all operational errors encountered.
+// If the 'force' is not set, any error will be fatal and no operations will be applied.
+// TODO (anastasiamac) This needs to be changed to be an Operation
+// similar to Application and Unit DestroyOperation. This will stop
+// the confusion around what errors are operational, business logic related
+// and which are database errors.
 func (r *Relation) internalDestroy(force bool) (errs []error, err error) {
 	defer errors.DeferredAnnotatef(&err, "cannot destroy relation %q", r)
 	if len(r.doc.Endpoints) == 1 && r.doc.Endpoints[0].Role == charm.RolePeer {
@@ -332,6 +339,9 @@ func (r *Relation) internalDestroy(force bool) (errs []error, err error) {
 				return nil, err
 			}
 		}
+		// When 'force' is set, this call will return both needed operations
+		// as well as all operational errors encountered.
+		// If the 'force' is not set, any error will be fatal and no operations will be returned.
 		ops, _, opErrs, err := rel.destroyOps("", force)
 		errs = append(errs, opErrs...)
 		if err == errAlreadyDying {
@@ -359,13 +369,19 @@ func (r *Relation) internalDestroy(force bool) (errs []error, err error) {
 // operations may include changes to the relation's applications; however, if
 // ignoreApplication is not empty, no operations modifying that application will
 // be generated.
+// When 'force' is set, this call will return both operations to remove this
+// relation as well as all operational errors encountered.
+// If the 'force' is not set, any error will be fatal and no operations will be returned.
 func (r *Relation) destroyOps(ignoreApplication string, force bool) (ops []txn.Op, isRemove bool, opErrs []error, err error) {
 	if r.doc.Life != Alive {
 		if !force {
 			return nil, false, nil, errAlreadyDying
 		}
 	}
 	if r.doc.UnitCount == 0 {
+		// When 'force' is set, this call will return both needed operations
+		// as well as all operational errors encountered.
+		// If the 'force' is not set, any error will be fatal and no operations will be returned.
 		removeOps, opErrs, err := r.removeOps(ignoreApplication, "", force)
 		if err != nil {
 			if !force {
@@ -388,6 +404,9 @@ func (r *Relation) destroyOps(ignoreApplication string, force bool) (ops []txn.O
 // included; if departingUnitName is non-empty, this implies that the
 // relation's applications may be Dying and otherwise unreferenced, and may thus
 // require removal themselves.
+// When 'force' is set, this call will return both needed operations
+// as well as all operational errors encountered.
+// If the 'force' is not set, any error will be fatal and no operations will be returned.
 func (r *Relation) removeOps(ignoreApplication string, departingUnitName string, force bool) ([]txn.Op, []error, error) {
 	relOp := txn.Op{
 		C:      relationsC,
@@ -416,6 +435,9 @@ func (r *Relation) removeOps(ignoreApplication string, departingUnitName string,
 				}
 				ops = append(ops, epOps...)
 			} else {
+				// When 'force' is set, this call will return both needed operations
+				// as well as all operational errors encountered.
+				// If the 'force' is not set, any error will be fatal and no operations will be returned.
 				epOps, opErrs, err := r.removeLocalEndpointOps(ep, departingUnitName, force)
 				errs = append(errs, opErrs...)
 				if err != nil {
@@ -437,6 +459,9 @@ func (r *Relation) removeOps(ignoreApplication string, departingUnitName string,
 	return append(ops, cleanupOp), errs, nil
 }
 
+// When 'force' is set, this call will return both needed operations
+// as well as all operational errors encountered.
+// If the 'force' is not set, any error will be fatal and no operations will be returned.
 func (r *Relation) removeLocalEndpointOps(ep Endpoint, departingUnitName string, force bool) ([]txn.Op, []error, error) {
 	var asserts bson.D
 	hasRelation := bson.D{{"relationcount", bson.D{{"$gt", 0}}}}
@@ -463,6 +488,9 @@ func (r *Relation) removeLocalEndpointOps(ep Endpoint, departingUnitName string,
 		hasLastRef := bson.D{{"life", Dying}, {"unitcount", 0}, {"relationcount", 1}}
 		removable := append(bson.D{{"_id", ep.ApplicationName}}, hasLastRef...)
 		if err := applications.Find(removable).One(&app.doc); err == nil {
+			// When 'force' is set, this call will return both needed operations
+			// as well as all operational errors encountered.
+			// If the 'force' is not set, any error will be fatal and no operations will be returned.
 			return app.removeOps(hasLastRef, force)
 		} else if err != mgo.ErrNotFound {
 			if !force {

diff --git a/state/relationunit.go b/state/relationunit.go
@@ -274,6 +274,9 @@ func (ru *RelationUnit) PrepareLeaveScope() error {
 // LeaveScopeWithForce in addition to doing what LeaveScope() does,
 // when force is passed in as 'true', forces relation unit to leave scope,
 // ignoring errors.
+// TODO (anastasiamac) Need to consider to do this an Operation,
+// similar to Unit and Apllication DestroyOperation to better differentiate between
+// business logic and opeational errors and database errors.
 func (ru *RelationUnit) LeaveScopeWithForce(force bool) ([]error, error) {
 	return ru.internalLeaveScope(force)
 }
@@ -288,6 +291,9 @@ func (ru *RelationUnit) LeaveScope() error {
 	return err
 }
 
+// When 'force' is set, this call will construct and apply needed operations
+// and return all operational errors encountered.
+// If the 'force' is not set, any error will be fatal and no operations will be applied.
 func (ru *RelationUnit) internalLeaveScope(force bool) ([]error, error) {
 	relationScopes, closer := ru.st.db().GetCollection(relationScopesC)
 	defer closer()
@@ -357,6 +363,9 @@ func (ru *RelationUnit) internalLeaveScope(force bool) ([]error, error) {
 				Update: bson.D{{"$inc", bson.D{{"unitcount", -1}}}},
 			})
 		} else {
+			// When 'force' is set, this call will return both needed operations
+			// as well as all operational errors encountered.
+			// If the 'force' is not set, any error will be fatal and no operations will be returned.
 			relOps, opErrs, err := ru.relation.removeOps("", ru.unitName, force)
 			errs = append(errs, opErrs...)
 			if err != nil {

diff --git a/state/remoteapplication.go b/state/remoteapplication.go
@@ -251,6 +251,10 @@ func copyAttributes(values attributeMap) attributeMap {
 // Destroy in addition to doing what Destroy() does,
 // when force is passed in as 'true', forces th destruction of remote application,
 // ignoring errors.
+// TODO (anastasiamac) This needs to be converted to an Operation
+// similar to Unit and Application DestroyOperation to better separate logic
+// of building operations and related errors from the database errors of
+// applying these operations.
 func (s *RemoteApplication) DestroyWithForce(force bool) ([]error, error) {
 	return s.internalDestroy(force)
 }
@@ -263,6 +267,9 @@ func (s *RemoteApplication) Destroy() error {
 	return err
 }
 
+// When 'force' is set, this call will construct and apply needed operations
+// as well as return all operational errors encountered.
+// If the 'force' is not set, any error will be fatal and no operations will be applied.
 func (s *RemoteApplication) internalDestroy(force bool) (errs []error, err error) {
 	defer errors.DeferredAnnotatef(&err, "cannot destroy remote application %q", s)
 	defer func() {
@@ -279,6 +286,9 @@ func (s *RemoteApplication) internalDestroy(force bool) (errs []error, err error
 				return nil, err
 			}
 		}
+		// When 'force' is set, this call will return both needed operations
+		// as well as all operational errors encountered.
+		// If the 'force' is not set, any error will be fatal and no operations will be returned.
 		ops, opErrs, err := app.destroyOps(force)
 		errs = append(errs, opErrs...)
 		switch err {
@@ -304,6 +314,9 @@ func (s *RemoteApplication) internalDestroy(force bool) (errs []error, err error
 // destroyOps returns the operations required to destroy the application. If it
 // returns errRefresh, the application should be refreshed and the destruction
 // operations recalculated.
+// When 'force' is set, this call will return both needed operations
+// as well as all operational errors encountered.
+// If the 'force' is not set, any error will be fatal and no operations will be returned.
 func (s *RemoteApplication) destroyOps(force bool) ([]txn.Op, []error, error) {
 	if s.doc.Life == Dying {
 		if !force {
@@ -355,6 +368,7 @@ func (s *RemoteApplication) destroyOps(force bool) ([]txn.Op, []error, error) {
 				if countRemoteUnits := len(remoteUnits); countRemoteUnits != 0 {
 					logger.Debugf("got %v relation units to clean", countRemoteUnits)
 					for _, ru := range remoteUnits {
+						// This call will not construct operations but will try to leave scope immediately.
 						opErrs, err := ru.LeaveScopeWithForce(force)
 						errs = append(errs, opErrs...)
 						if err != nil {
@@ -366,6 +380,9 @@ func (s *RemoteApplication) destroyOps(force bool) ([]txn.Op, []error, error) {
 				}
 			}
 
+			// When 'force' is set, this call will return both needed operations
+			// as well as all operational errors encountered.
+			// If the 'force' is not set, any error will be fatal and no operations will be returned.
 			relOps, isRemove, opErrs, err := rel.destroyOps(s.doc.Name, force)
 			errs = append(errs, opErrs...)
 			if err == errAlreadyDying {