New charm revision updater api

New charm revision updater api

Add an api which provides functionality to query the
charm store and record in state the latest revision
for the charms. The charms queried are those which
are currently deployed. A charm record is created
and is marked as a placeholder. This makes the charm
invisible to state.Charm() queries. If the charm
is subsequently deployed and thus state.AddCharm()
is called, the placeholder record is updated accordngly.

The next step is to add a worker which
will periodically call this api.

https://code.launchpad.net/~wallyworld/juju-core/charm-version-updater/+merge/200758

Requires: https://code.launchpad.net/~wallyworld/juju-core/refactor-charm-repo/+merge/200753

(do not edit description out of merge proposal)

[wallyworld, 2014-01-08 02:19:30]

Please take a look.

[william.reade, 2014-01-13 09:10:13]

Not so sure about the data, or about where we're storing it. Chat after standup?

https://codereview.appspot.com/48880043/diff/1/state/apiserver/charmversionup...
File state/apiserver/charmversionupdater/updater.go (right):

https://codereview.appspot.com/48880043/diff/1/state/apiserver/charmversionup...
state/apiserver/charmversionupdater/updater.go:148:
context.collateDeployedUnitRevisions(service.Name())
I'm a bit uncertain about this. Why don't we collect the revisions of deployed
subordinates? They function just the same as do principals.

https://codereview.appspot.com/48880043/diff/1/state/apiserver/charmversionup...
state/apiserver/charmversionupdater/updater.go:157:
context.serviceRevisions[serviceName].unitVersions[unit.Name()] =
charmRevision{revision: url.Revision}
This isn't quite enough. You can plausibly upgrade from cs:precise/mysql-33 to
local:precise/mysql-33, and so a useful unit upgrade status needs the whole
charm url, not just the revision.

