resources: net: Add net resource #289

emmmmygold · 2017-12-22T12:56:42Z

emmmmygold · 2017-12-22T13:13:52Z

@purpleidea Still needs tests (but the code was written with testing in mind,) and I want to chat with you about whether or not to pull out the dbus properties eavesdropping code into a helper function with the signature func dbusEavesdrop(dbusIface, path string) (chan *dbus.Signal, error) {

emmmmygold · 2017-12-22T15:08:33Z

resources/net.go

+	}
+	if obj.State == "up" && ifaceUp {
+		// error if we lose the connection and softfail is false
+		if !obj.SoftFail && obj.oState == dbus.MakeVariant("no-carrier") {


This is a bug. If we haven't yet received an event from the NIC, obj.oState will be nil. I'll figure out an alternative.

emmmmygold · 2017-12-23T11:05:41Z

examples/net1.yaml

@@ -0,0 +1,9 @@
+---
+graph: mygraph


I realize these examples are pretty specific to my environment. Let's chat about generalizing ("consistent network device naming" doesn't really apply across systems :/)

Yeah :P -- There is some good news:

The consistently named devices are actually useful for automation when you have more than one network card. But annoying when you just have one. The good news: we could easily add a function in the language that returns the eth device name if there's only one :) -- More on that in February :)

For this patch, let's just name them all eth0 so users aren't confused about what they are. This will also work best in vms.

emmmmygold · 2017-12-23T13:29:00Z

resources/net.go

+		return false, errwrap.Wrapf(err, "error getting addrs")
+	}
+	if obj.State == "up" && ifaceUp {
+		// error if we lose the connection and softfail is false


I'd like to discuss this with you at some point, maybe before you review. This doesn't really work like it should. Instead I should be getting the /org/freedesktop/network1/link/$foo OperationalState property so I can detect when it's no-carrier to know when we've lost connectivity. But, I can't figure out how to get the property with the dbus api (need to input a systemd unit, nothing I've tried works, but I can get it in d-feet)

emmmmygold · 2017-12-26T01:47:44Z

resources/net.go

+		// until the timeout expires. If the connection comes back, continue
+		// as if nothing happened. Error if the timeout expires (or isn't set.)
+		if obj.OpState == dbus.MakeVariant(OpStateNoCarrier) {
+			for i := 0; i < int(obj.Timeout); i++ {


I need some mentoring on making this exit cleanly. I'll try to figure it out in the meantime.

Happy to-- let's discuss why we need a timeout at all... I think explaining this in the struct will be the best starting point, although yes, I agree having a blocking time.Sleep in the code is not ideal.

purpleidea

My apologies for the enormous number of comments and the delay in getting it done. I think this is pretty great, so thank you!

The two major structural things are the ip list, and whether we need to use ip link set command or not.

Thank you again!

PS: While Gateway could be a list, let's keep it as a single string for now.

purpleidea · 2017-12-27T19:02:47Z

examples/net1.yaml

@@ -0,0 +1,9 @@
+---
+graph: mygraph


Yeah :P -- There is some good news:

The consistently named devices are actually useful for automation when you have more than one network card. But annoying when you just have one. The good news: we could easily add a function in the language that returns the eth device name if there's only one :) -- More on that in February :)

For this patch, let's just name them all eth0 so users aren't confused about what they are. This will also work best in vms.

purpleidea · 2017-12-27T19:05:19Z

examples/net1.yaml

+  net:
+  - name: enp4s0
+    state: up
+    ipv4: 192.168.2.55/24


nit: how about: 192.168.42.13/24
and 192.168.42.1 please? ;)

purpleidea · 2017-12-27T19:07:51Z

resources/net.go

+
+	// ActiveState is the systemd-networkd service's active state.
+	ActiveState = "ActiveState"
+	// ActiveStateActive is the active service state.


These and a few other comments on these consts are in the form: foo is the foo of the whatever. While I've certainly been guilt of this kind of thing before, if you could update a few of them so that they're a little bit more self descriptive for those that don't know what the internal networkd API is all about. Remember, these comments get displayed in godoc.

The fact that you have all these const's is probably good! Let's just make them useful in the docs if someone would ever need them, or private if they should never be used externally.

Why should they be public or private? If a third party wants to predict what we might be doing, knowing the prefix (eg mgmt-) is something we need public. Also if a consumer of this resource when using mgmt as a lib wants to know something that it should be able to gleam from this, that's useful too.

purpleidea · 2017-12-27T19:11:36Z

resources/net.go

+// NetRes is an systemd-networkd resource.
+type NetRes struct {
+	BaseRes `yaml:",inline"`
+	State   string        `yaml:"state"`   // up or down


Should we allow this to be empty so that it doesn't change the state? (Eg: maybe it only send's the IPV4 address if one exists?)

purpleidea · 2017-12-27T19:12:08Z

resources/net.go

+	IPv4    string        `yaml:"ipv4"`    // ipv4 address in cidr format
+	IPv6    string        `yaml:"ipv6"`    // ipv6 address in cidr format
+	Gateway string        `yaml:"gateway"` // gateway address
+	Timeout time.Duration `yaml:"timeout"` // connection timeout in seconds


Could you better document what this does for the godoc string please? A multi-line above it might be appropriate.

purpleidea · 2017-12-27T19:39:08Z

resources/net.go

+	// enable or disable the interface
+	cmd := exec.Command("ip", "link", "set", obj.GetName(), obj.State)
+
+	// make sure we run the command as root


If mgmt is not running as root, does this error appropriately? Is it possible that something like CAP_SYS_ADMIN or similar would give us this without root?

purpleidea · 2017-12-27T19:40:33Z

resources/net.go

+		}
+	}
+	// build the unit file contents from the definition
+	contents := unitFileContents(obj.GetName(), obj.IPv4, obj.IPv6, obj.Gateway)


