[Pkg-ofed-devel] Infiniband Kernel Module Initializer and port config tool
Ana Guerrero Lopez
ana at debian.org
Thu May 5 13:57:45 UTC 2016
Hi Talat,
On Mon, May 02, 2016 at 10:41:02AM +0000, Talat Batheesh wrote:
> Hi Ana
> Thank you for your detailed response.
> I'm sorry, our mail server is not working with plain text.
> please see below my response marked with [TB].
Could you please address all the points from my initial mail before asking for
review? The licensing part is very important, copying the debian/copyright
file over is not the way to go.
The ITP is fine, just make clear what would be the long description of the
package and leave the rest as a longer explanation for those interested.
Ana
> -----Original Message-----
> From: Ana Guerrero Lopez [mailto:ana at debian.org]
> Sent: Wednesday, March 23, 2016 10:41 PM
> To: Talat Batheesh <talatb at mellanox.com>
> Cc: pkg-ofed-devel at lists.alioth.debian.org; Tal Alon <talal at mellanox.com>; Vladimir Sokolovsky <vlad at mellanox.com>; Brian Fromme <brian.fromme at canonical.com>; Hadar Hen Zion <hadarh at mellanox.com>; Yishai Hadas <yishaih at mellanox.com>; Alaa Hleihel <alaa at mellanox.com>
> Subject: Re: Infiniband Kernel Module Initializer and port config tool
>
>
> Hi Talat,
>
> Sorry for taking so much to reply but I wanted to take the time to reply thoroughly as there were plenty of things to point to. Also, before I continue, thank you for packaging this for Debian, something like this was definitively missing.
>
> On Sun, Jan 17, 2016 at 01:26:07PM +0200, Talat mellanox wrote:
> > Hi Ana and OFED and Debian Developement and Discussion team,
> >
> >
> > Till now, in Ubuntu and Debian Operation systems, rdma and Infiniband
> > modules loaded manually by the user, compared with other operation
> > systems there is a service that control them (load, unload, status, autoload .. ).
> >
> > This services is required and needed by Infiniband and RoCE users, in
> > order to load the Infiniband kernel modules/ configure ports in VPI
> > mode, the service already integrated in other operation system named rdma.
> >
> > We prepare the package that contain this tools in Debian format – attached .
> >
> > What do you think about adding this package to debian under the
> > umbrella of “OFED and Debian Development and Discussion team”
>
> > Could you please review and share it with the relevant reviewer ?
>
> So here goes a first review.
>
> I'm making my comments in two groups, one related to Debian processes about things that should be done independently of the package and another about the packaging itself (as in the files in debian/).
>
> # Processes
>
> * Code source: the source should be published somewhere. E.g. github or some Mellanox's *public* git repository. The URL later goes in the "Homepage" field in debian/control and in debian/copyright as well.
> [TB] Now we host the rdma-utils at Mellanox github https://github.com/Mellanox/rdma-utils
>
> I know this is packaged in RedHat already but in Debian we must specify clearly the origin.
> Some files have Copyright lines from RedHat, are they the original upstream?
> [TB] Right, it's the original.
>
> I see you distributed the same content that rdma-7.2_4.1_rc6-1.el7.src.rpm plus a new file rdma-initramfs-tools-hook that has no authorship or licensing.
> Is this file from Mellanox?
> [TB] it is from Mellanox, Added copyright.
>
>
> * License: this point is related with the point above. There is no doubt this is meant to be licensed under the GPL but it's not properly documented.
> It's missing a file with the full license. Only the files rdma.ifdown-ib and rdma.ifup-ib say they're under the GPL v2 only, and the spec says GPLv2+, this is usually interpreted as GPL version 2 or any later version, but it would be nice to have something more explicit.
>
> * Git: please, use pkg-ofed's git for hosting the packaging. This is independent to where you publish your source code. Notice we use some branches specially for integration with git-buildpackage.
> The repositories are at:
> http://anonscm.debian.org/gitweb/?a=project_list;pf=pkg-ofed
> Alioth's git help:
> https://wiki.debian.org/Alioth/Git
>
> I can create a empty git repository for you if needed.
> [TB] Once we finish the ITP, we will use the pkg-ofed host it.
>
>
> * Before packaging something for Debian, you should send an ITP. More information at:
> https://wiki.debian.org/ITP
> https://www.debian.org/devel/wnpp/
> [TB] Attached ITP draft.
>
>
> * Package name: Finally something that maybe is a bit nitpicking. I'm not sure that "rdma" is a good name for your package. I'm aware this is the RedHat name and this is certainly something subjective, but it makes more sense to me something like rdma-scripts or rdma-modules-scripts. I would interpret 'rdma'
> like a meta-package that installs everything related to rdma, which isn't the case. I would welcome more opinions on this from other lurkers in the list :)
> [TB] Agree with you, what about rdma-utils ?
>
>
> # Packaging
>
> * debian/changelog:
> It closes bug #676403 ( https://bugs.debian.org/676403 ) that's unrelated
> to this package or OFED. It should close the bug of the ITP mentioned earlier.
> [TB] will remove this lines when the ITP accepted.
>
> * debian/compat:
> It says 8, these days we're in debhelper compatibility level 9.
> [TB] Will change to 9
>
> * debian/control:
> To work, your package needs a set of packages installed. Maybe some of them
> should be in Depends, Recommends or Suggests. Please take a look at this.
> Basically, you should make sure your package shouldn't fail if no other ofed
> packages are installed.
>
> The long description is too short, it should be quite more longer and
> explain clearly what this package does.
> [TB] attached a ITP .
>
> * debian/copyright:
> The source is missing, it can't point to a RedHat package, it must be
> at the very least a git repository.
> [TB] Added.
>
> The copyright is incomplete, e.g. this is missing:
> rdma.ifdown-ib:# Copyright (c) 1996-2013 Red Hat, Inc. all rights reserved.
> rdma.ifup-ib:# Copyright (c) 1996-2013 Red Hat, Inc. all rights reserved
> [TB] Added.
>
> All the boilerplate from the template (those line starting with #) should be
> removed.
>
> Finally, see the licensing issue I mentioned above.
>
> * debian/postinst debian/postrm debian/prerm
>
> You won't need those files if you install the systemd unit using the dh
> systemd helper.
>
> And if you were to use those files, they're missing #DEBHELPER#
> in the end of the file
>
> * debian/rules
>
> You define pversion but you don't use it later.
>
> Have you tried installing the files using a makefile or a debian/install
> file instead of doing it by hand in the debian/rules?
>
> For installing the systemd unit there is also a special dh helper,
> don't do it by hand.
> Read: https://wiki.debian.org/Teams/pkg-systemd/Packaging
>
> Why are you overriding the conffiles installation?
> Files in /etc must be marked conffiles if they are included in a
> package. Otherwise they should be created by maintainer scripts.
> Refer to Debian Policy Manual section 10.7 (Configuration files) for details.
>
> * debian/patches
>
> It would be good if the patches have the authorship and a description of
> why they're needed.
>
> * files installed
>
> Very important, your package shouldn't modify files installed by another
> package in any way, it's forbidden by the Debian policy.
>
> The package ships a udev rule and installs it under /etc/udev/rules.d,
> which is reserved for user-installed files. The correct directory for
> system rules is /lib/udev/rules.d.
>
> The *.sh files the package installs shouldn't include the extension ".sh"
>
> The scripts you're installing are also missing a manpage.
>
> Finally, a read of the maintainer's guide:
> https://www.debian.org/doc/manuals/maint-guide/
> is strongly encouraged. It would help to understand better the mentioned
> issues and have pointers to further documentation when needed.
>
> Please, don't hurry up, and take time to go through this list before pushing a
> new version of the package!
>
> I didn't build your package or test it, so there will be probably more
> issues.
>
> What do you mean with the relevant reviever?
>
>
> > - The package is based on rdma package from other Operation system.
>
> Yes, I have seen it's from RedHat.
>
> > - Currently only systemd is supported.
>
> That's ok
>
> > - Do we need to support upstart as well ?
>
> No.
>
> > - How do we publish the code git tree somewhere ?
>
> You can use github. Your company seems to be there https://github.com/Mellanox ?
> There is also http://git.openfabrics.org/
> And for the packaging, debian's git.
>
> > - [very important AR] The conf file “/etc/modprobe.d/mlx4.conf”
> > (that comes with
> > “kmod” package) is not needed anymore, and must be removed.
> > This conf file conflicts with “/lib/modprobe.d/libmlx4.conf”
> > that will come with
> > “rdma” package, and prevents running a script (/sbin/mlx4-setup.sh) that
> > configures ConnectX-3 devices port types (IB vs. ETH).
> >
>
> Which kmod package? Debian's kmod doesn't include a file /etc/modprobe.d/mlx4.conf
> If your package conflicts with another package, it should have a Conflicts:
> line.
>
> I hope all this helps.
> Ana
>
>
> Subject: ITP: rdma-utils -- Infiniband Kernel Module Initializer and port config tool
> Package: wnpp
> Owner: Talat Batheesh <talatb at mellanox.com>
> Severity: wishlist
>
> * Package name : rdma-utils
> Version : 4.1.1
> Upstream Author : Doug Ledford <dledford at redhat.com>
> * URL : https://github.com/Mellanox/rdma-utils
> * License : GPLv2+
> Programming Lang: Bourne Shell, Bash
> Description : Infiniband Kernel Module Initializer and port config tool
>
> Overview
> rdma-utils package allows InfiniBand users to manage user-level RDMA protocols that
> want to be loaded by default, change port type to Ethernet or InfiniBand and
> manage the SR-IOV for Mellanox ConnectX family adapret cards.
> The rdma service is user space initialization scripts for the kernel InfiniBand
> /iWARP drivers.
> It reads the /etc/rdma/rdma.conf file to find out which kernel-level and
> user-level RDMA protocols the administrator wants to be loaded by default. This
> file can be modified to turn various drivers ON or OFF.
> The various drivers that can be enabled and disabled are:
> ?? IPoIB - an IP network emulation layer that allows IP applications to run
> over InfiniBand networks.
> ?? SRP - the SCSI Request Protocol. It allows a machine to mount a remot
> drive or drive array that is exported via the SRP protocol on the
> machine as though it were a local hard disk.
> ?? SRPT - the target mode, or server mode, of the SRP potocol. This load
> the kernel support necessary for exporting a drive or drive array for
> other machines to mount as though it were local on their machine.
> Further configuration of the target mode support is required before any
> devices will actually be exported.
> ?? iSER - a low-level driver for the general iSCSI layer of the Linux kerne
> that provides transport over InfiniBand networks for iSCSI devices.
> ?? RDS - the Reliable Datagram Service in the Linux kernel.
>
> Starting up the rdma service is done automatically. When RDMA capable hardware,
> whether InfiniBand or iWARP or RoCE/IBoE is detected, udev instructs systemd to
> start the rdma service. The rdma service can be enabled to be set as ON all the
> time but it is not mandatory to do so.
> The rdma package provides the /etc/udev.d/rules.d/70-persistent-ipoib.rules file
> which is used to rename IPoIB devices default names (such as ib0 and ib1) to a
> more descriptive names. Users must edit this file to change how their devices
> are named.
> This package will be maintained under the umbrella of "OFED and Debian
> Development and Discussion team", project name "OFED Infiniband Distribution".
>
>
> * Configuring Mellanox Cards Ports for Ethernet Operation
>
> Certain Mellanox ConnectX adapter cards (which use the mlx4 driver) can be configured
> to run in either InfiniBand or Ethernet mode (default InfiniBand).
> To set the mode:
> 1. Find the right PCI device ID of your adapter card. For information
> on how to, follow the instruction in the file /etc/rdma/mlx4.conf.
> 2.Create a line in the file using that device ID and the requested port
> type.
> 3.Rebuild the initramfs to make sure the updated port settings are
> copied into the initramfs.
>
> * RDMA Configuration File(s)
>
> The rdma service loads the configuration from /etc/rdma/rdma.conf file which
> controls which modules will be loaded during the service startup and some
> attributes about the RDMA modules. The following parameters are supported:
> +------------------+----------------------------------+-----------------------+
> | Parameter name | Description | Supported values |
> +-----------------------------------------------------------------------------+
> |IPOIB_LOAD | Load IPoIB module | yes/no |
> +-----------------------------------------------------------------------------+
> |SRP_LOAD | Load SRP initiator module | yes/no |
> +-----------------------------------------------------------------------------+
> |SRPT_LOAD | Load SRP target module | yes/no |
> +-----------------------------------------------------------------------------+
> |ISER_LOAD | Load ISER initiator module | yes/no |
> +-----------------------------------------------------------------------------+
> |RDS_LOAD | Load RDS module | yes/no |
> +-----------------------------------------------------------------------------+
> |FIXUP_MTRR_REGS | Modify the system mtrr registers | yes/no |
> +------------------+----------------------------------+-----------------------+
>
>
> * Starting the RDMA Services
>
> Loading/unloading the RDMA drivers is done using the Infiniband Kernel Module
> Initializer and port config tool.
> - To load/unload the rdma service:
> [root at localhost]# systemctl start/stop rdma.service
> - To start the rdma service automatically when the operating system is
> loaded:
> [root at localhost]# systemctl enable rdma
>
More information about the Pkg-ofed-devel
mailing list