Keyboard Shortcuts

File
u :up to issue
m :publish + mail comments
M :edit review message
j / k :jump to file after / before current file
J / K :jump to next file with a comment after / before current file
Side-by-side diff
i :toggle intra-line diffs
e :expand all comments
c :collapse all comments
s :toggle showing all comments
n / p :next / previous diff chunk or comment
N / P :next / previous comment
<Up> / <Down> :next / previous line
<Enter> :respond to / edit current comment
d :mark current comment as done
Issue
u :up to list of issues
m :publish + mail comments
j / k :jump to patch after / before current patch
o / <Enter> :open current patch in side-by-side view
i :open current patch in unified diff view
Issue List
j / k :jump to issue after / before current issue
o / <Enter> :open current issue
# : close issue
Comment/message editing
<Ctrl> + s or <Ctrl> + Enter :save comment
<Esc> :cancel edit
Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(26)
Issues Repositories Search
Open Issues | Closed Issues | All Issues | Sign in with your Google Account to create issues and add comments

Issue 37200043: provider/openstack/provider: safer matching

Can't Edit
Can't Publish+Mail
Start Review
Created:
12 years, 1 month ago by jameinel
Modified:
12 years, 1 month ago
Reviewers:
gz , mp+197730
Visibility:
Public.
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://code.launchpad.net/~jameinel/juju-core/openstack-filter-1257481/+merge/197730 (do not edit description out of merge proposal)

Patch Set 1 #

Created: 12 years, 1 month ago
Download [raw] [tar.bz2]
Unified diffs Side-by-side diffs Delta from patch set Stats (+3 lines, -1 line) Patch
A [revision details] View 1 chunk +2 lines, -0 lines 0 comments Download
M provider/openstack/provider.go View 1 chunk +1 line, -1 line 0 comments Download
Total messages: 3
|
jameinel
Please take a look.
12 years, 1 month ago (2013年12月04日 15:12:55 UTC) #1
Please take a look.
Sign in to reply to this message.
gz
Replacement of <https://codereview.appspot.com/37140043/> LGTM. I think this would benefit from even a local live test, ...
12 years, 1 month ago (2013年12月04日 15:39:30 UTC) #2
Replacement of <https://codereview.appspot.com/37140043/>
LGTM. I think this would benefit from even a local live test, though I see the
desire for a real live test as the pattern matching is done server side by
Openstack. The test could have a named environment then nova boot machine(s)
with names that would match under the previous pattern, and verify that
Instances does not return them when nova list does.
Sign in to reply to this message.
jameinel
On 2013年12月04日 15:39:30, gz wrote: > Replacement of <https://codereview.appspot.com/37140043/> > > LGTM. I think this ...
12 years, 1 month ago (2013年12月08日 09:44:58 UTC) #3
On 2013年12月04日 15:39:30, gz wrote:
> Replacement of <https://codereview.appspot.com/37140043/>
> 
> LGTM. I think this would benefit from even a local live test, though I see the
> desire for a real live test as the pattern matching is done server side by
> Openstack. The test could have a named environment then nova boot machine(s)
> with names that would match under the previous pattern, and verify that
> Instances does not return them when nova list does.
Test has been added, but it looks like LBOX decided to move me to yet-another
page:
https://codereview.appspot.com/39030043 
Sign in to reply to this message.
|
Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b

AltStyle によって変換されたページ (->オリジナル) /