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.

Sign up for GitHub

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

Jump to bottom

add tests for container kill and exists #5388

Merged
merged 1 commit into from
Apr 8, 2020
Merged

add tests for container kill and exists #5388

merged 1 commit into from
Apr 8, 2020

Conversation

lsm5
Copy link
Member

@lsm5 lsm5 commented Mar 4, 2020

depends on #5374 . Also, tests are currently failing due to a crun issue being worked on separately.

@openshift-ci-robot openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Mar 4, 2020
@lsm5 lsm5 added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 4, 2020
@rh-atomic-bot
Copy link
Collaborator

☔ The latest upstream changes (presumably #5344) made this pull request unmergeable. Please resolve the merge conflicts.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 4, 2020
@rhatdan
Copy link
Member

rhatdan commented Mar 9, 2020

@lsm5 needs a rebase

@lsm5 lsm5 force-pushed the tests-apiv2-kill-exists branch from 7ca2e4f to 221f47b Compare March 23, 2020 12:19
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 23, 2020
@lsm5 lsm5 changed the title [DO NOT MERGE] add tests for container kill and exists add tests for container kill and exists Mar 23, 2020
@lsm5 lsm5 removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 23, 2020
@lsm5 lsm5 changed the title add tests for container kill and exists [DO NOT MERGE] add tests for container kill and exists Mar 23, 2020
@lsm5 lsm5 force-pushed the tests-apiv2-kill-exists branch from 221f47b to 232f6d5 Compare March 24, 2020 16:37
lsm5
lsm5 commented Mar 24, 2020
View reviewed changes
pkg/bindings/test/containers_test.go Outdated
Expect(err).To(BeNil())
})

// This test panics after a 10 minute timeout.
Copy link
Member Author

Choose a reason for hiding this comment

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

@baude PTAL ^

Copy link
Member Author

Choose a reason for hiding this comment

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

@baude re-ping

@lsm5 lsm5 changed the title [DO NOT MERGE] add tests for container kill and exists add tests for container kill and exists Mar 24, 2020
@rh-atomic-bot
Copy link
Collaborator

☔ The latest upstream changes (presumably #5618) made this pull request unmergeable. Please resolve the merge conflicts.

@lsm5 lsm5 force-pushed the tests-apiv2-kill-exists branch 2 times, most recently from 5eba6b7 to 563468a Compare April 2, 2020 13:30
@lsm5 lsm5 force-pushed the tests-apiv2-kill-exists branch 2 times, most recently from 3143ce0 to 891f999 Compare April 7, 2020 14:12
@lsm5
Copy link
Member Author

lsm5 commented Apr 7, 2020

@baude @mheon all tests passed. PTAL..

@rhatdan
Copy link
Member

rhatdan commented Apr 7, 2020

LGTM
@mheon @baude @vrothberg @giuseppe @edsantiago @cevich PTAL

edsantiago
edsantiago reviewed Apr 7, 2020
View reviewed changes
Copy link
Member

@edsantiago edsantiago left a comment

Choose a reason for hiding this comment

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

Is it really necessary to use SIGKILL so often? I was trained to use that only as a last resort. Maybe you could alternate SIGINT and SIGTERM?

Also, what would you think of adding an Exists() test after one or two of the kill tests? E.g. after line 483? See, the thing is, you're not really testing kill: you're testing "kill does not give me an error". Not the same thing.

@lsm5 lsm5 force-pushed the tests-apiv2-kill-exists branch from 891f999 to 394d08f Compare April 8, 2020 14:43
@lsm5
add tests for kill and exists
ce35fe3 
Signed-off-by: Lokesh Mandvekar <[email protected]>
@lsm5 lsm5 force-pushed the tests-apiv2-kill-exists branch from 394d08f to ce35fe3 Compare April 8, 2020 14:47
@lsm5
Copy link
Member Author

lsm5 commented Apr 8, 2020

Is it really necessary to use SIGKILL so often? I was trained to use that only as a last resort. Maybe you could alternate SIGINT and SIGTERM?

Also, what would you think of adding an Exists() test after one or two of the kill tests? E.g. after line 483? See, the thing is, you're not really testing kill: you're testing "kill does not give me an error". Not the same thing.

@edsantiago addressed both comments. PTAL..

@edsantiago
Copy link
Member

LGTM assuming tests pass. Thank you!

@edsantiago
Copy link
Member

Test failure in rootless local:

Error: error removing container b52d9a50bceff47592a01cde287cf54a3f072570f045998efdcb0ee507388803 root filesystem: 1 error occurred:
 	* unlinkat /home/some18944dude/.local/share/containers/storage/overlay/428c1faaae3530ee1f58f0e7c55f6e45c0eb76580e75abc9bbc08cf2e28bc8d5/merged: device or resource busy

I'm guessing it's a flake but once again it's not one I've seen before. Could someone please investigate?

@edsantiago
Copy link
Member

/lgtm

...but I would really like someone mount-knowledgeable (@mheon? @giuseppe?) to look at the flake above and help me understand how to address it / prevent it in the future (or, if it's a real podman problem, which I kind of think it might be, how to gather more data).

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 8, 2020
@mheon
Copy link
Member

mheon commented Apr 8, 2020

It seems like the mountpoint is still active when we try and remove it. Hard to tell exactly what's going on without having a look at what exactly is mounted when we try and remove - could be overlay failing to unmount, could be our SHM mount?

@edsantiago
Copy link
Member

All of those are such simple tests: podman run X. Unfortunately I don't have a way of knowing which of those containers was the one leaving the mount behind. Would it help if I reworked the test such that it does 'podman rmimmediately after eachrun` command?

@lsm5
Copy link
Member Author

lsm5 commented Apr 8, 2020

@edsantiago everything passes now, so maybe we're good to go?

@edsantiago
Copy link
Member

I gave it my /lgtm blessing but I don't have privs to /approve

@mheon
Copy link
Member

mheon commented Apr 8, 2020

/approve
/lgtm

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lsm5, mheon

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-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 8, 2020
@openshift-merge-robot openshift-merge-robot merged commit f71e4d3 into containers:master Apr 8, 2020
@lsm5 lsm5 deleted the tests-apiv2-kill-exists branch April 8, 2020 19:52
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@edsantiago edsantiago edsantiago left review comments

@mrunalp mrunalp Awaiting requested review from mrunalp

@umohnani8 umohnani8 Awaiting requested review from umohnani8

Assignees

@edsantiago edsantiago

@mheon mheon

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. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@lsm5 @rh-atomic-bot @rhatdan @edsantiago @mheon @openshift-ci-robot @openshift-merge-robot