Re: [RFC PATCH 0/3 V3] add DCB support to NetworkManager

[Dan Williams <dcbw redhat com>]

On Mon, 2012-07-02 at 17:59 +0800, Weiping Pan wrote:
> This patchset is to add DCB support to NetworkManager.
> Its function is like dcbtool, and it will communicate with lldpad through
> liblldp_clif shared library.

One question I've got is about when the DCB stuff gets run.  The current
patch runs it when the connection is found and read by NetworkManager.
This gets done by the system_handle_dcb_setting() function for any
connection that has a DCB setting, even if that interface is not
present?  Is that useful?  I'd actually expect that instead of sending
the DCB commands when the connection showed up, instead that would be
done when NetworkManager found a new network device, and if any
connections were compatible with that network device, then the DCB stuff
would get run from those compatible connections.

(Also, what happens if there are more than one connection with DCB info
for the same device?  I think the patches apply both sets of info.)

I'm wondering if it really makes sense to store the info in ifcfg files
along with the interface's IP configuration and such, since it's really
independent of that.  At least on Fedora/RH, ifcfg files are a weird
conglomeration of both lower level stuff like MTU and duplex and higher
level stuff like IP addressing.  On the other hand, it would probably be
rare to physically move a machine to a different network and keep all
the DCB information the same, since it does seem quite network-specific.
Plus keeping the info together helps management since all the config is
in the same place (the connection).

One crazy thought I had: what if the DCB stuff was more of a "plugin",
where NM called it at certain times for certain events (like
connection-added, connection-removed, interface-added,
interface-removed, interface-activated, etc) and the DCB plugin could
use the information passed along with the event (stuff like HW address,
interface name, connection details) to look up the relevant DCB
configuration information and send it to lldpad?  That way we could
achieve the NM event integration, but allow more flexibility with the
DCB configuration.  That doesn't solve configuration management issues
though, since the configuration info wouldn't necessarily be in the same
place as the IP configuration info, for the reasons suggested below.

One thing I'm a bit worried about here is the proliferation of config
options.  You may have noticed that it's a bit complicated to add new
settings; that's something I'd like to change.  There's not a lot of
flexibility once the options are created, and the DCB patch adds a *lot*
of options.  The problem we might face is that perhaps lldpad releases a
new version that changes some config options, and now NetworkManager has
to work with both  old *and* new lldpad versions, because we keep the
same NM API on a stable branch, and we usually don't bump required
versions of major dependencies either.

Also, are we expecting some sort of GUI editor for these options too, at
some future point?  Is that what users want, or are they satisfied with
editing config files directly for now?

Dan

> This patchset is on top of commit c3b29cec713f(ip6: only change ra_flags with
> device_set_ra_flags()).
> 
> 1 DCB ifcfg example
> 
> TYPE=Ethernet
> DEVICE=eth0
> HWADDR=00:11:22:33:44:ee
> BOOTPROTO=none
> IPADDR=1.2.3.4
> PREFIX=24
> GATEWAY=1.1.1.1
> 
> # dcb global switch
> # use DCB=off to turn it off
> DCB=on
> 
> # application settings
> # It also supports DCB_APP_ISCSI_XXX and DCB_APP_FIP_XXX.
> DCB_APP_FCOE=yes
> DCB_APP_FCOE_CFG=08
> DCB_APP_FCOE_ENABLE=yes
> DCB_APP_FCOE_ADVERTISE=yes
> DCB_APP_FCOE_WILLING=yes
> 
> DCB_APP_ISCSI=yes
> DCB_APP_ISCSI_CFG=40
> DCB_APP_ISCSI_ENABLE=yes
> DCB_APP_ISCSI_ADVERTISE=yes
> DCB_APP_ISCSI_WILLING=yes
> 
> # priority group settings
> DCB_PG_UP2TC=76543210
> DCB_PG_PCT=25,0,0,75,0,0,0,0
> DCB_PG_ID=0000111f
> DCB_PG_UPPCT=25,25,25,25,50,25,25,0
> DCB_PG_STRICT=01010101
> DCB_PG_ENABLE=yes
> DCB_PG_ADVERTISE=yes
> DCB_PG_WILLING=yes
> 
> # priority flow control settings
> DCB_PFC_UP=00010000
> DCB_PFC_ENABLE=yes
> DCB_PFC_ADVERTISE=yes
> DCB_PFC_WILLING=no
> 
> NOTE:
> 1 What forms a minimum valid ifcfg for DCB?
> DCB=on
> 
> DCB_APP_FCOE=yes
> DCB_APP_FCOE_ENABLE=yes
> DCB_APP_FCOE_ADVERTISE=yes
> DCB_APP_FCOE_WILLING=yes
> 
> DCB_PFC_ENABLE=yes
> DCB_PFC_ADVERTISE=yes
> DCB_PFC_WILLING=yes
> 
> 2 For application settings, it also supports DCB_APP_ISCSI_XXX and
> DCB_APP_FIP_XXX. 
> 
> TEST:
> I compared the commands (messages that liblldp_clif will send to lldpad) that
> are generated by NetworkManager and dcbtool, and tried most combinations,
> and can confirm that NetworkManager will generate the same commands with
> dcbtool in the same situation.
> 
> TODO:
> 1 lldpad monitor
> If lldpad daemon has not been started, NetworkManager should start it, and
> postpone activation of DCB capable interface. 
> 2 test it on real hardware
> I will test the whole patchset with real hardware (FCoE and iSCSI).
> 
> Changelog:
> V2: make it accept more than one application settings,
> store app settings with GSList.
> V3: add ifcfg-dcb writer
> 
> 
> Weiping Pan (3):
>   ifcfg-rh: add ifcfg-dcb parser
>   DCB: generate commands and send them to lldpad
>   ifcfg-rh: add ifcfg-dcb writer
> 
>  configure.ac                                       |   17 +
>  docs/api/generate-settings-spec.c                  |    2 +
>  docs/libnm-util/libnm-util-docs.sgml               |    1 +
>  include/nm-dbus-glib-types.h                       |    2 +
>  libnm-util/Makefile.am                             |    2 +
>  libnm-util/libnm-util.ver                          |   26 +
>  libnm-util/nm-connection.c                         |   24 +-
>  libnm-util/nm-connection.h                         |    2 +
>  libnm-util/nm-setting-dcb.c                        | 1185 ++++++++++++++++++++
>  libnm-util/nm-setting-dcb.h                        |  165 +++
>  libnm-util/nm-utils.c                              |  191 ++++
>  libnm-util/nm-utils.h                              |   10 +
>  src/Makefile.am                                    |    1 +
>  src/nm-manager.c                                   |   94 ++
>  src/settings/plugins/ifcfg-rh/reader.c             |  479 ++++++++-
>  .../ifcfg-rh/tests/network-scripts/ifcfg-test-dcb  |   47 +
>  .../plugins/ifcfg-rh/tests/test-ifcfg-rh.c         |   84 ++
>  src/settings/plugins/ifcfg-rh/writer.c             |  189 ++++
>  18 files changed, 2519 insertions(+), 2 deletions(-)
>  create mode 100644 libnm-util/nm-setting-dcb.c
>  create mode 100644 libnm-util/nm-setting-dcb.h
>  create mode 100644 src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-dcb
>