juju-core

Merge lp://staging/~wallyworld/juju-core/openstack-live-test-fixes into lp://staging/~juju/juju-core/trunk

openstack-live-test-fixes
Merge into trunk

Proposed by Ian Booth on 2013-04-23

Status:	Merged
Approved by:	William Reade on 2013-04-30
Approved revision:	no longer in the source branch.
Merged at revision:	1205
Proposed branch:	lp://staging/~wallyworld/juju-core/openstack-live-test-fixes
Merge into:	lp://staging/~juju/juju-core/trunk
Prerequisite:	lp://staging/~wallyworld/juju-core/openstack-image-lookup
Diff against target:	244 lines (+61/-30) 8 files modified environs/openstack/export_test.go (+9/-0) environs/openstack/live_test.go (+3/-6) environs/openstack/local_test.go (+10/-8) environs/openstack/provider.go (+4/-2) environs/openstack/provider_test.go (+1/-11) environs/testing/tools.go (+15/-1) environs/tools/storage.go (+9/-2) environs/tools/storage_test.go (+10/-0)
To merge this branch:	bzr merge lp://staging/~wallyworld/juju-core/openstack-live-test-fixes
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Juju Engineering		2013-04-23	Pending
Review via email: mp+160275@code.staging.launchpad.net

Description of the change

Remove hard coded Canonistack image id

Now that we have cloud image metadata for Canonistack, the hard coded
image id and instance type used for live tests are no longer required.

Another change is that the live tests now create a test "container"
in which to upload the fake tools to the public bucket. This allows
any specified public bucket to be used to read the image metadata files
but does not risk any legitimate tools in that public bucket being deleted.
The clean up for the live tests only removes the fake tools from the
test "container" and leaves the other public bucket files untouched.

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

Revision history for this message

Ian Booth (wallyworld) wrote on 2013-04-23:

Download full text (6.8 KiB)

Reviewers: mp+160275_code.launchpad.net,

Message:
Please take a look.

Description:
Remove hard coded Canonistack image id

Now that we have cloud image metadata for Canonistack, the hard coded
image id and instance type used for live tests are no longer required.

Another change is that the clean up for the live tests was changed to
only remove the fake tools from the public bucket otherwise we were
deleting the entire public bucket and this removed all the legitimate
tools and image metadata files.

https://code.launchpad.net/~wallyworld/juju-core/openstack-live-test-fixes/+merge/160275

Requires:
https://code.launchpad.net/~wallyworld/juju-core/openstack-image-lookup/+merge/159301

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M environs/openstack/live_test.go
   M environs/openstack/local_test.go
   M environs/openstack/provider.go
   M environs/openstack/provider_test.go
   M environs/testing/tools.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: <email address hidden>
+New revision: <email address hidden>

Index: environs/openstack/live_test.go
=== modified file 'environs/openstack/live_test.go'
--- environs/openstack/live_test.go 2013-04-19 02:39:42 +0000
+++ environs/openstack/live_test.go 2013-04-23 04:03:33 +0000
@@ -70,8 +70,6 @@
     // this flag to True.
     HasProvisioner: false,
    },
- testImageId: testImageDetails.ImageId,
- testFlavor: testImageDetails.Flavor,
   })
  }

@@ -82,8 +80,6 @@
   coretesting.LoggingSuite
   jujutest.LiveTests
   cred *identity.Credentials
- testImageId string
- testFlavor string
   writeablePublicStorage environs.Storage
  }

@@ -118,8 +114,7 @@
    return
   }
   if t.writeablePublicStorage != nil {
- err := openstack.DeleteStorageContent(t.writeablePublicStorage)
- c.Check(err, IsNil)
+ envtesting.RemoveFakeTools(c, t.writeablePublicStorage)
   }
   t.LiveTests.TearDownSuite(c)
   t.LoggingSuite.TearDownSuite(c)

