remote: fix name and ID collisions of containers and pods #7867
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vrothberg 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 |
openshift-ci-robot
added
the
approved
|
@containers/podman-maintainers PTAL ... something to look for at other places as well (e.g., volumes, networks, etc.). |
vrothberg
commented
Oct 1, 2020
|
Maybe, container removal needs a similar batch-removal endpoint as we have for images? |
|
I don't think fixing this by adding a batch endpoint is a good idea. This
code is used in dozens of places and we'd need new endpoints for each. I
think we may want to investigate improving the list endpoint so we only
need to make one call - adding a nameOrId filter maybe?
…On Thu, Oct 1, 2020, 08:45 Valentin Rothberg ***@***.***> wrote:
Maybe, container removal needs a similar batch-removal endpoint as we have
for images?
—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub
<#7867 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AB3AOCGTCGTBUBLRX54Q4XLSIR2WNANCNFSM4SALSJFQ>
.
|
|
This change LGTM though. We can optimize later.
…On Thu, Oct 1, 2020, 09:21 Matthew Heon ***@***.***> wrote:
I don't think fixing this by adding a batch endpoint is a good idea. This
code is used in dozens of places and we'd need new endpoints for each. I
think we may want to investigate improving the list endpoint so we only
need to make one call - adding a nameOrId filter maybe?
On Thu, Oct 1, 2020, 08:45 Valentin Rothberg ***@***.***>
wrote:
> Maybe, container removal needs a similar batch-removal endpoint as we
> have for images?
>
> —
> You are receiving this because you are on a team that was mentioned.
> Reply to this email directly, view it on GitHub
> <#7867 (comment)>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AB3AOCGTCGTBUBLRX54Q4XLSIR2WNANCNFSM4SALSJFQ>
> .
>
|
Sounds good to me. I had the same idea to extend the list endpoint (see code comments). |
|
Failures look legit, will have a look |
Fix the look up of containers and pods in the remote client. User input can refer to both, names or IDs of containers and pods, so there is a fair chance of collisions (e.g., "c1" name with a "c1...." ID). Those collisions are well handled (and battle tested) in the local client which is directly using the libpod backend. Hence, the remote client should not attempt to introduce its own logic to prevent bugs and divergence between the local and the remote clients. To prevent collisions such as in containers#7837, do a container/pod inspect on the user-provided input to find the corresponding ID and eventually do full ID comparisons to avoid potential collisions with names. Note that this has a cost that I am not entirely happy with. Looking at issue containers#7837, the collisions are happening when removing the two containers. Remote container removal is now very chatty with the server as it first queries for all containers, then iterates over the provided names or IDs and does a remote inspect to figure out the IDs and find a matching container object. However, remote removal could just pass the names and IDs directly to the batch removal endpoint. Querying for all containers could be prevented if the batch removal endpoint would remove all if the slice is empty. In other words, the bug is fixed but there's room for performance improvements. Fixes: containers#7837 Signed-off-by: Valentin Rothberg <[email protected]>
|
Updated, pod tests should pass now. Had to add some |
|
/lgtm |
openshift-ci-robot
added
the
do-not-merge/hold
|
/hold cancel |
openshift-ci-robot
removed
the
do-not-merge/hold
github-actions
bot
added
the
locked - please file new issue/PR
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix the look up of containers and pods in the remote client. User input
can refer to both, names or IDs of containers and pods, so there is a fair
chance of collisions (e.g., "c1" name with a "c1...." ID).
Those collisions are well handled (and battle tested) in the local
client which is directly using the libpod backend. Hence, the remote
client should not attempt to introduce its own logic to prevent bugs and
divergence between the local and the remote clients. To prevent
collisions such as in #7837, do a container/pod inspect on the
user-provided input to find the corresponding ID and eventually do full
ID comparisons to avoid potential collisions with names.
Note that this has a cost that I am not entirely happy with. Looking at
issue #7837, the collisions are happening when removing the two
containers. Remote container removal is now very chatty with the server
as it first queries for all containers, then iterates over the provided
names or IDs and does a remote inspect to figure out the IDs and find a
matching container object. However, remote removal could just pass the
names and IDs directly to the batch removal endpoint. Querying for all
containers could be prevented if the batch removal endpoint would remove
all if the slice is empty.
In other words, the bug is fixed but there's room for performance
improvements.
Fixes: #7837
Signed-off-by: Valentin Rothberg [email protected]