state: Prepare/UpdateUploadedCharm operations

state: Prepare/UpdateUploadedCharm operations

This adds a new field to state.Charm's underlying
document: PendingUpload (false by default) and
two new state methods: PrepareLocalCharmUpload
and UpdateUploadedCharm, which are intended
to be called before and after the upcoming
charm upload support in the API server.

When a charm is uploaded, a unique revision
for it must be reserved in state, before the
actual upload to provider storage happens.
The revision is chosen as max(charmRev, stateRev+1),
where charmRev is the revision in charm URL
given to PrepareLocalCharmUpload() and stateRev
is the highest existing revision of the same
charm in state.

PendingUpload is false by default and can only
be changed by using the Prepare/Update methods.
Its inverse value is exposed through IsUploaded()
method of state.Charm.

PrepareLocalCharmUpload requires a charm URL
with a revision and local schema, and returns
the charm URL that was added to state.

UpdateUploadedCharm takes the same arguments
like AddCharm and returns the same as well.
It takes care of ensuring the given charm
URL refers to a charm in pending state and
updates its other info (meta, config, bundle
URL and SHA256), reseting the pending flag
to false.

https://code.launchpad.net/~dimitern/juju-core/219-state-charm-upload/+merge/198405

(do not edit description out of merge proposal)

[dimitern, 2013-12-10 14:26:10]

Please take a look.

[rog, 2013-12-10 16:32:31]

This looks great. Some comments and suggestions below.

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

https://codereview.appspot.com/40160043/diff/1/state/state.go#newcode459
state/state.go:459: // PrepareCharmUpload must be called before a charm is
uploaded to the
Since it's limited to local charms, I suggest we reflect
that in the name, e.g. PrepareLocalCharmUpload.

