[1LP][RFR] Prepare connect_ssh (#10056)

* Make trying_ips not default to ininite loop, make it use math.inf.

Having the rounds parameter may not be the best idea. Perhaps it
is better to make it mandatory parameter.

Setting it to math.inf will result in ininite ammount of attempts to
connect.

* Prepare connect_ssh

* Redesign reconnection stuff. Make it use unquirking client-factory method.

* Remove retry_connect_rounds

* React on review of unquirked_ssh_client.

Add more doc, accept only kwargs.

* Make _try_batch_of_ips underscored.

* Remove f string.

Author: Jaroslav Henner

cfme/fixtures/utility_vm.py

@@ -16,77 +16,54 @@
 ping, but the required service may not be up and ready.
 """
 import os.path
-import time
-from contextlib import contextmanager
 
 import pytest
 
 from cfme.base.credential import SSHCredential
 from cfme.utils.conf import cfme_data
 from cfme.utils.generators import random_vm_name
 from cfme.utils.log import logger
-from cfme.utils.net import net_check
+from cfme.utils.net import pick_responding_ip
 from cfme.utils.ssh import SSHClient
 from cfme.utils.virtual_machines import deploy_template
-from cfme.utils.wait import TimedOutError
 
 
-def _trying_ips(vm, attempts=60, interval=10):
-    for attempt in range(attempts):
-        for ip in getattr(vm, 'all_ips', []):
-            yield ip
-            return
-        time.sleep(interval)
-
-
-@contextmanager
-def connect_ssh(vm, creds):
-    for ip in _trying_ips(vm):
-        try:
-            with SSHClient(hostname=ip, username=creds.principal, password=creds.secret) as client:
-                logger.info("SSH connected to IP %s", ip)
-                result = client.run_command("true")
-                if not result.success:
-                    raise Exception(f"Command `true` failed on ip {ip}.")
-                yield client
-                return
-        except Exception as ex:
-            logger.info("Failed to connect with SSH to %s: %s", ip, ex)
-    else:
-        raise TimedOutError(f"Coudln't find an IP responding to ssh for vm {vm}")
-
-
-def _pick_responding_ip(vm, port):
-    for ip in _trying_ips(vm):
-        if net_check(port, ip):
-            return ip
-    else:
-        raise TimedOutError(f"Coudln't find an IP of vm {vm} with port {port} responding")
+ATTEMPT_TIMEOUT = 10
+""" How long to wait until connection is determined as not successful. """
+IP_PICK_TIMEOUT = 600
+""" How many rounds to connect all the vm IPs. """
+ROUNDS_DELAY = 10
+""" How long to delay after unsucessful connection round. """
 
 
 @pytest.fixture
 def utility_vm_nfs_ip(utility_vm):
     vm, _, _ = utility_vm
     one_of_the_nfs_ports = 111
-    yield _pick_responding_ip(vm, one_of_the_nfs_ports)
+    yield pick_responding_ip(lambda: vm.all_ips, one_of_the_nfs_ports,
+        IP_PICK_TIMEOUT, ROUNDS_DELAY, ATTEMPT_TIMEOUT)
 
 
 @pytest.fixture
 def utility_vm_samba_ip(utility_vm):
     vm, _, _ = utility_vm
-    yield _pick_responding_ip(vm, 445)
+    yield pick_responding_ip(lambda: vm.all_ips, 445,
+        IP_PICK_TIMEOUT, ROUNDS_DELAY, ATTEMPT_TIMEOUT)
 
 
 @pytest.fixture(scope='module')
 def utility_vm_proxy_data(utility_vm):
     vm, __, data = utility_vm
-    yield _pick_responding_ip(vm, data.proxy.port), data.proxy.port
+    ip = pick_responding_ip(lambda: vm.all_ips, data.proxy.port,
+        IP_PICK_TIMEOUT, ROUNDS_DELAY, ATTEMPT_TIMEOUT)
+    yield ip, data.proxy.port
 
 
 @pytest.fixture(scope='module')
 def utility_vm_ssh(utility_vm):
     vm, injected_user_cred, __ = utility_vm
-    ip = _pick_responding_ip(vm, 22)
+    ip = pick_responding_ip(lambda: vm.all_ips, 22,
+        IP_PICK_TIMEOUT, ROUNDS_DELAY, ATTEMPT_TIMEOUT)
 
     with SSHClient(
             hostname=ip,

cfme/utils/net.py

@@ -58,6 +58,51 @@ def ip_echo_socket(port=32123):
             conn.close()
 
 
+def pick_responding_ip(vm, port, num_sec, rounds_delay_second, attempt_timeout):
+    """
+    Given a vm and port, pick one of the vm's addresses that is connectible
+    on the given port
+
+    Args:
+        vm: mgmt vm
+        port: port number to attempt connecting to
+        num_sec: Minimal ammount of time how long to keep checking (slight
+                 variation may happen -- approximately the attempt_timeout).
+        rounds_delay_second: The delay to wait after checking each IP round in
+                             immediate succession.
+        attempt_timeout: A connection timeout for every connection attempt.
+
+    Raise TimedOutError if no such IP is found.
+    """
+
+    def connection_factory(ip):
+        if net_check(port, ip, attempt_timeout):
+            return ip
+
+    return retry_connect(vm, connection_factory, num_sec, rounds_delay_second)
+
+
+def retry_connect(ips_getter, connection_factory, num_sec, delay):
+    def _try_batch_of_ips():
+        for ip in ips_getter():
+            try:
+                connection = connection_factory(ip)
+            except Exception as ex:
+                logger.warning(f"Failed to connect {ip}: {ex}")
+                continue
+            else:
+                logger.info(f"Connected to IP {ip}")
+                # No other IPs should be attempted, so return.
+                return connection
+        return False
+    connection, _ = wait_for(_try_batch_of_ips, num_sec=num_sec, delay=delay)
+    return connection
+
+
+def retry_connect_vm(vm, connection_factory, num_sec, delay):
+    return retry_connect(lambda: vm.all_ips, connection_factory, num_sec, delay)
+
+
 def net_check(port, addr=None, force=False, timeout=10):
     """Checks the availablility of a port"""
     port = int(port)

cfme/utils/ssh.py

@@ -18,6 +18,7 @@
 from cfme.utils import ports
 from cfme.utils.log import logger
 from cfme.utils.net import net_check
+from cfme.utils.net import retry_connect_vm
 from cfme.utils.path import project_path
 from cfme.utils.quote import quote
 from cfme.utils.timeutil import parsetime
@@ -28,6 +29,9 @@
 # Default blocking time before giving up on an ssh command execution,
 # in seconds (float)
 RUNCMD_TIMEOUT = 1200.0
+CONNECT_RETRIES_TIMEOUT = 2 * 60
+CONNECT_TIMEOUT = 10
+CONNECT_SSH_DELAY = 1
 
 
 @attr.s(frozen=True, eq=False)
@@ -121,7 +125,7 @@ class SSHClient(paramiko.SSHClient):
         stdout: If specified, overrides the system stdout file for streaming output.
         stderr: If specified, overrides the system stderr file for streaming output.
     """
-    def __init__(self, stream_output=False, **connect_kwargs):
+    def __init__(self, *, stream_output=False, **connect_kwargs):
         super().__init__()
         self._streaming = stream_output
         # deprecated/useless karg, included for backward-compat
@@ -135,11 +139,12 @@ def __init__(self, stream_output=False, **connect_kwargs):
         self.oc_password = connect_kwargs.pop('oc_password', False)
         self.f_stdout = connect_kwargs.pop('stdout', sys.stdout)
         self.f_stderr = connect_kwargs.pop('stderr', sys.stderr)
+        self._use_check_port = connect_kwargs.pop('use_check_port', True)
         self.strict_host_key_checking = connect_kwargs.pop('strict_host_key_checking', True)
 
         # load the defaults for ssh, including current_appliance and default credentials keys
         compiled_kwargs = dict(
-            timeout=10,
+            timeout=CONNECT_TIMEOUT,
             allow_agent=False,
             look_for_keys=False,
             gss_auth=False,
@@ -231,7 +236,9 @@ def connect(self, hostname=None, **kwargs):
 
         if not self.connected:
             self._connect_kwargs.update(kwargs)
-            wait_for(self._check_port, handle_exception=True, timeout='2m', delay=5)
+            if self._use_check_port:
+                wait_for(self._check_port, handle_exception=True,
+                         timeout=CONNECT_TIMEOUT, delay=5)
             try:
                 conn = super().connect(**self._connect_kwargs)
             except paramiko.ssh_exception.BadHostKeyException:
@@ -842,3 +849,42 @@ def keygen():
     with _ssh_pubkey_file.open('w') as f:
         f.write("{} {} {}\n".format(pub.get_name(), pub.get_base64(),
             'autogenerated cfme_tests key'))
+
+
+def unquirked_ssh_client(**kwargs):
+    """ A factory for the SSHClient dealing with some of its quirks:
+
+    * When hostname is None, the SSHClient will connect to the default
+      appliance.
+    * Set use_check_port to False to disable waiting for port being
+      available in the SSHClient.
+    * The SSHClient does not connect in the object creation time.
+    """
+    if kwargs['hostname'] is None:
+        raise ValueError('The hostname argument cannot be None!')
+    client = SSHClient(use_check_port=False, **kwargs)
+    client.connect()
+    return client
+
+
+def connect_ssh(vm, creds,
+                num_sec=CONNECT_RETRIES_TIMEOUT,
+                connect_timeout=CONNECT_TIMEOUT,
+                delay=CONNECT_SSH_DELAY):
+    """
+    This function attempts to connect all the IPs of the vm in several
+    round. After each round it will delay an ammount of seconds specified as
+    `delay`. Once connective IP is found, it will return connected SSHClient.
+    The `connect_timeout` specifies a timeout for each connection attempt.
+    """
+
+    def _connection_factory(ip):
+        return unquirked_ssh_client(
+            hostname=ip,
+            username=creds.principal, password=creds.secret,
+            timeout=connect_timeout)
+
+    msg = "The num_sec is smaller then connect_timeout. This looks like a bug."
+    assert num_sec > connect_timeout, msg
+
+    return retry_connect_vm(vm, _connection_factory, num_sec=num_sec, delay=delay)

cfme/utils/tests/test_connect_ssh.py

@@ -0,0 +1,68 @@
+from types import SimpleNamespace
+from unittest.mock import Mock
+from unittest.mock import PropertyMock
+
+import pytest
+
+from cfme.utils.ssh import connect_ssh
+
+
+creds = SimpleNamespace(principal='mockuser', secret='mockpass')
+
+
+@pytest.fixture(autouse=True)
+def nosleep(mocker):
+    mocker.patch('time.sleep')
+
+
+@pytest.fixture
+def vm_mock():
+    vm = Mock()
+    all_ips_mock = PropertyMock()
+    type(vm).all_ips = all_ips_mock
+    all_ips_mock.side_effect = [
+        [None],
+        ['NOT_WORKING_IP_MOCK'],
+        ['NOT_WORKING_IP_MOCK', 'OTHER_NOT_WORKING_IP_MOCK',
+            'WORKING_IP_MOCK', 'SHOULD_NOT_REACH_THIS_MOCK'],
+        ['SHOULD_NOT_REACH_THIS_MOCK', 'SHOULD_NOT_REACH_THIS_MOCK']
+    ]
+    return vm
+
+
+@pytest.fixture
+def vm_mock2():
+    vm = Mock()
+    all_ips_mock = PropertyMock()
+    type(vm).all_ips = all_ips_mock
+    all_ips_mock.side_effect = [
+        ['WORKING_IP_MOCK', 'SHOULD_NOT_REACH_THIS_MOCK']
+    ]
+    return vm
+
+
+class SSHClientMock:
+    def __init__(self, **kwargs):
+        self.kwargs = kwargs
+        self.close = Mock()
+        self.run_command = Mock()
+        self.run_command.success.return_value = True
+
+        # disabling check_port is required for not making repeated connection
+        # attempts in the SSHClient
+        assert kwargs['use_check_port'] is False
+
+    def connect(self):
+        hostname = self.kwargs['hostname']
+        if hostname == 'WORKING_IP_MOCK':
+            return self
+        elif hostname == 'SHOULD_NOT_REACH_THIS_MOCK':
+            pytest.fail('We should not have reached checking this IP!')
+        else:
+            raise Exception(f'This was raised for IP {hostname}.')
+
+
+def test_connect_ssh(mocker, vm_mock, vm_mock2):
+    mocker.patch('cfme.utils.ssh.SSHClient', SSHClientMock)
+    assert connect_ssh(vm_mock, creds, num_sec=3, connect_timeout=1, delay=1).run_command()
+    assert connect_ssh(vm_mock2, creds, num_sec=3, connect_timeout=1, delay=1).run_command()

requirements/frozen.txt

@@ -259,6 +259,7 @@ pyperclip==1.7.0
 pytesseract==0.3.3
 pytest==5.4.1
 pytest-polarion-collect==0.23.1
+pytest-mock==2.0.0
 python-box==3.2.4
 python-bugzilla==2.3.0
 python-cinderclient==4.1.0

requirements/template_non_imported.txt

@@ -9,5 +9,6 @@ pdfminer.six
 polarion-docstrings
 polarion-tools-common
 pre-commit
+pytest-mock
 pytest-polarion-collect
 pytest-report-parameters