Merge lp:~axwalk/juju-core/lp1233924-simplestreams-getmetadata-fallback into lp:~go-bot/juju-core/trunk

[Andrew Wilkins]

environs/simplestreams: fix GetMetadata

GetMetadata was dropping out if a datasource
returned no matching product IDs, without an
error. I've changed it to check that a non-
empty list of products is returned before
bailing out.

Fixes #1233924

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

environs/simplestreams: fix GetMetadata

GetMetadata was dropping out if a datasource
returned no matching product IDs, without an
error. I've changed it to check that a non-
empty list of products is returned before
bailing out.

Fixes #1233924

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

[Andrew Wilkins (axwalk)]

Reviewers: mp+188758_code.launchpad.net,

Message:
Please take a look.

Description:
environs/simplestreams: fix GetMetadata

GetMetadata was dropping out if a datasource
returned no matching product IDs, without an
error. I've changed it to check that a non-
empty list of products is returned before
bailing out.

Fixes #1233924

https://code.launchpad.net/~axwalk/juju-core/lp1233924-simplestreams-getmetadata-fallback/+merge/188758

(do not edit description out of merge proposal)

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

Affected files (+71, -2 lines):
   A [revision details]
   M environs/simplestreams/simplestreams.go
   M environs/simplestreams/simplestreams_test.go
   M environs/simplestreams/testing/testing.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: tarmac-20131001213504-4ynrkuya2i22q87q
+New revision: <email address hidden>

Index: environs/simplestreams/simplestreams.go
=== modified file 'environs/simplestreams/simplestreams.go'
--- environs/simplestreams/simplestreams.go 2013-09-27 07:05:45 +0000
+++ environs/simplestreams/simplestreams.go 2013-10-02 03:04:13 +0000
@@ -17,6 +17,7 @@
   "io/ioutil"
   "net/http"
   "os"
+ "path"
   "reflect"
   "sort"
   "strings"
@@ -25,7 +26,6 @@
   "launchpad.net/loggo"

   "launchpad.net/juju-core/errors"
- "path"
  )

  var logger = loggo.GetLogger("juju.environs.simplestreams")
@@ -403,7 +403,7 @@
    if err != nil && len(items) == 0 && !onlySigned {
     items, err = getMaybeSignedMetadata(source, baseIndexPath, cons, false,
params)
    }
- if err == nil {
+ if err == nil && len(items) != 0 {
     break
    }
   }

Index: environs/simplestreams/simplestreams_test.go
=== modified file 'environs/simplestreams/simplestreams_test.go'
--- environs/simplestreams/simplestreams_test.go 2013-09-26 04:11:52 +0000
+++ environs/simplestreams/simplestreams_test.go 2013-10-02 03:04:13 +0000
@@ -9,9 +9,11 @@
   "testing"

   gc "launchpad.net/gocheck"
+ "launchpad.net/loggo"

   "launchpad.net/juju-core/environs/simplestreams"
   sstesting "launchpad.net/juju-core/environs/simplestreams/testing"
+ jc "launchpad.net/juju-core/testing/checkers"
  )

  func Test(t *testing.T) {
@@ -284,6 +286,47 @@
   c.Check(dotOFormats, gc.DeepEquals,
simplestreams.IndexMetadataSlice{array[0], array[2]})
  }

