Issue 7226068: openstack: bootstrap w/ public IP + fix (Closed)
|
Created:
11 years, 3 months ago by dimitern Modified:
11 years, 3 months ago Reviewers:
mp+145818 Visibility:
Public. |
Descriptionopenstack: bootstrap w/ public IP + fix
Now after starting the bootstrap instance, a public floating IP
address will be assigned (either available or a new one).
Also fixes mgo/api ports, because it was not building it properly
with constants.
https://code.launchpad.net/~dimitern/juju-core/openstack-bootstrap-fixes/+merge/145818
(do not edit description out of merge proposal)
Patch Set 1 #
Total comments: 6
Patch Set 2 : openstack: bootstrap w/ public IP + fix #
Total comments: 6
Patch Set 3 : openstack: bootstrap w/ public IP + fix #
Total comments: 20
Patch Set 4 : openstack: bootstrap w/ public IP + fix #Patch Set 5 : openstack: bootstrap w/ public IP + fix #
Total comments: 8
Patch Set 6 : openstack: bootstrap w/ public IP + fix #Patch Set 7 : openstack: bootstrap w/ public IP + fix #
MessagesTotal messages: 15
Please take a look.
Sign in to reply to this message.
looks good. a couple of comments below, and could do with some tests, as discussed on line. https://codereview.appspot.com/7226068/diff/1/environs/openstack/provider.go File environs/openstack/provider.go (right): https://codereview.appspot.com/7226068/diff/1/environs/openstack/provider.go#... environs/openstack/provider.go:490: if fipInstId, ok := fip.InstanceId.(string); ok && fipInstId == serverId { if fipInstId, _ := fib.InstanceId.(string); fipInstId == serverId { // the instance already has an IP assigned return nil } else if fipInstId == "" { newfip = &fip break } ? but as mentioned on line, it would be better if fib.InstanceId was *string not interface{} https://codereview.appspot.com/7226068/diff/1/environs/openstack/provider.go#... environs/openstack/provider.go:508: if err == nil { shouldn't this be checking for some specific kind of error that happens in the expected non-cached case? https://codereview.appspot.com/7226068/diff/1/environs/openstack/provider.go#... environs/openstack/provider.go:579: if err = e.assignPublicIP(string(inst.Id())); err != nil { s/=/:=/ (no need to make the reader look for side effects when they're not needed)
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/7226068/diff/1/environs/openstack/provider.go File environs/openstack/provider.go (right): https://codereview.appspot.com/7226068/diff/1/environs/openstack/provider.go#... environs/openstack/provider.go:490: if fipInstId, ok := fip.InstanceId.(string); ok && fipInstId == serverId { On 2013/01/31 12:53:11, rog wrote: > if fipInstId, _ := fib.InstanceId.(string); fipInstId == serverId { > // the instance already has an IP assigned > return nil > } else if fipInstId == "" { > newfip = &fip > break > } > > ? > > but as mentioned on line, it would be better if fib.InstanceId was *string not > interface{} As agreed, I'll leave this like it is for now, then fix goose and do a follow-up. https://codereview.appspot.com/7226068/diff/1/environs/openstack/provider.go#... environs/openstack/provider.go:508: if err == nil { On 2013/01/31 12:53:11, rog wrote: > shouldn't this be checking for some specific kind of error that happens in the > expected non-cached case? There could be a whole lot of reasons why this fails, so it's not easy to check specific cases. But as discussed, in a follow-up I'll do better live/local tests about this (after goose is changed). https://codereview.appspot.com/7226068/diff/1/environs/openstack/provider.go#... environs/openstack/provider.go:579: if err = e.assignPublicIP(string(inst.Id())); err != nil { On 2013/01/31 12:53:11, rog wrote: > s/=/:=/ > > (no need to make the reader look for side effects when they're not needed) Done.
Sign in to reply to this message.
Aside from the comments below, we should have a test for the new logic. https://codereview.appspot.com/7226068/diff/5/environs/openstack/provider.go File environs/openstack/provider.go (right): https://codereview.appspot.com/7226068/diff/5/environs/openstack/provider.go#... environs/openstack/provider.go:32: var apiPortSuffix = fmt.Sprintf(":%d", apiPort) Why did these change? Doesn't look like a bad change, but maybe not for this CL? https://codereview.appspot.com/7226068/diff/5/environs/openstack/provider.go#... environs/openstack/provider.go:483: func (e *environ) assignPublicIP(serverId string) error { As discussed live, this should probably be split into allocatePublicIp and assignPublicIp, so we can fail fast when bootstrapping in environments with none available (but with the appropriate config setting set). https://codereview.appspot.com/7226068/diff/5/environs/openstack/provider.go#... environs/openstack/provider.go:581: e.terminateInstances([]state.InstanceId{inst.Id()}) For this sort of situation, please log the error. "error while handling public IP assignment failure: %v" perhaps?
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/7226068/diff/5/environs/openstack/provider.go File environs/openstack/provider.go (right): https://codereview.appspot.com/7226068/diff/5/environs/openstack/provider.go#... environs/openstack/provider.go:32: var apiPortSuffix = fmt.Sprintf(":%d", apiPort) On 2013/01/31 14:22:59, fwereade wrote: > Why did these change? Doesn't look like a bad change, but maybe not for this CL? Because string(int) converts the int rune to string, rather than the number to string. https://codereview.appspot.com/7226068/diff/5/environs/openstack/provider.go#... environs/openstack/provider.go:483: func (e *environ) assignPublicIP(serverId string) error { On 2013/01/31 14:22:59, fwereade wrote: > As discussed live, this should probably be split into allocatePublicIp and > assignPublicIp, so we can fail fast when bootstrapping in environments with none > available (but with the appropriate config setting set). Done. https://codereview.appspot.com/7226068/diff/5/environs/openstack/provider.go#... environs/openstack/provider.go:581: e.terminateInstances([]state.InstanceId{inst.Id()}) On 2013/01/31 14:22:59, fwereade wrote: > For this sort of situation, please log the error. "error while handling public > IP assignment failure: %v" perhaps? Done.
Sign in to reply to this message.
LGTM with a few minor points below. https://codereview.appspot.com/7226068/diff/7001/environs/openstack/export_te... File environs/openstack/export_test.go (right): https://codereview.appspot.com/7226068/diff/7001/environs/openstack/export_te... environs/openstack/export_test.go:89: return env.allocatePublicIP() or return e.(*environ).allocatePublicIP() *shrug* https://codereview.appspot.com/7226068/diff/7001/environs/openstack/provider.go File environs/openstack/provider.go (right): https://codereview.appspot.com/7226068/diff/7001/environs/openstack/provider.... environs/openstack/provider.go:602: log.Debugf("environs/openstack: failed to terminate instance %q", inst.Id()) you've missed out the error you were intending to log. there's no particular reason to use err1 rather than err here either. https://codereview.appspot.com/7226068/diff/7001/environs/openstack/provider.... environs/openstack/provider.go:604: return nil, fmt.Errorf("cannot assign a public address %s to instance %q: %s", publicIP.IP, inst.Id(), err.Error()) s/ a //
Sign in to reply to this message.
One remark about the code; but some concern that we're not writing the tests at the right level, and that leaves a fair amount of logic untested. Opinions? https://codereview.appspot.com/7226068/diff/7001/environs/openstack/live_test.go File environs/openstack/live_test.go (right): https://codereview.appspot.com/7226068/diff/7001/environs/openstack/live_test... environs/openstack/live_test.go:173: c.Skip("targets the test double only") This is a bit confusing, to be honest... does anyone have any thoughts on how we could maybe express which tests should be run when with a bit more clarity? (Unrelated to CL) https://codereview.appspot.com/7226068/diff/7001/environs/openstack/live_test... environs/openstack/live_test.go:184: c.Assert(fip, IsNil) I think we need to test the behaviour at the resolution of the Environ interface. We should be able to call Bootstrap and verify that a floating ip is allocated and assigned to a fresh instance; or verify how we react to failure in any given step, in terms of how the double's state ends up; and since this code affects StartInstance too, I feel we should have more tests there. Thoughts? https://codereview.appspot.com/7226068/diff/7001/environs/openstack/provider.go File environs/openstack/provider.go (right): https://codereview.appspot.com/7226068/diff/7001/environs/openstack/provider.... environs/openstack/provider.go:603: } I think we should probably release the floating IP at this point. Counterarguments?
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/7226068/diff/7001/environs/openstack/export_te... File environs/openstack/export_test.go (right): https://codereview.appspot.com/7226068/diff/7001/environs/openstack/export_te... environs/openstack/export_test.go:89: return env.allocatePublicIP() On 2013/02/01 18:01:55, rog wrote: > or return e.(*environ).allocatePublicIP() > *shrug* Done. https://codereview.appspot.com/7226068/diff/7001/environs/openstack/live_test.go File environs/openstack/live_test.go (right): https://codereview.appspot.com/7226068/diff/7001/environs/openstack/live_test... environs/openstack/live_test.go:173: c.Skip("targets the test double only") On 2013/02/01 18:15:37, fwereade wrote: > This is a bit confusing, to be honest... does anyone have any thoughts on how we > could maybe express which tests should be run when with a bit more clarity? > (Unrelated to CL) It's code smell, that's what it is :) I need environ to call AllocationPublicIP on (it's needed to use the nova client in there), which is initialized here, and I need the openstackservice instance to register the hooks onto, which gets initialized in local_test. Couldn't find a better way around it. If I move it to local_test (as I originally thought) how can I get the environ there? https://codereview.appspot.com/7226068/diff/7001/environs/openstack/live_test... environs/openstack/live_test.go:184: c.Assert(fip, IsNil) On 2013/02/01 18:15:37, fwereade wrote: > I think we need to test the behaviour at the resolution of the Environ > interface. We should be able to call Bootstrap and verify that a floating ip is > allocated and assigned to a fresh instance; or verify how we react to failure in > any given step, in terms of how the double's state ends up; and since this code > affects StartInstance too, I feel we should have more tests there. > > Thoughts? I decided to test the IP allocation, because is the only different step from bootstrapping an ec2 env. But thinking about it, I could simplify a lot by just calling Bootstrap here instead and the test will be the same. Would it be enough? https://codereview.appspot.com/7226068/diff/7001/environs/openstack/provider.go File environs/openstack/provider.go (right): https://codereview.appspot.com/7226068/diff/7001/environs/openstack/provider.... environs/openstack/provider.go:602: log.Debugf("environs/openstack: failed to terminate instance %q", inst.Id()) On 2013/02/01 18:01:55, rog wrote: > you've missed out the error you were intending to log. > there's no particular reason to use err1 rather than err here either. Done. https://codereview.appspot.com/7226068/diff/7001/environs/openstack/provider.... environs/openstack/provider.go:603: } On 2013/02/01 18:15:37, fwereade wrote: > I think we should probably release the floating IP at this point. > Counterarguments? No need - nova reclaims them once their assigned instance dies. https://codereview.appspot.com/7226068/diff/7001/environs/openstack/provider.... environs/openstack/provider.go:604: return nil, fmt.Errorf("cannot assign a public address %s to instance %q: %s", publicIP.IP, inst.Id(), err.Error()) On 2013/02/01 18:01:55, rog wrote: > s/ a // Done.
Sign in to reply to this message.
https://codereview.appspot.com/7226068/diff/7001/environs/openstack/live_test.go File environs/openstack/live_test.go (right): https://codereview.appspot.com/7226068/diff/7001/environs/openstack/live_test... environs/openstack/live_test.go:173: c.Skip("targets the test double only") On 2013/02/01 18:26:42, dimitern wrote: > On 2013/02/01 18:15:37, fwereade wrote: > > This is a bit confusing, to be honest... does anyone have any thoughts on how > we > > could maybe express which tests should be run when with a bit more clarity? > > (Unrelated to CL) > > It's code smell, that's what it is :) I need environ to call AllocationPublicIP > on (it's needed to use the nova client in there), which is initialized here, and > I need the openstackservice instance to register the hooks onto, which gets > initialized in local_test. Couldn't find a better way around it. If I move it to > local_test (as I originally thought) how can I get the environ there? As discussed last night, these tests all really ought to be working against actual Environs -- certainly there is sometimes justification for testing individual private components via export_test, but this should usually not replace testing of the public interface. https://codereview.appspot.com/7226068/diff/7001/environs/openstack/live_test... environs/openstack/live_test.go:184: c.Assert(fip, IsNil) On 2013/02/01 18:26:42, dimitern wrote: > On 2013/02/01 18:15:37, fwereade wrote: > > I think we need to test the behaviour at the resolution of the Environ > > interface. We should be able to call Bootstrap and verify that a floating ip > is > > allocated and assigned to a fresh instance; or verify how we react to failure > in > > any given step, in terms of how the double's state ends up; and since this > code > > affects StartInstance too, I feel we should have more tests there. > > > > Thoughts? > > I decided to test the IP allocation, because is the only different step from > bootstrapping an ec2 env. But thinking about it, I could simplify a lot by just > calling Bootstrap here instead and the test will be the same. Would it be > enough? Calling Bootstrap is definitely good here -- but we should follow up by verifying that no instance is running and no floating ip is allocated (and in the usual case, we should be verifying that that an instance does exist and has a floating ip assigned). https://codereview.appspot.com/7226068/diff/7001/environs/openstack/local_tes... File environs/openstack/local_test.go (right): https://codereview.appspot.com/7226068/diff/7001/environs/openstack/local_tes... environs/openstack/local_test.go:51: s.LiveTests.SetUpSuite(c, srv) We shouldn't be doing this to the live tests.
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/7226068/diff/7001/environs/openstack/live_test.go File environs/openstack/live_test.go (right): https://codereview.appspot.com/7226068/diff/7001/environs/openstack/live_test... environs/openstack/live_test.go:173: c.Skip("targets the test double only") On 2013/02/04 16:02:36, fwereade wrote: > On 2013/02/01 18:26:42, dimitern wrote: > > On 2013/02/01 18:15:37, fwereade wrote: > > > This is a bit confusing, to be honest... does anyone have any thoughts on > how > > we > > > could maybe express which tests should be run when with a bit more clarity? > > > (Unrelated to CL) > > > > It's code smell, that's what it is :) I need environ to call > AllocationPublicIP > > on (it's needed to use the nova client in there), which is initialized here, > and > > I need the openstackservice instance to register the hooks onto, which gets > > initialized in local_test. Couldn't find a better way around it. If I move it > to > > local_test (as I originally thought) how can I get the environ there? > > As discussed last night, these tests all really ought to be working against > actual Environs -- certainly there is sometimes justification for testing > individual private components via export_test, but this should usually not > replace testing of the public interface. Done. https://codereview.appspot.com/7226068/diff/7001/environs/openstack/live_test... environs/openstack/live_test.go:184: c.Assert(fip, IsNil) On 2013/02/04 16:02:36, fwereade wrote: > On 2013/02/01 18:26:42, dimitern wrote: > > On 2013/02/01 18:15:37, fwereade wrote: > > > I think we need to test the behaviour at the resolution of the Environ > > > interface. We should be able to call Bootstrap and verify that a floating ip > > is > > > allocated and assigned to a fresh instance; or verify how we react to > failure > > in > > > any given step, in terms of how the double's state ends up; and since this > > code > > > affects StartInstance too, I feel we should have more tests there. > > > > > > Thoughts? > > > > I decided to test the IP allocation, because is the only different step from > > bootstrapping an ec2 env. But thinking about it, I could simplify a lot by > just > > calling Bootstrap here instead and the test will be the same. Would it be > > enough? > > Calling Bootstrap is definitely good here -- but we should follow up by > verifying that no instance is running and no floating ip is allocated (and in > the usual case, we should be verifying that that an instance does exist and has > a floating ip assigned). Done. https://codereview.appspot.com/7226068/diff/7001/environs/openstack/local_tes... File environs/openstack/local_test.go (right): https://codereview.appspot.com/7226068/diff/7001/environs/openstack/local_tes... environs/openstack/local_test.go:51: s.LiveTests.SetUpSuite(c, srv) On 2013/02/04 16:02:36, fwereade wrote: > We shouldn't be doing this to the live tests. Done.
Sign in to reply to this message.
LGTM with a couple of trivials below. https://codereview.appspot.com/7226068/diff/7001/environs/openstack/live_test.go File environs/openstack/live_test.go (right): https://codereview.appspot.com/7226068/diff/7001/environs/openstack/live_test... environs/openstack/live_test.go:68: openstack *openstackservice.Openstack glad to see this go. that's what the localLiveSuite type is for. https://codereview.appspot.com/7226068/diff/15001/environs/openstack/provider.go File environs/openstack/provider.go (right): https://codereview.appspot.com/7226068/diff/15001/environs/openstack/provider... environs/openstack/provider.go:602: log.Debugf("environs/openstack: failed to terminate instance %q: %s", inst.Id(), err.Error()) log.Debugf("environs/openstack: failed to terminate instance %q: %v", inst.Id(), err) (no need to use .Error, and %v is more conventional for error printing) https://codereview.appspot.com/7226068/diff/15001/environs/openstack/provider... environs/openstack/provider.go:604: return nil, fmt.Errorf("cannot assign public address %s to instance %q: %s", publicIP.IP, inst.Id(), err.Error()) ditto
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/7226068/diff/7001/environs/openstack/live_test.go File environs/openstack/live_test.go (right): https://codereview.appspot.com/7226068/diff/7001/environs/openstack/live_test... environs/openstack/live_test.go:68: openstack *openstackservice.Openstack On 2013/02/04 18:02:53, rog wrote: > glad to see this go. that's what the localLiveSuite type is for. Whew :) yeah https://codereview.appspot.com/7226068/diff/15001/environs/openstack/provider.go File environs/openstack/provider.go (right): https://codereview.appspot.com/7226068/diff/15001/environs/openstack/provider... environs/openstack/provider.go:602: log.Debugf("environs/openstack: failed to terminate instance %q: %s", inst.Id(), err.Error()) On 2013/02/04 18:02:53, rog wrote: > log.Debugf("environs/openstack: failed to terminate instance %q: %v", inst.Id(), > err) > > (no need to use .Error, and %v is more conventional for error printing) Done. https://codereview.appspot.com/7226068/diff/15001/environs/openstack/provider... environs/openstack/provider.go:604: return nil, fmt.Errorf("cannot assign public address %s to instance %q: %s", publicIP.IP, inst.Id(), err.Error()) On 2013/02/04 18:02:53, rog wrote: > ditto Done.
Sign in to reply to this message.
LGTM, just trivials https://codereview.appspot.com/7226068/diff/15001/environs/openstack/live_tes... File environs/openstack/live_test.go (right): https://codereview.appspot.com/7226068/diff/15001/environs/openstack/live_tes... environs/openstack/live_test.go:110: May as well drop this change ;). https://codereview.appspot.com/7226068/diff/15001/environs/openstack/provider.go File environs/openstack/provider.go (right): https://codereview.appspot.com/7226068/diff/15001/environs/openstack/provider... environs/openstack/provider.go:605: } else { Drop the else?
Sign in to reply to this message.
Cheers! Submitting with the proposed changes. https://codereview.appspot.com/7226068/diff/15001/environs/openstack/live_tes... File environs/openstack/live_test.go (right): https://codereview.appspot.com/7226068/diff/15001/environs/openstack/live_tes... environs/openstack/live_test.go:110: On 2013/02/04 19:19:26, fwereade wrote: > May as well drop this change ;). Done. https://codereview.appspot.com/7226068/diff/15001/environs/openstack/provider.go File environs/openstack/provider.go (right): https://codereview.appspot.com/7226068/diff/15001/environs/openstack/provider... environs/openstack/provider.go:605: } else { On 2013/02/04 19:19:26, fwereade wrote: > Drop the else? Done.
Sign in to reply to this message.
*** Submitted: openstack: bootstrap w/ public IP + fix Now after starting the bootstrap instance, a public floating IP address will be assigned (either available or a new one). Also fixes mgo/api ports, because it was not building it properly with constants. R=rog, fwereade CC= https://codereview.appspot.com/7226068
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