Index: environs/openstack/local_test.go
=== modified file 'environs/openstack/local_test.go'
--- environs/openstack/local_test.go 2013-04-19 02:39:42 +0000
+++ environs/openstack/local_test.go 2013-04-23 04:03:33 +0000
@@ -100,9 +100,7 @@
   config["default-instance-type"] = "m1.small"
   Suite(&localLiveSuite{
    LiveTests: LiveTests{
- cred: cred,
- testImageId: "1",
- testFlavor: "m1.small",
+ cred: cred,
     LiveTests: jujutest.LiveTests{
      TestConfig: jujutest.TestConfig{config},
     },
@@ -183,9 +181,10 @@
  type localServerSuite struct {
   coretesting.LoggingSuite
   jujutest.Tests
- cred *identity.Credentials
- srv localServer
- env environs.Environ
+ cred *identity.Credentials
+ srv localServer
+ env environs.Environ
+ writeablePublicStorage environs.Storage
  }

func (s *localServerSuite) SetUpSuite(c *C) {
@@ -206,8 +20...

Reviewers: mp+160275_code.launchpad.net,

Message:
Please take a look.

Description:
Remove hard coded Canonistack image id

Now that we have cloud image metadata for Canonistack, the hard coded
image id and instance type used for live tests are no longer required.

https://code.launchpad.net/~wallyworld/juju-core/openstack-live-test-fixes/+merge/160275

Requires:
https://code.launchpad.net/~wallyworld/juju-core/openstack-image-lookup/+merge/159301

(do not edit description out of merge proposal)

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

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: ian.booth@canonical.com-20130422055547-hkff5j05f9dc4k7p
+New revision: ian.booth@canonical.com-20130423040333-lc8rywt2mq65h1ku

Index: environs/openstack/live_test.go
=== modified file 'environs/openstack/live_test.go'
--- environs/openstack/live_test.go	2013-04-19 02:39:42 +0000
+++ environs/openstack/live_test.go	2013-04-23 04:03:33 +0000
@@ -70,8 +70,6 @@
  			//       this flag to True.
  			HasProvisioner: false,
  		},
-		testImageId: testImageDetails.ImageId,
-		testFlavor:  testImageDetails.Flavor,
  	})
  }

@@ -82,8 +80,6 @@
  	coretesting.LoggingSuite
  	jujutest.LiveTests
  	cred                   *identity.Credentials
-	testImageId            string
-	testFlavor             string
  	writeablePublicStorage environs.Storage
  }

@@ -118,8 +114,7 @@
  		return
  	}
  	if t.writeablePublicStorage != nil {
-		err := openstack.DeleteStorageContent(t.writeablePublicStorage)
-		c.Check(err, IsNil)
+		envtesting.RemoveFakeTools(c, t.writeablePublicStorage)
  	}
  	t.LiveTests.TearDownSuite(c)
  	t.LoggingSuite.TearDownSuite(c)