+func (s *simplestreamsSuite) TestGetMetadataNoMatching(c *gc.C) {
+ sources := []simplestreams.DataSource{
+ simplestreams.NewURLDataSource("test:/daily",
simplestreams.VerifySSLHostnames),
+ simplestreams.NewURLDataSource("test:/daily",
simplestreams.VerifySSLHostnames),
+ }
+ params := simplestreams.ValueParams{DataType: "image-ids"}
+ constraint := sstesting.NewTestConstraint(simplestreams.LookupParams{
+ CloudSpec: simplestreams.CloudSpec{
+ Region: "us-east-1",
+ Endpoint: "https://ec2.us-east-1.amazonaws.com",
+ },
+ Series: []string{"precise"}, // never match
+ Arches: []string{"arm"},
+ })
+
+ tw := &loggo.TestWriter{}
+ c.Assert(loggo.RegisterWriter("filter-tester", tw, loggo.DEB...

[Tim Penhey (thumper)]

https://codereview.appspot.com/14218044/diff/1/environs/simplestreams/simplestreams_test.go
File environs/simplestreams/simplestreams_test.go (right):

https://codereview.appspot.com/14218044/diff/1/environs/simplestreams/simplestreams_test.go#newcode300
environs/simplestreams/simplestreams_test.go:300: Series:
[]string{"precise"}, // never match
What is the never match? precise or arm? Also, perhaps better to give a
region that doesn't exist? Someone may add arm at some stage. Would
that work?

https://codereview.appspot.com/14218044/diff/1/environs/simplestreams/simplestreams_test.go#newcode326
environs/simplestreams/simplestreams_test.go:326: messages =
append(messages, messages...)
I agree with our IRC chat, ideally we would have a simple mock
datasource. What about...

type countingSource struct {
   simplestreams.DataSource
   count int
}

func (s *countingSource) URL(path string) (string, error) {
   s.count++
   return s.DataSource.URL(path)
}

Then have sources defined by:

   first :=
&countingSource{DataSource:simplestreams.NewURLDataSource("test:/daily",
simplestreams.VerifySSLHostnames)}
   second :=
&countingSource{DataSource:simplestreams.NewURLDataSource("test:/daily",
simplestreams.VerifySSLHostnames)}
   sources := []simplestream.DataSource{first, second}

   // do stuff

   c.Check(first.count, gc.Equals, 1)
   c.Check(second.count, gc.Equals, 1)

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

[Andrew Wilkins (axwalk)]

Please take a look.

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

[Andrew Wilkins (axwalk)]

https://codereview.appspot.com/14218044/diff/1/environs/simplestreams/simplestreams_test.go
File environs/simplestreams/simplestreams_test.go (right):

https://codereview.appspot.com/14218044/diff/1/environs/simplestreams/simplestreams_test.go#newcode300
environs/simplestreams/simplestreams_test.go:300: Series:
[]string{"precise"}, // never match
On 2013/10/02 04:02:00, thumper wrote:
> What is the never match? precise or arm? Also, perhaps better to give
a region
> that doesn't exist? Someone may add arm at some stage. Would that
work?

precise, which isn't in the daily stream I added to testing. I can't use
a non-existent region, as that causes GetMetadata to error, which we
don't want.

The series are validated, but architectures aren't. I'll put in an
invalid arch.

https://codereview.appspot.com/14218044/diff/1/environs/simplestreams/simplestreams_test.go#newcode326
environs/simplestreams/simplestreams_test.go:326: messages =
append(messages, messages...)
On 2013/10/02 04:02:00, thumper wrote:
> I agree with our IRC chat, ideally we would have a simple mock
datasource. What
> about...

> type countingSource struct {
> simplestreams.DataSource
> count int
> }

> func (s *countingSource) URL(path string) (string, error) {
> s.count++
> return s.DataSource.URL(path)
> }

> Then have sources defined by:

> first :=

&countingSource{DataSource:simplestreams.NewURLDataSource("test:/daily",
> simplestreams.VerifySSLHostnames)}
> second :=

&countingSource{DataSource:simplestreams.NewURLDataSource("test:/daily",
> simplestreams.VerifySSLHostnames)}
> sources := []simplestream.DataSource{first, second}

> // do stuff

> c.Check(first.count, gc.Equals, 1)
> c.Check(second.count, gc.Equals, 1)

Done, tho I'm reusing a single countingSource.

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

[Andrew Wilkins (axwalk)]

On 2013/10/02 05:07:52, axw1 wrote:

https://codereview.appspot.com/14218044/diff/1/environs/simplestreams/simplestreams_test.go
> File environs/simplestreams/simplestreams_test.go (right):

https://codereview.appspot.com/14218044/diff/1/environs/simplestreams/simplestreams_test.go#newcode300
> environs/simplestreams/simplestreams_test.go:300: Series:
[]string{"precise"},
> // never match
> On 2013/10/02 04:02:00, thumper wrote:
> > What is the never match? precise or arm? Also, perhaps better to
give a
> region
> > that doesn't exist? Someone may add arm at some stage. Would that
work?

> precise, which isn't in the daily stream I added to testing. I can't
use a
> non-existent region, as that causes GetMetadata to error, which we
don't want.

> The series are validated, but architectures aren't. I'll put in an
invalid arch.

https://codereview.appspot.com/14218044/diff/1/environs/simplestreams/simplestreams_test.go#newcode326
> environs/simplestreams/simplestreams_test.go:326: messages =
append(messages,
> messages...)
> On 2013/10/02 04:02:00, thumper wrote:
> > I agree with our IRC chat, ideally we would have a simple mock
datasource.
> What
> > about...
> >
> > type countingSource struct {
> > simplestreams.DataSource
> > count int
> > }
> >
> > func (s *countingSource) URL(path string) (string, error) {
> > s.count++
> > return s.DataSource.URL(path)
> > }
> >
> > Then have sources defined by:
> >
> > first :=
> >
&countingSource{DataSource:simplestreams.NewURLDataSource("test:/daily",
> > simplestreams.VerifySSLHostnames)}
> > second :=
> >
&countingSource{DataSource:simplestreams.NewURLDataSource("test:/daily",
> > simplestreams.VerifySSLHostnames)}
> > sources := []simplestream.DataSource{first, second}
> >
> > // do stuff
> >
> > c.Check(first.count, gc.Equals, 1)
> > c.Check(second.count, gc.Equals, 1)
> >

> Done, tho I'm reusing a single countingSource.

So, after discussing with jam, GetMetadata *might* be how it is on
purpose. You might want to populate metadata into your private cloud,
and then expect only to be able to create images that match your private
metadata.

I'll leave this for now, until it's decided how it should be working.

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

[Tim Penhey (thumper)]

> So, after discussing with jam, GetMetadata *might* be how it is on
purpose. You
> might want to populate metadata into your private cloud, and then
expect only to
> be able to create images that match your private metadata.

> I'll leave this for now, until it's decided how it should be working.

I think that it should be a configuration option to specify "don't
fall-back to other sources".

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

[Tim Penhey (thumper)]

LGTM

https://codereview.appspot.com/14218044/diff/6001/environs/simplestreams/simplestreams_test.go
File environs/simplestreams/simplestreams_test.go (right):

https://codereview.appspot.com/14218044/diff/6001/environs/simplestreams/simplestreams_test.go#newcode327
environs/simplestreams/simplestreams_test.go:327: c.Assert(source.count,
gc.Equals, 2*len(sources))
Much nicer.

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

[Tim Penhey (thumper)]

review: Approve

[William Reade (fwereade)]

Fine with the idea, I think the implementation's in the wrong place.

https://codereview.appspot.com/14218044/diff/6001/environs/simplestreams/simplestreams.go
File environs/simplestreams/simplestreams.go (right):

https://codereview.appspot.com/14218044/diff/6001/environs/simplestreams/simplestreams.go#newcode406
environs/simplestreams/simplestreams.go:406: if err == nil && len(items)
!= 0 {
I'm not sure this is the right place for it -- it's a hack-on-a-hack,
instead of removing the original hack in getmaybeSignedMetadata that
replaces errors with nil.

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

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/14218044/diff/6001/environs/simplestreams/simplestreams.go
File environs/simplestreams/simplestreams.go (right):

https://codereview.appspot.com/14218044/diff/6001/environs/simplestreams/simplestreams.go#newcode406
environs/simplestreams/simplestreams.go:406: if err == nil && len(items)
!= 0 {
On 2013/10/04 10:26:42, fwereade wrote:
> I'm not sure this is the right place for it -- it's a hack-on-a-hack,
instead of
> removing the original hack in getmaybeSignedMetadata that replaces
errors with
> nil.

I've moved it, but I think it's much the same either way. What we have
is an internal error that is used to communicate that there are no
matching products, for one of several reasons. It can be quashed in
either GetMetadata or getMaybeSignedMetadata - doesn't really matter
which, I think.

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

[Go Bot (go-bot)]

The attempt to merge lp:~axwalk/juju-core/lp1233924-simplestreams-getmetadata-fallback into lp:juju-core failed. Below is the output from the failed tests.

ok launchpad.net/juju-core/agent 1.331s
ok launchpad.net/juju-core/agent/tools 0.266s
ok launchpad.net/juju-core/bzr 7.459s
ok launchpad.net/juju-core/cert 3.287s
ok launchpad.net/juju-core/charm 0.588s
? launchpad.net/juju-core/charm/hooks [no test files]
ok launchpad.net/juju-core/cloudinit 0.026s
ok launchpad.net/juju-core/cmd 0.213s
? launchpad.net/juju-core/cmd/charmd [no test files]
? launchpad.net/juju-core/cmd/charmload [no test files]
ok launchpad.net/juju-core/cmd/juju 169.091s
ok launchpad.net/juju-core/cmd/jujud 54.653s
ok launchpad.net/juju-core/cmd/plugins/juju-metadata 2.711s
ok launchpad.net/juju-core/constraints 0.028s
ok launchpad.net/juju-core/container/lxc 0.291s
? launchpad.net/juju-core/container/lxc/mock [no test files]
ok launchpad.net/juju-core/downloader 5.263s
ok launchpad.net/juju-core/environs 3.169s
ok launchpad.net/juju-core/environs/bootstrap 5.058s
ok launchpad.net/juju-core/environs/cloudinit 0.539s
ok launchpad.net/juju-core/environs/config 0.792s
ok launchpad.net/juju-core/environs/configstore 0.042s
ok launchpad.net/juju-core/environs/filestorage 0.030s
ok launchpad.net/juju-core/environs/httpstorage 0.993s
ok launchpad.net/juju-core/environs/imagemetadata 0.564s
? launchpad.net/juju-core/environs/imagemetadata/testing [no test files]
ok launchpad.net/juju-core/environs/instances 0.054s
ok launchpad.net/juju-core/environs/jujutest 0.248s
ok launchpad.net/juju-core/environs/manual 4.904s
ok launchpad.net/juju-core/environs/simplestreams 0.331s
? launchpad.net/juju-core/environs/simplestreams/testing [no test files]
ok launchpad.net/juju-core/environs/sshstorage 1.067s
ok launchpad.net/juju-core/environs/storage 1.039s
ok launchpad.net/juju-core/environs/sync 28.241s
ok launchpad.net/juju-core/environs/testing 0.204s

----------------------------------------------------------------------
FAIL: tools_test.go:290: SimpleStreamsToolsSuite.TestFindBootstrapTools

[LOG] 64.15651 INFO juju.provider.dummy reset environment

test 0: no tools at all
[LOG] 64.26877 INFO juju.provider.dummy reset environment
[LOG] 64.37642 INFO juju.environs.tools filtering tools by released version
[LOG] 64.37647 INFO juju.environs.tools reading tools with major.minor version 1.0
[LOG] 64.37649 INFO juju.environs.tools filtering tools by series: precise
[LOG] 64.37651 DEBUG juju.environs.tools no architecture specified when finding tools, looking for any
[LOG] 64.37667 DEBUG juju.environs.simplestreams fetchData failed for "file:///tmp/gocheck-894385949183117216/0/streams/v1/index.sjson": cannot find URL "file:///tmp/gocheck-894385949183117216/0/streams/v1/index.sjson" not found
[LOG] 64.37668 DEBUG juju.environs.simplestreams cannot load index "file:///tmp/gocheck-894385949183117216/0/streams/v1/index.sjson": invalid URL "file:///tmp/gocheck-894385949183117216/0/streams/v1/index.sjson" not found
[LOG] 64.37674 DEBUG juju.environs.simplestreams fetchData failed for "file:///tmp/gocheck-894385949183117216/0...

[Andrew Wilkins (axwalk)]

Please take a look.

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

=== modified file 'environs/simplestreams/simplestreams.go'
--- environs/simplestreams/simplestreams.go	2013-10-29 22:58:43 +0000
+++ environs/simplestreams/simplestreams.go	2013-11-05 04:38:36 +0000
@@ -431,6 +431,10 @@
 			break
 		}
 	}
+	if _, ok := err.(*noMatchingProductsError); ok {
+		// no matching products is an internal error only
+		err = nil
+	}
 	return items, err
 }
 
@@ -463,9 +467,6 @@
 		}
 		if _, ok := err.(*noMatchingProductsError); ok {
 			logger.Debugf("%v", err)
-			// No matching products is not considered an error which will allow another source to be
-			// searched, so return err = nil here.
-			return items, nil
 		}
 	}
 	return items, err

