[PATCH] Block.py: New methods: initiators, initiator_create and initiator_delete

[Gris Ge]

 * New method 'initiators':
    Usage:      Return a list of iSCSI initiator.
    Parameter:  NULL
    Return:     [{'initiator_wwn': initiator_wwn, 'wwn_type': wwn_type},]

 * New method 'initiator_create':
    Usage:      Create a initiators.
    Parameter:  initiator_wwn   # iSCSI IQN
    Return:     [{'initiator_wwn': initiator_wwn, 'wwn_type': wwn_type},]

 * New method 'initiator_delete':
    Usage:      Delete a initiators along with masking information.
    Parameter:  initiator_wwn   # iSCSI IQN
    Return:     None

 * With these methods, libstoragemgmt can use routine method to do
   volume masking:

    1. Create access group(for targetd, each initiator is a access group).
    2. Mask volume to this new access group.

   This can simplify volume mask design of libstoragemgmt as all found storage
   array are following above way to do volume masking.

 * Tested via libstoragemgmt targetd plugin.

Signed-off-by: Gris Ge <fge(a)redhat.com&gt;
---
 targetd/block.py | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/targetd/block.py b/targetd/block.py
index e8206b2..cbcfaee 100644
--- a/targetd/block.py
+++ b/targetd/block.py
@@ -105,6 +105,9 @@ def initialize(config_dict):
         export_create=export_create,
         export_destroy=export_destroy,
         initiator_set_auth=initiator_set_auth,
+        initiators=initiators,
+        initiator_create=initiator_create,
+        initiator_delete=initiator_delete,
     )
 
 
@@ -277,6 +280,39 @@ def export_destroy(req, pool, vol, initiator_wwn):
     RTSRoot().save_to_file()
 
 
+def initiators(req):
+    try:
+        fm = FabricModule('iscsi')
+        t = Target(fm, target_name, mode='lookup')
+        tpg = TPG(t, 1, mode='lookup')
+    except RTSLibNotInCFS:
+        return []
+
+    initiators = []
+    for na in tpg.node_acls:
+        initiators.append(
+            dict(initiator_wwn=na.node_wwn, wwn_type=na.wwn_type))
+    return initiators
+
+
+def initiator_create(req, initiator_wwn):
+    fm = FabricModule('iscsi')
+    t = Target(fm, target_name)
+    tpg = TPG(t, 1)
+    na = NodeACL(tpg, initiator_wwn)
+    RTSRoot().save_to_file()
+    return dict(initiator_wwn=na.node_wwn, wwn_type=na.wwn_type)
+
+
+def initiator_delete(req, initiator_wwn):
+    fm = FabricModule('iscsi')
+    t = Target(fm, target_name)
+    tpg = TPG(t, 1)
+    na = NodeACL(tpg, initiator_wwn)
+    na.delete()
+    RTSRoot().save_to_file()
+
+
 def initiator_set_auth(req, initiator_wwn, in_user, in_pass, out_user,
                        out_pass):
     fm = FabricModule('iscsi')
-- 
1.8.3.1

[Andy Grover]

On 11/07/2014 03:39 AM, Gris Ge wrote:
>    * New method 'initiators':
>       Usage:      Return a list of iSCSI initiator.
>       Parameter:  NULL
>       Return:     [{'initiator_wwn': initiator_wwn, 'wwn_type':
> wwn_type},]
>
>    * New method 'initiator_create':
>       Usage:      Create a initiators. 
Create an initiator.

>       Parameter:  initiator_wwn   # iSCSI IQN
>       Return:     [{'initiator_wwn': initiator_wwn, 'wwn_type':
> wwn_type},] 
Hi Gris. Following the style of the other current targetd API methods, 
initiator_create should not return anything on success. Can we keep that 
style for this new method? Would this require a change in the 
libstoragemgmt target plugin patch that goes along with this patch?

Thanks -- Andy

[Gris Ge]