Index: environs/openstack/local_test.go
=== modified file 'environs/openstack/local_test.go'
--- environs/openstack/local_test.go	2013-04-19 02:39:42 +0000
+++ environs/openstack/local_test.go	2013-04-23 04:03:33 +0000
@@ -100,9 +100,7 @@
  	config["default-instance-type"] = "m1.small"
  	Suite(&localLiveSuite{
  		LiveTests: LiveTests{
-			cred:        cred,
-			testImageId: "1",
-			testFlavor:  "m1.small",
+			cred: cred,
  			LiveTests: jujutest.LiveTests{
  				TestConfig: jujutest.TestConfig{config},
  			},
@@ -183,9 +181,10 @@
  type localServerSuite struct {
  	coretesting.LoggingSuite
  	jujutest.Tests
-	cred *identity.Credentials
-	srv  localServer
-	env  environs.Environ
+	cred                   *identity.Credentials
+	srv                    localServer
+	env                    environs.Environ
+	writeablePublicStorage environs.Storage
  }

func (s *localServerSuite) SetUpSuite(c *C) {
@@ -206,8 +205,8 @@
  		"auth-url": s.cred.URL,
  	})
  	s.Tests.SetUpTest(c)
-	writeablePublicStorage := openstack.WritablePublicStorage(s.Env)
-	envtesting.UploadFakeTools(c, writeablePublicStorage)
+	s.writeablePublicStorage = openstack.WritablePublicStorage(s.Env)
+	envtesting.UploadFakeTools(c, s.writeablePublicStorage)
  	s.env = s.Tests.Env
  	openstack.UseTestImageData(s.env, false)
  }
@@ -216,6 +215,9 @@
  	if s.env != nil {
  		openstack.RemoveTestImageData(s.env)
  	}
+	if s.writeablePublicStorage != nil {
+		envtesting.RemoveFakeTools(c, s.writeablePublicStorage)
+	}
  	s.Tests.TearDownTest(c)
  	s.srv.stop()
  	s.LoggingSuite.TearDownTest(c)

Index: environs/openstack/provider.go
=== modified file 'environs/openstack/provider.go'
--- environs/openstack/provider.go	2013-04-22 03:26:40 +0000
+++ environs/openstack/provider.go	2013-04-23 04:03:33 +0000
@@ -77,8 +77,10 @@
    # auth-url: https://yourkeystoneurl:443/v2.0/
    # override if your workstation is running a different series to which  
you are deploying
    # default-series: precise
-  default-image-id: c876e5fe-abb0-41f0-8f29-f0b47481f523
-  default-instance-type: "m1.small"
+  # The attributes below allow user specified defaults to be used if a  
suitable image
+  # or instance type cannot be found.
+  # default-image-id: <fallback image id>
+  # default-instance-type: <fallback flavor name>
    # The following are used for userpass authentication (the default)
    auth-mode: userpass
    # Usually set via the env variable OS_USERNAME, but can be specified here

Index: environs/openstack/provider_test.go
=== modified file 'environs/openstack/provider_test.go'
--- environs/openstack/provider_test.go	2013-02-27 00:53:19 +0000
+++ environs/openstack/provider_test.go	2013-04-23 04:03:33 +0000
@@ -13,8 +13,6 @@

// Out-of-the-box, we support live testing using Canonistack or HP Cloud.
  var testConstraints = map[string]openstack.ImageDetails{
-	"canonistack": openstack.ImageDetails{
-		Flavor: "m1.tiny", ImageId: "c876e5fe-abb0-41f0-8f29-f0b47481f523"},
  	"hpcloud": openstack.ImageDetails{
  		Flavor: "standard.xsmall", ImageId: "75845"},
  }
@@ -35,14 +33,6 @@
  				t.Fatalf("Unknown vendor %s. Must be one of %s", *vendor, keys)
  			}
  		} else {
-			if *imageId == "" {
-				t.Fatalf("Must specify image id to use for test instance, "+
-					"eg %s for Canonistack", "-image  
c876e5fe-abb0-41f0-8f29-f0b47481f523")
-			}
-			if *flavor == "" {
-				t.Fatalf("Must specify flavor to use for test instance, "+
-					"eg %s for Canonistack", "-flavor m1.tiny")
-			}
  			testImageDetails = openstack.ImageDetails{*flavor, *imageId}
  		}
  		cred, err := identity.CompleteCredentialsFromEnv()

Index: environs/testing/tools.go
=== modified file 'environs/testing/tools.go'
--- environs/testing/tools.go	2013-04-17 20:41:15 +0000
+++ environs/testing/tools.go	2013-04-23 04:03:33 +0000
@@ -71,6 +71,20 @@
  	}
  }

+// RemoveFakeTools deletes the fake tools from the supplied storage.
+func RemoveFakeTools(c *C, storage environs.Storage) {
+	toolsVersion := version.Current
+	name := tools.StorageName(toolsVersion)
+	err := storage.Remove(name)
+	c.Check(err, IsNil)
+	if version.Current.Series != config.DefaultSeries {
+		toolsVersion.Series = config.DefaultSeries
+		name := tools.StorageName(toolsVersion)
+		err := storage.Remove(name)
+		c.Check(err, IsNil)
+	}
+}
+
  // RemoveTools deletes all tools from the supplied storage.
  func RemoveTools(c *C, storage environs.Storage) {
  	names, err := storage.List("tools/juju-")
@@ -78,7 +92,7 @@
  	c.Logf("removing files: %v", names)
  	for _, name := range names {
  		err = storage.Remove(name)
-		c.Assert(err, IsNil)
+		c.Check(err, IsNil)
  	}
  }

Revision history for this message

Dimiter Naydenov (dimitern) wrote on 2013-04-23:

LGTM, it's coming along nicely.

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

Revision history for this message