It's probably okay to just grab these values off the struct directly and have unitFileContents be defined as part of the struct, so you don't have to pass them in.

purpleidea · 2017-12-27T19:41:50Z

resources/net.go

+	return false, nil
+}
+
+// ipCheckApply checks the network device's ip addresses and performs the


purpleidea · 2017-12-27T19:42:27Z

resources/net.go

+	if svcState.Value != dbus.MakeVariant(ActiveStateReloading) {
+		return false, nil
+	}
+	// restart networkd to re-initialize the interface


Is this what needs doing? Is there a reload instead?

purpleidea · 2017-12-27T19:43:57Z

resources/net.go

+	}
+	for _, addr := range addrs {
+		// ipv4
+		if strings.Contains(addr.String(), ".") {


Cute-- Is this the way to check for ipv4 vs. v6?

In any case, as discussed if IP is an array, this just becomes easier, and we don't need to distinguish :)
If need be someone could write an isIPv4 func for the language.

My first pass used this. https://golang.org/pkg/net/#IP.To4 but ACK on the array (slice?)

Oh! Cool--- Why not use that?

emmmmygold · 2017-12-28T18:28:10Z

@purpleidea No worries re: number of comments. :) Maybe we can hangout this weekend to nail down the unknowns. My pace is a little slower than usual this week, so no rush.

purpleidea · 2018-02-26T21:08:39Z

@jonathangold I happened to notice that you updated this and it passed... Is it ready for another review and possible merge, or are you still hacking on it?

emmmmygold · 2018-02-27T01:06:15Z

@purpleidea still a few things to iron out. I should be able to finish it tomorrow.

purpleidea

This is shaping up!

This isn't a final review, but if you can address each item and a bunch of XXX's, we'll be in good shape.

Take your time, good code takes polish, and you're 80% done. The last 20% will take 80% of the time, but it's the most fun. Leave lots of source comments (even full paragraphs with full sentences) if you want to document everything that you learn.

Thanks!

purpleidea · 2018-02-28T20:06:47Z

examples/yaml/net1.yaml

@@ -0,0 +1,9 @@
+---
+graph: mygraph


All the examples are good, but do you mind converting them to mcl instead, and just ensuring that this works please?

purpleidea · 2018-02-28T20:09:11Z

resources/net.go

+	// it is in the process of configuring.
+	adminStateConfiguring = "configuring"
+
+	// eventSources is the number of event sources in watch.


We should probably talk about this. AIUI it shouldn't be necessary.

@purpleidea I could use some guidance on keeping track of which channels are open/closed with a map. Even just a hint like the types in the map I should be using would be helpful. ;)

purpleidea · 2018-02-28T21:10:23Z

resources/net.go

+	BaseRes `yaml:",inline"`
+	State   string   `yaml:"state"`   // up or down or empty
+	DHCP    bool     `yaml:"dhcp"`    // enable dhcp
+	Addrs   []string `yaml:"addrs"`   // list of addresses in cidr format


Public API in struct looks perfect.

purpleidea · 2018-02-28T21:16:29Z

resources/net.go

