Skip to content

Implement sparse keyword for containers.list() #540

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

inknos
Copy link
Contributor

@inknos inknos commented Apr 14, 2025

@jwhonce I need your guidance here. I made this pr mirroring what the docker behaviour is but this is a change for us. what's your opinion? this is a slowdown, would you rather see sparse=True by default to keep the behavior consistent with the current one?

Copy link
Contributor

openshift-ci bot commented Apr 14, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: inknos

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@jwhonce
Copy link
Member

jwhonce commented Apr 28, 2025

@inknos We could do something crazy like default to True for libpod and False when compatible is True. Is that too odd for developers? It doesn't break current libpod scripts and it will work as expected for docker scripts.

The other option is to wait for podman 6 and announce it as a breaking change.

@inknos
Copy link
Contributor Author

inknos commented Apr 29, 2025

We could do something crazy like default to True for libpod and False when compatible is True. Is that too odd for developers? It doesn't break current libpod scripts and it will work as expected for docker scripts.

Right, I am in for the oddness, as it's not odd at all.

(I'll put it down for future-me, who will forget about it)

Having sparse=True has two big benefits. First, not breaking anything, second, it's faster and generally a safer call (especially for scripting). Furthermore, splitting the podman/docker's behaviours is just a developer's decision. We sacrifice 1-1 compatibility for consistency, faster list calls, and fewer failures by retrievals of statuses for hanging containers. In my opinion, giving the option to sparse=False is the best.

The other option is to wait for podman 6 and announce it as a breaking change.

We can always do this if we get a push from the community to unify the behaviours. Then we only need to flip the default.

@inknos inknos force-pushed the list-sparse branch 4 times, most recently from fe3553b to 9a322ed Compare April 29, 2025 12:38
@inknos
Copy link
Contributor Author

inknos commented Apr 29, 2025

Fixes: #459
Fixes: #446

Defaults to True for Libpod calls and False for Docker Compat calls

This ensures:
1. Docker API compatibility
2. No breaking changes with Libpod

It also provides:
1. Possibility to inspect containers on demand for list calls
2. Safer behavior if container hangs
3. Fewer expensive calls to the API by default

Note: Requests need to pass compat explicitely to reload containers. A
unit test has been added.

Fixes: containers#459
Fixes: containers#446

Signed-off-by: Nicola Sella <nsella@redhat.com>
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements a new "sparse" option for containers.list() to better mirror Docker behavior and introduces a "compatible" flag to control the endpoint selection in both list and reload methods. Key changes include:

  • Adding new unit tests for container reloads and sparse listing behavior under both libpod and Docker compatibility modes.
  • Modifying the reload method in the resource manager to accept additional keyword arguments.
  • Updating the containers_manager get and list methods to support the "compatible" flag and adjust the "sparse" default behavior accordingly.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
podman/tests/unit/test_domainmanager.py Adds tests to verify container reload endpoint resolution with options.
podman/tests/unit/test_containersmanager.py Introduces tests for sparse listing under various compatibility scenarios.
podman/domain/manager.py Updates the reload method to accept keyword arguments (including compatible).
podman/domain/containers_manager.py Adjusts get and list methods to use the "compatible" flag and apply sparse reloads based on mode.

@jwhonce
Copy link
Member

jwhonce commented Jun 17, 2025

LGTM

@Edward5hen Please review and add the "slash lgtm" command in a comment if you agree with the change. TIA

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

Successfully merging this pull request may close these issues.

2 participants