[PKG-Openstack-devel] Changes I have in mind for the python-rtslib-fb package in Debian

Sat Oct 8 16:50:35 UTC 2016

Hi Thomas,

[Adding Christian because I do not see him in the list of recipients]

On Fri, 7 Oct 2016 14:45:53 +0200, Thomas Goirand wrote :
> First of all, sorry for writing back a bit late, but this week was the
> release week for OpenStack (Newton version). I was *very* busy uploading
> 100s of packages to Debian, and couldn't reply earlier.

No worry. I am busy handling a *couple* of packages so I can't imagine
what handling hundreds of packages looks like :)

> On 10/02/2016 08:25 PM, Christian Seiler wrote:
> > (Adding the openstack alioth list to CC.)
> > 
> > On 09/13/2016 11:36 AM, Christophe Vu-Brugier wrote:  
> >> I would like to discuss with you some changes for the python-rtslib-fb
> >> package you maintain in Debian.
> >>
> >> As you may know, I am trying to help with packaging targetcli-fb in
> >> Debian. Since targetcli-fb depends on rtslib-fb, I looked at your
> >> python-rtslib-fb package and already submitted some very simple
> >> patches. I have larger changes in mind and I would like to discuss
> >> them with you before implementing them eventually.  
> > 
> > As I'll be co-maintaining the configshell-fb and targetcli-fb
> > packages with Christophe and Ritesh, I'd also like to add a
> > few comments from my perspective:  
> 
> FYI, I moved the maintenance of this package to OpenStack upstream
> gerrit, so we have a full CI / CD for it. If you'd like to send patches,
> by all means, please do so. Simply close as usual:
> 
> git clone https://git.openstack.org/openstack/deb-python-rtslib-fb
> cd deb-python-rtslib-fb
> git checkout debian/newton
> 
> Note that currently, it's still debian/newton, but later on, we will add
> a new debian/ocata (for the next 6 months), and then debian/pike,
> following OpenStack naming scheme.
> 
> Then do your changes, commit them, and send the patch to gerrit for
> review with "git review". Note that you unfortunately will need a
> launchpad account, and will have to sign the OpenStack CLA. Everything
> is explained here:
> https://wiki.openstack.org/wiki/How_To_Contribute#If_you.27re_a_developer
> 
> Lucky, it's not an evil CLA, but it's annoying still.
> 
> >> 1. Python library / Python program split
> >>
> >> rtslib-fb is a Python library and a Python program, targetctl, that
> >> uses the library to save on disk or restore from disk the
> >> configuration of the "target" driver in Linux.
> >>
> >> Today, the python-rtslib-fb and python3-rtslib-fb packages contain the
> >> library and targetctl built for Python2 or Python3. Additionally, the
> >> python-rtslib-fb package contain an init script.
> >>
> >> As a consequence, python3-rtslib-fb depends on python-rtslib-fb for
> >> the init script. Having a python3-foo package depend on a python-foo
> >> package is unusual: aside from python3-rtslib-fb, I found only two
> >> packages doing that (python3-cxx-dev and python3-regex).  
> 
> There's a simple way to address this: create a rtslib-fb-common (we can
> use a better name if you prefer) and have it to contain the init script.

OK. You named this package "targetctl" in your change request, right?

> >> Moreover, since targetctl is either targetctl-python2 or
> >> targetctl-python3, the packages use `update-alternatives` to make
> >> targetctl point to one of them.  
> 
> I don't see the update-alternatives as a problem, but as a solution.
> 
> >> Instead of that, I suggest splitting the source package as follows:
> >>  * python-rtslib-fb would contain only the Python 2 library
> >>  * python3-rtslib-fb would contain only the Python 3 library
> >>  * a new package (named "targetctl" or "target-service", ...) would
> >>    contain the targetctl Python program, the init script and some
> >>    additional directories (see section 2).  
> 
> The init script, yes, but the python program, no: this belongs to the
> Python 2 or 3 implementation, so our users can choose.
> 
> >> The "targetctl" package would be built for a single version of
> >> Python. I think building for Python 3 would be more future-proof.  
> > 
> > I completely agree with this.  
> 
> I don't. I prefer to let our user choose, and continue using
> update-alternatives.

