Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Track Machine Spaces #6690
Conversation
frobware
suggested changes
Dec 12, 2016
Generally LGTM as long as we understand the limitations of not handling multiple bridges/devices associated with each space.
| + err = s.machine.SetDevicesAddresses( | ||
| + state.LinkLayerDeviceAddress{ | ||
| + DeviceName: "eth0", | ||
| + CIDRAddress: "10.0.0.20/24", |
jameinel
Dec 13, 2016
Owner
So, in our experimentation, it does. AIUI 'bridgectl' defaults to doing exactly this. We should probably look into supporting both. Find the host device that has a 'parent' of the bridge that has the right information. I think we also need to be aware of using Provider level information for an interface in order to match up what Space the device is in, so that we don't have to give it an IP address (or even a Subnet).
As it stands, if it doesn't have a CIDR, we don't have a way to know what space it is in, because our only linkage is the Subnet.
jameinel
Dec 13, 2016
Owner
We, unfortunately, require a CIDR. If you try to strip the CIDR you get "illegal address" when CIDR is "".
Maybe we could avoid setting an address at all, but then we'll definitely not show up in the space. (maybe we don't care)
| +// parseDelimitedValues parses a slice of raw values coming from constraints | ||
| +// (Tags or Spaces). The result is split into two slices - positives and | ||
| +// negatives (prefixed with "^"). Empty values are ignored. | ||
| +func parseDelimitedValues(rawValues []string) (positives, negatives []string) { |
frobware
Dec 12, 2016
Contributor
Curious if we do this elsewhere and, if so, can it be shared/public?
jameinel
Dec 13, 2016
Owner
I just copied these 2 functions from the provider/maas directory, because it really doesn't belong there. It either belongs inside something to do with "constraints" (probably my preference), or something to do with "networking". Thoughts?
| return networkAddresses, nil | ||
| } | ||
| + | ||
| +// Should this be NetworkInterface rather than NetworkInfo? |
| +} | ||
| + | ||
| +// BridgeDevicesForSpaces takes a list of spaces, and returns the associated | ||
| +// bridge that should be used for that space. If no bridge has been created on |
frobware
Dec 12, 2016
Contributor
If we take a list of spaces, can we (possibly) return a list of bridgeS too?
jameinel
Dec 13, 2016
Owner
So we take a list of spaces, and return a map. We could return a list, but if we did, I'd rather put the space name into the object, so that you always know the context of each object. Having not used it in anger, I wasn't sure what would be best. Also, we should consider that we might change it to return lists of bridges/host devices for each space name, since we may end up with more than one host device in the same space.
jameinel
Dec 13, 2016
Owner
Fixed. Now it is LinkLayerDevicesForSpaces, and just returns a list for each space.
| + // one. | ||
| + if ni.BridgeDevice != nil { | ||
| + logger.Warningf("Found >1 bridge devices for the same space: %s vs %s for %s", | ||
| + ni.BridgeDevice.Name(), device.Name(), spaceName) |
frobware
Dec 12, 2016
Contributor
%q for space name. Makes it easier to read if the name has ascii(32) in it. :)
| + } else { | ||
| + // TODO(jam): What do do about bonds vs underlying devices, etc. | ||
| + if ni.HostDevice != nil { | ||
| + logger.Warningf("Found >1 host devices for the same space: %s vs %s for %s", |
frobware
Dec 12, 2016
Contributor
%q for space name. Makes it easier to read if the name has ascii(32) in it. :)
| + machine, err := s.State.AddMachine("quantal", state.JobHostUnits) | ||
| + spaces, err := machine.AllSpaces() | ||
| + c.Assert(err, jc.ErrorIsNil) | ||
| + c.Check(spaces.SortedValues(), gc.DeepEquals, []string{}) |
jameinel
Dec 13, 2016
Owner
I could have done:
c.Check(spaces, gc.DeepEquals, set.NewStrings())
But
a) consistency
b) easier to read a list of strings
c) usually better comparisons (easier to look at a mismatched list and figure out what is wrong)
| +} | ||
| + | ||
| +func (s *MachineSuite) TestAllSpacesEndpoints(c *gc.C) { | ||
| + _, err := s.State.AddSpace("space", "", nil, true) |
frobware
Dec 12, 2016
Contributor
A minor nit: it's nice to have the space names be concrete (e.g., dmz, db, internet), particularly as we later reference mysql. I think it also helps people understand spaces when seen in conjunction with AddServiceWithBindings(<space-name>).
|
!!build!! |
|
!!build!! y'all |
|
Responded to what we talked about today. |
|
!!build!! |
jameinel
changed the title from
Track Machine spaces (WIP)
to
Track Machine Spaces
Dec 15, 2016
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
jameinel commentedDec 9, 2016
•
Edited 1 time
-
jameinel
Dec 15, 2016
This provides 2 key functions on Machine that I think will be necessary for our work on changing how containers are getting network devices on their host machine's bridges.
Machine.AllSpaces() combines the spaces that are specified in constraints with the endpoint bindings that are part of the active primary units on the machine.
Machine.LinkLayerDevicesForSpaces() filter's the link layer devices we know about for the machine, and sorts them into what space they are associated with. Between the two functions we are able to map the desired spaces for a new container onto the host's devices. This both lets us know if we have a bridge to use, and lets us create network devices only for the right ones.