state;apiserver: Fix a race - lp bug #1067979

state;apiserver: Fix a race - lp bug #1067979

This introduces some changes to how charm store
charms are added through the API (in state and
to provider storage). Now PrepareStoreCharmUpload
is called before trying to download the charm,
repackage it and upload it to storage, in order
to reserve a charm URL in state with pending
state. Added a test that demonstrates multiple
concurrent deployments of the same charm does
not cause the race issues, like mentioned in
the bug.

A few drive-by fixes brought up during review:
* Added ReadSHA256 and ReadFileSHA256 helpers in
  utils, and changed most places where hashes
  are calculated to use them.
* Charms are now uploaded to storage with a
  randomly generated archive names with the
  format "<charm-name>-<revision>-<uuid>".
  This allows multiple concurrent uploads
  to happen safely, and at the end AddCharm
  in the API checks to see if the charm info
  is already updated in state and if so, deletes
  the duplicated upload.
* Added GetEnvironStorage helper to environs/testing.
* Fixed potential compatibility issues with older
  versions and the recently added PendingUpload
  and Placeholder fields of the charm document.

Also tested multiple concurrent deployments with
the local provider manually and updated the bug
accordingly.

https://code.launchpad.net/~dimitern/juju-core/260-lp-1067979-duplicate-charm/+merge/203291

(do not edit description out of merge proposal)

[dimitern, 2014-01-27 09:41:12]

Please take a look.

[fwereade, 2014-01-27 12:46:16]

A few comments, reprising what we discussed at lunch

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

https://codereview.appspot.com/53210044/diff/1/state/apiserver/client/client....
state/apiserver/client/client.go:885: storageURL, err := storage.URL(name)
I'd prefer to generate unique names than to potentially have multiple writers to
the same storage location. I think it'd be best to have multiple independent
downloads, and multiple independent uploads, and then just to have a single
worker win the race and set a URL that definitely worked in state. No need to
rename or anything -- as far as I'm aware, charm URL is totally independent of
storage URL anyway, and if it's not that's a bug that should be fixed anyway ;).

https://codereview.appspot.com/53210044/diff/1/state/apiserver/client/client_...
File state/apiserver/client/client_test.go (right):

https://codereview.appspot.com/53210044/diff/1/state/apiserver/client/client_...
state/apiserver/client/client_test.go:1850: }
in light of the previous comment,we *do* want multiple charms uploaded (but all
except one to be deleted). And whichever one isn't deleted should match the
archive url of the charm in state.

[dimitern, 2014-01-27 19:15:05]

Please take a look.

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

https://codereview.appspot.com/53210044/diff/1/state/apiserver/client/client....
state/apiserver/client/client.go:885: storageURL, err := storage.URL(name)
On 2014/01/27 12:46:16, fwereade wrote:
> I'd prefer to generate unique names than to potentially have multiple writers
to
> the same storage location. I think it'd be best to have multiple independent
> downloads, and multiple independent uploads, and then just to have a single
> worker win the race and set a URL that definitely worked in state. No need to
> rename or anything -- as far as I'm aware, charm URL is totally independent of
> storage URL anyway, and if it's not that's a bug that should be fixed anyway
;).

I'll add code to generate a storage URL based on a random UUID string.
If you don't mind, I'll also follow John's suggestion to include the charm name
and revision as prefix, to make the string a bit less cryptic.

https://codereview.appspot.com/53210044/diff/1/state/apiserver/client/client_...
File state/apiserver/client/client_test.go (right):

https://codereview.appspot.com/53210044/diff/1/state/apiserver/client/client_...
state/apiserver/client/client_test.go:1850: }
On 2014/01/27 12:46:16, fwereade wrote:
> in light of the previous comment,we *do* want multiple charms uploaded (but
all
> except one to be deleted). And whichever one isn't deleted should match the
> archive url of the charm in state.

Will change accordingly.

