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.

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

Container object uses wrong key path for ports on listed containers. #459

Open
MattBelle opened this issue Oct 28, 2024 · 2 comments
Open

Comments

@MattBelle
Copy link
Contributor

MattBelle commented Oct 28, 2024

The container object is currently pulling ports from Container.NetworkSettings.Ports:

@property
def ports(self):
"""dict[str, int]: Return ports exposed by container."""
with suppress(KeyError):
return self.attrs["NetworkSettings"]["Ports"]
return {}

This works when you are inspecting an individual container like:

container = podman_client.containers.get(some_container_id)
print(container.ports)

but fails when you get containers from a list operation eg:

for container in podman_client.containers.list():
    print(container.ports)

Some brief playing around with the raw REST API makes it look like the object schema returned from those 2 endpoints differs dramatically. The correct spot in the list schema appears to be Container.Ports, but the format of the data would still need to be massaged if we want a coherent schema in the SDK.

Single container inspect returns:

{
'18630/tcp': [
            {
                'HostIp': '0.0.0.0',
                'HostPort': '45753',
            },
        ]
}

while the listed container payload is:

 [
        {
            'host_ip': '',
            'container_port': 18630,
            'host_port': 45753,
            'range': 1,
            'protocol': 'tcp',
        },
    ]
@MattBelle
Copy link
Contributor Author

Assuming we want Container objects with identical IDs to have equivalent property values regardless of their source there are couple different solutions I can think of:

  1. Manually attempt to build the format provided by the inspect command. Only issue I can see with this is that the host_ip doesn't seem to be getting properly populated and there doesn't seem to be another spot in the response to pull that data from.
  2. Post-process what is returned by the list endpoint with individual GET operations. Eg something like:
[podman_client.containers.get(container.id) for container in podman_client.containers.list()]

This obviously has the downside of processing overhead. I could also see this being unwanted behavior if someone explicitly wanted the schema returned by the LIST object.

  1. Some kinda of background upgrade procedure. This would be similar to solution 2, but would be lazily evaluated. EG if someone tried to access the ports for example we would "upgrade" the object by doing the GET behind the scenes and update the field states accordingly.
  2. Make the list object a different type of python object to be more transparent to the users that they need to be treated differently.

I'd be happy to implement the fix, but am relatively new to contributing to and using this project so I would be interested in hearing the thoughts/preferences of people more experienced before I settle on a particular type of solution.

@inknos
Copy link
Contributor

inknos commented Oct 29, 2024

I'd be happy to implement the fix, but am relatively new to contributing to and using this project so I would be interested in hearing the thoughts/preferences of people more experienced before I settle on a particular type of solution.

Thanks for the report. You can always ask and me or other maintainers will try to help you with reviews and advices.

So, my quick take on the issue.

Regarding your first comment

This works when you are inspecting an individual container like:
...
but fails when you get containers from a list operation eg:
...

This can be solved by

for container in podman_client.containers.list():
    container.reload()
    print(container.ports)

We know that the reload call is expensive so we don't fetch the status while doing list. A possible solution to this is to implement the sparse keyword. See another issue here


Some brief playing around with the raw REST API makes it look like the object schema returned from those 2 endpoints differs dramatically. The correct spot in the list schema appears to be Container.Ports, but the format of the data would still need to be massaged if we want a coherent schema in the SDK.

This I am not sure why it happens but changing the payload needs to be done on podman side and it will break the API anyway.


About your second comment.

Options 1 and 2:

I would avoid the post process for the reasons you said

Option 3:

This is probably the best approach

Option 4:

I would avoid since it would break the api


Given that the issue seems to be the difference in get and list behavior I would squeeze the requirements to change the list method to use the sparse keyword.
This will not break the api, and it will also add the overhead of reloading the containers on-demand.

What do you think @MattBelle ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants