Repository: containers/podman-py

Issue:
Streaming not supported in Container.top
Current implementation of `top` method on container doesn't support streaming. 
If we try to stream, it just hangs up infinitely.

```
Python 3.10.9 (main, Dec 19 2022, 17:35:49) [GCC 12.2.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import podman
>>> client = podman.PodmanClient(base_uri="http+unix:///run/user/1000/podman/podman.sock")
>>> client.containers.list()[0].top(stream=False)
{'Processes': [['mongodb', '1', '0', '0.446', '41m4.487375206s', '?', '11s', 'mongod --bind_ip_all']], 'Titles': ['USER', 'PID', 'PPID', '%CPU', 'ELAPSED', 'TTY', 'TIME', 'COMMAND']}
>>> client.containers.list()[0].top(stream=True)

```
This cause for this issue is similar to #218 

Other problems in `top` is that, it decodes non-stream response to json, but sends the streamed responses as plain binary strings.


Discussion:
openshift-ci[bot]: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/containers/podman-py/pull/250#" title="Author self-approved">RazCrimson</a>*
**Once this PR has been reviewed and has the lgtm label**, please assign vrothberg for approval. For more information see [the Kubernetes Code Review Process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process).

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=containers%2Fpodman-py).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/containers/podman-py/blob/main/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["vrothberg"]} -->
umohnani8: Changes LGTM
@rhatdan @lsm5 @jwhonce PTAL
openshift-ci[bot]: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/containers/podman-py/pull/250#pullrequestreview-1337648517" title="Approved">jwhonce</a>*, *<a href="https://github.com/containers/podman-py/pull/250#" title="Author self-approved">RazCrimson</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=containers%2Fpodman-py).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/containers/podman-py/blob/main/OWNERS)~~ [jwhonce]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
lsm5: LGTM if @rhatdan and @jwhonce are happy.

@RazCrimson might wanna squash commits?
lsm5: > @lsm5 Is this current level of squashing fine or would you like me to squash everything into one commit?

i was just checking if you needed / wanted to squash at all :) . Either way is fine by me.

Also, there's a ModuleNotFoundError now :(
RazCrimson: Weird, there are no actual changes in the files before and after squashing.

Triggering another build did not fix it too.

Any ideas? 🤔 
lsm5: @cevich any idea whatsup with the fedora-37 failure?
cevich: > @cevich any idea whatsup with the fedora-37 failure?

Unfortunately no, I encountered the same/similar errors while trying to update the CI VM images over in https://github.com/containers/podman-py/pull/251

I think we need a python-guru to take a look.
mwhahaha: Probably need to add extras into the requirements. It was removed from `fixtures` via https://github.com/testing-cabal/fixtures/commit/f8df2192bf1190ff21a3067bb9e132b6db85191e however we're using `fixtures~=3.0.0`. Could try bumping fixtures to `4.0.0` or including extras in the requirements.
mwhahaha: Fixing CI via #252
lsm5: @mwhahaha thanks! @RazCrimson rebasing should make CI green.

Patch:
diff --git a/podman/api/__init__.py b/podman/api/__init__.py
index 393f653..b16bc38 100644
--- a/podman/api/__init__.py
+++ b/podman/api/__init__.py
@@ -11,6 +11,7 @@ from podman.api.parse_utils import (
     prepare_cidr,
     prepare_timestamp,
     stream_frames,
+    stream_helper,
 )
 from podman.api.tar_utils import create_tar, prepare_containerfile, prepare_containerignore
 from .. import version
@@ -58,4 +59,5 @@ __all__ = [
     'prepare_filters',
     'prepare_timestamp',
     'stream_frames',
+    'stream_helper',
 ]
diff --git a/podman/api/parse_utils.py b/podman/api/parse_utils.py
index ce66bcf..ffd3d8b 100644
--- a/podman/api/parse_utils.py
+++ b/podman/api/parse_utils.py
@@ -97,3 +97,14 @@ def stream_frames(response: Response) -> Iterator[bytes]:
         if not data:
             return
         yield data
