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.

Sign up for GitHub

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

Jump to bottom

added configurable scp put timeout. added tests #121

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

added configurable scp put timeout. added tests #121

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9017bef
added configurable scp put timeout. added tests
Jun 17, 2022
1639d84
Merge branch 'main' into issue120-scpclient-timeout-config
TAlonglong Jun 17, 2022
ef0b681
fix stikler
Jun 17, 2022
a720abb
Merge branch 'issue120-scpclient-timeout-config' of github.com:TAlong…
Jun 17, 2022
570883d
need to be a value
Jun 17, 2022
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
13 changes: 11 additions & 2 deletions trollmoves/movers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@

"""Movers for the move_it scripts."""

from doctest import ELLIPSIS_MARKER
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see this in main at all, so don't know where it's coming from. Should be safe to remove.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, actually, it's coming from this PR since it shows up in the diff.

import logging
import os
import shutil
Expand All @@ -35,7 +36,7 @@

from ftplib import FTP, all_errors, error_perm
from paramiko import SSHClient, SSHException, AutoAddPolicy
from scp import SCPClient
from scp import SCPClient, SCPException
try:
from s3fs import S3FileSystem
except ImportError:
Expand Down Expand Up @@ -358,7 +359,8 @@ def copy(self):
self._dest_username)

try:
scp = SCPClient(ssh_connection.get_transport())
scp = SCPClient(ssh_connection.get_transport(),
socket_timeout=int(self.attrs.get('scpclient_timeout_seconds', 10)))
except Exception as err:
LOGGER.error("Failed to initiate SCPClient: %s", str(err))
ssh_connection.close()
Expand All @@ -374,6 +376,13 @@ def copy(self):
else:
LOGGER.error("OSError in scp.put: %s", str(osex))
raise
except SCPException as scpe:
if str(scpe) in "Timeout waiting for scp response":
LOGGER.error("SCPClient put got a socket timeout. You could add scpclient_timeout_seconds "
"to your config to increase the timeout interval. Default timeout is 10 seconds.")
else:
LOGGER.error("SCPException: %s", str(scpe))
raise
except Exception as err:
LOGGER.error("Something went wrong with scp: %s", str(err))
LOGGER.error("Exception name %s", type(err).__name__)
Expand Down
35 changes: 34 additions & 1 deletion trollmoves/tests/test_ssh_server.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
#!/usr/bin/env python
# -*- coding: utf-8 -*-
#
# Copyright (c) 2019
# Copyright (c) 2019, 2022
#
# Author(s):
#
Expand Down Expand Up @@ -54,6 +54,7 @@ def setUp(self):

self._attrs_empty = {}
self._attrs_connection_uptime = {'connection_uptime': 0}
self._attrs_timeout = {'scpclient_timeout_seconds': 1}

def tearDown(self):
try:
Expand Down Expand Up @@ -178,6 +179,18 @@ def test_scp_copy(self, mock_scp_client, mock_sshclient):

mocked_scp_client.put.assert_called_once_with(self.origin, urlparse(self.destination_no_port).path)

@patch('trollmoves.movers.SSHClient', autospec=True)
@patch('trollmoves.movers.SCPClient', autospec=True)
def test_scp_copy_custom_timeout(self, mock_scp_client, mock_sshclient):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The timeout isn't tested in this test, so either adjust the name or test the timeout.

"""Check scp copy."""
mocked_scp_client = MagicMock()
mock_scp_client.return_value = mocked_scp_client

scp_mover = trollmoves.movers.ScpMover(self.origin, self.destination_no_port, attrs=self._attrs_timeout)
scp_mover.copy()

mocked_scp_client.put.assert_called_once_with(self.origin, urlparse(self.destination_no_port).path)

@patch('trollmoves.movers.SSHClient', autospec=True)
@patch('trollmoves.movers.SCPClient', autospec=True)
def test_scp_copy_generic_exception(self, mock_scp_client, mock_sshclient):
Expand Down Expand Up @@ -216,6 +229,26 @@ def test_scp_copy_put_exception(self, mock_scp_client):
with pytest.raises(Exception):
scp_mover.copy()

@patch('trollmoves.movers.SCPClient', autospec=True)
def test_scp_copy_put_SCPException(self, mock_scp_client):
"""Check scp client.put() raising SCPException."""
from scp import SCPException
scp_mover = trollmoves.movers.ScpMover(self.origin, self.destination_no_port, attrs=self._attrs_empty)
mock_scp_client.return_value.put.side_effect = SCPException('Timeout waiting for scp response')

with pytest.raises(SCPException, match='Timeout waiting for scp response'):
scp_mover.copy()

@patch('trollmoves.movers.SCPClient', autospec=True)
def test_scp_copy_put_SCPException2(self, mock_scp_client):
"""Check scp client.put() raising SCPException."""
from scp import SCPException
scp_mover = trollmoves.movers.ScpMover(self.origin, self.destination_no_port, attrs=self._attrs_empty)
mock_scp_client.return_value.put.side_effect = SCPException('Other exception')

with pytest.raises(SCPException, match='Other exception'):
scp_mover.copy()

@patch('trollmoves.movers.SSHClient', autospec=True)
@patch('trollmoves.movers.SCPClient', autospec=True)
def test_scp_move(self, mock_scp_client, mock_sshclient):
Expand Down