juju-core

Merge lp://staging/~axwalk/juju-core/wire-up-prechecker into lp://staging/~go-bot/juju-core/trunk

  1. wire-up-prechecker
  2. Merge into trunk
Proposed by Andrew Wilkins
Status: Work in progress
Proposed branch: lp://staging/~axwalk/juju-core/wire-up-prechecker
Merge into: lp://staging/~go-bot/juju-core/trunk
Diff against target: 768 lines (+351/-69) (has conflicts)
20 files modified
cmd/jujud/agent.go (+9/-0)
cmd/jujud/machine.go (+13/-0)
environs/base.go (+22/-0)
environs/interface.go (+5/-22)
environs/jujutest/livetests.go (+4/-9)
juju/conn.go (+28/-11)
juju/conn_test.go (+36/-0)
provider/azure/environ.go (+2/-2)
provider/dummy/environs.go (+2/-0)
provider/ec2/ec2.go (+2/-2)
provider/ec2/local_test.go (+2/-4)
provider/local/environ.go (+27/-1)
provider/local/environ_test.go (+2/-5)
provider/maas/environ.go (+2/-0)
provider/null/environ.go (+12/-0)
provider/openstack/local_test.go (+2/-4)
provider/openstack/provider.go (+2/-2)
state/open.go (+2/-2)
state/state.go (+57/-5)
state/state_test.go (+120/-0)
Text conflict in provider/local/environ.go
Text conflict in provider/null/environ.go
Text conflict in state/state_test.go
To merge this branch: bzr merge lp://staging/~axwalk/juju-core/wire-up-prechecker
Reviewer Review Type Date Requested Status
Juju Engineering Pending
Review via email: mp+188001@code.staging.launchpad.net

Description of the change

Wire up prechecker

This change is to set a Prechecker in
the CLI, and in cmd/jujud. The Environ
itself is a Prechecker, and that is
what is assigned to the state object.

State now calls the prechecker when
decoding to create instance/container
machine entries in state. The prechecker
calls are elided for "injected" machines.

Environ embeds the Prechecker interface.
All Environ implementations now embed
environs.EnvironBase, which implements
defaults for an environs.Environ. Currently
it only implements Prechecker, by
embedding a NilPrechecker (i.e. one that
allows everything through).

All Environs, apart from MAAS, disallow
containers. The null provider disallows
everything (unless done "manually").

https://codereview.appspot.com/14032043/

To post a comment you must log in.
Revision history for this message
William Reade (fwereade) wrote :

Thoughts:

https://codereview.appspot.com/14032043/diff/1/state/state.go
File state/state.go (right):

https://codereview.appspot.com/14032043/diff/1/state/state.go#newcode292
state/state.go:292: // Prechecker is a clone of environs.Prechecker,
here to avoid the dependency.
Better I think to define it in state, and document (in code) that
appropriate Environs implement it. environs already knows about state,
not vice versa, so we may as well just define the interface in the place
that uses it. After all, it really *doesn't* have to be just an environ
that satsisfies that interface: we'd surely want the state tests to be
written with a suitable test implementation, not of a whole environ.

https://codereview.appspot.com/14032043/diff/1/state/state.go#newcode302
state/state.go:302: }
/me strokes chin

Leaving aside the fact that we'll need lots of locking, and that makes
me a bit suspicious, this is kinda wrong; I'm worried that we would not
in general be in a position to track changes to the environment, and are
therefore in some danger of screwing up by using out-of-date
environments.

But my spidey-sense is tingling wrt the provisioner. That too needs an
up-to-date environment, and we've tied ourselves in frightful knots over
it; and, really, I don't think we need to at all. An Environ is meant to
be goroutine-safe, after all, so it might be reasonable to get a single
Environ per (authorized) agent and run a task that keeps it updated via
the API by validating changes and calling SetConfig; we can then use
that single task to keep all the environs in sync.