+
+
+def stream_helper(
+    response: Response, decode_to_json: bool = False
+) -> Union[Iterator[bytes], Iterator[Dict[str, Any]]]:
+    """Helper to stream results and optionally decode to json"""
+    for value in response.iter_lines():
+        if decode_to_json:
+            yield json.loads(value)
+        else:
+            yield value
diff --git a/podman/domain/containers.py b/podman/domain/containers.py
index 590cd76..8236edf 100644
--- a/podman/domain/containers.py
+++ b/podman/domain/containers.py
@@ -6,7 +6,6 @@ from contextlib import suppress
 from typing import Any, Dict, Iterable, Iterator, List, Mapping, Optional, Tuple, Union
 
 import requests
-from requests import Response
 
 from podman import api
 from podman.domain.images import Image
@@ -389,7 +388,9 @@ class Container(PodmanResource):
         )
         response.raise_for_status()
 
-    def stats(self, **kwargs) -> Iterator[Union[bytes, Dict[str, Any]]]:
+    def stats(
+        self, **kwargs
+    ) -> Union[bytes, Dict[str, Any], Iterator[bytes], Iterator[Dict[str, Any]]]:
         """Return statistics for container.
 
         Keyword Args:
@@ -413,20 +414,9 @@ class Container(PodmanResource):
         response.raise_for_status()
 
         if stream:
-            return self._stats_helper(decode, response.iter_lines())
+            return api.stream_helper(response, decode_to_json=decode)
 
-        return json.loads(response.text) if decode else response.content
-
-    @staticmethod
-    def _stats_helper(
-        decode: bool, body: Iterator[bytes]
-    ) -> Iterator[Union[bytes, Dict[str, Any]]]:
-        """Helper needed to allow stats() to return either a generator or a bytes."""
-        for entry in body:
-            if decode:
-                yield json.loads(entry)
-            else:
-                yield entry
+        return json.loads(response.content) if decode else response.content
 
     def stop(self, **kwargs) -> None:
         """Stop container.
