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

remote: fix invalid --cidfile + --ignore #23581

Merged
merged 2 commits into from
Aug 12, 2024

Conversation

Luap99
Copy link
Member

@Luap99 Luap99 commented Aug 12, 2024

When the cidfile does not exists and ignore is set the cli parser skips
the file without error and we call into the backend code without any
names at all. This should logically be a NOP but on remote it caused all
containers to be returned which caused podman stop to stop everything in
this case.

Fixes #23554


pkg/bindings/containers: handle ignore for stop

When the client gets a 404 back we know the container does not exists,
if ignore is set as well we should just ignore the error client side.

seen in #23554

Does this PR introduce a user-facing change?

Fixed a bug that causes podman-remote --cidfile doesnotexists --ignore to stop all containers.

@Luap99
Copy link
Member Author

Luap99 commented Aug 12, 2024

@edsantiago PTAL

Copy link
Contributor

openshift-ci bot commented Aug 12, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Luap99

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 12, 2024
Copy link

We were not able to find or create Copr project packit/containers-podman-23581 specified in the config with the following error:

Packit received HTTP 500 Internal Server Error from Copr Service. Check the Copr status page: https://copr.fedorainfracloud.org/status/stats/, or ask for help in Fedora Build System matrix channel https://matrix.to/#/#buildsys:fedoraproject.org.

Unless the HTTP status code above is >= 500, please check your configuration for:

  1. typos in owner and project name (groups need to be prefixed with @)
  2. whether the project name doesn't contain not allowed characters (only letters, digits, underscores, dashes and dots must be used)
  3. whether the project itself exists (Packit creates projects only in its own namespace)
  4. whether Packit is allowed to build in your Copr project
  5. whether your Copr project/group is not private

@edsantiago
Copy link
Member

LGTM but I'm still seeing the error when I test on my laptop?