John A Meinel (jameinel) wrote on 2013-04-24:

LGTM

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

Revision history for this message

William Reade (fwereade) wrote on 2013-04-24:

NOT LGTM until we've had a chat -- if we're in danger of wiping out the
real public bucket with our tests, we're Doing It Wrong at a deeper
level and we should address that. What are the followup plans here?

https://codereview.appspot.com/8812053/diff/1/environs/testing/tools.go
File environs/testing/tools.go (right):

https://codereview.appspot.com/8812053/diff/1/environs/testing/tools.go#newcode76
environs/testing/tools.go:76: toolsVersion := version.Current
Won't this still nuke the real tools if the version happens to match?

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

Revision history for this message

Ian Booth (wallyworld) wrote on 2013-04-29:

The issue with the public bucket referred to in the covering letter of
the mp is trigger if the user running the tests decides to use
credentials that give them write access to the public bucket and have
their env set up to use that bucket. It's sort of like deliberately
using su to delete some protected files. Perhaps the tests could be set
up to use a different public bucket name just in case?

https://codereview.appspot.com/8812053/diff/1/environs/testing/tools.go
File environs/testing/tools.go (right):

https://codereview.appspot.com/8812053/diff/1/environs/testing/tools.go#newcode76
environs/testing/tools.go:76: toolsVersion := version.Current
On 2013/04/24 16:12:00, fwereade wrote:
> Won't this still nuke the real tools if the version happens to match?

This method uses the same logic as uploadFakeTools, which uses
version.Current as the tools version when uploading the fake tarball.

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

Revision history for this message

Ian Booth (wallyworld) wrote on 2013-04-29:

Please take a look.

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

Revision history for this message

William Reade (fwereade) wrote on 2013-04-30:

LGTM on the condition that we schedule some followup work to eliminate
the use of fake tools from live tests; I don't think they can possibly
be valid/meaningful.

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

Revision history for this message

Ian Booth (wallyworld) wrote on 2013-04-30:

*** Submitted:

Remove hard coded Canonistack image id

Now that we have cloud image metadata for Canonistack, the hard coded
image id and instance type used for live tests are no longer required.

Another change is that the live tests now create a test "container"
in which to upload the fake tools to the public bucket. This allows
any specified public bucket to be used to read the image metadata files
but does not risk any legitimate tools in that public bucket being
deleted.
The clean up for the live tests only removes the fake tools from the
test "container" and leaves the other public bucket files untouched.

R=dimitern, jameinel, fwereade
CC=
https://codereview.appspot.com/8812053

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

Revision history for this message

William Reade (fwereade) wrote on 2013-04-30:

On 2013/04/30 08:26:44, wallyworld wrote:
> *** Submitted:

> Remove hard coded Canonistack image id

> Now that we have cloud image metadata for Canonistack, the hard coded
> image id and instance type used for live tests are no longer required.

> Another change is that the live tests now create a test "container"
> in which to upload the fake tools to the public bucket. This allows
> any specified public bucket to be used to read the image metadata
files
> but does not risk any legitimate tools in that public bucket being
deleted.
> The clean up for the live tests only removes the fake tools from the
> test "container" and leaves the other public bucket files untouched.

> R=dimitern, jameinel, fwereade
> CC=
> https://codereview.appspot.com/8812053

BTW, please add a note to this review referencing the bugs added as a
result.

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

Revision history for this message

Ian Booth (wallyworld) wrote on 2013-05-10:

On 2013/04/30 08:50:21, fwereade wrote:
> On 2013/04/30 08:26:44, wallyworld wrote:
> > *** Submitted:
> >
> > Remove hard coded Canonistack image id
> >
> > Now that we have cloud image metadata for Canonistack, the hard
coded
> > image id and instance type used for live tests are no longer
required.
> >
> > Another change is that the live tests now create a test "container"
> > in which to upload the fake tools to the public bucket. This allows
> > any specified public bucket to be used to read the image metadata
files
> > but does not risk any legitimate tools in that public bucket being
deleted.
> > The clean up for the live tests only removes the fake tools from the
> > test "container" and leaves the other public bucket files untouched.
> >
> > R=dimitern, jameinel, fwereade
> > CC=
> > https://codereview.appspot.com/8812053