https://codereview.appspot.com/48880043/diff/1/state/apiserver/charmversionup...
state/apiserver/charmversionupdater/updater.go:174: if
repoCharmRevision.revision == 0 {
0 is a valid charm revision..?

https://codereview.appspot.com/48880043/diff/1/state/apiserver/charmversionup...
state/apiserver/charmversionupdater/updater.go:185: setServiceRevision:
if repoCharmRevision.revision != -1 { // (or whatever the actual value we're
checking for is)
    // blah blah
}
// no goto!
if err := s.SetRevisionStatus(...

https://codereview.appspot.com/48880043/diff/1/state/apiserver/charmversionup...
state/apiserver/charmversionupdater/updater.go:198: // Only report if unit
revision is known, and is different to service revision, and is out of date.
else if, no goto?

https://codereview.appspot.com/48880043/diff/1/state/apiserver/charmversionup...
state/apiserver/charmversionupdater/updater.go:220: // a path to the local repo.
This may change if the need arises.
Maybe add a note/TODO pointing out that for local repos we could usefully track
services with different revisions of the same charm, and offer to upgrade those
running (say) local:precise/foo-22 to the latest one in state, ie (say)
local:precise/foo-26.

Did that make sense?

https://codereview.appspot.com/48880043/diff/1/state/apiserver/root.go
File state/apiserver/root.go (right):

https://codereview.appspot.com/48880043/diff/1/state/apiserver/root.go#newcod...
state/apiserver/root.go:211: return nil, common.ErrBadId
So, we should not be doing this. If we're reserving that param for future use,
then erroring out if someone provides it just causes an unnecessary burden when
updating to a version that uses it.

https://codereview.appspot.com/48880043/diff/1/state/service.go
File state/service.go (right):

https://codereview.appspot.com/48880043/diff/1/state/service.go#newcode515
state/service.go:515: return s.doc.RevisionStatus
I don't really like having this in the service doc. Any revision status change
will trigger all the service watchers, most of which only care about Life.

A thought: we do currently have "incomplete" charms in the db. Would it be
reasonable to have the updater look up the most recent versions of the charms,
and then add references to those in state? That way we can grab (1) a list of
all services and their charms and (2) a list of all known charm urls, and
correlate the two on demand? As a bonus, that'd give us the behaviour I wanted
for local charms across the board automatically. Should we chat about this?
Dimitern might have useful input.

https://codereview.appspot.com/48880043/diff/1/state/service.go#newcode519
state/service.go:519: func (s *Service) SetRevisionStatus(status string) (err
error) {
Hmm. I'd rather have a type other than plain string, I think, lack of structure
makes me nervous.

https://codereview.appspot.com/48880043/diff/1/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/48880043/diff/1/state/unit.go#newcode160
state/unit.go:160: func (u *Unit) SetRevisionStatus(status string) (err error) {
Similarly, I'd prefer more structure -- but equally, I don't see what good the
unit revision statuses do us. The important thing to know is surely "is this
unit's curl different from its service's?" -- just as for a service it's "is
there a newer revision of this service's charm available" -- and neither of
those things implies the need for a whole new field.

[axw, 2014-01-13 09:26:29]

https://codereview.appspot.com/48880043/diff/1/state/api/charmversionupdater/...
File state/api/charmversionupdater/updater.go (right):

https://codereview.appspot.com/48880043/diff/1/state/api/charmversionupdater/...
state/api/charmversionupdater/updater.go:29: return result.Error
if result.Error != nil ...

(I know you've fixed it further down the pipe, just FTR)

https://codereview.appspot.com/48880043/diff/1/state/apiserver/charmversionup...
File state/apiserver/charmversionupdater/testing/suite.go (right):

https://codereview.appspot.com/48880043/diff/1/state/apiserver/charmversionup...
state/apiserver/charmversionupdater/testing/suite.go:25: server
*charmtesting.MockStore
If you exposed server, then you could test the UUID header?
https://codereview.appspot.com/50330043/patch/1/10007

https://codereview.appspot.com/48880043/diff/1/state/apiserver/charmversionup...
File state/apiserver/charmversionupdater/updater.go (right):

https://codereview.appspot.com/48880043/diff/1/state/apiserver/charmversionup...
state/apiserver/charmversionupdater/updater.go:108: unitVersions
map[string]charmRevision
Is it really necessary to have the rev status on units as well as services?
Isn't a unit's rev only different from its service when it's pending (but not
completed) upgrade? Is it really useful to know the current rev in that case?

https://codereview.appspot.com/48880043/diff/1/state/apiserver/charmversionup...
state/apiserver/charmversionupdater/updater.go:169: if repoCharmRevision.err !=
nil {
IMO a chain of if-else-if would be better here: simpler control flow than mixing
if and goto statements.

https://codereview.appspot.com/48880043/diff/1/state/apiserver/charmversionup...
state/apiserver/charmversionupdater/updater.go:183: serviceStatus =
fmt.Sprintf("out of date (available: %d)", repoCharmRevision.revision)
Do we really want to store the status, and not the raw data (what's current,
what's available)? This makes it difficult to use other than for presentation;
presentation is made difficult if we need to do i18n.

Would it be terrible to store the current and available in state, and then do
this presentation in "juju status"?

[wallyworld, 2014-01-15 07:16:56]

Please take a look.

[wallyworld, 2014-01-15 23:21:45]

Please take a look.

[jameinel, 2014-01-16 09:56:15]

Everything I've seen so far looks pretty good, but I didn't get to the
'state.go' files to give a final approve. I'll see if I can finish the review
later tonight.

https://codereview.appspot.com/48880043/diff/40001/state/api/charmrevisionupd...
File state/api/charmrevisionupdater/updater_test.go (right):

https://codereview.appspot.com/48880043/diff/40001/state/api/charmrevisionupd...
state/api/charmrevisionupdater/updater_test.go:48: pending, err :=
s.State.LatestPlaceholderCharm(curl)
Just to confirm, this isn't actually going to the real charm store, right?

https://codereview.appspot.com/48880043/diff/40001/state/apiserver/charmrevis...
File state/apiserver/charmrevisionupdater/updater.go (right):

https://codereview.appspot.com/48880043/diff/40001/state/apiserver/charmrevis...
state/apiserver/charmrevisionupdater/updater.go:80:
deployedCharms[baseCharm.String()] = baseCharm
This breaks if you have the same charm deployed to two differently named
services, right?

juju deploy mysql mysqlA
juju deploy mysql mysqlB

It may be that WithRevision(-1) will be the same even if mysqlA and mysqlB are
at different revs.

I'd just like to confirm that. Maybe test worthy?

[wallyworld, 2014-01-16 10:11:46]

https://codereview.appspot.com/48880043/diff/40001/state/api/charmrevisionupd...
File state/api/charmrevisionupdater/updater_test.go (right):

https://codereview.appspot.com/48880043/diff/40001/state/api/charmrevisionupd...
state/api/charmrevisionupdater/updater_test.go:48: pending, err :=
s.State.LatestPlaceholderCharm(curl)
On 2014/01/16 09:56:16, jameinel wrote:
> Just to confirm, this isn't actually going to the real charm store, right?

Right, LatestPlaceholderCharm only queries the db. Only the worker task goes to
the charm store, and it updates the db and hence allows this
LatestPlaceholderCharm call to function.

https://codereview.appspot.com/48880043/diff/40001/state/apiserver/charmrevis...
File state/apiserver/charmrevisionupdater/updater.go (right):

https://codereview.appspot.com/48880043/diff/40001/state/apiserver/charmrevis...
state/apiserver/charmrevisionupdater/updater.go:80:
deployedCharms[baseCharm.String()] = baseCharm
On 2014/01/16 09:56:16, jameinel wrote:
> This breaks if you have the same charm deployed to two differently named
> services, right?
> 
> juju deploy mysql mysqlA
> juju deploy mysql mysqlB
> 
> It may be that WithRevision(-1) will be the same even if mysqlA and mysqlB are
> at different revs.
> 

Yes, that's the intent. See below.

> I'd just like to confirm that. Maybe test worthy?

The way this works is that deployedCharms is used to hold the latest revisions
from the charm store, as queried from the db. These latest revisions are then
compared against each service. So mysqlA and B may well have cs:mysql-3 and
cs:mysql-4. deployedCharms might have cs:mysql5 (since that's what the store
has). So the status will compare cs:mysql-3 and cs:mysql-4 to cs:mysql-5 and
present accordingly.

[thumper, 2014-01-16 22:34:35]

On 2014/01/16 10:11:46, wallyworld wrote:
>
https://codereview.appspot.com/48880043/diff/40001/state/api/charmrevisionupd...
> File state/api/charmrevisionupdater/updater_test.go (right):
> 
>
https://codereview.appspot.com/48880043/diff/40001/state/api/charmrevisionupd...
> state/api/charmrevisionupdater/updater_test.go:48: pending, err :=
> s.State.LatestPlaceholderCharm(curl)
> On 2014/01/16 09:56:16, jameinel wrote:
> > Just to confirm, this isn't actually going to the real charm store, right?
> 
> Right, LatestPlaceholderCharm only queries the db. Only the worker task goes
to
> the charm store, and it updates the db and hence allows this
> LatestPlaceholderCharm call to function.
> 
>
https://codereview.appspot.com/48880043/diff/40001/state/apiserver/charmrevis...
> File state/apiserver/charmrevisionupdater/updater.go (right):
> 
>
https://codereview.appspot.com/48880043/diff/40001/state/apiserver/charmrevis...
> state/apiserver/charmrevisionupdater/updater.go:80:
> deployedCharms[baseCharm.String()] = baseCharm
> On 2014/01/16 09:56:16, jameinel wrote:
> > This breaks if you have the same charm deployed to two differently named
> > services, right?
> > 
> > juju deploy mysql mysqlA
> > juju deploy mysql mysqlB
> > 
> > It may be that WithRevision(-1) will be the same even if mysqlA and mysqlB
are
> > at different revs.
> > 
> 
> Yes, that's the intent. See below.
> 
> > I'd just like to confirm that. Maybe test worthy?
> 
> The way this works is that deployedCharms is used to hold the latest revisions
> from the charm store, as queried from the db. These latest revisions are then
> compared against each service. So mysqlA and B may well have cs:mysql-3 and
> cs:mysql-4. deployedCharms might have cs:mysql5 (since that's what the store
> has). So the status will compare cs:mysql-3 and cs:mysql-4 to cs:mysql-5 and
> present accordingly.

LGTM, and thanks for addressing all the comments.