Fix tools metadata generation for null provider

Fix tools metadata generation for null provider

There's two functional changes here:
 - If there's existing tools metadata, use it
   if it has sha256/size in it, and the tools
   haven't been replaced.
 - Don't use net/http or the tools URL when
   fetching tools to compute sha256/size. That
   doesn't work with sshstorage.
 - Don't fetch existing tools to resolve metadata
   during normal tools syncing operations. Only
   the "juju metadata generate-tools" command
   will do this now.

Additionally, I have refactored
environs/tools/simplestreams.go, to make the
intent of WriteMetadata (now MergeAndWriteMetadata)
more obvious. That function is now composed of
several other more specific functions.

Fixes #1235717

https://code.launchpad.net/~axwalk/juju-core/lp1235717-refactor-simplestreams-writemetadata/+merge/189767

(do not edit description out of merge proposal)

[axw, 2013-10-08 06:23:00]

Please take a look.

[axw, 2013-10-08 07:08:00]

On 2013/10/08 06:23:00, axw wrote:
> Please take a look.

More tests on the way, in case anyone gets to this before I repropose.

[axw, 2013-10-08 07:40:23]

Please take a look.

[rog, 2013-10-09 12:08:57]

This looks great in general, with a few remarks and suggestions below.

https://codereview.appspot.com/14527043/diff/5001/cmd/plugins/juju-metadata/t...
File cmd/plugins/juju-metadata/toolsmetadata.go (right):

https://codereview.appspot.com/14527043/diff/5001/cmd/plugins/juju-metadata/t...
cmd/plugins/juju-metadata/toolsmetadata.go:76: return
tools.MergeAndWriteMetadata(targetStorage, toolsList,
tools.ResolveFlag(c.fetch))
I'd prefer to see explicit values for the resolve flag here - using it as a
boolean gets around the advantages of using the named constants, I think.

https://codereview.appspot.com/14527043/diff/5001/environs/tools/simplestream...
File environs/tools/simplestreams.go (right):

https://codereview.appspot.com/14527043/diff/5001/environs/tools/simplestream...
environs/tools/simplestreams.go:217: // the metadata locally.
s/metadata/size and hash/ ?

