daemonized-mode: add interactive shell support

[pkalever]

set interactive mode as default to make it appear similar to cli approach,

$ targetcli
targetcli shell version 2.1.51
Entering targetcli interactive mode for daemonized approach.
Type 'exit' to quit.

/> pwd
/
/> cd /iscsi
/> pwd
/iscsi
/> exit

Here we introduce a new global varibale daemon_use_batch_mode for switching betwe
batch and interactive modes.

$ targetcli set global daemon_use_batch_mode=true
Parameter daemon_use_batch_mode is now 'true'.

$ targetcli
targetcli shell version 2.1.51
Entering targetcli batch mode for daemonized approach.
Enter multiple commands separated by newline and type 'exit' to run them
all in one go.

/> pwd
/> cd /iscsi
/> pwd
/
/iscsi

Fixes: #160
Signed-off-by: Prasanna Kumar Kalever <[email protected]>

[iammattcoleman]

This is confusing for the user. The shell, batch mode, and single commands should behave the same regardless of whether or not the daemon is being used.

I propose one of the following:

• change the default to interactive mode and add a --batch parameter to make the shell run in batch mode
• change the shell to always run in interactive mode, and only support batch mode via multi-line redirected input (e.g. here documents or file redirection)

[pkalever]

I understand I didn't keep interactive mode as default mainly because our gluster-block tool uses daemonized batch mode already, and I fear making interactive default will break gluster-block for sure.

Now that you raised the voice, I'II will work on fixing gluster-block later.

Now, let me make interactive shell as default as to make it look alike cli mode. I will also provide a global option something like 'use_batch_mode' which should help switch between interactive and batch modes.

[pkalever]

@gonzoleeman @iammattcoleman @maurizio-lombardi @lxbsz Hope this is the expectation. In case if we want this to be part of the next release v2.1.51 (which should ideally happen early next week), then please help me with the tests and review.

Thanks!

[lxbsz]

Maybe here also could just break the loop like the above.

[lxbsz]

We can move the Line#199 just before Line#204, no need to join it here every time.

[pkalever]

@lxbsz modified the conditioning for better readability. Thanks!

[pkalever]

@lxbsz updated. Thanks!

[lxbsz]

This looks good to me.
Thanks.

[pkalever]

@iammattcoleman @gonzoleeman can you guys please share your first impression on this?

Thanks!

[iammattcoleman]

This is definitely on the right track. Thanks for working on this!

I found three bugs...

1. The prompt always shows / as the path:

targetcli shell version 2.1.51
Entering targetcli interactive mode for daemonized approach.
Type 'exit' to quit.

/> pwd
/
/> cd iscsi
/> pwd
/iscsi
/>

2. If I type a partial command and then press Alt+Backspace (which should delete to the beginning of the current word), it deletes the current word as well as the prompt. If I immediately type a new command, it will execute correctly and the command prompt will reappear afterwards, as expected. If I do not type a command and press enter, the input handler breaks.

targetcli shell version 2.1.51
Entering targetcli interactive mode for daemonized approach.
Type 'exit' to quit.

ls
o- iscsi .............................................................................................................. [Targets: 0]
/> ls
o- iscsi .............................................................................................................. [Targets: 0]




^CTraceback (most recent call last):
  File "/usr/bin/targetcli", line 289, in <module>
    main()
  File "/usr/bin/targetcli", line 245, in main
    switch_to_daemon(shell, interactive_mode)
  File "/usr/bin/targetcli", line 206, in switch_to_daemon
    call_daemon(shell, command.encode())
  File "/usr/bin/targetcli", line 150, in call_daemon
    var = sock.recv(4) # get length of data
  File "/usr/lib/python3/dist-packages/configshell_fb/shell.py", line 39, in handle_sigint
    raise KeyboardInterrupt
KeyboardInterrupt

3. Pressing Backspace followed by Enter at the prompt breaks the input loop similar to what I mentioned in (2).

Entering targetcli interactive mode for daemonized approach.
Type 'exit' to quit.

/>



^CTraceback (most recent call last):
  File "/usr/bin/targetcli", line 289, in <module>
    main()
  File "/usr/bin/targetcli", line 245, in main
    switch_to_daemon(shell, interactive_mode)
  File "/usr/bin/targetcli", line 206, in switch_to_daemon
    call_daemon(shell, command.encode())
  File "/usr/bin/targetcli", line 150, in call_daemon
    var = sock.recv(4) # get length of data
  File "/usr/lib/python3/dist-packages/configshell_fb/shell.py", line 39, in handle_sigint
    raise KeyboardInterrupt
KeyboardInterrupt

[pkalever]

This is definitely on the right track. Thanks for working on this!

Thanks @iammattcoleman for testing this.

I found three bugs...

1. The prompt always shows / as the path:

Yeah, I know this. This is because the cli will be a dummy in daemon mode, it will not have handle for root_node.

One option always returns pwd as part of the daemon response.

2. If I type a partial command and then press Alt+Backspace (which should delete to the beginning of the current word), it deletes the current word as well as the prompt. If I immediately type a new command, it will execute correctly and the command prompt will reappear afterwards, as expected. If I do not type a command and press enter, the input handler breaks.

3. Pressing Backspace followed by Enter at the prompt breaks the input loop similar to what I mentioned in (2).

Again, maybe I need to see the rtslib code more to understand how to not let the stdout chars not modifiable or unchanged.

[iammattcoleman]

@pkalever I've been thinking about a significant rewrite to enforce separation of concerns a bit better by removing the UI handling code entirely from the daemon and making it just an RPC server. It could use JSON-RPC, Protocol Buffers, Thrift, etc.

