Code review comment for lp:~axwalk/juju-core/addmachine-placement
Revision history for this message
| Andrew Wilkins (axwalk) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
a60fb26
(Get the code!)
Please take a look.
https:/
File cmd/juju/
https:/
cmd/juju/
client.
On 2014/04/09 05:20:22, axw wrote:
> On 2014/04/09 04:52:18, wallyworld wrote:
> > What about backwards compatibility?
> Yeah, I suppose we will need to continue setting ContainerType and
ParentId for
> backwards compatibility.
Done.
https:/
cmd/juju/
whether it's a container?
On 2014/04/09 05:20:22, axw wrote:
> On 2014/04/09 04:52:18, wallyworld wrote:
> > Can't we inspect the scope to see if it's a valid container type?
> Yes indeed. I had previously implemented things differently, but with
the
> current implementation it's trivial to do so.
Done.
https:/
File environs/
https:/
environs/
On 2014/04/11 12:27:11, fwereade wrote:
> I'm not clear on how this is distinct from Prechecker -- isn't
placement just
> another StartInstance parameter?
My intention was for PlacementValidator to be used for validating
add-unit/deploy placement too. I suppose those can just be handled by a
unit-placement policy.
> Food for thought and maybe future implementation: should the
prechecker perhaps
> return something that implements InstanceBroker? that'd keep
what-we-ask-for and
> what-we-check in sync...
Sounds like a decent idea, but I'd rather keep this CL to a minimum.
https:/
File instance/
https:/
instance/
isContainerType
On 2014/04/11 12:27:11, fwereade wrote:
> Hmm. Problem coming: environments named after a container type will
act in a
> surprising way. Can you think of any way around this?
Our current placement directives currently define the lxc, kvm, ssh and
machine-id special cases. Can't do anything about this without breaking
that existing format.
In the initial implementation we won't be able to handle environment
placement directives for an environment named "lxc" or "kvm". We *could*
add special casing so that it looks for "env:lxc:" or something, but
really I don't think it's worth it.
https:/
instance/
On 2014/04/11 12:27:11, fwereade wrote:
> I'm not following this case. `add-machine --to kvm`? I think we need a
target
> machine for this to make sense.
It means "add a kvm container on a new machine"; see the bottom of
state/apiserver
https:/
instance/
On 2014/04/11 12:27:11, fwereade wrote:
> I think I'd prefer an explict ErrScopeUnclear, and explicit handling
thereof in
> the client.
Done.
https:/
instance/
specified string and create a
On 2014/04/11 12:27:11, fwereade wrote:
> s/Parse/MustParse/
Done.
https:/
File instance/
https:/
instance/
instance.
On 2014/04/09 05:20:22, axw wrote:
> On 2014/04/09 04:52:18, wallyworld wrote:
> > Perhaps to reduce the verbosity, the test struct can have
expectScope and
> > expectDirective and the test would use those individual values to
construct a
> > Placement to compare with?
> Sure. There's a special case: empty scope and value == nil placement.
Done.
https:/
File provider/
https:/
provider/
directive: %s", p)
On 2014/04/11 12:27:11, fwereade wrote:
> Do we want a whole Placement here, or should we maybe just handle the
directive
> alone?
Just the directive I suppose. I was thinking we might want to handle
provider-type vs. environment name, but it's probably not a sensible
distinction.
https:/
File state/api/
https:/
state/api/
gc.ErrorMatches, "machine 1 not found")
On 2014/04/11 12:27:11, fwereade wrote:
> On 2014/04/09 04:52:18, wallyworld wrote:
> > We have started returning perm denied if a machine is not found.
> > We should have tests for genuine perm denied cases as well?
> The distinction is whether or not the client ought to be able to
access it. But
> we also have a slight tendency to be lazy, and to ErrPerm in *all*
cases of
> NotFound; I don't entirely love this, but often it simplifies all the
related
> code enough that it's a win regardless.
> In particular, though, I think we should ErrPerm for containers not on
the
> machine running this provisioner. (ideally we'd ErrPerm for *all*
containers in
> the case of an environ-
making
> that distinction)
Placement follows the same authorization rules as other provisioner API,
which is essentially what you've described.
Added some tests to apiserver/
https:/
File state/apiserver
https:/
state/apiserver
c.api.state.
On 2014/04/11 12:27:11, fwereade wrote:
> Based on this, I reckon placement certainly ought to be part of
MachineTemplate.
Done.
https:/
File state/machine.go (right):
https:/
state/machine.
On 2014/04/11 12:27:11, fwereade wrote:
> Hmm. I think scope is redundant in here, too. If it's a container,
it's encoded
> in the id; if it's not, it's *currently* implicit -- but long-term, it
would
> probably be best to make it an explicit "Environ"... except our
terminology is
> all messed up, because of the implicit 1:1 between juju environments
and
> envirns.Environ objects. Grr.
> So: I'd most like to see machines explicitly tagged with their
environ, but I'm
> not sure it's a good idea. Shall we run with provider-
for now?
> (and skip it for containers, because it's already been handled?)
Okay.
https:/