Bug 785773

Summary: CLI exporter variable not respecting settings
Product: [Other] RHQ Project Reporter: Jay Shaughnessy <jshaughn>
Component: CLIAssignee: Jay Shaughnessy <jshaughn>
Status: CLOSED CURRENTRELEASE QA Contact: Mike Foley <mfoley>
Severity: medium Docs Contact:
Priority: high    
Version: 4.2CC: hrupp, lkrejci, mfoley
Target Milestone: ---Keywords: NeedsTestCase
Target Release: JON 3.0.1   
Hardware: All   
OS: All   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2013-09-03 11:08:20 EDT Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Bug Depends On:    
Bug Blocks: 782579    

Description Jay Shaughnessy 2012-01-30 10:31:38 EST
According to the help, documentation and examples I've seen, the built-in
CLI 'exporter' variable should work like a singleton.  But, it seems to
be reset after each call.  For example:

> exporter.setTarget('raw', 'someFile.txt')
> exporter
file: 

It seems that on each command the 'exporter' variable is rebound to a new
instance.

This workaround works:

> var e = exporter
> e.setTarget( 'raw', 'someFile.txt' )
> e
file: someFile.txt


So, either help and docs need to change, or the built-in variable handling
needs to change.
Comment 1 Mike Foley 2012-01-30 11:30:30 EST
12/30/2012 BZ triage meeting mfoley, ccrouch, loleary, asantos
Comment 2 Jay Shaughnessy 2012-01-31 17:14:23 EST
After looking at this for a bit I'm not sure if this should be a code
change or a doc change.

This behavior is not new. At least a year old, I think.

The 'exporter' variable is provided by StandardBindings.  The issue is
that each command line execution is basically a new 'exec' command (passed
the command line text as args).  And each exec command execution gets new StandardBindings.  It's this last bit that needs to change, I think, if
we're going to address this in code.

I'm not sure why we don't keep the same bindings for the same Client (the
same client meaning what? the same Subject Object?)

I'm not sure if this would have detrimental effects in some other way, like
maybe paging.  On the other hand, I'm not sure there aren't issues with
resetting other bindings every time as well.

A possible change may look like this for the ScriptCommand:

     public void initBindings(ClientMain client) {
+        if (null != bindings) {
+            Subject bindingsSubject = bindings.getSubject().getValue();
+            Subject clientSubject = client.getSubject();
+            if (null != clientSubject &&
+                clientSubject.equals(bindingsSubject)) {   
+                return;
+            }
+        }
...

I'm not sure. Asking for review and feedback from Lukas...
Comment 3 Lukas Krejci 2012-02-01 04:54:32 EST
Ok, I think this is a regression from RHQ3.

In RHQ3, *some* of the variables were reset on each command line execution, while the refactoring in RHQ4 made the command line execution reset ALL predefined variables.

The reason for the bindings to be reinitialized in each call is that the script command is oblivious of any state changes made by the commands it executes - the user might have logged out, the session timed out, etc. By reiniting the variables, we make sure that the bindings are "fresh" and correct for each command line (or at least that's why the RHQ3 code did this).

But not all variables are sensitive to the user's session state, in fact most of them aren't. During the refactoring I tried to mimic the original behavior and not change the way it worked, but I must say I don't like this reinitialization. The CLI session should be just a simple single session, it is very unintuitive that some of your variables get reset everytime you run a command.

So this problem is actually bigger than the "exporter not respecting settings". This is a problem with ensuring that the internal state of the various bound objects is consistent with the current state of the user session (by session i essentially mean the connection state to the RHQ server).
Comment 4 Lukas Krejci 2012-02-01 05:20:28 EST
Note that ideally, I'd like us to do another round of refactoring that would move the managing of the state into a standalone object - let's say RhqScriptContext (extending the javax.script.ScriptContext) that would encapsulate the RhqFacade and the ScriptContext with all the standard variables and user defined variables which would make sure that if someone calls logout() on the RhqFacade, the bound variables would react to that appropriately.

Currently this is being done "manually" in the ScriptCommand code (badly as this BZ proves). The server-side CLI executions are managed using the "LocalClient" facade to the remote interfaces that outright disables the login/logout functionality so this is not a problem on the server side, because we circumvent it there.

A generic solution like the RhqScriptContext could, on the other hand, be used both remotely and on the server to further unify the behavior and simplify the interaction with the RHQ scripting environment. 

Having a fully managed ScriptContext would enable us, for example, to have a pool of ScriptEngines available globally inside the server, where various plugins and other possible serverside functionality could borrow the script engines from the pool for the time of a single script execution passing in an RhqScriptContext managed by the respective lifecycles of the plugins. Again, currently this is half baked and every subsystem using RHQ scripting is assumed to instantiate its own script engines, which is wasting resources unnecessarily.
Comment 5 Charles Crouch 2012-02-01 13:03:26 EST
*** Bug 784690 has been marked as a duplicate of this bug. ***
Comment 6 Jay Shaughnessy 2012-02-01 14:37:19 EST
After a long discussion on this with Lukas we decided:

1) A fix for this issue should be done immediately and another BZ
   should be created for the RFE he describes above in comment 4.

2) To provide the intended behavior for interactive command line
   sessions these things need to be true:

   * each command line execution can not reset standard bindings
   * login/logout should affect only bindings related to the client change
   * script files invoked via 'exec -f <filename>' should be executes as
     extensions of the command line session.

And of course existing scripts should continue to work.
Comment 7 Jay Shaughnessy 2012-02-01 14:50:12 EST
master commit d003658a94b0f88852f469e9f473eee418377849

This was rooted in the handling of the standard bindings we inject
into the script engine in an interactive CLI session.  See the BZ for
details but in short this commit:
- stops treating every command line entry as a new script with new bindings
- ensures that non-standard bindings (like var x = 10) and non-client-dependent
  standard bindings (like exporter) are maintained between command line
  entries as well as through login/logout.
- treat 'exec -f' commands like extendsions of the command line session.
  They inherit the current bindings and any modifications made by the script
  file will affect the command line session after the script file has completed.
- ensure that login/logout update client-related bindings as needed, and
  no others.
- don't leave behind manager bindings when the client changes (like on logout)
- protect against NPE for consecutive logout commands
- trivial: removed unused import from CliSender


Lukas, please review/test this commit.  Also, you may want to create that
RFE BZ. Thanks -Jay
Comment 8 Charles Crouch 2012-02-01 16:15:17 EST
Jay, Lukas: Can we make sure that we've tested this change with at least the 
RHQ Sample scripts, e.g. https://github.com/rhq-project/samples
Comment 9 Lukas Krejci 2012-02-02 07:57:50 EST
FYI, the RFE is here: bug 786807.
Comment 10 Jay Shaughnessy 2012-02-02 10:35:48 EST
I've tested with a couple of the sample scripts. All looks good to me. 
If Lukas gives the green light I think we can move to ON_QA and make 
available for backporting.
Comment 11 Lukas Krejci 2012-02-02 13:27:56 EST
The code looks fine and I couldn't find anything not working during the smoke tests..

Settings this to ON_QA.
Comment 12 Mike Foley 2012-02-02 13:43:24 EST
Testcase #1 from original description

[mfoley@foleymonsterbox1 bin]$ ./rhq-cli.sh -u rhqadmin -p rhqadmin
RHQ Enterprise Remote CLI 4.2.0.JON.3.0.1.GA
Remote server version is: 3.0.1.GA (b2cb23b:859b914)
Login successful
rhqadmin@localhost:7080$ exporter.setTarget('raw','someFile.txt')

rhqadmin@localhost:7080$ exporter
Exporter:
	     file: 
	   format: raw
	pageWidth: 160

rhqadmin@localhost:7080$ 



Testcase #2 from original description

Remote server version is: 3.0.1.GA (b2cb23b:859b914)
Login successful
rhqadmin@localhost:7080$ exporter.setTarget('raw','someFile.txt')

rhqadmin@localhost:7080$ exporter
Exporter:
	     file: 
	   format: raw
	pageWidth: 160

rhqadmin@localhost:7080$ var e = exporter

rhqadmin@localhost:7080$ e.

close       file        format      pageWidth   setTarget   write
rhqadmin@localhost:7080$ e.set

setFile        setFormat      setPageWidth   setTarget
rhqadmin@localhost:7080$ e.setTarget('raw', 'someFile.txt')

rhqadmin@localhost:7080$ e
Exporter:
	     file: someFile.txt
	   format: raw
	pageWidth: 160

rhqadmin@localhost:7080$
Comment 13 Lukas Krejci 2012-02-03 07:42:24 EST
Additional things to test:

1) start cli without logging in
2) setup exporter
3) log in (using "login username password" or "rhq.login('username', 'password');") 
4)check that exporter is still setup
5)logout (using "logout" or "rhq.logout();")
6) check that exporter is still setup
Comment 14 Charles Crouch 2012-02-06 23:53:34 EST
Setting this back to ON_QA for verification of additional steps Lukas added
Comment 15 Mike Foley 2012-02-07 10:27:51 EST
verified steps in comment #13
Comment 16 Heiko W. Rupp 2013-09-03 11:08:20 EDT
Bulk closing of old issues in VERIFIED state.