=== modified file 'environs/simplestreams/simplestreams_test.go'
--- environs/simplestreams/simplestreams_test.go	2013-10-24 21:05:33 +0000
+++ environs/simplestreams/simplestreams_test.go	2013-11-05 04:38:36 +0000
@@ -300,6 +300,49 @@
 	c.Check(dotOFormats, gc.DeepEquals, simplestreams.IndexMetadataSlice{array[0], array[2]})
 }
 
+// countingSource is used to check that a DataSource has been queried.
+type countingSource struct {
+	simplestreams.DataSource
+	count int
+}
+
+func (s *countingSource) URL(path string) (string, error) {
+	s.count++
+	return s.DataSource.URL(path)
+}
+
+func (s *simplestreamsSuite) TestGetMetadataNoMatching(c *gc.C) {
+	source := &countingSource{
+		DataSource: simplestreams.NewURLDataSource(
+			"test:/daily", simplestreams.VerifySSLHostnames,
+		),
+	}
+	sources := []simplestreams.DataSource{source, source, source}
+	params := simplestreams.ValueParams{DataType: "image-ids"}
+	constraint := sstesting.NewTestConstraint(simplestreams.LookupParams{
+		CloudSpec: simplestreams.CloudSpec{
+			Region:   "us-east-1",
+			Endpoint: "https://ec2.us-east-1.amazonaws.com",
+		},
+		Series: []string{"precise"},
+		Arches: []string{"not-a-real-arch"}, // never matches
+	})
+
+	items, err := simplestreams.GetMetadata(
+		sources,
+		simplestreams.DefaultIndexPath,
+		constraint,
+		false,
+		params,
+	)
+	c.Assert(err, gc.IsNil)
+	c.Assert(items, gc.HasLen, 0)
+
+	// There should be 2 calls to each data-source:
+	// one for .sjson, one for .json.
+	c.Assert(source.count, gc.Equals, 2*len(sources))
+}
+
 func (s *simplestreamsSuite) TestMetadataCatalog(c *gc.C) {
 	metadata := s.AssertGetMetadata(c)
 	c.Check(len(metadata.Products), gc.Equals, 2)

=== modified file 'environs/simplestreams/testing/testing.go'
--- environs/simplestreams/testing/testing.go	2013-10-24 18:03:53 +0000
+++ environs/simplestreams/testing/testing.go	2013-11-05 04:38:36 +0000
@@ -58,6 +58,30 @@
 -----END PGP PUBLIC KEY BLOCK-----`
 
 var imageData = map[string]string{
+	"/daily/streams/v1/index.json": `
+        {
+         "index": {
+          "com.ubuntu.cloud:released:raring": {
+           "updated": "Wed, 01 May 2013 13:31:26 +0000",
+           "clouds": [
+            {
+             "region": "us-east-1",
+             "endpoint": "https://ec2.us-east-1.amazonaws.com"
+            }
+           ],
+           "cloudname": "aws",
+           "datatype": "image-ids",
+           "format": "products:1.0",
+           "products": [
+            "com.ubuntu.cloud:server:13.04:amd64"
+           ],
+           "path": "streams/v1/raring_metadata.json"
+          }
+         },
+         "updated": "Wed, 01 May 2013 13:31:26 +0000",
+         "format": "index:1.0"
+        }
+    `,
 	"/streams/v1/index.json": `
 		{
 		 "index": {

=== modified file 'environs/tools/tools_test.go'
--- environs/tools/tools_test.go	2013-11-04 02:01:19 +0000
+++ environs/tools/tools_test.go	2013-11-05 04:38:36 +0000
@@ -210,9 +210,10 @@
 }, {
 	info:   "custom tools completely block public ones",
 	major:  1,
+	minor:  -1,
 	custom: envtesting.V220all,
 	public: envtesting.VAll,
-	err:    coretools.ErrNoMatches,
+	expect: envtesting.V1all,
 }, {
 	info:   "tools matching major version only",
 	major:  1,
@@ -235,14 +236,13 @@
 			c.Check(err, jc.Satisfies, errors.IsNotFoundError)
 			continue
 		}
-		source := custom
-		if len(source) == 0 {
-			// We only use the public tools if there are *no* custom tools.
-			source = public
-		}
 		expect := map[version.Binary]string{}
 		for _, expected := range test.expect {
-			expect[expected] = source[expected]
+			// If the tools exist in custom, that's preferred.
+			var ok bool
+			if expect[expected], ok = custom[expected]; !ok {
+				expect[expected] = public[expected]
+			}
 		}
 		c.Check(actual.URLs(), gc.DeepEquals, expect)
 	}
@@ -302,10 +302,6 @@
 		s.reset(c, attrs)
 		version.Current = test.CliVersion
 		available := s.uploadCustom(c, test.Available...)
-		if len(available) > 0 {
-			// These should never be chosen.
-			s.uploadPublic(c, envtesting.VAll...)
-		}
 
 		params := envtools.BootstrapToolsParams{
 			Version: agentVersion,
@@ -393,10 +389,6 @@
 			"agent-version": test.agentVersion.String(),
 		})
 		available := s.uploadCustom(c, test.available...)
-		if len(available) > 0 {
-			// These should never be chosen.
-			s.uploadPublic(c, envtesting.VAll...)
-		}
 
 		agentVersion, _ := s.env.Config().AgentVersion()
 		actual, err := envtools.FindInstanceTools(s.env, agentVersion, test.series, &test.arch)
@@ -444,11 +436,10 @@
 	public: []version.Binary{envtesting.V100p64},
 	seek:   envtesting.V100p64,
 }, {
-	info:   "exact match in public blocked by custom",
+	info:   "exact match in public not blocked by custom",
 	custom: envtesting.V110all,
 	public: []version.Binary{envtesting.V100p64},
 	seek:   envtesting.V100p64,
-	err:    coretools.ErrNoMatches,
 }}
 
 func (s *SimpleStreamsToolsSuite) TestFindExactTools(c *gc.C) {
@@ -463,12 +454,11 @@
 				continue
 			}
 			c.Check(actual.Version, gc.Equals, test.seek)
-			source := custom
-			if len(source) == 0 {
-				// We only use public tools if there are *no* private tools.
-				source = public
+			if _, ok := custom[actual.Version]; ok {
+				c.Check(actual.URL, gc.DeepEquals, custom[actual.Version])
+			} else {
+				c.Check(actual.URL, gc.DeepEquals, public[actual.Version])
 			}
-			c.Check(actual.URL, gc.DeepEquals, source[actual.Version])
 		} else {
 			c.Check(err, jc.Satisfies, errors.IsNotFoundError)
 		}

