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

Fri Oct 7 12:45:53 UTC 2016

Hi,

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.

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.

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

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

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

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

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

> 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