[Libstoragemgmt-devel] [PATCH] lsmcli: Fix bug of volume-masking '--init' option.

[Gris Ge]

* Volume mask will fail with '--init' option[1].
* Update simulator plugin to support creating access group when volume mask.
* Add a test into cmdtest.py for this.
* Manpage updated to guide targetd user.

[1] It's the only way to mask volume to new WWPN/iSCSI IQN on targetd.

Signed-off-by: Gris Ge <***@redhat.com>
---
doc/man/lsmcli.1.in | 10 ++++++++--
plugin/sim/simulator.py | 21 ++++++++++++++++++++-
test/cmdtest.py | 27 +++++++++++++++++++++++++++
tools/lsmcli/cmdline.py | 10 +++++-----
4 files changed, 60 insertions(+), 8 deletions(-)

diff --git a/doc/man/lsmcli.1.in b/doc/man/lsmcli.1.in
index ec4e3eb..0315ca3 100644
--- a/doc/man/lsmcli.1.in
+++ b/doc/man/lsmcli.1.in
@@ -326,13 +326,19 @@ Required. The ID of volume to query access.
.SS volume-mask
.TP 15
Grant access group RW access to certain volume. Like LUN masking
-or NFS export.
+or NFS export. For targetd which does not support creating access group,
+\fB--init\fR could be used for masking volume to new WWPN or iSCSI IQN.
+Either \fB--ag\fR or \f--init\fR should be defined.
.TP
\fB--vol\fR \fI<VOL_ID>\fR
Required. The ID of volume to access.
.TP
\fB--ag\fR \fI<AG_ID>\fR
-Required. The ID of access group to grant.
+Optional. The ID of access group to grant. Conflict with \fB--init\fR option.
+.TP
+\fB--init\fR \fI<INIT_ID>\fR
+Optional. The initiator ID, WWPN or iSCSI IQN. Only used for masking volume
+to new iSCSI IQN or WWPN on targetd.

.SS volume-unmask
.TP 15
diff --git a/plugin/sim/simulator.py b/plugin/sim/simulator.py
index 79b6df5..b350233 100644
--- a/plugin/sim/simulator.py
+++ b/plugin/sim/simulator.py
@@ -17,7 +17,7 @@
# Gris Ge <***@redhat.com>

from lsm import (uri_parse, VERSION, Capabilities, INfs,
- IStorageAreaNetwork, search_property)
+ IStorageAreaNetwork, search_property, LsmError, ErrorNumber)

from simarray import SimArray

@@ -180,6 +180,25 @@ class SimPlugin(INfs, IStorageAreaNetwork):
return SimPlugin._sim_data_2_lsm(sim_ag)

def volume_mask(self, access_group, volume, flags=0):
+ if access_group.id == 0:
+ # access_group.id == 0 means user want to create new access group
+ # when doing volume masking. It's only for targetd.
+ # We support it here purely for testing lsmcli.
+ systems = self.sim_array.systems()
+ system = None
+ for tmp_system in systems:
+ if tmp_system.id == access_group.system_id:
+ system = tmp_system
+ break
+ if system is None:
+ raise LsmError(
+ ErrorNumber.NOT_FOUND_SYSTEM,
+ "System %s not found" % access_group.system_id)
+
+ access_group = self.sim_array.access_group_create(
+ access_group.init_ids[0], access_group.init_ids[0],
+ access_group.init_type, system)
+
return self.sim_array.volume_mask(
access_group.id, volume.id, flags)

diff --git a/test/cmdtest.py b/test/cmdtest.py
index 790fa39..91caa11 100755
--- a/test/cmdtest.py
+++ b/test/cmdtest.py
@@ -46,6 +46,10 @@ from optparse import OptionParser

(SYS_STATUS,) = (2,)

+# access group listing index
+AG_QUERY_ID_INDEX = 0
+AG_QUERY_INIT_IDS_INDEX = 2
+
iqn = ['iqn.1994-05.com.domain:01.89bd01', 'iqn.1994-05.com.domain:01.89bd02']

cmd = "lsmcli"
@@ -214,6 +218,8 @@ def access_group_delete(group_id):
def volume_mask(group, volume_id):
call([cmd, 'volume-mask', '--ag', group, '--vol', volume_id])

+def volume_mask_init(init_id, volume_id):
+ call([cmd, 'volume-mask', '--init', init_id, '--vol', volume_id])

def volume_unmask(group, volume_id):
call([cmd, 'volume-unmask', '--ag', group, '--vol', volume_id])
@@ -617,6 +623,26 @@ def test_mapping(cap, system_id):
if cap['ACCESS_GROUP_DELETE']:
access_group_delete(ag_id)

+def test_volume_mask_init(cap, system_id):
+ # Test volume mask using init id.
+ # Assuming we are testing against simulator
+ pool_id = name_to_id(OP_POOL, test_pool_name)
+ iqn1 = random_iqn()
+ vol_id = create_volume(pool_id)
+ volume_mask_init(iqn1, vol_id)
+ out = call([cmd, '-t' + sep, 'la', '--vol', vol_id])[1]
+ new_ag_id = None
+ for ag in parse(out):
+ if ag[AG_QUERY_INIT_IDS_INDEX] == iqn1:
+ new_ag_id = ag[AG_QUERY_ID_INDEX]
+ break
+ if new_ag_id is None:
+ raise Exception("Failed to find newly created access group by "
+ "volume-mask --init")
+
+ volume_unmask(new_ag_id, vol_id)
+ access_group_delete(new_ag_id)
+ volume_delete(vol_id)

