Introduce the null provider

Introduce the null provider

The null provider is a provider which
does, essentially, nothing. Instances
cannot be provisioned automatically;
they must be manually provisioned via
"add-machine ssh:...".

To bootstrap the environment, you name
the bootstrap node, which must be an
existing Ubuntu host, and it will be
"manually bootstrapped" via SSH. This
procedure is essentially the same as
manual provisioning.

Storage is managed by the bootstrap node.

TODO: storage is currently unauthenticated,
and the world can write to it. I will
address this next.

https://code.launchpad.net/~axwalk/juju-core/null-provider-take3/+merge/186954

(do not edit description out of merge proposal)

[axw, 2013-09-23 02:04:48]

Please take a look.

[fwereade, 2013-09-23 16:32:25]

OK, I am torn. There's some really great stuff here, and then there are some
really evil bits that are completely my fault because I didn't put my foot down
last time and demand that the local provider not get special treatment.

So, not your fault, but we need to figure out the right path forward. There are
a few different problems, all of which need to be addressed at some stage:

1) Tools data sources for both these environments really should be done
properly, with simplestreams, and without abusing the upload-tools pathway.

2) JobHostEnvironStorage ought to exist, be set by the *provider* by configuring
the cloud-init (or equivalent) such that jujud bootstrap-state injects a machine
with the right jobs and we can eliminate the special-casing in jujud/machine.go

3) JobManageEnvironFirewall, done as above. Along with JHES above, we'll have
interesting problems to solve come HA time, but it's a step in a sane direction.

...but it's plainly ridiculous to block this CL on implementing those three
things. So.

(1) Please talk to ian to figure out the state of play here; it may be that
upload-tools does everything nicely and simplestreamsily already, in which case
my only quibble is the special-casing by provider type in juju/bootstrap and
that's a tech-debt bug for another day.

(2) This can be landed as-is, but must be resolved quickly. Write a bug, assign
it to yourself, and queue it behind the storage authentication work.

(3) Eh, it's a tech-debt bug if you just return nil from all the ports methods.
It will become much easier to fix once (2) is resolved, though, because there'll
be a custom-jobs pathway from the provider into state.

I'll try to be online tonight so we can chat live -- I think this is very close
to landing if we can agree on the issue and responses.

https://codereview.appspot.com/13255051/diff/1/cmd/juju/bootstrap.go
File cmd/juju/bootstrap.go (left):

https://codereview.appspot.com/13255051/diff/1/cmd/juju/bootstrap.go#oldcode90
cmd/juju/bootstrap.go:90: c.UploadTools = true
This bit stresses me out every time I see it, and ISTM that we're expanding the
hack rather than fixing it properly. Why can't the null provider get its tools
from simplestreams as usual, and allow the user to specify a tools url
explicitly for isolated environments, like openstack does?

(for that matter, while this is not your problem specifically, why can't the
local provider itself know how to look for suitable tools locally, and expose
*that* as a data source for simplestreams?)

Ian, I'd appreciate your input here.

https://codereview.appspot.com/13255051/diff/1/cmd/juju/bootstrap.go
File cmd/juju/bootstrap.go (right):

https://codereview.appspot.com/13255051/diff/1/cmd/juju/bootstrap.go#newcode92
cmd/juju/bootstrap.go:92: case provider.Local:
case provider.Null, provider.Local:
    ?

https://codereview.appspot.com/13255051/diff/1/cmd/jujud/machine.go
File cmd/jujud/machine.go (left):

https://codereview.appspot.com/13255051/diff/1/cmd/jujud/machine.go#oldcode240
cmd/jujud/machine.go:240: // the storage provider on one machine, and that is
the "bootstrap" node.
Oo, I don't like that comment. If we're going to be HA, storage is going to have
to be HA too. Not one for today though.

https://codereview.appspot.com/13255051/diff/1/cmd/jujud/machine.go
File cmd/jujud/machine.go (right):