(I'm thinking of a Task that also implements several one-method
interfaces -- WaitPrechecker, WaitInstanceBroker, WaitPortOpenCloseThing
-- that all share an implementation (return the single Environ held by
the task once it's available); and which just watches the API and does
the whole SetConfig lark.)

Can anyone think of a reason not to do this?

https://codereview.appspot.com/14032043/

Reply
Revision history for this message
Andrew Wilkins (axwalk) wrote :

https://codereview.appspot.com/14032043/diff/1/state/state.go
File state/state.go (right):

https://codereview.appspot.com/14032043/diff/1/state/state.go#newcode292
state/state.go:292: // Prechecker is a clone of environs.Prechecker,
here to avoid the dependency.
On 2013/09/27 10:59:22, fwereade wrote:
> Better I think to define it in state, and document (in code) that
 appropriate
> Environs implement it. environs already knows about state, not vice
 versa, so we
> may as well just define the interface in the place that uses it. After
 all, it
> really *doesn't* have to be just an environ that satsisfies that
 interface: we'd
> surely want the state tests to be written with a suitable test
 implementation,
> not of a whole environ.

Fair enough, I'll do that. Do you think it'd be worthwhile adding
something like below to environs/interface.go?

// Prechecker is an optional Environ interface. See state.Prechecker for
details.
type Prechecker interface {
     state.Prechecker
}

https://codereview.appspot.com/14032043/diff/1/state/state.go#newcode302
state/state.go:302: }
On 2013/09/27 10:59:22, fwereade wrote:
> /me strokes chin

> Leaving aside the fact that we'll need lots of locking, and that makes
me a bit
> suspicious, this is kinda wrong; I'm worried that we would not in
general be in
> a position to track changes to the environment, and are therefore in
some danger
> of screwing up by using out-of-date environments.

> But my spidey-sense is tingling wrt the provisioner. That too needs an
> up-to-date environment, and we've tied ourselves in frightful knots
over it;
> and, really, I don't think we need to at all. An Environ is meant to
be
> goroutine-safe, after all, so it might be reasonable to get a single
Environ per
> (authorized) agent and run a task that keeps it updated via the API by
> validating changes and calling SetConfig; we can then use that single
task to
> keep all the environs in sync.

> (I'm thinking of a Task that also implements several one-method
interfaces --
> WaitPrechecker, WaitInstanceBroker, WaitPortOpenCloseThing -- that all
share an
> implementation (return the single Environ held by the task once it's
available);
> and which just watches the API and does the whole SetConfig lark.)

> Can anyone think of a reason not to do this?

That sounds sensible to me, though I don't know the finer points of the
machine agent.

I lbox proposed with -wip, so this wouldn't have circulated; "anyone" is
just me at the moment.

https://codereview.appspot.com/14032043/

Reply
Revision history for this message
Andrew Wilkins (axwalk) wrote :

Reviewers: mp+188001_code.launchpad.net, fwereade, axw1,

Message:
Please take a look.

Description:
Wire up prechecker

This is (currently) a POC only.

https://code.launchpad.net/~axwalk/juju-core/wire-up-prechecker/+merge/188001

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/14032043/

Affected files (+204, -42 lines):
   A [revision details]
   M cmd/juju/addmachine.go
   M cmd/jujud/agent.go
   M cmd/jujud/machine.go
   M environs/interface.go
   M environs/jujutest/livetests.go
   M juju/conn.go
   M provider/azure/environ.go
   M provider/ec2/ec2.go
   M provider/ec2/local_test.go
   M provider/local/environ.go
   M provider/local/environ_test.go
   M provider/null/environ.go
   M provider/openstack/local_test.go
   M provider/openstack/provider.go
   M state/open.go
   M state/state.go
   M state/state_test.go

Reply
Revision history for this message
William Reade (fwereade) wrote :
Download full text (3.8 KiB)

I feel like we may be converging on something... let me know what you
think

https://codereview.appspot.com/14032043/diff/1/state/state.go
File state/state.go (right):

https://codereview.appspot.com/14032043/diff/1/state/state.go#newcode292
state/state.go:292: // Prechecker is a clone of environs.Prechecker,
here to avoid the dependency.
On 2013/09/29 16:20:44, axw1 wrote:
> On 2013/09/27 10:59:22, fwereade wrote:
> > Better I think to define it in state, and document (in code) that
 appropriate
> > Environs implement it. environs already knows about state, not vice
 versa, so
> we
> > may as well just define the interface in the place that uses it.
 After all, it
> > really *doesn't* have to be just an environ that satsisfies that
 interface:
> we'd
> > surely want the state tests to be written with a suitable test
 implementation,
> > not of a whole environ.

> Fair enough, I'll do that. Do you think it'd be worthwhile adding
something like
> below to environs/interface.go?

> // Prechecker is an optional Environ interface. See state.Prechecker
for
> details.
> type Prechecker interface {
> state.Prechecker
> }

