`verdi computer` option to switch off login shell #4271

unkcpz · 2020-07-16T09:46:10Z

Issue closing #2978 happened to me when connecting to TianHe-2 . The messages output to the login shell was set in file /etc/profile. I make a rough fix and let it set in verdi computer configuration ssh .

EDIT:
local transport plugin should also configurable with on/off login shell. As well as gotocomputer_command of all transport plugins. To avoid repeating define bash_command and repeating gotocomputer command string, encapsulating them in Tranport class.

codecov · 2020-07-16T10:03:00Z

Codecov Report

Merging #4271 into develop will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop    #4271      +/-   ##
===========================================
+ Coverage    79.19%   79.24%   +0.05%     
===========================================
  Files          468      468              
  Lines        34476    34488      +12     
===========================================
+ Hits         27301    27325      +24     
+ Misses        7175     7163      -12

Flag	Coverage Δ
#django	`71.16% <100.00%> (+0.04%)`	⬆️
#sqlalchemy	`71.99% <100.00%> (+0.05%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
aiida/transports/plugins/local.py	`81.29% <100.00%> (+0.82%)`	⬆️
aiida/transports/plugins/ssh.py	`77.76% <100.00%> (+1.38%)`	⬆️
aiida/transports/transport.py	`65.30% <100.00%> (+1.49%)`	⬆️
aiida/engine/daemon/client.py	`73.57% <0.00%> (+1.15%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ef1caa0...d30fa6f. Read the comment docs.

ltalirz

Thanks for this PR, @unkcpz !

I was originally responsible for switching to using the login shell, the reason is explained in #1502, but if there are systems that force outputs on login shells that a user cannot suppress, then I guess it makes sense to be able to not use a login shell.
I think making this configurable is potentially quite useful.

Some minor change requests.

@giovannipizzi Do you agree with making this configurable?
It does mean another "variable" one has to keep in mind when debugging things...

aiida/transports/plugins/ssh.py

unkcpz

Thanks @ltalirz

aiida/transports/plugins/ssh.py

ltalirz

thanks @unkcpz , for me this is good to go.

would be good to have a brief feedback from @giovannipizzi or @sphuber as well.

sphuber

Thanks @unkcpz . I have gone through the issue and the original commit that added the -l flag by default, and based on the information provided there this seems reasonable, especially since the default remains untouched. But I have to say that I don't have an enormous amount of experience with the difference in interactive and non-interactive shells. I have just two minor suggestions for help strings, but won't force an update. Would be good if @giovannipizzi can maybe give a final sign off

sphuber · 2020-07-17T06:09:41Z

aiida/transports/plugins/ssh.py

+                'default': True,
+                'switch': True,
+                'prompt': 'Use login shell when executing command',
+                'help': ' Avoiding a login shell can help suppress spurious text output'


Suggested change

'help': ' Avoiding a login shell can help suppress spurious text output'

'help': 'Not using a login shell can help suppress any potential spurious text output that can interfere with AiiDA executing commands, '

I guess it might not be immediately clear why suppressing spurious output is something desirable, per se.

aiida/transports/plugins/ssh.py

sphuber · 2020-07-17T06:19:38Z

I just realized that this can also apply to local transports, although more unlikely probably. However, for completeness, should we also add this switch there?

And finally, if I understand correctly, this would then solve #2978 ? Actually, that made me realize that probably we will want to add a small note about this new switch in this part of the documentation troubleshooting.
I would propose to add something like the following at the end of that section:

.. note::

    If, for whatever reason, the methods explained above do not work, you can also opt to change the computer configuration to not use a login shell.
    During `verdi computer configure`, set `-no-use-login-shell` or when asked to use a login shell, set it to `False`.
    This will tell AiiDA to not use a login shell when connecting, which may potentially prevent the spurious output from being printed.
    Note, however, that this may also prevent other code from being executed, such as certain environment variables being set.

ltalirz · 2020-07-17T06:52:10Z

It's true that this does make a difference for local transports as well.
A practical use case might be if someone wants to install AiiDA directly on the login node of the "communicative" cluster.

unkcpz · 2020-07-17T07:10:46Z

A practical use case might be if someone wants to install AiiDA directly on the login node of the "communicative" cluster.

emm.. that's true. So, put this option to setup?

giovannipizzi · 2020-07-17T07:11:16Z

Thanks @unkcpz !
Indeed, I think it's ok to add this as an option keeping the old default - it might create other problems, but being optional it's ok. Also yes, I think it fixes #2978?

A few final points:

Is this automatically configurable now with verdi computer configure ssh? Can we have a test maybe?
Indeed, it would be good to have the same logic for local transports

giovannipizzi · 2020-07-17T07:13:48Z

I think configure is still the right place instead of setup, but done for both backends. I had tried an attempt to simplify how the logic is managed in #3663 but it turned out to be more complex than I thought and I didn't have the time to finish that one. Maybe for now we just follow the same thing that is done for the cool down time that is defined for both plugins.

unkcpz · 2020-07-17T07:22:46Z

ok, I'll also add it to local transports.

Is this automatically configurable now with verdi computer configure ssh? Can we have a test maybe?

I add a simple test to check that autoinfo getting set with this option. @giovannipizzi
https://github.com/aiidateam/aiida-core/blob/91d1e845878816acc8c2e1e77d94bf8bae5d706a/tests/cmdline/commands/test_computer.py#L414

unkcpz · 2020-07-19T02:35:43Z

The use_login_shell option is now added to local transports.

To avoid repeating this setting code in two files, I add it to abstract Transport class will take effect for all transports plugins.
Also add test to make sure computer's auto info is set in test_local_interactive.
DOCS: note is added in troubleshooting part as @sphuber suggested.

unkcpz · 2020-07-19T07:43:45Z

@ltalirz I find one more place where login shell is used, here at gotocomputer_command.

aiida-core/aiida/transports/plugins/ssh.py

Line 1305 in 0d7baa5

    
           """ then cd {escaped_remotedir} ; bash -l ; else echo '  ** The directory' ; """

why force user to bash login shell when they goto the specific computer, is that necessary? If there are spurious output being printed the login shell here will also cause problem.

ltalirz · 2020-07-19T09:52:18Z

why force user to bash login shell when they goto the specific computer, is that necessary?

I think it's the expected behavior - a user will expect the same environment variables as when connecting manually and changing to that directory, no?

If there are spurious output being printed the login shell here will also cause problem.

If it causes problems, then we may need to make it configurable here as well, but I'm not aware that AiiDA is processing the output of this command.
Can you check whether it indeed causes a problem (and why)?

aiida/transports/transport.py

docs/source/intro/troubleshooting.rst

unkcpz · 2020-07-19T10:31:17Z

Ah, the problem comes from the bashrc config of remote cluster, it somehow execute cd to the home directory after login. So the gotocomputer command should keep it the way it is.

For transports plugins(now we have ssh and local), add `use_login_shell` option to allowed user to not use login shell when executing the command.

unkcpz · 2020-07-19T10:46:07Z

@ltalirz thanks! looks better now.

ltalirz · 2020-07-19T12:04:07Z

Ah, the problem comes from the bashrc config of remote cluster, it somehow execute cd to the home directory after login. So the gotocomputer command should keep it the way it is.

Just so that I understand: some system-level bashrc file is being sourced only for login shells, and this breaks the gotocomputer command? Strange...

In any case, reading back through what I wrote, I think you were right after all.
It may be the "expected behavior" by default, but for people who explicitly configured AiiDA not to use a login shell, it is probably rather unexpected that AiiDA would use continue using a login shell for gotocomputer.

Given the issue you encountered, I think it's best apply the user configuration in gotocomputer as well..
Since this switching logic will now be present in three places, I suggest you encapsulate it, e.g. in a _bash_command property of the Transport.

unkcpz · 2020-07-20T01:31:04Z

I am not very familiar with login/non-login and interactive/non-interactive shell stuff, correct me if I understand it wrong.

Just so that I understand: some system-level bashrc file is being sourced only for login shells, and this breaks the gotocomputer command? Strange...

My fault for not clarifying this, not the system-level bashrc but the user's ~/.bashrc which did the cd action in the remote cluster. Using gotocomputer will always activate the ~/.bashrc no matter login shell is on or off, correct? So the problem is definitely not from AiiDA side, and can be solved by modifying the ~/.bashrc file which user always have permission to edit.

It may be the "expected behavior" by default, but for people who explicitly configured AiiDA not to use a login shell, it is probably rather unexpected that AiiDA would use continue using a login shell for gotocomputer.

Agree! If the cd action is setting in the system wide profile which users have no permission to edit, the weird cd action should be suppressed by turn off the login shell. That is when the login shell is set to false, we also expect gotocomputer use non-login shell.

`bash_command` are used in three places and set by `use_login_shell`. Encapsulate it as attribute `_bash_command_str` of `Transport`. gotocompyter_command have same string for local and ssh transport plugin, Encapsulate the string as `_gotocomputer_string` in `Transport` class.

ltalirz

@sphuber I'm fine with this.

One could make the _bash_command_str a @property instead of setting it in the constructor, so that it will react to changes in _use_login_shell but I don't think it's mandatory.

Thanks @unkcpz !

sphuber

Thanks a lot @unkcpz

unkcpz requested a review from ltalirz July 16, 2020 09:46

ltalirz suggested changes Jul 16, 2020

View reviewed changes

aiida/transports/plugins/ssh.py Outdated Show resolved Hide resolved

aiida/transports/plugins/ssh.py Outdated Show resolved Hide resolved

ltalirz reviewed Jul 16, 2020

View reviewed changes

aiida/transports/plugins/ssh.py Outdated Show resolved Hide resolved

unkcpz force-pushed the fix/issue/2978 branch from 1b388cd to 91d1e84 Compare July 17, 2020 02:53

unkcpz commented Jul 17, 2020

View reviewed changes

aiida/transports/plugins/ssh.py Outdated Show resolved Hide resolved

ltalirz previously approved these changes Jul 17, 2020

View reviewed changes

sphuber reviewed Jul 17, 2020

View reviewed changes

unkcpz dismissed ltalirz’s stale review via dff8d33 July 19, 2020 02:17

unkcpz force-pushed the fix/issue/2978 branch 3 times, most recently from 9d7a4fb to a109608 Compare July 19, 2020 02:32

ltalirz reviewed Jul 19, 2020

View reviewed changes

aiida/transports/transport.py Outdated Show resolved Hide resolved

ltalirz reviewed Jul 19, 2020

View reviewed changes

aiida/transports/transport.py Outdated Show resolved Hide resolved

ltalirz reviewed Jul 19, 2020

View reviewed changes

docs/source/intro/troubleshooting.rst Outdated Show resolved Hide resolved

Add setting option to switch off login shell

d88fc5e

For transports plugins(now we have ssh and local), add `use_login_shell` option to allowed user to not use login shell when executing the command.

unkcpz force-pushed the fix/issue/2978 branch from a109608 to d88fc5e Compare July 19, 2020 10:44

unkcpz added priority/quality-of-life would simplify development topic/computers topic/transports topic/verdi labels Jul 20, 2020

unkcpz force-pushed the fix/issue/2978 branch from a1c58fd to dd5afaa Compare July 20, 2020 06:36

ltalirz approved these changes Jul 22, 2020

View reviewed changes

Merge branch 'develop' into fix/issue/2978

d30fa6f

sphuber self-requested a review July 23, 2020 08:24

sphuber approved these changes Jul 23, 2020

View reviewed changes

sphuber merged commit 3a4eff7 into aiidateam:develop Jul 23, 2020

sphuber mentioned this pull request Jul 23, 2020

Exhaustive printing by remote frontend #2978

Closed

unkcpz deleted the fix/issue/2978 branch July 23, 2020 08:27

sphuber mentioned this pull request Feb 16, 2023

Should the use_login_shell property of the AuthInfo also be considered in the submission script shebang #5895

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`verdi computer` option to switch off login shell #4271

`verdi computer` option to switch off login shell #4271

unkcpz commented Jul 16, 2020 •

edited

Loading

codecov bot commented Jul 16, 2020 •

edited

Loading

ltalirz left a comment

unkcpz left a comment

ltalirz left a comment

sphuber left a comment

sphuber Jul 17, 2020

sphuber commented Jul 17, 2020

ltalirz commented Jul 17, 2020

unkcpz commented Jul 17, 2020

giovannipizzi commented Jul 17, 2020

giovannipizzi commented Jul 17, 2020

unkcpz commented Jul 17, 2020

unkcpz commented Jul 19, 2020

unkcpz commented Jul 19, 2020 •

edited

Loading

ltalirz commented Jul 19, 2020

unkcpz commented Jul 19, 2020

unkcpz commented Jul 19, 2020

ltalirz commented Jul 19, 2020 •

edited

Loading

unkcpz commented Jul 20, 2020 •

edited

Loading

ltalirz left a comment

sphuber left a comment

	'help': ' Avoiding a login shell can help suppress spurious text output'
	'help': 'Not using a login shell can help suppress any potential spurious text output that can interfere with AiiDA executing commands, '

verdi computer option to switch off login shell #4271

verdi computer option to switch off login shell #4271

Conversation

unkcpz commented Jul 16, 2020 • edited Loading

codecov bot commented Jul 16, 2020 • edited Loading

Codecov Report

ltalirz left a comment

Choose a reason for hiding this comment

unkcpz left a comment

Choose a reason for hiding this comment

ltalirz left a comment

Choose a reason for hiding this comment

sphuber left a comment

Choose a reason for hiding this comment

sphuber Jul 17, 2020

Choose a reason for hiding this comment

sphuber commented Jul 17, 2020

ltalirz commented Jul 17, 2020

unkcpz commented Jul 17, 2020

giovannipizzi commented Jul 17, 2020

giovannipizzi commented Jul 17, 2020

unkcpz commented Jul 17, 2020

unkcpz commented Jul 19, 2020

unkcpz commented Jul 19, 2020 • edited Loading

ltalirz commented Jul 19, 2020

unkcpz commented Jul 19, 2020

unkcpz commented Jul 19, 2020

ltalirz commented Jul 19, 2020 • edited Loading

unkcpz commented Jul 20, 2020 • edited Loading

ltalirz left a comment

Choose a reason for hiding this comment

sphuber left a comment

Choose a reason for hiding this comment

`verdi computer` option to switch off login shell #4271

`verdi computer` option to switch off login shell #4271

unkcpz commented Jul 16, 2020 •

edited

Loading

codecov bot commented Jul 16, 2020 •

edited

Loading

unkcpz commented Jul 19, 2020 •

edited

Loading

ltalirz commented Jul 19, 2020 •

edited

Loading

unkcpz commented Jul 20, 2020 •

edited

Loading