https://codereview.appspot.com/13255051/diff/1/cmd/jujud/machine.go#newcode241
cmd/jujud/machine.go:241: if (providerType == provider.Local || providerType ==
provider.Null) && m.Id() == bootstrapMachineId {
This *really* should not be expressed like this. Special-casing one provider
type was a pretty bad layering violation, and two exceptions feel uncomfortably
like a trend.

I think the only way we can reasonably do this is to tweak the bootstrap
pathways for the local and null providers, such that they inject an additional
machine job (say, JobHostEnvironStorage) on the bootstrap node.

https://codereview.appspot.com/13255051/diff/1/provider/null/config.go
File provider/null/config.go (right):

https://codereview.appspot.com/13255051/diff/1/provider/null/config.go#newcode23
provider/null/config.go:23: "bootstrap-host": "",
Isn't this required? Or, ha, will it just run locally? :)

https://codereview.appspot.com/13255051/diff/1/provider/null/config.go#newcode25
provider/null/config.go:25: "storage-ip":     "",
storage-listen-ip, maybe?

https://codereview.appspot.com/13255051/diff/1/provider/null/config.go#newcode27
provider/null/config.go:27: "storage-tmpdir": "/var/lib/juju/tmp-storage",
Not sure these two should be configurable at this level. I reckon a storage job
is just another job, run by an agent, storing its state inside the agent's dir
somewhere. *Might* make sense to have the global juju data-dir configurable for
a given environment, but that feels a bit tangential at this stage.

https://codereview.appspot.com/13255051/diff/1/provider/null/config_test.go
File provider/null/config_test.go (right):

https://codereview.appspot.com/13255051/diff/1/provider/null/config_test.go#n...
provider/null/config_test.go:33: "bootstrap-host": "hostname",
Don't we need several of the others?

https://codereview.appspot.com/13255051/diff/1/provider/null/config_test.go#n...
provider/null/config_test.go:73: 
separate test here?

https://codereview.appspot.com/13255051/diff/1/provider/null/environ.go
File provider/null/environ.go (right):

https://codereview.appspot.com/13255051/diff/1/provider/null/environ.go#newco...
provider/null/environ.go:101: return instances, err
This method feels a bit barbed. I see why you need it (for instance.Addresses
etc) but it's definitely hairy enough that it could use a warning in the
comment.

https://codereview.appspot.com/13255051/diff/1/provider/null/environ.go#newco...
provider/null/environ.go:130: return errNoClosePorts
Better to log and return nil in the ports methods (and on the instance) lest the
firewaller send itself into a frenzy of repeated failure.

Better yet, probably: fiddle with machine jobs a bit more, so that providers
that don't use them don't even start firewaller tasks.

https://codereview.appspot.com/13255051/diff/1/provider/null/instance.go
File provider/null/instance.go (right):

https://codereview.appspot.com/13255051/diff/1/provider/null/instance.go#newc...
provider/null/instance.go:58: func (_ nullBootstrapInstance) OpenPorts(machineId
string, ports []instance.Port) error {
Similarly, these should return nil (or firewalling should be fixed;))

https://codereview.appspot.com/13255051/diff/1/provider/null/provider.go
File provider/null/provider.go (right):

https://codereview.appspot.com/13255051/diff/1/provider/null/provider.go#newc...
provider/null/provider.go:105: # storage-tmpdir: /var/lib/juju/tmp-storage
Yeah, I'd rather not put this level of detail in. Can we skip configurable
storage directories for now, and just say that at this stage you get
"/var/lib/juju" and that's that?

[axw1, 2013-09-24 03:05:56]

https://codereview.appspot.com/13255051/diff/1/cmd/juju/bootstrap.go
File cmd/juju/bootstrap.go (left):

https://codereview.appspot.com/13255051/diff/1/cmd/juju/bootstrap.go#oldcode90
cmd/juju/bootstrap.go:90: c.UploadTools = true
On 2013/09/23 16:32:25, fwereade wrote:
> This bit stresses me out every time I see it, and ISTM that we're expanding
the
> hack rather than fixing it properly. Why can't the null provider get its tools
> from simplestreams as usual, and allow the user to specify a tools url
> explicitly for isolated environments, like openstack does?

No good reason. I did it this way during development (I think there might have
been some breakage with simplestreams along the way), and never updated to do it
properly. This file can safely be reverted.

> (for that matter, while this is not your problem specifically, why can't the
> local provider itself know how to look for suitable tools locally, and expose
> *that* as a data source for simplestreams?)
> 
> Ian, I'd appreciate your input here.

https://codereview.appspot.com/13255051/diff/1/cmd/juju/bootstrap.go
File cmd/juju/bootstrap.go (right):

https://codereview.appspot.com/13255051/diff/1/cmd/juju/bootstrap.go#newcode92
cmd/juju/bootstrap.go:92: case provider.Local:
On 2013/09/23 16:32:25, fwereade wrote:
> case provider.Null, provider.Local:
>     ?

Heh, yes. Excuse me while I take my C hat off.

https://codereview.appspot.com/13255051/diff/1/cmd/jujud/machine.go
File cmd/jujud/machine.go (left):

https://codereview.appspot.com/13255051/diff/1/cmd/jujud/machine.go#oldcode240
cmd/jujud/machine.go:240: // the storage provider on one machine, and that is
the "bootstrap" node.
On 2013/09/23 16:32:25, fwereade wrote:
> Oo, I don't like that comment. If we're going to be HA, storage is going to
have
> to be HA too. Not one for today though.

Is the null provider ever going to need to be HA? I thought this was purely for
"scale down", though I suppose we *might* want to support a fully fledged
scale-out environment, just without the automatic provisioning.

https://codereview.appspot.com/13255051/diff/1/cmd/jujud/machine.go
File cmd/jujud/machine.go (right):

https://codereview.appspot.com/13255051/diff/1/cmd/jujud/machine.go#newcode241
cmd/jujud/machine.go:241: if (providerType == provider.Local || providerType ==
provider.Null) && m.Id() == bootstrapMachineId {
On 2013/09/23 16:32:25, fwereade wrote:
> This *really* should not be expressed like this. Special-casing one provider
> type was a pretty bad layering violation, and two exceptions feel
uncomfortably
> like a trend.
> 
> I think the only way we can reasonably do this is to tweak the bootstrap
> pathways for the local and null providers, such that they inject an additional
> machine job (say, JobHostEnvironStorage) on the bootstrap node.

That makes sense. There's a bunch of things to touch, so I have created:
https://bugs.launchpad.net/juju-core/+bug/1229507
and will follow up. If you prefer I do this now, though, I can.

https://codereview.appspot.com/13255051/diff/1/provider/null/config.go
File provider/null/config.go (right):

https://codereview.appspot.com/13255051/diff/1/provider/null/config.go#newcode23
provider/null/config.go:23: "bootstrap-host": "",
On 2013/09/23 16:32:25, fwereade wrote:
> Isn't this required? Or, ha, will it just run locally? :)

Yes, it is required; Validate checks that it's != "". I'll take this default
out.

https://codereview.appspot.com/13255051/diff/1/provider/null/config.go#newcode25
provider/null/config.go:25: "storage-ip":     "",
On 2013/09/23 16:32:25, fwereade wrote:
> storage-listen-ip, maybe?

Done.

https://codereview.appspot.com/13255051/diff/1/provider/null/config.go#newcode27
provider/null/config.go:27: "storage-tmpdir": "/var/lib/juju/tmp-storage",
On 2013/09/23 16:32:25, fwereade wrote:
> Not sure these two should be configurable at this level. I reckon a storage
job
> is just another job, run by an agent, storing its state inside the agent's dir
> somewhere. *Might* make sense to have the global juju data-dir configurable
for
> a given environment, but that feels a bit tangential at this stage.

Fair enough, I have fixed the location relative to data-dir.

https://codereview.appspot.com/13255051/diff/1/provider/null/config_test.go
File provider/null/config_test.go (right):

https://codereview.appspot.com/13255051/diff/1/provider/null/config_test.go#n...
provider/null/config_test.go:33: "bootstrap-host": "hostname",
On 2013/09/23 16:32:25, fwereade wrote:
> Don't we need several of the others?

This is the minimal set. Everything else has defaults. Here's my real, working
environments.yaml:

"null":
  type: "null"
  admin-secret: b3661e7a8949b955bc447d4123417943
  default-series: raring
  bootstrap-host: wimpy

https://codereview.appspot.com/13255051/diff/1/provider/null/config_test.go#n...
provider/null/config_test.go:73: 
On 2013/09/23 16:32:25, fwereade wrote:
> separate test here?

Done.

https://codereview.appspot.com/13255051/diff/1/provider/null/environ.go
File provider/null/environ.go (right):

https://codereview.appspot.com/13255051/diff/1/provider/null/environ.go#newco...
provider/null/environ.go:101: return instances, err
On 2013/09/23 16:32:25, fwereade wrote:
> This method feels a bit barbed. I see why you need it (for instance.Addresses
> etc) but it's definitely hairy enough that it could use a warning in the
> comment.

I've done so, though its behaviour is as described in the Environ interface.

https://codereview.appspot.com/13255051/diff/1/provider/null/environ.go#newco...
provider/null/environ.go:130: return errNoClosePorts
On 2013/09/23 16:32:25, fwereade wrote:
> Better to log and return nil in the ports methods (and on the instance) lest
the
> firewaller send itself into a frenzy of repeated failure.
> 
> Better yet, probably: fiddle with machine jobs a bit more, so that providers
> that don't use them don't even start firewaller tasks.

Have modified it to return nil for now.

I've briefly looked into this, and found that it touches quite a few areas of
code. I'd like to leave a TODO here, and come back to it shortly.

https://codereview.appspot.com/13255051/diff/1/provider/null/instance.go
File provider/null/instance.go (right):

https://codereview.appspot.com/13255051/diff/1/provider/null/instance.go#newc...
provider/null/instance.go:58: func (_ nullBootstrapInstance) OpenPorts(machineId
string, ports []instance.Port) error {
On 2013/09/23 16:32:25, fwereade wrote:
> Similarly, these should return nil (or firewalling should be fixed;))

Returning nil for now.

https://codereview.appspot.com/13255051/diff/1/provider/null/provider.go
File provider/null/provider.go (right):

https://codereview.appspot.com/13255051/diff/1/provider/null/provider.go#newc...
provider/null/provider.go:105: # storage-tmpdir: /var/lib/juju/tmp-storage
On 2013/09/23 16:32:25, fwereade wrote:
> Yeah, I'd rather not put this level of detail in. Can we skip configurable
> storage directories for now, and just say that at this stage you get
> "/var/lib/juju" and that's that?

Done.

[axw, 2013-09-24 04:01:46]

Please take a look.

[fwereade, 2013-09-26 08:03:41]

We need to do more work wrt "shutting down" manually provisioned instances -- ie
removing juju therefrom -- but most of that I'm happy to leave to a followup.

I'm not quite sure whether the complete non-functioning of Destroy is a good
thing or not. Given that destroying doesn't actually destroy anything, maybe
it's good that we don't trash storage either... but in that case I think we
should just return a not-implemented sort of error, and keep going to provide a
decent shutdown experience. Thoughts? Quick discussion/confirmation here is the
only thing delaying approval.

https://codereview.appspot.com/13255051/diff/1/cmd/jujud/machine.go
File cmd/jujud/machine.go (left):

https://codereview.appspot.com/13255051/diff/1/cmd/jujud/machine.go#oldcode240
cmd/jujud/machine.go:240: // the storage provider on one machine, and that is
the "bootstrap" node.
On 2013/09/24 03:05:57, axw1 wrote:
> On 2013/09/23 16:32:25, fwereade wrote:
> > Oo, I don't like that comment. If we're going to be HA, storage is going to
> have
> > to be HA too. Not one for today though.
> 
> Is the null provider ever going to need to be HA? I thought this was purely
for
> "scale down", though I suppose we *might* want to support a fully fledged
> scale-out environment, just without the automatic provisioning.

One day it will. Happy to defer consideration of that for now though.

https://codereview.appspot.com/13255051/diff/11001/cmd/jujud/machine.go
File cmd/jujud/machine.go (right):

https://codereview.appspot.com/13255051/diff/11001/cmd/jujud/machine.go#newco...
cmd/jujud/machine.go:259: // are capable of managing ports at a global level.
FWIW the firewaller handles instance-level provisioning too.

https://codereview.appspot.com/13255051/diff/11001/provider/null/environ.go
File provider/null/environ.go (right):

https://codereview.appspot.com/13255051/diff/11001/provider/null/environ.go#n...
provider/null/environ.go:76: func (e *nullEnviron) Bootstrap(_
constraints.Value, possibleTools tools.List, machineID string) error {
I really think we ought to drop machineID (which would be more conventionally
spelled machineId, I think) from Bootstrap, and the id should always be "0". But
that's not a job for today.

https://codereview.appspot.com/13255051/diff/11001/provider/null/environ.go#n...
provider/null/environ.go:142: return fmt.Errorf("null provider cannot destroy
instances: %v", insts)
Is there *any* way to destroy an environment then? It's always got the bootstrap
machine in it, regardless of whether it's passed those instances.

https://codereview.appspot.com/13255051/diff/11001/provider/null/environ_test.go
File provider/null/environ_test.go (right):

https://codereview.appspot.com/13255051/diff/11001/provider/null/environ_test...
provider/null/environ_test.go:72: c.Assert(s.env.Destroy(nil), gc.IsNil)
Yeah, it bothers me that this can complete without either errors or impact. It's
a deeper issue than can be managed in this CL, though -- it kinda comes down to:

1) destroy-environment should probably involve setting every machine except the
manager to Dead (possibly breaking expectations in order to do so -- eg killing
them with active units in play), allowing the provisioner to clean them all up
neatly, before setting the provisioner's own machine to Dead. Not clear how much
work this is in practice.

2) manually provisioned machine agents should be able to clean themselves up
completely once they've set themselves to Dead.

Separately, remind me: what cleans up environment storage? That might be our
responsibility in this method.

https://codereview.appspot.com/13255051/diff/11001/provider/null/instance.go
File provider/null/instance.go (right):

https://codereview.appspot.com/13255051/diff/11001/provider/null/instance.go#...
provider/null/instance.go:41: // If the user specified bootsrap-host as an IP
address,
s/bootsrap/bootstrap/

[axw1, 2013-09-26 08:23:00]

https://codereview.appspot.com/13255051/diff/11001/cmd/jujud/machine.go
File cmd/jujud/machine.go (right):

https://codereview.appspot.com/13255051/diff/11001/cmd/jujud/machine.go#newco...
cmd/jujud/machine.go:259: // are capable of managing ports at a global level.
On 2013/09/26 08:03:41, fwereade wrote:
> FWIW the firewaller handles instance-level provisioning too.

Probably a better way of writing the comment would be "centrally", rather than
"at a global level".

https://codereview.appspot.com/13255051/diff/11001/provider/null/environ_test.go
File provider/null/environ_test.go (right):

https://codereview.appspot.com/13255051/diff/11001/provider/null/environ_test...
provider/null/environ_test.go:72: c.Assert(s.env.Destroy(nil), gc.IsNil)
On 2013/09/26 08:03:41, fwereade wrote:
> Yeah, it bothers me that this can complete without either errors or impact.
It's
> a deeper issue than can be managed in this CL, though -- it kinda comes down
to:
> 
> 1) destroy-environment should probably involve setting every machine except
the
> manager to Dead (possibly breaking expectations in order to do so -- eg
killing
> them with active units in play), allowing the provisioner to clean them all up
> neatly, before setting the provisioner's own machine to Dead. Not clear how
much
> work this is in practice.

I wasn't really sure where to draw the line for null/manual. This sounds
reasonable, though. I think this shouldn't be *too* much work.

> 2) manually provisioned machine agents should be able to clean themselves up
> completely once they've set themselves to Dead.

