Merge lp:~dimitern/juju-core/394-network-name-to-id-and-add-tags into lp:~go-bot/juju-core/trunk
- 394-network-name-to-id-and-add-tags
- Merge into trunk
| Status: | Merged |
|---|---|
| Approved by: | Dimiter Naydenov |
| Approved revision: | no longer in the source branch. |
| Merged at revision: | 2621 |
| Proposed branch: | lp:~dimitern/juju-core/394-network-name-to-id-and-add-tags |
| Merge into: | lp:~go-bot/juju-core/trunk |
| Prerequisite: | lp:~dimitern/juju-core/393-provisioner-allow-adding-networks |
| Diff against target: |
1769 lines (+653/-226) 33 files modified
cmd/juju/deploy.go (+2/-2) environs/broker.go (+19/-4) errors/errors.go (+4/-5) errors/errors_test.go (+9/-0) names/network.go (+24/-0) names/network_test.go (+52/-0) names/tag.go (+3/-0) names/tag_test.go (+13/-1) provider/dummy/environs.go (+6/-1) provider/maas/environ.go (+3/-2) state/addmachine.go (+4/-4) state/address.go (+9/-2) state/api/client.go (+3/-2) state/api/params/internal.go (+25/-9) state/api/provisioner/machine.go (+2/-2) state/api/provisioner/provisioner_test.go (+29/-20) state/apiserver/client/client.go (+25/-3) state/apiserver/client/client_test.go (+10/-1) state/apiserver/provisioner/provisioner.go (+51/-7) state/apiserver/provisioner/provisioner_test.go (+30/-22) state/apiserver/uniter/uniter_test.go (+3/-3) state/apiserver/usermanager/usermanager_test.go (+2/-2) state/machine.go (+29/-19) state/machine_test.go (+85/-52) state/networkinterfaces.go (+35/-3) state/networkinterfaces_test.go (+3/-1) state/networks.go (+32/-5) state/networks_test.go (+4/-2) state/open.go (+13/-10) state/state.go (+41/-16) state/state_test.go (+69/-18) worker/provisioner/provisioner_task.go (+8/-6) worker/provisioner/provisioner_test.go (+6/-2) |
| To merge this branch: | bzr merge lp:~dimitern/juju-core/394-network-name-to-id-and-add-tags |
| Related bugs: | |
| Related blueprints: |
Support MaaS VLANs in Juju
(Essential)
|
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Dimiter Naydenov (community) | Approve | ||
|
Review via email:
|
|||
Commit message
various: Add network tags from names; provider id
Fixes #1304905: Networks now have juju names used to
identify them, and construct tags from that for the
API, provider-specific id (currently the same as the
name, but is a distinct field).
Comments updated to better explain the difference
between juju-specific network names and provider-
specific ones (at present both are the same, but
this will change).
Few drive-by fixes are included as well.
https:/
R=fwereade
Description of the change
various: Add network tags from names; provider id
Fixes #1304905: Networks now have juju names used to
identify them, and construct tags from that for the
API, provider-specific id (currently the same as the
name, but is a distinct field).
Comments updated to better explain the difference
between juju-specific network names and provider-
specific ones (at present both are the same, but
this will change).
Few drive-by fixes are included as well.
| Dimiter Naydenov (dimitern) wrote : | # |
| Dimiter Naydenov (dimitern) wrote : | # |
Please take a look.
| William Reade (fwereade) wrote : | # |
At least one of us is confused... possibly both. Hopefully these
comments will elucidate where I'm coming from; ping me for a chat if you
need to.
https:/
File names/network_
https:/
names/network_
Let's not let users do this. Same restrictions as service names perhaps?
https:/
File names/tag_test.go (left):
https:/
names/tag_
Tag functions.
Thanks
https:/
File state/api/
https:/
state/api/
Hmm, doesn't this need both the tag and the id?
https:/
File state/apiserver
https:/
state/apiserver
networkTagsToId
tags to names, not ids, surely?
I've been thinking of names as juju vocab, ids as provider vocab, and
tags as derived from names. Am I confused?
https:/
File state/apiserver
https:/
state/apiserver
to tags before returning.
Doesn't this need to be ids? They're for consumption by the provider,
right? Or will the provisioner be using the tags to look up the networks
and get the ids to pass on to StartInstance?
https:/
File state/networkin
https:/
state/networkin
This is an internal reference to a juju entity. Shouldn't it be using
the name?
https:/
File state/networks.go (right):
https:/
state/networks.
It might make things clearer if we had an explicit Name() func here that
just happened to return the provider id?
Would be clearest of all tbh of we had separate Name and ProviderId
fields...
https:/
File state/state.go (right):
https:/
state/state.
like for machines. For
huh? surely names need to be user-specified?
| Dimiter Naydenov (dimitern) wrote : | # |
Please take a look.
https:/
File names/network_
https:/
names/network_
On 2014/04/09 15:34:48, fwereade wrote:
> Let's not let users do this. Same restrictions as service names
perhaps?
This is now changed, as we have two separate fields for Name and
ProviderId, the former is validated more strictly (alphanumeric with
only dashes allowed as separators, like in MAAS, but MAAS also allows
"---" or "a--b-", which I decided to forbid).
https:/
File state/api/
https:/
state/api/
On 2014/04/09 15:34:48, fwereade wrote:
> Hmm, doesn't this need both the tag and the id?
Added ProviderId as well.
https:/
File state/apiserver
https:/
state/apiserver
networkTagsToId
On 2014/04/09 15:34:48, fwereade wrote:
> tags to names, not ids, surely?
> I've been thinking of names as juju vocab, ids as provider vocab, and
tags as
> derived from names. Am I confused?
OK, now I got you. I'll add name and id (both the same for now -
provider-specific until we change that) to networks doc and other
places.
https:/
File state/apiserver
https:/
state/apiserver
to tags before returning.
On 2014/04/09 15:34:48, fwereade wrote:
> Doesn't this need to be ids? They're for consumption by the provider,
right? Or
> will the provisioner be using the tags to look up the networks and get
the ids
> to pass on to StartInstance?
Dropped that and added comment. Since networks do not exist before
provisioning, we can't really look up the names in
include/
just returning names here.
https:/
File state/networkin
https:/
state/networkin
On 2014/04/09 15:34:48, fwereade wrote:
> This is an internal reference to a juju entity. Shouldn't it be using
the name?
Fair enough, NetworkName is the juju entity id, will use it here.
NetworkId is provider-specific (but for now both are the same), and tag
is created from the name.
https:/
File state/networks.go (right):
| Roger Peppe (rogpeppe) wrote : | # |
Looks good in general with a bunch of comments and questions. I'd like
it if someone with better knowledge of what we're actually trying to do
here could review this.
https:/
File environs/broker.go (right):
https:/
environs/
is a provider-specific
This extra sentence is unnecessary - it's already mentioned in the doc
comment for the field.
https:/
environs/
Perhaps document the format of this e.g.
// CIDR of network, in 123.45.67.89/24 format.
and for MACAddress above too (does it contain colons or not?)
https:/
environs/
Is this specified by the provider or juju?
https:/
environs/
Is this the device interface name?
https:/
File errors/errors.go (right):
https:/
errors/
strings.
why?
this seems wrong to me.
https:/
errors/
strings.
same here.
https:/
File names/network.go (right):
https:/
names/network.
network name.
s/returns/reports/
https:/
File names/tag.go (right):
https:/
names/tag.go:18: NetworkTagKind = "network"
s/network/net/ ?
https:/
File provider/
https:/
provider/
perhaps make this an invalid, but well formed CIDR, e.g. 0.1.2.3/0
https:/
File provider/
https:/
provider/
interface %q: missing network information", info.InterfaceName)
I wonder if it would be nice to log the value of info here, so we can
see precisely which network information is lost.
https:/
provider/
missing network interface information", info.NetworkId)
ditto
| William Reade (fwereade) wrote : | # |
Looking very good, but I think the txn bits need a bit of work; and I
think the NotFound changes are bad. What's the motivation there?
https:/
File environs/broker.go (right):
https:/
environs/
If we're commenting fields (+1 to that) let's have blank lines before
comments.
https:/
File errors/errors.go (right):
https:/
errors/
strings.
On 2014/04/10 15:48:10, rog wrote:
> why?
> this seems wrong to me.
Yeah, I'm not too keen on this myself.
https:/
errors/
strings.
On 2014/04/10 15:48:10, rog wrote:
> same here.
Yeah, I has a scared.
https:/
File state/address.go (right):
https:/
state/address.
`bson:",omitempty"`
hmm. Scope should really be on Network, shouldn't it...
That's probably not one for this CL, though.
https:/
state/address.
`bson:",omitempty"`
ditto
https:/
File state/apiserver
https:/
state/apiserver
state.NetworkPa
I'm wondering whether NetworkInfo is less overloaded -- the params
package confuses the issue a bit
https:/
File state/machine.go (right):
https:/
state/machine.
s/available on an instance/ I think -- and move these to networks.go?
https:/
state/machine.
On 2014/04/10 15:48:10, rog wrote:
> similar question to State.AddNetwork, why are we iterating here?
We shouldn't be iterating like this anyway, I just realised -- if
there's any reason to iterate, there's reason to reread state inside the
loop.
https:/
state/machine.
mgo, and
On 2014/04/10 15:48:10, rog wrote:
> The only output from a transaction is ErrAborted - we never get to
know whether
> the actual operations succeeded or not (because they might have been
executed on
> another machine).
> BTW if we asserted that the doc was missing, how could the insertion
fail?
Yeah, nil means ...
| Dimiter Naydenov (dimitern) wrote : | # |
Please take a look.
https:/
File environs/broker.go (right):
https:/
environs/
is a provider-specific
On 2014/04/10 15:48:10, rog wrote:
> This extra sentence is unnecessary - it's already mentioned in the doc
comment
> for the field.
I wanted to be extra clear. Removed.
https:/
environs/
On 2014/04/10 15:48:10, rog wrote:
> Perhaps document the format of this e.g.
> // CIDR of network, in 123.45.67.89/24 format.
> and for MACAddress above too (does it contain colons or not?)
Done.
https:/
environs/
On 2014/04/11 11:09:36, fwereade wrote:
> If we're commenting fields (+1 to that) let's have blank lines before
comments.
Done.
https:/
environs/
On 2014/04/10 15:48:10, rog wrote:
> Is this specified by the provider or juju?
For now, the provider, in the future possibly juju as well.
https:/
environs/
On 2014/04/10 15:48:10, rog wrote:
> Is this the device interface name?
Yes, added comment.
https:/
File errors/errors.go (right):
https:/
errors/
strings.
On 2014/04/11 11:09:36, fwereade wrote:
> On 2014/04/10 15:48:10, rog wrote:
> > why?
> > this seems wrong to me.
> Yeah, I'm not too keen on this myself.
Because of ErrorContextf and wrapping errors. It does work like this and
survives the wrapping.
https:/
errors/
strings.
On 2014/04/11 11:09:36, fwereade wrote:
> On 2014/04/10 15:48:10, rog wrote:
> > same here.
> Yeah, I has a scared.
See above.
https:/
File names/network.go (right):
https:/
names/network.
network name.
On 2014/04/10 15:48:10, rog wrote:
> s/returns/reports/
Done.
https:/
File names/tag.go (right):
https:/
names/tag.go:18: NetworkTagKind = "network"
On 2014/04/10 15:48:10, rog wrote:
> s/network/net/ ?
Why? None of the other tags kinds are abbreviated.
https:/
File provider/
https:/
| William Reade (fwereade) wrote : | # |
would be nice to s/Params/Info/ as discussed live; otherwise LGTM
| Dimiter Naydenov (dimitern) wrote : | # |
Please take a look.
| Go Bot (go-bot) wrote : | # |
The attempt to merge lp:~dimitern/juju-core/394-network-name-to-id-and-add-tags into lp:juju-core failed. Below is the output from the failed tests.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
? launchpad.
? launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
| Go Bot (go-bot) wrote : | # |
The attempt to merge lp:~dimitern/juju-core/394-network-name-to-id-and-add-tags into lp:juju-core failed. Below is the output from the failed tests.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
? launchpad.
? launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
| Go Bot (go-bot) wrote : | # |
The attempt to merge lp:~dimitern/juju-core/394-network-name-to-id-and-add-tags into lp:juju-core failed. Below is the output from the failed tests.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
? launchpad.
? launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
| Go Bot (go-bot) wrote : | # |
The attempt to merge lp:~dimitern/juju-core/394-network-name-to-id-and-add-tags into lp:juju-core failed. Below is the output from the failed tests.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
? launchpad.
? launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
| Go Bot (go-bot) wrote : | # |
The attempt to merge lp:~dimitern/juju-core/394-network-name-to-id-and-add-tags into lp:juju-core failed. Below is the output from the failed tests.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
? launchpad.
? launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
| Go Bot (go-bot) wrote : | # |
The attempt to merge lp:~dimitern/juju-core/394-network-name-to-id-and-add-tags into lp:juju-core failed. Below is the output from the failed tests.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
? launchpad.
? launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
| Go Bot (go-bot) wrote : | # |
The attempt to merge lp:~dimitern/juju-core/394-network-name-to-id-and-add-tags into lp:juju-core failed. Below is the output from the failed tests.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
? launchpad.
? launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
| Go Bot (go-bot) wrote : | # |
The attempt to merge lp:~dimitern/juju-core/394-network-name-to-id-and-add-tags into lp:juju-core failed. Below is the output from the failed tests.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
? launchpad.
? launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
-------
FAIL: storage_
[LOG] 98.41030 DEBUG juju.utils.ssh running: ssh -o "StrictHostKeyC
storage_
c.Assert(err, gc.IsNil)
... value *errors.errorString = &errors.
OOPS: 21 passed, 1 FAILED
---...
| Go Bot (go-bot) wrote : | # |
The attempt to merge lp:~dimitern/juju-core/394-network-name-to-id-and-add-tags into lp:juju-core failed. Below is the output from the failed tests.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
? launchpad.
? launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
-------
FAIL: storage_
[LOG] 49.63328 DEBUG juju.utils.ssh running: ssh -o "StrictHostKeyC
[LOG] 49.63954 DEBUG juju.utils.ssh running: ssh -o "StrictHostKeyC
[LOG] 49.64373 DEBUG juju.environs.
| Dimiter Naydenov (dimitern) wrote : | # |
Self-approving after fixing 2 tests.
| Go Bot (go-bot) wrote : | # |
The attempt to merge lp:~dimitern/juju-core/394-network-name-to-id-and-add-tags into lp:juju-core failed. Below is the output from the failed tests.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
? launchpad.
? launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
| William Reade (fwereade) wrote : | # |
On 2014/04/11 15:14:06, dimitern wrote:
> Please take a look.
Except, wait, we never fixed the errors changes. We should really *not*
be guessing types based on strings; if ErrorContextf is causing trouble,
don't use it -- if you need something similar that explicitly lets
particular error types pass, so be it.
| Dimiter Naydenov (dimitern) wrote : | # |
On 2014/04/13 20:20:55, fwereade wrote:
> On 2014/04/11 15:14:06, dimitern wrote:
> > Please take a look.
> Except, wait, we never fixed the errors changes. We should really
*not* be
> guessing types based on strings; if ErrorContextf is causing trouble,
don't use
> it -- if you need something similar that explicitly lets particular
error types
> pass, so be it.
Right, I'll propose a follow-up to fix that.
| Roger Peppe (rogpeppe) wrote : | # |
Sorry, I know this is already merged, but I still have a few comments.
https:/
File state/machine.go (right):
https:/
state/machine.
interfaceName, networkName string) (iface *NetworkInterface, err error)
{
On 2014/04/11 14:05:04, dimitern wrote:
> On 2014/04/10 15:48:10, rog wrote:
> > It seems a bit odd to me that we're specifying os-specific interface
names (if
> > that's what interfaceName is) in the state, but I guess this has
already been
> > thrashed out already. I guess I'd have expected the machine to
choose its
> > interface names for itself.
> We need the interface names to be OS-specific, because that's how we
can
> discover, refer to, and manage them on the machine.
Is it always the machine itself that calls AddNetworkInter
https:/
state/machine.
On 2014/04/11 14:05:04, dimitern wrote:
> On 2014/04/10 15:48:10, rog wrote:
> > similar question to State.AddNetwork, why are we iterating here?
> See answer above.
I still don't see it. If the transaction fails because the machine
is now dead, how will iterating help matters? (the machine is
still going to be dead no matter how many times we try).
Similarly, a machine cannot be unprovisioned once it's provisioned.
Why are we iterating, again?
https:/
state/machine.
mgo, and
The comment sounds like we don't know what's going on, but we
do know, so I think the comment could be better.
// We have a unique key restriction on the InterfaceName
// field, which will cause the insert to fail if there
// is another record with the same interface name in the
// table. The txn logic does not report insertion errors,
// so we check that the record has actually been inserted
// correctly before reporting success.
?
https:/
File state/state.go (right):
https:/
state/state.
mgo, and
On 2014/04/11 14:05:04, dimitern wrote:
> On 2014/04/10 15:48:10, rog wrote:
> > Again, how can this fail if the document is missing?
> >
> > Ah, I think I see. You've got a unique index on provider id.
> >
> > Assuming that's the case, the error is misleading here - it's
> > not really that it already exists (we know the name is different)
> > but that there's already another network with a duplicate provider
id.
> The message is `cannot add network "blah": network with provider id
"foo"
> already exists` - how is that unclear? name and providerId are both
unique and
> trying to add a duplicated _id (name) is the same as adding a
duplicated
> provider id. We consider both things primary keys, so the "already
exists" error
> is what we need. Am I missing something?
No, you're right - I'd missed the "p...
| Dimiter Naydenov (dimitern) wrote : | # |
https:/
File state/machine.go (right):
https:/
state/machine.
interfaceName, networkName string) (iface *NetworkInterface, err error)
{
On 2014/04/14 07:52:07, rog wrote:
> On 2014/04/11 14:05:04, dimitern wrote:
> > On 2014/04/10 15:48:10, rog wrote:
> > > It seems a bit odd to me that we're specifying os-specific
interface names
> (if
> > > that's what interfaceName is) in the state, but I guess this has
already
> been
> > > thrashed out already. I guess I'd have expected the machine to
choose its
> > > interface names for itself.
> >
> > We need the interface names to be OS-specific, because that's how we
can
> > discover, refer to, and manage them on the machine.
> Is it always the machine itself that calls AddNetworkInter
The provisioner task that starts the instance for machine m is the one
that calls m.AddNetworkInt
future as we can manage networks independently of provisioning.
https:/
state/machine.
On 2014/04/14 07:52:07, rog wrote:
> On 2014/04/11 14:05:04, dimitern wrote:
> > On 2014/04/10 15:48:10, rog wrote:
> > > similar question to State.AddNetwork, why are we iterating here?
> >
> > See answer above.
> I still don't see it. If the transaction fails because the machine
> is now dead, how will iterating help matters? (the machine is
> still going to be dead no matter how many times we try).
> Similarly, a machine cannot be unprovisioned once it's provisioned.
> Why are we iterating, again?
Fair enough, I'll fix this and remove the loop in a follow-up.
https:/
state/machine.
mgo, and
On 2014/04/14 07:52:07, rog wrote:
> The comment sounds like we don't know what's going on, but we
> do know, so I think the comment could be better.
> // We have a unique key restriction on the InterfaceName
> // field, which will cause the insert to fail if there
> // is another record with the same interface name in the
> // table. The txn logic does not report insertion errors,
> // so we check that the record has actually been inserted
> // correctly before reporting success.
> ?
Done in a follow-up. And yes, it *does* sound like that - this specific
behavior of mgo/txn is not documented anywhere!
https:/
File state/state.go (right):
https:/
state/state.
mgo, and
On 2014/04/14 07:52:07, rog wrote:
> I'd suggest a similar comment here to my suggestion elsewhere.
Done in a follow-up.
Preview Diff
| 1 | === modified file 'cmd/juju/deploy.go' |
| 2 | --- cmd/juju/deploy.go 2014-04-12 05:53:58 +0000 |
| 3 | +++ cmd/juju/deploy.go 2014-04-13 09:27:07 +0000 |
| 4 | @@ -281,7 +281,7 @@ |
| 5 | return curl, nil |
| 6 | } |
| 7 | |
| 8 | -// parseNetworks returns a list of networks by parsing the |
| 9 | +// parseNetworks returns a list of network tags by parsing the |
| 10 | // comma-delimited string value of --networks or --exclude-networks |
| 11 | // arguments. |
| 12 | func parseNetworks(networksValue string) (networks []string) { |
| 13 | @@ -289,7 +289,7 @@ |
| 14 | for _, part := range parts { |
| 15 | network := strings.TrimSpace(part) |
| 16 | if network != "" { |
| 17 | - networks = append(networks, network) |
| 18 | + networks = append(networks, names.NetworkTag(network)) |
| 19 | } |
| 20 | } |
| 21 | return networks |
| 22 | |
| 23 | === modified file 'environs/broker.go' |
| 24 | --- environs/broker.go 2014-04-04 15:55:19 +0000 |
| 25 | +++ environs/broker.go 2014-04-13 09:27:07 +0000 |
| 26 | @@ -37,10 +37,25 @@ |
| 27 | // instance. For providers that support networks, this will be |
| 28 | // available at StartInstance() time. |
| 29 | type NetworkInfo struct { |
| 30 | - MACAddress string |
| 31 | - CIDR string |
| 32 | - NetworkName string |
| 33 | - VLANTag int |
| 34 | + // MACAddress is the network interface's hardware MAC address |
| 35 | + // (e.g. "aa:bb:cc:dd:ee:ff"). |
| 36 | + MACAddress string |
| 37 | + |
| 38 | + // CIDR of the network, in 123.45.67.89/24 format. |
| 39 | + CIDR string |
| 40 | + |
| 41 | + // NetworkName is juju-internal name of the network. |
| 42 | + NetworkName string |
| 43 | + |
| 44 | + // NetworkId is a provider-specific network id. |
| 45 | + NetworkId string |
| 46 | + |
| 47 | + // VLANTag needs to be between 1 and 4094 for VLANs and 0 for |
| 48 | + // normal networks. It's defined by IEEE 802.1Q standard. |
| 49 | + VLANTag int |
| 50 | + |
| 51 | + // InterfaceName is the OS-specific network device name (e.g. |
| 52 | + // "eth0" or "eth1.42" for a VLAN virtual interface). |
| 53 | InterfaceName string |
| 54 | } |
| 55 | |
| 56 | |
| 57 | === modified file 'errors/errors.go' |
| 58 | --- errors/errors.go 2014-04-10 23:12:43 +0000 |
| 59 | +++ errors/errors.go 2014-04-13 09:27:07 +0000 |
| 60 | @@ -5,6 +5,7 @@ |
| 61 | |
| 62 | import ( |
| 63 | "fmt" |
| 64 | + "strings" |
| 65 | ) |
| 66 | |
| 67 | // errorWrapper defines a way to encapsulate an error inside another error. |
| 68 | @@ -33,10 +34,8 @@ |
| 69 | // IsNotFoundError is satisfied by errors created by this package representing |
| 70 | // resources that can't be found. |
| 71 | func IsNotFoundError(err error) bool { |
| 72 | - if _, ok := err.(notFoundError); ok { |
| 73 | - return true |
| 74 | - } |
| 75 | - return false |
| 76 | + _, ok := err.(notFoundError) |
| 77 | + return ok || (err != nil && strings.HasSuffix(err.Error(), " not found")) |
| 78 | } |
| 79 | |
| 80 | // NotFoundf returns a error which satisfies IsNotFoundError(). |
| 81 | @@ -125,7 +124,7 @@ |
| 82 | // was created with NewAlreadyExistsError. |
| 83 | func IsAlreadyExistsError(err error) bool { |
| 84 | _, ok := err.(*alreadyExistsError) |
| 85 | - return ok |
| 86 | + return ok || (err != nil && strings.HasSuffix(err.Error(), " already exists")) |
| 87 | } |
| 88 | |
| 89 | type notSupportedError struct { |
| 90 | |
| 91 | === modified file 'errors/errors_test.go' |
| 92 | --- errors/errors_test.go 2014-04-10 23:12:43 +0000 |
| 93 | +++ errors/errors_test.go 2014-04-13 09:27:07 +0000 |
| 94 | @@ -5,6 +5,7 @@ |
| 95 | |
| 96 | import ( |
| 97 | stderrors "errors" |
| 98 | + "fmt" |
| 99 | "reflect" |
| 100 | "runtime" |
| 101 | "testing" |
| 102 | @@ -75,6 +76,10 @@ |
| 103 | "", |
| 104 | isNotFoundError, |
| 105 | }, { |
| 106 | + fmt.Errorf("some prefix: %v", errors.NotFoundf("something")), |
| 107 | + "some prefix: something not found", |
| 108 | + isNotFoundError, |
| 109 | + }, { |
| 110 | errors.Unauthorizedf("woo %s", "hoo"), |
| 111 | "woo hoo", |
| 112 | isUnauthorizedError, |
| 113 | @@ -91,6 +96,10 @@ |
| 114 | "something already exists", |
| 115 | isAlreadyExistsError, |
| 116 | }, { |
| 117 | + fmt.Errorf("some prefix: %v", errors.NewAlreadyExistsError("something")), |
| 118 | + "some prefix: something already exists", |
| 119 | + isAlreadyExistsError, |
| 120 | + }, { |
| 121 | errors.NewNotSupportedError("something"), |
| 122 | "something not supported", |
| 123 | isNotSupportedError, |
| 124 | |
| 125 | === added file 'names/network.go' |
| 126 | --- names/network.go 1970-01-01 00:00:00 +0000 |
| 127 | +++ names/network.go 2014-04-13 09:27:07 +0000 |
| 128 | @@ -0,0 +1,24 @@ |
| 129 | +// Copyright 2014 Canonical Ltd. |
| 130 | +// Licensed under the AGPLv3, see LICENCE file for details. |
| 131 | + |
| 132 | +package names |
| 133 | + |
| 134 | +import ( |
| 135 | + "fmt" |
| 136 | + "regexp" |
| 137 | +) |
| 138 | + |
| 139 | +var validNetwork = regexp.MustCompile("^([a-z0-9]+(-[a-z0-9]+)*)$") |
| 140 | + |
| 141 | +// IsNetwork reports whether name is a valid network name. |
| 142 | +func IsNetwork(name string) bool { |
| 143 | + return validNetwork.MatchString(name) |
| 144 | +} |
| 145 | + |
| 146 | +// NetworkTag returns the tag of a network with the given name. |
| 147 | +func NetworkTag(name string) string { |
| 148 | + if !IsNetwork(name) { |
| 149 | + panic(fmt.Sprintf("%q is not a valid network name", name)) |
| 150 | + } |
| 151 | + return makeTag(NetworkTagKind, name) |
| 152 | +} |
| 153 | |
| 154 | === added file 'names/network_test.go' |
| 155 | --- names/network_test.go 1970-01-01 00:00:00 +0000 |
| 156 | +++ names/network_test.go 2014-04-13 09:27:07 +0000 |
| 157 | @@ -0,0 +1,52 @@ |
| 158 | +// Copyright 2014 Canonical Ltd. |
| 159 | +// Licensed under the AGPLv3, see LICENCE file for details. |
| 160 | + |
| 161 | +package names_test |
| 162 | + |
| 163 | +import ( |
| 164 | + "fmt" |
| 165 | + "regexp" |
| 166 | + |
| 167 | + gc "launchpad.net/gocheck" |
| 168 | + |
| 169 | + "launchpad.net/juju-core/names" |
| 170 | +) |
| 171 | + |
| 172 | +type networkSuite struct{} |
| 173 | + |
| 174 | +var _ = gc.Suite(&networkSuite{}) |
| 175 | + |
| 176 | +var networkNameTests = []struct { |
| 177 | + pattern string |
| 178 | + valid bool |
| 179 | +}{ |
| 180 | + {pattern: "", valid: false}, |
| 181 | + {pattern: "eth0", valid: true}, |
| 182 | + {pattern: "-my-net-", valid: false}, |
| 183 | + {pattern: "42", valid: true}, |
| 184 | + {pattern: "%not", valid: false}, |
| 185 | + {pattern: "$PATH", valid: false}, |
| 186 | + {pattern: "but-this-works", valid: true}, |
| 187 | + {pattern: "----", valid: false}, |
| 188 | + {pattern: "oh--no", valid: false}, |
| 189 | + {pattern: "777", valid: true}, |
| 190 | + {pattern: "is-it-", valid: false}, |
| 191 | + {pattern: "also_not", valid: false}, |
| 192 | + {pattern: "a--", valid: false}, |
| 193 | + {pattern: "foo-2", valid: true}, |
| 194 | +} |
| 195 | + |
| 196 | +func (s *networkSuite) TestNetworkNames(c *gc.C) { |
| 197 | + for i, test := range networkNameTests { |
| 198 | + c.Logf("test %d: %q", i, test.pattern) |
| 199 | + c.Check(names.IsNetwork(test.pattern), gc.Equals, test.valid) |
| 200 | + if test.valid { |
| 201 | + expectTag := fmt.Sprintf("%s-%s", names.NetworkTagKind, test.pattern) |
| 202 | + c.Check(names.NetworkTag(test.pattern), gc.Equals, expectTag) |
| 203 | + } else { |
| 204 | + expectErr := fmt.Sprintf("%q is not a valid network name", test.pattern) |
| 205 | + testNetworkTag := func() { names.NetworkTag(test.pattern) } |
| 206 | + c.Check(testNetworkTag, gc.PanicMatches, regexp.QuoteMeta(expectErr)) |
| 207 | + } |
| 208 | + } |
| 209 | +} |
| 210 | |
| 211 | === modified file 'names/tag.go' |
| 212 | --- names/tag.go 2013-09-03 11:04:55 +0000 |
| 213 | +++ names/tag.go 2014-04-13 09:27:07 +0000 |
| 214 | @@ -15,6 +15,7 @@ |
| 215 | EnvironTagKind = "environment" |
| 216 | UserTagKind = "user" |
| 217 | RelationTagKind = "relation" |
| 218 | + NetworkTagKind = "network" |
| 219 | ) |
| 220 | |
| 221 | var validKinds = map[string]bool{ |
| 222 | @@ -24,6 +25,7 @@ |
| 223 | EnvironTagKind: true, |
| 224 | UserTagKind: true, |
| 225 | RelationTagKind: true, |
| 226 | + NetworkTagKind: true, |
| 227 | } |
| 228 | |
| 229 | var tagSuffixToId = map[string]func(string) string{ |
| 230 | @@ -39,6 +41,7 @@ |
| 231 | UserTagKind: IsUser, |
| 232 | EnvironTagKind: IsEnvironment, |
| 233 | RelationTagKind: IsRelation, |
| 234 | + NetworkTagKind: IsNetwork, |
| 235 | } |
| 236 | |
| 237 | // TagKind returns one of the *TagKind constants for the given tag, or |
| 238 | |
| 239 | === modified file 'names/tag_test.go' |
| 240 | --- names/tag_test.go 2013-12-04 05:23:29 +0000 |
| 241 | +++ names/tag_test.go 2014-04-13 09:27:07 +0000 |
| 242 | @@ -27,6 +27,8 @@ |
| 243 | {tag: "relation-service.peerRelation", kind: names.RelationTagKind}, |
| 244 | {tag: "foo", err: `"foo" is not a valid tag`}, |
| 245 | {tag: "unit", err: `"unit" is not a valid tag`}, |
| 246 | + {tag: "network", err: `"network" is not a valid tag`}, |
| 247 | + {tag: "network-42", kind: names.NetworkTagKind}, |
| 248 | } |
| 249 | |
| 250 | func (*tagSuite) TestTagKind(c *gc.C) { |
| 251 | @@ -117,6 +119,14 @@ |
| 252 | expectKind: names.UserTagKind, |
| 253 | resultErr: `"user-/" is not a valid user tag`, |
| 254 | }, { |
| 255 | + tag: "network-", |
| 256 | + expectKind: names.NetworkTagKind, |
| 257 | + resultErr: `"network-" is not a valid network tag`, |
| 258 | +}, { |
| 259 | + tag: "network-mynet1", |
| 260 | + expectKind: names.NetworkTagKind, |
| 261 | + resultId: "mynet1", |
| 262 | +}, { |
| 263 | tag: "foo", |
| 264 | expectKind: "", |
| 265 | resultErr: `"foo" is not a valid tag`, |
| 266 | @@ -127,7 +137,9 @@ |
| 267 | names.UnitTagKind: names.UnitTag, |
| 268 | names.ServiceTagKind: names.ServiceTag, |
| 269 | names.RelationTagKind: names.RelationTag, |
| 270 | - // TODO(rog) environment and user, when they have Tag functions. |
| 271 | + names.EnvironTagKind: names.EnvironTag, |
| 272 | + names.UserTagKind: names.UserTag, |
| 273 | + names.NetworkTagKind: names.NetworkTag, |
| 274 | } |
| 275 | |
| 276 | func (*tagSuite) TestParseTag(c *gc.C) { |
| 277 | |
| 278 | === modified file 'provider/dummy/environs.go' |
| 279 | --- provider/dummy/environs.go 2014-04-11 17:51:58 +0000 |
| 280 | +++ provider/dummy/environs.go 2014-04-13 09:27:07 +0000 |
| 281 | @@ -770,9 +770,14 @@ |
| 282 | for i, network := range args.MachineConfig.IncludeNetworks { |
| 283 | if strings.HasPrefix(network, "bad-") { |
| 284 | // Simulate we didn't get correct information for the network. |
| 285 | - networkInfo[i] = environs.NetworkInfo{} |
| 286 | + networkInfo[i] = environs.NetworkInfo{ |
| 287 | + NetworkId: network, |
| 288 | + NetworkName: network, |
| 289 | + CIDR: "invalid", |
| 290 | + } |
| 291 | } else { |
| 292 | networkInfo[i] = environs.NetworkInfo{ |
| 293 | + NetworkId: network, |
| 294 | NetworkName: network, |
| 295 | CIDR: fmt.Sprintf("0.%d.2.0/24", i+1), |
| 296 | InterfaceName: fmt.Sprintf("eth%d", i), |
| 297 | |
| 298 | === modified file 'provider/maas/environ.go' |
| 299 | --- provider/maas/environ.go 2014-04-09 10:36:00 +0000 |
| 300 | +++ provider/maas/environ.go 2014-04-13 09:27:07 +0000 |
| 301 | @@ -365,6 +365,7 @@ |
| 302 | info := networkInfoMap[mac] |
| 303 | info.CIDR = netCIDR.String() |
| 304 | info.VLANTag = network.VLANTag |
| 305 | + info.NetworkId = network.Name |
| 306 | info.NetworkName = network.Name |
| 307 | networkInfoMap[mac] = info |
| 308 | } |
| 309 | @@ -374,12 +375,12 @@ |
| 310 | // and drop incomplete records. |
| 311 | var networkInfo []environs.NetworkInfo |
| 312 | for _, info := range networkInfoMap { |
| 313 | - if info.NetworkName == "" || info.CIDR == "" { |
| 314 | + if info.NetworkId == "" || info.NetworkName == "" || info.CIDR == "" { |
| 315 | logger.Warningf("ignoring network interface %q: missing network information", info.InterfaceName) |
| 316 | continue |
| 317 | } |
| 318 | if info.MACAddress == "" || info.InterfaceName == "" { |
| 319 | - logger.Warningf("ignoring network %q: missing network interface information", info.NetworkName) |
| 320 | + logger.Warningf("ignoring network %q: missing network interface information", info.NetworkId) |
| 321 | continue |
| 322 | } |
| 323 | networkInfo = append(networkInfo, info) |
| 324 | |
| 325 | === modified file 'state/addmachine.go' |
| 326 | --- state/addmachine.go 2014-04-04 11:14:49 +0000 |
| 327 | +++ state/addmachine.go 2014-04-13 09:27:07 +0000 |
| 328 | @@ -53,12 +53,12 @@ |
| 329 | // be associated with the machine. |
| 330 | HardwareCharacteristics instance.HardwareCharacteristics |
| 331 | |
| 332 | - // IncludeNetworks holds a list of networks the machine should be |
| 333 | - // part of. |
| 334 | + // IncludeNetworks holds a list of network names the machine |
| 335 | + // should be part of. |
| 336 | IncludeNetworks []string |
| 337 | |
| 338 | - // ExcludeNetworks holds a list of network the machine should not |
| 339 | - // be part of. |
| 340 | + // ExcludeNetworks holds a list of network names the machine |
| 341 | + // should not be part of. |
| 342 | ExcludeNetworks []string |
| 343 | |
| 344 | // Nonce holds a unique value that can be used to check |
| 345 | |
| 346 | === modified file 'state/address.go' |
| 347 | --- state/address.go 2014-03-20 01:23:05 +0000 |
| 348 | +++ state/address.go 2014-04-13 09:27:07 +0000 |
| 349 | @@ -139,8 +139,12 @@ |
| 350 | }, nil |
| 351 | } |
| 352 | |
| 353 | -// address represents the location of a machine, including metadata about what |
| 354 | -// kind of location the address describes. |
| 355 | +// address represents the location of a machine, including metadata |
| 356 | +// about what kind of location the address describes. |
| 357 | +// |
| 358 | +// TODO(dimitern) Make sure we integrate this with other networking |
| 359 | +// stuff at some point. We want to use juju-specific network names |
| 360 | +// that point to existing documents in the networks collection. |
| 361 | type address struct { |
| 362 | Value string |
| 363 | AddressType instance.AddressType |
| 364 | @@ -148,6 +152,9 @@ |
| 365 | NetworkScope instance.NetworkScope `bson:",omitempty"` |
| 366 | } |
| 367 | |
| 368 | +// TODO(dimitern) Make sure we integrate this with other networking |
| 369 | +// stuff at some point. We want to use juju-specific network names |
| 370 | +// that point to existing documents in the networks collection. |
| 371 | type hostPort struct { |
| 372 | Value string |
| 373 | AddressType instance.AddressType |
| 374 | |
| 375 | === modified file 'state/api/client.go' |
| 376 | --- state/api/client.go 2014-04-10 23:15:49 +0000 |
| 377 | +++ state/api/client.go 2014-04-13 09:27:07 +0000 |
| 378 | @@ -271,8 +271,9 @@ |
| 379 | } |
| 380 | |
| 381 | // ServiceDeployWithNetworks works exactly like ServiceDeploy, but |
| 382 | -// allows specifying networks to either include or exclude on the |
| 383 | -// machine where the charm is deployed. |
| 384 | +// allows the specification of networks that will be included or |
| 385 | +// excluded on the machines where the service is deployed. Networks |
| 386 | +// are specified with network tags (see names.NetworkTag). |
| 387 | func (c *Client) ServiceDeployWithNetworks(charmURL string, serviceName string, numUnits int, configYAML string, cons constraints.Value, toMachineSpec string, includeNetworks, excludeNetworks []string) error { |
| 388 | params := params.ServiceDeploy{ |
| 389 | ServiceName: serviceName, |
| 390 | |
| 391 | === modified file 'state/api/params/internal.go' |
| 392 | --- state/api/params/internal.go 2014-04-10 10:27:40 +0000 |
| 393 | +++ state/api/params/internal.go 2014-04-13 09:27:07 +0000 |
| 394 | @@ -313,17 +313,33 @@ |
| 395 | |
| 396 | // Network describes a single network available on an instance. |
| 397 | type Network struct { |
| 398 | - Name string |
| 399 | - CIDR string |
| 400 | + // Tag is the network's tag. |
| 401 | + Tag string |
| 402 | + |
| 403 | + // ProviderId is the provider-specific network id. |
| 404 | + ProviderId string |
| 405 | + |
| 406 | + // CIDR of the network, in "123.45.67.89/12" format. |
| 407 | + CIDR string |
| 408 | + |
| 409 | + // VLANTag needs to be between 1 and 4094 for VLANs and 0 for |
| 410 | + // normal networks. It's defined by IEEE 802.1Q standard. |
| 411 | VLANTag int |
| 412 | } |
| 413 | |
| 414 | // NetworkInterface describes a single network interface available on |
| 415 | // an instance. |
| 416 | type NetworkInterface struct { |
| 417 | - MACAddress string |
| 418 | + // MACAddress is the network interface's hardware MAC address |
| 419 | + // (e.g. "aa:bb:cc:dd:ee:ff"). |
| 420 | + MACAddress string |
| 421 | + |
| 422 | + // InterfaceName is the OS-specific network device name (e.g. |
| 423 | + // "eth0" or "eth1.42" for a VLAN virtual interface). |
| 424 | InterfaceName string |
| 425 | - NetworkName string |
| 426 | + |
| 427 | + // NetworkTag is this interface's network tag. |
| 428 | + NetworkTag string |
| 429 | } |
| 430 | |
| 431 | // InstanceInfo holds a machine tag, provider-specific instance id, a |
| 432 | @@ -343,16 +359,16 @@ |
| 433 | Machines []InstanceInfo |
| 434 | } |
| 435 | |
| 436 | -// NetworkResult holds machine networks or an error. |
| 437 | -type NetworkResult struct { |
| 438 | +// RequestedNetworkResult holds requested networks or an error. |
| 439 | +type RequestedNetworkResult struct { |
| 440 | Error *Error |
| 441 | IncludeNetworks []string |
| 442 | ExcludeNetworks []string |
| 443 | } |
| 444 | |
| 445 | -// NetworksResults holds multiple networks results. |
| 446 | -type NetworksResults struct { |
| 447 | - Results []NetworkResult |
| 448 | +// RequestedNetworksResults holds multiple requested networks results. |
| 449 | +type RequestedNetworksResults struct { |
| 450 | + Results []RequestedNetworkResult |
| 451 | } |
| 452 | |
| 453 | // EntityStatus holds an entity tag, status and extra info. |
| 454 | |
| 455 | === modified file 'state/api/provisioner/machine.go' |
| 456 | --- state/api/provisioner/machine.go 2014-04-12 05:53:58 +0000 |
| 457 | +++ state/api/provisioner/machine.go 2014-04-13 09:27:07 +0000 |
| 458 | @@ -58,7 +58,7 @@ |
| 459 | // RequestedNetworks returns a pair of lists of networks to |
| 460 | // enable/disable on the machine. |
| 461 | func (m *Machine) RequestedNetworks() (includeNetworks, excludeNetworks []string, err error) { |
| 462 | - var results params.NetworksResults |
| 463 | + var results params.RequestedNetworksResults |
| 464 | args := params.Entities{Entities: []params.Entity{{m.tag}}} |
| 465 | err = m.st.call("RequestedNetworks", args, &results) |
| 466 | if err != nil { |
| 467 | @@ -205,7 +205,7 @@ |
| 468 | return result.Result, nil |
| 469 | } |
| 470 | |
| 471 | -// SetInstanceInfo sets the provider specific machine id, nonce, |
| 472 | +// SetInstanceInfo sets the provider specific instance id, nonce, |
| 473 | // metadata, networks and interfaces for this machine. Once set, the |
| 474 | // instance id cannot be changed. |
| 475 | func (m *Machine) SetInstanceInfo( |
| 476 | |
| 477 | === modified file 'state/api/provisioner/provisioner_test.go' |
| 478 | --- state/api/provisioner/provisioner_test.go 2014-04-10 07:42:39 +0000 |
| 479 | +++ state/api/provisioner/provisioner_test.go 2014-04-13 09:27:07 +0000 |
| 480 | @@ -220,33 +220,36 @@ |
| 481 | c.Assert(ifacesMachine, gc.HasLen, 0) |
| 482 | |
| 483 | networks := []params.Network{{ |
| 484 | - Name: "net1", |
| 485 | - CIDR: "0.1.2.0/24", |
| 486 | - VLANTag: 0, |
| 487 | - }, { |
| 488 | - Name: "vlan42", |
| 489 | - CIDR: "0.2.2.0/24", |
| 490 | - VLANTag: 42, |
| 491 | - }, { |
| 492 | - Name: "vlan42", // duplicated; ignored |
| 493 | - CIDR: "0.2.2.0/24", |
| 494 | - VLANTag: 42, |
| 495 | + Tag: "network-net1", |
| 496 | + ProviderId: "net1", |
| 497 | + CIDR: "0.1.2.0/24", |
| 498 | + VLANTag: 0, |
| 499 | + }, { |
| 500 | + Tag: "network-vlan42", |
| 501 | + ProviderId: "vlan42", |
| 502 | + CIDR: "0.2.2.0/24", |
| 503 | + VLANTag: 42, |
| 504 | + }, { |
| 505 | + Tag: "network-vlan42", // duplicated; ignored |
| 506 | + ProviderId: "vlan42", |
| 507 | + CIDR: "0.2.2.0/24", |
| 508 | + VLANTag: 42, |
| 509 | }} |
| 510 | ifaces := []params.NetworkInterface{{ |
| 511 | MACAddress: "aa:bb:cc:dd:ee:f0", |
| 512 | - NetworkName: "net1", |
| 513 | + NetworkTag: "network-net1", |
| 514 | InterfaceName: "eth0", |
| 515 | }, { |
| 516 | MACAddress: "aa:bb:cc:dd:ee:f1", |
| 517 | - NetworkName: "net1", |
| 518 | + NetworkTag: "network-net1", |
| 519 | InterfaceName: "eth1", |
| 520 | }, { |
| 521 | MACAddress: "aa:bb:cc:dd:ee:f2", |
| 522 | - NetworkName: "vlan42", |
| 523 | + NetworkTag: "network-vlan42", |
| 524 | InterfaceName: "eth2", |
| 525 | }, { |
| 526 | MACAddress: "aa:bb:cc:dd:ee:f2", // duplicated; ignored |
| 527 | - NetworkName: "vlan42", |
| 528 | + NetworkTag: "network-vlan42", |
| 529 | InterfaceName: "eth2", |
| 530 | }} |
| 531 | |
| 532 | @@ -274,9 +277,13 @@ |
| 533 | // Last one was ignored, so skip it. |
| 534 | break |
| 535 | } |
| 536 | - network, err := s.State.Network(networks[i].Name) |
| 537 | - c.Assert(err, gc.IsNil) |
| 538 | - c.Check(network.Name(), gc.Equals, networks[i].Name) |
| 539 | + _, networkName, err := names.ParseTag(networks[i].Tag, names.NetworkTagKind) |
| 540 | + c.Assert(err, gc.IsNil) |
| 541 | + network, err := s.State.Network(networkName) |
| 542 | + c.Assert(err, gc.IsNil) |
| 543 | + c.Check(network.Name(), gc.Equals, networkName) |
| 544 | + c.Check(network.ProviderId(), gc.Equals, networks[i].ProviderId) |
| 545 | + c.Check(network.Tag(), gc.Equals, networks[i].Tag) |
| 546 | c.Check(network.VLANTag(), gc.Equals, networks[i].VLANTag) |
| 547 | c.Check(network.CIDR(), gc.Equals, networks[i].CIDR) |
| 548 | } |
| 549 | @@ -288,10 +295,12 @@ |
| 550 | actual := make([]params.NetworkInterface, len(ifacesMachine)) |
| 551 | for i, iface := range ifacesMachine { |
| 552 | actual[i].InterfaceName = iface.InterfaceName() |
| 553 | - actual[i].NetworkName = iface.NetworkName() |
| 554 | + actual[i].NetworkTag = iface.NetworkTag() |
| 555 | actual[i].MACAddress = iface.MACAddress() |
| 556 | - c.Check(names.MachineTag(iface.MachineId()), gc.Equals, notProvisionedMachine.Tag()) |
| 557 | + c.Check(iface.MachineTag(), gc.Equals, notProvisionedMachine.Tag()) |
| 558 | + c.Check(iface.MachineId(), gc.Equals, notProvisionedMachine.Id()) |
| 559 | } |
| 560 | + c.Assert(actual, jc.SameContents, ifaces[:3]) // skip [3] as it's ignored. |
| 561 | } |
| 562 | |
| 563 | func (s *provisionerSuite) TestSeries(c *gc.C) { |
| 564 | |
| 565 | === modified file 'state/apiserver/client/client.go' |
| 566 | --- state/apiserver/client/client.go 2014-04-10 10:27:40 +0000 |
| 567 | +++ state/apiserver/client/client.go 2014-04-13 09:27:07 +0000 |
| 568 | @@ -238,6 +238,18 @@ |
| 569 | |
| 570 | var CharmStore charm.Repository = charm.Store |
| 571 | |
| 572 | +func networkTagsToNames(tags []string) ([]string, error) { |
| 573 | + netNames := make([]string, len(tags)) |
| 574 | + for i, tag := range tags { |
| 575 | + _, name, err := names.ParseTag(tag, names.NetworkTagKind) |
| 576 | + if err != nil { |
| 577 | + return nil, err |
| 578 | + } |
| 579 | + netNames[i] = name |
| 580 | + } |
| 581 | + return netNames, nil |
| 582 | +} |
| 583 | + |
| 584 | // ServiceDeploy fetches the charm from the charm store and deploys it. |
| 585 | // AddCharm or AddLocalCharm should be called to add the charm |
| 586 | // before calling ServiceDeploy, although for backward compatibility |
| 587 | @@ -280,6 +292,15 @@ |
| 588 | if err != nil { |
| 589 | return err |
| 590 | } |
| 591 | + // Convert network tags to names for any given networks. |
| 592 | + includeNetworks, err := networkTagsToNames(args.IncludeNetworks) |
| 593 | + if err != nil { |
| 594 | + return err |
| 595 | + } |
| 596 | + excludeNetworks, err := networkTagsToNames(args.ExcludeNetworks) |
| 597 | + if err != nil { |
| 598 | + return err |
| 599 | + } |
| 600 | |
| 601 | _, err = juju.DeployService(c.api.state, |
| 602 | juju.DeployServiceParams{ |
| 603 | @@ -290,15 +311,16 @@ |
| 604 | ConfigSettings: settings, |
| 605 | Constraints: args.Constraints, |
| 606 | ToMachineSpec: args.ToMachineSpec, |
| 607 | - IncludeNetworks: args.IncludeNetworks, |
| 608 | - ExcludeNetworks: args.ExcludeNetworks, |
| 609 | + IncludeNetworks: includeNetworks, |
| 610 | + ExcludeNetworks: excludeNetworks, |
| 611 | }) |
| 612 | return err |
| 613 | } |
| 614 | |
| 615 | // ServiceDeployWithNetworks works exactly like ServiceDeploy, but |
| 616 | // allows specifying networks to include or exclude on the machine |
| 617 | -// where the charm gets deployed. |
| 618 | +// where the charm gets deployed. Each given network to |
| 619 | +// include/exclude needs to be specified using its network tag. |
| 620 | func (c *Client) ServiceDeployWithNetworks(args params.ServiceDeploy) error { |
| 621 | return c.ServiceDeploy(args) |
| 622 | } |
| 623 | |
| 624 | === modified file 'state/apiserver/client/client_test.go' |
| 625 | --- state/apiserver/client/client_test.go 2014-04-10 10:27:40 +0000 |
| 626 | +++ state/apiserver/client/client_test.go 2014-04-13 09:27:07 +0000 |
| 627 | @@ -686,8 +686,17 @@ |
| 628 | defer restore() |
| 629 | curl, bundle := addCharm(c, store, "dummy") |
| 630 | mem4g := constraints.MustParse("mem=4G") |
| 631 | + |
| 632 | + // Check for invalid network tags handling. |
| 633 | err := s.APIState.Client().ServiceDeployWithNetworks( |
| 634 | - curl.String(), "service", 3, "", mem4g, "", []string{"net1", "net2"}, []string{"net3"}, |
| 635 | + curl.String(), "service", 3, "", mem4g, "", |
| 636 | + []string{"net1", "net2"}, []string{"net3"}, |
| 637 | + ) |
| 638 | + c.Assert(err, gc.ErrorMatches, `"net1" is not a valid network tag`) |
| 639 | + |
| 640 | + err = s.APIState.Client().ServiceDeployWithNetworks( |
| 641 | + curl.String(), "service", 3, "", mem4g, "", |
| 642 | + []string{"network-net1", "network-net2"}, []string{"network-net3"}, |
| 643 | ) |
| 644 | c.Assert(err, gc.IsNil) |
| 645 | service := s.assertPrincipalDeployed(c, "service", curl, false, bundle, mem4g) |
| 646 | |
| 647 | === modified file 'state/apiserver/provisioner/provisioner.go' |
| 648 | --- state/apiserver/provisioner/provisioner.go 2014-04-09 16:09:20 +0000 |
| 649 | +++ state/apiserver/provisioner/provisioner.go 2014-04-13 09:27:07 +0000 |
| 650 | @@ -409,10 +409,43 @@ |
| 651 | return result, nil |
| 652 | } |
| 653 | |
| 654 | -// RequestedNetworks returns the requested networks for each given machine entity. |
| 655 | -func (p *ProvisionerAPI) RequestedNetworks(args params.Entities) (params.NetworksResults, error) { |
| 656 | - result := params.NetworksResults{ |
| 657 | - Results: make([]params.NetworkResult, len(args.Entities)), |
| 658 | +func networkParamsToStateParams(networks []params.Network, ifaces []params.NetworkInterface) ( |
| 659 | + []state.NetworkInfo, []state.NetworkInterfaceInfo, error, |
| 660 | +) { |
| 661 | + stateNetworks := make([]state.NetworkInfo, len(networks)) |
| 662 | + for i, network := range networks { |
| 663 | + _, networkName, err := names.ParseTag(network.Tag, names.NetworkTagKind) |
| 664 | + if err != nil { |
| 665 | + return nil, nil, err |
| 666 | + } |
| 667 | + stateNetworks[i] = state.NetworkInfo{ |
| 668 | + Name: networkName, |
| 669 | + ProviderId: network.ProviderId, |
| 670 | + CIDR: network.CIDR, |
| 671 | + VLANTag: network.VLANTag, |
| 672 | + } |
| 673 | + } |
| 674 | + stateInterfaces := make([]state.NetworkInterfaceInfo, len(ifaces)) |
| 675 | + for i, iface := range ifaces { |
| 676 | + _, networkName, err := names.ParseTag(iface.NetworkTag, names.NetworkTagKind) |
| 677 | + if err != nil { |
| 678 | + return nil, nil, err |
| 679 | + } |
| 680 | + stateInterfaces[i] = state.NetworkInterfaceInfo{ |
| 681 | + MACAddress: iface.MACAddress, |
| 682 | + NetworkName: networkName, |
| 683 | + InterfaceName: iface.InterfaceName, |
| 684 | + } |
| 685 | + } |
| 686 | + return stateNetworks, stateInterfaces, nil |
| 687 | +} |
| 688 | + |
| 689 | +// RequestedNetworks returns the requested networks for each given |
| 690 | +// machine entity. Each entry in both lists is returned with its |
| 691 | +// provider specific id. |
| 692 | +func (p *ProvisionerAPI) RequestedNetworks(args params.Entities) (params.RequestedNetworksResults, error) { |
| 693 | + result := params.RequestedNetworksResults{ |
| 694 | + Results: make([]params.RequestedNetworkResult, len(args.Entities)), |
| 695 | } |
| 696 | canAccess, err := p.getAuthFunc() |
| 697 | if err != nil { |
| 698 | @@ -425,6 +458,12 @@ |
| 699 | var excludeNetworks []string |
| 700 | includeNetworks, excludeNetworks, err = machine.RequestedNetworks() |
| 701 | if err == nil { |
| 702 | + // TODO(dimitern) For now, since network names and |
| 703 | + // provider ids are the same, we return what we got |
| 704 | + // from state. In the future, when networks can be |
| 705 | + // added before provisioning, we should convert both |
| 706 | + // slices from juju network names to provider-specific |
| 707 | + // ids before returning them. |
| 708 | result.Results[i].IncludeNetworks = includeNetworks |
| 709 | result.Results[i].ExcludeNetworks = excludeNetworks |
| 710 | } |
| 711 | @@ -473,9 +512,14 @@ |
| 712 | for i, arg := range args.Machines { |
| 713 | machine, err := p.getMachine(canAccess, arg.Tag) |
| 714 | if err == nil { |
| 715 | - err = machine.SetInstanceInfo( |
| 716 | - arg.InstanceId, arg.Nonce, arg.Characteristics, |
| 717 | - arg.Networks, arg.Interfaces) |
| 718 | + var networks []state.NetworkInfo |
| 719 | + var interfaces []state.NetworkInterfaceInfo |
| 720 | + networks, interfaces, err = networkParamsToStateParams(arg.Networks, arg.Interfaces) |
| 721 | + if err == nil { |
| 722 | + err = machine.SetInstanceInfo( |
| 723 | + arg.InstanceId, arg.Nonce, arg.Characteristics, |
| 724 | + networks, interfaces) |
| 725 | + } |
| 726 | if err != nil { |
| 727 | // Give the user more context about the error. |
| 728 | err = fmt.Errorf("aborted instance %q: %v", arg.InstanceId, err) |
| 729 | |
| 730 | === modified file 'state/apiserver/provisioner/provisioner_test.go' |
| 731 | --- state/apiserver/provisioner/provisioner_test.go 2014-04-11 17:51:58 +0000 |
| 732 | +++ state/apiserver/provisioner/provisioner_test.go 2014-04-13 09:27:07 +0000 |
| 733 | @@ -782,8 +782,8 @@ |
| 734 | }} |
| 735 | result, err := s.provisioner.RequestedNetworks(args) |
| 736 | c.Assert(err, gc.IsNil) |
| 737 | - c.Assert(result, gc.DeepEquals, params.NetworksResults{ |
| 738 | - Results: []params.NetworkResult{ |
| 739 | + c.Assert(result, gc.DeepEquals, params.RequestedNetworksResults{ |
| 740 | + Results: []params.RequestedNetworkResult{ |
| 741 | { |
| 742 | IncludeNetworks: includeNetsMachine0, |
| 743 | ExcludeNetworks: excludeNetsMachine0, |
| 744 | @@ -852,33 +852,36 @@ |
| 745 | c.Assert(err, gc.IsNil) |
| 746 | |
| 747 | networks := []params.Network{{ |
| 748 | - Name: "net1", |
| 749 | - CIDR: "0.1.2.0/24", |
| 750 | - VLANTag: 0, |
| 751 | - }, { |
| 752 | - Name: "vlan42", |
| 753 | - CIDR: "0.2.2.0/24", |
| 754 | - VLANTag: 42, |
| 755 | - }, { |
| 756 | - Name: "vlan42", // duplicated; ignored |
| 757 | - CIDR: "0.2.2.0/24", |
| 758 | - VLANTag: 42, |
| 759 | + Tag: "network-net1", |
| 760 | + ProviderId: "net1", |
| 761 | + CIDR: "0.1.2.0/24", |
| 762 | + VLANTag: 0, |
| 763 | + }, { |
| 764 | + Tag: "network-vlan42", |
| 765 | + ProviderId: "vlan42", |
| 766 | + CIDR: "0.2.2.0/24", |
| 767 | + VLANTag: 42, |
| 768 | + }, { |
| 769 | + Tag: "network-vlan42", // duplicated; ignored |
| 770 | + ProviderId: "vlan42", |
| 771 | + CIDR: "0.2.2.0/24", |
| 772 | + VLANTag: 42, |
| 773 | }} |
| 774 | ifaces := []params.NetworkInterface{{ |
| 775 | MACAddress: "aa:bb:cc:dd:ee:f0", |
| 776 | - NetworkName: "net1", |
| 777 | + NetworkTag: "network-net1", |
| 778 | InterfaceName: "eth0", |
| 779 | }, { |
| 780 | MACAddress: "aa:bb:cc:dd:ee:f1", |
| 781 | - NetworkName: "net1", |
| 782 | + NetworkTag: "network-net1", |
| 783 | InterfaceName: "eth1", |
| 784 | }, { |
| 785 | MACAddress: "aa:bb:cc:dd:ee:f2", |
| 786 | - NetworkName: "vlan42", |
| 787 | + NetworkTag: "network-vlan42", |
| 788 | InterfaceName: "eth2", |
| 789 | }, { |
| 790 | MACAddress: "aa:bb:cc:dd:ee:f2", // duplicated; ignored |
| 791 | - NetworkName: "vlan42", |
| 792 | + NetworkTag: "network-vlan42", |
| 793 | InterfaceName: "eth2", |
| 794 | }} |
| 795 | args := params.InstancesInfo{Machines: []params.InstanceInfo{{ |
| 796 | @@ -940,9 +943,10 @@ |
| 797 | actual := make([]params.NetworkInterface, len(ifacesMachine1)) |
| 798 | for i, iface := range ifacesMachine1 { |
| 799 | actual[i].InterfaceName = iface.InterfaceName() |
| 800 | - actual[i].NetworkName = iface.NetworkName() |
| 801 | + actual[i].NetworkTag = iface.NetworkTag() |
| 802 | actual[i].MACAddress = iface.MACAddress() |
| 803 | - c.Check(names.MachineTag(iface.MachineId()), gc.Equals, s.machines[1].Tag()) |
| 804 | + c.Check(iface.MachineId(), gc.Equals, s.machines[1].Id()) |
| 805 | + c.Check(iface.MachineTag(), gc.Equals, s.machines[1].Tag()) |
| 806 | } |
| 807 | c.Assert(actual, jc.SameContents, ifaces[:3]) |
| 808 | ifacesMachine2, err := s.machines[2].NetworkInterfaces() |
| 809 | @@ -953,9 +957,13 @@ |
| 810 | // Last one was ignored, so don't check. |
| 811 | break |
| 812 | } |
| 813 | - network, err := s.State.Network(networks[i].Name) |
| 814 | - c.Assert(err, gc.IsNil) |
| 815 | - c.Check(network.Name(), gc.Equals, networks[i].Name) |
| 816 | + _, networkName, err := names.ParseTag(networks[i].Tag, names.NetworkTagKind) |
| 817 | + c.Assert(err, gc.IsNil) |
| 818 | + network, err := s.State.Network(networkName) |
| 819 | + c.Assert(err, gc.IsNil) |
| 820 | + c.Check(network.Name(), gc.Equals, networkName) |
| 821 | + c.Check(network.ProviderId(), gc.Equals, networks[i].ProviderId) |
| 822 | + c.Check(network.Tag(), gc.Equals, networks[i].Tag) |
| 823 | c.Check(network.VLANTag(), gc.Equals, networks[i].VLANTag) |
| 824 | c.Check(network.CIDR(), gc.Equals, networks[i].CIDR) |
| 825 | } |
| 826 | |
| 827 | === modified file 'state/apiserver/uniter/uniter_test.go' |
| 828 | --- state/apiserver/uniter/uniter_test.go 2014-04-12 05:53:58 +0000 |
| 829 | +++ state/apiserver/uniter/uniter_test.go 2014-04-13 09:27:07 +0000 |
| 830 | @@ -1178,13 +1178,13 @@ |
| 831 | result, err := s.uniter.ReadRemoteSettings(args) |
| 832 | |
| 833 | // We don't set the remote unit settings on purpose to test the error. |
| 834 | - expectErr := `cannot read settings for unit "mysql/0" in relation "wordpress:db mysql:server": settings not found` |
| 835 | + expectErr := `cannot read settings for unit "mysql/0" in relation "wordpress:db mysql:server": settings` |
| 836 | c.Assert(err, gc.IsNil) |
| 837 | - c.Assert(result, gc.DeepEquals, params.RelationSettingsResults{ |
| 838 | + c.Assert(result, jc.DeepEquals, params.RelationSettingsResults{ |
| 839 | Results: []params.RelationSettingsResult{ |
| 840 | {Error: apiservertesting.ErrUnauthorized}, |
| 841 | {Error: apiservertesting.ErrUnauthorized}, |
| 842 | - {Error: ¶ms.Error{Message: expectErr}}, |
| 843 | + {Error: apiservertesting.NotFoundError(expectErr)}, |
| 844 | {Error: apiservertesting.ErrUnauthorized}, |
| 845 | {Error: apiservertesting.ErrUnauthorized}, |
| 846 | {Error: apiservertesting.ErrUnauthorized}, |
| 847 | |
| 848 | === modified file 'state/apiserver/usermanager/usermanager_test.go' |
| 849 | --- state/apiserver/usermanager/usermanager_test.go 2014-03-10 08:27:32 +0000 |
| 850 | +++ state/apiserver/usermanager/usermanager_test.go 2014-04-13 09:27:07 +0000 |
| 851 | @@ -106,8 +106,8 @@ |
| 852 | c.Assert(err, gc.IsNil) |
| 853 | _, err = s.State.User("addremove") |
| 854 | result, err := s.usermanager.AddUser(args) |
| 855 | - expectedError := params.Error{Code: "", Message: "Failed to create user: user already exists"} |
| 856 | + expectedError := apiservertesting.AlreadyExistsError("Failed to create user: user") |
| 857 | c.Assert(result, gc.DeepEquals, params.ErrorResults{ |
| 858 | Results: []params.ErrorResult{ |
| 859 | - params.ErrorResult{&expectedError}}}) |
| 860 | + params.ErrorResult{expectedError}}}) |
| 861 | } |
| 862 | |
| 863 | === modified file 'state/machine.go' |
| 864 | --- state/machine.go 2014-04-12 05:53:58 +0000 |
| 865 | +++ state/machine.go 2014-04-13 09:27:07 +0000 |
| 866 | @@ -837,11 +837,11 @@ |
| 867 | // Merge SetProvisioned() in here or drop it at that point. |
| 868 | func (m *Machine) SetInstanceInfo( |
| 869 | id instance.Id, nonce string, characteristics *instance.HardwareCharacteristics, |
| 870 | - networks []params.Network, interfaces []params.NetworkInterface) error { |
| 871 | + networks []NetworkInfo, interfaces []NetworkInterfaceInfo) error { |
| 872 | |
| 873 | // Add the networks and interfaces first. |
| 874 | for _, network := range networks { |
| 875 | - _, err := m.st.AddNetwork(network.Name, network.CIDR, network.VLANTag) |
| 876 | + _, err := m.st.AddNetwork(network.Name, network.ProviderId, network.CIDR, network.VLANTag) |
| 877 | if err != nil && errors.IsAlreadyExistsError(err) { |
| 878 | // Ignore already existing networks. |
| 879 | continue |
| 880 | @@ -954,13 +954,14 @@ |
| 881 | return nil |
| 882 | } |
| 883 | |
| 884 | -// RequestedNetworks returns the list of networks the machine should |
| 885 | -// be on (includeNetworks) or not (excludeNetworks). |
| 886 | +// RequestedNetworks returns the list of network names the machine |
| 887 | +// should be on (includeNetworks) or not (excludeNetworks). |
| 888 | func (m *Machine) RequestedNetworks() (includeNetworks, excludeNetworks []string, err error) { |
| 889 | return readRequestedNetworks(m.st, m.globalKey()) |
| 890 | } |
| 891 | |
| 892 | // Networks returns the list of configured networks on the machine. |
| 893 | +// The configured and requested networks on a machine must match. |
| 894 | func (m *Machine) Networks() ([]*Network, error) { |
| 895 | includeNetworks, _, err := m.RequestedNetworks() |
| 896 | if err != nil { |
| 897 | @@ -997,26 +998,25 @@ |
| 898 | // AddNetworkInterface creates a new network interface on the given |
| 899 | // network for the machine. The machine must be alive and not yet |
| 900 | // provisioned, and there must be no other interface with the same MAC |
| 901 | -// address for this to succeed. If a network interface with the given |
| 902 | -// MAC address already exists, the returned error satisfies |
| 903 | +// address or the same name on that machine for this to succeed. If a |
| 904 | +// network interface already exists, the returned error satisfies |
| 905 | // errors.IsAlreadyExistsError. |
| 906 | -func (m *Machine) AddNetworkInterface(macAddress, name, networkName string) (iface *NetworkInterface, err error) { |
| 907 | - defer func() { |
| 908 | - if !errors.IsAlreadyExistsError(err) { |
| 909 | - utils.ErrorContextf(&err, "cannot add network interface to machine %s", m.doc.Id) |
| 910 | - } |
| 911 | - }() |
| 912 | +func (m *Machine) AddNetworkInterface(macAddress, interfaceName, networkName string) (iface *NetworkInterface, err error) { |
| 913 | + defer utils.ErrorContextf(&err, "cannot add network interface %q to machine %q", interfaceName, m.doc.Id) |
| 914 | |
| 915 | + if macAddress == "" { |
| 916 | + return nil, fmt.Errorf("MAC address must be not empty") |
| 917 | + } |
| 918 | if _, err = net.ParseMAC(macAddress); err != nil { |
| 919 | return nil, err |
| 920 | } |
| 921 | - if name == "" { |
| 922 | + if interfaceName == "" { |
| 923 | return nil, fmt.Errorf("interface name must be not empty") |
| 924 | } |
| 925 | aliveAndNotProvisioned := append(isAliveDoc, bson.D{{"nonce", ""}}...) |
| 926 | doc := &networkInterfaceDoc{ |
| 927 | MACAddress: macAddress, |
| 928 | - InterfaceName: name, |
| 929 | + InterfaceName: interfaceName, |
| 930 | NetworkName: networkName, |
| 931 | MachineId: m.doc.Id, |
| 932 | } |
| 933 | @@ -1039,8 +1039,11 @@ |
| 934 | err = m.st.runTransaction(ops) |
| 935 | switch err { |
| 936 | case txn.ErrAborted: |
| 937 | - _, err = m.st.Network(networkName) |
| 938 | - if err != nil { |
| 939 | + if err = m.st.networkInterfaces.FindId(macAddress).One(nil); err == nil { |
| 940 | + msg := fmt.Sprintf("interface with MAC address %q", macAddress) |
| 941 | + return nil, errors.NewAlreadyExistsError(msg) |
| 942 | + } |
| 943 | + if _, err = m.st.Network(networkName); err != nil { |
| 944 | return nil, err |
| 945 | } |
| 946 | if err = m.Refresh(); err != nil { |
| 947 | @@ -1048,11 +1051,18 @@ |
| 948 | } else if m.doc.Life != Alive { |
| 949 | return nil, fmt.Errorf("machine is not alive") |
| 950 | } else if m.doc.Nonce != "" { |
| 951 | - return nil, fmt.Errorf("machine already provisioned: dynamic network interfaces not currently supported") |
| 952 | + msg := "machine already provisioned: dynamic network interfaces not currently supported" |
| 953 | + return nil, fmt.Errorf(msg) |
| 954 | } |
| 955 | - // Network and machine both OK, so the other assert must have failed. |
| 956 | - return nil, errors.NewAlreadyExistsError("interface with MAC address " + macAddress) |
| 957 | case nil: |
| 958 | + // For some reason when using unique indices with mgo, and |
| 959 | + // we have an index violation the error is nil, but the |
| 960 | + // document is not added. So we check if the supposedly |
| 961 | + // successful transaction did actually add the document. |
| 962 | + if err = m.st.networkInterfaces.FindId(macAddress).One(nil); err != nil { |
| 963 | + msg := fmt.Sprintf("%q on machine %q", interfaceName, m.doc.Id) |
| 964 | + return nil, errors.NewAlreadyExistsError(msg) |
| 965 | + } |
| 966 | return newNetworkInterface(m.st, doc), nil |
| 967 | default: |
| 968 | return nil, err |
| 969 | |
| 970 | === modified file 'state/machine_test.go' |
| 971 | --- state/machine_test.go 2014-04-12 05:53:58 +0000 |
| 972 | +++ state/machine_test.go 2014-04-13 09:27:07 +0000 |
| 973 | @@ -4,8 +4,8 @@ |
| 974 | package state_test |
| 975 | |
| 976 | import ( |
| 977 | - "fmt" |
| 978 | "sort" |
| 979 | + "strings" |
| 980 | |
| 981 | jc "github.com/juju/testing/checkers" |
| 982 | "labix.org/v2/mgo/bson" |
| 983 | @@ -14,7 +14,6 @@ |
| 984 | "launchpad.net/juju-core/constraints" |
| 985 | "launchpad.net/juju-core/errors" |
| 986 | "launchpad.net/juju-core/instance" |
| 987 | - "launchpad.net/juju-core/names" |
| 988 | "launchpad.net/juju-core/state" |
| 989 | "launchpad.net/juju-core/state/api/params" |
| 990 | "launchpad.net/juju-core/state/testing" |
| 991 | @@ -390,9 +389,10 @@ |
| 992 | } |
| 993 | |
| 994 | func addNetworkAndInterface(c *gc.C, st *state.State, machine *state.Machine, |
| 995 | - networkName, cidr string, vlanTag int, |
| 996 | - mac, ifaceName string) (*state.Network, *state.NetworkInterface) { |
| 997 | - net, err := st.AddNetwork(networkName, cidr, vlanTag) |
| 998 | + networkName, providerId, cidr string, vlanTag int, |
| 999 | + mac, ifaceName string, |
| 1000 | +) (*state.Network, *state.NetworkInterface) { |
| 1001 | + net, err := st.AddNetwork(networkName, providerId, cidr, vlanTag) |
| 1002 | c.Assert(err, gc.IsNil) |
| 1003 | iface, err := machine.AddNetworkInterface(mac, ifaceName, networkName) |
| 1004 | c.Assert(err, gc.IsNil) |
| 1005 | @@ -419,11 +419,11 @@ |
| 1006 | |
| 1007 | net1, _ := addNetworkAndInterface( |
| 1008 | c, s.State, machine, |
| 1009 | - "net1", "0.1.2.0/24", 0, |
| 1010 | + "net1", "net1", "0.1.2.0/24", 0, |
| 1011 | "aa:bb:cc:dd:ee:f0", "eth0") |
| 1012 | net2, _ := addNetworkAndInterface( |
| 1013 | c, s.State, machine, |
| 1014 | - "net2", "0.2.2.0/24", 0, |
| 1015 | + "net2", "net2", "0.2.2.0/24", 0, |
| 1016 | "aa:bb:cc:dd:ee:f1", "eth1") |
| 1017 | |
| 1018 | nets, err = machine.Networks() |
| 1019 | @@ -448,15 +448,15 @@ |
| 1020 | // And a few networks and NICs. |
| 1021 | _, iface0 := addNetworkAndInterface( |
| 1022 | c, s.State, machine, |
| 1023 | - "net1", "0.1.2.0/24", 0, |
| 1024 | + "net1", "net1", "0.1.2.0/24", 0, |
| 1025 | "aa:bb:cc:dd:ee:f0", "eth0") |
| 1026 | _, iface1 := addNetworkAndInterface( |
| 1027 | c, s.State, machine, |
| 1028 | - "vlan42", "0.1.2.0/30", 42, |
| 1029 | + "vlan42", "vlan42", "0.1.2.0/30", 42, |
| 1030 | "aa:bb:cc:dd:ee:f1", "eth0.42") |
| 1031 | _, iface2 := addNetworkAndInterface( |
| 1032 | c, s.State, machine, |
| 1033 | - "net2", "0.2.2.0/24", 0, |
| 1034 | + "net2", "net2", "0.2.2.0/24", 0, |
| 1035 | "aa:bb:cc:dd:ee:f2", "eth1") |
| 1036 | |
| 1037 | ifaces, err = machine.NetworkInterfaces() |
| 1038 | @@ -475,6 +475,56 @@ |
| 1039 | c.Assert(ifaces, gc.HasLen, 0) |
| 1040 | } |
| 1041 | |
| 1042 | +var addNetworkInterfaceErrorsTests = []struct { |
| 1043 | + macAddress string |
| 1044 | + interfaceName string |
| 1045 | + networkName string |
| 1046 | + beforeAdding func(*gc.C, *state.Machine) |
| 1047 | + expectErr string |
| 1048 | +}{{ |
| 1049 | + "", "eth1", "net1", |
| 1050 | + nil, |
| 1051 | + `cannot add network interface "eth1" to machine "2": MAC address must be not empty`, |
| 1052 | +}, { |
| 1053 | + "invalid", "eth1", "net1", |
| 1054 | + nil, |
| 1055 | + `cannot add network interface "eth1" to machine "2": invalid MAC address: invalid`, |
| 1056 | +}, { |
| 1057 | + "aa:bb:cc:dd:ee:f0", "eth1", "net1", |
| 1058 | + nil, |
| 1059 | + `cannot add network interface "eth1" to machine "2": interface with MAC address "aa:bb:cc:dd:ee:f0" already exists`, |
| 1060 | +}, { |
| 1061 | + "aa:bb:cc:dd:ee:ff", "", "net1", |
| 1062 | + nil, |
| 1063 | + `cannot add network interface "" to machine "2": interface name must be not empty`, |
| 1064 | +}, { |
| 1065 | + "aa:bb:cc:dd:ee:ff", "eth0", "net1", |
| 1066 | + nil, |
| 1067 | + `cannot add network interface "eth0" to machine "2": "eth0" on machine "2" already exists`, |
| 1068 | +}, { |
| 1069 | + "aa:bb:cc:dd:ee:ff", "eth1", "missing", |
| 1070 | + nil, |
| 1071 | + `cannot add network interface "eth1" to machine "2": network "missing" not found`, |
| 1072 | +}, { |
| 1073 | + "aa:bb:cc:dd:ee:f1", "eth1", "net1", |
| 1074 | + func(c *gc.C, m *state.Machine) { |
| 1075 | + c.Check(m.SetProvisioned("i-am", "fake_nonce", nil), gc.IsNil) |
| 1076 | + }, |
| 1077 | + `cannot add network interface "eth1" to machine "2": machine already provisioned: dynamic network interfaces not currently supported`, |
| 1078 | +}, { |
| 1079 | + "aa:bb:cc:dd:ee:f1", "eth1", "net1", |
| 1080 | + func(c *gc.C, m *state.Machine) { |
| 1081 | + c.Check(m.EnsureDead(), gc.IsNil) |
| 1082 | + }, |
| 1083 | + `cannot add network interface "eth1" to machine "2": machine is not alive`, |
| 1084 | +}, { |
| 1085 | + "aa:bb:cc:dd:ee:f1", "eth1", "net1", |
| 1086 | + func(c *gc.C, m *state.Machine) { |
| 1087 | + c.Check(m.Remove(), gc.IsNil) |
| 1088 | + }, |
| 1089 | + `cannot add network interface "eth1" to machine "2": machine 2 not found`, |
| 1090 | +}} |
| 1091 | + |
| 1092 | func (s *MachineSuite) TestAddNetworkInterfaceErrors(c *gc.C) { |
| 1093 | machine, err := s.State.AddOneMachine(state.MachineTemplate{ |
| 1094 | Series: "quantal", |
| 1095 | @@ -484,44 +534,26 @@ |
| 1096 | c.Assert(err, gc.IsNil) |
| 1097 | addNetworkAndInterface( |
| 1098 | c, s.State, machine, |
| 1099 | - "net1", "0.1.2.0/24", 0, |
| 1100 | + "net1", "provider-net1", "0.1.2.0/24", 0, |
| 1101 | "aa:bb:cc:dd:ee:f0", "eth0") |
| 1102 | |
| 1103 | - errorPrefix := fmt.Sprintf("cannot add network interface to machine %s: ", machine.Id()) |
| 1104 | - _, err = machine.AddNetworkInterface("invalid", "eth0", "net1") |
| 1105 | - expectErr := errorPrefix + "invalid MAC address: invalid" |
| 1106 | - c.Assert(err, gc.ErrorMatches, expectErr) |
| 1107 | - |
| 1108 | - _, err = machine.AddNetworkInterface("aa:bb:cc:dd:ee:ff", "", "net1") |
| 1109 | - expectErr = errorPrefix + "interface name must be not empty" |
| 1110 | - c.Assert(err, gc.ErrorMatches, expectErr) |
| 1111 | - |
| 1112 | - _, err = machine.AddNetworkInterface("aa:bb:cc:dd:ee:f0", "eth2", "net1") |
| 1113 | - expectErr = `interface with MAC address aa:bb:cc:dd:ee:f0 already exists` |
| 1114 | - c.Assert(err, gc.ErrorMatches, expectErr) |
| 1115 | - c.Assert(err, jc.Satisfies, errors.IsAlreadyExistsError) |
| 1116 | - |
| 1117 | - _, err = machine.AddNetworkInterface("aa:bb:cc:dd:ee:f0", "eth2", "invalid") |
| 1118 | - expectErr = errorPrefix + `network "invalid" not found` |
| 1119 | - c.Assert(err, gc.ErrorMatches, expectErr) |
| 1120 | - |
| 1121 | - err = machine.SetProvisioned("i-am", "fake_nonce", nil) |
| 1122 | - c.Assert(err, gc.IsNil) |
| 1123 | - _, err = machine.AddNetworkInterface("aa:bb:cc:dd:ee:ff", "eth2", "net1") |
| 1124 | - expectErr = errorPrefix + "machine already provisioned: dynamic network interfaces not currently supported" |
| 1125 | - c.Assert(err, gc.ErrorMatches, expectErr) |
| 1126 | - |
| 1127 | - err = machine.EnsureDead() |
| 1128 | - c.Assert(err, gc.IsNil) |
| 1129 | - _, err = machine.AddNetworkInterface("aa:bb:cc:dd:ee:ff", "eth2", "net1") |
| 1130 | - expectErr = errorPrefix + "machine is not alive" |
| 1131 | - c.Assert(err, gc.ErrorMatches, expectErr) |
| 1132 | - |
| 1133 | - err = machine.Remove() |
| 1134 | - c.Assert(err, gc.IsNil) |
| 1135 | - _, err = machine.AddNetworkInterface("aa:bb:cc:dd:ee:ff", "eth2", "net1") |
| 1136 | - expectErr = errorPrefix + fmt.Sprintf("machine %s not found", machine.Id()) |
| 1137 | - c.Assert(err, gc.ErrorMatches, expectErr) |
| 1138 | + for i, test := range addNetworkInterfaceErrorsTests { |
| 1139 | + c.Logf("test %d: mac=%q, name=%q, network=%q", |
| 1140 | + i, test.macAddress, test.interfaceName, test.networkName) |
| 1141 | + |
| 1142 | + if test.beforeAdding != nil { |
| 1143 | + test.beforeAdding(c, machine) |
| 1144 | + } |
| 1145 | + |
| 1146 | + _, err = machine.AddNetworkInterface(test.macAddress, test.interfaceName, test.networkName) |
| 1147 | + c.Check(err, gc.ErrorMatches, test.expectErr) |
| 1148 | + if strings.Contains(test.expectErr, "not found") { |
| 1149 | + c.Check(err, jc.Satisfies, errors.IsNotFoundError) |
| 1150 | + } |
| 1151 | + if strings.Contains(test.expectErr, "already exists") { |
| 1152 | + c.Check(err, jc.Satisfies, errors.IsAlreadyExistsError) |
| 1153 | + } |
| 1154 | + } |
| 1155 | } |
| 1156 | |
| 1157 | func (s *MachineSuite) TestMachineInstanceId(c *gc.C) { |
| 1158 | @@ -641,20 +673,20 @@ |
| 1159 | |
| 1160 | func (s *MachineSuite) TestMachineSetInstanceInfoFailureDoesNotProvision(c *gc.C) { |
| 1161 | c.Assert(s.machine.CheckProvisioned("fake_nonce"), gc.Equals, false) |
| 1162 | - invalidNetworks := []params.Network{{Name: ""}} |
| 1163 | - invalidInterfaces := []params.NetworkInterface{{MACAddress: ""}} |
| 1164 | + invalidNetworks := []state.NetworkInfo{{Name: ""}} |
| 1165 | + invalidInterfaces := []state.NetworkInterfaceInfo{{MACAddress: ""}} |
| 1166 | err := s.machine.SetInstanceInfo("umbrella/0", "fake_nonce", nil, invalidNetworks, nil) |
| 1167 | c.Assert(err, gc.ErrorMatches, `cannot add network "": name must be not empty`) |
| 1168 | c.Assert(s.machine.CheckProvisioned("fake_nonce"), gc.Equals, false) |
| 1169 | err = s.machine.SetInstanceInfo("umbrella/0", "fake_nonce", nil, nil, invalidInterfaces) |
| 1170 | - c.Assert(err, gc.ErrorMatches, "cannot add network interface to machine 1: invalid MAC address: ") |
| 1171 | + c.Assert(err, gc.ErrorMatches, `cannot add network interface "" to machine "1": MAC address must be not empty`) |
| 1172 | c.Assert(s.machine.CheckProvisioned("fake_nonce"), gc.Equals, false) |
| 1173 | } |
| 1174 | |
| 1175 | func (s *MachineSuite) TestMachineSetInstanceInfoSuccess(c *gc.C) { |
| 1176 | c.Assert(s.machine.CheckProvisioned("fake_nonce"), gc.Equals, false) |
| 1177 | - networks := []params.Network{{Name: "net1", CIDR: "0.1.2.0/24", VLANTag: 0}} |
| 1178 | - interfaces := []params.NetworkInterface{ |
| 1179 | + networks := []state.NetworkInfo{{Name: "net1", ProviderId: "net1", CIDR: "0.1.2.0/24", VLANTag: 0}} |
| 1180 | + interfaces := []state.NetworkInterfaceInfo{ |
| 1181 | {MACAddress: "aa:bb:cc:dd:ee:ff", NetworkName: "net1", InterfaceName: "eth0"}, |
| 1182 | } |
| 1183 | err := s.machine.SetInstanceInfo("umbrella/0", "fake_nonce", nil, networks, interfaces) |
| 1184 | @@ -663,6 +695,7 @@ |
| 1185 | network, err := s.State.Network(networks[0].Name) |
| 1186 | c.Assert(err, gc.IsNil) |
| 1187 | c.Check(network.Name(), gc.Equals, networks[0].Name) |
| 1188 | + c.Check(network.ProviderId(), gc.Equals, networks[0].ProviderId) |
| 1189 | c.Check(network.VLANTag(), gc.Equals, networks[0].VLANTag) |
| 1190 | c.Check(network.CIDR(), gc.Equals, networks[0].CIDR) |
| 1191 | ifaces, err := s.machine.NetworkInterfaces() |
| 1192 | @@ -671,7 +704,7 @@ |
| 1193 | c.Check(ifaces[0].InterfaceName(), gc.Equals, interfaces[0].InterfaceName) |
| 1194 | c.Check(ifaces[0].NetworkName(), gc.Equals, interfaces[0].NetworkName) |
| 1195 | c.Check(ifaces[0].MACAddress(), gc.Equals, interfaces[0].MACAddress) |
| 1196 | - c.Check(names.MachineTag(ifaces[0].MachineId()), gc.Equals, s.machine.Tag()) |
| 1197 | + c.Check(ifaces[0].MachineTag(), gc.Equals, s.machine.Tag()) |
| 1198 | } |
| 1199 | |
| 1200 | func (s *MachineSuite) TestMachineSetProvisionedWhenNotAlive(c *gc.C) { |
| 1201 | |
| 1202 | === modified file 'state/networkinterfaces.go' |
| 1203 | --- state/networkinterfaces.go 2014-04-04 11:04:48 +0000 |
| 1204 | +++ state/networkinterfaces.go 2014-04-13 09:27:07 +0000 |
| 1205 | @@ -3,6 +3,10 @@ |
| 1206 | |
| 1207 | package state |
| 1208 | |
| 1209 | +import ( |
| 1210 | + "launchpad.net/juju-core/names" |
| 1211 | +) |
| 1212 | + |
| 1213 | // NetworkInterface represents the state of a machine network |
| 1214 | // interface. |
| 1215 | type NetworkInterface struct { |
| 1216 | @@ -10,11 +14,29 @@ |
| 1217 | doc networkInterfaceDoc |
| 1218 | } |
| 1219 | |
| 1220 | +// NetworkInterfaceInfo describes a single network interface available |
| 1221 | +// on an instance. |
| 1222 | +type NetworkInterfaceInfo struct { |
| 1223 | + // MACAddress is the network interface's hardware MAC address |
| 1224 | + // (e.g. "aa:bb:cc:dd:ee:ff"). |
| 1225 | + MACAddress string |
| 1226 | + |
| 1227 | + // InterfaceName is the OS-specific network device name (e.g. |
| 1228 | + // "eth0" or "eth1.42" for a VLAN virtual interface). |
| 1229 | + InterfaceName string |
| 1230 | + |
| 1231 | + // NetworkName is this interface's network name. |
| 1232 | + NetworkName string |
| 1233 | +} |
| 1234 | + |
| 1235 | // networkInterfaceDoc represents a network interface for a machine on |
| 1236 | // a given network. |
| 1237 | +// |
| 1238 | +// TODO(dimitern) To allow multiple virtual (e.g. VLAN) interfaces on |
| 1239 | +// the same MAC address, we need to change the key and uniqueness |
| 1240 | +// constraints. |
| 1241 | type networkInterfaceDoc struct { |
| 1242 | - MACAddress string `bson:"_id"` |
| 1243 | - // InterfaceName is the network interface name (e.g. "eth0"). |
| 1244 | + MACAddress string `bson:"_id"` |
| 1245 | InterfaceName string |
| 1246 | NetworkName string |
| 1247 | MachineId string |
| 1248 | @@ -34,12 +56,22 @@ |
| 1249 | return ni.doc.InterfaceName |
| 1250 | } |
| 1251 | |
| 1252 | -// NetworkName returns the machine network name of the interface. |
| 1253 | +// NetworkId returns the network name of the interface. |
| 1254 | func (ni *NetworkInterface) NetworkName() string { |
| 1255 | return ni.doc.NetworkName |
| 1256 | } |
| 1257 | |
| 1258 | +// NetworkTag returns the network tag of the interface. |
| 1259 | +func (ni *NetworkInterface) NetworkTag() string { |
| 1260 | + return names.NetworkTag(ni.doc.NetworkName) |
| 1261 | +} |
| 1262 | + |
| 1263 | // MachineId returns the machine id of the interface. |
| 1264 | func (ni *NetworkInterface) MachineId() string { |
| 1265 | return ni.doc.MachineId |
| 1266 | } |
| 1267 | + |
| 1268 | +// MachineTag returns the machine tag of the interface. |
| 1269 | +func (ni *NetworkInterface) MachineTag() string { |
| 1270 | + return names.MachineTag(ni.doc.MachineId) |
| 1271 | +} |
| 1272 | |
| 1273 | === modified file 'state/networkinterfaces_test.go' |
| 1274 | --- state/networkinterfaces_test.go 2014-04-04 09:08:42 +0000 |
| 1275 | +++ state/networkinterfaces_test.go 2014-04-13 09:27:07 +0000 |
| 1276 | @@ -23,7 +23,7 @@ |
| 1277 | var err error |
| 1278 | s.machine, err = s.State.AddMachine("quantal", state.JobHostUnits) |
| 1279 | c.Assert(err, gc.IsNil) |
| 1280 | - s.network, err = s.State.AddNetwork("net1", "0.1.2.3/24", 42) |
| 1281 | + s.network, err = s.State.AddNetwork("net1", "net1", "0.1.2.3/24", 42) |
| 1282 | c.Assert(err, gc.IsNil) |
| 1283 | s.iface, err = s.machine.AddNetworkInterface("aa:bb:cc:dd:ee:ff", "eth0", "net1") |
| 1284 | c.Assert(err, gc.IsNil) |
| 1285 | @@ -33,5 +33,7 @@ |
| 1286 | c.Assert(s.iface.MACAddress(), gc.Equals, "aa:bb:cc:dd:ee:ff") |
| 1287 | c.Assert(s.iface.InterfaceName(), gc.Equals, "eth0") |
| 1288 | c.Assert(s.iface.NetworkName(), gc.Equals, s.network.Name()) |
| 1289 | + c.Assert(s.iface.NetworkTag(), gc.Equals, s.network.Tag()) |
| 1290 | c.Assert(s.iface.MachineId(), gc.Equals, s.machine.Id()) |
| 1291 | + c.Assert(s.iface.MachineTag(), gc.Equals, s.machine.Tag()) |
| 1292 | } |
| 1293 | |
| 1294 | === modified file 'state/networks.go' |
| 1295 | --- state/networks.go 2014-04-09 15:08:51 +0000 |
| 1296 | +++ state/networks.go 2014-04-13 09:27:07 +0000 |
| 1297 | @@ -5,6 +5,8 @@ |
| 1298 | |
| 1299 | import ( |
| 1300 | "labix.org/v2/mgo/bson" |
| 1301 | + |
| 1302 | + "launchpad.net/juju-core/names" |
| 1303 | ) |
| 1304 | |
| 1305 | // Network represents the state of a network. |
| 1306 | @@ -13,17 +15,32 @@ |
| 1307 | doc networkDoc |
| 1308 | } |
| 1309 | |
| 1310 | +// NetworkInfo describes a single network. |
| 1311 | +type NetworkInfo struct { |
| 1312 | + // Name is juju-internal name of the network. |
| 1313 | + Name string |
| 1314 | + |
| 1315 | + // ProviderId is a provider-specific network id. |
| 1316 | + ProviderId string |
| 1317 | + |
| 1318 | + // CIDR of the network, in 123.45.67.89/24 format. |
| 1319 | + CIDR string |
| 1320 | + |
| 1321 | + // VLANTag needs to be between 1 and 4094 for VLANs and 0 for |
| 1322 | + // normal networks. It's defined by IEEE 802.1Q standard. |
| 1323 | + VLANTag int |
| 1324 | +} |
| 1325 | + |
| 1326 | // networkDoc represents a configured network that a machine can be a |
| 1327 | // part of. |
| 1328 | type networkDoc struct { |
| 1329 | // Name is the network's name. It should be one of the machine's |
| 1330 | // included networks. |
| 1331 | Name string `bson:"_id"` |
| 1332 | - // CIDR holds the network CIDR in the form 192.168.100.0/24. |
| 1333 | - CIDR string |
| 1334 | - // VLANTag needs to be between 1 and 4094 for VLANs and 0 for |
| 1335 | - // normal networks. |
| 1336 | - VLANTag int |
| 1337 | + |
| 1338 | + ProviderId string |
| 1339 | + CIDR string |
| 1340 | + VLANTag int |
| 1341 | } |
| 1342 | |
| 1343 | func newNetwork(st *State, doc *networkDoc) *Network { |
| 1344 | @@ -35,6 +52,16 @@ |
| 1345 | return n.doc.Name |
| 1346 | } |
| 1347 | |
| 1348 | +// ProviderId returns the provider-specific id of the network. |
| 1349 | +func (n *Network) ProviderId() string { |
| 1350 | + return n.doc.ProviderId |
| 1351 | +} |
| 1352 | + |
| 1353 | +// Tag returns the network tag. |
| 1354 | +func (n *Network) Tag() string { |
| 1355 | + return names.NetworkTag(n.doc.Name) |
| 1356 | +} |
| 1357 | + |
| 1358 | // CIDR returns the network CIDR (e.g. 192.168.50.0/24). |
| 1359 | func (n *Network) CIDR() string { |
| 1360 | return n.doc.CIDR |
| 1361 | |
| 1362 | === modified file 'state/networks_test.go' |
| 1363 | --- state/networks_test.go 2014-04-04 09:08:42 +0000 |
| 1364 | +++ state/networks_test.go 2014-04-13 09:27:07 +0000 |
| 1365 | @@ -24,14 +24,16 @@ |
| 1366 | var err error |
| 1367 | s.machine, err = s.State.AddMachine("quantal", state.JobHostUnits) |
| 1368 | c.Assert(err, gc.IsNil) |
| 1369 | - s.network, err = s.State.AddNetwork("net1", "0.1.2.3/24", 0) |
| 1370 | + s.network, err = s.State.AddNetwork("net1", "net1", "0.1.2.3/24", 0) |
| 1371 | c.Assert(err, gc.IsNil) |
| 1372 | - s.vlan, err = s.State.AddNetwork("vlan", "0.1.2.3/30", 42) |
| 1373 | + s.vlan, err = s.State.AddNetwork("vlan", "vlan", "0.1.2.3/30", 42) |
| 1374 | c.Assert(err, gc.IsNil) |
| 1375 | } |
| 1376 | |
| 1377 | func (s *NetworkSuite) TestGetterMethods(c *gc.C) { |
| 1378 | c.Assert(s.network.Name(), gc.Equals, "net1") |
| 1379 | + c.Assert(s.network.ProviderId(), gc.Equals, "net1") |
| 1380 | + c.Assert(s.network.Tag(), gc.Equals, "network-net1") |
| 1381 | c.Assert(s.network.CIDR(), gc.Equals, "0.1.2.3/24") |
| 1382 | c.Assert(s.network.VLANTag(), gc.Equals, 0) |
| 1383 | c.Assert(s.vlan.VLANTag(), gc.Equals, 42) |
| 1384 | |
| 1385 | === modified file 'state/open.go' |
| 1386 | --- state/open.go 2014-04-11 17:51:58 +0000 |
| 1387 | +++ state/open.go 2014-04-13 09:27:07 +0000 |
| 1388 | @@ -196,18 +196,21 @@ |
| 1389 | var indexes = []struct { |
| 1390 | collection string |
| 1391 | key []string |
| 1392 | + unique bool |
| 1393 | }{ |
| 1394 | // After the first public release, do not remove entries from here |
| 1395 | // without adding them to a list of indexes to drop, to ensure |
| 1396 | // old databases are modified to have the correct indexes. |
| 1397 | - {"relations", []string{"endpoints.relationname"}}, |
| 1398 | - {"relations", []string{"endpoints.servicename"}}, |
| 1399 | - {"units", []string{"service"}}, |
| 1400 | - {"units", []string{"principal"}}, |
| 1401 | - {"units", []string{"machineid"}}, |
| 1402 | - {"users", []string{"name"}}, |
| 1403 | - {"networkinterfaces", []string{"networkname"}}, |
| 1404 | - {"networkinterfaces", []string{"machineid"}}, |
| 1405 | + {"relations", []string{"endpoints.relationname"}, false}, |
| 1406 | + {"relations", []string{"endpoints.servicename"}, false}, |
| 1407 | + {"units", []string{"service"}, false}, |
| 1408 | + {"units", []string{"principal"}, false}, |
| 1409 | + {"units", []string{"machineid"}, false}, |
| 1410 | + {"users", []string{"name"}, false}, |
| 1411 | + {"networks", []string{"providerid"}, true}, |
| 1412 | + {"networkinterfaces", []string{"interfacename", "machineid"}, true}, |
| 1413 | + {"networkinterfaces", []string{"networkname"}, false}, |
| 1414 | + {"networkinterfaces", []string{"machineid"}, false}, |
| 1415 | } |
| 1416 | |
| 1417 | // The capped collection used for transaction logs defaults to 10MB. |
| 1418 | @@ -278,7 +281,7 @@ |
| 1419 | relations: db.C("relations"), |
| 1420 | relationScopes: db.C("relationscopes"), |
| 1421 | services: db.C("services"), |
| 1422 | - requestedNetworks: db.C("linkednetworks"), |
| 1423 | + requestedNetworks: db.C("requestednetworks"), |
| 1424 | networks: db.C("networks"), |
| 1425 | networkInterfaces: db.C("networkinterfaces"), |
| 1426 | minUnits: db.C("minunits"), |
| 1427 | @@ -306,7 +309,7 @@ |
| 1428 | st.watcher = watcher.New(db.C("txns.log")) |
| 1429 | st.pwatcher = presence.NewWatcher(pdb.C("presence")) |
| 1430 | for _, item := range indexes { |
| 1431 | - index := mgo.Index{Key: item.key} |
| 1432 | + index := mgo.Index{Key: item.key, Unique: item.unique} |
| 1433 | if err := db.C(item.collection).EnsureIndex(index); err != nil { |
| 1434 | return nil, fmt.Errorf("cannot create database index: %v", err) |
| 1435 | } |
| 1436 | |
| 1437 | === modified file 'state/state.go' |
| 1438 | --- state/state.go 2014-04-09 15:48:45 +0000 |
| 1439 | +++ state/state.go 2014-04-13 09:27:07 +0000 |
| 1440 | @@ -467,6 +467,8 @@ |
| 1441 | return env, nil |
| 1442 | case names.RelationTagKind: |
| 1443 | return st.KeyRelation(id) |
| 1444 | + case names.NetworkTagKind: |
| 1445 | + return st.Network(id) |
| 1446 | } |
| 1447 | return nil, err |
| 1448 | } |
| 1449 | @@ -491,6 +493,8 @@ |
| 1450 | coll = st.relations.Name |
| 1451 | case names.EnvironTagKind: |
| 1452 | coll = st.environments.Name |
| 1453 | + case names.NetworkTagKind: |
| 1454 | + coll = st.networks.Name |
| 1455 | default: |
| 1456 | return "", "", fmt.Errorf("%q is not a valid collection tag", tag) |
| 1457 | } |
| 1458 | @@ -1010,15 +1014,12 @@ |
| 1459 | return svc, nil |
| 1460 | } |
| 1461 | |
| 1462 | -// AddNetwork creates a new network with the given name, CIDR and VLAN |
| 1463 | -// tag. If a network with the same name already exists in state, an |
| 1464 | -// error satisfying errors.IsAlreadyExistsError is returned. |
| 1465 | -func (st *State) AddNetwork(name, cidr string, vlanTag int) (n *Network, err error) { |
| 1466 | - defer func() { |
| 1467 | - if !errors.IsAlreadyExistsError(err) { |
| 1468 | - utils.ErrorContextf(&err, "cannot add network %q", name) |
| 1469 | - } |
| 1470 | - }() |
| 1471 | +// AddNetwork creates a new network with the given name, |
| 1472 | +// provider-specific id, CIDR and VLAN tag. If a network with the same |
| 1473 | +// name or provider id already exists in state, an error satisfying |
| 1474 | +// errors.IsAlreadyExistsError is returned. |
| 1475 | +func (st *State) AddNetwork(name, providerId, cidr string, vlanTag int) (n *Network, err error) { |
| 1476 | + defer utils.ErrorContextf(&err, "cannot add network %q", name) |
| 1477 | if cidr != "" { |
| 1478 | _, _, err := net.ParseCIDR(cidr) |
| 1479 | if err != nil { |
| 1480 | @@ -1028,13 +1029,20 @@ |
| 1481 | if name == "" { |
| 1482 | return nil, fmt.Errorf("name must be not empty") |
| 1483 | } |
| 1484 | + if !names.IsNetwork(name) { |
| 1485 | + return nil, fmt.Errorf("invalid name") |
| 1486 | + } |
| 1487 | + if providerId == "" { |
| 1488 | + return nil, fmt.Errorf("provider id must be not empty") |
| 1489 | + } |
| 1490 | if vlanTag < 0 || vlanTag > 4094 { |
| 1491 | return nil, fmt.Errorf("invalid VLAN tag %d: must be between 0 and 4094", vlanTag) |
| 1492 | } |
| 1493 | doc := &networkDoc{ |
| 1494 | - Name: name, |
| 1495 | - CIDR: cidr, |
| 1496 | - VLANTag: vlanTag, |
| 1497 | + Name: name, |
| 1498 | + ProviderId: providerId, |
| 1499 | + CIDR: cidr, |
| 1500 | + VLANTag: vlanTag, |
| 1501 | } |
| 1502 | ops := []txn.Op{{ |
| 1503 | C: st.networks.Name, |
| 1504 | @@ -1042,11 +1050,27 @@ |
| 1505 | Assert: txn.DocMissing, |
| 1506 | Insert: doc, |
| 1507 | }} |
| 1508 | - err = onAbort(st.runTransaction(ops), errors.NewAlreadyExistsError("network "+name)) |
| 1509 | - if err != nil { |
| 1510 | - return nil, err |
| 1511 | + err = st.runTransaction(ops) |
| 1512 | + switch err { |
| 1513 | + case txn.ErrAborted: |
| 1514 | + if _, err = st.Network(name); err == nil { |
| 1515 | + msg := fmt.Sprintf("network %q", name) |
| 1516 | + return nil, errors.NewAlreadyExistsError(msg) |
| 1517 | + } else if err != nil { |
| 1518 | + return nil, err |
| 1519 | + } |
| 1520 | + case nil: |
| 1521 | + // For some reason when using unique indices with mgo, and |
| 1522 | + // we have an index violation the error is nil, but the |
| 1523 | + // document is not added. So we check if the supposedly |
| 1524 | + // successful transaction did actually add the document. |
| 1525 | + if _, err = st.Network(name); err != nil { |
| 1526 | + msg := fmt.Sprintf("network with provider id %q", providerId) |
| 1527 | + return nil, errors.NewAlreadyExistsError(msg) |
| 1528 | + } |
| 1529 | + return newNetwork(st, doc), nil |
| 1530 | } |
| 1531 | - return newNetwork(st, doc), nil |
| 1532 | + return nil, err |
| 1533 | } |
| 1534 | |
| 1535 | // Network returns the network with the given name. |
| 1536 | @@ -1496,6 +1520,7 @@ |
| 1537 | 'u': names.UnitTagKind + "-", |
| 1538 | 'e': names.EnvironTagKind + "-", |
| 1539 | 'r': names.RelationTagKind + "-", |
| 1540 | + 'n': names.NetworkTagKind + "-", |
| 1541 | } |
| 1542 | |
| 1543 | func tagForGlobalKey(key string) (string, bool) { |
| 1544 | |
| 1545 | === modified file 'state/state_test.go' |
| 1546 | --- state/state_test.go 2014-04-08 10:20:15 +0000 |
| 1547 | +++ state/state_test.go 2014-04-13 09:27:07 +0000 |
| 1548 | @@ -8,6 +8,7 @@ |
| 1549 | "net/url" |
| 1550 | "sort" |
| 1551 | "strconv" |
| 1552 | + "strings" |
| 1553 | "time" |
| 1554 | |
| 1555 | jc "github.com/juju/testing/checkers" |
| 1556 | @@ -962,7 +963,39 @@ |
| 1557 | } |
| 1558 | } |
| 1559 | |
| 1560 | -func (s *StateSuite) TestAddAndGetNetwork(c *gc.C) { |
| 1561 | +var addNetworkErrorsTests = []struct { |
| 1562 | + name string |
| 1563 | + providerId string |
| 1564 | + cidr string |
| 1565 | + vlanTag int |
| 1566 | + expectErr string |
| 1567 | +}{{ |
| 1568 | + "", "provider-id", "0.3.1.0/24", 0, |
| 1569 | + `cannot add network "": name must be not empty`, |
| 1570 | +}, { |
| 1571 | + "-invalid-", "provider-id", "0.3.1.0/24", 0, |
| 1572 | + `cannot add network "-invalid-": invalid name`, |
| 1573 | +}, { |
| 1574 | + "net2", "", "0.3.1.0/24", 0, |
| 1575 | + `cannot add network "net2": provider id must be not empty`, |
| 1576 | +}, { |
| 1577 | + "net2", "provider-id", "invalid", 0, |
| 1578 | + `cannot add network "net2": invalid CIDR address: invalid`, |
| 1579 | +}, { |
| 1580 | + "net2", "provider-id", "0.3.1.0/24", -1, |
| 1581 | + `cannot add network "net2": invalid VLAN tag -1: must be between 0 and 4094`, |
| 1582 | +}, { |
| 1583 | + "net2", "provider-id", "0.3.1.0/24", 9999, |
| 1584 | + `cannot add network "net2": invalid VLAN tag 9999: must be between 0 and 4094`, |
| 1585 | +}, { |
| 1586 | + "net1", "provider-id", "0.3.1.0/24", 0, |
| 1587 | + `cannot add network "net1": network "net1" already exists`, |
| 1588 | +}, { |
| 1589 | + "net2", "provider-net1", "0.3.1.0/24", 0, |
| 1590 | + `cannot add network "net2": network with provider id "provider-net1" already exists`, |
| 1591 | +}} |
| 1592 | + |
| 1593 | +func (s *StateSuite) TestAddNetworkErrors(c *gc.C) { |
| 1594 | machine, err := s.State.AddOneMachine(state.MachineTemplate{ |
| 1595 | Series: "quantal", |
| 1596 | Jobs: []state.MachineJob{state.JobHostUnits}, |
| 1597 | @@ -973,32 +1006,27 @@ |
| 1598 | |
| 1599 | net1, _ := addNetworkAndInterface( |
| 1600 | c, s.State, machine, |
| 1601 | - "net1", "0.1.2.0/24", 0, |
| 1602 | + "net1", "provider-net1", "0.1.2.0/24", 0, |
| 1603 | "aa:bb:cc:dd:ee:f0", "eth0") |
| 1604 | |
| 1605 | net, err := s.State.Network("net1") |
| 1606 | c.Assert(err, gc.IsNil) |
| 1607 | c.Assert(net, gc.DeepEquals, net1) |
| 1608 | + c.Assert(net.Name(), gc.Equals, "net1") |
| 1609 | + c.Assert(net.ProviderId(), gc.Equals, "provider-net1") |
| 1610 | _, err = s.State.Network("missing") |
| 1611 | c.Assert(err, jc.Satisfies, errors.IsNotFoundError) |
| 1612 | c.Assert(err, gc.ErrorMatches, `network "missing" not found`) |
| 1613 | |
| 1614 | - _, err = s.State.AddNetwork("", "0.3.1.0/24", 0) |
| 1615 | - expectErr := `cannot add network "": name must be not empty` |
| 1616 | - c.Assert(err, gc.ErrorMatches, expectErr) |
| 1617 | - _, err = s.State.AddNetwork("net42", "invalid", 0) |
| 1618 | - expectErr = `cannot add network "net42": invalid CIDR address: invalid` |
| 1619 | - c.Assert(err, gc.ErrorMatches, expectErr) |
| 1620 | - _, err = s.State.AddNetwork("net69", "0.3.1.0/30", -1) |
| 1621 | - expectErr = `cannot add network "net69": invalid VLAN tag -1: must be between 0 and 4094` |
| 1622 | - c.Assert(err, gc.ErrorMatches, expectErr) |
| 1623 | - _, err = s.State.AddNetwork("net69", "0.3.1.0/30", 9999) |
| 1624 | - expectErr = `cannot add network "net69": invalid VLAN tag 9999: must be between 0 and 4094` |
| 1625 | - c.Assert(err, gc.ErrorMatches, expectErr) |
| 1626 | - _, err = s.State.AddNetwork("net1", "0.1.2.0/24", 0) |
| 1627 | - expectErr = `network net1 already exists` |
| 1628 | - c.Assert(err, gc.ErrorMatches, expectErr) |
| 1629 | - c.Assert(err, jc.Satisfies, errors.IsAlreadyExistsError) |
| 1630 | + for i, test := range addNetworkErrorsTests { |
| 1631 | + c.Logf("test %d: name=%q, providerId=%q, cidr=%q, vlanTag=%d", |
| 1632 | + i, test.name, test.providerId, test.cidr, test.vlanTag) |
| 1633 | + _, err := s.State.AddNetwork(test.name, test.providerId, test.cidr, test.vlanTag) |
| 1634 | + c.Check(err, gc.ErrorMatches, test.expectErr) |
| 1635 | + if strings.Contains(test.expectErr, "already exists") { |
| 1636 | + c.Check(err, jc.Satisfies, errors.IsAlreadyExistsError) |
| 1637 | + } |
| 1638 | + } |
| 1639 | } |
| 1640 | |
| 1641 | func (s *StateSuite) TestAddService(c *gc.C) { |
| 1642 | @@ -2208,6 +2236,14 @@ |
| 1643 | }, { |
| 1644 | tag: "user-arble", |
| 1645 | }, { |
| 1646 | + tag: "network-missing", |
| 1647 | + err: `network "missing" not found`, |
| 1648 | +}, { |
| 1649 | + tag: "network-", |
| 1650 | + err: `"network-" is not a valid network tag`, |
| 1651 | +}, { |
| 1652 | + tag: "network-net1", |
| 1653 | +}, { |
| 1654 | // TODO(axw) 2013-12-04 #1257587 |
| 1655 | // remove backwards compatibility for environment-tag; see state.go |
| 1656 | tag: "environment-notauuid", |
| 1657 | @@ -2224,6 +2260,7 @@ |
| 1658 | names.UnitTagKind: (*state.Unit)(nil), |
| 1659 | names.MachineTagKind: (*state.Machine)(nil), |
| 1660 | names.RelationTagKind: (*state.Relation)(nil), |
| 1661 | + names.NetworkTagKind: (*state.Network)(nil), |
| 1662 | } |
| 1663 | |
| 1664 | func (s *StateSuite) TestFindEntity(c *gc.C) { |
| 1665 | @@ -2240,6 +2277,10 @@ |
| 1666 | rel, err := s.State.AddRelation(eps...) |
| 1667 | c.Assert(err, gc.IsNil) |
| 1668 | c.Assert(rel.String(), gc.Equals, "wordpress:db ser-vice2:server") |
| 1669 | + net1, err := s.State.AddNetwork("net1", "provider-id", "0.1.2.0/24", 0) |
| 1670 | + c.Assert(err, gc.IsNil) |
| 1671 | + c.Assert(net1.Tag(), gc.Equals, "network-net1") |
| 1672 | + c.Assert(net1.ProviderId(), gc.Equals, "provider-id") |
| 1673 | |
| 1674 | // environment tag is dynamically generated |
| 1675 | env, err := s.State.Environment() |
| 1676 | @@ -2280,6 +2321,8 @@ |
| 1677 | "---", |
| 1678 | "foo-bar", |
| 1679 | "unit-foo", |
| 1680 | + "network", |
| 1681 | + "network-", |
| 1682 | } |
| 1683 | for _, name := range bad { |
| 1684 | c.Logf(name) |
| 1685 | @@ -2327,6 +2370,14 @@ |
| 1686 | c.Assert(coll, gc.Equals, "environments") |
| 1687 | c.Assert(id, gc.Equals, env.UUID()) |
| 1688 | c.Assert(err, gc.IsNil) |
| 1689 | + |
| 1690 | + // Parse a network name. |
| 1691 | + net1, err := s.State.AddNetwork("net1", "provider-id", "0.1.2.0/24", 0) |
| 1692 | + c.Assert(err, gc.IsNil) |
| 1693 | + coll, id, err = state.ParseTag(s.State, net1.Tag()) |
| 1694 | + c.Assert(coll, gc.Equals, "networks") |
| 1695 | + c.Assert(id, gc.Equals, net1.Name()) |
| 1696 | + c.Assert(err, gc.IsNil) |
| 1697 | } |
| 1698 | |
| 1699 | func (s *StateSuite) TestWatchCleanups(c *gc.C) { |
| 1700 | |
| 1701 | === modified file 'worker/provisioner/provisioner_task.go' |
| 1702 | --- worker/provisioner/provisioner_task.go 2014-04-10 08:29:34 +0000 |
| 1703 | +++ worker/provisioner/provisioner_task.go 2014-04-13 09:27:07 +0000 |
| 1704 | @@ -428,18 +428,20 @@ |
| 1705 | } |
| 1706 | visitedNetworks := set.NewStrings() |
| 1707 | for _, info := range networkInfo { |
| 1708 | - if !visitedNetworks.Contains(info.NetworkName) { |
| 1709 | + networkTag := names.NetworkTag(info.NetworkName) |
| 1710 | + if !visitedNetworks.Contains(info.NetworkId) { |
| 1711 | networks = append(networks, params.Network{ |
| 1712 | - Name: info.NetworkName, |
| 1713 | - CIDR: info.CIDR, |
| 1714 | - VLANTag: info.VLANTag, |
| 1715 | + Tag: networkTag, |
| 1716 | + ProviderId: info.NetworkId, |
| 1717 | + CIDR: info.CIDR, |
| 1718 | + VLANTag: info.VLANTag, |
| 1719 | }) |
| 1720 | - visitedNetworks.Add(info.NetworkName) |
| 1721 | + visitedNetworks.Add(info.NetworkId) |
| 1722 | } |
| 1723 | ifaces = append(ifaces, params.NetworkInterface{ |
| 1724 | InterfaceName: info.InterfaceName, |
| 1725 | MACAddress: info.MACAddress, |
| 1726 | - NetworkName: info.NetworkName, |
| 1727 | + NetworkTag: networkTag, |
| 1728 | }) |
| 1729 | } |
| 1730 | return networks, ifaces |
| 1731 | |
| 1732 | === modified file 'worker/provisioner/provisioner_test.go' |
| 1733 | --- worker/provisioner/provisioner_test.go 2014-04-10 08:29:34 +0000 |
| 1734 | +++ worker/provisioner/provisioner_test.go 2014-04-13 09:27:07 +0000 |
| 1735 | @@ -476,12 +476,14 @@ |
| 1736 | expectNetworkInfo := []environs.NetworkInfo{{ |
| 1737 | MACAddress: "aa:bb:cc:dd:ee:f0", |
| 1738 | InterfaceName: "eth0", |
| 1739 | + NetworkId: "net1", |
| 1740 | NetworkName: "net1", |
| 1741 | VLANTag: 0, |
| 1742 | CIDR: "0.1.2.0/24", |
| 1743 | }, { |
| 1744 | MACAddress: "aa:bb:cc:dd:ee:f1", |
| 1745 | InterfaceName: "eth1", |
| 1746 | + NetworkId: "net2", |
| 1747 | NetworkName: "net2", |
| 1748 | VLANTag: 1, |
| 1749 | CIDR: "0.2.2.0/24", |
| 1750 | @@ -518,7 +520,9 @@ |
| 1751 | includeNetworks := []string{"bad-net1"} |
| 1752 | // "bad-" prefix for networks causes dummy provider to report |
| 1753 | // invalid NetworkInfo. |
| 1754 | - expectNetworkInfo := []environs.NetworkInfo{{}} |
| 1755 | + expectNetworkInfo := []environs.NetworkInfo{ |
| 1756 | + {NetworkId: "bad-net1", NetworkName: "bad-net1", CIDR: "invalid"}, |
| 1757 | + } |
| 1758 | m, err := s.addMachineWithRequestedNetworks(includeNetworks, nil) |
| 1759 | c.Assert(err, gc.IsNil) |
| 1760 | inst := s.checkStartInstanceCustom( |
| 1761 | @@ -536,7 +540,7 @@ |
| 1762 | continue |
| 1763 | } |
| 1764 | c.Assert(status, gc.Equals, params.StatusError) |
| 1765 | - c.Assert(info, gc.Matches, `aborted instance "dummyenv-0": cannot add network "": name must be not empty`) |
| 1766 | + c.Assert(info, gc.Matches, `aborted instance "dummyenv-0": cannot add network "bad-net1": invalid CIDR address: invalid`) |
| 1767 | break |
| 1768 | } |
| 1769 | s.checkStopInstances(c, inst) |
Reviewers: mp+214962_
Message:
Please take a look.
Description:
various: Change NetworkName->Id, add network tags
Fixes #1304905: Changed NetworkName in state and API
to to NetworkId and NetworkTag, respectively. Added
support for network tags in names/ and in state/
(FindEntity, etc.).
Comments updated to better explain the difference
between juju-specific network ids and provider-
specific ones (at present both are the same, but
this will change).
Few drive-by fixes are included as well.
https:/
Requires:
https:/
(do not edit description out of merge proposal)
Please review this at https:/
Affected files (+372, -118 lines):
A [revision details]
M cmd/juju/deploy.go
M environs/broker.go
A names/network.go
A names/network_
M names/tag.go
M names/tag_test.go
M provider/
M provider/
M state/address.go
M state/api/
M state/api/
M state/api/
M state/apiserver
M state/apiserver
M state/apiserver
M state/apiserver
M state/machine.go
M state/machine_
M state/networkin
M state/networkin
M state/networks.go
M state/networks_
M state/state.go
M state/state_test.go
M worker/
M worker/