Merge lp:~axwalk/juju-core/lp1235717-refactor-simplestreams-writemetadata into lp:~go-bot/juju-core/trunk

[Andrew Wilkins]

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://codereview.appspot.com/14527043/

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://codereview.appspot.com/14527043/

[Andrew Wilkins (axwalk)]

Reviewers: mp+189767_code.launchpad.net,

Message:
Please take a look.

Description:
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.

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)

Please review this at https://codereview.appspot.com/14527043/

Affected files (+171, -122 lines):
   A [revision details]
   M cmd/jujud/agent_test.go
   M cmd/plugins/juju-metadata/toolsmetadata.go
   M cmd/plugins/juju-metadata/toolsmetadata_test.go
   M environs/sync/sync.go
   M environs/testing/tools.go
   M environs/tools/simplestreams.go
   M environs/tools/simplestreams_test.go
   M environs/tools/tools.go
   M worker/upgrader/upgrader_test.go

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/14527043/

[Roger Peppe (rogpeppe)]

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

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

https://codereview.appspot.com/14527043/diff/5001/cmd/plugins/juju-metadata/toolsmetadata.go#newcode76
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/simplestreams.go
File environs/tools/simplestreams.go (right):

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

https://codereview.appspot.com/14527043/diff/5001/environs/tools/simplestreams.go#newcode220
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/simplestreams.go#newcode240
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/simplestreams.go#newcode292
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/simplestreams.go#newcode300
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/simplestreams.go#newcode301
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/simplestreams.go#newcode304
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/simplestreams_test.go
File environs/tools/simplestreams_test.go (right):

https://codereview.appspot.com/...

[Andrew Wilkins (axwalk)]

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

https://codereview.appspot.com/14527043/diff/5001/cmd/plugins/juju-metadata/toolsmetadata.go#newcode76
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/simplestreams.go
File environs/tools/simplestreams.go (right):

https://codereview.appspot.com/14527043/diff/5001/environs/tools/simplestreams.go#newcode217
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/simplestreams.go#newcode220
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/simplestreams.go#newcode240
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/simplestreams.go#newcode292
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 constant...

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/14527043/

[Roger Peppe (rogpeppe)]

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/simplestreams.go
File environs/tools/simplestreams.go (right):

https://codereview.appspot.com/14527043/diff/5001/environs/tools/simplestreams.go#newcode240
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/simplestreams.go#newcode292
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 direc...

[Andrew Wilkins (axwalk)]

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/simplestreams.go
> File environs/tools/simplestreams.go (right):

https://codereview.appspot.com/14527043/diff/5001/environs/tools/simplestreams.go#newcode240
> 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/simplestreams.go#newcode292
> 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 th...

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/14527043/

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/14527043/

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/14527043/

[Andrew Wilkins (axwalk)]

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/simplestreams.go
> > File environs/tools/simplestreams.go (right):
> >
> >

https://codereview.appspot.com/14527043/diff/5001/environs/tools/simplestreams.go#newcode240
> > 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/simplestreams.go#newcode292
> > environs/tools/simplestreams.go:292: type ResolveFlag bool
> > On 2013/10/09 14:28:25, axw wrote:
> ...

=== modified file 'cmd/jujud/agent_test.go'
--- cmd/jujud/agent_test.go	2013-10-01 12:51:12 +0000
+++ cmd/jujud/agent_test.go	2013-10-16 05:27:02 +0000
@@ -192,7 +192,7 @@
 func (s *agentSuite) primeAgent(c *gc.C, tag, password string) (agent.Config, *coretools.Tools) {
 	stor := s.Conn.Environ.Storage()
 	agentTools := envtesting.PrimeTools(c, stor, s.DataDir(), version.Current)
-	err := envtools.WriteMetadata(coretools.List{agentTools}, true, stor)
+	err := envtools.MergeAndWriteMetadata(stor, coretools.List{agentTools})
 	c.Assert(err, gc.IsNil)
 	tools1, err := agenttools.ChangeAgentTools(s.DataDir(), tag, version.Current)
 	c.Assert(err, gc.IsNil)

=== modified file 'cmd/plugins/juju-metadata/toolsmetadata.go'
--- cmd/plugins/juju-metadata/toolsmetadata.go	2013-09-19 02:53:44 +0000
+++ cmd/plugins/juju-metadata/toolsmetadata.go	2013-10-16 05:27:02 +0000
@@ -12,9 +12,11 @@
 	"launchpad.net/juju-core/cmd"
 	"launchpad.net/juju-core/environs/config"
 	"launchpad.net/juju-core/environs/filestorage"
+	"launchpad.net/juju-core/environs/storage"
 	"launchpad.net/juju-core/environs/sync"
 	"launchpad.net/juju-core/environs/tools"
 	"launchpad.net/juju-core/provider/ec2/httpstorage"
+	coretools "launchpad.net/juju-core/tools"
 	"launchpad.net/juju-core/utils"
 	"launchpad.net/juju-core/version"
 )
@@ -42,7 +44,6 @@
 
 func (c *ToolsMetadataCommand) SetFlags(f *gnuflag.FlagSet) {
 	c.EnvCommandBase.SetFlags(f)
-	f.BoolVar(&c.fetch, "fetch", true, "fetch tools and compute content size and hash")
 	f.StringVar(&c.metadataDir, "d", "", "local directory in which to store metadata")
 }
 
@@ -73,5 +74,23 @@
 	if err != nil {
 		return err
 	}
-	return tools.WriteMetadata(toolsList, c.fetch, targetStorage)
+	return mergeAndWriteMetadata(targetStorage, toolsList)
+}
+
+// This is essentially the same as tools.MergeAndWriteMetadata, but also
+// resolves metadata for existing tools by fetching them and computing
+// size/sha256 locally.
+func mergeAndWriteMetadata(stor storage.Storage, toolsList coretools.List) error {
+	existing, err := tools.ReadMetadata(stor)
+	if err != nil {
+		return err
+	}
+	metadata := tools.MetadataFromTools(toolsList)
+	if metadata, err = tools.MergeMetadata(metadata, existing); err != nil {
+		return err
+	}
+	if err = tools.ResolveMetadata(stor, metadata); err != nil {
+		return err
+	}
+	return tools.WriteMetadata(stor, metadata)
 }

=== modified file 'cmd/plugins/juju-metadata/toolsmetadata_test.go'
--- cmd/plugins/juju-metadata/toolsmetadata_test.go	2013-09-30 19:52:11 +0000
+++ cmd/plugins/juju-metadata/toolsmetadata_test.go	2013-10-16 05:27:02 +0000
@@ -22,10 +22,12 @@
 	ttesting "launchpad.net/juju-core/environs/tools/testing"
 	"launchpad.net/juju-core/provider/dummy"
 	coretesting "launchpad.net/juju-core/testing"
+	"launchpad.net/juju-core/testing/testbase"
 	"launchpad.net/juju-core/version"
 )
 
 type ToolsMetadataSuite struct {
+	testbase.LoggingSuite
 	home *coretesting.FakeHome
 	env  environs.Environ
 }
@@ -33,7 +35,13 @@
 var _ = gc.Suite(&ToolsMetadataSuite{})
 
 func (s *ToolsMetadataSuite) SetUpTest(c *gc.C) {
+	s.LoggingSuite.SetUpTest(c)
 	s.home = coretesting.MakeSampleHome(c)
+	s.AddCleanup(func(*gc.C) {
+		s.home.Restore()
+		dummy.Reset()
+		loggo.ResetLoggers()
+	})
 	env, err := environs.PrepareFromName("erewhemos", configstore.NewMem())
 	c.Assert(err, gc.IsNil)
 	s.env = env
@@ -41,12 +49,6 @@
 	loggo.GetLogger("").SetLogLevel(loggo.INFO)
 }
 