https://codereview.appspot.com/14527043/diff/5001/environs/tools/simplestream...
environs/tools/simplestreams.go:220: if md.Size == 0 {
if md.Size != 0 {
    continue
}

to save an indent level?

https://codereview.appspot.com/14527043/diff/5001/environs/tools/simplestream...
environs/tools/simplestreams.go:240: func MergeMetadata(newMetadata, oldMetadata
[]*ToolsMetadata) []*ToolsMetadata {
I'm not sure that "new" and "old" are appropriate prefixes here - they seem to
imply that the arguments are treated differently, but isn't MergeMetadata
commutative?

Personally I'd use something like m0 and m1, but go with your own thoughts for
naming.

https://codereview.appspot.com/14527043/diff/5001/environs/tools/simplestream...
environs/tools/simplestreams.go:292: type ResolveFlag bool
I know we do this elsewhere, but I've come to dislike
this idiom - it's an unsatisfactory halfway house between
named constants and bools. There's still code that
uses the bool semantics, which means that these names aren't
really helping much because you have to know their values
anyway.

I'd prefer either to use int and iota or to stick with the bool
and lose the named constants.

https://codereview.appspot.com/14527043/diff/5001/environs/tools/simplestream...
environs/tools/simplestreams.go:300: // and merges it with metadata generated
from the given tools list.
It's not clear what targetTools is here. It sounds like it's
a list of tools in the target storage, but it doesn't actually
seem like that's the case.

https://codereview.appspot.com/14527043/diff/5001/environs/tools/simplestream...
environs/tools/simplestreams.go:301: // If resolve is true, incomplete metadata
is resolved by fetching the tools
// If resolve is Resolve

https://codereview.appspot.com/14527043/diff/5001/environs/tools/simplestream...
environs/tools/simplestreams.go:304: func MergeAndWriteMetadata(stor
storage.Storage, targetTools coretools.List, resolve ResolveFlag) error {
s/stor/target/ ?

(given that we refer to "the target storage" above)

https://codereview.appspot.com/14527043/diff/5001/environs/tools/simplestream...
File environs/tools/simplestreams_test.go (right):

https://codereview.appspot.com/14527043/diff/5001/environs/tools/simplestream...
environs/tools/simplestreams_test.go:561: func (*metadataHelperSuite)
TestMergeMetadata(c *gc.C) {
I think this might read better as a table-driven test,
with each item in the table containing the inputs and the
expected output.
Then it's easy to check commutativity in a uniform
way, for example.

[axw, 2013-10-09 14:28:25]

https://codereview.appspot.com/14527043/diff/5001/cmd/plugins/juju-metadata/t...
File cmd/plugins/juju-metadata/toolsmetadata.go (right):

https://codereview.appspot.com/14527043/diff/5001/cmd/plugins/juju-metadata/t...
cmd/plugins/juju-metadata/toolsmetadata.go:76: return
tools.MergeAndWriteMetadata(targetStorage, toolsList,
tools.ResolveFlag(c.fetch))
On 2013/10/09 12:08:57, rog wrote:
> I'd prefer to see explicit values for the resolve flag here - using it as a
> boolean gets around the advantages of using the named constants, I think.

Done.

https://codereview.appspot.com/14527043/diff/5001/environs/tools/simplestream...
File environs/tools/simplestreams.go (right):

https://codereview.appspot.com/14527043/diff/5001/environs/tools/simplestream...
environs/tools/simplestreams.go:217: // the metadata locally.
On 2013/10/09 12:08:57, rog wrote:
> s/metadata/size and hash/ ?

Done.

https://codereview.appspot.com/14527043/diff/5001/environs/tools/simplestream...
environs/tools/simplestreams.go:220: if md.Size == 0 {
On 2013/10/09 12:08:57, rog wrote:
> if md.Size != 0 {
>     continue
> }
> 
> to save an indent level?

Done.

https://codereview.appspot.com/14527043/diff/5001/environs/tools/simplestream...
environs/tools/simplestreams.go:240: func MergeMetadata(newMetadata, oldMetadata
[]*ToolsMetadata) []*ToolsMetadata {
On 2013/10/09 12:08:57, rog wrote:
> I'm not sure that "new" and "old" are appropriate prefixes here - they seem to
> imply that the arguments are treated differently, but isn't MergeMetadata
> commutative?
> 
> Personally I'd use something like m0 and m1, but go with your own thoughts for
> naming.

It's not commutative, per the last sentence of the doc comment:
"If both entries have information, prefer the entry from newMetadata".

The idea behind that is that if both have size&hash, then the one from
"newMetadata" is considered newer. That's to cater for copying over the top of
existing tools (say, a dev build).

https://codereview.appspot.com/14527043/diff/5001/environs/tools/simplestream...
environs/tools/simplestreams.go:292: type ResolveFlag bool
On 2013/10/09 12:08:57, rog wrote:
> I know we do this elsewhere, but I've come to dislike
> this idiom - it's an unsatisfactory halfway house between
> named constants and bools. There's still code that
> uses the bool semantics, which means that these names aren't
> really helping much because you have to know their values
> anyway.
>
> I'd prefer either to use int and iota or to stick with the bool
> and lose the named constants.

Consider a bool as (conceptually) a 1-bit unsigned int; then what's the
difference between that and int with constant named values? The only difference
I see is that the bool is range limited to [0, 1], which is a very good thing.

I'd be happy to drop the named type. Would that be a reasonable alternative?
i.e.:

const (
    DontResolve = false
    Resolve = true
)

func MergeAndWriteMetadata(stor storage.Storage, targetTools coretools.List,
resolve bool) error {
...
}

then people can use bools if they want, without much remorse, or they can use
the named constants as documentation.

https://codereview.appspot.com/14527043/diff/5001/environs/tools/simplestream...
environs/tools/simplestreams.go:300: // and merges it with metadata generated
from the given tools list.
On 2013/10/09 12:08:57, rog wrote:
> It's not clear what targetTools is here. It sounds like it's
> a list of tools in the target storage, but it doesn't actually
> seem like that's the case.

It is a list of tools in the target storage (it doesn't really make sense to
write metadata for anything else, as tools paths are relative to the metadata).

I'm going to drop "target" in all cases, because there is no knowledge of
"source" in this function.

https://codereview.appspot.com/14527043/diff/5001/environs/tools/simplestream...
environs/tools/simplestreams.go:301: // If resolve is true, incomplete metadata
is resolved by fetching the tools
On 2013/10/09 12:08:57, rog wrote:
> // If resolve is Resolve

I've dropped the named type and made it a bool, so I'll leave this alone.

https://codereview.appspot.com/14527043/diff/5001/environs/tools/simplestream...
environs/tools/simplestreams.go:304: func MergeAndWriteMetadata(stor
storage.Storage, targetTools coretools.List, resolve ResolveFlag) error {
On 2013/10/09 12:08:57, rog wrote:
> s/stor/target/ ?
> 
> (given that we refer to "the target storage" above)

I went the other way round: s/ target//.

https://codereview.appspot.com/14527043/diff/5001/environs/tools/simplestream...
File environs/tools/simplestreams_test.go (right):

https://codereview.appspot.com/14527043/diff/5001/environs/tools/simplestream...
environs/tools/simplestreams_test.go:561: func (*metadataHelperSuite)
TestMergeMetadata(c *gc.C) {
On 2013/10/09 12:08:57, rog wrote:
> I think this might read better as a table-driven test,
> with each item in the table containing the inputs and the
> expected output.
> Then it's easy to check commutativity in a uniform
> way, for example.

Done. (Not commutative as per my other comment, so no change in coverage.)

[axw, 2013-10-09 14:33:32]

Please take a look.

[rog, 2013-10-09 16:29:42]

LGTM with some thoughts below.

For the future, I think that the "resolve" mode is unnecessary and should go -
if we have items in the target bucket that we don't have hashes for, we should
just ignore them. This will actually work fine in practice, I think, even if we
abort a SyncTools half way through, because when we retry the SyncTools, any
items in the target bucket that we didn't get around to adding to the target
bucket simplestreams data should have metadata in the source bucket's
simplestreams data. 

In general, I think retro-discovering hash data by trying to find out what the
data currently is is a flawed idea - the hash of some tools should be generated
on first upload and preserved forever after.

https://codereview.appspot.com/14527043/diff/5001/environs/tools/simplestream...
File environs/tools/simplestreams.go (right):

https://codereview.appspot.com/14527043/diff/5001/environs/tools/simplestream...
environs/tools/simplestreams.go:240: func MergeMetadata(newMetadata, oldMetadata
[]*ToolsMetadata) []*ToolsMetadata {
On 2013/10/09 14:28:25, axw wrote:
> On 2013/10/09 12:08:57, rog wrote:
> > I'm not sure that "new" and "old" are appropriate prefixes here - they seem
to
> > imply that the arguments are treated differently, but isn't MergeMetadata
> > commutative?
> > 
> > Personally I'd use something like m0 and m1, but go with your own thoughts
for
> > naming.
> 
> It's not commutative, per the last sentence of the doc comment:
> "If both entries have information, prefer the entry from newMetadata".
> 
> The idea behind that is that if both have size&hash, then the one from
> "newMetadata" is considered newer. That's to cater for copying over the top of
> existing tools (say, a dev build).

Hmm, that *should* never happen - a version should always correspond only to one
particular tarball. I think if we do get a clash like this, then we have a
problem that I suspect our current logic won't cope with.

Perhaps this function should check for inconsistencies and return an error if it
finds one.

https://codereview.appspot.com/14527043/diff/5001/environs/tools/simplestream...
environs/tools/simplestreams.go:292: type ResolveFlag bool
On 2013/10/09 14:28:25, axw wrote:
> On 2013/10/09 12:08:57, rog wrote:
> > I know we do this elsewhere, but I've come to dislike
> > this idiom - it's an unsatisfactory halfway house between
> > named constants and bools. There's still code that
> > uses the bool semantics, which means that these names aren't
> > really helping much because you have to know their values
> > anyway.
> >
> > I'd prefer either to use int and iota or to stick with the bool
> > and lose the named constants.
> 
> Consider a bool as (conceptually) a 1-bit unsigned int; then what's the
> difference between that and int with constant named values? The only
difference
> I see is that the bool is range limited to [0, 1], which is a very good thing.

The main difference I see is that various pieces of code will use a boolean
directly, or a boolean test and the sense of the code isn't clear in that case.
For example, in another piece of code in this CL, there's a boolean field called
"fetch" which is passed directly as the resolve argument; and in
MergeAndWriteMetadata itself, we just use it as a bool.

I'd leave ResolveFlag, but explicitly use the named constants everywhere,
rather than just type converting to ResolveFlag.

But given that this function isn't long for this world (I'm hoping), I'm ok as
it is.

https://codereview.appspot.com/14527043/diff/13001/environs/tools/simplestrea...
File environs/tools/simplestreams_test.go (right):

https://codereview.appspot.com/14527043/diff/13001/environs/tools/simplestrea...
environs/tools/simplestreams_test.go:641: c.Assert(merged, gc.DeepEquals,
test.merged)
Thanks - I find this much easier to follow (and it's easier to change if we
decide to change the semantics of MergeMetadata)

[axw, 2013-10-10 02:26:45]

On 2013/10/09 16:29:42, rog wrote:
> LGTM with some thoughts below.

Thanks. I'll change the newMetadata/oldMetadata thing as suggested, but I think
I'll leave the others to a followup.

> For the future, I think that the "resolve" mode is unnecessary and should go -
> if we have items in the target bucket that we don't have hashes for, we should
> just ignore them. This will actually work fine in practice, I think, even if
we
> abort a SyncTools half way through, because when we retry the SyncTools, any
> items in the target bucket that we didn't get around to adding to the target
> bucket simplestreams data should have metadata in the source bucket's
> simplestreams data. 
> 
> In general, I think retro-discovering hash data by trying to find out what the
> data currently is is a flawed idea - the hash of some tools should be
generated
> on first upload and preserved forever after.

I'd like to say I agree, however I know the requirements here are a bit subtle.
I think it'd be good to talk with wallyworld at SFO to nut it out.

>
https://codereview.appspot.com/14527043/diff/5001/environs/tools/simplestream...
> File environs/tools/simplestreams.go (right):
> 
>
https://codereview.appspot.com/14527043/diff/5001/environs/tools/simplestream...
> environs/tools/simplestreams.go:240: func MergeMetadata(newMetadata,
oldMetadata
> []*ToolsMetadata) []*ToolsMetadata {
> On 2013/10/09 14:28:25, axw wrote:
> > On 2013/10/09 12:08:57, rog wrote:
> > > I'm not sure that "new" and "old" are appropriate prefixes here - they
seem
> to
> > > imply that the arguments are treated differently, but isn't MergeMetadata
> > > commutative?
> > > 
> > > Personally I'd use something like m0 and m1, but go with your own thoughts
> for
> > > naming.
> > 
> > It's not commutative, per the last sentence of the doc comment:
> > "If both entries have information, prefer the entry from newMetadata".
> > 
> > The idea behind that is that if both have size&hash, then the one from
> > "newMetadata" is considered newer. That's to cater for copying over the top
of
> > existing tools (say, a dev build).
> 
> Hmm, that *should* never happen - a version should always correspond only to
one
> particular tarball. I think if we do get a clash like this, then we have a
> problem that I suspect our current logic won't cope with.
> 
> Perhaps this function should check for inconsistencies and return an error if
it
> finds one.

Yeah, that sounds sensible. The tools won't be copied over if they are already
in the target, so the "newMetadata" entry won't have a size/hash. So this code
is only handling a theoretical case, anyway. I'll make this change and
repropose.

>
https://codereview.appspot.com/14527043/diff/5001/environs/tools/simplestream...
> environs/tools/simplestreams.go:292: type ResolveFlag bool
> On 2013/10/09 14:28:25, axw wrote:
> > On 2013/10/09 12:08:57, rog wrote:
> > > I know we do this elsewhere, but I've come to dislike
> > > this idiom - it's an unsatisfactory halfway house between
> > > named constants and bools. There's still code that
> > > uses the bool semantics, which means that these names aren't
> > > really helping much because you have to know their values
> > > anyway.
> > >
> > > I'd prefer either to use int and iota or to stick with the bool
> > > and lose the named constants.
> > 
> > Consider a bool as (conceptually) a 1-bit unsigned int; then what's the
> > difference between that and int with constant named values? The only
> difference
> > I see is that the bool is range limited to [0, 1], which is a very good
thing.
> 
> The main difference I see is that various pieces of code will use a boolean
> directly, or a boolean test and the sense of the code isn't clear in that
case.
> For example, in another piece of code in this CL, there's a boolean field
called
> "fetch" which is passed directly as the resolve argument; and in
> MergeAndWriteMetadata itself, we just use it as a bool.
> 
> I'd leave ResolveFlag, but explicitly use the named constants everywhere,
> rather than just type converting to ResolveFlag.
> 
> But given that this function isn't long for this world (I'm hoping), I'm ok as
> it is.
> 
>
https://codereview.appspot.com/14527043/diff/13001/environs/tools/simplestrea...
> File environs/tools/simplestreams_test.go (right):
> 
>
https://codereview.appspot.com/14527043/diff/13001/environs/tools/simplestrea...
> environs/tools/simplestreams_test.go:641: c.Assert(merged, gc.DeepEquals,
> test.merged)
> Thanks - I find this much easier to follow (and it's easier to change if we
> decide to change the semantics of MergeMetadata)

[axw, 2013-10-10 03:07:46]

Please take a look.

[axw, 2013-10-16 04:47:32]

Please take a look.

[axw, 2013-10-16 05:15:16]

Please take a look.

[axw, 2013-10-16 05:28:31]

On 2013/10/10 02:26:45, axw wrote:
> On 2013/10/09 16:29:42, rog wrote:
> > LGTM with some thoughts below.
> 
> Thanks. I'll change the newMetadata/oldMetadata thing as suggested, but I
think
> I'll leave the others to a followup.
> 
> > For the future, I think that the "resolve" mode is unnecessary and should go
-
> > if we have items in the target bucket that we don't have hashes for, we
should
> > just ignore them. This will actually work fine in practice, I think, even if
> we
> > abort a SyncTools half way through, because when we retry the SyncTools, any
> > items in the target bucket that we didn't get around to adding to the target
> > bucket simplestreams data should have metadata in the source bucket's
> > simplestreams data. 
> > 
> > In general, I think retro-discovering hash data by trying to find out what
the
> > data currently is is a flawed idea - the hash of some tools should be
> generated
> > on first upload and preserved forever after.
> 
> I'd like to say I agree, however I know the requirements here are a bit
subtle.
> I think it'd be good to talk with wallyworld at SFO to nut it out.

I discussed this with wallyworld on IRC, and he agreed that it should go in
1.18.
After 1.16, all tools in storage should have corresponding metadata with size
and hash.

> >
>
https://codereview.appspot.com/14527043/diff/5001/environs/tools/simplestream...
> > File environs/tools/simplestreams.go (right):
> > 
> >
>
https://codereview.appspot.com/14527043/diff/5001/environs/tools/simplestream...
> > environs/tools/simplestreams.go:240: func MergeMetadata(newMetadata,
> oldMetadata
> > []*ToolsMetadata) []*ToolsMetadata {
> > On 2013/10/09 14:28:25, axw wrote:
> > > On 2013/10/09 12:08:57, rog wrote:
> > > > I'm not sure that "new" and "old" are appropriate prefixes here - they
> seem
> > to
> > > > imply that the arguments are treated differently, but isn't
MergeMetadata
> > > > commutative?
> > > > 
> > > > Personally I'd use something like m0 and m1, but go with your own
thoughts
> > for
> > > > naming.
> > > 
> > > It's not commutative, per the last sentence of the doc comment:
> > > "If both entries have information, prefer the entry from newMetadata".
> > > 
> > > The idea behind that is that if both have size&hash, then the one from
> > > "newMetadata" is considered newer. That's to cater for copying over the
top
> of
> > > existing tools (say, a dev build).
> > 
> > Hmm, that *should* never happen - a version should always correspond only to
> one
> > particular tarball. I think if we do get a clash like this, then we have a
> > problem that I suspect our current logic won't cope with.
> > 
> > Perhaps this function should check for inconsistencies and return an error
if
> it
> > finds one.
> 
> Yeah, that sounds sensible. The tools won't be copied over if they are already
> in the target, so the "newMetadata" entry won't have a size/hash. So this code
> is only handling a theoretical case, anyway. I'll make this change and
> repropose.
> 
> >
>
https://codereview.appspot.com/14527043/diff/5001/environs/tools/simplestream...
> > environs/tools/simplestreams.go:292: type ResolveFlag bool
> > On 2013/10/09 14:28:25, axw wrote:
> > > On 2013/10/09 12:08:57, rog wrote:
> > > > I know we do this elsewhere, but I've come to dislike
> > > > this idiom - it's an unsatisfactory halfway house between
> > > > named constants and bools. There's still code that
> > > > uses the bool semantics, which means that these names aren't
> > > > really helping much because you have to know their values
> > > > anyway.
> > > >
> > > > I'd prefer either to use int and iota or to stick with the bool
> > > > and lose the named constants.
> > > 
> > > Consider a bool as (conceptually) a 1-bit unsigned int; then what's the
> > > difference between that and int with constant named values? The only
> > difference
> > > I see is that the bool is range limited to [0, 1], which is a very good
> thing.
> > 
> > The main difference I see is that various pieces of code will use a boolean
> > directly, or a boolean test and the sense of the code isn't clear in that
> case.
> > For example, in another piece of code in this CL, there's a boolean field
> called
> > "fetch" which is passed directly as the resolve argument; and in
> > MergeAndWriteMetadata itself, we just use it as a bool.
> > 
> > I'd leave ResolveFlag, but explicitly use the named constants everywhere,
> > rather than just type converting to ResolveFlag.
> > 
> > But given that this function isn't long for this world (I'm hoping), I'm ok
as
> > it is.
> > 
> >
>
https://codereview.appspot.com/14527043/diff/13001/environs/tools/simplestrea...
> > File environs/tools/simplestreams_test.go (right):
> > 
> >
>
https://codereview.appspot.com/14527043/diff/13001/environs/tools/simplestrea...
> > environs/tools/simplestreams_test.go:641: c.Assert(merged, gc.DeepEquals,
> > test.merged)
> > Thanks - I find this much easier to follow (and it's easier to change if we
> > decide to change the semantics of MergeMetadata)