Merge lp:~axwalk/juju-core/juju-add-machine into lp:~go-bot/juju-core/trunk
- juju-add-machine
- Merge into trunk
| Status: | Merged |
|---|---|
| Approved by: | Andrew Wilkins |
| Approved revision: | no longer in the source branch. |
| Merged at revision: | 1732 |
| Proposed branch: | lp:~axwalk/juju-core/juju-add-machine |
| Merge into: | lp:~go-bot/juju-core/trunk |
| Diff against target: |
1145 lines (+841/-28) 19 files modified
cloudinit/cloudinit_test.go (+19/-0) cloudinit/options.go (+31/-2) cmd/juju/addmachine.go (+34/-9) cmd/juju/destroyenvironment.go (+4/-0) cmd/jujud/machine.go (+15/-1) environs/manual/agent.go (+133/-0) environs/manual/detection.go (+118/-0) environs/manual/detection_test.go (+141/-0) environs/manual/provisioner.go (+184/-0) environs/manual/provisioner_test.go (+85/-0) environs/manual/suite_test.go (+14/-0) environs/tools/tools.go (+2/-1) instance/address.go (+21/-0) provider/local/environ.go (+2/-2) provider/provider.go (+1/-0) state/state_test.go (+1/-1) upstart/upstart.go (+13/-4) upstart/upstart_test.go (+22/-7) worker/deployer/simple.go (+1/-1) |
| To merge this branch: | bzr merge lp:~axwalk/juju-core/juju-add-machine |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Juju Engineering | Pending | ||
|
Review via email:
|
|||
Commit message
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.
Description of the change
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.
| Tim Penhey (thumper) wrote : | # |
| Andrew Wilkins (axwalk) wrote : | # |
On 2013/08/15 02:50:47, thumper wrote:
> https:/
> File cmd/juju/
https:/
> cmd/juju/
{
> 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:/
> cmd/juju/
> ([]instance.
> 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:/
> cmd/juju/
> What if the host is an ip address?
LookupIP don't care.
https:/
> cmd/juju/
> Oh how I wish cloud vendors would have good IPv6 support.
https:/
> cmd/juju/
> manuallyProvisi
> 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:/
> cmd/juju/
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:/
> cmd/juju/
environs.
> Why create a new environment when the conn parameter should have one
already?
https:/
> cmd/juju/
> constraints.
> 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 ...
| Andrew Wilkins (axwalk) wrote : | # |
Reviewers: mp+179840_
Message:
Please take a look.
Description:
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.
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.
TODO: injected machines do not currently set a
provider type, so providers have no way of knowing
that a manually provisioned machine is not theirs
to mess with. This will be handled in a follow-up
that introduces provider-type into the machine state
schema.
https:/
(do not edit description out of merge proposal)
Please review this at https:/
Affected files:
A [revision details]
M cmd/juju/
M cmd/juju/
M cmd/jujud/
M cmd/jujud/
M environs/
M environs/
A environs/
A environs/
A environs/
A environs/
A environs/
A environs/
M environs/
M instance/address.go
M state/state.go
M state/state_test.go
M upstart/upstart.go
M upstart/
M worker/
| Andrew Wilkins (axwalk) wrote : | # |
Please take a look.
| William Reade (fwereade) wrote : | # |
Lots and lots of comments, not all actionable; this is going in an
excellent direction. I'd appreciate your thoughts.
https:/
File cmd/juju/
https:/
cmd/juju/
s/tools storage/environment storage/
https:/
cmd/juju/
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:
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:/
File cmd/juju/
https:/
cmd/juju/
manually "destroyed".
FWIW, I think it's becoming clearer and clearer that destroy-environment
really ought to be handled server-side.
https:/
File cmd/jujud/
https:/
cmd/jujud/
os.Getenv(
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:/
cmd/jujud/
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:/
cmd/jujud/
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:/
cmd/jujud/
I think we also need to remove the machine in state.
https:/
File environs/
https:/
environs/
st.InjectMachin
state.Bootstrap
This is starting to look like it would ap...
| Andrew Wilkins (axwalk) wrote : | # |
Thanks for the review. I've addressed what I can today, and will
continue working on addressing the remaining comments.
https:/
File cmd/juju/
https:/
cmd/juju/
On 2013/08/21 11:21:51, fwereade wrote:
> s/tools storage/environment storage/
Done.
https:/
cmd/juju/
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:
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:/
File cmd/jujud/
https:/
cmd/jujud/
os.Getenv(
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:/
cmd/jujud/
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.JujuProvi
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:/
cmd/jujud/
On 2013/08/21 11:21:51, fwereade wrote:
> I'm not too sure about this either, ...
| William Reade (fwereade) wrote : | # |
A few more comments; if you've got a prereq on its way, I'm WIPping this
until it's a candidate again.
https:/
File cmd/jujud/
https:/
cmd/jujud/
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:/
File environs/
https:/
environs/
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-
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:/
File environs/
| Tim Penhey (thumper) wrote : | # |
More things to consider :-)
https:/
File cmd/juju/
https:/
cmd/juju/
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-
https:/
File cmd/jujud/
https:/
cmd/jujud/
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:/
File cmd/jujud/
https:/
cmd/jujud/
This will have to kinda change a bit now with the new
agent changes landed in trunk.
https:/
File environs/
https:/
environs/
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:/
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:/
environs/
"jujud-
On 2013/08/21 11:21:51, fwereade wrote:
> Please use the standard upstart job naming, which includes the agent
id. (or
> make everyth...
| Andrew Wilkins (axwalk) wrote : | # |
Thanks for the reviews.
I've addressed everything (I think?) apart from fixing the tools
selection. I'll work on that now.
https:/
File cmd/jujud/
https:/
cmd/jujud/
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:/
File environs/
https:/
environs/
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:/
environs/
On 2013/08/23 02:58:42, thumper wrote:
> You don't need APIPort nor StatePort here.
> This is more obvious now with agent.NewAgentC
ACK - this is all going away anyway, using cloudinit.
https:/
environs/
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:/
File environs/
https:/
environs/
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:/
File environs/
https:/
environs/
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:/
File environs/
https:/
environs/
| Andrew Wilkins (axwalk) wrote : | # |
Please take a look.
| Andrew Wilkins (axwalk) wrote : | # |
Please take a look.
| William Reade (fwereade) wrote : | # |
LGTM with the following addressed. Thank you very much for all the work.
https:/
File cmd/juju/
https:/
cmd/juju/
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:/
File environs/
https:/
environs/
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:/
environs/
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:/
File environs/
https:/
environs/
as usual, I find these much easier to scan when they're compressed a bit
https:/
File environs/
https:/
environs/
Oh, dammit, I see. OK, just keep the Environ, but please make sure it's
not one that came from a juju.Conn.
https:/
environs/
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:/
environs/
machine constraints are only used for provisioning, so these are
actually useless.
https:/
environs/
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.
| Andrew Wilkins (axwalk) wrote : | # |
https:/
File cmd/juju/
https:/
cmd/juju/
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.NewConnFro
https:/
File environs/
https:/
environs/
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:/
environs/
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:/
File environs/
https:/
environs/
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:/
File environs/
https:/
environs/
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:/
environs/
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:/
| Andrew Wilkins (axwalk) wrote : | # |
Please take a look.
| Tim Penhey (thumper) wrote : | # |
On 2013/08/29 05:10:02, axw wrote:
> Please take a look.
LGTM
| William Reade (fwereade) wrote : | # |
LGTM, just trivials.
https:/
File environs/
https:/
environs/
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
provisionMachin
> get the machine.
environs.
> 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
> provisionMachin
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:/
File cmd/juju/
https:/
cmd/juju/
| ssh:[user@]host]",
Huh, these aren't quite right, are they? I think it's more like:
[<(pseudo-
...where ssh and lxc are the available pseudo-clouds today, and "aws",
"hpcloud", etc, become viable at some point in the future.
https:/
File cmd/juju/
https:/
cmd/juju/
provisioned machines, or otherwise
Would you write a bug for this please?
https:/
File environs/
https:/
environs/
Would be good to log these errors if they happen.
https:/
File provider/
https:/
provider/
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.
| Andrew Wilkins (axwalk) wrote : | # |
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:/
> File cmd/juju/
https:/
> cmd/juju/
<container> |
> ssh:[user@]host]",
> Huh, these aren't quite right, are they? I think it's more like:
> [<(pseudo-
> ...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:/
> File cmd/juju/
https:/
> cmd/juju/
provisioned
> machines, or otherwise
> Would you write a bug for this please?
Done (I did it before, I just hadn't reproposed).
https:/
https:/
> File environs/
https:/
> environs/
> Would be good to log these errors if they happen.
Done (in a new MP).
https:/
> File provider/
https:/
> provider/
> 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/
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.
Preview Diff
| 1 | === modified file 'cloudinit/cloudinit_test.go' |
| 2 | --- cloudinit/cloudinit_test.go 2013-08-20 14:02:31 +0000 |
| 3 | +++ cloudinit/cloudinit_test.go 2013-08-30 01:29:18 +0000 |
| 4 | @@ -257,6 +257,25 @@ |
| 5 | } |
| 6 | } |
| 7 | |
| 8 | +func (S) TestRunCmds(c *gc.C) { |
| 9 | + cfg := cloudinit.New() |
| 10 | + c.Assert(cfg.RunCmds(), gc.HasLen, 0) |
| 11 | + cfg.AddScripts("a", "b") |
| 12 | + cfg.AddRunCmdArgs("c", "d") |
| 13 | + cfg.AddRunCmd("e") |
| 14 | + c.Assert(cfg.RunCmds(), gc.DeepEquals, []interface{}{ |
| 15 | + "a", "b", []string{"c", "d"}, "e", |
| 16 | + }) |
| 17 | +} |
| 18 | + |
| 19 | +func (S) TestPackages(c *gc.C) { |
| 20 | + cfg := cloudinit.New() |
| 21 | + c.Assert(cfg.Packages(), gc.HasLen, 0) |
| 22 | + cfg.AddPackage("a b c") |
| 23 | + cfg.AddPackage("d!") |
| 24 | + c.Assert(cfg.Packages(), gc.DeepEquals, []string{"a b c", "d!"}) |
| 25 | +} |
| 26 | + |
| 27 | //#cloud-config |
| 28 | //packages: |
| 29 | //- juju |
| 30 | |
| 31 | === modified file 'cloudinit/options.go' |
| 32 | --- cloudinit/options.go 2013-08-20 14:02:31 +0000 |
| 33 | +++ cloudinit/options.go 2013-08-30 01:29:18 +0000 |
| 34 | @@ -92,13 +92,42 @@ |
| 35 | // If any packages are specified, "apt-get update" |
| 36 | // will be called. |
| 37 | func (cfg *Config) AddPackage(name string) { |
| 38 | + cfg.attrs["packages"] = append(cfg.Packages(), name) |
| 39 | +} |
| 40 | + |
| 41 | +// Packages returns a list of packages that will be |
| 42 | +// installed on first boot. |
| 43 | +func (cfg *Config) Packages() []string { |
| 44 | pkgs, _ := cfg.attrs["packages"].([]string) |
| 45 | - cfg.attrs["packages"] = append(pkgs, name) |
| 46 | + return pkgs |
| 47 | } |
| 48 | |
| 49 | func (cfg *Config) addCmd(kind string, c *command) { |
| 50 | + cfg.attrs[kind] = append(cfg.getCmds(kind), c) |
| 51 | +} |
| 52 | + |
| 53 | +func (cfg *Config) getCmds(kind string) []*command { |
| 54 | cmds, _ := cfg.attrs[kind].([]*command) |
| 55 | - cfg.attrs[kind] = append(cmds, c) |
| 56 | + return cmds |
| 57 | +} |
| 58 | + |
| 59 | +// RunCmds returns a list of commands that will be |
| 60 | +// run at first boot. |
| 61 | +// |
| 62 | +// Each element in the resultant slice is either a |
| 63 | +// string or []string, corresponding to how the command |
| 64 | +// was added. |
| 65 | +func (cfg *Config) RunCmds() []interface{} { |
| 66 | + cmds := cfg.getCmds("runcmd") |
| 67 | + result := make([]interface{}, len(cmds)) |
| 68 | + for i, cmd := range cmds { |
| 69 | + if cmd.args != nil { |
| 70 | + result[i] = append([]string{}, cmd.args...) |
| 71 | + } else { |
| 72 | + result[i] = cmd.literal |
| 73 | + } |
| 74 | + } |
| 75 | + return result |
| 76 | } |
| 77 | |
| 78 | // AddRunCmd adds a command to be executed |
| 79 | |
| 80 | === modified file 'cmd/juju/addmachine.go' |
| 81 | --- cmd/juju/addmachine.go 2013-08-13 19:07:35 +0000 |
| 82 | +++ cmd/juju/addmachine.go 2013-08-30 01:29:18 +0000 |
| 83 | @@ -11,6 +11,7 @@ |
| 84 | |
| 85 | "launchpad.net/juju-core/cmd" |
| 86 | "launchpad.net/juju-core/constraints" |
| 87 | + "launchpad.net/juju-core/environs/manual" |
| 88 | "launchpad.net/juju-core/instance" |
| 89 | "launchpad.net/juju-core/juju" |
| 90 | "launchpad.net/juju-core/log" |
| 91 | @@ -18,6 +19,16 @@ |
| 92 | "launchpad.net/juju-core/state" |
| 93 | ) |
| 94 | |
| 95 | +// sshHostPrefix is the prefix for a machine to be "manually provisioned". |
| 96 | +const sshHostPrefix = "ssh:" |
| 97 | + |
| 98 | +var addMachineDoc = ` |
| 99 | +Machines are created in a clean state and ready to have units deployed. |
| 100 | + |
| 101 | +This command also supports configuring existing machines via SSH. The |
| 102 | +target machine must be able to communicate with the API servers, and |
| 103 | +be able to access the environment storage.`[1:] |
| 104 | + |
| 105 | // AddMachineCommand starts a new machine and registers it in the environment. |
| 106 | type AddMachineCommand struct { |
| 107 | cmd.EnvCommandBase |
| 108 | @@ -27,14 +38,15 @@ |
| 109 | Constraints constraints.Value |
| 110 | MachineId string |
| 111 | ContainerType instance.ContainerType |
| 112 | + SSHHost string |
| 113 | } |
| 114 | |
| 115 | func (c *AddMachineCommand) Info() *cmd.Info { |
| 116 | return &cmd.Info{ |
| 117 | Name: "add-machine", |
| 118 | - Args: "[<container>:machine | <container>]", |
| 119 | + Args: "[<container>:machine | <container> | ssh:[user@]host]", |
| 120 | Purpose: "start a new, empty machine and optionally a container, or add a container to a machine", |
| 121 | - Doc: "Machines are created in a clean state and ready to have units deployed.", |
| 122 | + Doc: addMachineDoc, |
| 123 | } |
| 124 | } |
| 125 | |
| 126 | @@ -55,14 +67,18 @@ |
| 127 | if containerSpec == "" { |
| 128 | return nil |
| 129 | } |
| 130 | - // container arg can either be 'type:machine' or 'type' |
| 131 | - if c.ContainerType, err = instance.ParseSupportedContainerType(containerSpec); err != nil { |
| 132 | - if names.IsMachine(containerSpec) || !cmd.IsMachineOrNewContainer(containerSpec) { |
| 133 | - return fmt.Errorf("malformed container argument %q", containerSpec) |
| 134 | + if strings.HasPrefix(containerSpec, sshHostPrefix) { |
| 135 | + c.SSHHost = containerSpec[len(sshHostPrefix):] |
| 136 | + } else { |
| 137 | + // container arg can either be 'type:machine' or 'type' |
| 138 | + if c.ContainerType, err = instance.ParseSupportedContainerType(containerSpec); err != nil { |
| 139 | + if names.IsMachine(containerSpec) || !cmd.IsMachineOrNewContainer(containerSpec) { |
| 140 | + return fmt.Errorf("malformed container argument %q", containerSpec) |
| 141 | + } |
| 142 | + sep := strings.Index(containerSpec, ":") |
| 143 | + c.MachineId = containerSpec[sep+1:] |
| 144 | + c.ContainerType, err = instance.ParseSupportedContainerType(containerSpec[:sep]) |
| 145 | } |
| 146 | - sep := strings.Index(containerSpec, ":") |
| 147 | - c.MachineId = containerSpec[sep+1:] |
| 148 | - c.ContainerType, err = instance.ParseSupportedContainerType(containerSpec[:sep]) |
| 149 | } |
| 150 | return err |
| 151 | } |
| 152 | @@ -74,6 +90,15 @@ |
| 153 | } |
| 154 | defer conn.Close() |
| 155 | |
| 156 | + if c.SSHHost != "" { |
| 157 | + args := manual.ProvisionMachineArgs{ |
| 158 | + Host: c.SSHHost, |
| 159 | + State: conn.State, |
| 160 | + } |
| 161 | + _, err = manual.ProvisionMachine(args) |
| 162 | + return err |
| 163 | + } |
| 164 | + |
| 165 | series := c.Series |
| 166 | if series == "" { |
| 167 | conf, err := conn.State.EnvironConfig() |
| 168 | |
| 169 | === modified file 'cmd/juju/destroyenvironment.go' |
| 170 | --- cmd/juju/destroyenvironment.go 2013-08-13 19:07:35 +0000 |
| 171 | +++ cmd/juju/destroyenvironment.go 2013-08-30 01:29:18 +0000 |
| 172 | @@ -49,6 +49,10 @@ |
| 173 | } |
| 174 | } |
| 175 | |
| 176 | + // TODO(axw) 2013-08-30 bug 1218688 |
| 177 | + // destroy manually provisioned machines, or otherwise |
| 178 | + // block destroy-environment until all manually provisioned |
| 179 | + // machines have been manually "destroyed". |
| 180 | return environ.Destroy(nil) |
| 181 | } |
| 182 | |
| 183 | |
| 184 | === modified file 'cmd/jujud/machine.go' |
| 185 | --- cmd/jujud/machine.go 2013-08-27 09:51:34 +0000 |
| 186 | +++ cmd/jujud/machine.go 2013-08-30 01:29:18 +0000 |
| 187 | @@ -23,6 +23,7 @@ |
| 188 | "launchpad.net/juju-core/state" |
| 189 | "launchpad.net/juju-core/state/api/params" |
| 190 | "launchpad.net/juju-core/state/apiserver" |
| 191 | + "launchpad.net/juju-core/upstart" |
| 192 | "launchpad.net/juju-core/worker" |
| 193 | "launchpad.net/juju-core/worker/cleaner" |
| 194 | "launchpad.net/juju-core/worker/firewaller" |
| 195 | @@ -117,7 +118,11 @@ |
| 196 | a.runner.StartWorker("api", func() (worker.Worker, error) { |
| 197 | return a.APIWorker(ensureStateWorker) |
| 198 | }) |
| 199 | - err := agentDone(a.runner.Wait()) |
| 200 | + err := a.runner.Wait() |
| 201 | + if err == worker.ErrTerminateAgent { |
| 202 | + err = a.uninstallAgent() |
| 203 | + } |
| 204 | + err = agentDone(err) |
| 205 | a.tomb.Kill(err) |
| 206 | return err |
| 207 | } |
| 208 | @@ -289,6 +294,15 @@ |
| 209 | return names.MachineTag(a.MachineId) |
| 210 | } |
| 211 | |
| 212 | +func (m *MachineAgent) uninstallAgent() error { |
| 213 | + // TODO(axw) get this from agent config when it's available |
| 214 | + name := os.Getenv("UPSTART_JOB") |
| 215 | + if name != "" { |
| 216 | + return upstart.NewService(name).Remove() |
| 217 | + } |
| 218 | + return nil |
| 219 | +} |
| 220 | + |
| 221 | // Below pieces are used for testing,to give us access to the *State opened |
| 222 | // by the agent, and allow us to trigger syncs without waiting 5s for them |
| 223 | // to happen automatically. |
| 224 | |
| 225 | === added directory 'environs/manual' |
| 226 | === added file 'environs/manual/agent.go' |
| 227 | --- environs/manual/agent.go 1970-01-01 00:00:00 +0000 |
| 228 | +++ environs/manual/agent.go 2013-08-30 01:29:18 +0000 |
| 229 | @@ -0,0 +1,133 @@ |
| 230 | +// Copyright 2013 Canonical Ltd. |
| 231 | +// Licensed under the AGPLv3, see LICENCE file for details. |
| 232 | + |
| 233 | +package manual |
| 234 | + |
| 235 | +import ( |
| 236 | + "encoding/base64" |
| 237 | + "fmt" |
| 238 | + "os" |
| 239 | + "os/exec" |
| 240 | + "strings" |
| 241 | + |
| 242 | + corecloudinit "launchpad.net/juju-core/cloudinit" |
| 243 | + "launchpad.net/juju-core/constraints" |
| 244 | + "launchpad.net/juju-core/environs" |
| 245 | + "launchpad.net/juju-core/environs/cloudinit" |
| 246 | + envtools "launchpad.net/juju-core/environs/tools" |
| 247 | + "launchpad.net/juju-core/juju/osenv" |
| 248 | + "launchpad.net/juju-core/provider" |
| 249 | + "launchpad.net/juju-core/state" |
| 250 | + "launchpad.net/juju-core/state/api" |
| 251 | + "launchpad.net/juju-core/tools" |
| 252 | + "launchpad.net/juju-core/utils" |
| 253 | +) |
| 254 | + |
| 255 | +type provisionMachineAgentArgs struct { |
| 256 | + host string |
| 257 | + dataDir string |
| 258 | + env environs.Environ |
| 259 | + machine *state.Machine |
| 260 | + nonce string |
| 261 | + stateInfo *state.Info |
| 262 | + apiInfo *api.Info |
| 263 | + series string |
| 264 | + arch string |
| 265 | + tools *tools.Tools |
| 266 | +} |
| 267 | + |
| 268 | +// provisionMachineAgent connects to a machine over SSH, |
| 269 | +// copies across the tools, and installs a machine agent. |
| 270 | +func provisionMachineAgent(args provisionMachineAgentArgs) error { |
| 271 | + script, err := provisionMachineAgentScript(args) |
| 272 | + if err != nil { |
| 273 | + return err |
| 274 | + } |
| 275 | + scriptBase64 := base64.StdEncoding.EncodeToString([]byte(script)) |
| 276 | + script = fmt.Sprintf(`F=$(mktemp); echo %s | base64 -d > $F; . $F`, scriptBase64) |
| 277 | + sshArgs := []string{ |
| 278 | + args.host, |
| 279 | + "-t", // allocate a pseudo-tty |
| 280 | + "--", fmt.Sprintf("sudo bash -c '%s'", script), |
| 281 | + } |
| 282 | + cmd := exec.Command("ssh", sshArgs...) |
| 283 | + cmd.Stdout = os.Stdout |
| 284 | + cmd.Stderr = os.Stderr |
| 285 | + cmd.Stdin = os.Stdin |
| 286 | + return cmd.Run() |
| 287 | +} |
| 288 | + |
| 289 | +// provisionMachineAgentScript generates the script necessary |
| 290 | +// to install a machine agent on the specified host. |
| 291 | +func provisionMachineAgentScript(args provisionMachineAgentArgs) (string, error) { |
| 292 | + tools := args.tools |
| 293 | + if tools == nil { |
| 294 | + var err error |
| 295 | + tools, err = findMachineAgentTools(args.env, args.series, args.arch) |
| 296 | + if err != nil { |
| 297 | + return "", err |
| 298 | + } |
| 299 | + } |
| 300 | + |
| 301 | + // We generate a cloud-init config, which we'll then pull the runcmds |
| 302 | + // and prerequisite packages out of. Rather than generating a cloud-config, |
| 303 | + // we'll just generate a shell script. |
| 304 | + mcfg := environs.NewMachineConfig(args.machine.Id(), args.nonce, args.stateInfo, args.apiInfo) |
| 305 | + if args.dataDir != "" { |
| 306 | + mcfg.DataDir = args.dataDir |
| 307 | + } |
| 308 | + mcfg.Tools = tools |
| 309 | + err := environs.FinishMachineConfig(mcfg, args.env.Config(), constraints.Value{}) |
| 310 | + if err != nil { |
| 311 | + return "", err |
| 312 | + } |
| 313 | + mcfg.MachineEnvironment[osenv.JujuProviderType] = provider.Manual |
| 314 | + cloudcfg := corecloudinit.New() |
| 315 | + if cloudcfg, err = cloudinit.Configure(mcfg, cloudcfg); err != nil { |
| 316 | + return "", err |
| 317 | + } |
| 318 | + |
| 319 | + // TODO(axw): 2013-08-23 bug 1215777 |
| 320 | + // Carry out configuration for ssh-keys-per-user, |
| 321 | + // machine-updates-authkeys, using cloud-init config. |
| 322 | + |
| 323 | + // Convert runcmds to a series of shell commands. |
| 324 | + script := []string{"#!/bin/sh"} |
| 325 | + for _, cmd := range cloudcfg.RunCmds() { |
| 326 | + switch cmd := cmd.(type) { |
| 327 | + case []string: |
| 328 | + // Quote args, so shell meta-characters are not interpreted. |
| 329 | + for i, arg := range cmd[1:] { |
| 330 | + cmd[i] = utils.ShQuote(arg) |
| 331 | + } |
| 332 | + script = append(script, strings.Join(cmd, " ")) |
| 333 | + case string: |
| 334 | + script = append(script, cmd) |
| 335 | + default: |
| 336 | + return "", fmt.Errorf("unexpected runcmd type: %T", cmd) |
| 337 | + } |
| 338 | + } |
| 339 | + |
| 340 | + // The first command is "set -xe", which we want to leave in place. |
| 341 | + head := []string{script[0]} |
| 342 | + tail := script[1:] |
| 343 | + for _, pkg := range cloudcfg.Packages() { |
| 344 | + cmd := fmt.Sprintf("apt-get -y install %s", utils.ShQuote(pkg)) |
| 345 | + head = append(head, cmd) |
| 346 | + } |
| 347 | + script = append(head, tail...) |
| 348 | + return strings.Join(script, "\n"), nil |
| 349 | +} |
| 350 | + |
| 351 | +func findMachineAgentTools(env environs.Environ, series, arch string) (*tools.Tools, error) { |
| 352 | + possibleTools, err := envtools.FindInstanceTools(env, series, constraints.Value{}) |
| 353 | + if err != nil { |
| 354 | + return nil, err |
| 355 | + } |
| 356 | + arches := possibleTools.Arches() |
| 357 | + possibleTools, err = possibleTools.Match(tools.Filter{Arch: arch}) |
| 358 | + if err != nil { |
| 359 | + return nil, fmt.Errorf("chosen architecture %v not present in %v", arch, arches) |
| 360 | + } |
| 361 | + return possibleTools[0], nil |
| 362 | +} |
| 363 | |
| 364 | === added file 'environs/manual/detection.go' |
| 365 | --- environs/manual/detection.go 1970-01-01 00:00:00 +0000 |
| 366 | +++ environs/manual/detection.go 2013-08-30 01:29:18 +0000 |
| 367 | @@ -0,0 +1,118 @@ |
| 368 | +// Copyright 2013 Canonical Ltd. |
| 369 | +// Licensed under the AGPLv3, see LICENCE file for details. |
| 370 | + |
| 371 | +package manual |
| 372 | + |
| 373 | +import ( |
| 374 | + "bytes" |
| 375 | + "fmt" |
| 376 | + "os/exec" |
| 377 | + "regexp" |
| 378 | + "strconv" |
| 379 | + "strings" |
| 380 | + |
| 381 | + "launchpad.net/juju-core/instance" |
| 382 | +) |
| 383 | + |
| 384 | +// checkProvisionedScript is the script to run on the remote machine |
| 385 | +// to check if a machine has already been provisioned. |
| 386 | +// |
| 387 | +// This is a little convoluted to avoid returning an error in the |
| 388 | +// common case of no matching files. |
| 389 | +const checkProvisionedScript = "ls /etc/init/ | grep juju.*\\.conf || exit 0" |
| 390 | + |
| 391 | +// checkProvisioned checks if any juju upstart jobs already |
| 392 | +// exist on the host machine. |
| 393 | +func checkProvisioned(sshHost string) (bool, error) { |
| 394 | + cmd := exec.Command("ssh", sshHost, "bash") |
| 395 | + cmd.Stdin = bytes.NewBufferString(checkProvisionedScript) |
| 396 | + out, err := cmd.CombinedOutput() |
| 397 | + if err != nil { |
| 398 | + return false, err |
| 399 | + } |
| 400 | + return len(strings.TrimSpace(string(out))) > 0, nil |
| 401 | +} |
| 402 | + |
| 403 | +// detectSeriesHardwareCharacteristics detects the OS series |
| 404 | +// and hardware characteristics of the remote machine by |
| 405 | +// connecting to the machine and executing a bash script. |
| 406 | +func detectSeriesAndHardwareCharacteristics(sshHost string) (hc instance.HardwareCharacteristics, series string, err error) { |
| 407 | + cmd := exec.Command("ssh", sshHost, "bash") |
| 408 | + cmd.Stdin = bytes.NewBufferString(detectionScript) |
| 409 | + out, err := cmd.CombinedOutput() |
| 410 | + if err != nil { |
| 411 | + if len(out) != 0 { |
| 412 | + err = fmt.Errorf("%v (%v)", err, strings.TrimSpace(string(out))) |
| 413 | + } |
| 414 | + return hc, "", err |
| 415 | + } |
| 416 | + lines := strings.Split(string(out), "\n") |
| 417 | + series = strings.TrimSpace(lines[0]) |
| 418 | + |
| 419 | + // Normalise arch. |
| 420 | + arch := strings.TrimSpace(lines[1]) |
| 421 | + for _, re := range archREs { |
| 422 | + if re.Match([]byte(arch)) { |
| 423 | + hc.Arch = &re.arch |
| 424 | + break |
| 425 | + } |
| 426 | + } |
| 427 | + if hc.Arch == nil { |
| 428 | + err = fmt.Errorf("unrecognised architecture: %s", arch) |
| 429 | + return hc, "", err |
| 430 | + } |
| 431 | + |
| 432 | + // HardwareCharacteristics wants memory in megabytes, |
| 433 | + // meminfo reports it in kilobytes. |
| 434 | + memkB := strings.Fields(lines[2])[1] // "MemTotal: NNN kB" |
| 435 | + hc.Mem = new(uint64) |
| 436 | + *hc.Mem, err = strconv.ParseUint(memkB, 10, 0) |
| 437 | + *hc.Mem /= 1024 |
| 438 | + |
| 439 | + // For each "physical id", count the number of cores. |
| 440 | + // This way we only count physical cores, not additional |
| 441 | + // logical cores due to hyperthreading. |
| 442 | + recorded := make(map[string]bool) |
| 443 | + var physicalId string |
| 444 | + hc.CpuCores = new(uint64) |
| 445 | + for _, line := range lines[3:] { |
| 446 | + if strings.HasPrefix(line, "physical id") { |
| 447 | + physicalId = strings.TrimSpace(strings.SplitN(line, ":", 2)[1]) |
| 448 | + } else if strings.HasPrefix(line, "cpu cores") { |
| 449 | + var cores uint64 |
| 450 | + value := strings.TrimSpace(strings.SplitN(line, ":", 2)[1]) |
| 451 | + if cores, err = strconv.ParseUint(value, 10, 0); err != nil { |
| 452 | + return hc, "", err |
| 453 | + } |
| 454 | + if !recorded[physicalId] { |
| 455 | + *hc.CpuCores += cores |
| 456 | + recorded[physicalId] = true |
| 457 | + } |
| 458 | + } |
| 459 | + } |
| 460 | + if *hc.CpuCores == 0 { |
| 461 | + // In the case of a single-core, non-HT CPU, we'll see no |
| 462 | + // "physical id" or "cpu cores" lines. |
| 463 | + *hc.CpuCores = 1 |
| 464 | + } |
| 465 | + |
| 466 | + // TODO(axw) calculate CpuPower. What algorithm do we use? |
| 467 | + return hc, series, nil |
| 468 | +} |
| 469 | + |
| 470 | +// archREs maps regular expressions for matching |
| 471 | +// `uname -m` to architectures recognised by Juju. |
| 472 | +var archREs = []struct { |
| 473 | + *regexp.Regexp |
| 474 | + arch string |
| 475 | +}{ |
| 476 | + {regexp.MustCompile("amd64|x86_64"), "amd64"}, |
| 477 | + {regexp.MustCompile("i[3-9]86"), "i386"}, |
| 478 | + {regexp.MustCompile("armv.*"), "arm"}, |
| 479 | +} |
| 480 | + |
| 481 | +const detectionScript = `#!/bin/bash |
| 482 | +lsb_release -cs |
| 483 | +uname -m |
| 484 | +grep MemTotal /proc/meminfo |
| 485 | +cat /proc/cpuinfo` |
| 486 | |
| 487 | === added file 'environs/manual/detection_test.go' |
| 488 | --- environs/manual/detection_test.go 1970-01-01 00:00:00 +0000 |
| 489 | +++ environs/manual/detection_test.go 2013-08-30 01:29:18 +0000 |
| 490 | @@ -0,0 +1,141 @@ |
| 491 | +// Copyright 2013 Canonical Ltd. |
| 492 | +// Licensed under the AGPLv3, see LICENCE file for details. |
| 493 | + |
| 494 | +package manual |
| 495 | + |
| 496 | +import ( |
| 497 | + "fmt" |
| 498 | + "io/ioutil" |
| 499 | + "os" |
| 500 | + "path/filepath" |
| 501 | + "strings" |
| 502 | + |
| 503 | + gc "launchpad.net/gocheck" |
| 504 | + |
| 505 | + "launchpad.net/juju-core/testing" |
| 506 | +) |
| 507 | + |
| 508 | +type detectionSuite struct { |
| 509 | + testing.LoggingSuite |
| 510 | +} |
| 511 | + |
| 512 | +var _ = gc.Suite(&detectionSuite{}) |
| 513 | + |
| 514 | +// sshscript should only print the result on the first execution, |
| 515 | +// to handle the case where it's called multiple times. On |
| 516 | +// subsequent executions, it should find the next 'ssh' in $PATH |
| 517 | +// and exec that. |
| 518 | +var sshscript = `#!/bin/bash |
| 519 | +if [ ! -e "$0.run" ]; then |
| 520 | + touch "$0.run" |
| 521 | + diff "$0.expected-input" - |
| 522 | + exitcode=$? |
| 523 | + if [ $exitcode -ne 0 ]; then |
| 524 | + echo "ERROR: did not match expected input" >&2 |
| 525 | + exit $exitcode |
| 526 | + fi |
| 527 | +%s |
| 528 | + exit %d |
| 529 | +else |
| 530 | + export PATH=${PATH#*:} |
| 531 | + exec ssh $* |
| 532 | +fi` |
| 533 | + |
| 534 | +// sshresponse creates a fake "ssh" command in a new $PATH, |
| 535 | +// updates $PATH, and returns a function to reset $PATH to |
| 536 | +// its original value when called. |
| 537 | +func sshresponse(c *gc.C, input, output string, rc int) func() { |
| 538 | + fakebin := c.MkDir() |
| 539 | + ssh := filepath.Join(fakebin, "ssh") |
| 540 | + sshexpectedinput := ssh + ".expected-input" |
| 541 | + if output != "" { |
| 542 | + output = fmt.Sprintf("cat<<EOF\n%s\nEOF", output) |
| 543 | + } |
| 544 | + script := fmt.Sprintf(sshscript, output, rc) |
| 545 | + err := ioutil.WriteFile(ssh, []byte(script), 0777) |
| 546 | + c.Assert(err, gc.IsNil) |
| 547 | + err = ioutil.WriteFile(sshexpectedinput, []byte(input), 0644) |
| 548 | + c.Assert(err, gc.IsNil) |
| 549 | + return testing.PatchEnvironment("PATH", fakebin+":"+os.Getenv("PATH")) |
| 550 | +} |
| 551 | + |
| 552 | +func (s *detectionSuite) TestDetectSeries(c *gc.C) { |
| 553 | + response := strings.Join([]string{ |
| 554 | + "edgy", |
| 555 | + "armv4", |
| 556 | + "MemTotal: 4096 kB", |
| 557 | + "processor: 0", |
| 558 | + }, "\n") |
| 559 | + defer sshresponse(c, detectionScript, response, 0)() |
| 560 | + _, series, err := detectSeriesAndHardwareCharacteristics("whatever") |
| 561 | + c.Assert(err, gc.IsNil) |
| 562 | + c.Assert(series, gc.Equals, "edgy") |
| 563 | +} |
| 564 | + |
| 565 | +func (s *detectionSuite) TestDetectionError(c *gc.C) { |
| 566 | + defer sshresponse(c, detectionScript, "oh noes", 33)() |
| 567 | + _, _, err := detectSeriesAndHardwareCharacteristics("whatever") |
| 568 | + c.Assert(err, gc.ErrorMatches, "exit status 33 \\(oh noes\\)") |
| 569 | +} |
| 570 | + |
| 571 | +func (s *detectionSuite) TestDetectHardwareCharacteristics(c *gc.C) { |
| 572 | + tests := []struct { |
| 573 | + summary string |
| 574 | + scriptResponse []string |
| 575 | + expectedHc string |
| 576 | + }{{ |
| 577 | + "Single CPU socket, single core, no hyper-threading", |
| 578 | + []string{"edgy", "armv4", "MemTotal: 4096 kB", "processor: 0"}, |
| 579 | + "arch=arm cpu-cores=1 mem=4M", |
| 580 | + }, { |
| 581 | + "Single CPU socket, single core, hyper-threading", |
| 582 | + []string{ |
| 583 | + "edgy", "armv4", "MemTotal: 4096 kB", |
| 584 | + "processor: 0", |
| 585 | + "physical id: 0", |
| 586 | + "cpu cores: 1", |
| 587 | + "processor: 1", |
| 588 | + "physical id: 0", |
| 589 | + "cpu cores: 1", |
| 590 | + }, |
| 591 | + "arch=arm cpu-cores=1 mem=4M", |
| 592 | + }, { |
| 593 | + "Single CPU socket, dual-core, no hyper-threading", |
| 594 | + []string{ |
| 595 | + "edgy", "armv4", "MemTotal: 4096 kB", |
| 596 | + "processor: 0", |
| 597 | + "physical id: 0", |
| 598 | + "cpu cores: 2", |
| 599 | + "processor: 1", |
| 600 | + "physical id: 0", |
| 601 | + "cpu cores: 2", |
| 602 | + }, |
| 603 | + "arch=arm cpu-cores=2 mem=4M", |
| 604 | + }, { |
| 605 | + "Dual CPU socket, each single-core, hyper-threading", |
| 606 | + []string{ |
| 607 | + "edgy", "armv4", "MemTotal: 4096 kB", |
| 608 | + "processor: 0", |
| 609 | + "physical id: 0", |
| 610 | + "cpu cores: 1", |
| 611 | + "processor: 1", |
| 612 | + "physical id: 0", |
| 613 | + "cpu cores: 1", |
| 614 | + "processor: 2", |
| 615 | + "physical id: 1", |
| 616 | + "cpu cores: 1", |
| 617 | + "processor: 3", |
| 618 | + "physical id: 1", |
| 619 | + "cpu cores: 1", |
| 620 | + }, |
| 621 | + "arch=arm cpu-cores=2 mem=4M", |
| 622 | + }} |
| 623 | + for i, test := range tests { |
| 624 | + c.Logf("test %d: %s", i, test.summary) |
| 625 | + scriptResponse := strings.Join(test.scriptResponse, "\n") |
| 626 | + defer sshresponse(c, detectionScript, scriptResponse, 0)() |
| 627 | + hc, _, err := detectSeriesAndHardwareCharacteristics("hostname") |
| 628 | + c.Assert(err, gc.IsNil) |
| 629 | + c.Assert(hc.String(), gc.Equals, test.expectedHc) |
| 630 | + } |
| 631 | +} |
| 632 | |
| 633 | === added file 'environs/manual/provisioner.go' |
| 634 | --- environs/manual/provisioner.go 1970-01-01 00:00:00 +0000 |
| 635 | +++ environs/manual/provisioner.go 2013-08-30 01:29:18 +0000 |
| 636 | @@ -0,0 +1,184 @@ |
| 637 | +// Copyright 2013 Canonical Ltd. |
| 638 | +// Licensed under the AGPLv3, see LICENCE file for details. |
| 639 | + |
| 640 | +package manual |
| 641 | + |
| 642 | +import ( |
| 643 | + "errors" |
| 644 | + "fmt" |
| 645 | + "strings" |
| 646 | + |
| 647 | + "launchpad.net/loggo" |
| 648 | + |
| 649 | + "launchpad.net/juju-core/environs" |
| 650 | + "launchpad.net/juju-core/instance" |
| 651 | + "launchpad.net/juju-core/juju" |
| 652 | + "launchpad.net/juju-core/state" |
| 653 | + "launchpad.net/juju-core/state/api" |
| 654 | + "launchpad.net/juju-core/tools" |
| 655 | + "launchpad.net/juju-core/utils" |
| 656 | + "launchpad.net/juju-core/worker/provisioner" |
| 657 | +) |
| 658 | + |
| 659 | +const manualInstancePrefix = "manual:" |
| 660 | + |
| 661 | +var logger = loggo.GetLogger("juju.environs.manual") |
| 662 | + |
| 663 | +type ProvisionMachineArgs struct { |
| 664 | + // Host is the SSH host: [user@]host |
| 665 | + Host string |
| 666 | + |
| 667 | + // DataDir is the root directory for juju data. |
| 668 | + // If left blank, the default location "/var/lib/juju" will be used. |
| 669 | + DataDir string |
| 670 | + |
| 671 | + // State is the *state.State object to register the machine with. |
| 672 | + State *state.State |
| 673 | + |
| 674 | + // Tools to install on the machine. If nil, tools will be automatically |
| 675 | + // chosen using environs/tools FindInstanceTools. |
| 676 | + Tools *tools.Tools |
| 677 | +} |
| 678 | + |
| 679 | +// ErrProvisioned is returned by ProvisionMachine if the target |
| 680 | +// machine has an existing machine agent. |
| 681 | +var ErrProvisioned = errors.New("machine is already provisioned") |
| 682 | + |
| 683 | +// ProvisionMachine provisions a machine agent to an existing host, via |
| 684 | +// an SSH connection to the specified host. The host may optionally be preceded |
| 685 | +// with a login username, as in [user@]host. |
| 686 | +// |
| 687 | +// On successful completion, this function will return the state.Machine |
| 688 | +// that was entered into state. |
| 689 | +func ProvisionMachine(args ProvisionMachineArgs) (m *state.Machine, err error) { |
| 690 | + defer func() { |
| 691 | + if m != nil && err != nil { |
| 692 | + m.EnsureDead() |
| 693 | + m.Remove() |
| 694 | + m = nil |
| 695 | + } |
| 696 | + }() |
| 697 | + |
| 698 | + var env environs.Environ |
| 699 | + if conn, err := juju.NewConnFromState(args.State); err != nil { |
| 700 | + return nil, err |
| 701 | + } else { |
| 702 | + env = conn.Environ |
| 703 | + } |
| 704 | + |
| 705 | + sshHostWithoutUser := args.Host |
| 706 | + if at := strings.Index(sshHostWithoutUser, "@"); at != -1 { |
| 707 | + sshHostWithoutUser = sshHostWithoutUser[at+1:] |
| 708 | + } |
| 709 | + addrs, err := instance.HostAddresses(sshHostWithoutUser) |
| 710 | + if err != nil { |
| 711 | + return nil, err |
| 712 | + } |
| 713 | + |
| 714 | + provisioned, err := checkProvisioned(args.Host) |
| 715 | + if err != nil { |
| 716 | + err = fmt.Errorf("error checking if provisioned: %v", err) |
| 717 | + return nil, err |
| 718 | + } |
| 719 | + if provisioned { |
| 720 | + return nil, ErrProvisioned |
| 721 | + } |
| 722 | + |
| 723 | + hc, series, err := detectSeriesAndHardwareCharacteristics(args.Host) |
| 724 | + if err != nil { |
| 725 | + err = fmt.Errorf("error detecting hardware characteristics: %v", err) |
| 726 | + return nil, err |
| 727 | + } |
| 728 | + |
| 729 | + // Generate a unique nonce for the machine. |
| 730 | + uuid, err := utils.NewUUID() |
| 731 | + if err != nil { |
| 732 | + return nil, err |
| 733 | + } |
| 734 | + |
| 735 | + // Inject a new machine into state. |
| 736 | + // |
| 737 | + // There will never be a corresponding "instance" that any provider |
| 738 | + // knows about. This is fine, and works well with the provisioner |
| 739 | + // task. The provisioner task will happily remove any and all dead |
| 740 | + // machines from state, but will ignore the associated instance ID |
| 741 | + // if it isn't one that the environment provider knows about. |
| 742 | + instanceId := instance.Id(manualInstancePrefix + sshHostWithoutUser) |
| 743 | + nonce := fmt.Sprintf("%s:%s", instanceId, uuid.String()) |
| 744 | + m, err = injectMachine(injectMachineArgs{ |
| 745 | + st: args.State, |
| 746 | + instanceId: instanceId, |
| 747 | + addrs: addrs, |
| 748 | + series: series, |
| 749 | + hc: hc, |
| 750 | + nonce: nonce, |
| 751 | + }) |
| 752 | + if err != nil { |
| 753 | + return nil, err |
| 754 | + } |
| 755 | + stateInfo, apiInfo, err := setupAuthentication(env, m) |
| 756 | + if err != nil { |
| 757 | + return m, err |
| 758 | + } |
| 759 | + |
| 760 | + // Finally, provision the machine agent. |
| 761 | + err = provisionMachineAgent(provisionMachineAgentArgs{ |
| 762 | + host: args.Host, |
| 763 | + dataDir: args.DataDir, |
| 764 | + env: env, |
| 765 | + machine: m, |
| 766 | + nonce: nonce, |
| 767 | + stateInfo: stateInfo, |
| 768 | + apiInfo: apiInfo, |
| 769 | + series: series, |
| 770 | + arch: *hc.Arch, |
| 771 | + tools: args.Tools, |
| 772 | + }) |
| 773 | + if err != nil { |
| 774 | + return m, err |
| 775 | + } |
| 776 | + |
| 777 | + logger.Infof("Provisioned machine %v", m) |
| 778 | + return m, nil |
| 779 | +} |
| 780 | + |
| 781 | +type injectMachineArgs struct { |
| 782 | + st *state.State |
| 783 | + instanceId instance.Id |
| 784 | + addrs []instance.Address |
| 785 | + series string |
| 786 | + hc instance.HardwareCharacteristics |
| 787 | + nonce string |
| 788 | +} |
| 789 | + |
| 790 | +// injectMachine injects a machine into state with provisioned status. |
| 791 | +func injectMachine(args injectMachineArgs) (m *state.Machine, err error) { |
| 792 | + defer func() { |
| 793 | + if m != nil && err != nil { |
| 794 | + m.EnsureDead() |
| 795 | + m.Remove() |
| 796 | + } |
| 797 | + }() |
| 798 | + m, err = args.st.InjectMachine(&state.AddMachineParams{ |
| 799 | + Series: args.series, |
| 800 | + InstanceId: args.instanceId, |
| 801 | + HardwareCharacteristics: args.hc, |
| 802 | + Nonce: args.nonce, |
| 803 | + Jobs: []state.MachineJob{state.JobHostUnits}, |
| 804 | + }) |
| 805 | + if err != nil { |
| 806 | + return nil, err |
| 807 | + } |
| 808 | + if err = m.SetAddresses(args.addrs); err != nil { |
| 809 | + return nil, err |
| 810 | + } |
| 811 | + return m, nil |
| 812 | +} |
| 813 | + |
| 814 | +func setupAuthentication(env environs.Environ, m *state.Machine) (*state.Info, *api.Info, error) { |
| 815 | + auth, err := provisioner.NewSimpleAuthenticator(env) |
| 816 | + if err != nil { |
| 817 | + return nil, nil, err |
| 818 | + } |
| 819 | + return auth.SetupAuthentication(m) |
| 820 | +} |
| 821 | |
| 822 | === added file 'environs/manual/provisioner_test.go' |
| 823 | --- environs/manual/provisioner_test.go 1970-01-01 00:00:00 +0000 |
| 824 | +++ environs/manual/provisioner_test.go 2013-08-30 01:29:18 +0000 |
| 825 | @@ -0,0 +1,85 @@ |
| 826 | +// Copyright 2013 Canonical Ltd. |
| 827 | +// Licensed under the AGPLv3, see LICENCE file for details. |
| 828 | + |
| 829 | +package manual |
| 830 | + |
| 831 | +import ( |
| 832 | + "fmt" |
| 833 | + "os" |
| 834 | + "strings" |
| 835 | + |
| 836 | + gc "launchpad.net/gocheck" |
| 837 | + |
| 838 | + "launchpad.net/juju-core/constraints" |
| 839 | + "launchpad.net/juju-core/environs/tools" |
| 840 | + "launchpad.net/juju-core/instance" |
| 841 | + "launchpad.net/juju-core/juju/testing" |
| 842 | +) |
| 843 | + |
| 844 | +type provisionerSuite struct { |
| 845 | + testing.JujuConnSuite |
| 846 | +} |
| 847 | + |
| 848 | +var _ = gc.Suite(&provisionerSuite{}) |
| 849 | + |
| 850 | +func (s *provisionerSuite) getArgs(c *gc.C) ProvisionMachineArgs { |
| 851 | + hostname, err := os.Hostname() |
| 852 | + c.Assert(err, gc.IsNil) |
| 853 | + return ProvisionMachineArgs{ |
| 854 | + Host: hostname, |
| 855 | + State: s.State, |
| 856 | + } |
| 857 | +} |
| 858 | + |
| 859 | +func (s *provisionerSuite) TestProvisionMachine(c *gc.C) { |
| 860 | + // Prepare a mock ssh response for the detection phase. |
| 861 | + detectionoutput := strings.Join([]string{ |
| 862 | + "edgy", |
| 863 | + "armv4", |
| 864 | + "MemTotal: 4096 kB", |
| 865 | + "processor: 0", |
| 866 | + }, "\n") |
| 867 | + |
| 868 | + args := s.getArgs(c) |
| 869 | + hostname := args.Host |
| 870 | + args.Host = "ubuntu@" + args.Host |
| 871 | + |
| 872 | + defer sshresponse(c, detectionScript, detectionoutput, 0)() |
| 873 | + defer sshresponse(c, checkProvisionedScript, "", 0)() |
| 874 | + m, err := ProvisionMachine(args) |
| 875 | + c.Assert(err, gc.ErrorMatches, "no matching tools available") |
| 876 | + c.Assert(m, gc.IsNil) |
| 877 | + |
| 878 | + toolsList, err := tools.FindBootstrapTools(s.Conn.Environ, constraints.Value{}) |
| 879 | + c.Assert(err, gc.IsNil) |
| 880 | + args.Tools = toolsList[0] |
| 881 | + |
| 882 | + for _, errorCode := range []int{255, 0} { |
| 883 | + defer sshresponse(c, "", "", errorCode)() // executing script |
| 884 | + defer sshresponse(c, detectionScript, detectionoutput, 0)() |
| 885 | + defer sshresponse(c, checkProvisionedScript, "", 0)() |
| 886 | + m, err = ProvisionMachine(args) |
| 887 | + if errorCode != 0 { |
| 888 | + c.Assert(err, gc.ErrorMatches, fmt.Sprintf("exit status %d", errorCode)) |
| 889 | + c.Assert(m, gc.IsNil) |
| 890 | + } else { |
| 891 | + c.Assert(err, gc.IsNil) |
| 892 | + c.Assert(m, gc.NotNil) |
| 893 | + // machine ID will be 2, not 1. Even though we failed and the |
| 894 | + // machine is removed, the ID is not reused. |
| 895 | + c.Assert(m.Id(), gc.Equals, "2") |
| 896 | + instanceId, err := m.InstanceId() |
| 897 | + c.Assert(err, gc.IsNil) |
| 898 | + c.Assert(instanceId, gc.Equals, instance.Id("manual:"+hostname)) |
| 899 | + } |
| 900 | + } |
| 901 | + |
| 902 | + // Attempting to provision a machine twice should fail. We effect |
| 903 | + // this by checking for existing juju upstart configurations. |
| 904 | + defer sshresponse(c, checkProvisionedScript, "/etc/init/jujud-machine-0.conf", 0)() |
| 905 | + _, err = ProvisionMachine(args) |
| 906 | + c.Assert(err, gc.Equals, ErrProvisioned) |
| 907 | + defer sshresponse(c, checkProvisionedScript, "/etc/init/jujud-machine-0.conf", 255)() |
| 908 | + _, err = ProvisionMachine(args) |
| 909 | + c.Assert(err, gc.ErrorMatches, "error checking if provisioned: exit status 255") |
| 910 | +} |
| 911 | |
| 912 | === added file 'environs/manual/suite_test.go' |
| 913 | --- environs/manual/suite_test.go 1970-01-01 00:00:00 +0000 |
| 914 | +++ environs/manual/suite_test.go 2013-08-30 01:29:18 +0000 |
| 915 | @@ -0,0 +1,14 @@ |
| 916 | +// Copyright 2013 Canonical Ltd. |
| 917 | +// Licensed under the AGPLv3, see LICENCE file for details. |
| 918 | + |
| 919 | +package manual |
| 920 | + |
| 921 | +import ( |
| 922 | + stdtesting "testing" |
| 923 | + |
| 924 | + "launchpad.net/juju-core/testing" |
| 925 | +) |
| 926 | + |
| 927 | +func Test(t *stdtesting.T) { |
| 928 | + testing.MgoTestPackage(t) |
| 929 | +} |
| 930 | |
| 931 | === modified file 'environs/tools/tools.go' |
| 932 | --- environs/tools/tools.go 2013-08-28 12:04:26 +0000 |
| 933 | +++ environs/tools/tools.go 2013-08-30 01:29:18 +0000 |
| 934 | @@ -6,12 +6,13 @@ |
| 935 | import ( |
| 936 | "fmt" |
| 937 | |
| 938 | + "launchpad.net/loggo" |
| 939 | + |
| 940 | "launchpad.net/juju-core/constraints" |
| 941 | "launchpad.net/juju-core/environs" |
| 942 | "launchpad.net/juju-core/errors" |
| 943 | coretools "launchpad.net/juju-core/tools" |
| 944 | "launchpad.net/juju-core/version" |
| 945 | - "launchpad.net/loggo" |
| 946 | ) |
| 947 | |
| 948 | var logger = loggo.GetLogger("juju.environs.tools") |
| 949 | |
| 950 | === modified file 'instance/address.go' |
| 951 | --- instance/address.go 2013-08-20 16:10:12 +0000 |
| 952 | +++ instance/address.go 2013-08-30 01:29:18 +0000 |
| 953 | @@ -59,6 +59,27 @@ |
| 954 | return Address{value, addresstype, "", NetworkUnknown} |
| 955 | } |
| 956 | |
| 957 | +// HostAddresses looks up the IP addresses of the specified |
| 958 | +// host, and translates them into instance.Address values. |
| 959 | +func HostAddresses(host string) ([]Address, error) { |
| 960 | + ipaddrs, err := net.LookupIP(host) |
| 961 | + if err != nil { |
| 962 | + return nil, err |
| 963 | + } |
| 964 | + addrs := make([]Address, len(ipaddrs)) |
| 965 | + for i, ipaddr := range ipaddrs { |
| 966 | + switch len(ipaddr) { |
| 967 | + case 4: |
| 968 | + addrs[i].Type = Ipv4Address |
| 969 | + addrs[i].Value = ipaddr.String() |
| 970 | + case 16: |
| 971 | + addrs[i].Type = Ipv6Address |
| 972 | + addrs[i].Value = ipaddr.String() |
| 973 | + } |
| 974 | + } |
| 975 | + return addrs, err |
| 976 | +} |
| 977 | + |
| 978 | // SelectPublicAddress picks one address from a slice that would |
| 979 | // be appropriate to display as a publicly accessible endpoint. |
| 980 | // If there are no suitable addresses, the empty string is returned. |
| 981 | |
| 982 | === modified file 'provider/local/environ.go' |
| 983 | --- provider/local/environ.go 2013-08-27 18:33:36 +0000 |
| 984 | +++ provider/local/environ.go 2013-08-30 01:29:18 +0000 |
| 985 | @@ -346,7 +346,7 @@ |
| 986 | logger.Infof("removing service %s", env.machineAgentServiceName()) |
| 987 | machineAgent := upstart.NewService(env.machineAgentServiceName()) |
| 988 | machineAgent.InitDir = upstartScriptLocation |
| 989 | - if err := machineAgent.Remove(); err != nil { |
| 990 | + if err := machineAgent.StopAndRemove(); err != nil { |
| 991 | logger.Errorf("could not remove machine agent service: %v", err) |
| 992 | return err |
| 993 | } |
| 994 | @@ -354,7 +354,7 @@ |
| 995 | logger.Infof("removing service %s", env.mongoServiceName()) |
| 996 | mongo := upstart.NewService(env.mongoServiceName()) |
| 997 | mongo.InitDir = upstartScriptLocation |
| 998 | - if err := mongo.Remove(); err != nil { |
| 999 | + if err := mongo.StopAndRemove(); err != nil { |
| 1000 | logger.Errorf("could not remove mongo service: %v", err) |
| 1001 | return err |
| 1002 | } |
| 1003 | |
| 1004 | === modified file 'provider/provider.go' |
| 1005 | --- provider/provider.go 2013-08-27 18:33:36 +0000 |
| 1006 | +++ provider/provider.go 2013-08-30 01:29:18 +0000 |
| 1007 | @@ -12,4 +12,5 @@ |
| 1008 | MAAS = "maas" |
| 1009 | Azure = "azure" |
| 1010 | OpenStack = "openstack" |
| 1011 | + Manual = "manual" |
| 1012 | ) |
| 1013 | |
| 1014 | === modified file 'state/state_test.go' |
| 1015 | --- state/state_test.go 2013-08-23 07:58:04 +0000 |
| 1016 | +++ state/state_test.go 2013-08-30 01:29:18 +0000 |
| 1017 | @@ -366,7 +366,7 @@ |
| 1018 | c.Assert(*characteristics, gc.DeepEquals, params.HardwareCharacteristics) |
| 1019 | |
| 1020 | // Make sure the bootstrap nonce value is set. |
| 1021 | - c.Assert(m.CheckProvisioned(state.BootstrapNonce), gc.Equals, true) |
| 1022 | + c.Assert(m.CheckProvisioned(params.Nonce), gc.Equals, true) |
| 1023 | } |
| 1024 | |
| 1025 | func (s *StateSuite) TestAddContainerToInjectedMachine(c *gc.C) { |
| 1026 | |
| 1027 | === modified file 'upstart/upstart.go' |
| 1028 | --- upstart/upstart.go 2013-05-02 15:55:42 +0000 |
| 1029 | +++ upstart/upstart.go 2013-08-30 01:29:18 +0000 |
| 1030 | @@ -77,14 +77,23 @@ |
| 1031 | return runCommand("stop", s.Name) |
| 1032 | } |
| 1033 | |
| 1034 | +// StopAndRemove stops the service and then deletes the service |
| 1035 | +// configuration from the init directory. |
| 1036 | +func (s *Service) StopAndRemove() error { |
| 1037 | + if !s.Installed() { |
| 1038 | + return nil |
| 1039 | + } |
| 1040 | + if err := s.Stop(); err != nil { |
| 1041 | + return err |
| 1042 | + } |
| 1043 | + return os.Remove(s.confPath()) |
| 1044 | +} |
| 1045 | + |
| 1046 | // Remove deletes the service configuration from the init directory. |
| 1047 | func (s *Service) Remove() error { |
| 1048 | if !s.Installed() { |
| 1049 | return nil |
| 1050 | } |
| 1051 | - if err := s.Stop(); err != nil { |
| 1052 | - return err |
| 1053 | - } |
| 1054 | return os.Remove(s.confPath()) |
| 1055 | } |
| 1056 | |
| 1057 | @@ -157,7 +166,7 @@ |
| 1058 | return err |
| 1059 | } |
| 1060 | if c.Installed() { |
| 1061 | - if err := c.Remove(); err != nil { |
| 1062 | + if err := c.StopAndRemove(); err != nil { |
| 1063 | return fmt.Errorf("upstart: could not remove installed service: %s", err) |
| 1064 | } |
| 1065 | } |
| 1066 | |
| 1067 | === modified file 'upstart/upstart_test.go' |
| 1068 | --- upstart/upstart_test.go 2013-08-19 11:20:02 +0000 |
| 1069 | +++ upstart/upstart_test.go 2013-08-30 01:29:18 +0000 |
| 1070 | @@ -12,7 +12,7 @@ |
| 1071 | |
| 1072 | gc "launchpad.net/gocheck" |
| 1073 | |
| 1074 | - "launchpad.net/juju-core/testing/checkers" |
| 1075 | + jc "launchpad.net/juju-core/testing/checkers" |
| 1076 | "launchpad.net/juju-core/upstart" |
| 1077 | ) |
| 1078 | |
| 1079 | @@ -107,26 +107,41 @@ |
| 1080 | func (s *UpstartSuite) TestRemoveMissing(c *gc.C) { |
| 1081 | err := os.Remove(filepath.Join(s.service.InitDir, "some-service.conf")) |
| 1082 | c.Assert(err, gc.IsNil) |
| 1083 | - c.Assert(s.service.Remove(), gc.IsNil) |
| 1084 | + c.Assert(s.service.StopAndRemove(), gc.IsNil) |
| 1085 | } |
| 1086 | |
| 1087 | func (s *UpstartSuite) TestRemoveStopped(c *gc.C) { |
| 1088 | s.StoppedStatus(c) |
| 1089 | - c.Assert(s.service.Remove(), gc.IsNil) |
| 1090 | + c.Assert(s.service.StopAndRemove(), gc.IsNil) |
| 1091 | _, err := os.Stat(filepath.Join(s.service.InitDir, "some-service.conf")) |
| 1092 | - c.Assert(err, checkers.Satisfies, os.IsNotExist) |
| 1093 | + c.Assert(err, jc.Satisfies, os.IsNotExist) |
| 1094 | } |
| 1095 | |
| 1096 | func (s *UpstartSuite) TestRemoveRunning(c *gc.C) { |
| 1097 | s.RunningStatus(c) |
| 1098 | s.MakeTool(c, "stop", "exit 99") |
| 1099 | - c.Assert(s.service.Remove(), gc.ErrorMatches, ".*exit status 99.*") |
| 1100 | + c.Assert(s.service.StopAndRemove(), gc.ErrorMatches, ".*exit status 99.*") |
| 1101 | _, err := os.Stat(filepath.Join(s.service.InitDir, "some-service.conf")) |
| 1102 | c.Assert(err, gc.IsNil) |
| 1103 | s.MakeTool(c, "stop", "exit 0") |
| 1104 | + c.Assert(s.service.StopAndRemove(), gc.IsNil) |
| 1105 | + _, err = os.Stat(filepath.Join(s.service.InitDir, "some-service.conf")) |
| 1106 | + c.Assert(err, jc.Satisfies, os.IsNotExist) |
| 1107 | +} |
| 1108 | + |
| 1109 | +func (s *UpstartSuite) TestStopAndRemove(c *gc.C) { |
| 1110 | + s.RunningStatus(c) |
| 1111 | + s.MakeTool(c, "stop", "exit 99") |
| 1112 | + |
| 1113 | + // StopAndRemove will fail, as it calls stop. |
| 1114 | + c.Assert(s.service.StopAndRemove(), gc.ErrorMatches, ".*exit status 99.*") |
| 1115 | + _, err := os.Stat(filepath.Join(s.service.InitDir, "some-service.conf")) |
| 1116 | + c.Assert(err, gc.IsNil) |
| 1117 | + |
| 1118 | + // Plain old Remove will succeed. |
| 1119 | c.Assert(s.service.Remove(), gc.IsNil) |
| 1120 | _, err = os.Stat(filepath.Join(s.service.InitDir, "some-service.conf")) |
| 1121 | - c.Assert(err, checkers.Satisfies, os.IsNotExist) |
| 1122 | + c.Assert(err, jc.Satisfies, os.IsNotExist) |
| 1123 | } |
| 1124 | |
| 1125 | func (s *UpstartSuite) TestInstallErrors(c *gc.C) { |
| 1126 | @@ -236,5 +251,5 @@ |
| 1127 | conf := s.dummyConf(c) |
| 1128 | err = conf.Install() |
| 1129 | c.Assert(err, gc.IsNil) |
| 1130 | - c.Assert(&conf.Service, checkers.Satisfies, (*upstart.Service).Running) |
| 1131 | + c.Assert(&conf.Service, jc.Satisfies, (*upstart.Service).Running) |
| 1132 | } |
| 1133 | |
| 1134 | === modified file 'worker/deployer/simple.go' |
| 1135 | --- worker/deployer/simple.go 2013-08-22 03:39:33 +0000 |
| 1136 | +++ worker/deployer/simple.go 2013-08-30 01:29:18 +0000 |
| 1137 | @@ -171,7 +171,7 @@ |
| 1138 | if svc == nil || !svc.Installed() { |
| 1139 | return fmt.Errorf("unit %q is not deployed", unitName) |
| 1140 | } |
| 1141 | - if err := svc.Remove(); err != nil { |
| 1142 | + if err := svc.StopAndRemove(); err != nil { |
| 1143 | return err |
| 1144 | } |
| 1145 | tag := names.UnitTag(unitName) |
https:/
File cmd/juju/
https:/
cmd/juju/
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:/
cmd/juju/
([]instance.
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:/
cmd/juju/
What if the host is an ip address?
https:/
cmd/juju/
Oh how I wish cloud vendors would have good IPv6 support.
https:/
cmd/juju/
manuallyProvisi
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:/
cmd/juju/
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:/
cmd/juju/
Why create a new environment when the conn parameter should have one
already?
https:/
cmd/juju/
constraints.
Care to add a comment here that this is just for the architecture? I'm
assuming it is.
https:/
cmd/juju/
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:/
cmd/juju/
c.SSHHost)
Oh, here goes the "ssh:" string again.
https:/
cmd/juju/
c.Constraints, instanceId, hc, state.JobHostUnits)
We need to add another thing to th...