Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(5)

Issue 39030043: provider/openstack/provider: safer matching

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 4 months ago by jameinel
Modified:
10 years, 4 months ago
Reviewers:
fwereade, mp+197730
Visibility:
Public.

Description

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. This branch is built on the 1.16 code base in case we decide to land it there. 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/37200043 https://code.launchpad.net/~jameinel/juju-core/openstack-filter-1257481/+merge/197730 (do not edit description out of merge proposal)

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+45 lines, -1 line) Patch
A [revision details] View 1 chunk +2 lines, -0 lines 0 comments Download
M provider/openstack/export_test.go View 1 chunk +4 lines, -0 lines 0 comments Download
M provider/openstack/local_test.go View 1 chunk +38 lines, -0 lines 0 comments Download
M provider/openstack/provider.go View 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 2
jameinel
Please take a look.
10 years, 4 months ago (2013-12-08 09:43:17 UTC) #1
fwereade
10 years, 4 months ago (2013-12-09 10:35:34 UTC) #2
LGTM, thanks for the test.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b