Thrift would make it very easy to switch between daemonized mode and direct execution by targetcli. However, it's got a pretty steep initial learning curve and a lot of people dislike generated code. (Although it does a pretty good job of making it so you can ignore the contents of the generated code and just use it.)

JSON-RPC would probably be my preference, but I haven't actually worked with it in Python previously.

I find that Protocol Buffers (without going to full gRPC, which doesn't make sense at all for this) tends to be just as verbose as Thrift but with a much steeper barrier to entry because you have to implement so much boilerplate yourself.

Are there any other options would recommend? What do you think about the idea, overall?

[pkalever]

@iammattcoleman Frankly, I initially wanted to introduce the socket based communication with some sort of rpc, but I didn't actually found a need for it. And I also didn't want to make it complicated, just for string query and response kind of usecase. I will consider rethinking on this and share my thoughts later this week.

---

@maurizio-lombardi @iammattcoleman fixed the issue 2 and 3 mentioned in the above comments by Matt.

For issue 1:
I need to run pwd command via shell.run_command() for every single request.
i.e It's easy to fix, but I was thinking is it worth it ? Do we really need it?

I was thinking just instead of printing
/>
/> ls
/>

May be lets just print

>
> ls
>

Thoughts?

[maurizio-lombardi]

May be lets just print

ls

Thoughts?

I think it makes sense

[gonzoleeman]

I will check it out today.

…

-- Lee-Man Duncan Sent from my iPhone, dude

On Apr 6, 2020, at 12:49 AM, Prasanna Kumar Kalever ***@***.***> wrote: @iammattcoleman @gonzoleeman can you guys please share your first impression on this? Thanks! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[pkalever]

@maurizio-lombardi @gonzoleeman @iammattcoleman @lxbsz with the patch2, now the interactive mode can print the prompt path.

Please take a look.

Thanks!

[iammattcoleman]

Your recent changes fixed the issues I had found.

I found another example of why the daemon should not be trying to format the terminal output: it is unaware of the terminal's width:

root@mdcSIRIS:/usr/bin# targetcli set global auto_use_daemon=False
Parameter auto_use_daemon is now 'false'.
root@mdcSIRIS:/usr/bin# targetcli ls
o- / ....................................................................... [...]
  o- backstores ............................................................ [...]
  | o- block ................................................ [Storage Objects: 0]
  | o- fileio ............................................... [Storage Objects: 0]
  | o- pscsi ................................................ [Storage Objects: 0]
  | o- ramdisk .............................................. [Storage Objects: 0]
  o- iscsi .......................................................... [Targets: 0]
  o- loopback ....................................................... [Targets: 0]
  o- vhost .......................................................... [Targets: 0]
# targetcli set global auto_use_daemon=True
Parameter auto_use_daemon is now 'true'.
# targetcli ls
o- / .............................................................................
............................................ [...]
  o- backstores ..................................................................
............................................ [...]
  | o- block .....................................................................
............................. [Storage Objects: 0]
  | o- fileio ....................................................................
............................. [Storage Objects: 0]
  | o- pscsi .....................................................................
............................. [Storage Objects: 0]
  | o- ramdisk ...................................................................
............................. [Storage Objects: 0]
  o- iscsi .......................................................................
..................................... [Targets: 0]
  o- loopback ....................................................................
..................................... [Targets: 0]
  o- vhost .......................................................................
..................................... [Targets: 0]

I really don't think that should be fixed in the daemon: the code is getting increasingly complicated because of the lack of separation of concerns. To resolve this would require adding code that lets targetcli tell targetclid what its terminal's width is. The daemon should never have to think about terminal widths.

[pkalever]

@iammattcoleman I agree. That we need to work on the UI positioning. But that's an another story for another PR!

These changes won't be happening in daemon, because daemon will be running via systemd and it has no idea about the width of its client (targetcli).

Thanks Matt for the retesting this sequence.

[gonzoleeman]

I have tested the branch. Thank you for your changes!

My take on this is that it is better than before. I just did a quick sanity test on my "standard" configuration, which is one backstore file being used by one LUN, and it looked good. But interactive mode is still different when he daemon is present: there is no "color" enhancement.

Last time I mentioned this problem others seemed to think it was not a big deal, but the color enhancement is there for a reason: it helps. It seems like, the way things are working now, the reason for this is that the daemon should not have to care about enhancing its output with color hints, but I disagree with that, at least partly.

I'll back up and explain what I thought we were getting and what I would prefer. I'd like to go to an architecture where we have the daemon running all the time, and there's a thin CLI client. The work should be divided IMHO in such a way that the daemon does the heavy lifting, like setup, event detection, sysfs interaction, etc, and the CLI handle the "display" and "input" parts. In such a world the "batch" API could be just another client or use the CLI in some special mode. I'm not sure where "coloring hints" (like good connections are green, bad connections are red) would fit in such an architecture, but I suspect it'd have to be shared between the backend and frontend, since the backend knows the status and state of things, and the front end knows how to display "things".

Bottom line: given that we probably will not get color, for now, from the daemon, this seems better than forcing batch mode on systems where the daemon is running, to me.

[pkalever]

@gonzoleeman Thanks for testing this series.

My understanding is that for adding coloring support we need to bring a part of configshell and rtslib code to targetclid.

As I always mention, targetclid is young and its at the very beginning, we can do much more with it. I will keep the coloring feature request in mind, and try to come up with the optimal solution for achieving it in the near feature.

Thanks!

[iammattcoleman]

@pkalever I just tested your most recent commit. Looks good! I totally agree that my other concerns (terminal width and coloring) should be handled in separate pull requests. I'll create an issue to discuss it further.

#169