Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

verdi computer option to switch off login shell #4271

Merged
merged 3 commits into from
Jul 23, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/config/localhost-config.yaml
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
---
use_login_shell: true
safe_interval: 0
12 changes: 5 additions & 7 deletions aiida/transports/plugins/local.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@ class LocalTransport(Transport):
``unset PYTHONPATH`` if you plan on running calculations that use Python.
"""

# There are no valid parameters for the local transport
_valid_auth_options = []

# There is no real limit on how fast you can safely connect to a localhost, unlike often the case with SSH transport
Expand Down Expand Up @@ -744,7 +743,9 @@ def _exec_command_internal(self, command, **kwargs): # pylint: disable=unused-a

# Note: The outer shell will eat one level of escaping, while
# 'bash -l -c ...' will eat another. Thus, we need to escape again.
command = 'bash -l -c ' + escape_for_bash(command)
bash_commmand = self._bash_command_str + '-c '

command = bash_commmand + escape_for_bash(command)

proc = subprocess.Popen(
command,
Expand Down Expand Up @@ -803,11 +804,8 @@ def gotocomputer_command(self, remotedir):

:param str remotedir: the full path of the remote directory
"""
script = ' ; '.join([
'if [ -d {escaped_remotedir} ]', 'then cd {escaped_remotedir}', 'bash', "else echo ' ** The directory'",
"echo ' ** {remotedir}'", "echo ' ** seems to have been deleted, I logout...'", 'fi'
]).format(escaped_remotedir="'{}'".format(remotedir), remotedir=remotedir)
cmd = 'bash -c "{}"'.format(script)
connect_string = self._gotocomputer_string(remotedir)
cmd = 'bash -c {}'.format(connect_string)
return cmd

def rename(self, oldpath, newpath):
Expand Down
25 changes: 10 additions & 15 deletions aiida/transports/plugins/ssh.py
Original file line number Diff line number Diff line change
Expand Up @@ -1232,7 +1232,9 @@ def _exec_command_internal(self, command, combine_stderr=False, bufsize=-1): #

# Note: The default shell will eat one level of escaping, while
# 'bash -l -c ...' will eat another. Thus, we need to escape again.
channel.exec_command('bash -l -c ' + escape_for_bash(command_to_execute))
bash_commmand = self._bash_command_str + '-c '

channel.exec_command(bash_commmand + escape_for_bash(command_to_execute))

stdin = channel.makefile('wb', bufsize)
stdout = channel.makefile('rb', bufsize)
Expand Down Expand Up @@ -1298,21 +1300,14 @@ def gotocomputer_command(self, remotedir):
further_params.append('-i {}'.format(escape_for_bash(self._connect_args['key_filename'])))

further_params_str = ' '.join(further_params)
# I use triple strings because I both have single and double quotes, but I still want everything in
# a single line
connect_string = (
"""ssh -t {machine} {further_params} "if [ -d {escaped_remotedir} ] ;"""
""" then cd {escaped_remotedir} ; bash -l ; else echo ' ** The directory' ; """
"""echo ' ** {remotedir}' ; echo ' ** seems to have been deleted, I logout...' ; fi" """.format(
further_params=further_params_str,
machine=self._machine,
escaped_remotedir="'{}'".format(remotedir),
remotedir=remotedir
)
)

# print connect_string
return connect_string
connect_string = self._gotocomputer_string(remotedir)
cmd = 'ssh -t {machine} {further_params} {connect_string}'.format(
further_params=further_params_str,
machine=self._machine,
connect_string=connect_string,
)
return cmd

def symlink(self, remotesource, remotedestination):
"""
Expand Down
64 changes: 56 additions & 8 deletions aiida/transports/transport.py
Original file line number Diff line number Diff line change
Expand Up @@ -47,21 +47,50 @@ class Transport(abc.ABC):
_valid_auth_params = None
_MAGIC_CHECK = re.compile('[*?[]')
_valid_auth_options = []
_common_auth_options = [(
'safe_interval', {
'type': float,
'prompt': 'Connection cooldown time (s)',
'help': 'Minimum time interval in seconds between opening new connections.',
'callback': validate_positive_number
}
)]
_common_auth_options = [
(
'use_login_shell', {
'default':
True,
'switch':
True,
'prompt':
'Use login shell when executing command',
'help':
' Not using a login shell can help suppress potential'
' spurious text output that can prevent AiiDA from parsing the output of commands,'
' but may result in some startup files (.profile) not being sourced.',
'non_interactive_default':
True
}
),
(
'safe_interval', {
'type': float,
'prompt': 'Connection cooldown time (s)',
'help': 'Minimum time interval in seconds between opening new connections.',
'callback': validate_positive_number
}
),
]

def __init__(self, *args, **kwargs): # pylint: disable=unused-argument
"""
__init__ method of the Transport base class.