On Mon, Nov 10, 2014 at 10:05:00AM -0800, Andy Grover wrote:
>  On 11/07/2014 03:39 AM, Gris Ge wrote:
>  >  * New method 'initiators':
>  >     Usage:      Return a list of iSCSI initiator.
>  >     Parameter:  NULL
>  >     Return:     [{'initiator_wwn': initiator_wwn, 'wwn_type':
> wwn_type},]
>  >
>  >  * New method 'initiator_create':
>  >     Usage:      Create a initiators.
>  
>  Create an initiator.
>  
>  >     Parameter:  initiator_wwn   # iSCSI IQN
>  >     Return:     [{'initiator_wwn': initiator_wwn, 'wwn_type':
> wwn_type},]
>  
>  Hi Gris. Following the style of the other current targetd API
>  methods, initiator_create should not return anything on success. Can
>  we keep that style for this new method? Would this require a change
>  in the libstoragemgmt target plugin patch that goes along with this
>  patch? 
Hi Andy,

No problem. I follow that style and provide both tgt and lsm V2 patch.

Thank you.
Best regards.
>  
>  Thanks -- Andy
>   
-- 
Gris Ge

[PATCH V2] Block.py: New methods: initiators, initiator_create and initiator_delete

[Gris Ge]

 * New method 'initiators':
    Usage:      Return a list of iSCSI initiator.
    Parameter:  NULL
    Return:     [{'initiator_wwn': initiator_wwn, 'wwn_type': wwn_type},]

 * New method 'initiator_create':
    Usage:      Create a initiator.
    Parameter:  initiator_wwn   # iSCSI IQN
    Return:     None

 * New method 'initiator_delete':
    Usage:      Delete a initiator along with masking information.
    Parameter:  initiator_wwn   # iSCSI IQN
    Return:     None

 * Tested via libstoragemgmt targetd plugin.

Changes in V2:

 * Fix typo.
 * Add method document as docstring.
 * Change initiator_create() to return None if succeeded.

Signed-off-by: Gris Ge <fge(a)redhat.com&gt;
---
 targetd/block.py | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/targetd/block.py b/targetd/block.py
index e8206b2..941c2a3 100644
--- a/targetd/block.py
+++ b/targetd/block.py
@@ -105,6 +105,9 @@ def initialize(config_dict):
         export_create=export_create,
         export_destroy=export_destroy,
         initiator_set_auth=initiator_set_auth,
+        initiators=initiators,
+        initiator_create=initiator_create,
+        initiator_delete=initiator_delete,
     )
 
 
@@ -277,6 +280,53 @@ def export_destroy(req, pool, vol, initiator_wwn):
     RTSRoot().save_to_file()
 
 
+def initiators(req):
+    """
+    Usage:      Return a list of iSCSI initiators.
+    Parameter:  NULL
+    Return:     [{'initiator_wwn': initiator_wwn, 'wwn_type':
'iqn'},]
+    """
+    try:
+        fm = FabricModule('iscsi')
+        t = Target(fm, target_name, mode='lookup')
+        tpg = TPG(t, 1, mode='lookup')
+    except RTSLibNotInCFS:
+        return []
+
+    initiators = []
+    for na in tpg.node_acls:
+        initiators.append(
+            dict(initiator_wwn=na.node_wwn, wwn_type=na.wwn_type))
+    return initiators
+
+
+def initiator_create(req, initiator_wwn):
+    """
+    Usage:      Create a initiator.
+    Parameter:  initiator_wwn   # iSCSI IQN
+    Return:     None
+    """
+    fm = FabricModule('iscsi')
+    t = Target(fm, target_name)
+    tpg = TPG(t, 1)
+    na = NodeACL(tpg, initiator_wwn)
+    RTSRoot().save_to_file()
+
+
+def initiator_delete(req, initiator_wwn):
+    """
+    Usage:      Delete a initiator along with its masking information.
+    Parameter:  initiator_wwn   # iSCSI IQN
+    Return:     None
+    """
+    fm = FabricModule('iscsi')
+    t = Target(fm, target_name)
+    tpg = TPG(t, 1)
+    na = NodeACL(tpg, initiator_wwn)
+    na.delete()
+    RTSRoot().save_to_file()
+
+
 def initiator_set_auth(req, initiator_wwn, in_user, in_pass, out_user,
                        out_pass):
     fm = FabricModule('iscsi')