-func (s *ToolsMetadataSuite) TearDownTest(c *gc.C) {
-	dummy.Reset()
-	loggo.ResetLoggers()
-	s.home.Restore()
-}
-
 var currentVersionStrings = []string{
 	// only these ones will make it into the JSON files.
 	version.Current.Number.String() + "-quantal-amd64",
@@ -66,13 +68,13 @@
 
 func makeExpectedOutputCommon() string {
 	expected := `Finding tools\.\.\.
-.*Fetching tools to generate hash: .*/tools/.*juju-1\.12\.0-precise-amd64\.tgz
-.*Fetching tools to generate hash: .*/tools/.*juju-1\.12\.0-precise-i386\.tgz
-.*Fetching tools to generate hash: .*/tools/.*juju-1\.12\.0-raring-amd64\.tgz
-.*Fetching tools to generate hash: .*/tools/.*juju-1\.12\.0-raring-i386\.tgz
-.*Fetching tools to generate hash: .*/tools/.*juju-1\.13\.0-precise-amd64\.tgz
+.*Fetching tools to generate hash: 1\.12\.0-precise-amd64
+.*Fetching tools to generate hash: 1\.12\.0-precise-i386
+.*Fetching tools to generate hash: 1\.12\.0-raring-amd64
+.*Fetching tools to generate hash: 1\.12\.0-raring-i386
+.*Fetching tools to generate hash: 1\.13\.0-precise-amd64
 `
-	f := ".*Fetching tools to generate hash: .*/tools/.*juju-%s\\.tgz\n"
+	f := ".*Fetching tools to generate hash: %s\n"
 	for _, v := range currentVersionStrings {
 		expected += fmt.Sprintf(f, regexp.QuoteMeta(v))
 	}
@@ -145,8 +147,8 @@
 	output := ctx.Stdout.(*bytes.Buffer).String()
 	expectedOutput := fmt.Sprintf(`
 Finding tools\.\.\.
-.*Fetching tools to generate hash: .*/tools/releases/juju-%s\.tgz
-.*Fetching tools to generate hash: .*/tools/releases/juju-%s\.tgz
+.*Fetching tools to generate hash: %s
+.*Fetching tools to generate hash: %s
 .*Writing tools/streams/v1/index\.json
 .*Writing tools/streams/v1/com\.ubuntu\.juju:released:tools\.json
 `[1:], regexp.QuoteMeta(versionStrings[0]), regexp.QuoteMeta(versionStrings[1]))

=== modified file 'environs/sync/sync.go'
--- environs/sync/sync.go	2013-10-10 11:40:54 +0000
+++ environs/sync/sync.go	2013-10-16 05:27:02 +0000
@@ -120,7 +120,7 @@
 	logger.Infof("generating tools metadata")
 	if !syncContext.DryRun {
 		targetTools = append(targetTools, missing...)
-		err = envtools.WriteMetadata(targetTools, true, targetStorage)
+		err = envtools.MergeAndWriteMetadata(targetStorage, targetTools)
 		if err != nil {
 			return err
 		}

=== modified file 'environs/testing/tools.go'
--- environs/testing/tools.go	2013-10-03 13:19:40 +0000
+++ environs/testing/tools.go	2013-10-16 05:27:02 +0000
@@ -112,16 +112,25 @@
 
 // UploadFakeToolsVersions puts fake tools in the supplied storage for the supplied versions.
 func UploadFakeToolsVersions(stor storage.Storage, versions ...version.Binary) ([]*coretools.Tools, error) {
+	// Leave existing tools alone.
+	existingTools := make(map[version.Binary]*coretools.Tools)
+	existing, _ := envtools.ReadList(stor, 1, -1)
+	for _, tools := range existing {
+		existingTools[tools.Version] = tools
+	}
 	var agentTools coretools.List = make(coretools.List, len(versions))
 	for i, version := range versions {
-		t, err := uploadFakeToolsVersion(stor, version)
-		if err != nil {
-			return nil, err
+		if tools, ok := existingTools[version]; ok {
+			agentTools[i] = tools
+		} else {
+			t, err := uploadFakeToolsVersion(stor, version)
+			if err != nil {
+				return nil, err
+			}
+			agentTools[i] = t
 		}
-		agentTools[i] = t
 	}
-	err := envtools.WriteMetadata(agentTools, true, stor)
-	if err != nil {
+	if err := envtools.MergeAndWriteMetadata(stor, agentTools); err != nil {
 		return nil, err
 	}
 	return agentTools, nil
@@ -144,7 +153,7 @@
 		}
 		agentTools[i] = t
 	}
-	err := envtools.WriteMetadata(agentTools, true, stor)
+	err := envtools.MergeAndWriteMetadata(stor, agentTools)
 	if err != nil {
 		panic(err)
 	}
@@ -182,6 +191,7 @@
 
 // RemoveFakeTools deletes the fake tools from the supplied storage.
 func RemoveFakeTools(c *gc.C, stor storage.Storage) {
+	c.Logf("removing fake tools")
 	toolsVersion := version.Current
 	name := envtools.StorageName(toolsVersion)
 	err := stor.Remove(name)

=== modified file 'environs/tools/simplestreams.go'
--- environs/tools/simplestreams.go	2013-10-10 11:40:54 +0000
+++ environs/tools/simplestreams.go	2013-10-16 05:27:02 +0000
@@ -12,7 +12,6 @@
 	"fmt"
 	"hash"
 	"io"
-	"net/http"
 	"strings"
 	"time"
 
@@ -138,7 +137,7 @@
 	Arch     string `json:"arch"`
 	Size     int64  `json:"size"`
 	Path     string `json:"path"`
-	FullPath string `json:"-,omitempty"`
+	FullPath string `json:"-"`
 	FileType string `json:"ftype"`
 	SHA256   string `json:"sha256"`
 }
@@ -147,6 +146,16 @@
 	return fmt.Sprintf("%+v", *t)
 }
 
+// binary returns the tools metadata's binary version,
+// which may be used for map lookup.
+func (t *ToolsMetadata) binary() version.Binary {
+	return version.Binary{
+		Number: version.MustParse(t.Version),
+		Series: t.Release,
+		Arch:   t.Arch,
+	}
+}
+
 func (t *ToolsMetadata) productId() (string, error) {
 	seriesVersion, err := simplestreams.SeriesVersion(t.Release)
 	if err != nil {
@@ -199,10 +208,10 @@
 func appendMatchingTools(source simplestreams.DataSource, matchingTools []interface{},
 	tools map[string]interface{}, cons simplestreams.LookupConstraint) []interface{} {
 
-	toolsMap := make(map[string]*ToolsMetadata, len(matchingTools))
+	toolsMap := make(map[version.Binary]*ToolsMetadata, len(matchingTools))
 	for _, val := range matchingTools {
 		tm := val.(*ToolsMetadata)
-		toolsMap[fmt.Sprintf("%s-%s-%s", tm.Release, tm.Version, tm.Arch)] = tm
+		toolsMap[tm.binary()] = tm
 	}
 	for _, val := range tools {
 		tm := val.(*ToolsMetadata)
@@ -227,7 +236,7 @@
 				}
 			}
 		}
-		if _, ok := toolsMap[fmt.Sprintf("%s-%s-%s", tm.Release, tm.Version, tm.Arch)]; !ok {
+		if _, ok := toolsMap[tm.binary()]; !ok {
 			tm.FullPath, _ = source.URL(tm.Path)
 			matchingTools = append(matchingTools, tm)
 		}
@@ -240,101 +249,142 @@
 	Data []byte
 }
 
-func WriteMetadata(toolsList coretools.List, fetch bool, metadataStore storage.Storage) error {
-	// Read any existing metadata so we can merge the new tools metadata with what's there.
-	// The metadata from toolsList is already present, the existing data is overwritten.
-	dataSource := storage.NewStorageSimpleStreamsDataSource(metadataStore, "tools")
-	toolsConstraint, err := makeToolsConstraint(simplestreams.CloudSpec{}, -1, -1, coretools.Filter{})
-	if err != nil {
-		return err
-	}
-	existingMetadata, err := Fetch([]simplestreams.DataSource{dataSource}, simplestreams.DefaultIndexPath, toolsConstraint, false)
-	if err != nil && !errors.IsNotFoundError(err) {
-		return err
-	}
-	newToolsVersions := make(map[string]bool)
-	for _, tool := range toolsList {
-		newToolsVersions[tool.Version.String()] = true
-	}
-	// Merge in existing records.
-	for _, toolsMetadata := range existingMetadata {
-		vers := version.Binary{version.MustParse(toolsMetadata.Version), toolsMetadata.Release, toolsMetadata.Arch}
-		if _, ok := newToolsVersions[vers.String()]; ok {
-			continue
-		}
-		tool := &coretools.Tools{
-			Version: vers,
-			SHA256:  toolsMetadata.SHA256,
-			Size:    toolsMetadata.Size,
-		}
-		toolsList = append(toolsList, tool)
-	}
-	metadataInfo, err := generateMetadata(toolsList, fetch)
-	if err != nil {
-		return err
-	}
-	for _, md := range metadataInfo {
-		logger.Infof("Writing %s", "tools/"+md.Path)
-		err = metadataStore.Put("tools/"+md.Path, bytes.NewReader(md.Data), int64(len(md.Data)))
-		if err != nil {
-			return err
-		}
-	}
-	return nil
-}
-
-func generateMetadata(toolsList coretools.List, fetch bool) ([]MetadataFile, error) {
+// MetadataFromTools returns a tools metadata list derived from the
+// given tools list. The size and sha256 will not be computed if
+// missing.
+func MetadataFromTools(toolsList coretools.List) []*ToolsMetadata {
 	metadata := make([]*ToolsMetadata, len(toolsList))
 	for i, t := range toolsList {
-		var size int64
-		var sha256hex string
-		var err error
-		if fetch && t.Size == 0 {
-			logger.Infof("Fetching tools to generate hash: %v", t.URL)
-			var sha256hash hash.Hash
-			size, sha256hash, err = fetchToolsHash(t.URL)
-			if err != nil {
-				return nil, err
-			}
-			sha256hex = fmt.Sprintf("%x", sha256hash.Sum(nil))
-		} else {
-			size = t.Size
-			sha256hex = t.SHA256
-		}
-
 		path := fmt.Sprintf("releases/juju-%s-%s-%s.tgz", t.Version.Number, t.Version.Series, t.Version.Arch)
 		metadata[i] = &ToolsMetadata{
 			Release:  t.Version.Series,
 			Version:  t.Version.Number.String(),
 			Arch:     t.Version.Arch,
+			FullPath: t.URL,
 			Path:     path,
 			FileType: "tar.gz",
-			Size:     size,
-			SHA256:   sha256hex,
-		}
-	}
-
+			Size:     t.Size,
+			SHA256:   t.SHA256,
+		}
+	}
+	return metadata
+}
+
+// ResolveMetadata resolves incomplete metadata
+// by fetching the tools from storage and computing
+// the size and hash locally.
+func ResolveMetadata(stor storage.StorageReader, metadata []*ToolsMetadata) error {
+	for _, md := range metadata {
+		if md.Size != 0 {
+			continue
+		}
+		binary := md.binary()
+		logger.Infof("Fetching tools to generate hash: %v", binary)
+		var sha256hash hash.Hash
+		size, sha256hash, err := fetchToolsHash(stor, binary)
+		if err != nil {
+			return err
+		}
+		md.Size = size
+		md.SHA256 = fmt.Sprintf("%x", sha256hash.Sum(nil))
+	}
+	return nil
+}
+
+// MergeMetadata merges the given tools metadata.
+// If metadata for the same tools version exists in both lists,
+// an entry with non-empty size/SHA256 takes precedence; if
+// the two entries have different sizes/hashes, then an error is
+// returned.
+func MergeMetadata(tmlist1, tmlist2 []*ToolsMetadata) ([]*ToolsMetadata, error) {
+	merged := make(map[version.Binary]*ToolsMetadata)
+	for _, tm := range tmlist1 {
+		merged[tm.binary()] = tm
+	}
+	for _, tm := range tmlist2 {
+		binary := tm.binary()
+		if existing, ok := merged[binary]; ok {
+			if tm.Size != 0 {
+				if existing.Size == 0 {
+					merged[binary] = tm
+				} else if existing.Size != tm.Size || existing.SHA256 != tm.SHA256 {
+					return nil, fmt.Errorf(
+						"metadata mismatch for %s: sizes=(%v,%v) sha256=(%v,%v)",
+						binary.String(),
+						existing.Size, tm.Size,
+						existing.SHA256, tm.SHA256,
+					)
+				}
+			}
+		} else {
+			merged[binary] = tm
+		}
+	}
+	list := make([]*ToolsMetadata, 0, len(merged))
+	for _, metadata := range merged {
+		list = append(list, metadata)
+	}
+	return list, nil
+}
+
+// ReadMetadata returns the tools metadata from the given storage.
+func ReadMetadata(store storage.StorageReader) ([]*ToolsMetadata, error) {
+	dataSource := storage.NewStorageSimpleStreamsDataSource(store, "tools")
+	toolsConstraint, err := makeToolsConstraint(simplestreams.CloudSpec{}, -1, -1, coretools.Filter{})
+	if err != nil {
+		return nil, err
+	}
+	metadata, err := Fetch([]simplestreams.DataSource{dataSource}, simplestreams.DefaultIndexPath, toolsConstraint, false)
+	if err != nil && !errors.IsNotFoundError(err) {
+		return nil, err
+	}
+	return metadata, nil
+}
+
+// WriteMetadata writes the given tools metadata to the given storage.
+func WriteMetadata(stor storage.Storage, metadata []*ToolsMetadata) error {
 	index, products, err := MarshalToolsMetadataJSON(metadata, time.Now())
 	if err != nil {
-		return nil, err
+		return err
 	}
-	objects := []MetadataFile{
+	metadataInfo := []MetadataFile{
 		{simplestreams.UnsignedIndex, index},
 		{ProductMetadataPath, products},
 	}
-	return objects, nil
-}
-
-// fetchToolsHash fetches the file at the specified URL,
-// and calculates its size in bytes and computes a SHA256
-// hash of its contents.
-func fetchToolsHash(url string) (size int64, sha256hash hash.Hash, err error) {
-	resp, err := http.Get(url)
+	for _, md := range metadataInfo {
+		logger.Infof("Writing %s", "tools/"+md.Path)
+		err = stor.Put("tools/"+md.Path, bytes.NewReader(md.Data), int64(len(md.Data)))
+		if err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
+// MergeAndWriteMetadata reads the existing metadata from storage (if any),
+// and merges it with metadata generated from the given tools list. The
+// resulting metadata is written to storage.
+func MergeAndWriteMetadata(stor storage.Storage, tools coretools.List) error {
+	existing, err := ReadMetadata(stor)
+	if err != nil {
+		return err
+	}
+	metadata := MetadataFromTools(tools)
+	if metadata, err = MergeMetadata(metadata, existing); err != nil {
+		return err
+	}
+	return WriteMetadata(stor, metadata)
+}
+
+// fetchToolsHash fetches the tools from storage and calculates
+// its size in bytes and computes a SHA256 hash of its contents.
+func fetchToolsHash(stor storage.StorageReader, ver version.Binary) (size int64, sha256hash hash.Hash, err error) {
+	r, err := storage.Get(stor, StorageName(ver))
 	if err != nil {
 		return 0, nil, err
 	}
+	defer r.Close()
 	sha256hash = sha256.New()
-	size, err = io.Copy(sha256hash, resp.Body)
-	resp.Body.Close()
+	size, err = io.Copy(sha256hash, r)
 	return size, sha256hash, err
 }

=== modified file 'environs/tools/simplestreams_test.go'
--- environs/tools/simplestreams_test.go	2013-10-10 11:40:54 +0000
+++ environs/tools/simplestreams_test.go	2013-10-16 05:27:02 +0000
@@ -7,8 +7,8 @@
 	"bytes"
 	"flag"
 	"fmt"
+	"io"
 	"net/http"
-	"path/filepath"
 	"reflect"
 	"strings"
 	"testing"
@@ -20,8 +20,10 @@
 	"launchpad.net/juju-core/environs/jujutest"
 	"launchpad.net/juju-core/environs/simplestreams"
 	sstesting "launchpad.net/juju-core/environs/simplestreams/testing"
+	"launchpad.net/juju-core/environs/storage"
 	"launchpad.net/juju-core/environs/tools"
 	ttesting "launchpad.net/juju-core/environs/tools/testing"
+	"launchpad.net/juju-core/testing/testbase"
 	coretools "launchpad.net/juju-core/tools"
 	"launchpad.net/juju-core/version"
 )
@@ -280,14 +282,9 @@
 	c.Assert(toolsMetadata[0], gc.DeepEquals, expectedMetadata)
 }
 
-func assertMetadataMatches(c *gc.C, toolList coretools.List, metadata []*tools.ToolsMetadata) {
+func assertMetadataMatches(c *gc.C, storageDir string, toolList coretools.List, metadata []*tools.ToolsMetadata) {
 	var expectedMetadata []*tools.ToolsMetadata = make([]*tools.ToolsMetadata, len(toolList))
 	for i, tool := range toolList {
-		if tool.URL != "" {
-			size, sha256 := ttesting.SHA256sum(c, tool.URL)
-			tool.SHA256 = sha256
-			tool.Size = size
-		}
 		expectedMetadata[i] = &tools.ToolsMetadata{
 			Release:  tool.Version.Series,
 			Version:  tool.Version.Number.String(),
@@ -316,10 +313,10 @@
 	dir := c.MkDir()
 	writer, err := filestorage.NewFileStorageWriter(dir, filestorage.UseDefaultTmpDir)
 	c.Assert(err, gc.IsNil)
-	err = tools.WriteMetadata(toolsList, false, writer)
+	err = tools.MergeAndWriteMetadata(writer, toolsList)
 	c.Assert(err, gc.IsNil)
 	metadata := ttesting.ParseMetadata(c, dir)
-	assertMetadataMatches(c, toolsList, metadata)
+	assertMetadataMatches(c, dir, toolsList, metadata)
 }
 
 func (s *simplestreamsSuite) TestWriteMetadata(c *gc.C) {
@@ -338,15 +335,16 @@
 			SHA256:  "abcd",
 		}, {
 			Version: version.MustParseBinary("2.0.1-raring-amd64"),
-			URL:     "file://" + filepath.Join(dir, "tools/releases/juju-2.0.1-raring-amd64.tgz"),
+			// The URL is not used for generating metadata.
+			URL: "bogus://",
 		},
 	}
 	writer, err := filestorage.NewFileStorageWriter(dir, filestorage.UseDefaultTmpDir)
 	c.Assert(err, gc.IsNil)
-	err = tools.WriteMetadata(toolsList, true, writer)
+	err = tools.MergeAndWriteMetadata(writer, toolsList)
 	c.Assert(err, gc.IsNil)
 	metadata := ttesting.ParseMetadata(c, dir)
-	assertMetadataMatches(c, toolsList, metadata)
+	assertMetadataMatches(c, dir, toolsList, metadata)
 }
 
 func (s *simplestreamsSuite) TestWriteMetadataMergeWithExisting(c *gc.C) {
@@ -364,7 +362,7 @@
 	}
 	writer, err := filestorage.NewFileStorageWriter(dir, filestorage.UseDefaultTmpDir)
 	c.Assert(err, gc.IsNil)
-	err = tools.WriteMetadata(existingToolsList, true, writer)
+	err = tools.MergeAndWriteMetadata(writer, existingToolsList)
 	c.Assert(err, gc.IsNil)
 	newToolsList := coretools.List{
 		existingToolsList[0],
@@ -374,11 +372,11 @@
 			SHA256:  "def",
 		},
 	}
-	err = tools.WriteMetadata(newToolsList, true, writer)
+	err = tools.MergeAndWriteMetadata(writer, newToolsList)
 	c.Assert(err, gc.IsNil)
 	requiredToolsList := append(existingToolsList, newToolsList[1])
 	metadata := ttesting.ParseMetadata(c, dir)
-	assertMetadataMatches(c, requiredToolsList, metadata)
+	assertMetadataMatches(c, dir, requiredToolsList, metadata)
 }
 
 type productSpecSuite struct{}
@@ -480,6 +478,219 @@
 	c.Assert(item.(*tools.ToolsMetadata).Size, gc.Equals, int64(9223372036854775807))
 }
 
+type metadataHelperSuite struct {
+	testbase.LoggingSuite
+}
+
+var _ = gc.Suite(&metadataHelperSuite{})
+
+func (*metadataHelperSuite) TestMetadataFromTools(c *gc.C) {
+	metadata := tools.MetadataFromTools(nil)
+	c.Assert(metadata, gc.HasLen, 0)
+
+	toolsList := coretools.List{{
+		Version: version.MustParseBinary("1.2.3-precise-amd64"),
+		Size:    123,
+		SHA256:  "abc",
+	}, {
+		Version: version.MustParseBinary("2.0.1-raring-amd64"),
+		URL:     "file:///tmp/releases/juju-2.0.1-raring-amd64.tgz",
+		Size:    456,
+		SHA256:  "xyz",
+	}}
+	metadata = tools.MetadataFromTools(toolsList)
+	c.Assert(metadata, gc.HasLen, len(toolsList))
+	for i, t := range toolsList {
+		md := metadata[i]
+		c.Assert(md.Release, gc.Equals, t.Version.Series)
+		c.Assert(md.Version, gc.Equals, t.Version.Number.String())
+		c.Assert(md.Arch, gc.Equals, t.Version.Arch)
+		c.Assert(md.FullPath, gc.Equals, t.URL)
+		c.Assert(md.Path, gc.Equals, tools.StorageName(t.Version)[len("tools/"):])
+		c.Assert(md.FileType, gc.Equals, "tar.gz")
+		c.Assert(md.Size, gc.Equals, t.Size)
+		c.Assert(md.SHA256, gc.Equals, t.SHA256)
+	}
+}
+
+type countingStorage struct {
+	storage.StorageReader
+	counter int
+}
+
+func (c *countingStorage) Get(name string) (io.ReadCloser, error) {
+	c.counter++
+	return c.StorageReader.Get(name)
+}
+
+func (*metadataHelperSuite) TestResolveMetadata(c *gc.C) {
+	var versionStrings = []string{"1.2.3-precise-amd64"}
+	dir := c.MkDir()
+	ttesting.MakeTools(c, dir, "releases", versionStrings)
+	toolsList := coretools.List{{
+		Version: version.MustParseBinary(versionStrings[0]),
+		Size:    123,
+		SHA256:  "abc",
+	}}
+
+	stor, err := filestorage.NewFileStorageReader(dir)
+	c.Assert(err, gc.IsNil)
+	err = tools.ResolveMetadata(stor, nil)
+	c.Assert(err, gc.IsNil)
+
+	// We already have size/sha256, so ensure that storage isn't consulted.
+	countingStorage := &countingStorage{StorageReader: stor}
+	metadata := tools.MetadataFromTools(toolsList)
+	err = tools.ResolveMetadata(countingStorage, metadata)
+	c.Assert(err, gc.IsNil)
+	c.Assert(countingStorage.counter, gc.Equals, 0)
+
+	// Now clear size/sha256, and check that it is called, and
+	// the size/sha256 sum are updated.
+	metadata[0].Size = 0
+	metadata[0].SHA256 = ""
+	err = tools.ResolveMetadata(countingStorage, metadata)
+	c.Assert(err, gc.IsNil)
+	c.Assert(countingStorage.counter, gc.Equals, 1)
+	c.Assert(metadata[0].Size, gc.Not(gc.Equals), 0)
+	c.Assert(metadata[0].SHA256, gc.Not(gc.Equals), "")
+}
+
+func (*metadataHelperSuite) TestMergeMetadata(c *gc.C) {
+	md1 := &tools.ToolsMetadata{
+		Release: "precise",
+		Version: "1.2.3",
+		Arch:    "amd64",
+		Path:    "path1",
+	}
+	md2 := &tools.ToolsMetadata{
+		Release: "precise",
+		Version: "1.2.3",
+		Arch:    "amd64",
+		Path:    "path2",
+	}
+	md3 := &tools.ToolsMetadata{
+		Release: "raring",
+		Version: "1.2.3",
+		Arch:    "amd64",
+		Path:    "path3",
+	}
+
+	withSize := func(md *tools.ToolsMetadata, size int64) *tools.ToolsMetadata {
+		clone := *md
+		clone.Size = size
+		return &clone
+	}
+	withSHA256 := func(md *tools.ToolsMetadata, sha256 string) *tools.ToolsMetadata {
+		clone := *md
+		clone.SHA256 = sha256
+		return &clone
+	}
+
+	type mdlist []*tools.ToolsMetadata
+	type test struct {
+		name             string
+		lhs, rhs, merged []*tools.ToolsMetadata
+		err              string
+	}
+	tests := []test{{
+		name:   "non-empty lhs, empty rhs",
+		lhs:    mdlist{md1},
+		rhs:    nil,
+		merged: mdlist{md1},
+	}, {
+		name:   "empty lhs, non-empty rhs",
+		lhs:    nil,
+		rhs:    mdlist{md2},
+		merged: mdlist{md2},
+	}, {
+		name:   "identical lhs, rhs",
+		lhs:    mdlist{md1},
+		rhs:    mdlist{md1},
+		merged: mdlist{md1},
+	}, {
+		name:   "same tools in lhs and rhs, neither have size: prefer lhs",
+		lhs:    mdlist{md1},
+		rhs:    mdlist{md2},
+		merged: mdlist{md1},
+	}, {
+		name:   "same tools in lhs and rhs, only lhs has a size: prefer lhs",
+		lhs:    mdlist{withSize(md1, 123)},
+		rhs:    mdlist{md2},
+		merged: mdlist{withSize(md1, 123)},
+	}, {
+		name:   "same tools in lhs and rhs, only rhs has a size: prefer rhs",
+		lhs:    mdlist{md1},
+		rhs:    mdlist{withSize(md2, 123)},
+		merged: mdlist{withSize(md2, 123)},
+	}, {
+		name:   "same tools in lhs and rhs, both have the same size: prefer lhs",
+		lhs:    mdlist{withSize(md1, 123)},
+		rhs:    mdlist{withSize(md2, 123)},
+		merged: mdlist{withSize(md1, 123)},
+	}, {
+		name: "same tools in lhs and rhs, both have different sizes: error",
+		lhs:  mdlist{withSize(md1, 123)},
+		rhs:  mdlist{withSize(md2, 456)},
+		err:  "metadata mismatch for 1\\.2\\.3-precise-amd64: sizes=\\(123,456\\) sha256=\\(,\\)",
+	}, {
+		name: "same tools in lhs and rhs, both have same size but different sha256: error",
+		lhs:  mdlist{withSHA256(withSize(md1, 123), "a")},
+		rhs:  mdlist{withSHA256(withSize(md2, 123), "b")},
+		err:  "metadata mismatch for 1\\.2\\.3-precise-amd64: sizes=\\(123,123\\) sha256=\\(a,b\\)",
+	}, {
+		name:   "lhs is a proper superset of rhs: union of lhs and rhs",
+		lhs:    mdlist{md1, md3},
+		rhs:    mdlist{md1},
+		merged: mdlist{md1, md3},
+	}, {
+		name:   "rhs is a proper superset of lhs: union of lhs and rhs",
+		lhs:    mdlist{md1},
+		rhs:    mdlist{md1, md3},
+		merged: mdlist{md1, md3},
+	}}
+	for i, test := range tests {
+		c.Logf("test %d: %s", i, test.name)
+		merged, err := tools.MergeMetadata(test.lhs, test.rhs)
+		if test.err == "" {
+			c.Assert(err, gc.IsNil)
+			c.Assert(merged, gc.DeepEquals, test.merged)
+		} else {
+			c.Assert(err, gc.ErrorMatches, test.err)
+			c.Assert(merged, gc.IsNil)
+		}
+	}
+}
+
+func (*metadataHelperSuite) TestReadWriteMetadata(c *gc.C) {
+	metadata := []*tools.ToolsMetadata{{
+		Release: "precise",
+		Version: "1.2.3",
+		Arch:    "amd64",
+		Path:    "path1",
+	}, {
+		Release: "raring",
+		Version: "1.2.3",
+		Arch:    "amd64",
+		Path:    "path2",
+	}}
+
+	stor, err := filestorage.NewFileStorageWriter(c.MkDir(), filestorage.UseDefaultTmpDir)
+	c.Assert(err, gc.IsNil)
+	out, err := tools.ReadMetadata(stor)
+	c.Assert(out, gc.HasLen, 0)
+	c.Assert(err, gc.IsNil) // non-existence is not an error
+	err = tools.WriteMetadata(stor, metadata)
+	c.Assert(err, gc.IsNil)
+	out, err = tools.ReadMetadata(stor)
+	for _, md := range out {
+		// FullPath is set by ReadMetadata.
+		c.Assert(md.FullPath, gc.Not(gc.Equals), "")
+		md.FullPath = ""
+	}
+	c.Assert(out, gc.DeepEquals, metadata)
+}
+
 type signedSuite struct {
 	origKey string
 }

=== modified file 'environs/tools/tools.go'
--- environs/tools/tools.go	2013-09-26 23:07:24 +0000
+++ environs/tools/tools.go	2013-10-16 05:27:02 +0000
@@ -121,13 +121,8 @@
 	}
 	list = make(coretools.List, len(toolsMetadata))
 	for i, metadata := range toolsMetadata {
-		binary := version.Binary{
-			Number: version.MustParse(metadata.Version),
-			Arch:   metadata.Arch,
-			Series: metadata.Release,
-		}
 		list[i] = &coretools.Tools{
-			Version: binary,
+			Version: metadata.binary(),
 			URL:     metadata.FullPath,
 			Size:    metadata.Size,
 			SHA256:  metadata.SHA256,

=== modified file 'testing/targz.go'
--- testing/targz.go	2013-09-25 00:18:35 +0000
+++ testing/targz.go	2013-10-16 05:27:02 +0000
@@ -11,7 +11,6 @@
 	"fmt"
 	"io"
 	"os"
-	"time"
 )
 
 // TarFile represents a file to be archived.
@@ -33,17 +32,16 @@
 	if ftype == 0 {
 		panic(fmt.Errorf("unexpected mode %v", mode))
 	}
+	// NOTE: Do not set attributes (e.g. times) dynamically, as various
+	// tests expect the contents of fake tools archives to be unchanging.
 	return &TarFile{
 		Header: tar.Header{
-			Typeflag:   ftype,
-			Name:       name,
-			Size:       int64(len(contents)),
-			Mode:       int64(mode & 0777),
-			ModTime:    time.Now(),
-			AccessTime: time.Now(),
-			ChangeTime: time.Now(),
-			Uname:      "ubuntu",
-			Gname:      "ubuntu",
+			Typeflag: ftype,
+			Name:     name,
+			Size:     int64(len(contents)),
+			Mode:     int64(mode & 0777),
+			Uname:    "ubuntu",
+			Gname:    "ubuntu",
 		},
 		Contents: contents,
 	}

=== modified file 'worker/upgrader/upgrader_test.go'
--- worker/upgrader/upgrader_test.go	2013-10-02 16:28:16 +0000
+++ worker/upgrader/upgrader_test.go	2013-10-16 05:27:02 +0000
@@ -83,7 +83,7 @@
 	c.Assert(err, gc.IsNil)
 	stor := s.Conn.Environ.Storage()
 	agentTools := envtesting.PrimeTools(c, stor, s.DataDir(), vers)
-	err = envtools.WriteMetadata(coretools.List{agentTools}, true, stor)
+	err = envtools.MergeAndWriteMetadata(stor, coretools.List{agentTools})
 	_, err = s.machine.AgentTools()
 	c.Assert(err, jc.Satisfies, errors.IsNotFoundError)
 
@@ -119,7 +119,7 @@
 	oldTools := envtesting.PrimeTools(c, stor, s.DataDir(), version.MustParseBinary("5.4.3-precise-amd64"))
 	newTools := envtesting.AssertUploadFakeToolsVersions(
 		c, stor, version.MustParseBinary("5.4.5-precise-amd64"))[0]
-	err := envtools.WriteMetadata(coretools.List{oldTools, newTools}, true, stor)
+	err := envtools.MergeAndWriteMetadata(stor, coretools.List{oldTools, newTools})
 	c.Assert(err, gc.IsNil)
 	err = statetesting.SetAgentVersion(s.State, newTools.Version.Number)
 	c.Assert(err, gc.IsNil)
@@ -147,7 +147,7 @@
 	oldTools := envtesting.PrimeTools(c, stor, s.DataDir(), version.MustParseBinary("5.4.3-precise-amd64"))
 	newTools := envtesting.AssertUploadFakeToolsVersions(
 		c, stor, version.MustParseBinary("5.4.5-precise-amd64"))[0]
-	err := envtools.WriteMetadata(coretools.List{oldTools, newTools}, true, stor)
+	err := envtools.MergeAndWriteMetadata(stor, coretools.List{oldTools, newTools})
 	c.Assert(err, gc.IsNil)
 	err = statetesting.SetAgentVersion(s.State, newTools.Version.Number)
 	c.Assert(err, gc.IsNil)
@@ -202,7 +202,7 @@
 	}
 	stor := s.Conn.Environ.Storage()
 	newTools := envtesting.PrimeTools(c, stor, s.DataDir(), version.MustParseBinary("5.4.3-precise-amd64"))
-	err := envtools.WriteMetadata(coretools.List{newTools}, true, stor)
+	err := envtools.MergeAndWriteMetadata(stor, coretools.List{newTools})
 	c.Assert(err, gc.IsNil)
 	ugErr := &upgrader.UpgradeReadyError{
 		AgentName: "anAgent",