:param safe_interval: (optional, default self._DEFAULT_SAFE_OPEN_INTERVAL)
Minimum time interval in seconds between opening new connections.
:param use_login_shell: (optional, default True)
if False, do not use a login shell when executing command
"""
from aiida.common import AIIDA_LOGGER
self._safe_open_interval = kwargs.pop('safe_interval', self._DEFAULT_SAFE_OPEN_INTERVAL)
self._use_login_shell = kwargs.pop('use_login_shell', True)
if self._use_login_shell:
self._bash_command_str = 'bash -l '
else:
self._bash_command_str = 'bash '

self._logger = AIIDA_LOGGER.getChild('transport').getChild(self.__class__.__name__)
self._logger_extra = None
self._is_open = False
Expand Down Expand Up @@ -193,6 +222,13 @@ def _get_safe_interval_suggestion_string(cls, computer): # pylint: disable=unus
"""
return cls._DEFAULT_SAFE_OPEN_INTERVAL

@classmethod
def _get_use_login_shell_suggestion_string(cls, computer): # pylint: disable=unused-argument
"""
Return a suggestion for the specific field.
"""
return 'True'

@property
def logger(self):
"""
Expand Down Expand Up @@ -756,6 +792,18 @@ def glob0(self, dirname, basename):
def has_magic(self, string):
return self._MAGIC_CHECK.search(string) is not None

def _gotocomputer_string(self, remotedir):
"""command executed when goto computer."""
connect_string = (
""" "if [ -d {escaped_remotedir} ] ;"""
""" then cd {escaped_remotedir} ; {bash_command} ; else echo ' ** The directory' ; """
"""echo ' ** {remotedir}' ; echo ' ** seems to have been deleted, I logout...' ; fi" """.format(
bash_command=self._bash_command_str, escaped_remotedir="'{}'".format(remotedir), remotedir=remotedir
)
)

return connect_string


class TransportInternalError(InternalError):
"""
Expand Down
7 changes: 7 additions & 0 deletions docs/source/intro/troubleshooting.rst
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,13 @@ To test if a the computer does not produce spurious output, run (after configuri

which checks and, in case of problems, suggests how to solve the problem.

.. note::

If the methods explained above do not work, you can configure AiiDA to not use a login shell when connecting to your computer, which may prevent the spurious output from being printed:
During ``verdi computer configure``, set ``-no-use-login-shell`` or when asked to use a login shell, set it to ``False``.
Note, however, that this may result in a slightly different environment, since `certain startup files are only sourced for login shells <https://unix.stackexchange.com/a/46856/155909>`_.


.. _StackExchange thread: https://apple.stackexchange.com/questions/51036/what-is-the-difference-between-bash-profile-and-bashrc


Expand Down
10 changes: 9 additions & 1 deletion tests/cmdline/commands/test_computer.py
Original file line number Diff line number Diff line change
Expand Up @@ -370,9 +370,16 @@ def test_local_interactive(self):
comp = self.comp_builder.new()
comp.store()

result = self.cli_runner.invoke(computer_configure, ['local', comp.label], input='\n', catch_exceptions=False)
command_input = ('{use_login_shell}\n{safe_interval}\n').format(use_login_shell='False', safe_interval='1.0')
result = self.cli_runner.invoke(
computer_configure, ['local', comp.label], input=command_input, catch_exceptions=False
)
self.assertTrue(comp.is_user_configured(self.user), msg=result.output)

new_auth_params = comp.get_authinfo(self.user).get_auth_params()
self.assertEqual(new_auth_params['use_login_shell'], False)
self.assertEqual(new_auth_params['safe_interval'], 1.0)

def test_ssh_interactive(self):
"""
Check that the interactive prompt is accepting the correct values.
Expand Down Expand Up @@ -411,6 +418,7 @@ def test_ssh_interactive(self):
self.assertEqual(new_auth_params['port'], port)
self.assertEqual(new_auth_params['look_for_keys'], look_for_keys)
self.assertEqual(new_auth_params['key_filename'], key_filename)
self.assertEqual(new_auth_params['use_login_shell'], True)

def test_local_from_config(self):
"""Test configuring a computer from a config file"""
Expand Down
13 changes: 13 additions & 0 deletions tests/transports/test_local.py
Original file line number Diff line number Diff line change
Expand Up @@ -48,3 +48,16 @@ def test_basic():
"""Test constructor."""
with LocalTransport():
pass


def test_gotocomputer():
"""Test gotocomputer"""
with LocalTransport() as transport:
cmd_str = transport.gotocomputer_command('/remote_dir/')

expected_str = (
"""bash -c "if [ -d '/remote_dir/' ] ;"""
""" then cd '/remote_dir/' ; bash -l ; else echo ' ** The directory' ; """
"""echo ' ** /remote_dir/' ; echo ' ** seems to have been deleted, I logout...' ; fi" """
)
assert cmd_str == expected_str
13 changes: 13 additions & 0 deletions tests/transports/test_ssh.py
Original file line number Diff line number Diff line change
Expand Up @@ -55,3 +55,16 @@ def test_no_host_key(self):

# Reset logging level
logging.disable(logging.NOTSET)


def test_gotocomputer():
"""Test gotocomputer"""
with SshTransport(machine='localhost', timeout=30, use_login_shell=False, key_policy='AutoAddPolicy') as transport:
cmd_str = transport.gotocomputer_command('/remote_dir/')

expected_str = (
"""ssh -t localhost "if [ -d '/remote_dir/' ] ;"""
""" then cd '/remote_dir/' ; bash ; else echo ' ** The directory' ; """
"""echo ' ** /remote_dir/' ; echo ' ** seems to have been deleted, I logout...' ; fi" """
)
assert cmd_str == expected_str