-- 
1.8.3.1

[PATCH V2] Block.py: New methods: initiators, initiator_create and initiator_delete

[Tony Asleson]

Previously targetd supported just initiator operations with
libStorageMgmt.  Previously the code used:

export_list, export_create, export_destroy

Why do we need to introduce these new API calls in targetd?

Thanks,
Tony


On 11/14/2014 03:35 AM, Gris Ge wrote:
>   * New method 'initiators':
>      Usage:      Return a list of iSCSI initiator.
>      Parameter:  NULL
>      Return:     [{'initiator_wwn': initiator_wwn, 'wwn_type':
> wwn_type},]
>  
>   * New method 'initiator_create':
>      Usage:      Create a initiator.
>      Parameter:  initiator_wwn   # iSCSI IQN
>      Return:     None
>  
>   * New method 'initiator_delete':
>      Usage:      Delete a initiator along with masking information.
>      Parameter:  initiator_wwn   # iSCSI IQN
>      Return:     None
>  
>   * Tested via libstoragemgmt targetd plugin.
>  
>  Changes in V2:
>  
>   * Fix typo.
>   * Add method document as docstring.
>   * Change initiator_create() to return None if succeeded.
>  
>  Signed-off-by: Gris Ge <fge(a)redhat.com&gt;
>  ---
>   targetd/block.py | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 50 insertions(+)
>  
>  diff --git a/targetd/block.py b/targetd/block.py
>  index e8206b2..941c2a3 100644
>  --- a/targetd/block.py
>  +++ b/targetd/block.py
>  @@ -105,6 +105,9 @@ def initialize(config_dict):
>           export_create=export_create,
>           export_destroy=export_destroy,
>           initiator_set_auth=initiator_set_auth,
>  +        initiators=initiators,
>  +        initiator_create=initiator_create,
>  +        initiator_delete=initiator_delete,
>       )
>   
>   
>  @@ -277,6 +280,53 @@ def export_destroy(req, pool, vol, initiator_wwn):
>       RTSRoot().save_to_file()
>   
>   
>  +def initiators(req):
>  +    """
>  +    Usage:      Return a list of iSCSI initiators.
>  +    Parameter:  NULL
>  +    Return:     [{'initiator_wwn': initiator_wwn, 'wwn_type':
> 'iqn'},]
>  +    """
>  +    try:
>  +        fm = FabricModule('iscsi')
>  +        t = Target(fm, target_name, mode='lookup')
>  +        tpg = TPG(t, 1, mode='lookup')
>  +    except RTSLibNotInCFS:
>  +        return []
>  +
>  +    initiators = []
>  +    for na in tpg.node_acls:
>  +        initiators.append(
>  +            dict(initiator_wwn=na.node_wwn, wwn_type=na.wwn_type))
>  +    return initiators
>  +
>  +
>  +def initiator_create(req, initiator_wwn):
>  +    """
>  +    Usage:      Create a initiator.
>  +    Parameter:  initiator_wwn   # iSCSI IQN
>  +    Return:     None
>  +    """
>  +    fm = FabricModule('iscsi')
>  +    t = Target(fm, target_name)
>  +    tpg = TPG(t, 1)
>  +    na = NodeACL(tpg, initiator_wwn)
>  +    RTSRoot().save_to_file()
>  +
>  +
>  +def initiator_delete(req, initiator_wwn):
>  +    """
>  +    Usage:      Delete a initiator along with its masking information.
>  +    Parameter:  initiator_wwn   # iSCSI IQN
>  +    Return:     None
>  +    """
>  +    fm = FabricModule('iscsi')
>  +    t = Target(fm, target_name)
>  +    tpg = TPG(t, 1)
>  +    na = NodeACL(tpg, initiator_wwn)
>  +    na.delete()
>  +    RTSRoot().save_to_file()
>  +
>  +
>   def initiator_set_auth(req, initiator_wwn, in_user, in_pass, out_user,
>                          out_pass):
>       fm = FabricModule('iscsi')
>

[PATCH V2] Block.py: New methods: initiators, initiator_create and initiator_delete

[Gris Ge]

On Mon, Nov 17, 2014 at 04:09:42PM -0600, Tony Asleson wrote:
>  Previously targetd supported just initiator operations with
>  libStorageMgmt.  Previously the code used:
>  
>  export_list, export_create, export_destroy
>  
>  Why do we need to introduce these new API calls in targetd?
>  
>  Thanks,
>  Tony 
Hi Tony,

To support this in targetd:

    Mask certain volume to a new initiator.

we have two approaches to solve this:

    1. Reintroduce initiator based volume mask. Nothing need to be
       changed on targetd side.
        

    2. Access group create, then volume mask against access group.


For option 1): 
    Nothing need to be changed on targetd side, but LSM will have two
    set of volume mask API: access group masking, initiator masking.
    It's easy for design and code from LSM side, but for LSM user,
    they will have to code much more lines.

For option 2): 
    Targetd need to allow create or list a initiator with no volume
    masked.
    We use these new methods(initiators(), initiator_create(),
    initiator_delete()) to handle.

Thank you.
Best regards.

-- 
Gris Ge

[PATCH V2] Block.py: New methods: initiators, initiator_create and initiator_delete

[Tony Asleson]

On 11/18/2014 02:31 AM, Gris Ge wrote:
>  On Mon, Nov 17, 2014 at 04:09:42PM -0600, Tony Asleson wrote:
> > Previously targetd supported just initiator operations with
> > libStorageMgmt.  Previously the code used:
> >
> > export_list, export_create, export_destroy
> >
> > Why do we need to introduce these new API calls in targetd?
> >
> > Thanks,
> > Tony
>  Hi Tony,
>  
>  To support this in targetd:
>  
>      Mask certain volume to a new initiator.
>  
>  we have two approaches to solve this:
>  
>      1. Reintroduce initiator based volume mask. Nothing need to be
>         changed on targetd side.
>          
>  
>      2. Access group create, then volume mask against access group.
>  
>  
>  For option 1): 
>      Nothing need to be changed on targetd side, but LSM will have two
>      set of volume mask API: access group masking, initiator masking.
>      It's easy for design and code from LSM side, but for LSM user,
>      they will have to code much more lines.
>  For option 2): 
>      Targetd need to allow create or list a initiator with no volume
>      masked.
>      We use these new methods(initiators(), initiator_create(),
>      initiator_delete()) to handle. 

Previously the functionality was:

int LSM_DLL_EXPORT lsm_initiator_grant(lsm_connect *conn,
                                     const char *initiator_id,
                                     lsm_initiator_type initiator_type,
                                     lsm_volume *volume,
                                     lsm_access_type access,
                                     lsm_flag flags);

So the user didn't need to create or get initiator list object
separately and then do the mask.

So why couldn't we do something like (need better name):

lsm_volume_mask_initiator_as_ag(connection,
"iqn.1994-05.com.domain:01.89bd01",
LSM_ACCESS_GROUP_INIT_TYPE_ISCSI_IQN, vol, LSM_CLIENT_FLAG_RSVD);


instead of this:

lsm_string_list *initiators = lsm_string_list_alloc(1);
lsm_string_list_append(initiators, "iqn.1994-05.com.domain:01.89bd01");

