Update add-machine for manual provisioning

Update add-machine for manual provisioning

juju add-machine is updated to use a new package,
environs/manual, to manually provision tools and
a machine agent to an existing machine.

When a manually provisioned machine is destroyed
via juju destroy-machine, the machine agent will
detect its termination and remove its upstart
configuration file. There is currently no cleanup
of the data or log directories; this will be done
in a follow-up pending discussion.

When the machine goes to Dead, a provisioner will
remove the machine from state just like any other
machine.

TODO: destroy-environment will currently leak
manually provisioned machines. A follow-up will
address this by requiring users to individually
destroy-machine before destroy-environment will
proceed. Alternatively (or perhaps additionally),
destroy-environment may take a flag to automatically
do this.

https://code.launchpad.net/~axwalk/juju-core/juju-add-machine/+merge/179840

(do not edit description out of merge proposal)

[thumper, 2013-08-15 02:50:47]

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

https://codereview.appspot.com/12831043/diff/1/cmd/juju/addmachine.go#newcode72
cmd/juju/addmachine.go:72: if strings.HasPrefix(containerSpec, "ssh:") {
Since we use the "ssh:" magic string twice, perhaps we move it to a constant,
sshSpec (my background makes me want to name it SSH_SPEC, but then it'd be
exported :-( )

https://codereview.appspot.com/12831043/diff/1/cmd/juju/addmachine.go#newcode127
cmd/juju/addmachine.go:127: func sshHostAddresses(host string)
([]instance.Address, error) {
I imagine that we may want this in a utility package.  Seems like we could
easily get really good test coverage of this method.

https://codereview.appspot.com/12831043/diff/1/cmd/juju/addmachine.go#newcode133
cmd/juju/addmachine.go:133: ipaddrs, err := net.LookupIP(host)
What if the host is an ip address?

https://codereview.appspot.com/12831043/diff/1/cmd/juju/addmachine.go#newcode144
cmd/juju/addmachine.go:144: addrs[i].Type = instance.Ipv6Address
Oh how I wish cloud vendors would have good IPv6 support.

https://codereview.appspot.com/12831043/diff/1/cmd/juju/addmachine.go#newcode151
cmd/juju/addmachine.go:151: func (c *AddMachineCommand)
manuallyProvisionMachine(conn *juju.Conn) (resultErr error) {
I'd really like to see this method outside of the command package.  To me it
doesn't feel right to be there.  Perhaps move to a file inside "environ" ?

Second obvious thing is the length of this function.  We should break it up into
little bits.  Perhaps a method per list item.

https://codereview.appspot.com/12831043/diff/1/cmd/juju/addmachine.go#newcode161
cmd/juju/addmachine.go:161: // 4. Set up remote port forwarding to the storage
host/port.
Why do we need this port forwarding?  If the machine being added can see the
state server, this shouldn't be a problem no?

https://codereview.appspot.com/12831043/diff/1/cmd/juju/addmachine.go#newcode178
cmd/juju/addmachine.go:178: env, err := environs.NewFromName(c.EnvName)
Why create a new environment when the conn parameter should have one already?

https://codereview.appspot.com/12831043/diff/1/cmd/juju/addmachine.go#newcode182
cmd/juju/addmachine.go:182: hcCons :=
constraints.MustParse(strings.Fields(hc.String())...)
Care to add a comment here that this is just for the architecture?  I'm assuming
it is.

https://codereview.appspot.com/12831043/diff/1/cmd/juju/addmachine.go#newcode198
cmd/juju/addmachine.go:198: // InjectMachine implicitly specifies the nonce as
BootstrapNonce. Is that wrong?
Yeah... inject machine assumes that the only reason you'd be injecting a machine
is because you are bootstrapping.  This assumption should be parameterised or
removed, or someone changed.

https://codereview.appspot.com/12831043/diff/1/cmd/juju/addmachine.go#newcode199
cmd/juju/addmachine.go:199: instanceId := instance.Id("ssh:" + c.SSHHost)
Oh, here goes the "ssh:" string again.

https://codereview.appspot.com/12831043/diff/1/cmd/juju/addmachine.go#newcode200
cmd/juju/addmachine.go:200: m, err := conn.State.InjectMachine(series,
c.Constraints, instanceId, hc, state.JobHostUnits)
We need to add another thing to the machine doc in mongo to identify the "type"
of provider.  The problem at this stage is we aren't entirely too sure how this
is going to work.  I'm thinking that perhaps we just have a single string, that
defaults to omit, and if it isn't there or empty it is the default provider (we
only have one for now), but otherwise it should be the provider type ("maas",
"ec2", "openstack", "local", "azure"), or "manual" for manually provisioned, or
the container type ("lxc", "kvm").

https://codereview.appspot.com/12831043/diff/1/cmd/juju/addmachine.go#newcode204
cmd/juju/addmachine.go:204: fmt.Println("Created machine:", m)
Should be outputting to the context's Stdout.

https://codereview.appspot.com/12831043/diff/1/cmd/juju/addmachine.go#newcode236
cmd/juju/addmachine.go:236: serviceEnv :=
map[string]string{"JUJU_PROVIDER_TYPE": env.Config().Type()}
well, the provider type here is really "manual" :)  also, the name here is now
in juju/osenv package.  But when I added it, I didn't try to change all uses of
the names, but instead opted to change as we go.

https://codereview.appspot.com/12831043/diff/1/cmd/juju/addmachine.go#newcode364
cmd/juju/addmachine.go:364: const detectionScript = `#!/bin/bash
Obviously this will only work for linux.  How would we have an early escape
valve if someone tried to run this on a windows box?

Also probably worth testing against a non-ubuntu linux, like CentOS to see what
explodes.  I'd be tempted to explicitly not support adding any non-ubuntu
machines for the first iteration.

[axw1, 2013-08-15 05:22:05]

On 2013/08/15 02:50:47, thumper wrote:
> https://codereview.appspot.com/12831043/diff/1/cmd/juju/addmachine.go
> File cmd/juju/addmachine.go (right):
> 
>
https://codereview.appspot.com/12831043/diff/1/cmd/juju/addmachine.go#newcode72
> cmd/juju/addmachine.go:72: if strings.HasPrefix(containerSpec, "ssh:") {
> Since we use the "ssh:" magic string twice, perhaps we move it to a constant,
> sshSpec (my background makes me want to name it SSH_SPEC, but then it'd be
> exported :-( )

Will do. A number of things are ugly and need to be cleaned up, just putting
this out there to see that it wasn't fundamentally wrong.

>
https://codereview.appspot.com/12831043/diff/1/cmd/juju/addmachine.go#newcode127
> cmd/juju/addmachine.go:127: func sshHostAddresses(host string)
> ([]instance.Address, error) {
> I imagine that we may want this in a utility package.  Seems like we could
> easily get really good test coverage of this method.
> 
>
https://codereview.appspot.com/12831043/diff/1/cmd/juju/addmachine.go#newcode133
> cmd/juju/addmachine.go:133: ipaddrs, err := net.LookupIP(host)
> What if the host is an ip address?

LookupIP don't care.

>
https://codereview.appspot.com/12831043/diff/1/cmd/juju/addmachine.go#newcode144
> cmd/juju/addmachine.go:144: addrs[i].Type = instance.Ipv6Address
> Oh how I wish cloud vendors would have good IPv6 support.
> 
>
https://codereview.appspot.com/12831043/diff/1/cmd/juju/addmachine.go#newcode151
> cmd/juju/addmachine.go:151: func (c *AddMachineCommand)
> manuallyProvisionMachine(conn *juju.Conn) (resultErr error) {
> I'd really like to see this method outside of the command package.  To me it
> doesn't feel right to be there.  Perhaps move to a file inside "environ" ?

Will do. Perhaps environs/manual?

> Second obvious thing is the length of this function.  We should break it up
into
> little bits.  Perhaps a method per list item.
> 
>
https://codereview.appspot.com/12831043/diff/1/cmd/juju/addmachine.go#newcode161
> cmd/juju/addmachine.go:161: // 4. Set up remote port forwarding to the storage
> host/port.
> Why do we need this port forwarding?  If the machine being added can see the
> state server, this shouldn't be a problem no?

Yeah, I took this out afterwards. It was dumb :)
Instead, I've checking the result of wget and printing out a semi-helpful
message along the lines of "ensure the machine has access to the state server
and storage".

>
https://codereview.appspot.com/12831043/diff/1/cmd/juju/addmachine.go#newcode178
> cmd/juju/addmachine.go:178: env, err := environs.NewFromName(c.EnvName)
> Why create a new environment when the conn parameter should have one already?
> 
>
https://codereview.appspot.com/12831043/diff/1/cmd/juju/addmachine.go#newcode182
> cmd/juju/addmachine.go:182: hcCons :=
> constraints.MustParse(strings.Fields(hc.String())...)
> Care to add a comment here that this is just for the architecture?  I'm
assuming
> it is.

This is gone now. Turns out using FindInstanceTools doesn't work so well here,
as it filters based on the version of the context, which in this case is the
juju command. The juju command version may not relate to any of the tools in
storage. Instead, I'm just using the same tools as the bootstrap machine.

> 
>
https://codereview.appspot.com/12831043/diff/1/cmd/juju/addmachine.go#newcode198
> cmd/juju/addmachine.go:198: // InjectMachine implicitly specifies the nonce as
> BootstrapNonce. Is that wrong?
> Yeah... inject machine assumes that the only reason you'd be injecting a
machine
> is because you are bootstrapping.  This assumption should be parameterised or
> removed, or someone changed.

Will do.

>
https://codereview.appspot.com/12831043/diff/1/cmd/juju/addmachine.go#newcode199
> cmd/juju/addmachine.go:199: instanceId := instance.Id("ssh:" + c.SSHHost)
> Oh, here goes the "ssh:" string again.
> 
>
https://codereview.appspot.com/12831043/diff/1/cmd/juju/addmachine.go#newcode200
> cmd/juju/addmachine.go:200: m, err := conn.State.InjectMachine(series,
> c.Constraints, instanceId, hc, state.JobHostUnits)
> We need to add another thing to the machine doc in mongo to identify the
"type"
> of provider.  The problem at this stage is we aren't entirely too sure how
this
> is going to work.  I'm thinking that perhaps we just have a single string,
that
> defaults to omit, and if it isn't there or empty it is the default provider
(we
> only have one for now), but otherwise it should be the provider type ("maas",
> "ec2", "openstack", "local", "azure"), or "manual" for manually provisioned,
or
> the container type ("lxc", "kvm").

Okay, I'll tackle that next.
 
>
https://codereview.appspot.com/12831043/diff/1/cmd/juju/addmachine.go#newcode204
> cmd/juju/addmachine.go:204: fmt.Println("Created machine:", m)
> Should be outputting to the context's Stdout.

Shouldn't be there at all :) Leftover debugging.

>
https://codereview.appspot.com/12831043/diff/1/cmd/juju/addmachine.go#newcode236
> cmd/juju/addmachine.go:236: serviceEnv :=
> map[string]string{"JUJU_PROVIDER_TYPE": env.Config().Type()}
> well, the provider type here is really "manual" :)  also, the name here is now
> in juju/osenv package.  But when I added it, I didn't try to change all uses
of
> the names, but instead opted to change as we go.

Thanks, I'll update. Obviously there's no manual provider (yet), so for now I'll
add a TODO to set it correctly when it exists.

>
https://codereview.appspot.com/12831043/diff/1/cmd/juju/addmachine.go#newcode364
> cmd/juju/addmachine.go:364: const detectionScript = `#!/bin/bash
> Obviously this will only work for linux.  How would we have an early escape
> valve if someone tried to run this on a windows box?

Yes, only for Linux.
How many people run SSH daemons on Windows?

> Also probably worth testing against a non-ubuntu linux, like CentOS to see
what
> explodes.  I'd be tempted to explicitly not support adding any non-ubuntu
> machines for the first iteration.

I will look into CentOS after things are in good shape for Ubuntu.

[axw, 2013-08-20 03:56:19]

Please take a look.

[axw, 2013-08-20 06:47:47]

Please take a look.

[fwereade, 2013-08-21 11:21:50]

Lots and lots of comments, not all actionable; this is going in an excellent
direction. I'd appreciate your thoughts.

https://codereview.appspot.com/12831043/diff/8002/cmd/juju/addmachine.go
File cmd/juju/addmachine.go (right):

https://codereview.appspot.com/12831043/diff/8002/cmd/juju/addmachine.go#newc...
cmd/juju/addmachine.go:30: be able to access the tools storage.`[1:]
s/tools storage/environment storage/

https://codereview.appspot.com/12831043/diff/8002/cmd/juju/addmachine.go#newc...
cmd/juju/addmachine.go:102: }
I'm a little bit concerned about the structure here. "lxc:3" and "ssh:user@host"
will before long be joined by thinks like "aws:us-east-1b" and
"my-maas:some-ginormous-server"; I think it would be sensible to start as we
mean to go on, by making explicit the notion of a placement directive and
parsing these strings as such.

I'd probably accept a 3-strikes-refactor argument so long as the requirement to
do so were clearly articulated in a comment here.

https://codereview.appspot.com/12831043/diff/8002/cmd/juju/destroyenvironment.go
File cmd/juju/destroyenvironment.go (right):

https://codereview.appspot.com/12831043/diff/8002/cmd/juju/destroyenvironment...
cmd/juju/destroyenvironment.go:54: //           machines have been manually
"destroyed".
FWIW, I think it's becoming clearer and clearer that destroy-environment really
ought to be handled server-side.

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

https://codereview.appspot.com/12831043/diff/8002/cmd/jujud/machine.go#newcod...
cmd/jujud/machine.go:168: providerType := os.Getenv(osenv.JujuProviderType)
I'm not sure this should be communicated via an environment variable. Why can't
we ask the API server what sort of machine we are?

https://codereview.appspot.com/12831043/diff/8002/cmd/jujud/machine.go#newcod...
cmd/jujud/machine.go:173: worker = &uninstallWorker{worker}
I don't think this is valid, is it? If the agent gets bounced at the wrong time,
we'll fail to connect and never get uninstalled because we never get here.

IIRC, there's some top-level handling of ErrTerminateAgent that converts it into
a nil return; can you think of a reason not to uninstall oneself at that point
regardless of provider type?

https://codereview.appspot.com/12831043/diff/8002/cmd/jujud/machine.go#newcod...
cmd/jujud/machine.go:316: name := os.Getenv("UPSTART_JOB")
I'm not too sure about this either, even though it is locally determined
information. Talk to thumper about his agent.Conf changes; I think that might be
a better place for it.

https://codereview.appspot.com/12831043/diff/8002/cmd/jujud/machine.go#newcod...
cmd/jujud/machine.go:320: return upstart.NewService(name).Remove(false)
I think we also need to remove the machine in state.

https://codereview.appspot.com/12831043/diff/8002/environs/bootstrap.go
File environs/bootstrap.go (right):

https://codereview.appspot.com/12831043/diff/8002/environs/bootstrap.go#newco...
environs/bootstrap.go:127: m, err := st.InjectMachine(version.Current.Series,
cons, instId, characteristics, state.BootstrapNonce, jobs...)
This is starting to look like it would appreciate a params struct.

https://codereview.appspot.com/12831043/diff/8002/environs/manual/agent.go
File environs/manual/agent.go (right):

https://codereview.appspot.com/12831043/diff/8002/environs/manual/agent.go#ne...
environs/manual/agent.go:24: var defaultLogDir = "/var/log/juju"
Ouch... how many places define this now? It's not especially easy to clean up,
but I think it possibly deserves a tech-debt bug.

https://codereview.appspot.com/12831043/diff/8002/environs/manual/agent.go#ne...
environs/manual/agent.go:26: const upstartServiceName = "jujud-machine-manual"
Please use the standard upstart job naming, which includes the agent id. (or
make everything just use "jujud-machine-agent", I guess -- one agent per machine
should be guaranteed)

https://codereview.appspot.com/12831043/diff/8002/environs/manual/agent.go#ne...
environs/manual/agent.go:89: return fmt.Errorf("error generating agent
configuration: %v", err)
Can we split the func at about this point please? There was a verbal requirement
communicated from on high at IOM that we should be able to run this command
manually on a machine we've already connected to; absent compelling technical
reasons not to, I'd like to make it as easy as possible to expose that
functionality a few CLs down the line (and I suspect it'd ease the testing
complexity a little too).

https://codereview.appspot.com/12831043/diff/8002/environs/manual/agent.go#ne...
environs/manual/agent.go:138: %s`
I'm a bit concerned about the overlap between this and the stuff in cloudinit.
Is there any way we can extract the common features? This will only become more
important as we try to provision more and more OSs... long-term, I'd hope for a
platform- and situation-agnostic core type with different renderers for
different situations.

https://codereview.appspot.com/12831043/diff/8002/environs/manual/detection.go
File environs/manual/detection.go (right):

https://codereview.appspot.com/12831043/diff/8002/environs/manual/detection.g...
environs/manual/detection.go:55: // logical cores due to hyperthreading.
I'm a bit worried that we don't have a consistent definition of cpu-cores across
providers, but I guess it's not even really possible given that this could be
working against hardware, or on a VM provisioned by any number of other possible
backends. This seems sensible, though.

https://codereview.appspot.com/12831043/diff/8002/environs/manual/detection_t...
File environs/manual/detection_test.go (right):

https://codereview.appspot.com/12831043/diff/8002/environs/manual/detection_t...
environs/manual/detection_test.go:42: func sshresponse(c *gc.C, output string,
rc int) func() {
Is it reasonable to add something that checks for expected input too?

https://codereview.appspot.com/12831043/diff/8002/environs/manual/detection_t...
environs/manual/detection_test.go:83: 
Can we split these into separate test cases please? Or make them table-based?

https://codereview.appspot.com/12831043/diff/8002/environs/manual/provisioner.go
File environs/manual/provisioner.go (right):

https://codereview.appspot.com/12831043/diff/8002/environs/manual/provisioner...
environs/manual/provisioner.go:50: Tools *tools.Tools
I don't think we can decide this at this point. The environment config's
agent-version, plus the series/arch determined by detectSeries..., should be
used instead.

https://codereview.appspot.com/12831043/diff/8002/environs/manual/provisioner...
environs/manual/provisioner.go:92: //           know not to touch it.
I think this kinda needs to be a prereq, doesn't it?

https://codereview.appspot.com/12831043/diff/8002/environs/manual/provisioner...
environs/manual/provisioner.go:172: // bootstrap machine's.
I think we can and should. It uses env.Config().AgentVersion, which I think is
correct.

https://codereview.appspot.com/12831043/diff/8002/environs/manual/provisioner...
environs/manual/provisioner.go:197: }
Thanks for this file in particular, which is very nicely readable.

https://codereview.appspot.com/12831043/diff/8002/environs/manual/provisioner...
File environs/manual/provisioner_test.go (right):

https://codereview.appspot.com/12831043/diff/8002/environs/manual/provisioner...
environs/manual/provisioner_test.go:84: // Why would you permit an instance ID
to be reused?
I'm not sure we can guarantee that all providers have non-overlapping sets of
instance ids.

Maybe we should be checking for the presence of an existing juju machine agent
and bailing before we even try to install/inject?

[axw1, 2013-08-22 06:17:40]

Thanks for the review. I've addressed what I can today, and will continue
working on addressing the remaining comments.

https://codereview.appspot.com/12831043/diff/8002/cmd/juju/addmachine.go
File cmd/juju/addmachine.go (right):

https://codereview.appspot.com/12831043/diff/8002/cmd/juju/addmachine.go#newc...
cmd/juju/addmachine.go:30: be able to access the tools storage.`[1:]
On 2013/08/21 11:21:51, fwereade wrote:
> s/tools storage/environment storage/

Done.

https://codereview.appspot.com/12831043/diff/8002/cmd/juju/addmachine.go#newc...
cmd/juju/addmachine.go:102: }
On 2013/08/21 11:21:51, fwereade wrote:
> I'm a little bit concerned about the structure here. "lxc:3" and
"ssh:user@host"
> will before long be joined by thinks like "aws:us-east-1b" and
> "my-maas:some-ginormous-server"; I think it would be sensible to start as we
> mean to go on, by making explicit the notion of a placement directive and
> parsing these strings as such.
> 
> I'd probably accept a 3-strikes-refactor argument so long as the requirement
to
> do so were clearly articulated in a comment here.

Baking "ssh:" into placement directives sounds sensible, but I'm not aware of
all the requirements for placement directives. Since placement directives is a
whole other task unto itself, I would prefer deferring if you'll accept that. I
will add a TODO here.

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

https://codereview.appspot.com/12831043/diff/8002/cmd/jujud/machine.go#newcod...
cmd/jujud/machine.go:168: providerType := os.Getenv(osenv.JujuProviderType)
On 2013/08/21 11:21:51, fwereade wrote:
> I'm not sure this should be communicated via an environment variable. Why
can't
> we ask the API server what sort of machine we are?

We could, but I don't see why you would. That information is static and known
from the beginning. What's the benefit of going through the API server to
introspect?

Anyway, this is no longer needed after addressing your next comment.

https://codereview.appspot.com/12831043/diff/8002/cmd/jujud/machine.go#newcod...
cmd/jujud/machine.go:173: worker = &uninstallWorker{worker}
On 2013/08/21 11:21:51, fwereade wrote:
> I don't think this is valid, is it? If the agent gets bounced at the wrong
time,
> we'll fail to connect and never get uninstalled because we never get here.

Yes, you're right. I missed the fact that opening the state/API may return
ErrTerminateAgent as well.

> IIRC, there's some top-level handling of ErrTerminateAgent that converts it
into
> a nil return; can you think of a reason not to uninstall oneself at that point
> regardless of provider type?

That sounds sensible, and moots the above discussion about the
osenv.JujuProviderType environment variable. I can't think of any reason not to
do it for other provider types.

I'll have to do it in the machine agent code, though, not in agent, as the
deployer takes care of removing unit upstart config.

https://codereview.appspot.com/12831043/diff/8002/cmd/jujud/machine.go#newcod...
cmd/jujud/machine.go:316: name := os.Getenv("UPSTART_JOB")
On 2013/08/21 11:21:51, fwereade wrote:
> I'm not too sure about this either, even though it is locally determined
> information. Talk to thumper about his agent.Conf changes; I think that might
be
> a better place for it.

thumper intends to add a section to agent config for arbitrary key/value pairs.
I'll add a TODO here to use that when it is available (which he said won't be
until he's finished refactoring the current stuff).

Though I have to say, in this case, I prefer the environment because it's being
communicated directly by upstart. Recording the job name in agent config seems
redundant.

https://codereview.appspot.com/12831043/diff/8002/cmd/jujud/machine.go#newcod...
cmd/jujud/machine.go:320: return upstart.NewService(name).Remove(false)
On 2013/08/21 11:21:51, fwereade wrote:
> I think we also need to remove the machine in state.

This is currently done by the provisioner task, in the bootstrap node's machine
agent.

Should the manually provisioned machine agent be responsible for cleaning itself
up from state? What if, for whatever reason, it can't access state? Do we deal
with that with "destroy-machine --force" when we have it?

https://codereview.appspot.com/12831043/diff/8002/environs/bootstrap.go
File environs/bootstrap.go (right):

https://codereview.appspot.com/12831043/diff/8002/environs/bootstrap.go#newco...
environs/bootstrap.go:127: m, err := st.InjectMachine(version.Current.Series,
cons, instId, characteristics, state.BootstrapNonce, jobs...)
On 2013/08/21 11:21:51, fwereade wrote:
> This is starting to look like it would appreciate a params struct.

Agreed. Especially when we add in the provider type. I'll do this in a followup,
as this will require a bunch of changes in state.

https://codereview.appspot.com/12831043/diff/8002/environs/manual/agent.go
File environs/manual/agent.go (right):

https://codereview.appspot.com/12831043/diff/8002/environs/manual/agent.go#ne...
environs/manual/agent.go:24: var defaultLogDir = "/var/log/juju"
On 2013/08/21 11:21:51, fwereade wrote:
> Ouch... how many places define this now? It's not especially easy to clean up,
> but I think it possibly deserves a tech-debt bug.

https://bugs.launchpad.net/juju-core/+bug/1215252

https://codereview.appspot.com/12831043/diff/8002/environs/manual/agent.go#ne...
environs/manual/agent.go:26: const upstartServiceName = "jujud-machine-manual"
On 2013/08/21 11:21:51, fwereade wrote:
> Please use the standard upstart job naming, which includes the agent id. (or
> make everything just use "jujud-machine-agent", I guess -- one agent per
machine
> should be guaranteed)

I'll just change this one for now.

https://codereview.appspot.com/12831043/diff/8002/environs/manual/agent.go#ne...
environs/manual/agent.go:89: return fmt.Errorf("error generating agent
configuration: %v", err)
On 2013/08/21 11:21:51, fwereade wrote:
> Can we split the func at about this point please? There was a verbal
requirement
> communicated from on high at IOM that we should be able to run this command
> manually on a machine we've already connected to; absent compelling technical
> reasons not to, I'd like to make it as easy as possible to expose that
> functionality a few CLs down the line (and I suspect it'd ease the testing
> complexity a little too).

I'll rename this function to something that returns a script (cut just after
"script := ..." below), and then create a new function that runs that and
executes it via SSH.

https://codereview.appspot.com/12831043/diff/8002/environs/manual/agent.go#ne...
environs/manual/agent.go:138: %s`
On 2013/08/21 11:21:51, fwereade wrote:
> I'm a bit concerned about the overlap between this and the stuff in cloudinit.
> Is there any way we can extract the common features? This will only become
more
> important as we try to provision more and more OSs... long-term, I'd hope for
a
> platform- and situation-agnostic core type with different renderers for
> different situations.

I will see what I can come up with. The cloud-init stuff rightly assumes that it
can do whatever it wants with the machine, whereas I think manual provisioning
should not.

I think we could just generate the cloud-init config, then pull out the bits
we're interested in. i.e. packages to install, and the runcmd script. Messing
with SSH keys and so on, I think, should be left to the user.

https://codereview.appspot.com/12831043/diff/8002/environs/manual/detection_t...
File environs/manual/detection_test.go (right):

https://codereview.appspot.com/12831043/diff/8002/environs/manual/detection_t...
environs/manual/detection_test.go:42: func sshresponse(c *gc.C, output string,
rc int) func() {
On 2013/08/21 11:21:51, fwereade wrote:
> Is it reasonable to add something that checks for expected input too?

Sure, done.

https://codereview.appspot.com/12831043/diff/8002/environs/manual/detection_t...
environs/manual/detection_test.go:83: 
On 2013/08/21 11:21:51, fwereade wrote:
> Can we split these into separate test cases please? Or make them table-based?

Done.

https://codereview.appspot.com/12831043/diff/8002/environs/manual/provisioner.go
File environs/manual/provisioner.go (right):

https://codereview.appspot.com/12831043/diff/8002/environs/manual/provisioner...
environs/manual/provisioner.go:50: Tools *tools.Tools
On 2013/08/21 11:21:51, fwereade wrote:
> I don't think we can decide this at this point. The environment config's
> agent-version, plus the series/arch determined by detectSeries..., should be
> used instead.

Oops, yes, of course. I'll rework this.

https://codereview.appspot.com/12831043/diff/8002/environs/manual/provisioner...
environs/manual/provisioner.go:92: //           know not to touch it.
On 2013/08/21 11:21:51, fwereade wrote:
> I think this kinda needs to be a prereq, doesn't it?

Okay. I'll work on a proposal for that before going further with this.

https://codereview.appspot.com/12831043/diff/8002/environs/manual/provisioner...
environs/manual/provisioner.go:172: // bootstrap machine's.
On 2013/08/21 11:21:51, fwereade wrote:
> I think we can and should. It uses env.Config().AgentVersion, which I think is
> correct.

I think it might be just a local provider thing. When it uploads tools to the
storage, it increments the build number. So the version in storage doesn't match
the version of the juju command.

If you look at provider/local/environprovider.go, it sets agent-version to the
version of the juju tool if it's not already set in the environment config.
These two things were getting in the way, but I can see that using the bootstrap
tools isn't any good either. I'll take another look into it.

https://codereview.appspot.com/12831043/diff/8002/environs/manual/provisioner...
environs/manual/provisioner.go:197: }
On 2013/08/21 11:21:51, fwereade wrote:
> Thanks for this file in particular, which is very nicely readable.

You're welcome :)

https://codereview.appspot.com/12831043/diff/8002/environs/manual/provisioner...
File environs/manual/provisioner_test.go (right):

https://codereview.appspot.com/12831043/diff/8002/environs/manual/provisioner...
environs/manual/provisioner_test.go:84: // Why would you permit an instance ID
to be reused?
On 2013/08/21 11:21:51, fwereade wrote:
> I'm not sure we can guarantee that all providers have non-overlapping sets of
> instance ids.

Are you talking about cross-provider? Surely within a provider you can? If the
provider can guarantee uniqueness within its scope, then all you need to do is
add a unique provider-specific prefix to the instance IDs.

Sorry, probably not the place for this discussion!

> Maybe we should be checking for the presence of an existing juju machine agent
> and bailing before we even try to install/inject?

I guess so. It avoids adding/removing a machine, which would allocate a machine
ID that can't be reused. I'd rather not make another round-trip, but OTOH I
don't want to overload the detection function/script.

[fwereade, 2013-08-22 16:29:47]

A few more comments; if you've got a prereq on its way, I'm WIPping this until
it's a candidate again.

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

https://codereview.appspot.com/12831043/diff/8002/cmd/jujud/machine.go#newcod...
cmd/jujud/machine.go:320: return upstart.NewService(name).Remove(false)
On 2013/08/22 06:17:41, axw1 wrote:
> On 2013/08/21 11:21:51, fwereade wrote:
> > I think we also need to remove the machine in state.
> 
> This is currently done by the provisioner task, in the bootstrap node's
machine
> agent.

The trouble is that that bit's flaky -- the provisioner (or associated
infrastructure) needs to change so that it doesn't attempt to stop a
non-instance and fall over. Running distinct provisioners whose watchers report
per-environment machines is probably the smartest approach.

> Should the manually provisioned machine agent be responsible for cleaning
itself
> up from state? What if, for whatever reason, it can't access state? Do we deal
> with that with "destroy-machine --force" when we have it?

Ideally it will indeed be handled by the provisioner(s) -- I got the
maybe-mistaken impression that we were minimizing the provisioner changes at
this point. I'm a bit ambivalent about the practicality of doing so -- might
well be smarter to make sure they can handle it *first* -- but I'm also somewhat
sensitive to tim's argument that the observable suckage is (I *think*) internal
and that progress on the actual provisioning is riskier and hence higher
priority.

https://codereview.appspot.com/12831043/diff/8002/environs/manual/agent.go
File environs/manual/agent.go (right):

https://codereview.appspot.com/12831043/diff/8002/environs/manual/agent.go#ne...
environs/manual/agent.go:138: %s`
On 2013/08/22 06:17:41, axw1 wrote:
> On 2013/08/21 11:21:51, fwereade wrote:
> > I'm a bit concerned about the overlap between this and the stuff in
cloudinit.
> > Is there any way we can extract the common features? This will only become
> more
> > important as we try to provision more and more OSs... long-term, I'd hope
for
> a
> > platform- and situation-agnostic core type with different renderers for
> > different situations.
> 
> I will see what I can come up with. The cloud-init stuff rightly assumes that
it
> can do whatever it wants with the machine, whereas I think manual provisioning
> should not.
> 
> I think we could just generate the cloud-init config, then pull out the bits
> we're interested in. i.e. packages to install, and the runcmd script. Messing
> with SSH keys and so on, I think, should be left to the user.

Hmm. If/when ssh-keys-per-user, and machine-updates-authkeys, functionality
lands, this'll make manually provisioned machines second-class citizens. I think
we should aim to minimize the observable differences between manually and
automatically provisioned machines, really.

FWIW I don't think it's critical that we consolidate it in this CL, I just don't
want us to forget about it.

https://codereview.appspot.com/12831043/diff/8002/environs/manual/provisioner.go
File environs/manual/provisioner.go (right):

https://codereview.appspot.com/12831043/diff/8002/environs/manual/provisioner...
environs/manual/provisioner.go:172: // bootstrap machine's.
On 2013/08/22 06:17:41, axw1 wrote:
> On 2013/08/21 11:21:51, fwereade wrote:
> > I think we can and should. It uses env.Config().AgentVersion, which I think
is
> > correct.
> 
> I think it might be just a local provider thing. When it uploads tools to the
> storage, it increments the build number. So the version in storage doesn't
match
> the version of the juju command.
> 
> If you look at provider/local/environprovider.go, it sets agent-version to the
> version of the juju tool if it's not already set in the environment config.
> These two things were getting in the way, but I can see that using the
bootstrap
> tools isn't any good either. I'll take another look into it.

The local provider abuses the upload-tools mechanism because we were rushing to
get it in by a deadline. What it's actually doing, conceptually, is sync-tools;
I suspect this weirdness is at the heart of the issue, and we probably need to
resolve that before we can unpick this sanely.

https://codereview.appspot.com/12831043/diff/8002/environs/manual/provisioner...
File environs/manual/provisioner_test.go (right):

https://codereview.appspot.com/12831043/diff/8002/environs/manual/provisioner...
environs/manual/provisioner_test.go:84: // Why would you permit an instance ID
to be reused?
On 2013/08/22 06:17:41, axw1 wrote:
> On 2013/08/21 11:21:51, fwereade wrote:
> > I'm not sure we can guarantee that all providers have non-overlapping sets
of
> > instance ids.
> 
> Are you talking about cross-provider? Surely within a provider you can? If the
> provider can guarantee uniqueness within its scope, then all you need to do is
> add a unique provider-specific prefix to the instance IDs.
> 
> Sorry, probably not the place for this discussion!

I think each provider can; but introducing manual provisioning *is* introducing
a new (pseudo-)provider into the environment, so I think this actually is the
time and place to figure it out cleanly :).

> > Maybe we should be checking for the presence of an existing juju machine
agent
> > and bailing before we even try to install/inject?
> 
> I guess so. It avoids adding/removing a machine, which would allocate a
machine
> ID that can't be reused. I'd rather not make another round-trip, but OTOH I
> don't want to overload the detection function/script.

I can see both sides; I expect I'll be comfortable enough with whichever you
pick. I feel your pain though ;).

[thumper, 2013-08-23 02:58:41]

More things to consider :-)

https://codereview.appspot.com/12831043/diff/8002/cmd/juju/destroyenvironment.go
File cmd/juju/destroyenvironment.go (right):

https://codereview.appspot.com/12831043/diff/8002/cmd/juju/destroyenvironment...
cmd/juju/destroyenvironment.go:54: //           machines have been manually
"destroyed".
On 2013/08/21 11:21:51, fwereade wrote:
> FWIW, I think it's becoming clearer and clearer that destroy-environment
really
> ought to be handled server-side.

Well I think that some of it is going to have to as it should be an API call
that does this now, and the server then responds to the "destroy-environment"
call.

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

https://codereview.appspot.com/12831043/diff/8002/cmd/jujud/machine.go#newcod...
cmd/jujud/machine.go:320: return upstart.NewService(name).Remove(false)
On 2013/08/22 16:29:47, fwereade wrote:
> On 2013/08/22 06:17:41, axw1 wrote:
> > On 2013/08/21 11:21:51, fwereade wrote:
> > > I think we also need to remove the machine in state.
> > 
> > This is currently done by the provisioner task, in the bootstrap node's
> machine
> > agent.
> 
> The trouble is that that bit's flaky -- the provisioner (or associated
> infrastructure) needs to change so that it doesn't attempt to stop a
> non-instance and fall over. Running distinct provisioners whose watchers
report
> per-environment machines is probably the smartest approach.

Actually, I was talking Andrew through this this afternoon, only to realise
that the environ provisioner won't fall over at all.  When we try to get the
instance ids for the machines, if the instance id isn't in the instances
returned
from the broker, it is silently ignored.  So as it is, right now, the existing
environment provisioner takes care of state for manually provisioned machines.

https://codereview.appspot.com/12831043/diff/8002/cmd/jujud/machine_test.go
File cmd/jujud/machine_test.go (right):

https://codereview.appspot.com/12831043/diff/8002/cmd/jujud/machine_test.go#n...
cmd/jujud/machine_test.go:68: nonce := state.BootstrapNonce
This will have to kinda change a bit now with the new
agent changes landed in trunk.

https://codereview.appspot.com/12831043/diff/8002/environs/manual/agent.go
File environs/manual/agent.go (right):

https://codereview.appspot.com/12831043/diff/8002/environs/manual/agent.go#ne...
environs/manual/agent.go:24: var defaultLogDir = "/var/log/juju"
On 2013/08/22 06:17:41, axw1 wrote:
> On 2013/08/21 11:21:51, fwereade wrote:
> > Ouch... how many places define this now? It's not especially easy to clean
up,
> > but I think it possibly deserves a tech-debt bug.
> 
> https://bugs.launchpad.net/juju-core/+bug/1215252

Please add a comment along the lines of:

// TODO(awx): 2013-08-23 bug 1215252
// Gather all these constants in a project wide package

https://codereview.appspot.com/12831043/diff/8002/environs/manual/agent.go#ne...
environs/manual/agent.go:26: const upstartServiceName = "jujud-machine-manual"
On 2013/08/21 11:21:51, fwereade wrote:
> Please use the standard upstart job naming, which includes the agent id. (or
> make everything just use "jujud-machine-agent", I guess -- one agent per
machine
> should be guaranteed)

We can't change the local provider ones, as it is possible for there to be
multiple local providers running.

https://codereview.appspot.com/12831043/diff/8002/environs/manual/agent.go#ne...
environs/manual/agent.go:79: var agentConf = agent.Conf{
You don't need APIPort nor StatePort here.

This is more obvious now with agent.NewAgentConfig

https://codereview.appspot.com/12831043/diff/8002/environs/manual/agent.go#ne...
environs/manual/agent.go:89: return fmt.Errorf("error generating agent
configuration: %v", err)
On 2013/08/22 06:17:41, axw1 wrote:
> On 2013/08/21 11:21:51, fwereade wrote:
> > Can we split the func at about this point please? There was a verbal
> requirement
> > communicated from on high at IOM that we should be able to run this command
> > manually on a machine we've already connected to; absent compelling
technical
> > reasons not to, I'd like to make it as easy as possible to expose that
> > functionality a few CLs down the line (and I suspect it'd ease the testing
> > complexity a little too).
> 
> I'll rename this function to something that returns a script (cut just after
> "script := ..." below), and then create a new function that runs that and
> executes it via SSH.

Awesome.  I'd be happy to land the add this other machine mode first.  Then work
out how to add "this machine to that juju environment" (which sounds like crazy
talk to me).

https://codereview.appspot.com/12831043/diff/8002/environs/manual/agent.go#ne...
environs/manual/agent.go:138: %s`
On 2013/08/22 16:29:47, fwereade wrote:
> FWIW I don't think it's critical that we consolidate it in this CL, I just
don't
> want us to forget about it.

In which case, file a bug and leave a referenced TODO comment.

https://codereview.appspot.com/12831043/diff/8002/environs/manual/detection.go
File environs/manual/detection.go (right):

https://codereview.appspot.com/12831043/diff/8002/environs/manual/detection.g...
environs/manual/detection.go:55: // logical cores due to hyperthreading.
On 2013/08/21 11:21:51, fwereade wrote:
> I'm a bit worried that we don't have a consistent definition of cpu-cores
across
> providers, but I guess it's not even really possible given that this could be
> working against hardware, or on a VM provisioned by any number of other
possible
> backends. This seems sensible, though.

Wondering why we don't go for something simple like:

 cat /proc/cpuinfo | grep "processor\s\+:" | wc -l

?

https://codereview.appspot.com/12831043/diff/8002/environs/manual/provisioner.go
File environs/manual/provisioner.go (right):

https://codereview.appspot.com/12831043/diff/8002/environs/manual/provisioner...
environs/manual/provisioner.go:92: //           know not to touch it.
On 2013/08/22 06:17:41, axw1 wrote:
> On 2013/08/21 11:21:51, fwereade wrote:
> > I think this kinda needs to be a prereq, doesn't it?
> 
> Okay. I'll work on a proposal for that before going further with this.

We looked and decided it isn't needed right now.

https://codereview.appspot.com/12831043/diff/8002/environs/manual/provisioner...
File environs/manual/provisioner_test.go (right):

https://codereview.appspot.com/12831043/diff/8002/environs/manual/provisioner...
environs/manual/provisioner_test.go:84: // Why would you permit an instance ID
to be reused?
I think if part of the bash script that gets executed to install the machine
agent first checks to see if one is there, and bails if it is, that should be
reasonable protection.

Also, I think it would be reasonable to have the newly added machine to state to
be removed if the script fails is fine.  Given that the machine needs to be in
state already for the machine agent to start properly.

https://codereview.appspot.com/12831043/diff/8002/provider/local/environ.go
File provider/local/environ.go (right):

https://codereview.appspot.com/12831043/diff/8002/provider/local/environ.go#n...
provider/local/environ.go:356: if err := machineAgent.Remove(true); err != nil {
Ick, I really hate boolean arguments to functions.  It tells me that there
really should be two better named functions.  What does "true" mean here?

https://codereview.appspot.com/12831043/diff/8002/upstart/upstart.go
File upstart/upstart.go (right):

https://codereview.appspot.com/12831043/diff/8002/upstart/upstart.go#newcode81
upstart/upstart.go:81: func (s *Service) Remove(stop bool) error {
I'd prefer two methods:

Remove() // which implies stop too

and

RemoveOnly()

[axw1, 2013-08-23 08:31:54]

Thanks for the reviews.

I've addressed everything (I think?) apart from fixing the tools selection. I'll
work on that now.

https://codereview.appspot.com/12831043/diff/8002/cmd/jujud/machine_test.go
File cmd/jujud/machine_test.go (right):

https://codereview.appspot.com/12831043/diff/8002/cmd/jujud/machine_test.go#n...
cmd/jujud/machine_test.go:68: nonce := state.BootstrapNonce
On 2013/08/23 02:58:42, thumper wrote:
> This will have to kinda change a bit now with the new
> agent changes landed in trunk.

Done.

https://codereview.appspot.com/12831043/diff/8002/environs/manual/agent.go
File environs/manual/agent.go (right):

https://codereview.appspot.com/12831043/diff/8002/environs/manual/agent.go#ne...
environs/manual/agent.go:24: var defaultLogDir = "/var/log/juju"
On 2013/08/23 02:58:42, thumper wrote:
> Please add a comment along the lines of:
> 
> // TODO(awx): 2013-08-23 bug 1215252
> // Gather all these constants in a project wide package

I would, but these two lines are disappearing with the move to use cloudinit.

https://codereview.appspot.com/12831043/diff/8002/environs/manual/agent.go#ne...
environs/manual/agent.go:79: var agentConf = agent.Conf{
On 2013/08/23 02:58:42, thumper wrote:
> You don't need APIPort nor StatePort here.
> 
> This is more obvious now with agent.NewAgentConfig

ACK - this is all going away anyway, using cloudinit.

https://codereview.appspot.com/12831043/diff/8002/environs/manual/agent.go#ne...
environs/manual/agent.go:138: %s`
On 2013/08/23 02:58:42, thumper wrote:
> On 2013/08/22 16:29:47, fwereade wrote:
> > FWIW I don't think it's critical that we consolidate it in this CL, I just
> don't
> > want us to forget about it.
> 
> In which case, file a bug and leave a referenced TODO comment.

Done, filed 1215777 and added a TODO in the format you suggested in your email
to juju-dev.

https://codereview.appspot.com/12831043/diff/8002/environs/manual/detection.go
File environs/manual/detection.go (right):

https://codereview.appspot.com/12831043/diff/8002/environs/manual/detection.g...
environs/manual/detection.go:55: // logical cores due to hyperthreading.
On 2013/08/23 02:58:42, thumper wrote:
> Wondering why we don't go for something simple like:
> 
>  cat /proc/cpuinfo | grep "processor\s\+:" | wc -l
> 
> ?

Well when I see "cpu-cores", I'm thinking physical cores, not logical cores. So
I think that would be misleading. If there's a precedent (or if we want to set
one), I'm open to changing this.

https://codereview.appspot.com/12831043/diff/8002/environs/manual/provisioner.go
File environs/manual/provisioner.go (right):

https://codereview.appspot.com/12831043/diff/8002/environs/manual/provisioner...
environs/manual/provisioner.go:92: //           know not to touch it.
On 2013/08/23 02:58:42, thumper wrote:
> We looked and decided it isn't needed right now.

I will just add a comment in the code here explaining that.

https://codereview.appspot.com/12831043/diff/8002/environs/manual/provisioner...
File environs/manual/provisioner_test.go (right):

https://codereview.appspot.com/12831043/diff/8002/environs/manual/provisioner...
environs/manual/provisioner_test.go:84: // Why would you permit an instance ID
to be reused?
On 2013/08/23 02:58:42, thumper wrote:
> I think if part of the bash script that gets executed to install the machine
> agent first checks to see if one is there, and bails if it is, that should be
> reasonable protection.
> 
> Also, I think it would be reasonable to have the newly added machine to state
to
> be removed if the script fails is fine.  Given that the machine needs to be in
> state already for the machine agent to start properly.

I was doing that, but I thought it was a bit crappy that the machine ID got
incremented even in the case of immediate failure. I've added another step that
checks for existing upstart scripts now, before hardware/series detection.

https://codereview.appspot.com/12831043/diff/8002/provider/local/environ.go
File provider/local/environ.go (right):

https://codereview.appspot.com/12831043/diff/8002/provider/local/environ.go#n...
provider/local/environ.go:356: if err := machineAgent.Remove(true); err != nil {
On 2013/08/23 02:58:42, thumper wrote:
> Ick, I really hate boolean arguments to functions.  It tells me that there
> really should be two better named functions.  What does "true" mean here?

Fair enough. I've gone with Remove and StopAndRemove in upstart.Service.

https://codereview.appspot.com/12831043/diff/8002/upstart/upstart.go
File upstart/upstart.go (right):

https://codereview.appspot.com/12831043/diff/8002/upstart/upstart.go#newcode81
upstart/upstart.go:81: func (s *Service) Remove(stop bool) error {
On 2013/08/23 02:58:42, thumper wrote:
> I'd prefer two methods:
> 
> Remove() // which implies stop too
> 
> and
> 
> RemoveOnly()

Done, though I've gone with Remove/StopAndRemove as discussed on IRC.

[axw, 2013-08-23 08:35:00]

Please take a look.

[axw, 2013-08-23 09:43:30]

Please take a look.

[fwereade, 2013-08-27 09:58:50]

LGTM with the following addressed. Thank you very much for all the work.

https://codereview.appspot.com/12831043/diff/29001/cmd/juju/addmachine.go
File cmd/juju/addmachine.go (right):

https://codereview.appspot.com/12831043/diff/29001/cmd/juju/addmachine.go#new...
cmd/juju/addmachine.go:96: Env:         conn.Environ,
Never use conn.Environ if you can possibly help it. It's basically never up to
date.

You could narrow the interface by just getting EnvironConfig out of the State
you pass in when you need it (do you actually need an environ? would a config be
sufficient? you can construct an environ if you need one, ofc)

https://codereview.appspot.com/12831043/diff/29001/environs/manual/agent.go
File environs/manual/agent.go (right):

https://codereview.appspot.com/12831043/diff/29001/environs/manual/agent.go#n...
environs/manual/agent.go:33: cons      constraints.Value
Hmm. I see why this is here, but it'll only be meaningful if we're doing manual
bootstrapping. If you're going to include it please name it explicitly (and
don't pass in add-machine constraints).

https://codereview.appspot.com/12831043/diff/29001/environs/manual/agent.go#n...
environs/manual/agent.go:86: // exists in our cloud-init configuration.
It exists already. That's why I was asking for it.

But I understand there are open questions here, so I'm fine leaving it off for a
bit -- after all, the user has ssh access by definition.

https://codereview.appspot.com/12831043/diff/29001/environs/manual/detection_...
File environs/manual/detection_test.go (right):

https://codereview.appspot.com/12831043/diff/29001/environs/manual/detection_...
environs/manual/detection_test.go:87: {
as usual, I find these much easier to scan when they're compressed a bit

https://codereview.appspot.com/12831043/diff/29001/environs/manual/provisione...
File environs/manual/provisioner.go (right):

https://codereview.appspot.com/12831043/diff/29001/environs/manual/provisione...
environs/manual/provisioner.go:38: Env environs.Environ
Oh, dammit, I see. OK, just keep the Environ, but please make sure it's not one
that came from a juju.Conn.

https://codereview.appspot.com/12831043/diff/29001/environs/manual/provisione...
environs/manual/provisioner.go:92: }
Constraints aren't checked. If we're going to pass them, we should definitely
check them, and take environment constraints into account.

Honestly, I'd just drop them from the args and disallow them in the command.

https://codereview.appspot.com/12831043/diff/29001/environs/manual/provisione...
environs/manual/provisioner.go:116: cons:       args.Constraints,
machine constraints are only used for provisioning, so these are actually
useless.

https://codereview.appspot.com/12831043/diff/29001/environs/manual/provisione...
environs/manual/provisioner.go:183: if args.tools == nil {
This whole block is redundant. It's the machine agent's responsibility to set
the tools it's using; it's *definitely* not a good idea for us to independently
calculate the tools it would use here; if we were to do this, we should set the
tools we actually picked for cloudinit.

https://codereview.appspot.com/12831043/diff/29001/environs/manual/provisione...
environs/manual/provisioner.go:201: arches := possibleTools.Arches()
can this handle a nil receiver? that was clever of me ;p

https://codereview.appspot.com/12831043/diff/29001/upstart/upstart.go
File upstart/upstart.go (right):

https://codereview.appspot.com/12831043/diff/29001/upstart/upstart.go#newcode98
upstart/upstart.go:98: }
I think this would be simpler if you dropped the parameter and did this bit
explicitly in StopAndRemove

https://codereview.appspot.com/12831043/diff/29001/upstart/upstart_test.go
File upstart/upstart_test.go (left):

https://codereview.appspot.com/12831043/diff/29001/upstart/upstart_test.go#ol...
upstart/upstart_test.go:130: }
Remove is no longer tested directly.

[axw1, 2013-08-29 04:07:36]

https://codereview.appspot.com/12831043/diff/29001/cmd/juju/addmachine.go
File cmd/juju/addmachine.go (right):

https://codereview.appspot.com/12831043/diff/29001/cmd/juju/addmachine.go#new...
cmd/juju/addmachine.go:96: Env:         conn.Environ,
On 2013/08/27 09:58:50, fwereade wrote:
> Never use conn.Environ if you can possibly help it. It's basically never up to
> date.
> 
> You could narrow the interface by just getting EnvironConfig out of the State
> you pass in when you need it (do you actually need an environ? would a config
be
> sufficient? you can construct an environ if you need one, ofc)

I've taken the Env out of the args. The manual provisioning code now uses
juju.NewConnFromState, and extracts the Environ field.

https://codereview.appspot.com/12831043/diff/29001/environs/manual/agent.go
File environs/manual/agent.go (right):

https://codereview.appspot.com/12831043/diff/29001/environs/manual/agent.go#n...
environs/manual/agent.go:33: cons      constraints.Value
On 2013/08/27 09:58:50, fwereade wrote:
> Hmm. I see why this is here, but it'll only be meaningful if we're doing
manual
> bootstrapping. If you're going to include it please name it explicitly (and
> don't pass in add-machine constraints).

Removed.

https://codereview.appspot.com/12831043/diff/29001/environs/manual/agent.go#n...
environs/manual/agent.go:86: // exists in our cloud-init configuration.
On 2013/08/27 09:58:50, fwereade wrote:
> It exists already. That's why I was asking for it.

Sorry, I misunderstood your earlier comment. Updated code comment to reduce
confusion.

https://codereview.appspot.com/12831043/diff/29001/environs/manual/detection_...
File environs/manual/detection_test.go (right):

https://codereview.appspot.com/12831043/diff/29001/environs/manual/detection_...
environs/manual/detection_test.go:87: {
On 2013/08/27 09:58:50, fwereade wrote:
> as usual, I find these much easier to scan when they're compressed a bit

Done, sorry.

https://codereview.appspot.com/12831043/diff/29001/environs/manual/provisione...
File environs/manual/provisioner.go (right):

https://codereview.appspot.com/12831043/diff/29001/environs/manual/provisione...
environs/manual/provisioner.go:38: Env environs.Environ
On 2013/08/27 09:58:50, fwereade wrote:
> Oh, dammit, I see. OK, just keep the Environ, but please make sure it's not
one
> that came from a juju.Conn.

I've removed the Env parameter field, since it can be derived from State.

https://codereview.appspot.com/12831043/diff/29001/environs/manual/provisione...
environs/manual/provisioner.go:92: }
On 2013/08/27 09:58:50, fwereade wrote:
> Constraints aren't checked. If we're going to pass them, we should definitely
> check them, and take environment constraints into account.
> 
> Honestly, I'd just drop them from the args and disallow them in the command.

Okay, I'll take them out. I had them in, originally thinking that InjectMachine
would (and then thinking it might later) check the constraints against hardware
characteristics. It's easy enough to add back later, if we do that.

https://codereview.appspot.com/12831043/diff/29001/environs/manual/provisione...
environs/manual/provisioner.go:116: cons:       args.Constraints,
On 2013/08/27 09:58:50, fwereade wrote:
> machine constraints are only used for provisioning, so these are actually
> useless.

Removed.

https://codereview.appspot.com/12831043/diff/29001/environs/manual/provisione...
environs/manual/provisioner.go:183: if args.tools == nil {
On 2013/08/27 09:58:50, fwereade wrote:
> This whole block is redundant. It's the machine agent's responsibility to set
> the tools it's using; it's *definitely* not a good idea for us to
independently
> calculate the tools it would use here; if we were to do this, we should set
the
> tools we actually picked for cloudinit.

That's exactly what's happening. At the top of provisionMachineAgentSCript, I
get the machine.AgentTools() back out, and set on the environs.MachineConfig,
which is used to generate the cloudinit config.

Anyway, it looks like setting the AgentTools is only used for upgrading agents'
tools? I'll move the tool selection out of here, and closer to
provisionMachineAgentScript.

https://codereview.appspot.com/12831043/diff/29001/environs/manual/provisione...
environs/manual/provisioner.go:201: arches := possibleTools.Arches()
On 2013/08/27 09:58:50, fwereade wrote:
> can this handle a nil receiver? that was clever of me ;p

Oops! Fixed.

https://codereview.appspot.com/12831043/diff/29001/upstart/upstart.go
File upstart/upstart.go (right):

https://codereview.appspot.com/12831043/diff/29001/upstart/upstart.go#newcode98
upstart/upstart.go:98: }
On 2013/08/27 09:58:50, fwereade wrote:
> I think this would be simpler if you dropped the parameter and did this bit
> explicitly in StopAndRemove

Done.

https://codereview.appspot.com/12831043/diff/29001/upstart/upstart_test.go
File upstart/upstart_test.go (left):

https://codereview.appspot.com/12831043/diff/29001/upstart/upstart_test.go#ol...
upstart/upstart_test.go:130: }
On 2013/08/27 09:58:50, fwereade wrote:
> Remove is no longer tested directly.

Added a test.

[axw, 2013-08-29 05:10:02]

Please take a look.

[thumper, 2013-08-29 22:03:55]

On 2013/08/29 05:10:02, axw wrote:
> Please take a look.

LGTM

[fwereade, 2013-08-30 06:15:00]

LGTM, just trivials.

https://codereview.appspot.com/12831043/diff/29001/environs/manual/provisione...
File environs/manual/provisioner.go (right):

https://codereview.appspot.com/12831043/diff/29001/environs/manual/provisione...
environs/manual/provisioner.go:183: if args.tools == nil {
On 2013/08/29 04:07:37, axw1 wrote:
> On 2013/08/27 09:58:50, fwereade wrote:
> > This whole block is redundant. It's the machine agent's responsibility to
set
> > the tools it's using; it's *definitely* not a good idea for us to
> independently
> > calculate the tools it would use here; if we were to do this, we should set
> the
> > tools we actually picked for cloudinit.
> 
> That's exactly what's happening. At the top of provisionMachineAgentSCript, I
> get the machine.AgentTools() back out, and set on the environs.MachineConfig,
> which is used to generate the cloudinit config.

Ah, I hadn't noticed that. Sorry.

> Anyway, it looks like setting the AgentTools is only used for upgrading
agents'
> tools? I'll move the tool selection out of here, and closer to
> provisionMachineAgentScript.

Yeah, it's mainly just that AgentTools have hitherto been set exclusively by the
agent, and the setting's existence thus implies that the agent has actually run,
and I'd rather not alter this behaviour without clear need.

https://codereview.appspot.com/12831043/diff/40001/cmd/juju/addmachine.go
File cmd/juju/addmachine.go (right):

https://codereview.appspot.com/12831043/diff/40001/cmd/juju/addmachine.go#new...
cmd/juju/addmachine.go:47: Args:    "[<container>:machine | <container> |
ssh:[user@]host]",
Huh, these aren't quite right, are they? I think it's more like:

[<(pseudo-)cloud>:<location>]

...where ssh and lxc are the available pseudo-clouds today, and "aws",
"hpcloud", etc, become viable at some point in the future.

https://codereview.appspot.com/12831043/diff/40001/cmd/juju/destroyenvironmen...
File cmd/juju/destroyenvironment.go (right):

https://codereview.appspot.com/12831043/diff/40001/cmd/juju/destroyenvironmen...
cmd/juju/destroyenvironment.go:52: // TODO(axw) destroy manually provisioned
machines, or otherwise
Would you write a bug for this please?

https://codereview.appspot.com/12831043/diff/40001/environs/manual/provisione...
File environs/manual/provisioner.go (right):

https://codereview.appspot.com/12831043/diff/40001/environs/manual/provisione...
environs/manual/provisioner.go:159: m.Remove()
Would be good to log these errors if they happen.

https://codereview.appspot.com/12831043/diff/40001/provider/provider.go
File provider/provider.go (right):

https://codereview.appspot.com/12831043/diff/40001/provider/provider.go#newco...
provider/provider.go:13: Manual    = "manual"
I'm not sure this is a thing yet. Where's it referenced? If not, the mooted
terminology is for a "null" provider... but that shouldn't be defined until we
have one, either.

[axw1, 2013-08-30 06:53:01]

On 2013/08/30 06:15:00, fwereade wrote:
> LGTM, just trivials.

I've already merged based on the last two LGTMs, so I will address them in a new
MP.

> https://codereview.appspot.com/12831043/diff/40001/cmd/juju/addmachine.go
> File cmd/juju/addmachine.go (right):
> 
>
https://codereview.appspot.com/12831043/diff/40001/cmd/juju/addmachine.go#new...
> cmd/juju/addmachine.go:47: Args:    "[<container>:machine | <container> |
> ssh:[user@]host]",
> Huh, these aren't quite right, are they? I think it's more like:
> 
> [<(pseudo-)cloud>:<location>]
> 
> ...where ssh and lxc are the available pseudo-clouds today, and "aws",
> "hpcloud", etc, become viable at some point in the future.

lxc I can buy, but calling ssh a pseudo-cloud sounds odd to me.

>
https://codereview.appspot.com/12831043/diff/40001/cmd/juju/destroyenvironmen...
> File cmd/juju/destroyenvironment.go (right):
> 
>
https://codereview.appspot.com/12831043/diff/40001/cmd/juju/destroyenvironmen...
> cmd/juju/destroyenvironment.go:52: // TODO(axw) destroy manually provisioned
> machines, or otherwise
> Would you write a bug for this please?

Done (I did it before, I just hadn't reproposed).
https://bugs.launchpad.net/juju-core/+bug/1218688

>
https://codereview.appspot.com/12831043/diff/40001/environs/manual/provisione...
> File environs/manual/provisioner.go (right):
> 
>
https://codereview.appspot.com/12831043/diff/40001/environs/manual/provisione...
> environs/manual/provisioner.go:159: m.Remove()
> Would be good to log these errors if they happen.

Done (in a new MP).

> https://codereview.appspot.com/12831043/diff/40001/provider/provider.go
> File provider/provider.go (right):
> 
>
https://codereview.appspot.com/12831043/diff/40001/provider/provider.go#newco...
> provider/provider.go:13: Manual    = "manual"
> I'm not sure this is a thing yet. Where's it referenced? If not, the mooted
> terminology is for a "null" provider... but that shouldn't be defined until we
> have one, either.

It's used as the specified provider type, as it gets written to the upstart conf
file (later to go into agent.conf?); see environs/manual/agent.conf.

I can take it out if you like, but it's just as wrong to tell the agent that its
provider is whatever the current environment's provider is.