Re: [patch NetworkManager 0/6] add support for network team devices

[Pavel Simerda <psimerda redhat com>]

> From: "Jiri Pirko" <jiri resnulli us>
>
> Fri, Jul 12, 2013 at 10:08:49PM CEST, danw gnome org wrote:
> > On 07/11/2013 11:31 AM, Jiri Pirko wrote:
> > > I heavily reused existing code, mainly bonding parts.
> >
> > Makes sense (and makes it easy to review...)
> >
> > > The only thing not
> > > implemented yet is dbus communication with teamd (used particulary for
> > > port config setup).
> >
> > Will that allow you to shut down teamd via d-bus too? The current
> > teamd-handling is kind of a weird mix of direct spawn/kill vs dbus... (I
> > guess it's not possible to use dbus activation to start teamd, because
> > you need a separate teamd process for each interface? Although the
> > initscripts seem to do this via some systemd magic... I wonder if that
> > magic is available to us programmatically somehow?)
>
> Not possible to start or stop teamd via d-bus. Standard process api is
> used for that work. Also, it is possible to use systemd to spawn teamd
> for NM but I think that NM should be doing such things himself to take
> control over all needed processes. Plus systemd approach would be
> limited only for systems based on systemd.

+1

NetworkManager currently doesn't require systemd.

> > I don't really like having a single string-valued "config" property on
> > NMSettingTeam... we did basically that with "options" in NMSettingBond,
> > and later decided it was a bad idea (which is why we didn't do the same
> > thing with bridges).
>
> teamd config is non-trivial tree structure (JSON). At this point, there is
> really no point to have NM to be aware of the teamd config structure.
> Once NM will need that, this can be changed.
>
> > But I guess if it's possible to add new runners and/or link_watches then
> > there's no way NMSettingTeam could define properties for all possible
> > options, and so we have no choice but to use the json, right?
>
> At this point, I strongly believe so.

I have the same feeling here.

> > In that case we will probably eventually want to have NMSettingTeam
> > parse the config, and ensure that it's valid json, and contains the
> > mandatory properties. Maybe libteam has a function for that?
>
> No function for that. Teamd will scream when you pass non-valid JSON to
> it. Also will scream in case some configuration is not done correctly.
> At this point, I would make NM non-aware at all of the config structure.
> Just leave it as string and NM to blindly pass it through...

Connection take over will require a bit more love but for now we can just consider it unsupported for team.

> > Is it expected/allowed for the NMSettingTeam's config to contain "port"
> > elements, and if so, what happens if they don't match up with the
> > available NMConnections for the port devices? I think we want to say
> > "no" here and pass "-n" to teamd. (Or if we're parsing the json in
> > NMSettingTeam we could strip out the ports, or reject the config if it
> > contains ports.) (Any of these would be inconsistent with the current
> > behavior of the Fedora initscripts, but maybe they should change too? I
> > don't think there's any other situation where one device's ifcfg file
> > can cause another device to be configured.)
>
> When config of a port is set over Dbus, the original config of this port
> is overwritten in case it have existed.
>
> Yes you are probably correct. Teamd could be rather executed with
> -n / --no-ports. Good point.

How do you store the port config then?

> > Other than the possible config issues, the code looks good. Two minor
> > bugs I noticed:
> >
> > You added an '#include "nm-setting-team.h"' to src/nm-device-ethernet.c,
> > but it's not actually used. (Nor is the nm-setting-bond.h include above
> > it... probably leftover from old code.)
>
> Yep, I forgave to strip this out. Noted.
>
> > write_team_port_setting() mistakenly writes out "TEAM_CONFIG=..."
> > instead of "TEAM_PORT_CONFIG=..."
>
> Noted.
>
> Will fix these and repost.

Nice.

Cheers,

Pavel