+	var err error
+	// get the network link's dbus path
+	obj.ifacePath, err = dbusIfacePath(obj.GetName())
+	if err != nil {


nit, but fwiw when you're doing an = instead of an := you can prefix this, eg:

if obj.ifacePath, err = dbusIfacePath(obj.GetName()); err != nil { ... }

In general I like to do those as much as possible, unless the line is extremely long.

purpleidea · 2018-02-28T21:17:51Z

resources/net.go

+	// Add a DBus rule to watch the "PropertiesChanged" signal and get messages
+	// from the network link's dbus path.
+	args := fmt.Sprintf(dBusEavesdropSprint, dBusPropertiesInterface, obj.ifacePath)
+	call := obj.gdbus.BusObject().Call(dBusAddMatch, 0, args)


same as above if call is not used elsewhere. inline with if

purpleidea · 2018-02-28T21:25:31Z

resources/net.go

+	objAddrs := obj.Addrs
+	sort.Strings(objAddrs)
+	sort.Strings(addrStrings)
+	if len(objAddrs) == len(addrStrings) {


this sort of deep nesting either needs inline-ing, or made into a helper function please

purpleidea · 2018-02-28T21:26:02Z

resources/net.go

+	if err := restartSystemdUnit(obj.sdbus, networkdService, dBusFailMode); err != nil {
+		return false, errwrap.Wrapf(err, "error restarting networkd service")
+	}
+


do we need to wait here? what happens with the restartSystemdUnit stuff?
please include your findings about how it works as a comment :)

purpleidea · 2018-02-28T21:26:52Z

resources/net.go

+		if len(obj.Addrs) != len(res.Addrs) {
+			return false
+		}
+		objAddrs := obj.Addrs


If we had this compare as a helper func, then it could be re-used here :)

purpleidea · 2018-02-28T21:27:06Z

resources/net.go

+// Close cleans up some resources so we can exit cleanly.
+func (obj *NetRes) Close() error {
+	// close the dbus connection
+	obj.sdbus.Close()


What about these errors that are returned...

purpleidea · 2018-02-28T21:29:34Z

resources/net.go

+		return errwrap.Wrapf(err, "failed to restart unit")
+	}
+
+	// XXX: will this ever hang?


you tell me :)

unfortunately, sometimes these sorts of things are difficult to know and occasionally involve reading the upstream code a little. it's good practice, but don't dive too long. if you can't find it in 20 min, move on or ask someone.

emmmmygold

@purpleidea Made most of the changes. Still want to discuss the close map and a few other things before it's ready for another review. Maybe we can hangout later. :)

emmmmygold · 2018-03-01T01:45:45Z

resources/net.go

+
+	// Add a DBus rule to watch the "PropertiesChanged" signal and get messages
+	// from the network link's dbus path.
+	args := fmt.Sprintf(dBusEavesdropSprint, dBusPropertiesInterface, obj.ifacePath)


dBusEavesdropSprint is the format string. Maybe a more descriptive name for the const, or a comment?

Edit: Maybe dBusEavesdropFmtString

emmmmygold · 2018-03-01T18:06:50Z

resources/net.go

+	log.Printf("%s: ifaceCheckApply(%t)", obj, apply)
+
+	// enable or disable the interface
+	cmd := exec.Command("ip", "link", "set", obj.GetName(), obj.State)


replaced with netlink

emmmmygold · 2018-03-01T18:08:33Z

resources/net.go

+	if err := cmd.Wait(); err != nil {
+		return false, errwrap.Wrapf(err, "%s", slurp)
+	}
+	// XXX: wait for state?


Yes. It may "reload" the unit file, but the ip doesn't change until the service is restarted. tried systemctl daemon-reload and dbus.Conn.Reload() but the ip does not change without restarting the service.

This patch adds a net resource for managing nework interfaces, based around systemd-networkd.

emmmmygold · 2018-03-30T10:03:42Z

Closing this since #371 is merged. I'll keep the branch in my repo for networkd reference.

emmmmygold commented Dec 22, 2017

View reviewed changes

emmmmygold commented Dec 23, 2017

View reviewed changes

emmmmygold commented Dec 26, 2017

View reviewed changes

purpleidea requested changes Dec 27, 2017

View reviewed changes

purpleidea requested changes Feb 28, 2018

View reviewed changes

emmmmygold commented Mar 1, 2018

View reviewed changes

resources: net: Add net resource

1f9b51c

This patch adds a net resource for managing nework interfaces, based around systemd-networkd.

emmmmygold closed this Mar 30, 2018

resources: net: Add net resource #289

resources: net: Add net resource #289

Conversation

emmmmygold commented Dec 22, 2017

emmmmygold commented Dec 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

purpleidea left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emmmmygold commented Dec 28, 2017

purpleidea commented Feb 26, 2018

emmmmygold commented Feb 27, 2018

purpleidea left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emmmmygold left a comment

Choose a reason for hiding this comment

emmmmygold Mar 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emmmmygold Mar 1, 2018 • edited Loading

Choose a reason for hiding this comment

emmmmygold commented Mar 30, 2018

emmmmygold commented Dec 22, 2017 •

edited

Loading

emmmmygold Mar 1, 2018 •

edited

Loading

emmmmygold Mar 1, 2018 •

edited

Loading