This still works, from the previous work on manual provisioning.

> Separately, remind me: what cleans up environment storage? That might be our
> responsibility in this method.

Currently not done for the null-provider. I think the simplest thing to do would
be for the machine agent to remove its data-dir on uninstallation (storage lives
under there).

https://codereview.appspot.com/13255051/diff/11001/provider/null/instance.go
File provider/null/instance.go (right):

https://codereview.appspot.com/13255051/diff/11001/provider/null/instance.go#...
provider/null/instance.go:41: // If the user specified bootsrap-host as an IP
address,
On 2013/09/26 08:03:41, fwereade wrote:
> s/bootsrap/bootstrap/

Thanks, will fix.

[fwereade, 2013-09-26 08:49:56]

On 2013/09/26 08:23:00, axw1 wrote:
> > 1) destroy-environment should probably involve setting every machine except
> the
> > manager to Dead (possibly breaking expectations in order to do so -- eg
> killing
> > them with active units in play), allowing the provisioner to clean them all
up
> > neatly, before setting the provisioner's own machine to Dead. Not clear how
> much
> > work this is in practice.
> 
> I wasn't really sure where to draw the line for null/manual. This sounds
> reasonable, though. I think this shouldn't be *too* much work.

Excellent. You'll probably want a reasonably detailed chat with me about state
soon, then.
 
