Refactor attach()/start() after podman changes

* Update examples
* Update/Clean up unittests
* Add Mixins for container attach()/start()

Signed-off-by: Jhon Honce <jhonce@redhat.com>

Closes: #1080
Approved by: baude

7 files changed, 132 insertions, 90 deletions

diff --git a/contrib/python/examples/eg_attach.py b/contrib/python/examples/eg_attach.py
index f8008163f..f5070dc53 100644
--- a/contrib/python/examples/eg_attach.py
+++ b/contrib/python/examples/eg_attach.py
@@ -1,5 +1,5 @@
 #!/usr/bin/env python3
-"""Example: Run Alpine container and attach."""
+"""Example: Run top on Alpine container."""
 
 import podman
 
@@ -8,10 +8,11 @@ print('{}\n'.format(__doc__))
 with podman.Client() as client:
     id = client.images.pull('alpine:latest')
     img = client.images.get(id)
-    cntr = img.create()
-    cntr.start()
+    cntr = img.create(detach=True, tty=True, command=['/usr/bin/top'])
+    cntr.attach(eot=4)
 
     try:
-        cntr.attach()
-    except BrokenPipeError:
-        print('Container disconnected.')
+        cntr.start()
+        print()
+    except (BrokenPipeError, KeyboardInterrupt):
+        print('\nContainer disconnected.')
diff --git a/contrib/python/podman/libs/_containers_attach.py b/contrib/python/podman/libs/_containers_attach.py
index bd73542b9..df12fa998 100644
--- a/contrib/python/podman/libs/_containers_attach.py
+++ b/contrib/python/podman/libs/_containers_attach.py
@@ -1,16 +1,11 @@
 """Exported method Container.attach()."""
 
+import collections
 import fcntl
-import os
-import select
-import signal
-import socket
+import logging
 import struct
 import sys
 import termios
-import tty
-
-CONMON_BUFSZ = 8192
 
 
 class Mixin:
@@ -20,10 +15,8 @@ class Mixin:
         """Attach to container's PID1 stdin and stdout.
 
         stderr is ignored.
+        PseudoTTY work is done in start().
         """
-        if not self.containerrunning:
-            raise Exception('you can only attach to running containers')
-
         if stdin is None:
             stdin = sys.stdin.fileno()
 
@@ -41,73 +34,42 @@ class Mixin:
             )
 
         # This is the control socket where resizing events are sent to conmon
-        ctl_socket = attach['sockets']['control_socket']
+        # attach['sockets']['control_socket']
+        self.pseudo_tty = collections.namedtuple(
+            'PseudoTTY',
+            ['stdin', 'stdout', 'io_socket', 'control_socket', 'eot'])(
+                stdin,
+                stdout,
+                attach['sockets']['io_socket'],
+                attach['sockets']['control_socket'],
+                eot,
+            )
 
