Implemented service.EnsureMinUnits().

state/minimumunits.go

 // Copyright 2012, 2013 Canonical Ltd.
 // Licensed under the AGPLv3, see LICENCE file for details.
 
 package state
 
 import (
         "errors"
 
         "labix.org/v2/mgo/txn"
 
@@ skipping 104 matching lines @@
                 C:      st.minUnits.Name,
                 Id:     serviceName,
                 Remove: true,
         }
 }
 
 // MinUnits returns the minimum units count for the service.
 func (s *Service) MinUnits() int {
         return s.doc.MinUnits
 }
+
+// EnsureMinUnits adds new units if the service's MinUnits value is greater
+// than the number of alive units.
+func (s *Service) EnsureMinUnits() (err error) {
+        defer utils.ErrorContextf(&err,

[dimitern, 2013/07/11 20:05:12]

please, either put the whole statement on one line (possibly defining the error
message beforehand

[frankban, 2013/07/12 10:07:22]

I'd be happier to do this after understanding the reason for this kind of
requests. Is there a codestyle guideline for Go? Or for this project? Is
avoiding new lines when calling functions a team preference? Is it just your own
personal preference? Do I need to buy a wider screen? ;-) I believe I can learn
something here.

[dimitern, 2013/07/15 11:28:24]

Sorry, probably personal preference and also makes it easier to read, I think.
You can put the message in a variable/const before that so the line doesn't get
too long. YMMV

+                "cannot ensure minimum units for service %q", s)
+        service := &Service{st: s.st, doc: s.doc}
+        for {
+                // Ensure the service is alive.
+                if service.doc.Life != Alive {

[dimitern, 2013/07/11 20:05:12]

this should be an transaction assert op, rather than like this, relying on a
cached (possibly stale) doc field.

[frankban, 2013/07/12 10:07:22]

The assertion is already included in the transaction. The check here gives us
the opportunity to exit without hitting the db in the case the service is not
alive. In this case, the stale document is not a problem: AFAICT if a service is
dying or dead, there is nothing  that could bring it back to life.
Moreover, I may be wrong but I think it's responsibility of the caller to make
this function receive a properly refreshed/not stale service.
What do you think?

[fwereade, 2013/07/15 11:02:53]

+1 to frankban. It's annoying that we have to define asserts that duplicate the
check we do against in-memory state, but AFAICT it's necessary until someone's
smart enough to come up with an abstraction that lets us express both
requirements with the same code.

[dimitern, 2013/07/15 11:28:24]

OK, good point objection withdrawn.

+                        return errors.New("service is no longer alive")
+                }
+                // Exit without errors if the MinUnits for the service is not set.
+                if service.doc.MinUnits == 0 {

[dimitern, 2013/07/11 20:05:12]

ditto

[frankban, 2013/07/12 10:07:22]

See the comment above.

+                        return nil
+                }
+                // Retrieve the number of alive units for the service.
+                aliveUnits, err := aliveUnitsCount(service)

[dimitern, 2013/07/11 20:05:12]

ditto

[frankban, 2013/07/12 10:07:22]

See the comment above.

+                if err != nil {
+                        return err
+                }
+                // Calculate the number of required units to be added.
+                missing := service.doc.MinUnits - aliveUnits

[dimitern, 2013/07/11 20:05:12]

ditto

[frankban, 2013/07/12 10:07:22]

See the comment above.

+                if missing <= 0 {
+                        return nil
+                }
+                name, ops, err := ensureMinUnitsOps(service)
+                if err != nil {
+                        return err
+                }
+                // Add missing unit.
+                switch err := s.st.runTransaction(ops); err {
+                case nil:
+                        // Assign the new unit.
+                        unit, err := service.Unit(name)
+                        if err != nil {
+                                return err
+                        }
+                        if err := service.st.AssignUnit(unit, AssignNew); err != nil {
+                                return err
+                        }
+                        // No need to proceed and refresh the service if this was the
+                        // last/only missing unit.
+                        if missing == 1 {
+                                return nil
+                        }
+                case txn.ErrAborted:
+                default:
+                        return err

[dimitern, 2013/07/11 20:05:12]

I don't think this is right - the transaction can be aborted because of asserts
in the ops, and this have to be handled here, possibly refreshing the service
doc and retrying, rather than just returning an error.

[frankban, 2013/07/12 10:07:22]

If I read this lines correctly, this is exactly what the code does: ErrAborted
errors are never returned, the loop proceeds and the service is refreshed by the
lines you can find below. AFAIK in the Go switch/case statements the code does
not fall through by default. Am I missing something?

[fwereade, 2013/07/15 11:02:53]

frankban's right, but I agree that a casual reading is misleading. You *can*
safely put the default case wherever you like, though, IIRC:

switch err {
default:
    return err
case nil:
    // blah blah
case txn.ErrAborted:
}

...but even that is potentially surprising for some readers. Dealer's choice.

[dimitern, 2013/07/15 11:28:24]

Sorry, I read it incorrectly (that's why you shouldn't review stuff in 2am I
suppose).

+                }
+                if err := service.Refresh(); err != nil {
+                        return err
+                }
+        }
+        panic("unreachable")
+}
+
+// aliveUnitsCount returns the number a alive units for the service.
+func aliveUnitsCount(service *Service) (int, error) {
+        query := D{{"service", service.doc.Name}, {"life", Alive}}

[dimitern, 2013/07/11 20:05:12]

this needs to be an assert op, take a look at service.SetCharm for example for
more complicated op with multiple asserts.

[frankban, 2013/07/12 10:07:22]

Could you please be more specific? Why do I need an assertion when executing a
count query?

[fwereade, 2013/07/15 11:02:53]

I don't think you do, but I'm interested to hear dimitern's thinking. We haven't
made a habit of this sort of code before and he may well have spotted something
we've missed.

[dimitern, 2013/07/15 11:28:24]

See the above comment and just ignore me, sorry :/

+        return service.st.units.Find(query).Count()
+}
+
+// ensureMinUnitsOps returns the operations required to add a unit for the
+// service in MongoDB and the name for the new unit. The resulting transaction
+// will be aborted if the service document changes when running the operations.
+func ensureMinUnitsOps(service *Service) (string, []txn.Op, error) {
+        asserts := D{{"txn-revno", service.doc.TxnRevno}}
+        return service.addUnitOps("", asserts)
+}