https://codereview.appspot.com/40160043/diff/1/state/state.go#newcode462
state/state.go:462: // 1. Finds all existing charm documents with the same URL
prefix (any
A lot of this seems more like internal documentation and should
perhaps live more happily inside the function.

I'd perhaps say something like:

// PrepareLocalCharmUpload must be called before a charm is uploaded to the
// provider storage in order to create a charm document in state.
// It returns a new Charm with no metadata and a unique revision number.
//
// The url's schema must be "local".

One thought: why do we return a *Charm when it can't actually be used for
anything? We could just return the new URL.

Alternatively, we could leverage the returned charm by implementing a
Charm.Uploaded method instead of having UpdateUploadedCharm (that's what
I was expecting to find, actually, when I first saw that this method returned
a Charm). It would save reading the charm doc again too.

https://codereview.appspot.com/40160043/diff/1/state/state.go#newcode476
state/state.go:476: if curl == nil {
I think it's reasonable to lose this test and just panic
if we pass in a nil url.

In general we don't test parameters for nil-ness (we don't check
that st != nil for example).

https://codereview.appspot.com/40160043/diff/1/state/state.go#newcode482
state/state.go:482: if curl.Revision < 0 {
Does it really matter if it's not set?

https://codereview.appspot.com/40160043/diff/1/state/state.go#newcode485
state/state.go:485: // Get a regex with the charm URL and no revision.
Rather than the string manipulation, we could do:

noRevURL := *curl
noRevURL.Revision = -1
curlRegex := "^" + regexp.QuoteMeta(noRevURL.String())

https://codereview.appspot.com/40160043/diff/1/state/state.go#newcode497
state/state.go:497: docs := []charmDoc{}
var docs []charmDoc

?

https://codereview.appspot.com/40160043/diff/1/state/state.go#newcode498
state/state.go:498: err = st.charms.Find(D{{"_id", D{{"$regex",
curlRegex}}}}).All(&docs)
I think we should select the _id field so we can avoid pulling all the metadata
over the wire.

https://codereview.appspot.com/40160043/diff/1/state/state.go#newcode510
state/state.go:510: chosenRevision := -1
or:

chosenRevision := curl.Revision
if maxRevision > chosenRevision {
    // More recent revision exists in state, pick the next.
    chosenRevision = maxRevision + 1
}

?

https://codereview.appspot.com/40160043/diff/1/state/state.go#newcode554
state/state.go:554: if doc.PendingUpload != true {
!doc.PendingUpload

https://codereview.appspot.com/40160043/diff/1/state/state.go#newcode557
state/state.go:557: for attempt := 0; attempt < 3; attempt++ {
I don't think there's any point in retrying here, is there?
PendingUpload can never be set to true once it's false,
can it?

[dimitern, 2013-12-11 09:44:18]

Please take a look.

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

https://codereview.appspot.com/40160043/diff/1/state/state.go#newcode459
state/state.go:459: // PrepareCharmUpload must be called before a charm is
uploaded to the
On 2013/12/10 16:32:31, rog wrote:
> Since it's limited to local charms, I suggest we reflect
> that in the name, e.g. PrepareLocalCharmUpload.

Done.

https://codereview.appspot.com/40160043/diff/1/state/state.go#newcode462
state/state.go:462: // 1. Finds all existing charm documents with the same URL
prefix (any
On 2013/12/10 16:32:31, rog wrote:
> A lot of this seems more like internal documentation and should
> perhaps live more happily inside the function.
> 
> I'd perhaps say something like:
> 
> // PrepareLocalCharmUpload must be called before a charm is uploaded to the
> // provider storage in order to create a charm document in state.
> // It returns a new Charm with no metadata and a unique revision number.
> //
> // The url's schema must be "local".
> 
> One thought: why do we return a *Charm when it can't actually be used for
> anything? We could just return the new URL.
> 
> Alternatively, we could leverage the returned charm by implementing a
> Charm.Uploaded method instead of having UpdateUploadedCharm (that's what
> I was expecting to find, actually, when I first saw that this method returned
> a Charm). It would save reading the charm doc again too.

Updated the comments and changed the return value to be *charm.URL.
The point of returning a *state.Charm instead of *charm.URL was to verify we
don't touch anything else than the charm url in the doc when added, but this can
be tested separately.

https://codereview.appspot.com/40160043/diff/1/state/state.go#newcode476
state/state.go:476: if curl == nil {
On 2013/12/10 16:32:31, rog wrote:
> I think it's reasonable to lose this test and just panic
> if we pass in a nil url.
> 
> In general we don't test parameters for nil-ness (we don't check
> that st != nil for example).

Done.

https://codereview.appspot.com/40160043/diff/1/state/state.go#newcode482
state/state.go:482: if curl.Revision < 0 {
On 2013/12/10 16:32:31, rog wrote:
> Does it really matter if it's not set?

It matters, because we want a complete URL, including revision. If no revision
is set the logic below to pick a revision will only select a state revision. And
as we agreed, the local charm revision should be respected and thus present in
the URL given here.

https://codereview.appspot.com/40160043/diff/1/state/state.go#newcode485
state/state.go:485: // Get a regex with the charm URL and no revision.
On 2013/12/10 16:32:31, rog wrote:
> Rather than the string manipulation, we could do:
> 
> noRevURL := *curl
> noRevURL.Revision = -1
> curlRegex := "^" + regexp.QuoteMeta(noRevURL.String())

Or even noRevURL := curl.WithRevision(-1).

https://codereview.appspot.com/40160043/diff/1/state/state.go#newcode497
state/state.go:497: docs := []charmDoc{}
On 2013/12/10 16:32:31, rog wrote:
> var docs []charmDoc
> 
> ?

Done.

https://codereview.appspot.com/40160043/diff/1/state/state.go#newcode498
state/state.go:498: err = st.charms.Find(D{{"_id", D{{"$regex",
curlRegex}}}}).All(&docs)
On 2013/12/10 16:32:31, rog wrote:
> I think we should select the _id field so we can avoid pulling all the
metadata
> over the wire.

Done.

https://codereview.appspot.com/40160043/diff/1/state/state.go#newcode510
state/state.go:510: chosenRevision := -1
On 2013/12/10 16:32:31, rog wrote:
> or:
> 
> chosenRevision := curl.Revision
> if maxRevision > chosenRevision {
>     // More recent revision exists in state, pick the next.
>     chosenRevision = maxRevision + 1
> }
> 
> ?

Actually it needs to be if maxRevision >= chosenRevision, so that if we're
trying to upload a curl with an existing revision the bumping will kick in.

https://codereview.appspot.com/40160043/diff/1/state/state.go#newcode554
state/state.go:554: if doc.PendingUpload != true {
On 2013/12/10 16:32:31, rog wrote:
> !doc.PendingUpload

:) ofc

https://codereview.appspot.com/40160043/diff/1/state/state.go#newcode557
state/state.go:557: for attempt := 0; attempt < 3; attempt++ {
On 2013/12/10 16:32:31, rog wrote:
> I don't think there's any point in retrying here, is there?
> PendingUpload can never be set to true once it's false,
> can it?

Initially, my idea was to have an assert combining txn.DocExists AND
pendingupload=true, but try as I might, couldn't find a way to do it. DocExists
is some weird mgo shortcut ('d+'), whereas in mongo parlance it should be
something like {$exists: true}. That didn' work either.

But perhaps you're right and we can skip the loop.

[dimitern, 2013-12-11 09:53:05]

Please take a look.

[rog, 2013-12-11 10:08:13]

LGTM, thanks.

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

https://codereview.appspot.com/40160043/diff/1/state/state.go#newcode476
state/state.go:476: if curl == nil {
On 2013/12/11 09:44:18, dimitern wrote:
> On 2013/12/10 16:32:31, rog wrote:
> > I think it's reasonable to lose this test and just panic
> > if we pass in a nil url.
> > 
> > In general we don't test parameters for nil-ness (we don't check
> > that st != nil for example).
> 
> Done.

not done :-)

https://codereview.appspot.com/40160043/diff/1/state/state.go#newcode485
state/state.go:485: // Get a regex with the charm URL and no revision.
On 2013/12/11 09:44:18, dimitern wrote:
> On 2013/12/10 16:32:31, rog wrote:
> > Rather than the string manipulation, we could do:
> > 
> > noRevURL := *curl
> > noRevURL.Revision = -1
> > curlRegex := "^" + regexp.QuoteMeta(noRevURL.String())
> 
> Or even noRevURL := curl.WithRevision(-1).

even better, yeah!

https://codereview.appspot.com/40160043/diff/40001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/40160043/diff/40001/state/state.go#newcode467
state/state.go:467: if curl == nil {
d

https://codereview.appspot.com/40160043/diff/40001/state/state.go#newcode556
state/state.go:556: // Run the transaction, and retry on abort.
d

https://codereview.appspot.com/40160043/diff/40001/state/state.go#newcode558
state/state.go:558: return nil, err
return nil, onAbort(err, fmt.Errorf("charm revision already uploaded"))
?

[dimitern, 2013-12-11 12:50:09]

Please take a look.

https://codereview.appspot.com/40160043/diff/40001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/40160043/diff/40001/state/state.go#newcode467
state/state.go:467: if curl == nil {
On 2013/12/11 10:08:13, rog wrote:
> d

Done.

https://codereview.appspot.com/40160043/diff/40001/state/state.go#newcode556
state/state.go:556: // Run the transaction, and retry on abort.
On 2013/12/11 10:08:13, rog wrote:
> d

Done.

https://codereview.appspot.com/40160043/diff/40001/state/state.go#newcode558
state/state.go:558: return nil, err
On 2013/12/11 10:08:13, rog wrote:
> return nil, onAbort(err, fmt.Errorf("charm revision already uploaded"))
> ?

Done.