Issue 98600047: api: new Networker API facade (Closed)
|
Created:
9 years, 11 months ago by vladislav.klyachin Modified:
9 years, 11 months ago Visibility:
Public. |
Descriptionapi: new Networker API facade
The original task was:
"new Networker API facade (client/server) with ListVLANsForMachine API call"
The additional tas was:
"Networker API" should do all network setup staff done by cloud-init
But all thing are changing and now Networker API has the only method calld
MachineNetworkInfo(machineTag string) ([]network.Info, error).
https://code.launchpad.net/~klyachin/juju-core/118-networker-facade/+merge/221180
(do not edit description out of merge proposal)
Patch Set 1 #
Total comments: 44
Patch Set 2 : api: new Networker API facade #
Total comments: 48
Patch Set 3 : api: new Networker API facade #Patch Set 4 : api: new Networker API facade #
MessagesTotal messages: 16
Please take a look.
Sign in to reply to this message.
I think this is an interesting start, but it sounds like the API we are writing is more generic than just VLANs, so we should name it appropriately. Also, I think we really do want it to be a "give me the information about all these machines" sort of API, since we have several places where we could use it. (We may not today, but it still is possible, and better to have it and not use it than need it and have to break compatibility.) We're also missing tests for the failure modes (requesting access to an API you don't have permission for.) https://codereview.appspot.com/98600047/diff/1/state/apiserver/networker/netw... File state/apiserver/networker/networker.go (right): https://codereview.appspot.com/98600047/diff/1/state/apiserver/networker/netw... state/apiserver/networker/networker.go:118: func (n *NetworkerAPI) ListVLANsForMachine(arg params.Entity) (params.ListVLANsForMachineResult, error) { Listing all NetworkInterfaces and Networks for a machine doesn't sound like a VLAN thing. Most of the APIs we've written in this direction have been bulk style, and in the case of containers, this can actually be useful. (Give me all of the interfaces for my machine, and all of the ones for all of the containers on this machine so that I can configure everything.) As such, it should be something more like: ListNetworksForMachines(arg []params.Entity) Though I believe our RPC style is that the argument must be a struct, so we need a new params type that contains an array of Entities. https://codereview.appspot.com/98600047/diff/1/state/apiserver/networker/netw... state/apiserver/networker/networker.go:125: panic("common.ErrPerm") This doesn't look right. Are we missing a test case for the auth checks? https://codereview.appspot.com/98600047/diff/1/state/apiserver/networker/netw... state/apiserver/networker/networker.go:130: panic(err.Error()) same here. I can imagine that this is hard to trigger in practice, since the Authentication stuff should have already ensured that the entity does exist. Logging some form of "this should never happen" and returning an error seems better than panicing, though. https://codereview.appspot.com/98600047/diff/1/state/apiserver/networker/netw... state/apiserver/networker/networker.go:133: machine := entity.(*state.Machine) If we know it is a Machine, is it better to use st.Machine() instead of FindEntity? It would mean unwrapping the Tag, I suppose. https://codereview.appspot.com/98600047/diff/1/state/apiserver/networker/netw... File state/apiserver/networker/networker_test.go (right): https://codereview.appspot.com/98600047/diff/1/state/apiserver/networker/netw... state/apiserver/networker/networker_test.go:42: s.machines = nil I feel like if this is evre "resetting previous machines" then we've done something wrong, and want to make sure we have a proper TearDownTest that cleans up what we set up. https://codereview.appspot.com/98600047/diff/1/state/apiserver/networker/netw... state/apiserver/networker/networker_test.go:225: } We are missing a Permissions check, that asserts you can call ListVLANs if you are a) the direct machine b) the parent of a container on your machine c) the environ manager of top level machines (We should be validating that we have the right logic in our getCanAccess code).
Sign in to reply to this message.
General agreement with jam, posting my own comments now so you see them. In particular, please do not panic in the API server. Even if it's a really terrible problem, we do *not* want to take down the whole server. https://codereview.appspot.com/98600047/diff/1/state/api/networker/machine.go File state/api/networker/machine.go (right): https://codereview.appspot.com/98600047/diff/1/state/api/networker/machine.go... state/api/networker/machine.go:48: func (m *Machine) WatchUnits() (watcher.StringsWatcher, error) { I don't think it's appropriate to watch units at this level of abstraction. We care about what networks we need to configure, and that information is (partly) derived from the units, but we shouldn't make the networker api know or care about unitd. https://codereview.appspot.com/98600047/diff/1/state/api/networker/machine.go... state/api/networker/machine.go:70: func (m *Machine) InstanceId() (instance.Id, error) { Suspicious about whether we want/need this too. reserving judgment until I look further. https://codereview.appspot.com/98600047/diff/1/state/api/networker/machine.go... state/api/networker/machine.go:91: func (m *Machine) ListVLANsForMachine() (result params.ListVLANsForMachineResult, err error) { Only VLANs? Shouldn't we be talking in terms of Networks? and: please write bulk APIs, please don't presuppose the entity types in the arguments, and really please don't name result data after methods. I think the APIserver method should look something like: Networks([]params.Entity) (params.NetworkResults, error) https://codereview.appspot.com/98600047/diff/1/state/api/networker/networker_... File state/api/networker/networker_test.go (right): https://codereview.appspot.com/98600047/diff/1/state/api/networker/networker_... state/api/networker/networker_test.go:108: VLANTag: 0, Yeah, this isn't a VLAN ;p.
Sign in to reply to this message.
This can be made a lot simpler for our current needs. It's a good direction, but does NOT LGTM until refactored as suggested below. https://codereview.appspot.com/98600047/diff/1/state/api/networker/machine.go File state/api/networker/machine.go (right): https://codereview.appspot.com/98600047/diff/1/state/api/networker/machine.go... state/api/networker/machine.go:15: // Machine represents a juju machine as seen by the firewaller worker. s/firewaller/networker/ But you don't really need a Machine proxy class in this facade at all (for now at least). Just a single API method, which I'd call: MachineNetworkInfo(args params.Entities) (params.MachineNetworkInfoResult, error). See the comments around the call, further down. I'd drop this whole file. https://codereview.appspot.com/98600047/diff/1/state/api/networker/machine.go... state/api/networker/machine.go:48: func (m *Machine) WatchUnits() (watcher.StringsWatcher, error) { On 2014/05/28 09:53:19, fwereade wrote: > I don't think it's appropriate to watch units at this level of abstraction. We > care about what networks we need to configure, and that information is (partly) > derived from the units, but we shouldn't make the networker api know or care > about unitd. +1 Even if we need to do it, now is not the time. https://codereview.appspot.com/98600047/diff/1/state/api/networker/machine.go... state/api/networker/machine.go:70: func (m *Machine) InstanceId() (instance.Id, error) { On 2014/05/28 09:53:19, fwereade wrote: > Suspicious about whether we want/need this too. reserving judgment until I look > further. Please, drop this and the rest of machine.go. https://codereview.appspot.com/98600047/diff/1/state/api/networker/machine.go... state/api/networker/machine.go:89: // ListVLANsForMachine returns information from the mashine state that is I'd call this MachineNetworkInfo, taking params.Entities and verifying internally that only machine tags are given. Permissions to access a machine's networks depends on whether the agent calling this API is the machine's agent or, in case of a container, its parent's agent. Return value for this call should be a params.MachineInfoResults struct and an error, like: type MachineNetworkInfoResults struct { Results []MachineNetworkInfoResult } type MachineNetworkInfo { Error *Error Info []network.Info } The Info field should contain the same information as setupNetworksAndInterfaces() in the maas provider needs. Also, please move this method inside networker.go after making the changes suggested. https://codereview.appspot.com/98600047/diff/1/state/api/networker/machine.go... state/api/networker/machine.go:91: func (m *Machine) ListVLANsForMachine() (result params.ListVLANsForMachineResult, err error) { On 2014/05/28 09:53:19, fwereade wrote: > Only VLANs? Shouldn't we be talking in terms of Networks? > > and: please write bulk APIs, please don't presuppose the entity types in the > arguments, and really please don't name result data after methods. I think the > APIserver method should look something like: > > Networks([]params.Entity) (params.NetworkResults, error) +1 on API method definitions (bulk calls). Instead of []params.Entity, use params.Entities, and for the results see previous comment. https://codereview.appspot.com/98600047/diff/1/state/api/networker/networker.go File state/api/networker/networker.go (right): https://codereview.appspot.com/98600047/diff/1/state/api/networker/networker.... state/api/networker/networker.go:25: return &State{ return &State{caller} please. https://codereview.appspot.com/98600047/diff/1/state/api/networker/networker.... state/api/networker/networker.go:29: // life requests the life cycle of the given entity from the server. You don't need this or the next method at all, please drop them and instead have the MachineNetworkInfo in here, as previously suggested. https://codereview.appspot.com/98600047/diff/1/state/api/networker/networker_... File state/api/networker/networker_test.go (right): https://codereview.appspot.com/98600047/diff/1/state/api/networker/networker_... state/api/networker/networker_test.go:46: s.machines[0], err = s.State.AddMachine("quantal", state.JobManageEnviron) This should be state.JobHostUnits instead, unless you're explicitly checking you can't access the facade from an environment manager agent. https://codereview.appspot.com/98600047/diff/1/state/api/networker/networker_... state/api/networker/networker_test.go:56: err = s.machines[0].SetAddresses(instance.NewAddress("0.1.2.3", instance.NetworkUnknown)) Not sure you need to set addresses at all - they're not yet integrated with networks and we don't need them here. https://codereview.appspot.com/98600047/diff/1/state/api/networker/networker_... state/api/networker/networker_test.go:71: func (s *networkerSuite) TestMachineTagAndId(c *gc.C) { Drop this please, you don't need it. https://codereview.appspot.com/98600047/diff/1/state/api/networker/networker_... state/api/networker/networker_test.go:88: instanceId, err := apiMachine.InstanceId() Drop this block - you don't need to call InstanceId via the API here. If you need to ensure the machine is not provisioned, use the state.Machine object directly from s.machines. https://codereview.appspot.com/98600047/diff/1/state/api/networker/networker_... state/api/networker/networker_test.go:95: _, err = s.State.Network("net1") Adding the networks should be part of SetUpSuite for now I think. You could still check first they're not in state before adding them (although I'd skip this and just add them). https://codereview.appspot.com/98600047/diff/1/state/api/networker/networker_... state/api/networker/networker_test.go:100: ifacesMachine, err := s.machines[1].NetworkInterfaces() Now this is wrong - we're assuming the networks and interfaces for a machine are already created in SetInstanceInfo (so you'll need to do that in SetUpTest), and here you can assume they are there. https://codereview.appspot.com/98600047/diff/1/state/api/networker/networker_... state/api/networker/networker_test.go:104: networks := []state.NetworkInfo{{ Instead of []state.NetworkInfo and []state.NetworkInterfaceInfo, here I'd just create a expectedInfo := []network.Info{{...}}, so you can compare it later to the result of MachineNetworkInfo(). https://codereview.appspot.com/98600047/diff/1/state/api/networker/networker_... state/api/networker/networker_test.go:147: // Add testing service with requested networks This is also something to do in SetUpTest initially. https://codereview.appspot.com/98600047/diff/1/state/api/networker/networker_... state/api/networker/networker_test.go:158: err = s.machines[1].SetInstanceInfo("i-will", "fake_nonce", &hwChars, networks, ifaces) And this. https://codereview.appspot.com/98600047/diff/1/state/api/networker/networker_... state/api/networker/networker_test.go:162: apiMachine, err = s.networker.Machine(s.machines[1].Tag()) No need to do this over the API, just use the underlying *state.Machine. https://codereview.appspot.com/98600047/diff/1/state/api/networker/networker_... state/api/networker/networker_test.go:168: res, err := apiMachine.ListVLANsForMachine() Now, that's what I'm talking about: except for the result type and method name, this right here (168-192) is the whole test that you need. All the rest before it is either redundant or must be part of SetUpTest. https://codereview.appspot.com/98600047/diff/1/state/api/networker/networker_... state/api/networker/networker_test.go:193: func (s *networkerSuite) TestListVLANsForMachineNoRawInterface(c *gc.C) { I totally don't get the point of this test - can you explain this special case? https://codereview.appspot.com/98600047/diff/1/state/api/networker/networker_... state/api/networker/networker_test.go:225: func (s *networkerSuite) TestListVLANsForMachineUnknownNetwork(c *gc.C) { Drop this please - in any case, you're not even testing the API here but SetInstanceInfo, which has its own tests. https://codereview.appspot.com/98600047/diff/1/state/api/params/internal.go File state/api/params/internal.go (right): https://codereview.appspot.com/98600047/diff/1/state/api/params/internal.go#n... state/api/params/internal.go:379: // VLANForMachine host one network interface should be up by networker. // MachineNetworkInfoResult holds the complete network information // for a machine. type MachineNetworkInfoResult struct { Error *Error Info []network.Info } // MachineNetworkInfoResults holds the results of a // MachineNetworkInfo API call. type MachineNetworkInfoResults struct { Results []MachineNetworkInfoResult } That's all you need. https://codereview.appspot.com/98600047/diff/1/state/api/params/internal.go#n... state/api/params/internal.go:383: RawInterfaceName string You do you need both InterfaceName and RawInterfaceName anyway? https://codereview.appspot.com/98600047/diff/1/state/apiserver/networker/netw... File state/apiserver/networker/networker.go (right): https://codereview.appspot.com/98600047/diff/1/state/apiserver/networker/netw... state/apiserver/networker/networker.go:19: type Networker interface { First, this is wrong - the interface as you implemented it includes a lot more calls. Also why do you need it, and it's exported as well? https://codereview.appspot.com/98600047/diff/1/state/apiserver/networker/netw... state/apiserver/networker/networker.go:25: *common.LifeGetter Please drop all of these *common mixins, as you don't need them. https://codereview.appspot.com/98600047/diff/1/state/apiserver/networker/netw... state/apiserver/networker/networker.go:46: isEnvironManager := authorizer.AuthEnvironManager() Drop this - environ manager won't need to run a networker. https://codereview.appspot.com/98600047/diff/1/state/apiserver/networker/netw... state/apiserver/networker/networker.go:84: func buildListVLANsResult(networks []*state.Network, ifaces []*state.NetworkInterface) ([]params.VLANForMachine, error) { This seems like a lot of work that can be done simpler - see the comment for the method below. https://codereview.appspot.com/98600047/diff/1/state/apiserver/networker/netw... state/apiserver/networker/networker.go:100: panic(fmt.Errorf("unknown network %q on interface %q", iface.NetworkName(), iface.InterfaceName())) Even if this might have happened (it can't) please don't panic, just return an error. https://codereview.appspot.com/98600047/diff/1/state/apiserver/networker/netw... state/apiserver/networker/networker.go:125: panic("common.ErrPerm") On 2014/05/28 09:27:50, jameinel wrote: > This doesn't look right. Are we missing a test case for the auth checks? Instead of panicking here, just: return result, common.ErrPerm. https://codereview.appspot.com/98600047/diff/1/state/apiserver/networker/netw... state/apiserver/networker/networker.go:130: panic(err.Error()) On 2014/05/28 09:27:50, jameinel wrote: > same here. I can imagine that this is hard to trigger in practice, since the > Authentication stuff should have already ensured that the entity does exist. > > Logging some form of "this should never happen" and returning an error seems > better than panicing, though. This can totally happen, but we should just return it, rather than panicking. https://codereview.appspot.com/98600047/diff/1/state/apiserver/networker/netw... state/apiserver/networker/networker.go:133: machine := entity.(*state.Machine) On 2014/05/28 09:27:50, jameinel wrote: > If we know it is a Machine, is it better to use st.Machine() instead of > FindEntity? It would mean unwrapping the Tag, I suppose. +1 to this, since we only take machine tags, the getAuthFunc in the constructor will take care of ensuring the given tag is valid and accessible, so you only need to use: machine := st.Machine(id). To get the id, use names.ParseTag(tag, names.MachineTagKind). https://codereview.appspot.com/98600047/diff/1/state/apiserver/networker/netw... state/apiserver/networker/networker.go:136: if err == nil { This is wrong - we already checked for err != nil above, no need to do it again. https://codereview.appspot.com/98600047/diff/1/state/apiserver/networker/netw... state/apiserver/networker/networker.go:137: networkInterfaces, err = machine.NetworkInterfaces() Basically, you need to obtain nics, err := machine.NetworkInterfaces() and iterate over it, calling st.Network(nics[i].NetworkName) and appending a populated network.Info{} struct for each one. You don't need AllNetworks yet. https://codereview.appspot.com/98600047/diff/1/state/apiserver/networker/netw... File state/apiserver/networker/networker_test.go (right): https://codereview.appspot.com/98600047/diff/1/state/apiserver/networker/netw... state/apiserver/networker/networker_test.go:72: anAuthorizer := s.authorizer As in the state/api/networker_test, please move the relevant bits into SetUpTest and just test the API call. https://codereview.appspot.com/98600047/diff/1/state/apiserver/networker/netw... state/apiserver/networker/networker_test.go:225: } On 2014/05/28 09:27:50, jameinel wrote: > We are missing a Permissions check, that asserts you can call ListVLANs if you > are > a) the direct machine > b) the parent of a container on your machine > c) the environ manager of top level machines > > (We should be validating that we have the right logic in our getCanAccess code). +1, although I'm not quite sure we need the c) case for now.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
A few more comments, sorry, not exhaustive. https://codereview.appspot.com/98600047/diff/20001/environs/network/network.go File environs/network/network.go (right): https://codereview.appspot.com/98600047/diff/20001/environs/network/network.g... environs/network/network.go:34: // Raw InterfaceName is the OS-specific network device name (e.g. Please start your docstrings with name of the object under discussion. "InterfaceName is the raw OS-specific..." https://codereview.appspot.com/98600047/diff/20001/environs/network/network.g... environs/network/network.go:43: func (i *Info) VirtualInterfaceName() string { I suspect things would be cleaner if we moved the choice inside the type: ie a single FinalInterfaceName (or something) method that returns either InterfaceName or InterfaceName:VLANTag? https://codereview.appspot.com/98600047/diff/20001/provider/maas/environ.go File provider/maas/environ.go (right): https://codereview.appspot.com/98600047/diff/20001/provider/maas/environ.go#n... provider/maas/environ.go:541: vlan := info.VirtualInterfaceName() Maybe I'm wrong with the previous comment. I'll leave it to someone's judgment who's more familiar. https://codereview.appspot.com/98600047/diff/20001/state/api/networker/networ... File state/api/networker/networker_test.go (right): https://codereview.appspot.com/98600047/diff/20001/state/api/networker/networ... state/api/networker/networker_test.go:41: } put this bit in a package_test.go please https://codereview.appspot.com/98600047/diff/20001/state/api/networker/networ... state/api/networker/networker_test.go:61: //c.Assert(err, gc.IsNil) delete the commented bits please :) https://codereview.appspot.com/98600047/diff/20001/state/api/params/internal.go File state/api/params/internal.go (right): https://codereview.appspot.com/98600047/diff/20001/state/api/params/internal.... state/api/params/internal.go:382: NetworkInfo []network.Info I don't think we should have the network.Info directly in the API -- we want explicit marshalling tags, and I *think* we really want them all in one place (params). Makes sense? https://codereview.appspot.com/98600047/diff/20001/state/apiserver/networker/... File state/apiserver/networker/networker.go (right): https://codereview.appspot.com/98600047/diff/20001/state/apiserver/networker/... state/apiserver/networker/networker.go:82: return entity.(*state.Machine), nil Let's not panic, though. It'd be much nicer to parse a machine id out of the tag, error on unexpected tag kind, and get a *state.Machine directly. https://codereview.appspot.com/98600047/diff/20001/state/apiserver/networker/... state/apiserver/networker/networker.go:136: networks, err := n.st.AllNetworks() Do we have a *Network for the flat internal juju network yet? https://codereview.appspot.com/98600047/diff/20001/state/apiserver/networker/... File state/apiserver/networker/networker_test.go (right): https://codereview.appspot.com/98600047/diff/20001/state/apiserver/networker/... state/apiserver/networker/networker_test.go:126: IsVirtual: false, ...yeah, I don't see any networks named "juju-private" or anything. Dimitern, do we have an ETA for that part of the model?
Sign in to reply to this message.
Lots of comments, but now it looks a lot better. Tests need to get simpler and some behavioral changes are in order, but it's definitely on the right track now. PS. Please update the description of this CL when reproposing (lbox propose -edit), and *please* answer to review comments, rather than just reproposing. It's hard to keep track of what got changed otherwise (I usually reply to each one as a draft here, but I'm not sending them - instead, I rely on lbox propose to send any pending drafts). https://codereview.appspot.com/98600047/diff/20001/environs/network/network.go File environs/network/network.go (right): https://codereview.appspot.com/98600047/diff/20001/environs/network/network.g... environs/network/network.go:34: // Raw InterfaceName is the OS-specific network device name (e.g. On 2014/06/02 10:43:44, fwereade wrote: > Please start your docstrings with name of the object under discussion. > "InterfaceName is the raw OS-specific..." +1 https://codereview.appspot.com/98600047/diff/20001/environs/network/network.g... environs/network/network.go:43: func (i *Info) VirtualInterfaceName() string { On 2014/06/02 10:43:44, fwereade wrote: > I suspect things would be cleaner if we moved the choice inside the type: ie a > single FinalInterfaceName (or something) method that returns either > InterfaceName or InterfaceName:VLANTag? +1, although I'd prefer the name ActualInterfaceName() for the method. https://codereview.appspot.com/98600047/diff/20001/provider/maas/environ.go File provider/maas/environ.go (right): https://codereview.appspot.com/98600047/diff/20001/provider/maas/environ.go#n... provider/maas/environ.go:537: if info.IsVirtual { Please, use info.VLANTag > 0 here, as we're specifically handling VLANs, not any type of virtual interface. See next comment. https://codereview.appspot.com/98600047/diff/20001/provider/maas/environ.go#n... provider/maas/environ.go:541: vlan := info.VirtualInterfaceName() On 2014/06/02 10:43:44, fwereade wrote: > Maybe I'm wrong with the previous comment. I'll leave it to someone's judgment > who's more familiar. The specific case we're handling here is a virtual VLAN interface. As discussed IsVirtual can mean other things than VLANTag > 0, like an alias. So your suggestion about having Info.FinalInterfaceName() and using it here is valid. I'd prefer to call the method "ActualInterfaceName" though. https://codereview.appspot.com/98600047/diff/20001/provider/maas/environ_test.go File provider/maas/environ_test.go (right): https://codereview.appspot.com/98600047/diff/20001/provider/maas/environ_test... provider/maas/environ_test.go:202: {InterfaceName: "eth0", VLANTag: 99, IsVirtual: true}, This is strictly correct, but not required by setupNetworksAndInterfaces(), so I'd revert these changes (see previous comment). https://codereview.appspot.com/98600047/diff/20001/state/api/networker/networ... File state/api/networker/networker.go (right): https://codereview.appspot.com/98600047/diff/20001/state/api/networker/networ... state/api/networker/networker.go:30: // NetworkInfo returns information from the state about networks to setup. We shouldn't need 2 API methods for that - please drop this one and leave the other only. https://codereview.appspot.com/98600047/diff/20001/state/api/networker/networ... state/api/networker/networker.go:54: } Please add this: if len(results.Results) != 1 { // TODO: Not directly tested err = fmt.Errorf("expected one result, got %d", len(results.Results)) return nil, err } https://codereview.appspot.com/98600047/diff/20001/state/api/networker/networ... File state/api/networker/networker_test.go (right): https://codereview.appspot.com/98600047/diff/20001/state/api/networker/networ... state/api/networker/networker_test.go:29: machines []*state.Machine You only really need one of these each, not multiple, so: machine *state.Machine container *state.Machine subcontainer *state.Machine You probably need to add networks []*state.Network though. See next comments. https://codereview.appspot.com/98600047/diff/20001/state/api/networker/networ... state/api/networker/networker_test.go:41: } On 2014/06/02 10:43:44, fwereade wrote: > put this bit in a package_test.go please +1, look for "package_test.go" in other facades for examples. https://codereview.appspot.com/98600047/diff/20001/state/api/networker/networ... state/api/networker/networker_test.go:46: // Create some machines for the tests. Please, drop most of these and ensure SetUpTest performs just these steps: 1) create several networks (say 5) and save them as s.networks; 2) create a machine to login and use, with JobHostUnits, save it as s.machine (it has to have a password and be provisioned with all the networks in s.networks and some interfaces - at least one per network, ideally several, like a physical nic and 2 virtual nics for VLANs); 3) create a s.container with parent s.machine, provisioned with some nics and networks (a few, not all like for s.machine, but in a compatible way, i.e. not on more networks than the host) 4) create a s.subcontainer with parent s.container and some nics/networks (again, a subset of its parent ones). 5) create the networker api facade as s.networker. That's all you need really, you might even find it easier to have s.ifaces as []*state.NetworkInterface on networkerSuite to contain the machine NICs created in SetUpTest, so you can use them directly. https://codereview.appspot.com/98600047/diff/20001/state/api/networker/networ... state/api/networker/networker_test.go:89: func (s *networkerSuite) TearDownTest(c *gc.C) { If calling s.JujuConnSuite.TearDownTest(c) is the only thing here, just drop the whole method, as JujuConnSuite's TearDownTest will be called automatically. https://codereview.appspot.com/98600047/diff/20001/state/api/networker/networ... state/api/networker/networker_test.go:93: func (s *networkerSuite) loginAsMachine(c *gc.C, machine *state.Machine) { You don't need this method, as you can do all that for the single machine in the suite in SetUpTest. https://codereview.appspot.com/98600047/diff/20001/state/api/networker/networ... state/api/networker/networker_test.go:121: func (s *networkerSuite) TestNetworkInfoNotMachineTag(c *gc.C) { I'll add several other cases here with valid non-machine tags, like "unit-mysql-0", "service-mysql", or "user-foo". For all of these the error should be the same. https://codereview.appspot.com/98600047/diff/20001/state/api/networker/networ... state/api/networker/networker_test.go:130: func (s *networkerSuite) TestNetworkInfoMachineAgentPermissions(c *gc.C) { I'd combine this and the next test into a single table-based test using tags like "machine-1", "machine-1-lxc-0", "machine-1-lxc-0-lxc-1" (or whatever the ids turn up like), and create new machine, container and subcontainer like in SetUpTest, so that we can test that valid machine tags for existing machines return Unauthorized when called from the wrong agent. You'll need to create a networker facade logged in against machine-1 as well and use that instead of s.networker. https://codereview.appspot.com/98600047/diff/20001/state/api/networker/networ... state/api/networker/networker_test.go:174: func (s *networkerSuite) TestNetworkInfo(c *gc.C) { Please, drop this test entirely, we don't need it or the method it's testing. https://codereview.appspot.com/98600047/diff/20001/state/api/networker/networ... state/api/networker/networker_test.go:303: networks := []state.NetworkInfo{{ These networks and ifaces should be initialized in SetUpTest on s.machine, not here. https://codereview.appspot.com/98600047/diff/20001/state/api/networker/networ... state/api/networker/networker_test.go:347: // TODO vladk: The following lines of code does not influence on networker facade behaviour right now. I'm not sure I get what this comment is about? Rephrase? https://codereview.appspot.com/98600047/diff/20001/state/api/networker/networ... state/api/networker/networker_test.go:361: results, err := s.networker.MachineNetworkInfo(s.machines[1].Tag()) This is more or less where this test should start from (the rest should be in SetUpTest, as suggested earlier). Assume the machine is provisioned and networks/ifaces are already set. https://codereview.appspot.com/98600047/diff/20001/state/api/networker/networ... state/api/networker/networker_test.go:364: c.Assert(results, gc.DeepEquals, []network.Info{{ To avoid confusion, as discussed please always use the physical "ethX" for interface names here - VLANs or not. https://codereview.appspot.com/98600047/diff/20001/state/api/networker/networ... state/api/networker/networker_test.go:399: func (s *networkerSuite) TestNetworkInfoNoRawInterface(c *gc.C) { Why do we need this? I think we should drop it. https://codereview.appspot.com/98600047/diff/20001/state/api/networker/networ... state/api/networker/networker_test.go:429: func (s *networkerSuite) TestNetworkInfoUnknownNetwork(c *gc.C) { Drop this - it's testing SetInstanceInfo, not the API (and there are already tests for that in state). https://codereview.appspot.com/98600047/diff/20001/state/api/params/internal.go File state/api/params/internal.go (right): https://codereview.appspot.com/98600047/diff/20001/state/api/params/internal.... state/api/params/internal.go:379: // NetworkInfoResult holds network info for a single machine. Please, use MachineNetworkInfoResult and MachineNetworkInfoResults for the types names, for consistency with the rest of the API? https://codereview.appspot.com/98600047/diff/20001/state/api/params/internal.... state/api/params/internal.go:382: NetworkInfo []network.Info On 2014/06/02 10:43:44, fwereade wrote: > I don't think we should have the network.Info directly in the API -- we want > explicit marshalling tags, and I *think* we really want them all in one place > (params). Makes sense? Why not? We already use instance.Id in params, network.Info is basically the same if more verbose. https://codereview.appspot.com/98600047/diff/20001/state/api/params/internal.... state/api/params/internal.go:382: NetworkInfo []network.Info Or just s/NetworkInfo/Info/. https://codereview.appspot.com/98600047/diff/20001/state/apiserver/networker/... File state/apiserver/networker/networker.go (right): https://codereview.appspot.com/98600047/diff/20001/state/apiserver/networker/... state/apiserver/networker/networker.go:20: type Networker interface { Drop this please, we don't need it. https://codereview.appspot.com/98600047/diff/20001/state/apiserver/networker/... state/apiserver/networker/networker.go:36: if !authorizer.AuthMachineAgent() && !authorizer.AuthEnvironManager() { I think this should be just: if !authorizer.AuthMachineAgent() { return nil, common.ErrPerm ) The environment manager is also a machine agent for machine-0, so it should work. https://codereview.appspot.com/98600047/diff/20001/state/apiserver/networker/... state/apiserver/networker/networker.go:40: isEnvironManager := authorizer.AuthEnvironManager() Drop this - see above. https://codereview.appspot.com/98600047/diff/20001/state/apiserver/networker/... state/apiserver/networker/networker.go:41: isMachineAgent := authorizer.AuthMachineAgent() Since we already verified only machine agents can call NewNetworkerAPI, isMachineAgent is not needed (it's always true). https://codereview.appspot.com/98600047/diff/20001/state/apiserver/networker/... state/apiserver/networker/networker.go:76: entity, err := n.st.FindEntity(tag) No need to go through all this, just use: machineId, _, err := names.ParseTag(tag, names.MachineTagKind) if err != nil { return nil, err } return n.st.Machine(machineId) Although, for now you actually need getMachineNICs(tag) ([]*state.NetworkInterface, error), so you can combine that with the networks for that machine. https://codereview.appspot.com/98600047/diff/20001/state/apiserver/networker/... state/apiserver/networker/networker.go:102: err := fmt.Errorf("unknown network %q on interface %q", iface.NetworkName(), iface.InterfaceName()) Drop the comment, the error message is enough. This can never happen, as network interfaces are validated when added in state. To avoid doing that, I'd drop the two maps, and instead range over ifaces, calling n.st.Network(iface.NetworkName) and returning whatever error it returns (if any), then just appending a populated network.Info for the result. https://codereview.appspot.com/98600047/diff/20001/state/apiserver/networker/... state/apiserver/networker/networker.go:126: func (n *NetworkerAPI) NetworkInfo(args params.Entities) (params.NetworkInfoResults, error) { Please, call this MachineNetworkInfo. // MachineNetworkInfo returns the complete network information // (networks and interfaces) for one or more machine entities. https://codereview.appspot.com/98600047/diff/20001/state/apiserver/networker/... state/apiserver/networker/networker.go:136: networks, err := n.st.AllNetworks() On 2014/06/02 10:43:44, fwereade wrote: > Do we have a *Network for the flat internal juju network yet? Not yet, but as discussed, we will have it soon. --- AllNetworks is probably not what you want (pre-fetching them and putting all in a name-to-*state.Network map for fast lookup seems premature). I'd just fetch each one as I range over the ifaces. See the comments for buildNetworkInfo. https://codereview.appspot.com/98600047/diff/20001/state/apiserver/networker/... File state/apiserver/networker/networker_test.go (right): https://codereview.appspot.com/98600047/diff/20001/state/apiserver/networker/... state/apiserver/networker/networker_test.go:21: func Test(t *stdtesting.T) { This should go in package_test.go, as with state/api/networker. https://codereview.appspot.com/98600047/diff/20001/state/apiserver/networker/... state/apiserver/networker/networker_test.go:38: func (s *networkerSuite) SetUpTest(c *gc.C) { So all the tests here are almost exactly the same as for state/api/networker, with some simplifications: you don't need to create an API client to login as a machine agent, as we're using a FakeAuthorizer, no need to create more than 1 machine, 1 container and 1 subcontainer, as we can test for "permission denied" errors with the wrong agent just by creating a copy of the networker api facade with a differently set up authorizer, https://codereview.appspot.com/98600047/diff/20001/state/apiserver/networker/... state/apiserver/networker/networker_test.go:98: func (s *networkerSuite) TestNetworkInfoPermissions(c *gc.C) { This is way too long and have a lot of duplication. No need to test every possible combo - just test the "success"/"failure" cases in one bulk call (i.e. check that for machine-0, machine-0-lxc-0, and machine-0-lxc-0-lxc-0 works, for other existing machine/containers, and for valid/invalid non-machine tags it returns Unauthorized). Or, test all error cases in here and have another for the success case. Most of the initialization code should be done once in SetUpTest. https://codereview.appspot.com/98600047/diff/20001/state/apiserver/networker/... state/apiserver/networker/networker_test.go:126: IsVirtual: false, On 2014/06/02 10:43:44, fwereade wrote: > ...yeah, I don't see any networks named "juju-private" or anything. Dimitern, do > we have an ETA for that part of the model? Some time this week I hope to have the instance.(Address|Network)* stuff integrated with environ/network and have explicit "private" / "public" pre-defined networks, as discussed today on the call. https://codereview.appspot.com/98600047/diff/20001/state/apiserver/networker/... state/apiserver/networker/networker_test.go:181: {Tag: s.machines[1].Tag() + "-lxc-0"}, In cases like this, please use the actual tag of the existing state object, like s.container.Tag() or s.subcontainer.Tag(). For made-up things, like "machine-42" or "machine-42-lxc-69", use literal strings. It helps readability and it's more consistent with other apiserver tests. https://codereview.appspot.com/98600047/diff/20001/state/apiserver/networker/... state/apiserver/networker/networker_test.go:390: func (s *networkerSuite) TestListRequiredMachineInterfacesNoRawInterface(c *gc.C) { Drop this and the next one please. https://codereview.appspot.com/98600047/diff/20001/state/apiserver/root.go File state/apiserver/root.go (right): https://codereview.appspot.com/98600047/diff/20001/state/apiserver/root.go#ne... state/apiserver/root.go:147: return networker.NewNetworkerAPI(r.srv.state, r) I think now all facades should take state, resources and an authorizer, even if they don't use them (due to John's API versioning). So to make his life easier, please follow that pattern.
Sign in to reply to this message.
On 2014/05/28 09:27:50, jameinel wrote: > I think this is an interesting start, but it sounds like the API we are writing > is more generic than just VLANs, so we should name it appropriately. > Also, I think we really do want it to be a "give me the information about all > these machines" sort of API, since we have several places where we could use it. > (We may not today, but it still is possible, and better to have it and not use > it than need it and have to break compatibility.) > > We're also missing tests for the failure modes (requesting access to an API you > don't have permission for.) > > https://codereview.appspot.com/98600047/diff/1/state/apiserver/networker/netw... > File state/apiserver/networker/networker.go (right): > > https://codereview.appspot.com/98600047/diff/1/state/apiserver/networker/netw... > state/apiserver/networker/networker.go:118: func (n *NetworkerAPI) > ListVLANsForMachine(arg params.Entity) (params.ListVLANsForMachineResult, error) > { > Listing all NetworkInterfaces and Networks for a machine doesn't sound like a > VLAN thing. This name was taken from the task in Kanban. I have changed it. > > Most of the APIs we've written in this direction have been bulk style, and in > the case of containers, this can actually be useful. (Give me all of the > interfaces for my machine, and all of the ones for all of the containers on this > machine so that I can configure everything.) > > As such, it should be something more like: > > ListNetworksForMachines(arg []params.Entity) > > Though I believe our RPC style is that the argument must be a struct, so we need > a new params type that contains an array of Entities. done > > https://codereview.appspot.com/98600047/diff/1/state/apiserver/networker/netw... > state/apiserver/networker/networker.go:125: panic("common.ErrPerm") > This doesn't look right. Are we missing a test case for the auth checks? this was copied from another API, although now that API has changed, so I modified my code accordingly > > https://codereview.appspot.com/98600047/diff/1/state/apiserver/networker/netw... > state/apiserver/networker/networker.go:130: panic(err.Error()) > same here. I can imagine that this is hard to trigger in practice, since the > Authentication stuff should have already ensured that the entity does exist. > > Logging some form of "this should never happen" and returning an error seems > better than panicing, though. done > > https://codereview.appspot.com/98600047/diff/1/state/apiserver/networker/netw... > state/apiserver/networker/networker.go:133: machine := entity.(*state.Machine) > If we know it is a Machine, is it better to use st.Machine() instead of > FindEntity? It would mean unwrapping the Tag, I suppose. this was copy-pasted from provisioner API, the code above assures that panic does not happen here > > https://codereview.appspot.com/98600047/diff/1/state/apiserver/networker/netw... > File state/apiserver/networker/networker_test.go (right): > > https://codereview.appspot.com/98600047/diff/1/state/apiserver/networker/netw... > state/apiserver/networker/networker_test.go:42: s.machines = nil > I feel like if this is evre "resetting previous machines" then we've done > something wrong, and want to make sure we have a proper TearDownTest that cleans > up what we set up. this is just a comment, I copy-pasted from provisioner API, really no cleaning occurs here > > https://codereview.appspot.com/98600047/diff/1/state/apiserver/networker/netw... > state/apiserver/networker/networker_test.go:225: } > We are missing a Permissions check, that asserts you can call ListVLANs if you > are > a) the direct machine > b) the parent of a container on your machine > c) the environ manager of top level machines > > (We should be validating that we have the right logic in our getCanAccess code). done, but dimitern has an opposite opinion about c)
Sign in to reply to this message.
On 2014/05/28 09:53:20, fwereade wrote: > General agreement with jam, posting my own comments now so you see them. > > In particular, please do not panic in the API server. Even if it's a really > terrible problem, we do *not* want to take down the whole server. > > https://codereview.appspot.com/98600047/diff/1/state/api/networker/machine.go > File state/api/networker/machine.go (right): > > https://codereview.appspot.com/98600047/diff/1/state/api/networker/machine.go... > state/api/networker/machine.go:48: func (m *Machine) WatchUnits() > (watcher.StringsWatcher, error) { > I don't think it's appropriate to watch units at this level of abstraction. We > care about what networks we need to configure, and that information is (partly) > derived from the units, but we shouldn't make the networker api know or care > about unitd. removed > > https://codereview.appspot.com/98600047/diff/1/state/api/networker/machine.go... > state/api/networker/machine.go:70: func (m *Machine) InstanceId() (instance.Id, > error) { > Suspicious about whether we want/need this too. reserving judgment until I look > further. removed > > https://codereview.appspot.com/98600047/diff/1/state/api/networker/machine.go... > state/api/networker/machine.go:91: func (m *Machine) ListVLANsForMachine() > (result params.ListVLANsForMachineResult, err error) { > Only VLANs? Shouldn't we be talking in terms of Networks? the name of function was taken from the Kanban task. the information was included so, that networker can do what the cloud-init does NOW with networks. > > and: please write bulk APIs, please don't presuppose the entity types in the > arguments, and really please don't name result data after methods. I think the > APIserver method should look something like: > > Networks([]params.Entity) (params.NetworkResults, error) done > > https://codereview.appspot.com/98600047/diff/1/state/api/networker/networker_... > File state/api/networker/networker_test.go (right): > > https://codereview.appspot.com/98600047/diff/1/state/api/networker/networker_... > state/api/networker/networker_test.go:108: VLANTag: 0, > Yeah, this isn't a VLAN ;p. :)
Sign in to reply to this message.
On 2014/05/28 14:48:08, dimitern wrote: > This can be made a lot simpler for our current needs. > > It's a good direction, but does NOT LGTM until refactored as suggested below. > > https://codereview.appspot.com/98600047/diff/1/state/api/networker/machine.go > File state/api/networker/machine.go (right): > > https://codereview.appspot.com/98600047/diff/1/state/api/networker/machine.go... > state/api/networker/machine.go:15: // Machine represents a juju machine as seen > by the firewaller worker. > s/firewaller/networker/ done, it was copy-pasted from firewaller API > > But you don't really need a Machine proxy class in this facade at all (for now > at least). Just a single API method, which I'd call: > > MachineNetworkInfo(args params.Entities) (params.MachineNetworkInfoResult, > error). > > See the comments around the call, further down. I'd drop this whole file. done > > https://codereview.appspot.com/98600047/diff/1/state/api/networker/machine.go... > state/api/networker/machine.go:48: func (m *Machine) WatchUnits() > (watcher.StringsWatcher, error) { > On 2014/05/28 09:53:19, fwereade wrote: > > I don't think it's appropriate to watch units at this level of abstraction. We > > care about what networks we need to configure, and that information is > (partly) > > derived from the units, but we shouldn't make the networker api know or care > > about unitd. > > +1 > > Even if we need to do it, now is not the time. done > > https://codereview.appspot.com/98600047/diff/1/state/api/networker/machine.go... > state/api/networker/machine.go:70: func (m *Machine) InstanceId() (instance.Id, > error) { > On 2014/05/28 09:53:19, fwereade wrote: > > Suspicious about whether we want/need this too. reserving judgment until I > look > > further. > > Please, drop this and the rest of machine.go. done > > https://codereview.appspot.com/98600047/diff/1/state/api/networker/machine.go... > state/api/networker/machine.go:89: // ListVLANsForMachine returns information > from the mashine state that is > I'd call this MachineNetworkInfo, taking params.Entities and verifying > internally that only machine tags are given. Permissions to access a machine's > networks depends on whether the agent calling this API is the machine's agent > or, in case of a container, its parent's agent. Return value for this call > should be a params.MachineInfoResults struct and an error, like: > > type MachineNetworkInfoResults struct { > Results []MachineNetworkInfoResult > } > > type MachineNetworkInfo { > Error *Error > Info []network.Info > } > > The Info field should contain the same information as > setupNetworksAndInterfaces() in the maas provider needs. > > Also, please move this method inside networker.go after making the changes > suggested. done, but fwereade has another opinion about place of network.Info struct: fwereade> I don't think we should have the network.Info directly in the API -- we want fwereade> explicit marshalling tags, and I *think* we really want them all in one place fwereade> (params). Makes sense? > > https://codereview.appspot.com/98600047/diff/1/state/api/networker/machine.go... > state/api/networker/machine.go:91: func (m *Machine) ListVLANsForMachine() > (result params.ListVLANsForMachineResult, err error) { > On 2014/05/28 09:53:19, fwereade wrote: > > Only VLANs? Shouldn't we be talking in terms of Networks? > > > > and: please write bulk APIs, please don't presuppose the entity types in the > > arguments, and really please don't name result data after methods. I think the > > APIserver method should look something like: > > > > Networks([]params.Entity) (params.NetworkResults, error) > > +1 on API method definitions (bulk calls). Instead of []params.Entity, use > params.Entities, and for the results see previous comment. done > > https://codereview.appspot.com/98600047/diff/1/state/api/networker/networker.go > File state/api/networker/networker.go (right): > > https://codereview.appspot.com/98600047/diff/1/state/api/networker/networker.... > state/api/networker/networker.go:25: return &State{ > return &State{caller} please. done > > https://codereview.appspot.com/98600047/diff/1/state/api/networker/networker.... > state/api/networker/networker.go:29: // life requests the life cycle of the > given entity from the server. > You don't need this or the next method at all, please drop them and instead have > the MachineNetworkInfo in here, as previously suggested. done > > https://codereview.appspot.com/98600047/diff/1/state/api/networker/networker_... > File state/api/networker/networker_test.go (right): > > https://codereview.appspot.com/98600047/diff/1/state/api/networker/networker_... > state/api/networker/networker_test.go:46: s.machines[0], err = > s.State.AddMachine("quantal", state.JobManageEnviron) > This should be state.JobHostUnits instead, unless you're explicitly checking you > can't access the facade from an environment manager agent. NOT DONE jam has an opinion, that Environ Manager needs an access to all top-level machines > > https://codereview.appspot.com/98600047/diff/1/state/api/networker/networker_... > state/api/networker/networker_test.go:56: err = > s.machines[0].SetAddresses(instance.NewAddress("0.1.2.3", > instance.NetworkUnknown)) > Not sure you need to set addresses at all - they're not yet integrated with > networks and we don't need them here. dropped this call > > https://codereview.appspot.com/98600047/diff/1/state/api/networker/networker_... > state/api/networker/networker_test.go:71: func (s *networkerSuite) > TestMachineTagAndId(c *gc.C) { > Drop this please, you don't need it. done > > https://codereview.appspot.com/98600047/diff/1/state/api/networker/networker_... > state/api/networker/networker_test.go:88: instanceId, err := > apiMachine.InstanceId() > Drop this block - you don't need to call InstanceId via the API here. If you > need to ensure the machine is not provisioned, use the state.Machine object > directly from s.machines. done > > https://codereview.appspot.com/98600047/diff/1/state/api/networker/networker_... > state/api/networker/networker_test.go:95: _, err = s.State.Network("net1") > Adding the networks should be part of SetUpSuite for now I think. You could > still check first they're not in state before adding them (although I'd skip > this and just add them). different tests require the different set of networks, so I stay SetInstanceInfo inside of tests but I removed checks for installed networks and network interfaces > > https://codereview.appspot.com/98600047/diff/1/state/api/networker/networker_... > state/api/networker/networker_test.go:100: ifacesMachine, err := > s.machines[1].NetworkInterfaces() > Now this is wrong - we're assuming the networks and interfaces for a machine are > already created in SetInstanceInfo (so you'll need to do that in SetUpTest), and > here you can assume they are there. I removed checks for installed networks and network interfaces > > https://codereview.appspot.com/98600047/diff/1/state/api/networker/networker_... > state/api/networker/networker_test.go:104: networks := []state.NetworkInfo{{ > Instead of []state.NetworkInfo and []state.NetworkInterfaceInfo, here I'd just > create a expectedInfo := []network.Info{{...}}, so you can compare it later to > the result of MachineNetworkInfo(). NOT DONE, didn't understand > > https://codereview.appspot.com/98600047/diff/1/state/api/networker/networker_... > state/api/networker/networker_test.go:147: // Add testing service with requested > networks > This is also something to do in SetUpTest initially. you are right I'll do it when will implement support for services in networker API > > https://codereview.appspot.com/98600047/diff/1/state/api/networker/networker_... > state/api/networker/networker_test.go:158: err = > s.machines[1].SetInstanceInfo("i-will", "fake_nonce", &hwChars, networks, > ifaces) > And this. different tests require the different set of networks, so I stay SetInstanceInfo inside of tests > > https://codereview.appspot.com/98600047/diff/1/state/api/networker/networker_... > state/api/networker/networker_test.go:162: apiMachine, err = > s.networker.Machine(s.machines[1].Tag()) > No need to do this over the API, just use the underlying *state.Machine. done > > https://codereview.appspot.com/98600047/diff/1/state/api/networker/networker_... > state/api/networker/networker_test.go:168: res, err := > apiMachine.ListVLANsForMachine() > Now, that's what I'm talking about: except for the result type and method name, > this right here (168-192) is the whole test that you need. All the rest before > it is either redundant or must be part of SetUpTest. different tests require the different set of networks, so I stay SetInstanceInfo inside of tests > > https://codereview.appspot.com/98600047/diff/1/state/api/networker/networker_... > state/api/networker/networker_test.go:193: func (s *networkerSuite) > TestListVLANsForMachineNoRawInterface(c *gc.C) { > I totally don't get the point of this test - can you explain this special case? this test is a mark that I have thought about this case The rest of my reply is in separate letter because rietveld limits message to 10000 bytes
Sign in to reply to this message.
> https://codereview.appspot.com/98600047/diff/1/state/api/networker/networker_... > state/api/networker/networker_test.go:193: func (s *networkerSuite) > TestListVLANsForMachineNoRawInterface(c *gc.C) { > I totally don't get the point of this test - can you explain this special case? this test is a mark that I have thought about this case > > https://codereview.appspot.com/98600047/diff/1/state/api/networker/networker_... > state/api/networker/networker_test.go:225: func (s *networkerSuite) > TestListVLANsForMachineUnknownNetwork(c *gc.C) { > Drop this please - in any case, you're not even testing the API here but > SetInstanceInfo, which has its own tests. I must be assure that provisioner API checks for it. If you change the logic of provisioner API I will know about it. > > https://codereview.appspot.com/98600047/diff/1/state/api/params/internal.go > File state/api/params/internal.go (right): > > https://codereview.appspot.com/98600047/diff/1/state/api/params/internal.go#n... > state/api/params/internal.go:379: // VLANForMachine host one network interface > should be up by networker. > // MachineNetworkInfoResult holds the complete network information > // for a machine. > type MachineNetworkInfoResult struct { > Error *Error > Info []network.Info > } > > // MachineNetworkInfoResults holds the results of a > // MachineNetworkInfo API call. > type MachineNetworkInfoResults struct { > Results []MachineNetworkInfoResult > } > > That's all you need. done > > https://codereview.appspot.com/98600047/diff/1/state/api/params/internal.go#n... > state/api/params/internal.go:383: RawInterfaceName string > You do you need both InterfaceName and RawInterfaceName anyway? It was misunderstanding, now we suppose that network.Info.InterfaceName contains raw interface name > > https://codereview.appspot.com/98600047/diff/1/state/apiserver/networker/netw... > File state/apiserver/networker/networker.go (right): > > https://codereview.appspot.com/98600047/diff/1/state/apiserver/networker/netw... > state/apiserver/networker/networker.go:19: type Networker interface { > First, this is wrong - the interface as you implemented it includes a lot more > calls. Also why do you need it, and it's exported as well? Firstly, I don't have the info about the other calls. Secondly, the similar interface exists in other APIs, e.g. upgrader but, I removed this interface type > > https://codereview.appspot.com/98600047/diff/1/state/apiserver/networker/netw... > state/apiserver/networker/networker.go:25: *common.LifeGetter > Please drop all of these *common mixins, as you don't need them. done > > https://codereview.appspot.com/98600047/diff/1/state/apiserver/networker/netw... > state/apiserver/networker/networker.go:46: isEnvironManager := > authorizer.AuthEnvironManager() > Drop this - environ manager won't need to run a networker. jam has another opinion > > https://codereview.appspot.com/98600047/diff/1/state/apiserver/networker/netw... > state/apiserver/networker/networker.go:84: func buildListVLANsResult(networks > []*state.Network, ifaces []*state.NetworkInterface) ([]params.VLANForMachine, > error) { > This seems like a lot of work that can be done simpler - see the comment for the > method below. I'm not sure about it. It should be added the code that create missing raw interfaces. > > https://codereview.appspot.com/98600047/diff/1/state/apiserver/networker/netw... > state/apiserver/networker/networker.go:100: panic(fmt.Errorf("unknown network %q > on interface %q", iface.NetworkName(), iface.InterfaceName())) > Even if this might have happened (it can't) please don't panic, just return an > error. done > > https://codereview.appspot.com/98600047/diff/1/state/apiserver/networker/netw... > state/apiserver/networker/networker.go:125: panic("common.ErrPerm") > On 2014/05/28 09:27:50, jameinel wrote: > > This doesn't look right. Are we missing a test case for the auth checks? > > Instead of panicking here, just: return result, common.ErrPerm. done > > https://codereview.appspot.com/98600047/diff/1/state/apiserver/networker/netw... > state/apiserver/networker/networker.go:130: panic(err.Error()) > On 2014/05/28 09:27:50, jameinel wrote: > > same here. I can imagine that this is hard to trigger in practice, since the > > Authentication stuff should have already ensured that the entity does exist. > > > > Logging some form of "this should never happen" and returning an error seems > > better than panicing, though. > > This can totally happen, but we should just return it, rather than panicking. done > > https://codereview.appspot.com/98600047/diff/1/state/apiserver/networker/netw... > state/apiserver/networker/networker.go:133: machine := entity.(*state.Machine) > On 2014/05/28 09:27:50, jameinel wrote: > > If we know it is a Machine, is it better to use st.Machine() instead of > > FindEntity? It would mean unwrapping the Tag, I suppose. > > +1 to this, since we only take machine tags, the getAuthFunc in the constructor > will take care of ensuring the given tag is valid and accessible, so you only > need to use: machine := st.Machine(id). To get the id, use names.ParseTag(tag, > names.MachineTagKind). I changed it to use function getMachine(), that I copy-pasted from another API > > https://codereview.appspot.com/98600047/diff/1/state/apiserver/networker/netw... > state/apiserver/networker/networker.go:136: if err == nil { > This is wrong - we already checked for err != nil above, no need to do it again. done > > https://codereview.appspot.com/98600047/diff/1/state/apiserver/networker/netw... > state/apiserver/networker/networker.go:137: networkInterfaces, err = > machine.NetworkInterfaces() > Basically, you need to obtain nics, err := machine.NetworkInterfaces() and > iterate over it, calling st.Network(nics[i].NetworkName) and appending a > populated network.Info{} struct for each one. > > You don't need AllNetworks yet. NOT DONE it's a point for discussion, I am not sure that reading networks from the state one by one is the better idea that to read them all at ones. > > https://codereview.appspot.com/98600047/diff/1/state/apiserver/networker/netw... > File state/apiserver/networker/networker_test.go (right): > > https://codereview.appspot.com/98600047/diff/1/state/apiserver/networker/netw... > state/apiserver/networker/networker_test.go:72: anAuthorizer := s.authorizer > As in the state/api/networker_test, please move the relevant bits into SetUpTest > and just test the API call. ok > > https://codereview.appspot.com/98600047/diff/1/state/apiserver/networker/netw... > state/apiserver/networker/networker_test.go:225: } > On 2014/05/28 09:27:50, jameinel wrote: > > We are missing a Permissions check, that asserts you can call ListVLANs if you > > are > > a) the direct machine > > b) the parent of a container on your machine > > c) the environ manager of top level machines > > > > (We should be validating that we have the right logic in our getCanAccess > code). > > +1, although I'm not quite sure we need the c) case for now. please, come to one opinion with jameinel
Sign in to reply to this message.
On 2014/06/02 10:43:45, fwereade wrote: > A few more comments, sorry, not exhaustive. > > https://codereview.appspot.com/98600047/diff/20001/environs/network/network.go > File environs/network/network.go (right): > > https://codereview.appspot.com/98600047/diff/20001/environs/network/network.g... > environs/network/network.go:34: // Raw InterfaceName is the OS-specific network > device name (e.g. > Please start your docstrings with name of the object under discussion. > "InterfaceName is the raw OS-specific..." done > > https://codereview.appspot.com/98600047/diff/20001/environs/network/network.g... > environs/network/network.go:43: func (i *Info) VirtualInterfaceName() string { > I suspect things would be cleaner if we moved the choice inside the type: ie a > single FinalInterfaceName (or something) method that returns either > InterfaceName or InterfaceName:VLANTag? changed to ActualInterfaceName as dimitern suggested > > https://codereview.appspot.com/98600047/diff/20001/provider/maas/environ.go > File provider/maas/environ.go (right): > > https://codereview.appspot.com/98600047/diff/20001/provider/maas/environ.go#n... > provider/maas/environ.go:541: vlan := info.VirtualInterfaceName() > Maybe I'm wrong with the previous comment. I'll leave it to someone's judgment > who's more familiar. changed to ActualInterfaceName as dimitern suggested > > https://codereview.appspot.com/98600047/diff/20001/state/api/networker/networ... > File state/api/networker/networker_test.go (right): > > https://codereview.appspot.com/98600047/diff/20001/state/api/networker/networ... > state/api/networker/networker_test.go:41: } > put this bit in a package_test.go please done > > https://codereview.appspot.com/98600047/diff/20001/state/api/networker/networ... > state/api/networker/networker_test.go:61: //c.Assert(err, gc.IsNil) > delete the commented bits please :) done > > https://codereview.appspot.com/98600047/diff/20001/state/api/params/internal.go > File state/api/params/internal.go (right): > > https://codereview.appspot.com/98600047/diff/20001/state/api/params/internal.... > state/api/params/internal.go:382: NetworkInfo []network.Info > I don't think we should have the network.Info directly in the API -- we want > explicit marshalling tags, and I *think* we really want them all in one place > (params). Makes sense? dimitern has another opinion > > https://codereview.appspot.com/98600047/diff/20001/state/apiserver/networker/... > File state/apiserver/networker/networker.go (right): > > https://codereview.appspot.com/98600047/diff/20001/state/apiserver/networker/... > state/apiserver/networker/networker.go:82: return entity.(*state.Machine), nil > Let's not panic, though. It'd be much nicer to parse a machine id out of the > tag, error on unexpected tag kind, and get a *state.Machine directly. this code copy-paster from another API (provisioner and machiner also have it) it will not panic, because authorize has assured that we have machine tag > > https://codereview.appspot.com/98600047/diff/20001/state/apiserver/networker/... > state/apiserver/networker/networker.go:136: networks, err := n.st.AllNetworks() > Do we have a *Network for the flat internal juju network yet? don't know anything about it, yet > > https://codereview.appspot.com/98600047/diff/20001/state/apiserver/networker/... > File state/apiserver/networker/networker_test.go (right): > > https://codereview.appspot.com/98600047/diff/20001/state/apiserver/networker/... > state/apiserver/networker/networker_test.go:126: IsVirtual: false, > ...yeah, I don't see any networks named "juju-private" or anything. Dimitern, do > we have an ETA for that part of the model? don't know anything about it, yet
Sign in to reply to this message.
On 2014/06/02 14:17:13, dimitern wrote: > Lots of comments, but now it looks a lot better. > > Tests need to get simpler and some behavioral changes are in order, but it's > definitely on the right track now. > > PS. Please update the description of this CL when reproposing (lbox propose > -edit), and *please* answer to review comments, rather than just reproposing. > It's hard to keep track of what got changed otherwise (I usually reply to each > one as a draft here, but I'm not sending them - instead, I rely on lbox propose > to send any pending drafts). Sorry, it my fault. I have answered all the previous reviews. > > https://codereview.appspot.com/98600047/diff/20001/environs/network/network.go > File environs/network/network.go (right): > > https://codereview.appspot.com/98600047/diff/20001/environs/network/network.g... > environs/network/network.go:34: // Raw InterfaceName is the OS-specific network > device name (e.g. > On 2014/06/02 10:43:44, fwereade wrote: > > Please start your docstrings with name of the object under discussion. > > "InterfaceName is the raw OS-specific..." > > +1 done > > https://codereview.appspot.com/98600047/diff/20001/environs/network/network.g... > environs/network/network.go:43: func (i *Info) VirtualInterfaceName() string { > On 2014/06/02 10:43:44, fwereade wrote: > > I suspect things would be cleaner if we moved the choice inside the type: ie a > > single FinalInterfaceName (or something) method that returns either > > InterfaceName or InterfaceName:VLANTag? > > +1, although I'd prefer the name ActualInterfaceName() for the method. done, with ActualInterfaceName name > > https://codereview.appspot.com/98600047/diff/20001/provider/maas/environ.go > File provider/maas/environ.go (right): > > https://codereview.appspot.com/98600047/diff/20001/provider/maas/environ.go#n... > provider/maas/environ.go:537: if info.IsVirtual { > Please, use info.VLANTag > 0 here, as we're specifically handling VLANs, not any > type of virtual interface. See next comment. done, see next answer > > https://codereview.appspot.com/98600047/diff/20001/provider/maas/environ.go#n... > provider/maas/environ.go:541: vlan := info.VirtualInterfaceName() > On 2014/06/02 10:43:44, fwereade wrote: > > Maybe I'm wrong with the previous comment. I'll leave it to someone's judgment > > who's more familiar. > > The specific case we're handling here is a virtual VLAN interface. As discussed > IsVirtual can mean other things than VLANTag > 0, like an alias. So your > suggestion about having Info.FinalInterfaceName() and using it here is valid. > I'd prefer to call the method "ActualInterfaceName" though. I would prefer to drop IsVirtual field ANYWHERE, we may create IsVirtual functions, that contains all the logic. But now this field is a potential point of failure. There are a lot places in the code where IsVirtual is used. As for aliases, I am sure we will have a field named AliasSuffix, so it will be func IsVirtual() { return VLANTag > 0 or AliasSuffix != "" } > > https://codereview.appspot.com/98600047/diff/20001/provider/maas/environ_test.go > File provider/maas/environ_test.go (right): > > https://codereview.appspot.com/98600047/diff/20001/provider/maas/environ_test... > provider/maas/environ_test.go:202: {InterfaceName: "eth0", VLANTag: 99, > IsVirtual: true}, > This is strictly correct, but not required by setupNetworksAndInterfaces(), so > I'd revert these changes (see previous comment). NOT DONE, see the previous answer > > https://codereview.appspot.com/98600047/diff/20001/state/api/networker/networ... > File state/api/networker/networker.go (right): > > https://codereview.appspot.com/98600047/diff/20001/state/api/networker/networ... > state/api/networker/networker.go:30: // NetworkInfo returns information from the > state about networks to setup. > We shouldn't need 2 API methods for that - please drop this one and leave the > other only. ok, I will stay only non-bulk one, it makes the code shorter > > https://codereview.appspot.com/98600047/diff/20001/state/api/networker/networ... > state/api/networker/networker.go:54: } > Please add this: > > if len(results.Results) != 1 { > // TODO: Not directly tested > err = fmt.Errorf("expected one result, got %d", len(results.Results)) > return nil, err > } done > > https://codereview.appspot.com/98600047/diff/20001/state/api/networker/networ... > File state/api/networker/networker_test.go (right): > > https://codereview.appspot.com/98600047/diff/20001/state/api/networker/networ... > state/api/networker/networker_test.go:29: machines []*state.Machine > You only really need one of these each, not multiple, so: > > machine *state.Machine > container *state.Machine > subcontainer *state.Machine > > You probably need to add networks []*state.Network though. > > See next comments. while we not come to one opinion about environment manager, I need an extra machine that acts as environment manager > > https://codereview.appspot.com/98600047/diff/20001/state/api/networker/networ... > state/api/networker/networker_test.go:41: } > On 2014/06/02 10:43:44, fwereade wrote: > > put this bit in a package_test.go please > > +1, look for "package_test.go" in other facades for examples. done just FYI, this is used only in 40% of API > > https://codereview.appspot.com/98600047/diff/20001/state/api/networker/networ... > state/api/networker/networker_test.go:46: // Create some machines for the tests. > Please, drop most of these and ensure SetUpTest performs just these steps: > > 1) create several networks (say 5) and save them as s.networks; > 2) create a machine to login and use, with JobHostUnits, save it as s.machine > (it has to have a password and be provisioned with all the networks in > s.networks and some interfaces - at least one per network, ideally several, like > a physical nic and 2 virtual nics for VLANs); > 3) create a s.container with parent s.machine, provisioned with some nics and > networks (a few, not all like for s.machine, but in a compatible way, i.e. not > on more networks than the host) > 4) create a s.subcontainer with parent s.container and some nics/networks > (again, a subset of its parent ones). > 5) create the networker api facade as s.networker. > > That's all you need really, you might even find it easier to have s.ifaces as > []*state.NetworkInterface on networkerSuite to contain the machine NICs created > in SetUpTest, so you can use them directly. NOT DONE I have only one test with a large set of networks. I am not sure that all of this staff will be need in the other tests. But I'll review my code. > > https://codereview.appspot.com/98600047/diff/20001/state/api/networker/networ... > state/api/networker/networker_test.go:89: func (s *networkerSuite) > TearDownTest(c *gc.C) { > If calling s.JujuConnSuite.TearDownTest(c) is the only thing here, just drop the > whole method, as JujuConnSuite's TearDownTest will be called automatically. done, TearDownTest appeared after jam comment > > https://codereview.appspot.com/98600047/diff/20001/state/api/networker/networ... > state/api/networker/networker_test.go:93: func (s *networkerSuite) > loginAsMachine(c *gc.C, machine *state.Machine) { > You don't need this method, as you can do all that for the single machine in the > suite in SetUpTest. this is also related to environ manager issue I use also this function to login as a container > > https://codereview.appspot.com/98600047/diff/20001/state/api/networker/networ... > state/api/networker/networker_test.go:121: func (s *networkerSuite) > TestNetworkInfoNotMachineTag(c *gc.C) { > I'll add several other cases here with valid non-machine tags, like > "unit-mysql-0", "service-mysql", or "user-foo". For all of these the error > should be the same. done > > https://codereview.appspot.com/98600047/diff/20001/state/api/networker/networ... > state/api/networker/networker_test.go:130: func (s *networkerSuite) > TestNetworkInfoMachineAgentPermissions(c *gc.C) { > I'd combine this and the next test into a single table-based test using tags > like "machine-1", "machine-1-lxc-0", "machine-1-lxc-0-lxc-1" (or whatever the > ids turn up like), and create new machine, container and subcontainer like in > SetUpTest, so that we can test that valid machine tags for existing machines > return Unauthorized when called from the wrong agent. You'll need to create a > networker facade logged in against machine-1 as well and use that instead of > s.networker. I tried this, but code become less readable, but I'll think about it. > > https://codereview.appspot.com/98600047/diff/20001/state/api/networker/networ... > state/api/networker/networker_test.go:174: func (s *networkerSuite) > TestNetworkInfo(c *gc.C) { > Please, drop this test entirely, we don't need it or the method it's testing. done, this was a test for bulk API call The rest of reply will be in the next message, because of rietveld limitation of 10000 bytes.
Sign in to reply to this message.
> https://codereview.appspot.com/98600047/diff/20001/state/api/networker/networ... > state/api/networker/networker_test.go:303: networks := []state.NetworkInfo{{ > These networks and ifaces should be initialized in SetUpTest on s.machine, not > here. done > > https://codereview.appspot.com/98600047/diff/20001/state/api/networker/networ... > state/api/networker/networker_test.go:347: // TODO vladk: The following lines of > code does not influence on networker facade behaviour right now. > I'm not sure I get what this comment is about? Rephrase? no more that code and comment > > https://codereview.appspot.com/98600047/diff/20001/state/api/networker/networ... > state/api/networker/networker_test.go:361: results, err := > s.networker.MachineNetworkInfo(s.machines[1].Tag()) > This is more or less where this test should start from (the rest should be in > SetUpTest, as suggested earlier). Assume the machine is provisioned and > networks/ifaces are already set. done > > https://codereview.appspot.com/98600047/diff/20001/state/api/networker/networ... > state/api/networker/networker_test.go:364: c.Assert(results, gc.DeepEquals, > []network.Info{{ > To avoid confusion, as discussed please always use the physical "ethX" for > interface names here - VLANs or not. our mind has changed > > https://codereview.appspot.com/98600047/diff/20001/state/api/networker/networ... > state/api/networker/networker_test.go:399: func (s *networkerSuite) > TestNetworkInfoNoRawInterface(c *gc.C) { > Why do we need this? I think we should drop it. done > > https://codereview.appspot.com/98600047/diff/20001/state/api/networker/networ... > state/api/networker/networker_test.go:429: func (s *networkerSuite) > TestNetworkInfoUnknownNetwork(c *gc.C) { > Drop this - it's testing SetInstanceInfo, not the API (and there are already > tests for that in state). done > > https://codereview.appspot.com/98600047/diff/20001/state/api/params/internal.go > File state/api/params/internal.go (right): > > https://codereview.appspot.com/98600047/diff/20001/state/api/params/internal.... > state/api/params/internal.go:379: // NetworkInfoResult holds network info for a > single machine. > Please, use MachineNetworkInfoResult and MachineNetworkInfoResults for the types > names, for consistency with the rest of the API? done > > https://codereview.appspot.com/98600047/diff/20001/state/api/params/internal.... > state/api/params/internal.go:382: NetworkInfo []network.Info > On 2014/06/02 10:43:44, fwereade wrote: > > I don't think we should have the network.Info directly in the API -- we want > > explicit marshalling tags, and I *think* we really want them all in one place > > (params). Makes sense? > > Why not? We already use instance.Id in params, network.Info is basically the > same if more verbose. ok > > https://codereview.appspot.com/98600047/diff/20001/state/api/params/internal.... > state/api/params/internal.go:382: NetworkInfo []network.Info > Or just s/NetworkInfo/Info/. done > > https://codereview.appspot.com/98600047/diff/20001/state/apiserver/networker/... > File state/apiserver/networker/networker.go (right): > > https://codereview.appspot.com/98600047/diff/20001/state/apiserver/networker/... > state/apiserver/networker/networker.go:20: type Networker interface { > Drop this please, we don't need it. done > > https://codereview.appspot.com/98600047/diff/20001/state/apiserver/networker/... > state/apiserver/networker/networker.go:36: if !authorizer.AuthMachineAgent() && > !authorizer.AuthEnvironManager() { > I think this should be just: > > if !authorizer.AuthMachineAgent() { > return nil, common.ErrPerm > ) > > The environment manager is also a machine agent for machine-0, so it should > work. done > > https://codereview.appspot.com/98600047/diff/20001/state/apiserver/networker/... > state/apiserver/networker/networker.go:40: isEnvironManager := > authorizer.AuthEnvironManager() > Drop this - see above. done > > https://codereview.appspot.com/98600047/diff/20001/state/apiserver/networker/... > state/apiserver/networker/networker.go:41: isMachineAgent := > authorizer.AuthMachineAgent() > Since we already verified only machine agents can call NewNetworkerAPI, > isMachineAgent is not needed (it's always true). done > > https://codereview.appspot.com/98600047/diff/20001/state/apiserver/networker/... > state/apiserver/networker/networker.go:76: entity, err := n.st.FindEntity(tag) > No need to go through all this, just use: > > machineId, _, err := names.ParseTag(tag, names.MachineTagKind) > if err != nil { > return nil, err > } > return n.st.Machine(machineId) > > Although, for now you actually need getMachineNICs(tag) > ([]*state.NetworkInterface, error), so you can combine that with the networks > for that machine. done > > https://codereview.appspot.com/98600047/diff/20001/state/apiserver/networker/... > state/apiserver/networker/networker.go:102: err := fmt.Errorf("unknown network > %q on interface %q", iface.NetworkName(), iface.InterfaceName()) > Drop the comment, the error message is enough. > This can never happen, as network interfaces are validated when added in state. > > To avoid doing that, I'd drop the two maps, and instead range over ifaces, > calling n.st.Network(iface.NetworkName) and returning whatever error it returns > (if any), then just appending a populated network.Info for the result. done > > https://codereview.appspot.com/98600047/diff/20001/state/apiserver/networker/... > state/apiserver/networker/networker.go:126: func (n *NetworkerAPI) > NetworkInfo(args params.Entities) (params.NetworkInfoResults, error) { > Please, call this MachineNetworkInfo. > > // MachineNetworkInfo returns the complete network information > // (networks and interfaces) for one or more machine entities. done > > https://codereview.appspot.com/98600047/diff/20001/state/apiserver/networker/... > state/apiserver/networker/networker.go:136: networks, err := n.st.AllNetworks() > On 2014/06/02 10:43:44, fwereade wrote: > > Do we have a *Network for the flat internal juju network yet? > > Not yet, but as discussed, we will have it soon. > > --- > > AllNetworks is probably not what you want (pre-fetching them and putting all in > a name-to-*state.Network map for fast lookup seems premature). I'd just fetch > each one as I range over the ifaces. See the comments for buildNetworkInfo. ok > > https://codereview.appspot.com/98600047/diff/20001/state/apiserver/networker/... > File state/apiserver/networker/networker_test.go (right): > > https://codereview.appspot.com/98600047/diff/20001/state/apiserver/networker/... > state/apiserver/networker/networker_test.go:21: func Test(t *stdtesting.T) { > This should go in package_test.go, as with state/api/networker. done > > https://codereview.appspot.com/98600047/diff/20001/state/apiserver/networker/... > state/apiserver/networker/networker_test.go:38: func (s *networkerSuite) > SetUpTest(c *gc.C) { > So all the tests here are almost exactly the same as for state/api/networker, > with some simplifications: you don't need to create an API client to login as a > machine agent, as we're using a FakeAuthorizer, no need to create more than 1 > machine, 1 container and 1 subcontainer, as we can test for "permission denied" > errors with the wrong agent just by creating a copy of the networker api facade > with a differently set up authorizer, done > > https://codereview.appspot.com/98600047/diff/20001/state/apiserver/networker/... > state/apiserver/networker/networker_test.go:98: func (s *networkerSuite) > TestNetworkInfoPermissions(c *gc.C) { > This is way too long and have a lot of duplication. No need to test every > possible combo - just test the "success"/"failure" cases in one bulk call (i.e. > check that for machine-0, machine-0-lxc-0, and machine-0-lxc-0-lxc-0 works, for > other existing machine/containers, and for valid/invalid non-machine tags it > returns Unauthorized). > > Or, test all error cases in here and have another for the success case. > > Most of the initialization code should be done once in SetUpTest. done
Sign in to reply to this message.
> > https://codereview.appspot.com/98600047/diff/20001/state/apiserver/networker/... > state/apiserver/networker/networker_test.go:126: IsVirtual: false, > On 2014/06/02 10:43:44, fwereade wrote: > > ...yeah, I don't see any networks named "juju-private" or anything. Dimitern, > do > > we have an ETA for that part of the model? > > Some time this week I hope to have the instance.(Address|Network)* stuff > integrated with environ/network and have explicit "private" / "public" > pre-defined networks, as discussed today on the call. ok > > https://codereview.appspot.com/98600047/diff/20001/state/apiserver/networker/... > state/apiserver/networker/networker_test.go:181: {Tag: s.machines[1].Tag() + > "-lxc-0"}, > In cases like this, please use the actual tag of the existing state object, like > s.container.Tag() or s.subcontainer.Tag(). For made-up things, like "machine-42" > or "machine-42-lxc-69", use literal strings. It helps readability and it's more > consistent with other apiserver tests. done > > https://codereview.appspot.com/98600047/diff/20001/state/apiserver/networker/... > state/apiserver/networker/networker_test.go:390: func (s *networkerSuite) > TestListRequiredMachineInterfacesNoRawInterface(c *gc.C) { > Drop this and the next one please. done > > https://codereview.appspot.com/98600047/diff/20001/state/apiserver/root.go > File state/apiserver/root.go (right): > > https://codereview.appspot.com/98600047/diff/20001/state/apiserver/root.go#ne... > state/apiserver/root.go:147: return networker.NewNetworkerAPI(r.srv.state, r) > I think now all facades should take state, resources and an authorizer, even if > they don't use them (due to John's API versioning). So to make his life easier, > please follow that pattern. done
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
