Skip to content

Commit

Permalink
presto-admin: Remove pty=False from running of init.d scripts
Browse files Browse the repository at this point in the history
Summary:
It doesn't work to run init.d scripts with pty, both via Fabric and via native
ssh.  However, CentOS and some other OSes have requiretty in their
/etc/sudoers file, meaning that you get the error "sudo: sorry, you must
have a tty to run sudo".  The only way to fix this is to remove the requiretty
default in a user's /etc/sudoers file, but we don't want to force them to do
that.

The work-around is to run the init script in job control mode (e.g. set -m),
because it avoids the race condition that is the cause of the daemons not
starting when executing commands with TTY.  See
fabric/fabric#395 (comment) for an
in-depth treatment of the issue.

We also add || true when running tar, because tar can sometimes have a
non-zero exit code even when the files correctly un-tarred.

Task: SWARM-363
Review Url: @@review_url@@

Test Plan: make clean lint test-all; manual

Reviewers: anu, rschlussel

Reviewed By: rschlussel

Subscribers: an186016, mf186042

Differential Revision: https://phabricator.td.teradata.com/D395
  • Loading branch information
cawallin committed May 4, 2015
1 parent 118876a commit bface38
Show file tree
Hide file tree
Showing 4 changed files with 6 additions and 6 deletions.
2 changes: 1 addition & 1 deletion packaging/install-prestoadmin.template
@@ -1,7 +1,7 @@
#!/bin/bash
set -e

tar xvzf third-party/virtualenv-%VIRTUALENV_VERSION%.tar.gz -C third-party
tar xvzf third-party/virtualenv-%VIRTUALENV_VERSION%.tar.gz -C third-party || true

python third-party/virtualenv-%VIRTUALENV_VERSION%/virtualenv.py presto-admin-install

Expand Down
2 changes: 1 addition & 1 deletion prestoadmin/server.py
Expand Up @@ -106,7 +106,7 @@ def uninstall():

def service(control=None):
_LOGGER.info("Executing %s on presto server" % control)
sudo(INIT_SCRIPTS + control, pty=False)
sudo("set -m; " + INIT_SCRIPTS + control)


def check_status_for_control_commands():
Expand Down
2 changes: 1 addition & 1 deletion tests/files/install-prestoadmin.sh.expected
@@ -1,7 +1,7 @@
#!/bin/bash
set -e

tar xvzf third-party/virtualenv-12.0.7.tar.gz -C third-party
tar xvzf third-party/virtualenv-12.0.7.tar.gz -C third-party || true

python third-party/virtualenv-12.0.7/virtualenv.py presto-admin-install

Expand Down
6 changes: 3 additions & 3 deletions tests/test_server.py
Expand Up @@ -91,7 +91,7 @@ def test_server_start_fail(self, mock_version_check, mock_warn,
mock_status.return_value = False
env.host = "failed_node1"
server.start()
mock_sudo.assert_called_with(INIT_SCRIPTS + ' start', pty=False)
mock_sudo.assert_called_with('set -m; ' + INIT_SCRIPTS + ' start')
mock_version_check.assert_called_with()
mock_warn.assert_called_with("Server failed to start on: failed_node1")

Expand All @@ -111,7 +111,7 @@ def test_server_start(self, mock_version_check, mock_status,
@patch('prestoadmin.server.sudo')
def test_server_stop(self, mock_sudo):
server.stop()
mock_sudo.assert_called_with(INIT_SCRIPTS + ' stop', pty=False)
mock_sudo.assert_called_with('set -m; ' + INIT_SCRIPTS + ' stop')

@patch('prestoadmin.server.sudo')
@patch('prestoadmin.server.check_server_status')
Expand All @@ -122,7 +122,7 @@ def test_server_restart_fail(self, mock_version_check, mock_warn,
mock_status.return_value = False
env.host = "failed_node1"
server.restart()
mock_sudo.assert_called_with(INIT_SCRIPTS + ' restart', pty=False)
mock_sudo.assert_called_with('set -m; ' + INIT_SCRIPTS + ' restart')
mock_version_check.assert_called_with()
mock_warn.assert_called_with("Server failed to start on: failed_node1")

Expand Down

0 comments on commit bface38

Please sign in to comment.