My primary concern was the Python 3 package depending on the Python 2
package. You just fixed it in your change request.

Thinking more about it, I am not really annoyed by the
update-alternatives. I see some value in letting our users choose.
Also, since python-cinder is currently a Python 2 package, it would
sound strange to make it depend on the "targetctl" Python 3 package I
proposed.

Speaking of python-cinder, I remember that an OpenStack developer
contributed patches to make rtslib-fb compatible with Python 3 using
the "six" library.

  https://github.com/open-iscsi/rtslib-fb/pull/62#issuecomment-104704820

Do you see the python-cinder package transition to Python 3 in Debian
in the foreseeable future? I am just curious...

> FYI, I now have a standard to use /usr/bin/python{2,3}-FOO, so I have
> switched the package to that in my last patch [1].
> 
> >> 2. Missing directories expected by the Linux kernel
> >>
> >> The "target" driver in the Linux kernel stores some information on
> >> disk so that it persists across reboots.
> >>
> >> Information about "Persistent Reservations" is stored in /var/target/pr
> >> and information about "Asymmetric Logical Unit Access" is stored
> >> in /var/target/alua.
> >>
> >> If these directories are missing then the persistent reservations or
> >> ALUA features fail. The link below describes one such failure.
> >>
> >>   http://target-devel.vger.kernel.narkive.com/Ujpp6wSs/pr-registration-on-lio-iscsi-returns-sense-key-not-ready
> >>
> >> I suggest that the package providing `targetctl` also create the
> >> /var/target/pr and /var/target/alua when installed.  
> 
> Done in the change request [1].

OK. Thanks!

> > I don't like the fact that something like /var/target is hardcoded in
> > the kernel, but as it is, I would also agree here that these paths
> > should be created by the corresponding package for now.
> > 
> > One could of course have a discussion with upstream on how to solve
> > this in a better way in the future, but for now I think we should be
> > able to provide this functionality within Debian.

For completeness, I should mention that the user can configure the path
were PR and ALUA persistent data are stored since Linux 4.7. The
default location is still /var/target though.

  https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=a96e9783e05851d5f06da0ae7635aec55a228e3d

> >> 3. Debian patch to store saveconfig.json to /etc/rtslib-fb-target/
> >>
> >> Debian changes the default location where the saveconfig.json file is
> >> retrieved/stored from /etc/target/ to /etc/rtslib-fb-target/. The
> >> `targetcli` command also manipulates this file, so either we patch
> >> targetcli-fb to retrieve/store saveconfig.json in
> >> /etc/rtslib-fb-target as well or we drop the patch from the
> >> python-rtslib-fb package.
> >>
> >> I understand your concern that /etc/target is too generic, but a lot
> >> of things related to the LIO iSCSI target are named after "target":
> >> the "driver/target/" directory in the Linux kernel source code, the
> >> "target-devel" mailing list, the "targetctl" and "targetcli"
> >> utilities...
> >>
> >> So I would favor dropping this patch to behave the same as
> >> targetctl/targetcli upstream.  
> 
> Ok, done in [1].
> 
> >> That means being able to move /etc/rtslib-fb-target/saveconfig.json to
> >> /etc/target/saveconfig.json during an upgrade.  
> 
> Done also.
> 
> > I also think that changing the name to /etc/rtslib-fb-target is not
> > ideal at the moment, especially since this diverges from upstream.
> > OTOH, I understand that /etc/target might seem a bit too generic. So
> > maybe we could talk to upstream and have the standard path be renamed
> > to /etc/lio-target-fb? (With a proper fallback logic if that doesn't
> > exist but the previous path does.) I personally think that that would
> > be far more descriptive as compared to rtslib-fb-target.  
> 
> Agreed. Can you engage with the discussion with upstream?
> 
> > I'd leave it as-is for the moment (changing it multiple times is
> > going to be too much of a mess), but have discussion with upstream to
> > perhaps change that.  
> 
> Let's not approve the change request then until discussion with upstream
> is over...