$ hack/bats -T --rootless --remote --tag='ci:parallel' 050
--------------------------------------------------                                                                                                              
$ bats -T --jobs 12 --filter-tags ci:parallel test/system/050-stop.bats                                                                                         
050-stop.bats                                                                                                                                                   
 ✓ |050| podman stop - basic test [10566]                                                                                                                       
 ✗ |050| podman stop --ignore [10459]                                                                                                                           
   tags: ci:parallel                                                                                                                                            
   (from function `bail-now' in file test/system/helpers.bash, line 189,                                                                                        
    from function `is' in file test/system/helpers.bash, line 1103,                                                                                             
    in test file test/system/050-stop.bats, line 116)                                                                                                           
     `is "$output" "" "podman stop with missing cidfile, with --ignore"' failed                                                                                 
                                                                                                                                                                
   [07:47:15.908604373] $ bin/podman-remote stop thiscontainerdoesnotexist                                           
   [07:47:15.934666476] Error: no container with name or ID "thiscontainerdoesnotexist" found: no such container                                                
   [07:47:15.938368935] [ rc=125 (expected) ]                                                                                                                   
                                                                                                                                                                
   [07:47:15.949413567] $ bin/podman-remote stop --ignore thiscontainerdoesnotexist                                  
                                                                                                                                                                
   [07:47:16.061030284] $ bin/podman-remote stop --cidfile=/dev/shm/podman_bats.pek5UO/no-such-file                  
   [07:47:16.085520773] Error: reading CIDFile: open /dev/shm/podman_bats.pek5UO/no-such-file: no such file or directory                                        
   [07:47:16.088495150] [ rc=125 (expected) ]                                                                                                                   
                                                                                                                                                                
   [07:47:16.096670405] $ bin/podman-remote stop --cidfile=/dev/shm/podman_bats.pek5UO/no-such-file --ignore         
   [07:47:26.230647320] e681e1eea6d7de67773a067763c57ae2bc2e9f32d27a0e032cd5e071741fe96e                                                                        
   fa0078372cc6be516d186a4c4f3050206555c00bc895712ca2263249219d40e5                                                                                             
   a0cbb322f6167b28159f520ea1e201341136b2b54305f6cd5c4ddad761298d08                                                                                             
   369a88fece6af6fd78e7989a235292a5c5845e79d0d70258f55da443f43a835d                                                                                             
   #/vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv                                                                                                              
   #|     FAIL: podman stop with missing cidfile, with --ignore                                                                                                 
   #| expected: '[no output]'                                                                                                                                   
   #|   actual: 'e681e1eea6d7de67773a067763c57ae2bc2e9f32d27a0e032cd5e071741fe96e'                                                                              
   #|         > 'fa0078372cc6be516d186a4c4f3050206555c00bc895712ca2263249219d40e5'                                                                              
   #|         > 'a0cbb322f6167b28159f520ea1e201341136b2b54305f6cd5c4ddad761298d08'                                                                              
   #|         > '369a88fece6af6fd78e7989a235292a5c5845e79d0d70258f55da443f43a835d'                                                                              
   #\^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^                                                                                                              
   # [teardown]                                                                                                                                                 
 ✓ |050| podman stop - can trap signal [4178]                                 

@Luap99
Copy link
Member Author

Luap99 commented Aug 12, 2024

@edsantiago Did you pick b8039a0? I don't see in your parallel PR: https://github.com/containers/podman/pull/23275/commits

The second commit fixes a minor issue that is causing the no such container error but the first commit is the important one so we do not stop all containers in that case.

@edsantiago
Copy link
Member

Argh. I missed that this was two commits. I'm sorry; too much at once. With both commits cherrypicked, tests pass, and I've pushed that.

@edsantiago
Copy link
Member

Looks like a regression in filters:

✗ [045] podman start --filter - start only containers that match the filter
...
[08:25:09.889599410] $ /home/esm/src/atomic/2018-02.podman/libpod/bin/podman-remote start --filter restart-policy=on-failure c2_on_failure_G5fPrpdNB3EE2hQ c3
_always_5MP8xyUUCk2nxwV                                                                                                                                         
   [08:25:10.086918173] a52a43b9b6f7e20aa4c99261aad575fa13821009e516d880efac77283ffa7a72                                                                        
   #/vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv                                                                                                              
   #|     FAIL: --filter finds container 2                                                                                                                      
   #| expected: 'c2_on_failure_G5fPrpdNB3EE2hQ'                                                                                                                 
   #|   actual: 'a52a43b9b6f7e20aa4c99261aad575fa13821009e516d880efac77283ffa7a72'                                                                              
   #\^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

@Luap99
Copy link
Member Author

Luap99 commented Aug 12, 2024

Yeah no idea why we even allow that, names and filters at the same time?

@edsantiago
Copy link
Member

I see value in filter + [list of container names]

Luap99 added 2 commits August 12, 2024 17:12
When the cidfile does not exists and ignore is set the cli parser skips
the file without error and we call into the backend code without any
names at all. This should logically be a NOP but on remote it caused all
containers to be returned which caused podman stop to stop everything in
this case.

Fixes containers#23554

Signed-off-by: Paul Holzinger <[email protected]>
When the client gets a 404 back we know the container does not exists,
if ignore is set as well we should just ignore the error client side.

seen in containers#23554

Signed-off-by: Paul Holzinger <[email protected]>
@Luap99
Copy link
Member Author

Luap99 commented Aug 12, 2024

Fixed the code, a bit more checks but now we at at skip the unnecessary list API call for that case.

@edsantiago
Copy link
Member

Passing in #23275 and in my own testing. LGTM, thank you

@rhatdan
Copy link
Member

rhatdan commented Aug 12, 2024

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 12, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 6738405 into containers:main Aug 12, 2024
83 checks passed
@Luap99 Luap99 deleted the remote-ignore branch August 12, 2024 16:15
@stale-locking-app stale-locking-app bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Nov 11, 2024
@stale-locking-app stale-locking-app bot locked as resolved and limited conversation to collaborators Nov 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

race: podman-remote stop --cidfile --ignore does very weird things
3 participants