lsm_access_group *ag =
lsm_access_group_record_alloc(LSM_ACCESS_GROUP_FAKE_ID, "Unused name",
initiators, LSM_ACCESS_GROUP_INIT_TYPE_ISCSI_IQN, LSM_CLIENT_FLAG_RSVD);

lsm_volume_mask(connection, ag, vol, LSM_CLIENT_FLAG_RSVD);


If we did this then I believe targetd would not need new functionality
as this is what we were supporting before.

Regards,
Tony

[PATCH V2] Block.py: New methods: initiators, initiator_create and initiator_delete

[Andy Grover]

On 11/18/2014 12:31 AM, Gris Ge wrote:
>  we have two approaches to solve this:
>
>       1. Reintroduce initiator based volume mask. Nothing need to be
>          changed on targetd side.
>
>
>       2. Access group create, then volume mask against access group.
>
>
>  For option 1):
>       Nothing need to be changed on targetd side, but LSM will have two
>       set of volume mask API: access group masking, initiator masking.
>       It's easy for design and code from LSM side, but for LSM user,
>       they will have to code much more lines.
>
>  For option 2):
>       Targetd need to allow create or list a initiator with no volume
>       masked.
>       We use these new methods(initiators(), initiator_create(),
>       initiator_delete()) to handle. 
I'm in favor of option 2. But, why those three new methods instead of 
implementing access_group_create, access_group_delete, 
access_group_initiator_add, access_group_initiator_delete, and 
access_groups?

targetd should be able to support these by treating all initiators with 
the same 'tag' attribute as belonging to the same access group.

Regards -- Andy

[PATCH V2] Block.py: New methods: initiators, initiator_create and initiator_delete

[Gris Ge]

On Tue, Nov 18, 2014 at 01:00:11PM -0800, Andy Grover wrote:
>  On 11/18/2014 12:31 AM, Gris Ge wrote:
>  >
>  >For option 2):
>  >     Targetd need to allow create or list a initiator with no volume
>  >     masked.
>  >     We use these new methods(initiators(), initiator_create(),
>  >     initiator_delete()) to handle.
>  
>  I'm in favor of option 2. But, why those three new methods instead
>  of implementing access_group_create, access_group_delete,
>  access_group_initiator_add, access_group_initiator_delete, and
>  access_groups?
>  
>  targetd should be able to support these by treating all initiators
>  with the same 'tag' attribute as belonging to the same access group.
>  
>  Regards -- Andy
>   
Hi Andy,

Thanks for the tips, I will try the 'tag' attribute.

Best regards.
-- 
Gris Ge

[PATCH V2] Block.py: New methods: initiators, initiator_create and initiator_delete

[Andy Grover]

On 11/18/2014 07:19 PM, Gris Ge wrote:
>  On Tue, Nov 18, 2014 at 01:00:11PM -0800, Andy Grover wrote:
> > On 11/18/2014 12:31 AM, Gris Ge wrote:
> >>
> >> For option 2):
> >>      Targetd need to allow create or list a initiator with no volume
> >>      masked.
> >>      We use these new methods(initiators(), initiator_create(),
> >>      initiator_delete()) to handle.
> >
> > I'm in favor of option 2. But, why those three new methods instead
> > of implementing access_group_create, access_group_delete,
> > access_group_initiator_add, access_group_initiator_delete, and
> > access_groups?
> >
> > targetd should be able to support these by treating all initiators
> > with the same 'tag' attribute as belonging to the same access group.
> >
> > Regards -- Andy
> >
>  Hi Andy,
>
>  Thanks for the tips, I will try the 'tag' attribute.
>
>  Best regards. 
I would wait for now.

I'm going to add better support for initiator groups to rtslib. Once 
that is completed then it will be much more natural to support those 
APIs in targetd.

-- Andy

[PATCH V2] Block.py: New methods: initiators, initiator_create and initiator_delete

[Gris Ge]

