targetclid: add daemonize component for targetcli

[pkalever]

Problem:

Overall creation time of a block using targetcli is raising linearly as the
block count increase.

This is because of the recurring issue involving refresh(reload) at
multiple objects/places, as the LIO's configfs is deeply nested.

Earlier discussion of the problem statement with stats and graphs about delays:
http://bit.ly/targetcli-create-delay

Solution:

Introduce a daemon component for targetcli[d] which will retain state of
Configshell object in memory, so that any new requests can directly use it,
instead of loading the storageObjects/targetObjects again.

Details about "how to use it ?":

$ systemctl start targetclid

$ systemctl status targetclid

● targetclid.service - Targetcli daemon
   Loaded: loaded (/usr/lib/systemd/system/targetclid.service; disabled; vendor preset: disabled)
   Active: active (running) since Wed 2019-04-10 12:19:51 IST; 2h 17min ago
 Main PID: 3950 (targetclid)
    Tasks: 3 (limit: 4915)
   CGroup: /system.slice/targetclid.service
           └─3950 /usr/bin/python /usr/bin/targetclid

Apr 10 12:19:51 localhost.localdomain systemd[1]: Started Targetcli daemon.

$ targetcli help

Usage: /usr/bin/targetcli [--version|--help|CMD|--tcp]
  --version		Print version
  --help		Print this information
  CMD			Run targetcli shell command and exit
  <nothing>		Enter configuration shell
  --tcp CMD		Pass targetcli command to targetclid
  --tcp <nothing>	Enter multi-line command mode for targetclid
See man page for more information.

One line command usage:

$ targetcli --tcp CMD

Eg:
$ targetcli --tcp pwd
/

Multiple line commands usage:

$ targetcli --tcp
CMD1
CMD2
.
.
CMDN
exit

Eg:
$ targetcli --tcp

^Tab
/              backstores/    iscsi/         loopback/      vhost/         xen-pvscsi/    cd
clearconfig    exit           get            help           ls             pwd            refresh
restoreconfig  saveconfig     set            status


pwd
get global logfile
get global auto_save_on_exit
/ saveconfig
exit

output follows:
/
logfile=/var/log/gluster-block/gluster-block-configshell.log
auto_save_on_exit=false
Configuration saved to /etc/target/saveconfig.json

Stats with and without changes:

Running simple 'pwd' command after creating 1000 blocks on a node:

Without this change:

$ time targetcli pwd
/

real 0m8.963s
user 0m7.775s
sys 0m1.103s

with daemonize changes:

$ time targetcli --tcp "pwd"
/

real 0m0.126s
user 0m0.099s
sys 0m0.024s

Thanks to Maurizio for hangingout with me for all the discussions involved.

Signed-off-by: Prasanna Kumar Kalever [email protected]

[pkalever]

@maurizio-lombardi please review :-)

[gonzoleeman]

As a regular user of targetcli, and one who supports it for others, I like the speed up but I dislike most the rest of this approach.

I like that the daemon caches information and speeds things up, but I dislike the complexity, and I really dislike having to pass some "--tcp" argument to targetcli -- shouldn't there be some sensible default? For example, when I run "iscsiadm" and it needs to talk to iscsid, I don't have to give it any extra parameter for that.

And, as a practicle matter, getting a distro to automatically enable a new service is difficult. So now targetcli will have two services instead of one?

Just my $.02 ...

[gonzoleeman]

After reviewing your actual code, I would like to add:

• I'm surprised it's in Python, but I guess that makes sense
• Since you seem to be talking using a socket, then the daemon should be socket-activated,
  so that just calling it starts the service. See iscsid.socket for an example.

[lxbsz]

Usage: /usr/bin/targetcli [--version|--help|CMD|--tcp]
--version Print version
--help Print this information
CMD Run targetcli shell command and exit
Enter configuration shell
--tcp CMD Pass targetcli command to targetclid
--tcp Enter multi-line command mode for targetclid
See man page for more information.

@pkalever
BTW, here the CMD without the --tcp, means it will still load the whole storageObjects/targetObjects every time as before it did ?

If so why not just make the --tcp as default in new targetcli version ?

Thanks.

[pkalever]

As a regular user of targetcli, and one who supports it for others, I like the speed up but I dislike most the rest of this approach.

I like that the daemon caches information and speeds things up, but I dislike the complexity, and I really dislike having to pass some "--tcp" argument to targetcli -- shouldn't there be some sensible default? For example, when I run "iscsiadm" and it needs to talk to iscsid, I don't have to give it any extra parameter for that.

@gonzoleeman First of all, thanks a bunch for your review and thoughts about this change.

The complexity part:

• Separate cli option was choose, keeping in mind that, we don't want to break the default behavior with the immediate release. Also, I'm very bad at naming options, excuse me there and feel free to suggest a better substitute for '--tcp'.

• My V2 patches, implements a global option for setting daemonize approach as default, see f5815e1

• So, now we have both 'auto_use_daemon' and cli option '--tcp' (to help non-regular usage of daemon)
  [ I welcome more comments on this ]

And, as a practicle matter, getting a distro to automatically enable a new service is difficult. So now targetcli will have two services instead of one?

Just my $.02 ...

After reviewing your actual code, I would like to add:

• I'm surprised it's in Python, but I guess that makes sense
• Since you seem to be talking using a socket, then the daemon should be socket-activated,
  so that just calling it starts the service. See iscsid.socket for an example.

I like this idea, hence have implemented the same here ae693a7

Hope I have not missed any point[s] from your valuable review comments.

TODO:
Once we are done with naming (cli and gobal option) part, will refresh man pages.

Thanks!

[pkalever]

@pkalever
BTW, here the CMD without the --tcp, means it will still load the whole storageObjects/targetObjects every time as before it did ?

@lxbsz

Yes

If so why not just make the --tcp as default in new targetcli version ?

My reply above should have already made my thoughts clear on this, pasting it here again to answer this specific query:

Separate cli option was choose, keeping in mind that, we don't want to break the default behavior with the immediate release.

Checkout V2 patches which implements a global option to do this.

Thanks!

[gonzoleeman]

I am still not happy about having a daemon running python, as root. I'm no security expert, but this worries me, as hacking on a python script for some bad person is easier than hacking on a C binary. I know the C-to-python interface sucks, so I won't ask for the daemon to be written in C (or Go, or whatever). Never the less, if nobody else objects, it seems reasonable.

I request, though, that you leave the default at "no daemon", so that paranoid folks can skip the daemon, at least for now.

[maurizio-lombardi]

Hello Prasanna, I think you should add "auto_use_daemon" to the "default_prefs" array in scripts/targetcli. The default value should be False.

[rjt]

I am sure many people i know think I wear a tinfoil hat. In my world of virtualization, hosts access block storage over tcp iSCSi implementations that are on private isolated storage vLANs, so i say turn that daemon on by default. We could really use a speedup. One option, Our Storage LAN has no access to the internet, so if ping 8.8.8.8 fails, raise that daemon.

…

On Thu, Apr 18, 2019 at 8:31 AM Maurizio Lombardi ***@***.***> wrote: Hello Prasanna, I think you should add "auto_use_daemon" to the "default_prefs" array in scripts/targetcli. The default value should be False. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#132 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACX7F27X7RO4QJXDYVFJ3TPRBZ3FANCNFSM4HE2IZFA> .

[pkalever]

@maurizio-lombardi Sorry for delaying this for so long. Added auto_use_daemon to default_prefs.
Please take a look.

Thanks!

[pkalever]

Changes:
V4: Added Usage() and Version() functions.
V3: added auto_use_daemon=False to default_prefs
V2: Added Socket based activation and global auto_use_daemon option
V1: Initial Patch

[maurizio-lombardi]

I am sure many people i know think I wear a tinfoil hat. In my world of virtualization, hosts access block storage over tcp iSCSi implementations that are on private isolated storage vLANs, so i say turn that daemon on by default. We could really use a speedup.

I understand your point but I think we should be cautious here and avoid the risk of breaking the normal behaviour until this feature will be properly tested.