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

[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.


> 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.

> 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...


> 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.


> 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.

Thank you very much for your review!

Jiri

>