def test_nfs_operations(cap, system_id):
pass
@@ -704,6 +730,7 @@ def run_all_tests(cap, system_id):
create_all(cap, system_id)

test_mapping(cap, system_id)
+ test_volume_mask_init(cap, system_id)

search_test(cap, system_id)

diff --git a/tools/lsmcli/cmdline.py b/tools/lsmcli/cmdline.py
index 66163b3..cd6ca8e 100644
--- a/tools/lsmcli/cmdline.py
+++ b/tools/lsmcli/cmdline.py
@@ -88,7 +88,7 @@ def parse_convert_init(init_id):
if valid:
return (init_id, init_type)

- raise ArgError("--init-id %s is not a valid WWPN or iSCSI IQN" % init_id)
+ raise ArgError("--init %s is not a valid WWPN or iSCSI IQN" % init_id)


## This class represents a command line argument error
@@ -327,9 +327,9 @@ cmds = (
],
optional=[
dict(ag_id_opt),
- dict(name='--init', metavar='<INIT_ID>', action='append',
- help='Initiator ID, only used when access-group-create is '
- 'not supported'),
+ dict(name='--init', metavar='<INIT_ID>',
+ help='Initiator ID, only used for masking new WWPN/iSCSI '
+ 'IQN on targetd not supported'),
],
),

@@ -959,7 +959,7 @@ class CmdLine:
def iscsi_chap(self, args):
(init_id, init_type) = parse_convert_init(args.init)
if init_type != AccessGroup.INIT_TYPE_ISCSI_IQN:
- raise ArgError("--init-id %s is not a valid iSCSI IQN" % args.init)
+ raise ArgError("--init %s is not a valid iSCSI IQN" % args.init)

self.c.iscsi_chap_auth(init_id, args.in_user,
self.args.in_pass,

[Tony Asleson]

Post by Gris Ge
* Volume mask will fail with '--init' option[1].
* Update simulator plugin to support creating access group when volume mask.
* Add a test into cmdtest.py for this.
* Manpage updated to guide targetd user.
[1] It's the only way to mask volume to new WWPN/iSCSI IQN on targetd.
---
doc/man/lsmcli.1.in | 10 ++++++++--
plugin/sim/simulator.py | 21 ++++++++++++++++++++-
test/cmdtest.py | 27 +++++++++++++++++++++++++++
tools/lsmcli/cmdline.py | 10 +++++-----
4 files changed, 60 insertions(+), 8 deletions(-)
diff --git a/doc/man/lsmcli.1.in b/doc/man/lsmcli.1.in
index ec4e3eb..0315ca3 100644
--- a/doc/man/lsmcli.1.in
+++ b/doc/man/lsmcli.1.in
@@ -326,13 +326,19 @@ Required. The ID of volume to query access.
.SS volume-mask
.TP 15
Grant access group RW access to certain volume. Like LUN masking
-or NFS export.
+or NFS export. For targetd which does not support creating access group,

^^^^^^^
Suggest: plug-ins which do not support access groups,

Post by Gris Ge
+\fB--init\fR could be used for masking volume to new WWPN or iSCSI IQN.
+Either \fB--ag\fR or \f--init\fR should be defined.
.TP
\fB--vol\fR \fI<VOL_ID>\fR
Required. The ID of volume to access.
.TP
\fB--ag\fR \fI<AG_ID>\fR
-Required. The ID of access group to grant.
+Optional. The ID of access group to grant. Conflict with \fB--init\fR option.

Instead of

'Conflict with --init option.

perhaps this should be:

'Mutually exclusive with --init option.

Post by Gris Ge
+.TP
+\fB--init\fR \fI<INIT_ID>\fR
+Optional. The initiator ID, WWPN or iSCSI IQN. Only used for masking volume
+to new iSCSI IQN or WWPN on targetd.

Instead of calling out targetd, I think we should say:

"Optional. The initiator ID. Only used for masking volume when
access-groups are not supported by plug-in."

We are thinking about adding access-group support to targetd, then we
have to change this again.

Post by Gris Ge
.SS volume-unmask
.TP 15
diff --git a/plugin/sim/simulator.py b/plugin/sim/simulator.py
index 79b6df5..b350233 100644
--- a/plugin/sim/simulator.py
+++ b/plugin/sim/simulator.py
@@ -17,7 +17,7 @@
from lsm import (uri_parse, VERSION, Capabilities, INfs,
- IStorageAreaNetwork, search_property)
+ IStorageAreaNetwork, search_property, LsmError, ErrorNumber)
from simarray import SimArray
return SimPlugin._sim_data_2_lsm(sim_ag)

access_group.id is a string, not an integer.