-        def resize_handler(signum, frame):
-            """Send the new window size to conmon.
+    @property
+    def resize_handler(self):
+        """Send the new window size to conmon."""
 
-            The method arguments are not used.
-            """
-            packed = fcntl.ioctl(stdout, termios.TIOCGWINSZ,
+        def wrapped(signum, frame):
+            packed = fcntl.ioctl(self.pseudo_tty.stdout, termios.TIOCGWINSZ,
                                  struct.pack('HHHH', 0, 0, 0, 0))
             rows, cols, _, _ = struct.unpack('HHHH', packed)
+            logging.debug('Resize window({}x{}) using {}'.format(
+                rows, cols, self.pseudo_tty.control_socket))
+
             # TODO: Need some kind of timeout in case pipe is blocked
-            with open(ctl_socket, 'w') as skt:
+            with open(self.pseudo_tty.control_socket, 'w') as skt:
                 # send conmon window resize message
                 skt.write('1 {} {}\n'.format(rows, cols))
 
-        def log_handler(signum, frame):
-            """Send command to reopen log to conmon.
+        return wrapped
+
+    @property
+    def log_handler(self):
+        """Send command to reopen log to conmon."""
 
-            The method arguments are not used.
-            """
-            with open(ctl_socket, 'w') as skt:
+        def wrapped(signum, frame):
+            with open(self.pseudo_tty.control_socket, 'w') as skt:
                 # send conmon reopen log message
                 skt.write('2\n')
 
-        try:
-            # save off the old settings for terminal
-            original_attr = termios.tcgetattr(stdout)
-            tty.setraw(stdin)
-
-            # initialize containers window size
-            resize_handler(None, sys._getframe(0))
-
-            # catch any resizing events and send the resize info
-            # to the control fifo "socket"
-            signal.signal(signal.SIGWINCH, resize_handler)
-
-        except termios.error:
-            original_attr = None
-
-        try:
-            # TODO: socket.SOCK_SEQPACKET may not be supported in Windows
-            with socket.socket(socket.AF_UNIX, socket.SOCK_SEQPACKET) as skt:
-                # Prepare socket for communicating with conmon/container
-                skt.connect(io_socket)
-                skt.sendall(b'\n')
-
-                sources = [skt, stdin]
-                while sources:
-                    readable, _, _ = select.select(sources, [], [])
-                    if skt in readable:
-                        data = skt.recv(CONMON_BUFSZ)
-                        if not data:
-                            sources.remove(skt)
-
-                        # Remove source marker when writing
-                        os.write(stdout, data[1:])
-
-                    if stdin in readable:
-                        data = os.read(stdin, CONMON_BUFSZ)
-                        if not data:
-                            sources.remove(stdin)
-
-                        skt.sendall(data)
-
-                        if eot in data:
-                            sources.clear()
-        finally:
-            if original_attr:
-                termios.tcsetattr(stdout, termios.TCSADRAIN, original_attr)
-                signal.signal(signal.SIGWINCH, signal.SIG_DFL)
+        return wrapped
diff --git a/contrib/python/podman/libs/_containers_start.py b/contrib/python/podman/libs/_containers_start.py
new file mode 100644
index 000000000..ad9f32eab
--- /dev/null
+++ b/contrib/python/podman/libs/_containers_start.py
@@ -0,0 +1,82 @@
+"""Exported method Container.start()."""
+import logging
+import os
+import select
+import signal
+import socket
+import sys
+import termios
+import tty
+
+CONMON_BUFSZ = 8192
+
+
+class Mixin:
+    """Publish start() for inclusion in Container class."""
+
+    def start(self):
+        """Start container, return container on success.
+
+        Will block if container has been detached.
+        """
+        with self._client() as podman:
+            results = podman.StartContainer(self.id)
+            logging.debug('Started Container "{}"'.format(
+                results['container']))
+
+            if not hasattr(self, 'pseudo_tty') or self.pseudo_tty is None:
+                return self._refresh(podman)
+
+            logging.debug('Setting up PseudoTTY for Container "{}"'.format(
+                results['container']))
+
+            try:
+                # save off the old settings for terminal
+                tcoldattr = termios.tcgetattr(self.pseudo_tty.stdin)
+                tty.setraw(self.pseudo_tty.stdin)
+
+                # initialize container's window size
+                self.resize_handler(None, sys._getframe(0))
+
+                # catch any resizing events and send the resize info
+                # to the control fifo "socket"
+                signal.signal(signal.SIGWINCH, self.resize_handler)
+
+            except termios.error:
+                tcoldattr = None
+
+            try:
+                # TODO: Is socket.SOCK_SEQPACKET supported in Windows?
+                with socket.socket(socket.AF_UNIX,
+                                   socket.SOCK_SEQPACKET) as skt:
+                    # Prepare socket for use with conmon/container
+                    skt.connect(self.pseudo_tty.io_socket)
+
+                    sources = [skt, self.pseudo_tty.stdin]
+                    while sources:
+                        logging.debug('Waiting on sources: {}'.format(sources))
+                        readable, _, _ = select.select(sources, [], [])
+
+                        if skt in readable:
+                            data = skt.recv(CONMON_BUFSZ)
+                            if data:
+                                # Remove source marker when writing
+                                os.write(self.pseudo_tty.stdout, data[1:])
+                            else:
+                                sources.remove(skt)
+
+                        if self.pseudo_tty.stdin in readable:
+                            data = os.read(self.pseudo_tty.stdin, CONMON_BUFSZ)
+                            if data:
+                                skt.sendall(data)
+
+                                if self.pseudo_tty.eot in data:
+                                    sources.clear()
+                            else:
+                                sources.remove(self.pseudo_tty.stdin)
+            finally:
+                if tcoldattr:
+                    termios.tcsetattr(self.pseudo_tty.stdin, termios.TCSADRAIN,
+                                      tcoldattr)
+                    signal.signal(signal.SIGWINCH, signal.SIG_DFL)
+            return self._refresh(podman)
diff --git a/contrib/python/podman/libs/containers.py b/contrib/python/podman/libs/containers.py
index a350a128a..6dc2c141e 100644
--- a/contrib/python/podman/libs/containers.py
+++ b/contrib/python/podman/libs/containers.py
@@ -7,9 +7,10 @@ import signal
 import time
 
 from ._containers_attach import Mixin as AttachMixin
+from ._containers_start import Mixin as StartMixin
 
 
-class Container(collections.UserDict, AttachMixin):
+class Container(AttachMixin, StartMixin, collections.UserDict):
     """Model for a container."""
 
     def __init__(self, client, id, data):
@@ -143,12 +144,6 @@ class Container(collections.UserDict, AttachMixin):
                                     message, pause)
         return results['image']
 