[dimitern, 2014-01-27 19:24:06]

Please take a look.

[fwereade, 2014-01-28 08:19:41]

few more comments, but the heart of it looks solid

https://codereview.appspot.com/53210044/diff/40001/charm/url.go
File charm/url.go (right):

https://codereview.appspot.com/53210044/diff/40001/charm/url.go#newcode261
charm/url.go:261: func (u *URL) ArchiveName() (string, error) {
I'm certain this is not part of the charm package -- np with the code, but it's
very definitely an internal detail of the state server.

https://codereview.appspot.com/53210044/diff/40001/state/apiserver/charms.go
File state/apiserver/charms.go (right):

https://codereview.appspot.com/53210044/diff/40001/state/apiserver/charms.go#...
state/apiserver/charms.go:243: func GetSHA256(source io.Reader) (string, int64,
error) {
whereas this feels maybe utilsy -- and we do this *all over the place*, so
please hunt them down and make them all use the same utils func, assuming that
is practical

https://codereview.appspot.com/53210044/diff/40001/state/apiserver/client/cli...
File state/apiserver/client/client.go (right):

https://codereview.appspot.com/53210044/diff/40001/state/apiserver/client/cli...
state/apiserver/client/client.go:908: return fmt.Errorf("cannot remove
duplicated charm from storage: %v", err)
log it, I think, but don't spoil the user's day -- we still managed to do what
she asked, after all

https://codereview.appspot.com/53210044/diff/40001/state/apiserver/client/cli...
File state/apiserver/client/client_test.go (right):

https://codereview.appspot.com/53210044/diff/40001/state/apiserver/client/cli...
state/apiserver/client/client_test.go:1821: close(ops)
race? we should expect 9 Deletes as well, I think

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

https://codereview.appspot.com/53210044/diff/40001/state/state.go#newcode606
state/state.go:606: err = st.charms.Find(D{{"_id", curl}, {"placeholder",
false}}).One(&uploadedCharm)
placeholder might be false in a new installation, or missing in an old one

https://codereview.appspot.com/53210044/diff/40001/state/state.go#newcode624
state/state.go:624: Insert: uploadedCharm,
if there *was* a placeholder charm, the insert would fail, wouldn't it?

https://codereview.appspot.com/53210044/diff/40001/state/state.go#newcode642
state/state.go:642: StillPlaceholder = D{{"placeholder", true}}
I think these should probably not be exported

[dimitern, 2014-01-28 10:55:04]

Please take a look.

https://codereview.appspot.com/53210044/diff/40001/charm/url.go
File charm/url.go (right):

https://codereview.appspot.com/53210044/diff/40001/charm/url.go#newcode261
charm/url.go:261: func (u *URL) ArchiveName() (string, error) {
On 2014/01/28 08:19:41, fwereade wrote:
> I'm certain this is not part of the charm package -- np with the code, but
it's
> very definitely an internal detail of the state server.

I'll move it to the state package then.

https://codereview.appspot.com/53210044/diff/40001/state/apiserver/charms.go
File state/apiserver/charms.go (right):

https://codereview.appspot.com/53210044/diff/40001/state/apiserver/charms.go#...
state/apiserver/charms.go:243: func GetSHA256(source io.Reader) (string, int64,
error) {
On 2014/01/28 08:19:41, fwereade wrote:
> whereas this feels maybe utilsy -- and we do this *all over the place*, so
> please hunt them down and make them all use the same utils func, assuming that
> is practical

Done.

https://codereview.appspot.com/53210044/diff/40001/state/apiserver/client/cli...
File state/apiserver/client/client.go (right):

https://codereview.appspot.com/53210044/diff/40001/state/apiserver/client/cli...
state/apiserver/client/client.go:908: return fmt.Errorf("cannot remove
duplicated charm from storage: %v", err)
On 2014/01/28 08:19:41, fwereade wrote:
> log it, I think, but don't spoil the user's day -- we still managed to do what
> she asked, after all

Wasn't sure whether to log an error or warning. I'll just log it as an error
then, because it seems serious enough (it shouldn't happen usually).

https://codereview.appspot.com/53210044/diff/40001/state/apiserver/client/cli...
File state/apiserver/client/client_test.go (right):

https://codereview.appspot.com/53210044/diff/40001/state/apiserver/client/cli...
state/apiserver/client/client_test.go:1821: close(ops)
On 2014/01/28 08:19:41, fwereade wrote:
> race? we should expect 9 Deletes as well, I think

There is no way to detect storage removals with the dummy provider I could find.
But I am checking below that we do have a single upload at the end. Not sure
what race do you mean?

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

https://codereview.appspot.com/53210044/diff/40001/state/state.go#newcode606
state/state.go:606: err = st.charms.Find(D{{"_id", curl}, {"placeholder",
false}}).One(&uploadedCharm)
On 2014/01/28 08:19:41, fwereade wrote:
> placeholder might be false in a new installation, or missing in an old one

I'll modify the condition, but this applies to other places in the code as well.

https://codereview.appspot.com/53210044/diff/40001/state/state.go#newcode624
state/state.go:624: Insert: uploadedCharm,
On 2014/01/28 08:19:41, fwereade wrote:
> if there *was* a placeholder charm, the insert would fail, wouldn't it?

Right, but since I changed how we select the document above it shouldn't be a
problem.

https://codereview.appspot.com/53210044/diff/40001/state/state.go#newcode642
state/state.go:642: StillPlaceholder = D{{"placeholder", true}}
On 2014/01/28 08:19:41, fwereade wrote:
> I think these should probably not be exported

I just reformatted the block - they were there before from Ian's branch about
placeholders. I'll unexport them.

[fwereade, 2014-01-28 12:00:31]

Very nice, last round I think

https://codereview.appspot.com/53210044/diff/40001/state/apiserver/client/cli...
File state/apiserver/client/client_test.go (right):

https://codereview.appspot.com/53210044/diff/40001/state/apiserver/client/cli...
state/apiserver/client/client_test.go:1821: close(ops)
On 2014/01/28 10:55:05, dimitern wrote:
> On 2014/01/28 08:19:41, fwereade wrote:
> > race? we should expect 9 Deletes as well, I think
> 
> There is no way to detect storage removals with the dummy provider I could
find.
> But I am checking below that we do have a single upload at the end. Not sure
> what race do you mean?

I was worried that the deletes *were* pushing ops, and that channel might
sometimes be closed before the ops stopped. But yeah, looks like no delete ops;
and I'm fine with the test. Just seemed a bit weird to mix the testing styles --
but I think we're good as is. Thanks.

https://codereview.appspot.com/53210044/diff/60001/state/apiserver/charms.go
File state/apiserver/charms.go (right):

https://codereview.appspot.com/53210044/diff/60001/state/apiserver/charms.go#...
state/apiserver/charms.go:229: func GetEnvironStorage(st *state.State)
(storage.Storage, error) {
maybe put this in export_test rather than growing the interface here?

https://codereview.appspot.com/53210044/diff/60001/state/apiserver/client/cli...
File state/apiserver/client/client_test.go (right):

https://codereview.appspot.com/53210044/diff/60001/state/apiserver/client/cli...
state/apiserver/client/client_test.go:1828: storage, err :=
apiserver.GetEnvironStorage(s.State)
ah, ok, you can't put it in export_test. Hmm. I think I'd maybe prefer it if
this were accessed via the environ on the test, but I'm not sure of the
tradeoffs -- I'll leave it to your judgment.

https://codereview.appspot.com/53210044/diff/60001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/53210044/diff/60001/state/state.go#newcode480
state/state.go:480: what := D{{"_id", curl}, {"placeholder", false},
{"pendingupload", false}}
Yeah, these bits do need to be fixed. Sorry, but they *will* break stuff.

https://codereview.appspot.com/53210044/diff/60001/state/state.go#newcode625
state/state.go:625: Insert: uploadedCharm,
Don't we still need to deal with a placeholder here?

https://codereview.appspot.com/53210044/diff/60001/utils/trivial.go
File utils/trivial.go (right):

https://codereview.appspot.com/53210044/diff/60001/utils/trivial.go#newcode127
utils/trivial.go:127: func GetSHA256(source io.Reader) (string, int64, error) {
ReadSHA256?

https://codereview.appspot.com/53210044/diff/60001/utils/trivial.go#newcode139
utils/trivial.go:139: func GetFileSHA256(filename string) (string, int64, error)
{
ReadFileSHA256?

https://codereview.appspot.com/53210044/diff/60001/utils/trivial.go#newcode146
utils/trivial.go:146: }
Also, please test these. Not quite trivial enough to be skippable IMO.

[dimitern, 2014-01-28 17:15:01]

Please take a look.

https://codereview.appspot.com/53210044/diff/60001/state/apiserver/charms.go
File state/apiserver/charms.go (right):

https://codereview.appspot.com/53210044/diff/60001/state/apiserver/charms.go#...
state/apiserver/charms.go:229: func GetEnvironStorage(st *state.State)
(storage.Storage, error) {
On 2014/01/28 12:00:32, fwereade wrote:
> maybe put this in export_test rather than growing the interface here?

It was there, but I needed it in two places. I'll put it in environs/testing.

https://codereview.appspot.com/53210044/diff/60001/state/apiserver/client/cli...
File state/apiserver/client/client_test.go (right):

https://codereview.appspot.com/53210044/diff/60001/state/apiserver/client/cli...
state/apiserver/client/client_test.go:1828: storage, err :=
apiserver.GetEnvironStorage(s.State)
On 2014/01/28 12:00:32, fwereade wrote:
> ah, ok, you can't put it in export_test. Hmm. I think I'd maybe prefer it if
> this were accessed via the environ on the test, but I'm not sure of the
> tradeoffs -- I'll leave it to your judgment.

I'll put it in environs/testing.

https://codereview.appspot.com/53210044/diff/60001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/53210044/diff/60001/state/state.go#newcode480
state/state.go:480: what := D{{"_id", curl}, {"placeholder", false},
{"pendingupload", false}}
On 2014/01/28 12:00:32, fwereade wrote:
> Yeah, these bits do need to be fixed. Sorry, but they *will* break stuff.

Done.

https://codereview.appspot.com/53210044/diff/60001/state/state.go#newcode625
state/state.go:625: Insert: uploadedCharm,
On 2014/01/28 12:00:32, fwereade wrote:
> Don't we still need to deal with a placeholder here?

If we reach this point we know it's not a placeholder and there's no document
for this charm URL. Not setting Placeholder: false here works, because it's the
default value. So it's OK I think.

https://codereview.appspot.com/53210044/diff/60001/utils/trivial.go
File utils/trivial.go (right):

https://codereview.appspot.com/53210044/diff/60001/utils/trivial.go#newcode127
utils/trivial.go:127: func GetSHA256(source io.Reader) (string, int64, error) {
On 2014/01/28 12:00:32, fwereade wrote:
> ReadSHA256?

Done.

https://codereview.appspot.com/53210044/diff/60001/utils/trivial.go#newcode139
utils/trivial.go:139: func GetFileSHA256(filename string) (string, int64, error)
{
On 2014/01/28 12:00:32, fwereade wrote:
> ReadFileSHA256?

Done.

https://codereview.appspot.com/53210044/diff/60001/utils/trivial.go#newcode146
utils/trivial.go:146: }
On 2014/01/28 12:00:32, fwereade wrote:
> Also, please test these. Not quite trivial enough to be skippable IMO.

Done.

[fwereade, 2014-01-28 23:09:30]

I still think there's something up with PrepareStoreCharmUpload, but it's
getting late and I may be being dense. Ping me tomorrow morning?

https://codereview.appspot.com/53210044/diff/60001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/53210044/diff/60001/state/state.go#newcode625
state/state.go:625: Insert: uploadedCharm,
On 2014/01/28 17:15:02, dimitern wrote:
> On 2014/01/28 12:00:32, fwereade wrote:
> > Don't we still need to deal with a placeholder here?
> 
> If we reach this point we know it's not a placeholder and there's no document
> for this charm URL. Not setting Placeholder: false here works, because it's
the
> default value. So it's OK I think.

We will still have fallen through the block above if err is nil but placeholder
is true. Right? I had to think about this harder than I'd like, which might
indicate that we could make that block clearer, but nothing really springs to
mind very obviously... perhaps a switch?

ops := []txn.Op{}
switch err {
case mgo.ErrNotFound:
    ops = append(ops, <insert op>)
case nil:
    if uploadedCharm.Placeholder {
        ops = append(ops, <update op>)
    } else {
        return newCharm(st, &uploadedCharm)
    }
default:
    return nil, err
}

..? Maybe not very compelling?

This might be a case where really pedantic SetTransactionHooks-style testing
would be a boon -- even if the logic is hard to follow we can just churn through
the possible test cases and have some more certainty.

[dimitern, 2014-01-29 13:49:05]

Please take a look.

https://codereview.appspot.com/53210044/diff/60001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/53210044/diff/60001/state/state.go#newcode625
state/state.go:625: Insert: uploadedCharm,
On 2014/01/28 23:09:31, fwereade wrote:
> On 2014/01/28 17:15:02, dimitern wrote:
> > On 2014/01/28 12:00:32, fwereade wrote:
> > > Don't we still need to deal with a placeholder here?
> > 
> > If we reach this point we know it's not a placeholder and there's no
document
> > for this charm URL. Not setting Placeholder: false here works, because it's
> the
> > default value. So it's OK I think.
> 
> We will still have fallen through the block above if err is nil but
placeholder
> is true. Right? I had to think about this harder than I'd like, which might
> indicate that we could make that block clearer, but nothing really springs to
> mind very obviously... perhaps a switch?
> 
> ops := []txn.Op{}
> switch err {
> case mgo.ErrNotFound:
>     ops = append(ops, <insert op>)
> case nil:
>     if uploadedCharm.Placeholder {
>         ops = append(ops, <update op>)
>     } else {
>         return newCharm(st, &uploadedCharm)
>     }
> default:
>     return nil, err
> }
> 
> ..? Maybe not very compelling?
> 
> This might be a case where really pedantic SetTransactionHooks-style testing
> would be a boon -- even if the logic is hard to follow we can just churn
through
> the possible test cases and have some more certainty.

I added a specific test case to cover "uploading a store charm when a
placeholder exists", and indeed it uncovered issues, which I fixed now. Thanks!

As for the transaction hooks, I can't use them unfortunately outside the state
packages (they're all defined in export_test). But despite that, I think the
code looks solid now.

[fwereade, 2014-01-29 16:01:21]

Most of these are genuinely trivial now -- if we're in agreement on them all,
LGTM, but ping me if there's any uncertainty

https://codereview.appspot.com/53210044/diff/100001/state/charm.go
File state/charm.go (right):

https://codereview.appspot.com/53210044/diff/100001/state/charm.go#newcode87
state/charm.go:87: func CharmArchiveName(name string, revision int) (string,
error) {
quibble quibble whine, I think this probably belongs in apiserver/client

https://codereview.appspot.com/53210044/diff/100001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/53210044/diff/100001/state/state.go#newcode633
state/state.go:633: Assert: txn.DocExists,
hmm, should be be asserting that it hasn't had archiveurl (etc) filled in? I
think there's a very very cornery corner case in which we read while it's still
a placeholder, but write when it's already uploaded.

That'd be a good transaction-hooks test ;).

https://codereview.appspot.com/53210044/diff/100001/state/state.go#newcode661
state/state.go:661: if err != nil {
this makes me twitch a little -- AFAICT the only possible value is
txn.ErrAborted, but that assumption feels very vulnerable to code perturbations
in the loop above. Maybe check for err == nil inside the loop, and return
newCharm() there, so that ErrAborted becomes the *only* way to fall out of the
loop, and just always return ErrExcessiveContention?

https://codereview.appspot.com/53210044/diff/100001/utils/trivial_test.go
File utils/trivial_test.go (right):

https://codereview.appspot.com/53210044/diff/100001/utils/trivial_test.go#new...
utils/trivial_test.go:87: {
}{{
    ...
},{
    ...

to compress both vertical and horizontal space?

And, hey, can we please just move these inside the test func? If they're not
used elsewhere they really don't deserve to be global IMO.

https://codereview.appspot.com/53210044/diff/100001/utils/trivial_test.go#new...
utils/trivial_test.go:105: func (utilsSuite) TestReadSHA256AndReadFileSHA256(c
*gc.C) {
*utilsSuite, please, across the board, lest someone add methods that require
pointer receivers and silently deactivate all the other tests.

[dimitern, 2014-01-29 20:59:12]

Please take a look.

https://codereview.appspot.com/53210044/diff/100001/state/charm.go
File state/charm.go (right):

https://codereview.appspot.com/53210044/diff/100001/state/charm.go#newcode87
state/charm.go:87: func CharmArchiveName(name string, revision int) (string,
error) {
On 2014/01/29 16:01:22, fwereade wrote:
> quibble quibble whine, I think this probably belongs in apiserver/client

Done.

https://codereview.appspot.com/53210044/diff/100001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/53210044/diff/100001/state/state.go#newcode633
state/state.go:633: Assert: txn.DocExists,
On 2014/01/29 16:01:22, fwereade wrote:
> hmm, should be be asserting that it hasn't had archiveurl (etc) filled in? I
> think there's a very very cornery corner case in which we read while it's
still
> a placeholder, but write when it's already uploaded.
> 
> That'd be a good transaction-hooks test ;).

Done.

https://codereview.appspot.com/53210044/diff/100001/state/state.go#newcode661
state/state.go:661: if err != nil {
On 2014/01/29 16:01:22, fwereade wrote:
> this makes me twitch a little -- AFAICT the only possible value is
> txn.ErrAborted, but that assumption feels very vulnerable to code
perturbations
> in the loop above. Maybe check for err == nil inside the loop, and return
> newCharm() there, so that ErrAborted becomes the *only* way to fall out of the
> loop, and just always return ErrExcessiveContention?

Done.

https://codereview.appspot.com/53210044/diff/100001/utils/trivial_test.go
File utils/trivial_test.go (right):

https://codereview.appspot.com/53210044/diff/100001/utils/trivial_test.go#new...
utils/trivial_test.go:87: {
On 2014/01/29 16:01:22, fwereade wrote:
> }{{
>     ...
> },{
>     ...
> 
> to compress both vertical and horizontal space?
> 
> And, hey, can we please just move these inside the test func? If they're not
> used elsewhere they really don't deserve to be global IMO.

Done.

https://codereview.appspot.com/53210044/diff/100001/utils/trivial_test.go#new...
utils/trivial_test.go:105: func (utilsSuite) TestReadSHA256AndReadFileSHA256(c
*gc.C) {
On 2014/01/29 16:01:22, fwereade wrote:
> *utilsSuite, please, across the board, lest someone add methods that require
> pointer receivers and silently deactivate all the other tests.

Done.

[dimitern, 2014-01-30 13:53:10]

Please take a look.

[dimitern, 2014-01-30 14:24:40]

Please take a look.