=== modified file 'provider/openstack/export_test.go'
--- provider/openstack/export_test.go	2013-10-24 00:20:59 +0000
+++ provider/openstack/export_test.go	2013-11-05 04:38:36 +0000
@@ -210,7 +210,7 @@
 
 const productMetadatafile = "image-metadata/products.json"
 
-func UseTestImageData(e environs.Environ, cred *identity.Credentials) {
+func UseTestImageData(stor storage.Storage, cred *identity.Credentials) {
 	// Put some image metadata files into the public storage.
 	t := template.Must(template.New("").Parse(indexData))
 	var metadata bytes.Buffer
@@ -218,14 +218,12 @@
 		panic(fmt.Errorf("cannot generate index metdata: %v", err))
 	}
 	data := metadata.Bytes()
-	stor := MetadataStorage(e)
 	stor.Put(simplestreams.DefaultIndexPath+".json", bytes.NewReader(data), int64(len(data)))
 	stor.Put(
 		productMetadatafile, strings.NewReader(imagesData), int64(len(imagesData)))
 }
 
-func RemoveTestImageData(e environs.Environ) {
-	stor := MetadataStorage(e)
+func RemoveTestImageData(stor storage.Storage) {
 	stor.Remove(simplestreams.DefaultIndexPath + ".json")
 	stor.Remove(productMetadatafile)
 }

=== modified file 'provider/openstack/local_test.go'
--- provider/openstack/local_test.go	2013-11-04 02:01:19 +0000
+++ provider/openstack/local_test.go	2013-11-05 04:38:36 +0000
@@ -167,11 +167,11 @@
 	c.Logf("Running live tests using openstack service test double")
 	s.srv.start(c, s.cred)
 	s.LiveTests.SetUpSuite(c)
-	openstack.UseTestImageData(s.Env, s.cred)
+	openstack.UseTestImageData(openstack.ImageMetadataStorage(s.Env), s.cred)
 }
 
 func (s *localLiveSuite) TearDownSuite(c *gc.C) {
-	openstack.RemoveTestImageData(s.Env)
+	openstack.RemoveTestImageData(openstack.ImageMetadataStorage(s.Env))
 	s.LiveTests.TearDownSuite(c)
 	s.srv.stop()
 	s.LoggingSuite.TearDownSuite(c)
@@ -180,6 +180,7 @@
 func (s *localLiveSuite) SetUpTest(c *gc.C) {
 	s.LoggingSuite.SetUpTest(c)
 	s.LiveTests.SetUpTest(c)
+	s.PatchValue(&imagemetadata.DefaultBaseURL, "")
 }
 
 func (s *localLiveSuite) TearDownTest(c *gc.C) {
@@ -194,9 +195,10 @@
 type localServerSuite struct {
 	testbase.LoggingSuite
 	jujutest.Tests
-	cred            *identity.Credentials
-	srv             localServer
-	metadataStorage storage.Storage
+	cred                 *identity.Credentials
+	srv                  localServer
+	toolsMetadataStorage storage.Storage
+	imageMetadataStorage storage.Storage
 }
 
 func (s *localServerSuite) SetUpSuite(c *gc.C) {
@@ -226,18 +228,21 @@
 	s.Tests.SetUpTest(c)
 	// For testing, we create a storage instance to which is uploaded tools and image metadata.
 	env := s.Prepare(c)
-	s.metadataStorage = openstack.MetadataStorage(env)
+	s.toolsMetadataStorage = openstack.MetadataStorage(env)
 	// Put some fake metadata in place so that tests that are simply
 	// starting instances without any need to check if those instances
 	// are running can find the metadata.
-	envtesting.UploadFakeTools(c, s.metadataStorage)
-	openstack.UseTestImageData(env, s.cred)
+	envtesting.UploadFakeTools(c, s.toolsMetadataStorage)
+	s.imageMetadataStorage = openstack.ImageMetadataStorage(env)
+	openstack.UseTestImageData(s.imageMetadataStorage, s.cred)
 }
 
 func (s *localServerSuite) TearDownTest(c *gc.C) {
-	openstack.RemoveTestImageData(s.Open(c))
-	if s.metadataStorage != nil {
-		envtesting.RemoveFakeToolsMetadata(c, s.metadataStorage)
+	if s.imageMetadataStorage != nil {
+		openstack.RemoveTestImageData(s.imageMetadataStorage)
+	}
+	if s.toolsMetadataStorage != nil {
+		envtesting.RemoveFakeToolsMetadata(c, s.toolsMetadataStorage)
 	}
 	s.Tests.TearDownTest(c)
 	s.srv.stop()
@@ -535,7 +540,11 @@
 }
 
 func (s *localServerSuite) TestFindImageBadDefaultImage(c *gc.C) {
+	// Prevent falling over to the public datasource.
+	s.PatchValue(&imagemetadata.DefaultBaseURL, "")
+
 	env := s.Open(c)
+
 	// An error occurs if no suitable image is found.
 	_, err := openstack.FindInstanceSpec(env, "saucy", "amd64", "mem=8G")
 	c.Assert(err, gc.ErrorMatches, `no "saucy" images in some-region with arches \[amd64\]`)
@@ -739,8 +748,8 @@
 	c.Logf("Generating fake tools for: %v", url)
 	envtesting.UploadFakeTools(c, metadataStorage)
 	defer envtesting.RemoveFakeTools(c, metadataStorage)
-	openstack.UseTestImageData(s.env, s.cred)
-	defer openstack.RemoveTestImageData(s.env)
+	openstack.UseTestImageData(metadataStorage, s.cred)
+	defer openstack.RemoveTestImageData(metadataStorage)
 
 	err = bootstrap.Bootstrap(s.env, constraints.Value{})
 	c.Assert(err, gc.IsNil)