-    def start(self):
-        """Start container, return container on success."""
-        with self._client() as podman:
-            podman.StartContainer(self.id)
-            return self._refresh(podman)
-
     def stop(self, timeout=25):
         """Stop container, return id on success."""
         with self._client() as podman:
diff --git a/contrib/python/podman/libs/images.py b/contrib/python/podman/libs/images.py
index 3beadec1d..334ff873c 100644
--- a/contrib/python/podman/libs/images.py
+++ b/contrib/python/podman/libs/images.py
@@ -3,6 +3,7 @@ import collections
 import copy
 import functools
 import json
+import logging
 
 from . import Config
 from .containers import Container
@@ -37,11 +38,8 @@ class Image(collections.UserDict):
 
         Pulls defaults from image.inspect()
         """
-        with self._client() as podman:
-            details = self.inspect()
+        details = self.inspect()
 
-        # TODO: remove network settings once defaults implemented in service
-        # Inialize config from parameters, then add image information
         config = Config(image_id=self.id, **kwargs)
         config['command'] = details.containerconfig['cmd']
         config['env'] = self._split_token(details.containerconfig['env'])
@@ -49,8 +47,8 @@ class Image(collections.UserDict):
         config['labels'] = copy.deepcopy(details.labels)
         config['net_mode'] = 'bridge'
         config['network'] = 'bridge'
-        config['work_dir'] = '/tmp'
 
+        logging.debug('Image {}: create config: {}'.format(self.id, config))
         with self._client() as podman:
             id = podman.CreateContainer(config)['container']
             cntr = podman.GetContainer(id)
diff --git a/contrib/python/test/test_containers.py b/contrib/python/test/test_containers.py
index 87d43adb4..ec2dcde03 100644
--- a/contrib/python/test/test_containers.py
+++ b/contrib/python/test/test_containers.py
@@ -72,14 +72,18 @@ class TestContainers(PodmanTestCase):
             mock_in.write('echo H"ello, World"; exit\n')
             mock_in.seek(0, 0)
 
-            self.alpine_ctnr.attach(
-                stdin=mock_in.fileno(), stdout=mock_out.fileno())
+            ctnr = self.pclient.images.get(self.alpine_ctnr.image).container(
+                detach=True, tty=True)
+            ctnr.attach(stdin=mock_in.fileno(), stdout=mock_out.fileno())
+            ctnr.start()
 
             mock_out.flush()
             mock_out.seek(0, 0)
             output = mock_out.read()
             self.assertIn('Hello', output)
 
+            ctnr.remove(force=True)
+
     def test_processes(self):
         actual = list(self.alpine_ctnr.processes())
         self.assertGreaterEqual(len(actual), 2)
@@ -133,8 +137,7 @@ class TestContainers(PodmanTestCase):
     def test_commit(self):
         # TODO: Test for STOPSIGNAL when supported by OCI
         # TODO: Test for message when supported by OCI
-        details = self.pclient.images.get(
-            self.alpine_ctnr.inspect().image).inspect()
+        details = self.pclient.images.get(self.alpine_ctnr.image).inspect()
         changes = ['ENV=' + i for i in details.containerconfig['env']]
         changes.append('CMD=/usr/bin/zsh')
         changes.append('ENTRYPOINT=/bin/sh date')
diff --git a/contrib/python/test/test_images.py b/contrib/python/test/test_images.py
index c5695b722..14bf90992 100644
--- a/contrib/python/test/test_images.py
+++ b/contrib/python/test/test_images.py
@@ -62,6 +62,7 @@ class TestImages(PodmanTestCase):
         actual = self.alpine_image.container()
         self.assertIsNotNone(actual)
         self.assertEqual(actual.status, 'configured')
+
         ctnr = actual.start()
         self.assertIn(ctnr.status, ['running', 'exited'])