@@ -466,23 +456,20 @@ class Container(PodmanResource):
             NotFound: when the container no longer exists
             APIError: when the service reports an error
         """
+        stream = kwargs.get("stream", False)
+
         params = {
+            "stream": stream,
             "ps_args": kwargs.get("ps_args"),
-            "stream": kwargs.get("stream", False),
         }
-        response = self.client.get(f"/containers/{self.id}/top", params=params)
+        response = self.client.get(f"/containers/{self.id}/top", params=params, stream=stream)
         response.raise_for_status()
 
-        if params["stream"]:
-            self._top_helper(response)
+        if stream:
+            return api.stream_helper(response, decode_to_json=True)
 
         return response.json()
 
-    @staticmethod
-    def _top_helper(response: Response) -> Iterator[Dict[str, Any]]:
-        for line in response.iter_lines():
-            yield line
-
     def unpause(self) -> None:
         """Unpause processes in container."""
         response = self.client.post(f"/containers/{self.id}/unpause")


Test patch:
diff --git a/podman/tests/unit/test_container.py b/podman/tests/unit/test_container.py
index f7294cf..955f48c 100644
--- a/podman/tests/unit/test_container.py
+++ b/podman/tests/unit/test_container.py
@@ -411,6 +411,53 @@ class ContainersTestCase(unittest.TestCase):
         self.assertDictEqual(actual, body)
         self.assertTrue(adapter.called_once)
 
+    @requests_mock.Mocker()
+    def test_top_with_streaming(self, mock):
+        stream = [
+            {
+                "Processes": [
+                    [
+                        'jhonce',
+                        '2417',
+                        '2274',
+                        '0',
+                        'Mar01',
+                        '?',
+                        '00:00:01',
+                        (
+                            '/usr/bin/ssh-agent /bin/sh -c exec -l /bin/bash -c'
+                            ' "/usr/bin/gnome-session"'
+                        ),
+                    ],
+                    ['jhonce', '5544', '3522', '0', 'Mar01', 'pts/1', '00:00:02', '-bash'],
+                    ['jhonce', '6140', '3522', '0', 'Mar01', 'pts/2', '00:00:00', '-bash'],
+                ],
+                "Titles": ["UID", "PID", "PPID", "C", "STIME", "TTY", "TIME CMD"],
+            }
+        ]
+
+        buffer = io.StringIO()
+        for entry in stream:
+            buffer.write(json.JSONEncoder().encode(entry))
+            buffer.write("\n")
+
+        adapter = mock.get(
+            tests.LIBPOD_URL
+            + "/containers/87e1325c82424e49a00abdd4de08009eb76c7de8d228426a9b8af9318ced5ecd/top"
+            "?stream=True",
+            text=buffer.getvalue(),
+        )
+
+        container = Container(attrs=FIRST_CONTAINER, client=self.client.api)
+        top_stats = container.top(stream=True)
+
+        self.assertIsInstance(top_stats, Iterable)
+        for response, actual in zip(top_stats, stream):
+            self.assertIsInstance(response, dict)
+            self.assertDictEqual(response, actual)
+
+        self.assertTrue(adapter.called_once)
+
 
 if __name__ == '__main__':
     unittest.main()
diff --git a/podman/tests/unit/test_parse_utils.py b/podman/tests/unit/test_parse_utils.py
index 80f30b3..a7768de 100644
--- a/podman/tests/unit/test_parse_utils.py
+++ b/podman/tests/unit/test_parse_utils.py
@@ -1,9 +1,12 @@
 import datetime
 import ipaddress
+import json
 import unittest
-from typing import Any, Optional
-
 from dataclasses import dataclass
+from typing import Any, Iterable, Optional, Tuple
+from unittest import mock
+
+from requests import Response
 
 from podman import api
 
@@ -14,7 +17,7 @@ class ParseUtilsTestCase(unittest.TestCase):
         class TestCase:
             name: str
             input: Any
-            expected: Optional[str]
+            expected: Tuple[str, Optional[str]]
 
         cases = [
             TestCase(name="empty str", input="", expected=("", None)),
@@ -56,12 +59,36 @@ class ParseUtilsTestCase(unittest.TestCase):
 
         self.assertEqual(api.prepare_timestamp(None), None)
         with self.assertRaises(ValueError):
-            api.prepare_timestamp("bad input")
+            api.prepare_timestamp("bad input")  # type: ignore
 
     def test_prepare_cidr(self):
         net = ipaddress.IPv4Network("127.0.0.0/24")
         self.assertEqual(api.prepare_cidr(net), ("127.0.0.0", "////AA=="))
 
+    def test_stream_helper(self):
+        streamed_results = [b'{"test":"val1"}', b'{"test":"val2"}']
+        mock_response = mock.Mock(spec=Response)
+        mock_response.iter_lines.return_value = iter(streamed_results)
+
+        streamable = api.stream_helper(mock_response)
+
+        self.assertIsInstance(streamable, Iterable)
+        for expected, actual in zip(streamed_results, streamable):
+            self.assertIsInstance(actual, bytes)
+            self.assertEqual(expected, actual)
+
+    def test_stream_helper_with_decode(self):
+        streamed_results = [b'{"test":"val1"}', b'{"test":"val2"}']
+        mock_response = mock.Mock(spec=Response)
+        mock_response.iter_lines.return_value = iter(streamed_results)
+
+        streamable = api.stream_helper(mock_response, decode_to_json=True)
+
+        self.assertIsInstance(streamable, Iterable)
+        for expected, actual in zip(streamed_results, streamable):
+            self.assertIsInstance(actual, dict)
+            self.assertDictEqual(json.loads(expected), actual)
+
 
 if __name__ == '__main__':
     unittest.main()