> > 2) manually provisioned machine agents should be able to clean themselves up
> > completely once they've set themselves to Dead.
> 
> This still works, from the previous work on manual provisioning.

Sweet! I couldn't remember if we'd done that.

> > Separately, remind me: what cleans up environment storage? That might be our
> > responsibility in this method.
> 
> Currently not done for the null-provider. I think the simplest thing to do
would
> be for the machine agent to remove its data-dir on uninstallation (storage
lives
> under there).

OK, that sounds pretty reasonable, but let's not set it in stone. We can figure
out what works best in the context of how we actually do it.

What do you think about returning an error from Destroy now, though? We really
don't have any sensible reason to claim the operation worked...

[axw1, 2013-09-26 08:54:27]

https://codereview.appspot.com/13255051/diff/11001/provider/null/environ.go
File provider/null/environ.go (right):

https://codereview.appspot.com/13255051/diff/11001/provider/null/environ.go#n...
provider/null/environ.go:142: return fmt.Errorf("null provider cannot destroy
instances: %v", insts)
On 2013/09/26 08:03:41, fwereade wrote:
> Is there *any* way to destroy an environment then? It's always got the
bootstrap
> machine in it, regardless of whether it's passed those instances.

No, not entirely at the moment. Agreed, this should return an error for now
while the rest is worked out. I will do that.

[axw, 2013-09-26 09:09:39]

Please take a look.

[fwereade, 2013-09-26 09:20:17]

LGTM then, destroying properly can come in followups.