Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Add NetworkInfo interface as a replacement for NetworkConfig #7314
Conversation
wupeka
added some commits
Apr 27, 2017
|
!!test!! |
jameinel
requested changes
May 11, 2017
Overall I think the direction is a good one. The API interfaces, etc, all look good. I had some comments, though most of them are stylistic to conform to common patterns.
| @@ -433,3 +434,31 @@ func mergeObservedAndProviderAddressConfig(observedConfig, providerConfig params | ||
| return finalConfig | ||
| } | ||
| + | ||
| +func networkNetworkInfoToNetworkInfo(info network.NetworkInfo) params.NetworkInfo { |
| @@ -662,3 +663,38 @@ type ProxyConfigResult struct { | ||
| type ProxyConfigResults struct { | ||
| Results []ProxyConfigResult `json:"results"` | ||
| } | ||
| + | ||
| +// InterfaceAddress represents the single address attached to the interface |
jameinel
May 11, 2017
Owner
InterfaceAddress represents a single address attached to the interface.
I think its 'a' as the interface can have more than one address. And comments should be full sentences with a stop at the end.
| + CIDR string `json:"cidr"` | ||
| +} | ||
| + | ||
| +// NetworkInfo describes one interface with IP addresses |
| + // "eth1", even for a VLAN eth1.42 virtual interface). | ||
| + InterfaceName string `json:"interface-name"` | ||
| + | ||
| + // Addresses contains a list of addresses configured on the interface |
| + Addresses []InterfaceAddress `json:"addresses"` | ||
| +} | ||
| + | ||
| +type NetworkInfoResult struct { |
| + Info []NetworkInfo `json:"network-info"` | ||
| +} | ||
| + | ||
| +// NetworkInfoResult holds a mapping from binding name to NetworkInfoResult |
| + Results map[string]NetworkInfoResult `json:"results"` | ||
| +} | ||
| + | ||
| +type NetworkInfoParams struct { |
| @@ -27,8 +28,8 @@ import ( | ||
| var logger = loggo.GetLogger("juju.apiserver.uniter") | ||
| -// UniterAPIV3 implements the API version 3, used by the uniter worker. | ||
| -type UniterAPIV3 struct { | ||
| +// UniterAPI implements the API version 6, used by the uniter worker. |
| @@ -1689,11 +1707,75 @@ func (u *UniterAPIV3) getOneNetworkConfig(canAccess common.AuthFunc, unitTagArg, | ||
| } | ||
| // SLALevel returns the model's SLA level. | ||
| -func (u *UniterAPIV3) SLALevel() (params.StringResult, error) { | ||
| +func (u *UniterAPI) SLALevel() (params.StringResult, error) { | ||
| result := params.StringResult{} | ||
| sla, err := u.st.SLALevel() | ||
| if err == nil { | ||
| result.Result = sla | ||
| } | ||
| return result, err | ||
| } |
jameinel
May 11, 2017
Owner
I assume everything above this point is just mechanical renames from a quick glance. If I missed something, let me know.
wupeka
May 12, 2017
Member
That is correct, the only thing standing out is with NetworkConfig stuff that was 'left' in UniterAPIV3
| +// NetworkInfo returns network interfaces/addresses for specified bindings. | ||
| +func (u *UniterAPI) NetworkInfo(args params.NetworkInfoParams) (params.NetworkInfoResults, error) { | ||
| + canAccess, err := u.accessUnit() | ||
| + if err != nil { |
jameinel
May 11, 2017
Owner
I definitely miss Python at times like this.... :)
Apparently Rust has a nice syntax for chaining "if any of these have an error, bubble it up". (try! and ?)
It is still explicit what parts could return an error, but much less actual content devoted to it.
Then again, lots of magic characters are something that Go is definitely against.
Digression aside this is all fine. :)
| + Results: make(map[string]params.NetworkInfoResult), | ||
| + } | ||
| + | ||
| + spaces := make(map[string]struct{}) |
jameinel
May 11, 2017
Owner
given this is a map[string]struct{} I think you can use juju/utils/set set.NewStrings() and then set.Add(), set.Values() rather than inlining all of that code here.
| + spacesList = append(spacesList, space) | ||
| + } | ||
| + | ||
| + networkInfos := machine.GetNetworkInfoForSpaces(spacesList) |
jameinel
May 11, 2017
Owner
I'm ok with a slice of space names, would it be better as a set of space names? (you can always set.NewStrings(...[]slice) or set.Values() to do the reverse.
Just trying to get a feel for what might be more natural for interactions.
| c.Assert(err, jc.ErrorIsNil) | ||
| c.Assert(result, jc.DeepEquals, params.UnitNetworkConfigResults{ | ||
| Results: []params.UnitNetworkConfigResult{ | ||
| {Config: expectedConfig}, | ||
| }, | ||
| }) | ||
| } | ||
| + | ||
| +type uniterNetworkInfoSuite struct { | ||
| + base uniterSuite |
jameinel
May 11, 2017
Owner
a comment here that we are not doing embedding because we just want some of the methods, but not to run the entire test suite again is probably warranted.
Ideally we would pull out the common code into a base/mixin and then embed it into both, but this is liveable.
wupeka
May 12, 2017
Member
It's completely based on NetworkConfig suite, I'll add a ticket to clean up the whole uniter_test.
| +} | ||
| + | ||
| +func (s *uniterNetworkInfoSuite) SetUpTest(c *gc.C) { | ||
| + s.base.JujuConnSuite.SetUpTest(c) |
jameinel
May 11, 2017
Owner
there isn't a base.SetUpTest() we should be calling instead? or is that setting up too much?
wupeka
May 15, 2017
Member
It's completely based on NetworkConfig suite, I'll add a ticket to clean up the whole uniter_test.
| +} | ||
| + | ||
| +func (s *uniterNetworkInfoSuite) makeMachineDevicesAndAddressesArgs(addrSuffix int) ([]state.LinkLayerDeviceArgs, []state.LinkLayerDeviceAddress) { | ||
| + return []state.LinkLayerDeviceArgs{{ |
jameinel
May 11, 2017
Owner
this seems odd that we wouldn't have different MACAddresses for the different machines that we are creating.
I realize it may not matter here, just feels weird to create multiple machines and not change the mac.
| +func (s *uniterNetworkInfoSuite) TestNetworkInfoPermissions(c *gc.C) { | ||
| + s.addRelationAndAssertInScope(c) | ||
| + | ||
| + args := []params.NetworkInfoParams{ |
jameinel
May 11, 2017
Owner
would this be better as more of a table based test that put the args next to the results, next to the errors?
Something like:
var tests []struct {
Arg params.NetworkInfoParams,
Result *params.NetworkInfoResults,
Error string,
} { {
Arg: params.NetworkInfoParams{Unit: ..., Bindings},
Error: "permission denied",
}, {
...
}
The way you have it, it isn't terrible to figure out what maps to what (the count is only 4, and most are only a single line long), but any bigger and its much easier to correlate if they are next to each other.
It also lets you have a "Description" or "Name" field for each test that helps give context.
| + } | ||
| + | ||
| + for i, arg := range args { | ||
| + result, err := s.base.uniter.NetworkInfo(arg) |
jameinel
May 11, 2017
Owner
At the very least you need a c.Logf("test %d", i)
So that if one of these fails, you can figure out what part of the list you are on. If you do it as a table test, then you would also log the name/description of each step.
| +} | ||
| + | ||
| +func (s *uniterNetworkInfoSuite) addRelationAndAssertInScope(c *gc.C) { | ||
| + // Add a relation between wordpress and mysql and enter scope with |
jameinel
May 11, 2017
Owner
nit: It feels a little weird to have this after one test, but before the other. I generally prefer to group helper functions at the start of the suite.
| + Bindings: []string{"db", "admin-api", "db-client"}, | ||
| + } | ||
| + // For the relation "wordpress:db mysql:server" we expect to see only | ||
| + // ifaces ibn the "internal" space, where the "db" endpoint itself |
| + | ||
| + result, err := s.base.uniter.NetworkInfo(args) | ||
| + c.Assert(err, jc.ErrorIsNil) | ||
| + c.Assert(result, jc.DeepEquals, params.NetworkInfoResults{ |
jameinel
May 11, 2017
Owner
another side topic. Generally for things that aren't fatal, using c.Check allows it to continue so that you can get all the errors at once, rather than 'oh that changed, fix it, run the test, the next one failed, fix it, run the test, next one failed', etc.
We generally do use c.Assert(err, ...) because usually if err is not nil, then 'result' will be nil, or some other issue that would cause a panic, etc, and its better to stop a test with a clear error than have a giant panic traceback and can't find the failure.
It obviously doesn't matter when there is only 1 Check, but its usually good to get in the habit of using Check when it makes sense.
| +} | ||
| + | ||
| +func (s *uniterNetworkInfoSuite) TestNetworkInfoL2Binding(c *gc.C) { | ||
| + c.Skip("L2 not supported yet") |
jameinel
May 11, 2017
Owner
damn, I was so hopeful with the test set up. :) I was looking forward to seeing this working.
| +func (s *uniterNetworkInfoSuite) TestNetworkInfoForImplicitlyBoundEndpoint(c *gc.C) { | ||
| + // Since wordpressUnit has explicit binding for "db", switch the API to | ||
| + // mysqlUnit and check "mysql:server" uses the machine preferred private | ||
| + // address. |
jameinel
May 11, 2017
Owner
so this is for a unit of an application without a default binding that we fall back to whatever we've decided is a machines preferred private address?
wupeka
May 12, 2017
Member
Yes, that is a fallback that was there and I don't want to change the behaviour.
| @@ -282,6 +282,26 @@ type Route struct { | ||
| Metric int | ||
| } | ||
| +// InterfaceAddress represents the single address attached to the interface |
| + CIDR string | ||
| +} | ||
| + | ||
| +// NetworkInfo describes one interface with assigned IP addresses, it's a mirror of params.NetworkInfo |
| @@ -1120,3 +1120,95 @@ func generateMACAddress() string { | ||
| } | ||
| return fmt.Sprintf(macAddressTemplate, digits...) | ||
| } | ||
| + | ||
| +type MachineNetworkInfoResult struct { |
| + Error *error | ||
| +} | ||
| + | ||
| +// Add address to a device in list or create a new device with this address |
| + } | ||
| + for i := range networkInfos { | ||
| + networkInfo := &networkInfos[i] | ||
| + if networkInfo.InterfaceName == address.DeviceName() { |
jameinel
May 11, 2017
Owner
Given the comment on InterfaceName, does this do weird thing with VLAN objects? Both the underlying device and the vlan both have name "eth0"? thus we might add the address to either one?
Or is the comment on InterfaceName wrong?
wupeka
May 12, 2017
Member
Interface name for vlan device is different than for vlan0 device - it has ".vlanid" appended.
jameinel
May 14, 2017
Owner
can you make sure the comments on the NetworkInfo struct are correct, then?
| + MAC := "" | ||
| + if device, err := address.Device(); err == nil { | ||
| + MAC = device.MACAddress() | ||
| + } |
jameinel
May 11, 2017
Owner
You're suppressing all errors here, even things like "database disconnected". Can you trap for just NotFound or some other specific set of errors that you are expecting?
It does mean that the interface to this function would need to return an 'error' and possibly bubble up the result.
| + return append(networkInfos, networkInfo) | ||
| +} | ||
| + | ||
| +// For each space in spaces this function returns MachineNetworkInfoResult with a list of devices in this space |
jameinel
May 11, 2017
Owner
comments on public functions are supposed to start with the name of the function by convention (some sort of doc tooling that works nicely if you do so.)
eg:
GetNetworkInfoForSpaces iterates the list of spaces and returns a MachineNetworkInforResult for all devices in each space.
| + | ||
| + var privateAddress network.Address | ||
| + | ||
| + if spacesmap[""] { |
jameinel
May 11, 2017
Owner
Needs some thought about how we handle "". Ultimately we may have multiple devices that haven't been labeled into explicit spaces. PrivateAddress is one of the answers, but I'm not sure it is the only one.
Maybe we need it for compatibility
In FindMissingBridgesFor... we use different logic around trying to find the device to use when asked for a space named "". I'd really like us to be consistent in how we handle "" as a space if possible.
| + case errors.IsNotFound(err): | ||
| + logger.Debugf("skipping %s: not linked to a known subnet (%v)", addr, err) | ||
| + case err != nil: | ||
| + logger.Errorf("cannot get subnet for address %q", addr) |
jameinel
May 11, 2017
Owner
we should definitely be logging the error as well, and I'm concerned that we're suppressing all errors, vs only ones like NotFound that we assume are ok to continue on.
| + defaultErrorResult := MachineNetworkInfoResult{Error: &defaultError} | ||
| + for _, space := range spaces { | ||
| + if _, ok := results[space]; !ok { | ||
| + results[space] = defaultErrorResult |
jameinel
May 11, 2017
Owner
given these are the errors that might bubble up to a user, or end up in a log for a improperly written charm, etc, it would probably be better to include a bit more context:
errors.Errorf("machine %q has no devices in space %q, only spaces %s", m.doc.Id, space, actualSpaces)
something along those lines.
short term you could leave off the actual spaces, though it is often useful.
| @@ -2472,3 +2472,24 @@ func (g *HistoryGetter) StatusHistory(filter status.StatusHistoryFilter) ([]stat | ||
| } | ||
| return statusHistory(args) | ||
| } | ||
| + | ||
| +func (u *Unit) GetSpaceForBinding(bindingName string) (string, error) { | ||
| + service, err := u.Application() |
jameinel
May 11, 2017
Owner
'service' is an old name, variables should prefer "app" or "application". (We renamed Service => Application as part of the 2.0 change, and did a decent job on public methods/API but didn't get all variables renamed internally)
| @@ -836,3 +836,8 @@ func (ctx *HookContext) SetUnitWorkloadVersion(version string) error { | ||
| } | ||
| return result.OneError() | ||
| } | ||
| + | ||
| +// NetworkConfig returns the network config for the given bindingName. |
| -supported flag for now is --primary-address, which is required and returns | ||
| -the IP address the local unit should advertise as its endpoint to its peers. | ||
| +network-get returns the network config for a given binding name. By default | ||
| +it returns the list of interfaces with addresses in the bindings space. |
jameinel
May 11, 2017
Owner
it returns the list of interfaces and associated addresses in the space for the binding.
or:
in the bound space.
or?
| - if len(netConfig) < 1 { | ||
| + | ||
| + ni, ok := netInfo[c.bindingName] | ||
| + |
| + | ||
| + ni, ok := netInfo[c.bindingName] | ||
| + | ||
| + if !ok || len(ni.Info) == 0 { | ||
| return fmt.Errorf("no network config found for binding %q", c.bindingName) |
| + summary: "unknown binding given, no --primary-address given", | ||
| + code: 1, | ||
| + args: []string{"unknown"}, | ||
| + out: `no network config found for binding "unknown"`, |
jameinel
May 11, 2017
Owner
'config' ?
maybe we shouldn't change error strings at this point, but something to consider.
| summary: "explicitly bound, extra-binding name given with --primary-address", | ||
| args: []string{"known-extra", "--primary-address"}, | ||
| out: "10.20.1.42", | ||
| }, { | ||
| + summary: "explicitly bound, extra-binding name given without --primary-address", | ||
| + args: []string{"known-extra"}, | ||
| + out: `- macaddress: "00:11:22:33:44:22" |
jameinel
May 11, 2017
Owner
given we probably want to support "network-get --format=json" (at least as an option), I think we need to be careful having a top level slice instead of a top level object.
Google says you can start JSON with a [], is it valid in YAML as well? For some reason I thought one of the standards didn't allow it. maybe it is BSON.
yamllint seems ok with this format, though it does change the actual representation to:
---
-
addresses:
-
address: "10.20.1.42"
cidr: 10.20.1.42/24
-
address: "fc00::1"
cidr: "fc00::/64`"
interfacename: eth2
macaddress: "00:11:22:33:44:22"
I think that's just reordering alphabetically, but not changing the 'arrays of objects'.
I don't think we have to implement --format=json in this PR, but I do think hooks are as likely to want JSON as they would want YAML so we should make sure the structures we are using are supported both ways. (make sure whatever JSON we output is valid for 'jq' to parse.)
wupeka
May 12, 2017
Member
Done, although I don't know if that's the right way to do it. Shouldn't 'omitempty' for error in NetworkInfoResult cause it not to be printed if it's nil?
| + cidr: 10.20.1.42/24 | ||
| + - address: fc00::1 | ||
| + cidr: fc00::/64`, | ||
| + }, { | ||
| summary: "explicitly bound relation name given with --primary-address", | ||
| args: []string{"known-relation", "--primary-address"}, | ||
| out: "10.10.0.23", |
jameinel
May 11, 2017
Owner
we should be a little careful with --primary-address when a given space has multiple devices/addresses associated with it.
there have been concerns about stability, and why some units end up with addresses that are in different subnets to other units just because of whatever iteration over a map we did.
we should try to make sure we have some sort of sorting when it comes to multiple addresses in a space.
wupeka
May 12, 2017
Member
The order is given by mongo - we're iterating over Addresses, and I believe this is stable. It's behaving as before, do we want to change it somehow? (order interfaces by name, order addresses on it?)
I added a note that we have to keep this order when we switch to L2-compatible method.
jameinel
May 13, 2017
Owner
We don't have to change it at this point, but there have been reported problems with our existing logic.
It isn't that we would give different values for the same unit. But that unit/0 ends up giving the IP for eth0, and unit/1 gives the IP for eth1, etc.
| summary: "implicitly bound binding name given with --primary-address", | ||
| args: []string{"known-unbound", "--primary-address"}, | ||
| - out: "10.33.1.8", // preferred private address used for unspecified bindings. | ||
| + out: "10.33.1.8", |
jameinel
May 11, 2017
Owner
'implicitly bound binding'? I wonder if this would be clearer as
'no user requested binding falls back to the machines private address'
|
!!test!! |
jameinel
approved these changes
May 16, 2017
I wasn't quite as thorough this time, so if there is anything you feel uncomfortable about, or want me to focus on, please ask.
Overall it looks good. I have one small tweak (network.QuoteSpaces) but LGTM.
| // NetworkConfig returns information about all given relation/unit pairs, | ||
| // including their id, key and the local endpoint. | ||
| +// It's not included in APIv6 | ||
| func (u *UniterAPIV3) NetworkConfig(args params.UnitsNetworkConfig) (params.UnitNetworkConfigResults, error) { |
jameinel
May 16, 2017
Owner
Don't you also need to create a
func (u *UniterAPIV3) NetworkInfo(InvalidParameters, and stuff like that)
So that we don't expose NetworkInfo in the v3/4/5 APIs?
Do we care?
wupeka
May 16, 2017
Member
We care and it's there:
// Mask it from old version
func (u *UniterAPIV3) NetworkInfo(_, _ struct{}) {}
| + } | ||
| + } | ||
| + | ||
| + actualSpacesStr := strings.Join(actualSpaces.Values(), ",") |
jameinel
May 16, 2017
Owner
there is network.QuoteSpaces which produces output that is a bit nicer when you have a list of possible spaces which might contain and empty string space name.
wupeka
added some commits
May 16, 2017
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
wupeka commentedMay 8, 2017
Description of change
QA steps
For network-get - launch an app with binding X, check that network-get X returns whole set of information (interface name, MAC, IP addresses)
For unit-get private-address:
juju add-machine --constraints "spaces=space1,space2"
juju deploy foobar --to 0 --bind space1
Documentation changes
Bug reference
This fixes https://bugs.launchpad.net/juju/+bug/1680100