Issue 6099051: Go port for local provider networking
|
Created:
12 years, 11 months ago by andrewsmedina Modified:
12 years, 10 months ago Visibility:
Public. |
DescriptionGo port for local provider networking
https://code.launchpad.net/~andrewsmedina/juju/go-local-network/+merge/103333
(do not edit description out of merge proposal)
Patch Set 1 #
Total comments: 41
Patch Set 2 : Go port for local provider networking #
Total comments: 7
Patch Set 3 : Go port for local provider networking #
Total comments: 8
Patch Set 4 : Go port for local provider networking #
Total comments: 5
Patch Set 5 : Go port for local provider networking #
MessagesTotal messages: 14
Please take a look.
Sign in to reply to this message.
Looks plausible in general. These comments are based on a very shallow reading of the code and most are trivial. As I'm not familiar with virsh and lxc, this needs to be looked at by someone that is. https://codereview.appspot.com/6099051/diff/1/environs/local/network.go File environs/local/network.go (right): https://codereview.appspot.com/6099051/diff/1/environs/local/network.go#newco... environs/local/network.go:13: type network struct { how do you expect network values to be created? should there be a NewNetwork function? https://codereview.appspot.com/6099051/diff/1/environs/local/network.go#newco... environs/local/network.go:16: Bridge bridge Bridge bridge `xml:"bridge"` https://codereview.appspot.com/6099051/diff/1/environs/local/network.go#newco... environs/local/network.go:17: Ip ip Ip ip `xml:"ip"` https://codereview.appspot.com/6099051/diff/1/environs/local/network.go#newco... environs/local/network.go:18: Subnet int Subnet int `xml:"subnet"` presumably? https://codereview.appspot.com/6099051/diff/1/environs/local/network.go#newco... environs/local/network.go:23: XMLName xml.Name `xml:"ip"` i think that it would be better to annotate the fields above and lose the XMLName fields here and in the bridge type below. https://codereview.appspot.com/6099051/diff/1/environs/local/network.go#newco... environs/local/network.go:24: Address string `xml:"address,attr"` perhaps mirror the fields of the net.IpNet type here and use IP and Mask as names. It would be nice to be able to use that type directly, but I'm not sure how. https://codereview.appspot.com/6099051/diff/1/environs/local/network.go#newco... environs/local/network.go:30: XMLName xml.Name `xml:"bridge"` d https://codereview.appspot.com/6099051/diff/1/environs/local/network.go#newco... environs/local/network.go:35: // It's use virsh net-dumpxml to get network info. i don't think the second line adds useful info. i'd delete it. https://codereview.appspot.com/6099051/diff/1/environs/local/network.go#newco... environs/local/network.go:41: xml.Unmarshal(output, &n) you need to check the error here. https://codereview.appspot.com/6099051/diff/1/environs/local/network.go#newco... environs/local/network.go:50: return false i don't think this should hide the error return from listNetworks. i think this function should return (bool, error) and this line should read: return false, err https://codereview.appspot.com/6099051/diff/1/environs/local/network.go#newco... environs/local/network.go:60: return false same applies here. https://codereview.appspot.com/6099051/diff/1/environs/local/network.go#newco... environs/local/network.go:62: check, _ := networks[n.Name] return networks[n.Name], nil https://codereview.appspot.com/6099051/diff/1/environs/local/network.go#newco... environs/local/network.go:69: <bridge name='vbr-{{.Name}}-%d' /> is the %d deliberate? BTW where is this XML format defined? could you include a link to the specification? https://codereview.appspot.com/6099051/diff/1/environs/local/network.go#newco... environs/local/network.go:82: // virsh net-define. // start ensures that a network is started. // If the network does not exist, it is created. https://codereview.appspot.com/6099051/diff/1/environs/local/network.go#newco... environs/local/network.go:87: } else { d (saves a level of indentation on the next line. https://codereview.appspot.com/6099051/diff/1/environs/local/network.go#newco... environs/local/network.go:94: } defer file.Close() defer os.Remove(file.Name()) https://codereview.appspot.com/6099051/diff/1/environs/local/network.go#newco... environs/local/network.go:103: file.Close() d https://codereview.appspot.com/6099051/diff/1/environs/local/network.go#newco... environs/local/network.go:116: // listNetworks Returns a map[string]bool of network name to active status. // listNetworks returns a map from network name to active status. https://codereview.appspot.com/6099051/diff/1/environs/local/network.go#newco... environs/local/network.go:118: networks := map[string]bool{} you can move this line to just before the loop, as that's where it's used. https://codereview.appspot.com/6099051/diff/1/environs/local/network.go#newco... environs/local/network.go:121: return networks, nil return nil, err https://codereview.appspot.com/6099051/diff/1/environs/local/network.go#newco... environs/local/network.go:123: // Take the header off // Remove the header. https://codereview.appspot.com/6099051/diff/1/environs/local/network.go#newco... environs/local/network.go:124: lines := strings.Split(string(output), "\n")[2:] you need to check that Split has returned a slice with the right number of elements in. https://codereview.appspot.com/6099051/diff/1/environs/local/network.go#newco... environs/local/network.go:129: fields := strings.Fields(line) check that the line has at least the expected number of fields. https://codereview.appspot.com/6099051/diff/1/environs/local/network_test.go File environs/local/network_test.go (right): https://codereview.appspot.com/6099051/diff/1/environs/local/network_test.go#... environs/local/network_test.go:7: err := n.start() we need to think about these tests as discussed on IRC. tests should not have side effects. some trivial remarks below: https://codereview.appspot.com/6099051/diff/1/environs/local/network_test.go#... environs/local/network_test.go:13: i := ip{Address: "192.168.122.1", Netmask: "255.255.255.0"} d (i'm not sure that this or the line below add much useful) https://codereview.appspot.com/6099051/diff/1/environs/local/network_test.go#... environs/local/network_test.go:14: b := bridge{Name: "virbr0"} d https://codereview.appspot.com/6099051/diff/1/environs/local/network_test.go#... environs/local/network_test.go:19: c.Assert(n.Bridge.Name, Equals, b.Name) s/b.Name/"virbr0"/ https://codereview.appspot.com/6099051/diff/1/environs/local/network_test.go#... environs/local/network_test.go:20: c.Assert(n.Ip.Address, Equals, i.Address) s/i.Address/"192.168.122.1"/ are we totally sure that the default network will always have these values? https://codereview.appspot.com/6099051/diff/1/environs/local/network_test.go#... environs/local/network_test.go:21: c.Assert(n.Ip.Netmask, Equals, i.Netmask) s/i.Netmask/"255.255.255.0"/ https://codereview.appspot.com/6099051/diff/1/environs/local/network_test.go#... environs/local/network_test.go:27: c.Assert(running, Equals, true) how do we know that it's running? these tests are not run in the order they are in the file.
Sign in to reply to this message.
https://codereview.appspot.com/6099051/diff/1/environs/local/network.go File environs/local/network.go (right): https://codereview.appspot.com/6099051/diff/1/environs/local/network.go#newco... environs/local/network.go:13: type network struct { I'm using the loadAttributes method to load the values from virsh dumpxml On 2012/04/26 14:50:43, rog wrote: > how do you expect network values to be created? > should there be a NewNetwork function? https://codereview.appspot.com/6099051/diff/1/environs/local/network.go#newco... environs/local/network.go:18: Subnet int We dont need that because virsh dont have a subnet property on your xml. The subnet attribute is used for create a network. On 2012/04/26 14:50:43, rog wrote: > Subnet int `xml:"subnet"` > presumably? https://codereview.appspot.com/6099051/diff/1/environs/local/network.go#newco... environs/local/network.go:69: <bridge name='vbr-{{.Name}}-%d' /> This string will be used to generate a template for virsh. The %d is used by virsh. On 2012/04/26 14:50:43, rog wrote: > is the %d deliberate? > BTW where is this XML format defined? could you include a link to the > specification? https://codereview.appspot.com/6099051/diff/1/environs/local/network.go#newco... environs/local/network.go:94: } I cant close using defer because this file is used by virsh, then I need close it before run the "virsh net-define" command. On 2012/04/26 14:50:43, rog wrote: > defer file.Close() > defer os.Remove(file.Name())
Sign in to reply to this message.
some replies and a few more remarks. thanks for doing this BTW! https://codereview.appspot.com/6099051/diff/1/environs/local/network.go File environs/local/network.go (right): https://codereview.appspot.com/6099051/diff/1/environs/local/network.go#newco... environs/local/network.go:12: // networks represents a network with the given name, bridge, ip and subnet informations. // network represents a local virtual network. https://codereview.appspot.com/6099051/diff/1/environs/local/network.go#newco... environs/local/network.go:13: type network struct { On 2012/04/27 01:18:03, andrewsmedina wrote: > I'm using the loadAttributes method to load the values from virsh dumpxml > > On 2012/04/26 14:50:43, rog wrote: > > how do you expect network values to be created? > > should there be a NewNetwork function? Sorry, I'm not sure I was clear. loadAttributes doesn't create a value of type network. How are you expecting the network type to be used? What fields are mandatory? Perhaps we should have a function: func newNetwork(name string) *network ? this would make it clearer how to use this type. https://codereview.appspot.com/6099051/diff/1/environs/local/network.go#newco... environs/local/network.go:69: <bridge name='vbr-{{.Name}}-%d' /> On 2012/04/27 01:18:03, andrewsmedina wrote: > This string will be used to generate a template for virsh. The %d is used by > virsh. > > On 2012/04/26 14:50:43, rog wrote: > > is the %d deliberate? > > BTW where is this XML format defined? could you include a link to the > > specification? > i'm sure you're right about the %d, but i've searched in http://libvirt.org/formatnetwork.html and http://libvirt.org/sources/virshcmdref/html/sect-net-define.html and i can't see any occurrences of the '%' symbol. can you point me to where this usage is defined? https://codereview.appspot.com/6099051/diff/1/environs/local/network.go#newco... environs/local/network.go:94: } On 2012/04/27 01:18:03, andrewsmedina wrote: > I cant close using defer because this file is used by virsh, then I need close > it before run the "virsh net-define" command. i don't think that's true. AFAIK there's no need that a file needs to be closed before it's read by another process. we're not using Windows. https://codereview.appspot.com/6099051/diff/1/environs/local/network.go#newco... environs/local/network.go:95: tmpl, err := template.New("network").Parse(libVirtNetworkTemplate) i think i'd put this parse into the global variable ( var libVirtNetworkTemplate = template.Must(template.New("").Parse(`data`) ) then we'll know immediately if we've got the template wrong.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
A few minors, and I think this may go in. https://codereview.appspot.com/6099051/diff/7001/environs/local/network.go File environs/local/network.go (right): https://codereview.appspot.com/6099051/diff/7001/environs/local/network.go#ne... environs/local/network.go:32: // newNetwork starts and returns a network // newNetwork returns a started network. https://codereview.appspot.com/6099051/diff/7001/environs/local/network.go#ne... environs/local/network.go:55: // running returns true if the network name is in the s/the/n's/ https://codereview.appspot.com/6099051/diff/7001/environs/local/network.go#ne... environs/local/network.go:65: // exists returns true if the network name is in the s/the/n's/ https://codereview.appspot.com/6099051/diff/7001/environs/local/network.go#ne... environs/local/network.go:88: // start ensure that a network is started. s/a/the n/ https://codereview.appspot.com/6099051/diff/7001/environs/local/network.go#ne... environs/local/network.go:89: // If the nework does not exists, it is created. s/the network/n's network name/ s/exists/exist/ https://codereview.appspot.com/6099051/diff/7001/environs/local/network_test.go File environs/local/network_test.go (right): https://codereview.appspot.com/6099051/diff/7001/environs/local/network_test.... environs/local/network_test.go:15: echo "default active yes" These would be cleaner with: cat <<END ... END https://codereview.appspot.com/6099051/diff/7001/environs/local/network_test.... environs/local/network_test.go:48: var oldPath string This may be put within the suite: type networkSuite struct { oldPath string }
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
very nearly there. thanks for making all of those changes! https://codereview.appspot.com/6099051/diff/1/environs/local/network.go File environs/local/network.go (right): https://codereview.appspot.com/6099051/diff/1/environs/local/network.go#newco... environs/local/network.go:124: lines := strings.Split(string(output), "\n")[2:] On 2012/04/26 14:50:43, rog wrote: > you need to check that Split has returned a slice with the right number of > elements in. not done. https://codereview.appspot.com/6099051/diff/1/environs/local/network.go#newco... environs/local/network.go:129: fields := strings.Fields(line) On 2012/04/26 14:50:43, rog wrote: > check that the line has at least the expected number of fields. not done. https://codereview.appspot.com/6099051/diff/12001/environs/local/network.go File environs/local/network.go (right): https://codereview.appspot.com/6099051/diff/12001/environs/local/network.go#n... environs/local/network.go:78: <bridge name='vbr-{{.Name}}-%d' /> I still think this (the %d) deserves a comment (including if possible a reference to the documentation where this is defined or to some code that uses it like this), as it the reason for it is obscure. what does the number interpolated by the %d actually signify?
Sign in to reply to this message.
LGTM with a little more testing and commenting as below https://codereview.appspot.com/6099051/diff/12001/environs/local/network.go File environs/local/network.go (right): https://codereview.appspot.com/6099051/diff/12001/environs/local/network.go#n... environs/local/network.go:12: // networks represents local virtual network. // network represents a local virtual network. https://codereview.appspot.com/6099051/diff/12001/environs/local/network.go#n... environs/local/network.go:78: <bridge name='vbr-{{.Name}}-%d' /> On 2012/05/22 10:41:17, rog wrote: > I still think this (the %d) deserves a comment (including if possible a > reference to the documentation where this is defined or to some code that uses > it like this), as it the reason for it is obscure. what does the number > interpolated by the %d actually signify? Indeed; I don't understand what the deal is here. A comment would be helpful. https://codereview.appspot.com/6099051/diff/12001/environs/local/network.go#n... environs/local/network.go:88: // start ensure that the network is started. s/ensure/ensures/ https://codereview.appspot.com/6099051/diff/12001/environs/local/network_test.go File environs/local/network_test.go (right): https://codereview.appspot.com/6099051/diff/12001/environs/local/network_test... environs/local/network_test.go:1: package local I'm a little uncomfortable about this internal testing, but I'm not quite sure why. Is this something that's actually quite gotastic that I'll just have to get used to? ;) https://codereview.appspot.com/6099051/diff/12001/environs/local/network_test... environs/local/network_test.go:16: default active yes It'd be quite nice to have a couple more lines to be parsed here, and a test that a network in the list but not "active" gets correctly reported as existing but inactive.
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/6099051/diff/12001/environs/local/network_test.go File environs/local/network_test.go (right): https://codereview.appspot.com/6099051/diff/12001/environs/local/network_test... environs/local/network_test.go:1: package local I also got that feeling at the beginning https://codereview.appspot.com/6099051/diff/12001/environs/local/network_test... environs/local/network_test.go:16: default active yes I will to this
Sign in to reply to this message.
LGTM https://codereview.appspot.com/6099051/diff/15001/environs/local/network.go File environs/local/network.go (right): https://codereview.appspot.com/6099051/diff/15001/environs/local/network.go#n... environs/local/network.go:92: // If network name does not exist, it is created. s/network/the network/
Sign in to reply to this message.
Looks very nice. A few trivial and LGTM. https://codereview.appspot.com/6099051/diff/15001/environs/local/network.go File environs/local/network.go (right): https://codereview.appspot.com/6099051/diff/15001/environs/local/network.go#n... environs/local/network.go:12: // networks represents a local virtual network. s/networks/network/ https://codereview.appspot.com/6099051/diff/15001/environs/local/network_test.go File environs/local/network_test.go (right): https://codereview.appspot.com/6099051/diff/15001/environs/local/network_test... environs/local/network_test.go:48: exit 0 Would you mind to drop "exit 0" + the empty line before it. That's the default behavior. https://codereview.appspot.com/6099051/diff/15001/environs/local/network_test... environs/local/network_test.go:69: //start a newtork that already exists // start a network ... https://codereview.appspot.com/6099051/diff/15001/environs/local/network_test... environs/local/network_test.go:74: //start a new network // start ... (space after //)
Sign in to reply to this message.
Moving it back onto WIP to wait for these last details.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
