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

Issue 12755043: Don't do "no instances found" after bootstrap. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 9 months ago by Michael Nelson
Modified:
10 years, 9 months ago
Reviewers:
jameinel, mp+179680
Visibility:
Public.

Description

Don't do "no instances found" after bootstrap. Fix for bug 1208504 (apparently only seen for HP Cloud?) Adding the fix on its own would have been trivial, but writing a failing test required some refactoring... and while there, I've tried to simplify the code, making some assumptions a little more explicit. Notes: * I can't reproduce this on trunk with canonistack, and I don't have an HPCould account to verify the problem and solution. That said, looking at the code for environs.StateInfo, it would cause described behaviour error in its current form. * I've added the 'broken-error' to the dummy environ config, which seems less than ideal. I added it there because the "broken" config option was already there, but I wonder if they'd both be best outside of the config itself (or is the "broken" option used outside of the unit-tests?). The result is that broken-error uses schema.Any. Another option would be to use string and change the WaitForInstances conditional to be dependent on the error message rather than the error, but that'd be even less ideal IMO? * Currently in trunk, Environs.StateInfo returns with only the info from the first available state instance (well, instances from the first successful call to getDNSNames). Is this intended as the desired functionality, or for other reasons (maybe historically there was only one state instance? I can't see any code refering to more than one state instance). Anyway, I've made this more explicit, but would be happy to update s/WaitForFirstDNSName/WaitForDNSNames/ if that would now be better functionality? * In the refactored StateInfo function, I'm re-using the one timer (LongAttempt) to keep the existing functionality (ie. waiting for both the instances and then the dns(s) happens all within one LongAttempt). But that requires two calls to .Next(), whereas the impatientStrategy test double only allowed one. I've updated impatientStrategy as the simplest option, but if that's not wanted, there are other ways around (ie. Adding an Attempt.Remaining() that returns a new attempt based on the existing but with the remaining counts/time). https://code.launchpad.net/~michael.nelson/juju-core/1208504-post-bootstrap-hp-no-instances-found/+merge/179680 (do not edit description out of merge proposal)

Patch Set 1 #

Patch Set 2 : Don't do "no instances found" after bootstrap. #

Patch Set 3 : Don't do "no instances found" after bootstrap. #

Patch Set 4 : Don't do "no instances found" after bootstrap. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+176 lines, -24 lines) Patch
A [revision details] View 1 chunk +2 lines, -0 lines 0 comments Download
M environs/dummy/config_test.go View 2 chunks +33 lines, -1 line 0 comments Download
M environs/dummy/environs.go View 5 chunks +30 lines, -6 lines 0 comments Download
M environs/polling.go View 2 chunks +26 lines, -0 lines 0 comments Download
M environs/polling_test.go View 2 chunks +72 lines, -0 lines 0 comments Download
M environs/state.go View 1 chunk +11 lines, -15 lines 0 comments Download
M environs/testing/polling.go View 1 chunk +2 lines, -2 lines 0 comments Download

Messages

Total messages: 2
Michael Nelson
Please take a look.
10 years, 9 months ago (2013-08-12 12:20:12 UTC) #1
jameinel
10 years, 9 months ago (2013-08-13 13:45:44 UTC) #2
I can try to reproduce on HP Cloud, certainly Antonio was able to trigger it on
demand (and I have an HP account from him).

I won't do it tonight, but I can try to get to it tomorrow if you want
confirmation.
Sign in to reply to this message.

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