HPCloud: Don't incorrectly return ErrNoInstances.

HPCloud: Don't incorrectly return ErrNoInstances.

So this is attempt 2 - with the knowledge that when environ.Instances()
returns ErrNoInstances we should not keep polling.

Checking the time details on the bug [1] shows that the ErrNoInstances was
returned after only 3 seconds - much shorter than the shortAttempt in
environs/openstack/provider.go. It turns out that e.collectInstances() is erronously
returning missing = [] when it finds a server, but that server doesn't have
an expected status of BUILD or ACTIV.

Moving the continue ensures that if the server is found but has the incorrect
status, it is included in the returned missing slice. I would also like to
explicitly test this change to collectInstances, but can't see a way to test
the unexposed function from environs.openstack.local_tests.go [2]. The added
test ensures simply that we do return for a BUILD(spawning) server.

Testing the `juju -v bootstrap && juju -v status` 10 times showed that
HPClound would get to BUILD(spawning) in around 9.1 seconds, so I've
increased the shortTimeout slightly to 15. Let me know if you prefer
something smaller.

Other things I tried:
 * Avoiding the increased shortAttempt by checking for:
      strings.HasPrefix(server.Status, nova.StatusBuild)
   Testing shows that HPCloud returns not "BUILD", but rather
   "BUILD(networking)" and "BUILD(spawning)" [4]. But this had one main issue
   in that we'd return with BUILD(networking) which is before the server has
   a public IP address.


[1] https://bugs.launchpad.net/juju-core/+bug/1208504
[2] http://paste.ubuntu.com/6002418/
[3] http://paste.ubuntu.com/5988133/
[4] See http://docs.hpcloud.com/api/compute#ServerStates (past where it states
the possible values :-))

https://code.launchpad.net/~michael.nelson/juju-core/1208504-post-bootstrap-hp-no-instances-found-try2/+merge/179899

(do not edit description out of merge proposal)

[Michael Nelson, 2013-08-13 13:02:18]

Please take a look.

[jameinel, 2013-08-13 13:29:39]

did you get a chance to test this live?

It seems like we would want a test for what this changes (instances in BUILD
status are treated differently, etc).

[Michael Nelson, 2013-08-13 13:39:31]

On 2013/08/13 13:29:39, jameinel wrote:
> did you get a chance to test this live?
> 
> It seems like we would want a test for what this changes (instances in BUILD
> status are treated differently, etc).

I'd love to write a test for it - but don't see any existing infrastructure to
do so (see the description). As I wrote there, I could go ahead and create a
test double for nova - but I don't want to do that unless I know it's the right
way to go (I may have missed existing tools for testing nova).

And no, I don't have any HP creds to test it live. I can sign up temporarily for
the free trial, but if someone else with creds can verify that'd be great too.

From the code, it shouldn't be changing the behaviour for the expected statuses
(state instances in Build or Active) - as they were not added to the returned
missing instances previously. What it will (should) change is to include
instances that are *not* in Build or Active in the missing instances and keep
polling for the shortAttempt (rather than returning ErrNoInstances). Let's see
with a real test.

[fwereade, 2013-08-13 17:18:25]

I think I'm ok with the change in itself, but... *surely* there's a test for
this that ought to change. John?

[jameinel, 2013-08-14 10:22:23]

NOT LGTM

testing this live showed that it didn't actually fix the problem. (juju
bootstrap && ./juju status still failed with the no instances found).

So while this change may be correct in its own right (though it needs a test),
it doesn't actually solve the problem.

[Michael Nelson, 2013-08-15 08:48:29]

On 2013/08/14 10:22:23, jameinel wrote:
> NOT LGTM
> 
> testing this live showed that it didn't actually fix the problem. (juju
> bootstrap && ./juju status still failed with the no instances found).
> 
> So while this change may be correct in its own right (though it needs a test),
> it doesn't actually solve the problem.

Hi John.

As I now have working creds for HP I'm able to check this myself rather than
just looking at the code and making stabs in the dark :) But as it turns out, I
think the above change *does* actually fix the underlying issue. You can double
check, but I'm betting you missed it because it then still failed for a
different reason - the 10 second timeout.