On Tue, Nov 18, 2014 at 09:19:55PM -0800, Andy Grover wrote:
>  >Hi Andy,
>  >
>  >Thanks for the tips, I will try the 'tag' attribute.
>  >
>  >Best regards.
>  
>  I would wait for now.
>  
>  I'm going to add better support for initiator groups to rtslib. Once
>  that is completed then it will be much more natural to support those
>  APIs in targetd.
>  
>  -- Andy
>   
Hi Andy,

As you suggested, I managed to using 'tag' of node_acl to simulate a access
group query.
But volume mask/unmask and access group initiator add/del will have many
concerns.

    1. For a initiator volume mask when requested initiator is a member of
       access group, we will got inconsistent volume masking status between
       initiators in access group. The same problem also goes to unmask.
        # We can change targetd to do access group masking whenever
        # initiator mask requested. But we cannot change targetcl.

    2. For initiator volume unmask, targetd will remove node_acl if
       that's the last mask. This is not acceptable when initiator is
       a member of access group. To support access group, we have to
       change expose_destroy() method of targetd which break backward
       compatibility.

    3. Locking issue. When adding a initiator to a access group,
       all current masking status will copy to that initiator. What if
       we do volume unmask at the same time?


Generally, supporting both initiator mask and access group mask will
cause inconsistence unless we break backward compatibility and
fix all possibilities. We also have to change targetcli also.

I would like to suggest we use previous V2 patch to allow LSM
simulator each initiator as access group.

For adding real access group support in targetcli or targetd,
removing initiator mask completely could make life much easier, which
is a major version change of course.


Thank you.
Best regards.
-- 
Gris Ge

[PATCH V2] Block.py: New methods: initiators, initiator_create and initiator_delete

[Andy Grover]

On 11/20/2014 01:36 AM, Gris Ge wrote:
>       1. For a initiator volume mask when requested initiator is a
> member of
>          access group, we will got inconsistent volume masking status between
>          initiators in access group. The same problem also goes to unmask.
>           # We can change targetd to do access group masking whenever
>           # initiator mask requested. But we cannot change targetcl. 
Let's just move to use real access groups and then only support those 
going forward.

I'm not sure what the point you were making about targetcli was, but yes 
we *could* change targetcli if needed but I don't think we'll need to. 
You can define access groups in targetcli (via the tpg/acls tag and 
untag commands) and you can also define initiators (NodeACLs in 
LIO-speak) but I'm assuming targetd, using the new NodeACLGroup support 
in rtslib-fb that I'm working on, would ignore NodeACLs not in a group 
(i.e. where nodeacl.tag == None).


>       2. For initiator volume unmask, targetd will remove node_acl if
>          that's the last mask. This is not acceptable when initiator is
>          a member of access group. To support access group, we have to
>          change expose_destroy() method of targetd which break backward
>          compatibility. 
True. I think we need to do this because what we have now is not usable 
and also probably not used by anyone :) so we should do what we need to 
in order to make it work right.

>       3. Locking issue. When adding a initiator to a access group,
>          all current masking status will copy to that initiator. What if
>          we do volume unmask at the same time? 
targetd currently serializes handling of API calls so this should not 
cause a race condition.

>  Generally, supporting both initiator mask and access group mask will
>  cause inconsistence unless we break backward compatibility and
>  fix all possibilities. We also have to change targetcli also. 
See above, let's just support access groups and break what we need to break.

>  I would like to suggest we use previous V2 patch to allow LSM
>  simulator each initiator as access group.
>
>  For adding real access group support in targetcli or targetd,
>  removing initiator mask completely could make life much easier, which
>  is a major version change of course. 
Like I said above, let's just fix it the way we want it to be long-term.

I'll have a development tree to share of the rtslib changes shortly. 
Then we can update targetd to take advantage of groups, and lastly 
update the libsm targetd plugin, if that sounds ok with everyone.

Regards -- Andy