Merge lp:~jameinel/juju-core/openstack-filter-1257481 into lp:juju-core/1.16

[John A Meinel]

provider/openstack/provider: safer matching

bug #1257481 is because of how we are matching machines, if you have 2
environments with similar names (one is a prefix of the other) then
you can have 'juju destroy-environment short-name' actually kill all
the instances of 'short-name-but-longer'.

However, we do put a little bit more data into how we name machines,
so we can make it harder to trigger that by accident.

The actual change is small and directly tested against HP and
Canonistack. I don't add a test because the actual test we would need
is to create 2 environments, and assert that AllInstances doesn't see
the instances in a similarly named environment.

I tried 2 regexes 'juju-ENV-machine-\d+' didn't work when testing
against canonistack, so I went with 'juju-ENV-machine-\d*'. That
should be pretty restrictive. Even if you name one environment 'ENV'
and another environment 'ENV-machine' the \d will prevent them from
matching (because the actual machines will be
juju-ENV-machine-machine-0).

I think this is as safe as we can be with a name-based approach, even
though we've discussed moving into a 'record the nodes only in state
and only delete ones recorded there', this is a pretty easy workaround
to make things nicer for real people.

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

This is the same filter fix that was proposed for trunk, but the fix is reasonable to have for 1.16.

=== modified file 'provider/openstack/export_test.go'
--- provider/openstack/export_test.go	2013-09-25 12:22:10 +0000
+++ provider/openstack/export_test.go	2013-12-09 10:38:14 +0000
@@ -316,3 +316,7 @@
 		swift:         swiftClient,
 	}
 }
+
+func GetNovaClient(e environs.Environ) *nova.Client {
+	return e.(*environ).nova()
+}

=== modified file 'provider/openstack/local_test.go'
--- provider/openstack/local_test.go	2013-10-01 13:34:02 +0000
+++ provider/openstack/local_test.go	2013-12-09 10:38:14 +0000
@@ -950,3 +950,41 @@
 	// We *don't* test Fetch for sources[3] because it points to
 	// juju.canonical.com
 }
+
+func (s *localServerSuite) TestAllInstancesIgnoresOtherMachines(c *gc.C) {
+	env := s.Prepare(c)
+	err := bootstrap.Bootstrap(env, constraints.Value{})
+	c.Assert(err, gc.IsNil)
+
+	// Check that we see 1 instance in the environment
+	insts, err := env.AllInstances()
+	c.Assert(err, gc.IsNil)
+	c.Check(insts, gc.HasLen, 1)
+
+	// Now start a machine 'manually' in the same account, with a similar
+	// but not matching name, and ensure it isn't seen by AllInstances
+	// See bug #1257481, for how similar names were causing them to get
+	// listed (and thus destroyed) at the wrong time
+	existingEnvName := s.TestConfig["name"]
+	newMachineName := fmt.Sprintf("juju-%s-2-machine-0", existingEnvName)
+
+	// We grab the Nova client directly from the env, just to save time
+	// looking all the stuff up
+	novaClient := openstack.GetNovaClient(env)
+	entity, err := novaClient.RunServer(nova.RunServerOpts{
+		Name:     newMachineName,
+		FlavorId: "1", // test service has 1,2,3 for flavor ids
+		ImageId:  "1", // UseTestImageData sets up images 1 and 2
+	})
+	c.Assert(err, gc.IsNil)
+	c.Assert(entity, gc.NotNil)
+
+	// List all servers with no filter, we should see both instances
+	servers, err := novaClient.ListServersDetail(nova.NewFilter())
+	c.Assert(err, gc.IsNil)
+	c.Assert(servers, gc.HasLen, 2)
+
+	insts, err = env.AllInstances()
+	c.Assert(err, gc.IsNil)
+	c.Check(insts, gc.HasLen, 1)
+}

=== modified file 'provider/openstack/provider.go'
--- provider/openstack/provider.go	2013-10-09 06:17:57 +0000
+++ provider/openstack/provider.go	2013-12-09 10:38:14 +0000
@@ -847,7 +847,7 @@
 // machinesFilter returns a nova.Filter matching all machines in the environment.
 func (e *environ) machinesFilter() *nova.Filter {
 	filter := nova.NewFilter()
-	filter.Set(nova.FilterServer, fmt.Sprintf("juju-%s-.*", e.Name()))
+	filter.Set(nova.FilterServer, fmt.Sprintf("juju-%s-machine-\\d*", e.Name()))
 	return filter
 }