That is on trunk, the time between the start of the status command and the error
is only 2 seconds (because of the 'continue' in the incorrect place), where as
on this branch with the moved continue the time between the start of the status
command and the error is exactly 2 + 10 seconds (where 10 seconds is the
shortAttempt timeout). I've demo'd that here [1].

Anyway, I'm updating the branch with both the fix, an updated shortTimeout and a
better error if it does timeout. I'll take a look at the test double you
mentioned and see if we can reuse it here too.

Thanks,
[1] http://paste.ubuntu.com/5988133/

[Michael Nelson, 2013-08-15 14:48:53]

Please take a look.

[Michael Nelson, 2013-08-19 11:01:59]

Please take a look.

[jameinel, 2013-08-19 12:39:54]

The standard way to test an 'unexported' function inside its own package tests
is to edit "export_test.go" and put an exported wrapper in there. So if you had:

provider.go:
  func myFunc(a string) error {
  }

You would add

export_test.go:
  func MyFunc(a string) error {
    return myFunc(a)
  }


The magic here is that '*_test.go' only gets compiled into the package during
'go test' and not 'go build/install'. So these magic wrappers won't be exposed
to other packages, but they *can* be available for the test suite itself.

The other changes look fine to me, there are enough comments that we can know
why it is set the way it is, and why we may (or may not) want to tweak them.

So if you want to add a test for the direct behavior, that is great for me, but
LGTM with that change.

[Michael Nelson, 2013-08-19 13:00:18]

On 2013/08/19 12:39:54, jameinel wrote:
> The standard way to test an 'unexported' function inside its own package tests
> is to edit "export_test.go" and put an exported wrapper in there. So if you
had:
> 
> provider.go:
>   func myFunc(a string) error {
>   }
> 
> You would add
> 
> export_test.go:
>   func MyFunc(a string) error {
>     return myFunc(a)
>   }
> 
> 
> The magic here is that '*_test.go' only gets compiled into the package during
> 'go test' and not 'go build/install'. So these magic wrappers won't be exposed
> to other packages, but they *can* be available for the test suite itself.
> 
> The other changes look fine to me, there are enough comments that we can know
> why it is set the way it is, and why we may (or may not) want to tweak them.
> 
> So if you want to add a test for the direct behavior, that is great for me,
but
> LGTM with that change.

Excellent, thanks for info John. I've added the test that demos the original
problem (the 'continue' in the incorrect place).

[Michael Nelson, 2013-08-19 13:07:11]

Please take a look.

[dimitern, 2013-08-19 13:29:36]

Just a couple of comments.

https://codereview.appspot.com/12795045/diff/26001/environs/openstack/local_t...
File environs/openstack/local_test.go (right):

https://codereview.appspot.com/12795045/diff/26001/environs/openstack/local_t...
environs/openstack/local_test.go:403: // HP servers are available once they are
BUILD(spawning).
Please put this comment as a first line of the function, since it's not a doc
comment.

https://codereview.appspot.com/12795045/diff/26001/environs/openstack/local_t...
environs/openstack/local_test.go:423: c.Assert(len(instances), Equals, 1)
c.Assert(instance, HasLen, 1)

[Michael Nelson, 2013-08-19 13:47:21]

Please take a look.

https://codereview.appspot.com/12795045/diff/26001/environs/openstack/local_t...
File environs/openstack/local_test.go (right):

https://codereview.appspot.com/12795045/diff/26001/environs/openstack/local_t...
environs/openstack/local_test.go:403: // HP servers are available once they are
BUILD(spawning).
On 2013/08/19 13:29:36, dimitern wrote:
> Please put this comment as a first line of the function, since it's not a doc
> comment.

Done.

https://codereview.appspot.com/12795045/diff/26001/environs/openstack/local_t...
environs/openstack/local_test.go:423: c.Assert(len(instances), Equals, 1)
On 2013/08/19 13:29:36, dimitern wrote:
> c.Assert(instance, HasLen, 1)

Done.