I think rog and nate have convinced me that these optional interfaces
may do more harm than good -- we can add state.Prechecker directly to
the Environ interface, and just supply an embeddable do-nothing
prechecker that people can implement to begin with.

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

https://codereview.appspot.com/14032043/diff/6001/cmd/juju/addmachine.go#newcode98
cmd/juju/addmachine.go:98: conn.State.SetPrechecker(nil)
This feels like a timebomb to me. What happens when this functionality
moves into the API server? One manually provisioned machine will disable
prechecking for everything.

I think we should probably be skipping prechecks on injected machines
anyway.

https://codereview.appspot.com/14032043/diff/6001/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/14032043/diff/6001/environs/interface.go#newcode105
environs/interface.go:105: type Environ interface {
yeah, it seems sane to stick state.Prechecker in here and embed a shared
nil-returning implementation in the various implementations. Reasonable?

https://codereview.appspot.com/14032043/diff/6001/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/14032043/diff/6001/juju/conn.go#newcode89
juju/conn.go:89: if prechecker, ok := environ.(state.Prechecker); ok {
It's that old wrong-environ problem again, I'm afraid. I'd reuse that
utility function from jujud myself -- I think it's basically
State.EnablePrechecking, or something like that, with the complication
that state doesn't know about environs and so we can't implement it
in-package.

Hmm:

type PrecheckerSource interface {
     GetPrechecker(st *State) (Prechecker, error)
}

how often do we directly Open state? often enough that supplying one of
those would be an inconvenience? Because if we could do that, we could
drop the locking in here, I think, and rely on safe/efficient external
implementations of GetPrechecker.

https://codereview.appspot.com/14032043/diff/6001/state...

Read more...

Reply
Revision history for this message
Andrew Wilkins (axwalk) wrote :
Download full text (3.4 KiB)

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

https://codereview.appspot.com/14032043/diff/6001/cmd/juju/addmachine.go#newcode98
cmd/juju/addmachine.go:98: conn.State.SetPrechecker(nil)
On 2013/10/01 10:24:33, fwereade wrote:
> This feels like a timebomb to me. What happens when this functionality
 moves
> into the API server? One manually provisioned machine will disable
 prechecking
> for everything.

> I think we should probably be skipping prechecks on injected machines
anyway.

Removed, and updated state to not call PrecheckInstance for injected
machines.

https://codereview.appspot.com/14032043/diff/6001/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/14032043/diff/6001/environs/interface.go#newcode105
environs/interface.go:105: type Environ interface {
On 2013/10/01 10:24:33, fwereade wrote:
> yeah, it seems sane to stick state.Prechecker in here and embed a
 shared
> nil-returning implementation in the various implementations.
 Reasonable?

Very. I've created a new file, base.go, which contains a default
implementation.

https://codereview.appspot.com/14032043/diff/6001/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/14032043/diff/6001/juju/conn.go#newcode89
juju/conn.go:89: if prechecker, ok := environ.(state.Prechecker); ok {
On 2013/10/01 10:24:33, fwereade wrote:
> It's that old wrong-environ problem again, I'm afraid.

So, my intention here was that the user would later update the environ.
But actually, thinking about it some more, this is just wrong. NewConn
shouldn't call SetPrechecker. If the user wants to do that, they can.

The one user that perhaps should SetPrechecker is NewConnFromName. Is
there any reason why we don't have it always set the environ's config
from state?

> I'd reuse that utility
> function from jujud myself -- I think it's basically
State.EnablePrechecking, or
> something like that, with the complication that state doesn't know
about
> environs and so we can't implement it in-package.

> Hmm:

> type PrecheckerSource interface {
> GetPrechecker(st *State) (Prechecker, error)
> }

> how often do we directly Open state? often enough that supplying one
of those
> would be an inconvenience? Because if we could do that, we could drop
the
> locking in here, I think, and rely on safe/efficient external
implementations of
> GetPrechecker.

https://codereview.appspot.com/14032043/diff/6001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/14032043/diff/6001/state/state.go#newcode70
state/state.go:70: precheckerMutex sync.Mutex
On 2013/10/01 10:24:33, fwereade wrote:
> RWMutex might be appropriate here?

Done.

https://codereview.appspot.com/14032043/diff/6001/state/state.go#newcode233
state/state.go:233: // we are creating a new machine instance (not a
container).
On 2013/10/01 10:24:33, fwereade wrote:
> we should be locking these bits as well, surely?

Indeed, thanks for picking that up. Done.

https://codereview.appspot.com/14032043/diff/6001/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/14032043/diff/6001/state/st...

Read more...

Reply
lp://staging/~axwalk/juju-core/wire-up-prechecker updated
1900. By Andrew Wilkins

Address review comments

- Add Prechecker into environs.Environ,
  introduce NilPrechecker for embedding
- Remove SetPrechecker from juju.NewConn
- Don't precheck instances for injected machines
- Don't set prechecker to nil for manual provisioning
- Learn to use mutexes properly ;)

Revision history for this message
Andrew Wilkins (axwalk) wrote :

Please take a look.

https://codereview.appspot.com/14032043/

Reply
lp://staging/~axwalk/juju-core/wire-up-prechecker updated
1901. By Andrew Wilkins

juju.NewConn: call SetPrechecker

But do it with an Environ whose
config was loaded from state.

Revision history for this message
Andrew Wilkins (axwalk) wrote :

Please take a look.

https://codereview.appspot.com/14032043/

Reply
Revision history for this message
Andrew Wilkins (axwalk) wrote :

On 2013/10/04 03:40:57, axw wrote:
> Please take a look.

Updated juju/conn.go to call SetPrechecker again, but with a fresh
Environ whose config is loaded from state.

I'll start on a new CL now for adding a task that updates an Environ,
and encapsulates worker.WaitForEnviron.

https://codereview.appspot.com/14032043/

Reply
Revision history for this message
William Reade (fwereade) wrote :

Sorry this one has hung around so long, but I think it's basically
there. Last round?

https://codereview.appspot.com/14032043/diff/19001/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/14032043/diff/19001/juju/conn.go#newcode171
juju/conn.go:171: func (c *Conn) setPrechecker() error {
Good point in your comment that I think was around here. Let's use this
to set the environ on the Conn itself -- I can't think of a good reason
not to tbh.

https://codereview.appspot.com/14032043/diff/19001/provider/azure/environ.go
File provider/azure/environ.go (right):

https://codereview.appspot.com/14032043/diff/19001/provider/azure/environ.go#newcode83
provider/azure/environ.go:83: var _ state.Prechecker =
(*azureEnviron)(nil)
Don't think we need this now it's in the Environ interface

https://codereview.appspot.com/14032043/diff/19001/provider/ec2/ec2.go
File provider/ec2/ec2.go (right):

https://codereview.appspot.com/14032043/diff/19001/provider/ec2/ec2.go#newcode69
provider/ec2/ec2.go:69: var _ state.Prechecker = (*environ)(nil)
ditto

https://codereview.appspot.com/14032043/diff/19001/provider/ec2/local_test.go
File provider/ec2/local_test.go (right):

https://codereview.appspot.com/14032043/diff/19001/provider/ec2/local_test.go#newcode196
provider/ec2/local_test.go:196: prechecker, ok := env.(state.Prechecker)
unnecessary again?

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

https://codereview.appspot.com/14032043/diff/19001/provider/null/environ.go#newcode165
provider/null/environ.go:165: func (*nullEnviron)
PrecheckContainer(series string, kind instance.ContainerType) error {
Hmm. Unless we can give containers their own addresses, we might need to
error out here too.

https://codereview.appspot.com/14032043/diff/19001/provider/openstack/local_test.go
File provider/openstack/local_test.go (right):

https://codereview.appspot.com/14032043/diff/19001/provider/openstack/local_test.go#newcode251
provider/openstack/local_test.go:251: prechecker, ok :=
env.(state.Prechecker)
again not necessary I think

https://codereview.appspot.com/14032043/diff/19001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/14032043/diff/19001/state/state.go#newcode236
state/state.go:236: // in which case we do not want to precheck it.
Is this really the best way to get it? Seems a little bit roundabout.
I'd prefer to use InstanceId, really, it seems a bit clearer to me.

https://codereview.appspot.com/14032043/

Reply
Revision history for this message
Andrew Wilkins (axwalk) wrote :

Thanks for the review.

https://codereview.appspot.com/14032043/diff/19001/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/14032043/diff/19001/juju/conn.go#newcode171
juju/conn.go:171: func (c *Conn) setPrechecker() error {
On 2013/10/08 07:30:42, fwereade wrote:
> Good point in your comment that I think was around here. Let's use
 this to set
> the environ on the Conn itself -- I can't think of a good reason not
 to tbh.

Done. It's slightly more complicated than getting the env from state: it
needs to have the admin-secret and ca-private-key copied across from the
input environ.

https://codereview.appspot.com/14032043/diff/19001/provider/azure/environ.go
File provider/azure/environ.go (right):

https://codereview.appspot.com/14032043/diff/19001/provider/azure/environ.go#newcode83
provider/azure/environ.go:83: var _ state.Prechecker =
(*azureEnviron)(nil)
On 2013/10/08 07:30:42, fwereade wrote:
> Don't think we need this now it's in the Environ interface

Done.

https://codereview.appspot.com/14032043/diff/19001/provider/ec2/ec2.go
File provider/ec2/ec2.go (right):

https://codereview.appspot.com/14032043/diff/19001/provider/ec2/ec2.go#newcode69
provider/ec2/ec2.go:69: var _ state.Prechecker = (*environ)(nil)
On 2013/10/08 07:30:42, fwereade wrote:
> ditto

Done.

https://codereview.appspot.com/14032043/diff/19001/provider/ec2/local_test.go
File provider/ec2/local_test.go (right):

https://codereview.appspot.com/14032043/diff/19001/provider/ec2/local_test.go#newcode196
provider/ec2/local_test.go:196: prechecker, ok := env.(state.Prechecker)
On 2013/10/08 07:30:42, fwereade wrote:
> unnecessary again?

Done.

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

https://codereview.appspot.com/14032043/diff/19001/provider/null/environ.go#newcode165
provider/null/environ.go:165: func (*nullEnviron)
PrecheckContainer(series string, kind instance.ContainerType) error {
On 2013/10/08 07:30:42, fwereade wrote:
> Hmm. Unless we can give containers their own addresses, we might need
 to error
> out here too.

Done.

https://codereview.appspot.com/14032043/diff/19001/provider/openstack/local_test.go
File provider/openstack/local_test.go (right):

https://codereview.appspot.com/14032043/diff/19001/provider/openstack/local_test.go#newcode251
provider/openstack/local_test.go:251: prechecker, ok :=
env.(state.Prechecker)
On 2013/10/08 07:30:42, fwereade wrote:
> again not necessary I think

Done.

https://codereview.appspot.com/14032043/diff/19001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/14032043/diff/19001/state/state.go#newcode236
state/state.go:236: // in which case we do not want to precheck it.
On 2013/10/08 07:30:42, fwereade wrote:
> Is this really the best way to get it? Seems a little bit roundabout.
 I'd prefer
> to use InstanceId, really, it seems a bit clearer to me.

I think it is. InstanceId is deprecated on machineDoc (and it's inside
metadata, but it's nil...)

https://codereview.appspot.com/14032043/

Reply
lp://staging/~axwalk/juju-core/wire-up-prechecker updated
1902. By Andrew Wilkins

juju: update Conn.Environ in NewConn

Always return a fresh, up-to-date Environ.

1903. By Andrew Wilkins

Address review comments

Change juju.NewConn to always return a fresh Environ,
and use that as the Prechecker.

Revision history for this message
Andrew Wilkins (axwalk) wrote :

Please take a look.

https://codereview.appspot.com/14032043/

Reply
Revision history for this message
William Reade (fwereade) wrote :

Apart from the extra attrs, this seems fine -- but they could use a spot
more investigation.

https://codereview.appspot.com/14032043/diff/19001/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/14032043/diff/19001/juju/conn.go#newcode171
juju/conn.go:171: func (c *Conn) setPrechecker() error {
On 2013/10/08 09:52:34, axw wrote:
> On 2013/10/08 07:30:42, fwereade wrote:
> > Good point in your comment that I think was around here. Let's use
 this to set
> > the environ on the Conn itself -- I can't think of a good reason not
 to tbh.

> Done. It's slightly more complicated than getting the env from state:
it needs
> to have the admin-secret and ca-private-key copied across from the
input
> environ.

/me has a suspicious. Shouldn't we do it after updating secrets?

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

https://codereview.appspot.com/14032043/diff/30001/cmd/jujud/machine.go#newcode245
cmd/jujud/machine.go:245: // TODO(axw) 2013-10-01 bug #pending
needs a bug# now :)

https://codereview.appspot.com/14032043/diff/30001/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/14032043/diff/30001/juju/conn.go#newcode106
juju/conn.go:106: "ca-private-key": string(caPrivateKey),
Yeah, I don't *think* we should need these.

https://codereview.appspot.com/14032043/

Reply
lp://staging/~axwalk/juju-core/wire-up-prechecker updated
1904. By Andrew Wilkins

Address review comments

- Remove unnecessary admin-secret/ca-private-key copy.
- Log/add bug number to TODO in cmd/jujud.

Revision history for this message
Andrew Wilkins (axwalk) wrote :

Please take a look.

https://codereview.appspot.com/14032043/

Reply
Revision history for this message
William Reade (fwereade) wrote :

Sorry, this lingered so long that I've lost context, so the comments
below may be a bit dumb. On the upside, I've heard surprisingly few
screams about its absence, so I feel we're a bit more free to think
about the approach a bit more.

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

https://codereview.appspot.com/14032043/diff/37001/cmd/jujud/machine.go#newcode249
cmd/jujud/machine.go:249: environ, err := getStateEnviron(st)
Remind me -- how does this work when the environ config is incomplete?
Don't we still need a state worker then?

https://codereview.appspot.com/14032043/diff/37001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/14032043/diff/37001/state/state.go#newcode257
state/state.go:257: }
OK, Kapil's concerns are getting to me a little bit. Gut feeling: is it
sane/easy/reasonable to allow container=lxc (which we *really* should
name isolation=lxc) constraints to create unroutable containers, but to
restrict add-machine so it only allows addressable ones?

(if we implemented lxc:X handling with a pseudo-provider maybe *that*
could be the bit responsible for container checking?)

https://codereview.appspot.com/14032043/

Reply
Revision history for this message
William Reade (fwereade) wrote :

WIP for a spot more discussion. Sorry this branch is such a hassle.

Reply
Revision history for this message
Andrew Wilkins (axwalk) wrote :

May want to hold off until synchronous bootstrap now, given the
oversight on getting an environ.

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

https://codereview.appspot.com/14032043/diff/37001/cmd/jujud/machine.go#newcode249
cmd/jujud/machine.go:249: environ, err := getStateEnviron(st)
On 2013/11/07 09:44:53, fwereade wrote:
> Remind me -- how does this work when the environ config is incomplete?
 Don't we
> still need a state worker then?

Well, oops, I don't think it does work.

I must have coded this against a version of the manual provider that
couldn't have an incomplete config (there was a time when it had
defaults for everything).

This would change after synchronous bootstrap, since the agent will
never have an incomplete config. Doesn't work for now, though.

https://codereview.appspot.com/14032043/diff/37001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/14032043/diff/37001/state/state.go#newcode257
state/state.go:257: }
On 2013/11/07 09:44:53, fwereade wrote:
> OK, Kapil's concerns are getting to me a little bit. Gut feeling: is
 it
> sane/easy/reasonable to allow container=lxc (which we *really* should
 name
> isolation=lxc) constraints to create unroutable containers, but to
 restrict
> add-machine so it only allows addressable ones?

It's probably doable, but that sounds a bit hackish to me. I'd rather
just be explicit about requiring (at worst) an unroutable container.

> (if we implemented lxc:X handling with a pseudo-provider maybe *that*
could be
> the bit responsible for container checking?)

https://codereview.appspot.com/14032043/

Reply

Unmerged revisions

1904. By Andrew Wilkins

Address review comments

- Remove unnecessary admin-secret/ca-private-key copy.
- Log/add bug number to TODO in cmd/jujud.

1903. By Andrew Wilkins

Address review comments

Change juju.NewConn to always return a fresh Environ,
and use that as the Prechecker.

1902. By Andrew Wilkins

juju: update Conn.Environ in NewConn

Always return a fresh, up-to-date Environ.

1901. By Andrew Wilkins

juju.NewConn: call SetPrechecker

But do it with an Environ whose
config was loaded from state.

1900. By Andrew Wilkins

Address review comments

- Add Prechecker into environs.Environ,
  introduce NilPrechecker for embedding
- Remove SetPrechecker from juju.NewConn
- Don't precheck instances for injected machines
- Don't set prechecker to nil for manual provisioning
- Learn to use mutexes properly ;)

1899. By Andrew Wilkins

Address review comments

- Move Prechecker from environs to state.
- Add "var _" assertions for Prechecker implementation.
- Acquire Mutex in SetPrechecker.
- Move Environ loaded from state into StateWorker in cmd/jujud.

1898. By Andrew Wilkins

Wire up Prechecker

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to status/vote changes: