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

use conmon for exec #3143

Merged
merged 2 commits into from
Jul 22, 2019
Merged

use conmon for exec #3143

merged 2 commits into from
Jul 22, 2019

Conversation

haircommander
Copy link
Collaborator

@haircommander haircommander commented May 16, 2019 •
edited
Loading

This is a starter PR. Do not merge, it is not complete at all.
depends on: cri-o/cri-o#2377

The point of pushing here is mostly to get feedback on the design. there is still more work to do:

  • --user and --workdir don't currently work and I'm not sure why
  • a small tty bug
  • all the TODO FIXMES

The following items could be added in follow up PRs:

  • currently, exec sessions create an attach socket similarly to how creates create one, but with sessionID instead of containerID. Right now, there's no global tracking of sessionID, so there exists the opportunity (1/2^128) of there being a duplicate. Are we comfortable with this?
  • I'd like to query the version of conmon in the runtime to make sure we won't crash in a confusing manner. I need to figure out the best way to require this while also prohibiting the conmon versions built from the CRI-O repo that are tagged as 1.9 - 1.14
  • add exit codes to the define package
  • actually implement remote exec

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 16, 2019
@openshift-ci-robot openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label May 16, 2019
@haircommander
Copy link
Collaborator Author

@mheon @baude @jwhonce PTAL

@haircommander haircommander force-pushed the conmon-exec branch 3 times, most recently from b345fa2 to 5b1f1a7 Compare May 17, 2019 14:51
jwhonce
jwhonce reviewed May 17, 2019
View reviewed changes
libpod/container_internal.go Show resolved Hide resolved
libpod/oci_internal_linux.go Show resolved Hide resolved
libpod/oci_internal_linux.go Outdated Show resolved Hide resolved
@rh-atomic-bot
Copy link
Collaborator

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

@haircommander haircommander force-pushed the conmon-exec branch 6 times, most recently from a6d124e to 9d5bc14 Compare May 17, 2019 22:41
@rh-atomic-bot
Copy link
Collaborator

☔ The latest upstream changes (presumably #3162) 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 May 20, 2019
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 23, 2019
@haircommander haircommander force-pushed the conmon-exec branch 6 times, most recently from ab50020 to 15c83ab Compare May 25, 2019 02:10
@haircommander haircommander force-pushed the conmon-exec branch 3 times, most recently from 9ceab1b to adee9d8 Compare May 28, 2019 14:49
@haircommander
Copy link
Collaborator Author

@mheon @TomSweeneyRedHat @QiWang19 @vrothberg @baude @rhatdan Here's an update:
We are passing all the tests this PR will pass. It will need to be force merged, as the in_podman images aren't updated until an hour or so after PRs are merged, and this PR needs to be merged for that to happen. that may mean the tests will be annoyed for a bit after merging this, so be wary of timing (cc: @cevich)

This PR is as ready as it will be, PTAL

@haircommander
Copy link
Collaborator Author

update, I actually just thought of a way to get this to pass organically, so hold on the manual merge

@haircommander
Copy link
Collaborator Author

update, i think I have a way to make in_podman green naturally. hold the force merge

@haircommander haircommander mentioned this pull request Jul 22, 2019
Merged
cevich
cevich approved these changes Jul 22, 2019
View reviewed changes
Copy link
Member

@cevich cevich left a comment

Choose a reason for hiding this comment

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

All the CI changes LGTM, just one goof but it's my fault and won't affect anything.

.cirrus.yml Outdated
@@ -43,7 +43,7 @@ env:
# Git commits to use while building dependencies into cache-images
FEDORA_CNI_COMMIT: "412b6d31280682bb4fab4446f113c22ff1886554"
CNI_COMMIT: "7480240de9749f9a0a5c8614b17f1f03e0c06ab9"
CONMON_COMMIT: "3e47d8dd45cdd973dbe256292d5e9c0bff195e56"
CONMON_COMMIT: "6f3572558b97bc60dd8f8c7f0807748e6ce2c440"
CRIU_COMMIT: "c74b83cd49c00589c0c0468ba5fe685b67fdbd0a"
Copy link
Member

Choose a reason for hiding this comment

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

FYI- this is my bad...these aren't used anywhere. I've got a commit floating on a PR somewhere that just clobbers them all.

TomSweeneyRedHat
TomSweeneyRedHat reviewed Jul 22, 2019
View reviewed changes
pkg/varlinkapi/containers.go
fmt.Printf("ExecContainer Start ctr.Exec\n")
// TODO detach keys and resize
// TODO add handling for exit code
// TODO capture exit code and return to main thread
Copy link
Member

Choose a reason for hiding this comment

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

Do we have Jira cards or issues for these?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the entirety of remote exec is a big TODO, I can make a jira card

@TomSweeneyRedHat
Copy link
Member

Changes LGTM, but what head nods from @mheon and @vrothberg too, especially if you can't get the tests to green.

Personally, I think once merged, this should sit in upstream for a few weeks before we make a new kit with it and that kit should bump the version from 1.4 to 1.5 or 1.5 to 1.6.

@haircommander
Implement conmon exec
a1a79c0 
This includes:
	Implement exec -i and fix some typos in description of -i docs
	pass failed runtime status to caller
	Add resize handling for a terminal connection
	Customize exec systemd-cgroup slice
	fix healthcheck
	fix top
	add --detach-keys
	Implement podman-remote exec (jhonce)
	* Cleanup some orphaned code (jhonce)
	adapt remote exec for conmon exec (pehunt)
	Fix healthcheck and exec to match docs
		Introduce two new OCIRuntime errors to more comprehensively describe situations in which the runtime can error
		Use these different errors in branching for exit code in healthcheck and exec
	Set conmon to use new api version

Signed-off-by: Jhon Honce <[email protected]>

Signed-off-by: Peter Hunt <[email protected]>
@haircommander
bump cirrus images to get new conmon
53e1ede 
Signed-off-by: Peter Hunt <[email protected]>
@mheon
Copy link
Member

mheon commented Jul 22, 2019

/lgtm

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

mheon commented Jul 22, 2019

/approve

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cevich, haircommander, 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 Jul 22, 2019
@haircommander
Copy link
Collaborator Author

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 22, 2019
@openshift-merge-robot openshift-merge-robot merged commit a12a231 into containers:master Jul 22, 2019
This was referenced Jul 22, 2019
Closed
Merged
Merged
Merged
@mheon
Copy link
Member

mheon commented Jul 23, 2019

Thanks for sticking with this one through the endless rebases, @haircommander - it's finally merged.

@debarshiray
Copy link
Member

This also fixed #3179

@debarshiray debarshiray mentioned this pull request Sep 10, 2019
Closed
@kolyshkin kolyshkin mentioned this pull request Jan 18, 2021
Closed
@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 26, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@TomSweeneyRedHat TomSweeneyRedHat TomSweeneyRedHat left review comments

@vrothberg vrothberg vrothberg left review comments

@mheon mheon mheon left review comments

@jwhonce jwhonce jwhonce left review comments

@giuseppe giuseppe giuseppe left review comments

@cevich cevich cevich approved these changes

@mrunalp mrunalp Awaiting requested review from mrunalp

@umohnani8 umohnani8 Awaiting requested review from umohnani8

Assignees

@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.