Possibly broken unit tests for TGT and LIO iSCSI backends

[Florian Haas]

I'm guessing (though I'm not really sure) that these two items in cinder/tests/test_iscsi.py are inaccurate:

from TgtAdmTestCase.setUp():

        self.script_template = "\n".join([
            'tgt-admin --update iqn.2011-09.org.foo.bar:blaa',
            'tgt-admin --force '
            '--delete iqn.2010-10.org.openstack:volume-blaa',
            'tgtadm --lld iscsi --op show --mode target'])

from LioAdmTestCase.setUp():

        self.script_template = "\n".join([
            'cinder-rtstool create '
            '/foo iqn.2011-09.org.foo.bar:blaa test_id test_pass',
            'cinder-rtstool delete iqn.2010-10.org.openstack:volume-blaa'])

Note how the IQNs for creation and deletion don't match.

The trouble is that if you replace those hardcoded IQNs with %(target)s, the unit tests break, possibly indicating an issue with the underlying brick implementations themselves.

[Florian Haas (fghaas)]

Here's a patch that exposes the issue. This "fix" for the unit tests actually causes them to fail.

[Eric Harney (eharney)]

Does this also perhaps finally explain why we have to use "lvremove -f" when deleting LVM volumes?

[Florian Haas (fghaas)]

Oh wow, what can of worms have I opened? :)

[Walt Boring (walter-boring)]

The way the unit test was written was actually causing this problem. The unit test mocked out the internal _get_target and returned a iqn, but yet failed to properly configure cinder with the same prefix. This was the source of the problem. The fix was to set the prefix properly in the config and then test the same iqn prefix in the tests.

The other option was to override the _get_target and return an iscsi_prefix that is set in the default value for brick's TargetAdmin::__init__

[Florian Haas (fghaas)]

OK Walt, at the risk of asking a silly question: is there something I can do to help fix this, or is it a completely trivial and easy fix to you?

[John Griffith (john-griffith)]

The problem here is strictly the unit test... there are actually two problems:
1. We have a inconsistencies in the name we use for the iqn (that's no good, but it's hard coded to work so we get by)
2. The problem that Florian exposed with his patch is it neglects the dependency on the volume prefix.

In other words, he's set the target_name to "iqn.2011-09.org.foo.bar:blaa" and massaged the code to use that as a var
in the tgt checks.

The problem is that the drivers auto add the prefix from the config file, in this case "volume-" so you get a mismatch in the test.

The submitted patch gets us part way there, a follow up will finish it off.

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/68223
Committed: https://git.openstack.org/cgit/openstack/cinder/commit/?id=28b8ecc15d874644952bb74539c61becaf5f7d20
Submitter: Jenkins
Branch: master

commit 28b8ecc15d874644952bb74539c61becaf5f7d20
Author: Walter A. Boring IV <email address hidden>
Date: Tue Jan 21 11:54:15 2014 -0800

    Fixed inconsistency in iqn

    Brick's TargetAdmin classes have a
    default value set in __init__ for
    iscsi_target_prefix. The unit test was changing
    the value returned for the internal _get_target,
    but wasn't changing the CONF.iscsi_target_prefix to
    match. So the default value in the __init__ was
    being used and hence the differences.

    This patch sets the configuration's iscsi_target_prefix
    which alters the values passed into the driver. This
    also makes the tests consistent between the iqn of the volume
    at create time and delete time.

    Closes-Bug: #1271249
    Change-Id: Ie3308cf68adcbdff6057ea795af1299dded82e14