I will engage with upstream developers through GitHub about
renaming /etc/target. We will see what other people think.

Also, I will ask for changes that may ease our work as packagers like
removing the "debian" directory from upstream. If you see other
annoying things please tell me.

> >> 4. Integration with systemd
> >>
> >> I think it would be nice to provide a systemd unit file in addition to
> >> the sysv init script.  
> > 
> > I think that's something I'd like to have in the long term, but not
> > very urgent, as it's not an rcS init script, and thus supported by
> > systemd just fine at the moment.
> > 
> > I'm more worried about the 'sleep 10' in the init script as that will
> > delay boot and shutdown, and I don't grok why that's in there. Any
> > reason?  
> 
> It's because it takes time for the *feature* of the kernel module to be
> operative (even though modprobe is nearly instantly loading the module).
> If you see a better way than just a "sleep 10" (for example, quickly
> polling for the feature provided by confgfs in a loop were we'd be
> waiting for a few milliseconds at a time only), then I'm all for it.

I think there is a bug in the "initscripts" package regarding configfs
mounting and that fixing this bug would also fix your "targetctl" init
script, allowing you to drop check_configfs_module() and
check_configfs_mounted().

The mountkernfs.sh init script contains this:

  if [ -d /sys/kernel/config ]
  then
      domount "$MNTMODE" configfs "" /sys/kernel/config configfs ""
  fi

The problem is that the /sys/kernel/config directory does not exist
when the script is invoked because the configfs kernel module is not
loaded at that time. The /sys/kernel/config does not exist until the
configfs module is probed. See:

  http://lxr.free-electrons.com/source/fs/configfs/mount.c#L142

Invoking `modprobe configfs` before testing for /sys/kernel/config
being a directory in mountkernfs.sh allows to mount configfs as
expected. I am not sure it is the proper fix and I will report this to
the initscripts maintainers.

> >> You are probably busy tackling other tasks and I don't want to consume
> >> too much of your time. However, the changes I mention may imply
> >> implementing transitions during a package upgrade (e.g. moving a
> >> configuration file, changing packages that depend on python-rtslib-fb
> >> to add a dependency to a potential "targetctl" package, etc.). Since I
> >> am not familiar with this, I may ask for some guidance.  
> 
> There's only 2 consumers of rtslib-fb, according to apt-rdepends -r:
> - python-cinder
> - targetcli
> 
> So we're fine just fixing these 2, and I don't think we even need to
> submit bugs if we fix it properly ourselves.
> 
> >> Also, I know nothing about OpenStack and debian-openstack release
> >> schedule: it may not be the right time to implement the above changes.  
> 
> The release was yesterday, and I uploaded more than 200 times... :)
> 
> Now would be a good moment.

OK. Good to hear.

With best regards,

> > OTOH, I think both Christophe and I could contribute to this
> > package (because our packages depend on it), and most changes
> > we had in mind (apart from the package split mentioned in
> > this email) won't have any real impact on OpenStack. So maybe
> > we could join the Alioth openstack team and start to co-
> > maintain the rtslib-fb package with you? It appears that
> > rtslib-fb has only one rdep in openstack (cinder), so I think
> > the only change that needs some coordination (the split of
> > the package to have separate packages for the Python module
> > one the one hand, and the CLI utility and the init script on
> > the other hand) is not going to be too difficult to manage.
> > 
> > What do you think?  
> 
> You can join the Alioth group, but that's not where we maintain things
> anymore. I'd welcome your reviews and patches on Gerrit.
> 
> Cheers,
> 
> Thomas Goirand (zigo)
> 
> [1] https://review.openstack.org/383714
> 

-- 
Christophe Vu-Brugier