If we need a token value to indicate that this isn't a real access group
we should create a class constant in Python and a constant in a header
file for C to represent this. Whatever we pick should be something that
isn't easily confused with a valid possibility of a plug-in. '0' seems
like a possible valid choice to me.

Also the declaration for lsm_access_group_record_alloc is in the
libstoragemgmt_plug_interface.h so a client app can't use it. We need
to move it to libstoragemgmt_accessgroups.h so both client apps and
plug-ins have visibility to call it.

Post by Gris Ge
+ # access_group.id == 0 means user want to create new access group
+ # when doing volume masking. It's only for targetd.
+ # We support it here purely for testing lsmcli.
+ systems = self.sim_array.systems()
+ system = None
+ system = tmp_system
+ break
+ raise LsmError(
+ ErrorNumber.NOT_FOUND_SYSTEM,
+ "System %s not found" % access_group.system_id)
+
+ access_group = self.sim_array.access_group_create(
+ access_group.init_ids[0], access_group.init_ids[0],
+ access_group.init_type, system)
+
return self.sim_array.volume_mask(
access_group.id, volume.id, flags)
diff --git a/test/cmdtest.py b/test/cmdtest.py
index 790fa39..91caa11 100755
--- a/test/cmdtest.py
+++ b/test/cmdtest.py
@@ -46,6 +46,10 @@ from optparse import OptionParser
(SYS_STATUS,) = (2,)
+# access group listing index
+AG_QUERY_ID_INDEX = 0
+AG_QUERY_INIT_IDS_INDEX = 2
+
iqn = ['iqn.1994-05.com.domain:01.89bd01', 'iqn.1994-05.com.domain:01.89bd02']
cmd = "lsmcli"
call([cmd, 'volume-mask', '--ag', group, '--vol', volume_id])
+ call([cmd, 'volume-mask', '--init', init_id, '--vol', volume_id])
call([cmd, 'volume-unmask', '--ag', group, '--vol', volume_id])
access_group_delete(ag_id)
+ # Test volume mask using init id.
+ # Assuming we are testing against simulator
+ pool_id = name_to_id(OP_POOL, test_pool_name)
+ iqn1 = random_iqn()
+ vol_id = create_volume(pool_id)
+ volume_mask_init(iqn1, vol_id)
+ out = call([cmd, '-t' + sep, 'la', '--vol', vol_id])[1]
+ new_ag_id = None
+ new_ag_id = ag[AG_QUERY_ID_INDEX]
+ break
+ raise Exception("Failed to find newly created access group by "
+ "volume-mask --init")
+
+ volume_unmask(new_ag_id, vol_id)
+ access_group_delete(new_ag_id)
+ volume_delete(vol_id)
pass
create_all(cap, system_id)
test_mapping(cap, system_id)
+ test_volume_mask_init(cap, system_id)
search_test(cap, system_id)
diff --git a/tools/lsmcli/cmdline.py b/tools/lsmcli/cmdline.py
index 66163b3..cd6ca8e 100644
--- a/tools/lsmcli/cmdline.py
+++ b/tools/lsmcli/cmdline.py
return (init_id, init_type)
- raise ArgError("--init-id %s is not a valid WWPN or iSCSI IQN" % init_id)
+ raise ArgError("--init %s is not a valid WWPN or iSCSI IQN" % init_id)
## This class represents a command line argument error
@@ -327,9 +327,9 @@ cmds = (
],
optional=[
dict(ag_id_opt),
- dict(name='--init', metavar='<INIT_ID>', action='append',
- help='Initiator ID, only used when access-group-create is '
- 'not supported'),
+ dict(name='--init', metavar='<INIT_ID>',
+ help='Initiator ID, only used for masking new WWPN/iSCSI '
+ 'IQN on targetd not supported'),

The help message went from:

'Initiator ID, only used when access-group-create is not supported'

to

'Initiator ID, only used for masking new WWPN/iSCSI IQN on targetd not
supported'

The first one makes sense to me, the second does not. I'm not sure why
the change was needed.

Post by Gris Ge
],
),
(init_id, init_type) = parse_convert_init(args.init)
- raise ArgError("--init-id %s is not a valid iSCSI IQN" % args.init)
+ raise ArgError("--init %s is not a valid iSCSI IQN" % args.init)
self.c.iscsi_chap_auth(init_id, args.in_user,
self.args.in_pass,

Thanks,
Tony


------------------------------------------------------------------------------

[Gris Ge]

This patch set is the V2 of:
[PATCH] lsmcli: Fix bug of volume-masking '--init' option.

Changes in V2:
1. Split one patch into patches.
2. Add new capability to indicate this special user case of volume mask.
3. Add new constant for ID of access group to identify a user generated
access group.
4. Updated cmdtest.py to check VOLUME_MASK_NEW_AG capability before
test in case someone run cmdtest.py against simc plugin.
5. Targetd plugin will indicate support of VOLUME_MASK_NEW_AG via
capabilities() method.

What we lacking:
* Require a way to allow C library user to generate a user defined
access group.

Tested:
1. 'make distcheck' PASS
2. Tested on targetd via lsmcli for masking volume to new iSCSI IQN.

Gris Ge (5):
LSM Library: New constants for initiator only plugin.
Simulator Plugin: Add support VOLUME_MASK_NEW_AG
Targetd Plugin: Set VOLUME_MASK_NEW_AG capability
lsmcli: Fix bug of command 'volume-mask --init'
cmdtest.py: Add test for --init argument of 'volume-mask' command.

.../libstoragemgmt/libstoragemgmt_capabilities.h | 1 +
.../include/libstoragemgmt/libstoragemgmt_types.h | 4 ++++
doc/man/lsmcli.1.in | 15 +++++++++---
plugin/sim/simulator.py | 22 +++++++++++++++--
plugin/targetd/targetd.py | 1 +
python_binding/lsm/_data.py | 4 ++++
test/cmdtest.py | 28 ++++++++++++++++++++++
tools/lsmcli/cmdline.py | 15 +++++++-----
8 files changed, 79 insertions(+), 11 deletions(-)

[Gris Ge]

* Targetd plugin accept user generated AccessGroup when masking volume.
This patch indicate this in capabilities() query call.

Signed-off-by: Gris Ge <***@redhat.com>
---
plugin/targetd/targetd.py | 1 +
1 file changed, 1 insertion(+)

diff --git a/plugin/targetd/targetd.py b/plugin/targetd/targetd.py
index b40aa4c..7027b6d 100644
--- a/plugin/targetd/targetd.py
+++ b/plugin/targetd/targetd.py
@@ -120,6 +120,7 @@ def capabilities(self, system, flags=0):
cap.set(Capabilities.VOLUME_REPLICATE_COPY)
cap.set(Capabilities.VOLUME_DELETE)
cap.set(Capabilities.VOLUME_MASK)
+ cap.set(Capabilities.VOLUME_MASK_NEW_AG)
cap.set(Capabilities.VOLUME_UNMASK)
cap.set(Capabilities.FS)
cap.set(Capabilities.FS_CREATE)

[Gris Ge]

* Check VOLUME_MASK_NEW_AG capability and test 'volume-mask --init <INIT_ID>'
command.

Signed-off-by: Gris Ge <***@redhat.com>
---
test/cmdtest.py | 28 ++++++++++++++++++++++++++++
1 file changed, 28 insertions(+)

diff --git a/test/cmdtest.py b/test/cmdtest.py
index 790fa39..fa09870 100755
--- a/test/cmdtest.py
+++ b/test/cmdtest.py
@@ -46,6 +46,10 @@

(SYS_STATUS,) = (2,)

+# access group listing index
+AG_QUERY_ID_INDEX = 0
+AG_QUERY_INIT_IDS_INDEX = 2
+
iqn = ['iqn.1994-05.com.domain:01.89bd01', 'iqn.1994-05.com.domain:01.89bd02']

cmd = "lsmcli"
@@ -214,6 +218,8 @@ def access_group_delete(group_id):
def volume_mask(group, volume_id):
call([cmd, 'volume-mask', '--ag', group, '--vol', volume_id])

+def volume_mask_init(init_id, volume_id):
+ call([cmd, 'volume-mask', '--init', init_id, '--vol', volume_id])

def volume_unmask(group, volume_id):
call([cmd, 'volume-unmask', '--ag', group, '--vol', volume_id])
@@ -617,6 +623,27 @@ def test_mapping(cap, system_id):
if cap['ACCESS_GROUP_DELETE']:
access_group_delete(ag_id)

+def test_volume_mask_init(cap, system_id):
+ # Test volume mask using init id.
+ # Assuming we are testing against simulator
+ if cap['VOLUME_MASK_NEW_AG']:
+ pool_id = name_to_id(OP_POOL, test_pool_name)
+ iqn1 = random_iqn()
+ vol_id = create_volume(pool_id)
+ volume_mask_init(iqn1, vol_id)
+ out = call([cmd, '-t' + sep, 'la', '--vol', vol_id])[1]
+ new_ag_id = None
+ for ag in parse(out):
+ if ag[AG_QUERY_INIT_IDS_INDEX] == iqn1:
+ new_ag_id = ag[AG_QUERY_ID_INDEX]
+ break
+ if new_ag_id is None:
+ raise Exception("Failed to find newly created access group by "
+ "volume-mask --init")
+
+ volume_unmask(new_ag_id, vol_id)
+ access_group_delete(new_ag_id)
+ volume_delete(vol_id)

def test_nfs_operations(cap, system_id):
pass
@@ -704,6 +731,7 @@ def run_all_tests(cap, system_id):
create_all(cap, system_id)

test_mapping(cap, system_id)
+ test_volume_mask_init(cap, system_id)

search_test(cap, system_id)

[Gris Ge]

* LSM_CAP_VOLUME_MASK_NEW_AG (C)
lsm.Capabilities.VOLUME_MASK_NEW_AG (Python)

Indicate plugin accept user generated access group when doing volume
mask.
Plugin which support access group create should __NOT__ use support
capability.
This is only for plugin which does not support creating new
access group but still want user to mask volume to new initiator.

* LSM_ACCESS_GROUP_USER_GENERATED_FAKE_AG_ID (C)
lsm.AccessGroup.USER_GENERATED_FAKE_AG_ID (Python)

The string used as lsm.AccessGroup.id for user generated
access group for volume mask.

* Example python client code:

new_access_group = AccessGroup(
AccessGroup.USER_GENERATED_FAKE_AG_ID,
'', [new_initiator_id],
init_type, system_id)
lsm_client_obj.volume_mask(volume, new_access_group)

Signed-off-by: Gris Ge <***@redhat.com>
---
c_binding/include/libstoragemgmt/libstoragemgmt_capabilities.h | 1 +
c_binding/include/libstoragemgmt/libstoragemgmt_types.h | 4 ++++
python_binding/lsm/_data.py | 4 ++++
3 files changed, 9 insertions(+)

diff --git a/c_binding/include/libstoragemgmt/libstoragemgmt_capabilities.h b/c_binding/include/libstoragemgmt/libstoragemgmt_capabilities.h
index 7d6182c..3d5f28a 100644
--- a/c_binding/include/libstoragemgmt/libstoragemgmt_capabilities.h
+++ b/c_binding/include/libstoragemgmt/libstoragemgmt_capabilities.h
@@ -75,6 +75,7 @@ typedef enum {
LSM_CAP_ACCESS_GROUP_CREATE_ISCSI_IQN = 47, /**< Create iSCSI access group */
LSM_CAP_ACCESS_GROUP_INITIATOR_ADD_ISCSI_IQN = 48, /**< For empty access group, this indicates it can add iSCSI IQN to it */

+ LSM_CAP_VOLUME_MASK_NEW_AG = 49,
LSM_CAP_VOLUME_ISCSI_CHAP_AUTHENTICATION = 53, /**< If you can configure iSCSI chap authentication */

LSM_CAP_VOLUME_THIN = 55, /**< Thin provisioned volumes are supported */
diff --git a/c_binding/include/libstoragemgmt/libstoragemgmt_types.h b/c_binding/include/libstoragemgmt/libstoragemgmt_types.h
index 309a5e8..2761123 100644
--- a/c_binding/include/libstoragemgmt/libstoragemgmt_types.h
+++ b/c_binding/include/libstoragemgmt/libstoragemgmt_types.h
@@ -157,6 +157,10 @@ typedef enum {
LSM_ACCESS_GROUP_INIT_TYPE_ISCSI_WWPN_MIXED = 7 /**< More than 1 type */
} lsm_access_group_init_type;

+
+#define LSM_ACCESS_GROUP_USER_GENERATED_FAKE_AG_ID "ONLY FOR LSM INTERNAL USE"
+
+
/**< \enum lsm_job_status Job states */
typedef enum {
LSM_JOB_INPROGRESS = 1, /**< Job is in progress */
diff --git a/python_binding/lsm/_data.py b/python_binding/lsm/_data.py
index 067c766..2118356 100644
--- a/python_binding/lsm/_data.py
+++ b/python_binding/lsm/_data.py
@@ -473,6 +473,8 @@ class AccessGroup(IData):
INIT_TYPE_ISCSI_IQN = 5
INIT_TYPE_ISCSI_WWPN_MIXED = 7

+ USER_GENERATED_FAKE_AG_ID = "ONLY FOR LSM INTERNAL USE"
+
def __init__(self, _id, _name, _init_ids, _init_type, _system_id,
_plugin_data=None):
self._id = _id
@@ -667,6 +669,8 @@ class Capabilities(IData):
ACCESS_GROUP_INITIATOR_ADD_ISCSI_IQN = 48
# For empty access group, this indicate it can add iSCSI IQN into it.

+ VOLUME_MASK_NEW_AG = 49
+
VOLUME_ISCSI_CHAP_AUTHENTICATION = 53

VOLUME_THIN = 55

[Tony Asleson]

Hi Gris,

Python: AccessGroup.USER_GENERATED_FAKE_AG_ID

The name space of the class AccessGroup makes the AG redundant in the
constant IMHO. The constant isn't user generated either.

I'm thinking that simply using:

AccessGroup.FAKE_ID

or maybe

AccessGroup.RSVD_ID

would be sufficient?

In addition, please remove the C constant out of this patch series. I
will add that with the separate C patch which changes the visibility of
the function to allocate an access group.

Thanks,
Tony

Post by Gris Ge
* LSM_CAP_VOLUME_MASK_NEW_AG (C)
lsm.Capabilities.VOLUME_MASK_NEW_AG (Python)
Indicate plugin accept user generated access group when doing volume
mask.
Plugin which support access group create should __NOT__ use support
capability.
This is only for plugin which does not support creating new
access group but still want user to mask volume to new initiator.
* LSM_ACCESS_GROUP_USER_GENERATED_FAKE_AG_ID (C)
lsm.AccessGroup.USER_GENERATED_FAKE_AG_ID (Python)
The string used as lsm.AccessGroup.id for user generated
access group for volume mask.
new_access_group = AccessGroup(
AccessGroup.USER_GENERATED_FAKE_AG_ID,
'', [new_initiator_id],
init_type, system_id)
lsm_client_obj.volume_mask(volume, new_access_group)
---
c_binding/include/libstoragemgmt/libstoragemgmt_capabilities.h | 1 +
c_binding/include/libstoragemgmt/libstoragemgmt_types.h | 4 ++++
python_binding/lsm/_data.py | 4 ++++
3 files changed, 9 insertions(+)
diff --git a/c_binding/include/libstoragemgmt/libstoragemgmt_capabilities.h b/c_binding/include/libstoragemgmt/libstoragemgmt_capabilities.h
index 7d6182c..3d5f28a 100644
--- a/c_binding/include/libstoragemgmt/libstoragemgmt_capabilities.h
+++ b/c_binding/include/libstoragemgmt/libstoragemgmt_capabilities.h
@@ -75,6 +75,7 @@ typedef enum {
LSM_CAP_ACCESS_GROUP_CREATE_ISCSI_IQN = 47, /**< Create iSCSI access group */
LSM_CAP_ACCESS_GROUP_INITIATOR_ADD_ISCSI_IQN = 48, /**< For empty access group, this indicates it can add iSCSI IQN to it */
+ LSM_CAP_VOLUME_MASK_NEW_AG = 49,
LSM_CAP_VOLUME_ISCSI_CHAP_AUTHENTICATION = 53, /**< If you can configure iSCSI chap authentication */
LSM_CAP_VOLUME_THIN = 55, /**< Thin provisioned volumes are supported */
diff --git a/c_binding/include/libstoragemgmt/libstoragemgmt_types.h b/c_binding/include/libstoragemgmt/libstoragemgmt_types.h
index 309a5e8..2761123 100644
--- a/c_binding/include/libstoragemgmt/libstoragemgmt_types.h
+++ b/c_binding/include/libstoragemgmt/libstoragemgmt_types.h
@@ -157,6 +157,10 @@ typedef enum {
LSM_ACCESS_GROUP_INIT_TYPE_ISCSI_WWPN_MIXED = 7 /**< More than 1 type */
} lsm_access_group_init_type;
+
+#define LSM_ACCESS_GROUP_USER_GENERATED_FAKE_AG_ID "ONLY FOR LSM INTERNAL USE"
+
+
/**< \enum lsm_job_status Job states */
typedef enum {
LSM_JOB_INPROGRESS = 1, /**< Job is in progress */
diff --git a/python_binding/lsm/_data.py b/python_binding/lsm/_data.py
index 067c766..2118356 100644
--- a/python_binding/lsm/_data.py
+++ b/python_binding/lsm/_data.py
INIT_TYPE_ISCSI_IQN = 5
INIT_TYPE_ISCSI_WWPN_MIXED = 7
+ USER_GENERATED_FAKE_AG_ID = "ONLY FOR LSM INTERNAL USE"
+
def __init__(self, _id, _name, _init_ids, _init_type, _system_id,
self._id = _id
ACCESS_GROUP_INITIATOR_ADD_ISCSI_IQN = 48
# For empty access group, this indicate it can add iSCSI IQN into it.
+ VOLUME_MASK_NEW_AG = 49
+
VOLUME_ISCSI_CHAP_AUTHENTICATION = 53
VOLUME_THIN = 55

[Gris Ge]

* Allowing user generated AccessGroup for volume mask.
Plugin will do access group create before volume mask.

* User generated AccessGroup is identified by
lsm.AccessGroup.id == AccessGroup.USER_GENERATED_FAKE_AG_ID

Signed-off-by: Gris Ge <***@redhat.com>
---
plugin/sim/simulator.py | 22 ++++++++++++++++++++--
1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/plugin/sim/simulator.py b/plugin/sim/simulator.py
index 79b6df5..8d021d1 100644
--- a/plugin/sim/simulator.py
+++ b/plugin/sim/simulator.py
@@ -16,8 +16,8 @@
# Author: tasleson
# Gris Ge <***@redhat.com>

-from lsm import (uri_parse, VERSION, Capabilities, INfs,
- IStorageAreaNetwork, search_property)
+from lsm import (uri_parse, VERSION, Capabilities, INfs, AccessGroup,
+ IStorageAreaNetwork, search_property, LsmError, ErrorNumber)

from simarray import SimArray

@@ -180,6 +180,24 @@ def access_group_initiator_delete(self, access_group, init_id, init_type,
return SimPlugin._sim_data_2_lsm(sim_ag)

def volume_mask(self, access_group, volume, flags=0):
+ if access_group.id == AccessGroup.USER_GENERATED_FAKE_AG_ID:
+ # User want to create new access group
+ # when doing volume masking. It's only for testing lsmcli.
+ systems = self.sim_array.systems()
+ system = None
+ for tmp_system in systems:
+ if tmp_system.id == access_group.system_id:
+ system = tmp_system
+ break
+ if system is None:
+ raise LsmError(
+ ErrorNumber.NOT_FOUND_SYSTEM,
+ "System %s not found" % access_group.system_id)
+
+ access_group = self.sim_array.access_group_create(
+ access_group.init_ids[0], access_group.init_ids[0],
+ access_group.init_type, system)
+
return self.sim_array.volume_mask(
access_group.id, volume.id, flags)

[Gris Ge]

* Previously the lsmcli 'volume-mask' will fail if using with '--init'
argument. The root cause is the argument parser store initiator as
a list while parse_convert_init() is expecting a string.

* Updated '--init' argument command line help of 'volume-mask'.

* Fix incorrect command line help about '--init-id' which has been changed
to '--init'.

* Use AccessGroup.USER_GENERATED_FAKE_AG_ID as the AccessGroup.id for
masking volume with '--init' argument.

* Manpage updated for 'volume-mask' command.

Signed-off-by: Gris Ge <***@redhat.com>
---
doc/man/lsmcli.1.in | 15 ++++++++++++---
tools/lsmcli/cmdline.py | 15 +++++++++------
2 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/doc/man/lsmcli.1.in b/doc/man/lsmcli.1.in
index ec4e3eb..6e6ad97 100644
--- a/doc/man/lsmcli.1.in
+++ b/doc/man/lsmcli.1.in
@@ -325,14 +325,23 @@ Required. The ID of volume to query access.

.SS volume-mask
.TP 15
-Grant access group RW access to certain volume. Like LUN masking
-or NFS export.
+Grant access group RW access to certain volume.
+For plugin support VOLUME_MASK_NEW_AG capability, \fB--init\fR argument could
+be used to create new access group create along when masking volume.
+Either \fB--ag\fR or \f--init\fR should be defined.
.TP
\fB--vol\fR \fI<VOL_ID>\fR
Required. The ID of volume to access.
.TP
\fB--ag\fR \fI<AG_ID>\fR
-Required. The ID of access group to grant.
+Optional. The ID of access group to grant.
+This argument is mutually exclusive with \fB--init\fR argument.
+.TP
+\fB--init\fR \fI<INIT_ID>\fR
+Optional. The initiator ID, WWPN or iSCSI IQN. New access group will be
+created with this initiator ID when masking volume. Require
+VOLUME_MASK_NEW_AG capability.
+This argument is mutually exclusive with \fB--init\fR argument.

.SS volume-unmask
.TP 15
diff --git a/tools/lsmcli/cmdline.py b/tools/lsmcli/cmdline.py
index 66163b3..cad8d4a 100644
--- a/tools/lsmcli/cmdline.py
+++ b/tools/lsmcli/cmdline.py
@@ -88,7 +88,7 @@ def parse_convert_init(init_id):
if valid:
return (init_id, init_type)

- raise ArgError("--init-id %s is not a valid WWPN or iSCSI IQN" % init_id)
+ raise ArgError("--init %s is not a valid WWPN or iSCSI IQN" % init_id)


## This class represents a command line argument error
@@ -327,9 +327,10 @@ def _get_item(l, the_id, friendly_name='item', raise_error=True):
],
optional=[
dict(ag_id_opt),
- dict(name='--init', metavar='<INIT_ID>', action='append',
- help='Initiator ID, only used when access-group-create is '
- 'not supported'),
+ dict(name='--init', metavar='<INIT_ID>',
+ help="Initiator ID, create new access group with defined "
+ "initiator when masking volume. "
+ "Require VOLUME_MASK_NEW_AG capability."),
],
),

@@ -959,7 +960,7 @@ def access_group_volumes(self, args):
def iscsi_chap(self, args):
(init_id, init_type) = parse_convert_init(args.init)
if init_type != AccessGroup.INIT_TYPE_ISCSI_IQN:
- raise ArgError("--init-id %s is not a valid iSCSI IQN" % args.init)
+ raise ArgError("--init %s is not a valid iSCSI IQN" % args.init)

self.c.iscsi_chap_auth(init_id, args.in_user,
self.args.in_pass,
@@ -1252,7 +1253,9 @@ def volume_mask(self, args):

if args.init:
(init_id, init_type) = parse_convert_init(args.init)
- ag = AccessGroup(0, '', [init_id], init_type, vol.system_id)
+ ag = AccessGroup(
+ AccessGroup.USER_GENERATED_FAKE_AG_ID, '', [init_id],
+ init_type, vol.system_id)
self.c.volume_mask(ag, vol)

def volume_unmask(self, args):

[Tony Asleson]

Post by Gris Ge
* Previously the lsmcli 'volume-mask' will fail if using with '--init'
argument. The root cause is the argument parser store initiator as
a list while parse_convert_init() is expecting a string.
* Updated '--init' argument command line help of 'volume-mask'.
* Fix incorrect command line help about '--init-id' which has been changed
to '--init'.
* Use AccessGroup.USER_GENERATED_FAKE_AG_ID as the AccessGroup.id for
masking volume with '--init' argument.
* Manpage updated for 'volume-mask' command.
---
doc/man/lsmcli.1.in | 15 ++++++++++++---
tools/lsmcli/cmdline.py | 15 +++++++++------
2 files changed, 21 insertions(+), 9 deletions(-)
diff --git a/doc/man/lsmcli.1.in b/doc/man/lsmcli.1.in
index ec4e3eb..6e6ad97 100644
--- a/doc/man/lsmcli.1.in
+++ b/doc/man/lsmcli.1.in
@@ -325,14 +325,23 @@ Required. The ID of volume to query access.
.SS volume-mask
.TP 15
-Grant access group RW access to certain volume. Like LUN masking
-or NFS export.
+Grant access group RW access to certain volume.
+For plugin support VOLUME_MASK_NEW_AG capability, \fB--init\fR argument could
+be used to create new access group create along when masking volume.
+Either \fB--ag\fR or \f--init\fR should be defined.
.TP
\fB--vol\fR \fI<VOL_ID>\fR
Required. The ID of volume to access.
.TP
\fB--ag\fR \fI<AG_ID>\fR
-Required. The ID of access group to grant.
+Optional. The ID of access group to grant.
+This argument is mutually exclusive with \fB--init\fR argument.
+.TP
+\fB--init\fR \fI<INIT_ID>\fR
+Optional. The initiator ID, WWPN or iSCSI IQN. New access group will be
+created with this initiator ID when masking volume. Require
+VOLUME_MASK_NEW_AG capability.
+This argument is mutually exclusive with \fB--init\fR argument.

To me we aren't really creating an access group. We are creating a
dummy access group to convey the initiator id to make the API call. The
access group name (meta data) gets thrown away.

When I read this comment I get the impression a new AG is created. What
if a user does this syntax on an array that does support access groups?
Are we going to create one?

For targetd, what does the access group listing look like?

Post by Gris Ge
.SS volume-unmask
.TP 15
diff --git a/tools/lsmcli/cmdline.py b/tools/lsmcli/cmdline.py
index 66163b3..cad8d4a 100644
--- a/tools/lsmcli/cmdline.py
+++ b/tools/lsmcli/cmdline.py
return (init_id, init_type)
- raise ArgError("--init-id %s is not a valid WWPN or iSCSI IQN" % init_id)
+ raise ArgError("--init %s is not a valid WWPN or iSCSI IQN" % init_id)
## This class represents a command line argument error
],
optional=[
dict(ag_id_opt),
- dict(name='--init', metavar='<INIT_ID>', action='append',
- help='Initiator ID, only used when access-group-create is '
- 'not supported'),
+ dict(name='--init', metavar='<INIT_ID>',
+ help="Initiator ID, create new access group with defined "
+ "initiator when masking volume. "
+ "Require VOLUME_MASK_NEW_AG capability."),

See comment above.

Thanks,
Tony

[Tony Asleson]

Before we make public API changes to make this one case work, perhaps we
should seriously consider adding access group support to targetd?

If we did this, what are the repercussions be for the existing API vs.
doing what this patch series is suggesting?

Thoughts?

Thanks,
Tony

Post by Gris Ge
[PATCH] lsmcli: Fix bug of volume-masking '--init' option.
1. Split one patch into patches.
2. Add new capability to indicate this special user case of volume mask.
3. Add new constant for ID of access group to identify a user generated
access group.
4. Updated cmdtest.py to check VOLUME_MASK_NEW_AG capability before
test in case someone run cmdtest.py against simc plugin.
5. Targetd plugin will indicate support of VOLUME_MASK_NEW_AG via
capabilities() method.
* Require a way to allow C library user to generate a user defined
access group.
1. 'make distcheck' PASS
2. Tested on targetd via lsmcli for masking volume to new iSCSI IQN.
LSM Library: New constants for initiator only plugin.
Simulator Plugin: Add support VOLUME_MASK_NEW_AG
Targetd Plugin: Set VOLUME_MASK_NEW_AG capability
lsmcli: Fix bug of command 'volume-mask --init'
cmdtest.py: Add test for --init argument of 'volume-mask' command.
.../libstoragemgmt/libstoragemgmt_capabilities.h | 1 +
.../include/libstoragemgmt/libstoragemgmt_types.h | 4 ++++
doc/man/lsmcli.1.in | 15 +++++++++---
plugin/sim/simulator.py | 22 +++++++++++++++--
plugin/targetd/targetd.py | 1 +
python_binding/lsm/_data.py | 4 ++++
test/cmdtest.py | 28 ++++++++++++++++++++++
tools/lsmcli/cmdline.py | 15 +++++++-----
8 files changed, 79 insertions(+), 11 deletions(-)

[Gris Ge]

Post by Tony Asleson
Before we make public API changes to make this one case work, perhaps we
should seriously consider adding access group support to targetd?
If we did this, what are the repercussions be for the existing API vs.
doing what this patch series is suggesting?
Thoughts?

I would suggest we simply fix lsmcli bug and mark this as obsoleted or
deprecated once we get targetd supported.

Post by Tony Asleson
Thanks,
Tony

Thank you.

[Tony Asleson]

Post by Gris Ge
I would suggest we simply fix lsmcli bug and mark this as obsoleted or
deprecated once we get targetd supported.

Originally the library was designed so that it could support:

a. Arrays that don't support access groups
b. Arrays that do support access groups

We took b. out and merged it into a. to fake it. Unfortunately this is
kind of a kluge and it shows. API users need to dummy up an access
group when masking for the sole purpose of calling the API if the
initiator isn't already known to the array.

Yes there is a bug, but the real question is, do we want to support
arrays that don't support access groups? If yes, then I believe we
should modify the API so that this isn't such a kluge.

Adding something to the API to fix a bug that we are planning on
deprecating is wrong.

Thanks!

Regards,
Tony