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

podman: allow usage of gVisor as OCI runtime #1570

Merged
merged 2 commits into from
Oct 4, 2018
Merged

podman: allow usage of gVisor as OCI runtime #1570

merged 2 commits into from
Oct 4, 2018

Conversation

giuseppe
Copy link
Member

@giuseppe giuseppe commented Oct 1, 2018

fix a couple of issues that prevented the usage of gVisor as runtime

  • Always set XDG_RUNTIME_DIR
  • read the $RUNTIME state output from stdout not stdout+stderr

@giuseppe
Copy link
Member Author

giuseppe commented Oct 1, 2018

/cc @rhatdan

@vrothberg
Copy link
Member

I think that having a package-internal variable with []string{fmt.Sprintf("XDG_RUNTIME_DIR=%s", runtimeDir)} is more maintainable than setting it for each call (but it's not a blocker).
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 1, 2018
@rhatdan
Copy link
Member

rhatdan commented Oct 1, 2018

/approve

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhatdan

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 Oct 1, 2018
rhatdan
rhatdan reviewed Oct 1, 2018
View reviewed changes
libpod/oci.go Outdated
@@ -453,7 +453,7 @@ func (r *OCIRuntime) updateContainerStatus(ctr *Container) error {
cmd := exec.Command(r.path, "state", ctr.ID())
cmd.Env = append(cmd.Env, fmt.Sprintf("XDG_RUNTIME_DIR=%s", runtimeDir))

out, err := cmd.CombinedOutput()
out, err := cmd.Output()
Copy link
Member

Choose a reason for hiding this comment

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

Will the error message show up?

@mheon
Copy link
Member

mheon commented Oct 1, 2018

/lgtm cancel

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Oct 1, 2018
@mheon
Copy link
Member

mheon commented Oct 1, 2018

NACK as is - we need to retain the error message from the runtime. In the runc case, checking STDERR for "container does not exist" allows us to fix bad bugs where Podman's state says a container is running but it actually doesn't exist in the runtime.

@mheon
Copy link
Member

mheon commented Oct 1, 2018

@vrothberg The bot actually doesn't support more than one LGTM, so mostly we're throwing old-fashioned LGTMs on and then doing a /lgtm when we have 2 or more and are good to merge

@vrothberg
Copy link
Member

@vrothberg The bot actually doesn't support more than one LGTM, so mostly we're throwing old-fashioned LGTMs on and then doing a /lgtm when we have 2 or more and are good to merge

Thanks, @mheon, I will keep that in mind :)

@TomSweeneyRedHat
Copy link
Member

LGTM, assuming happy tests.

@giuseppe
Copy link
Member Author

giuseppe commented Oct 1, 2018

NACK as is - we need to retain the error message from the runtime. In the runc case, checking STDERR for "container does not exist" allows us to fix bad bugs where Podman's state says a container is running but it actually doesn't exist in the runtime.

I've pushed a new version where I use two different pipes instead of their combined output.

mheon
mheon reviewed Oct 1, 2018
View reviewed changes
utils/utils.go Outdated
@@ -29,11 +29,12 @@ func ExecCmd(name string, args ...string) (string, error) {
}

// ExecCmdWithStdStreams execute a command with the specified standard streams.
func ExecCmdWithStdStreams(stdin io.Reader, stdout, stderr io.Writer, name string, args ...string) error {
func ExecCmdWithStdStreams(env []string, stdin io.Reader, stdout, stderr io.Writer, name string, args ...string) error {
Copy link
Member

Choose a reason for hiding this comment

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

@baude - Didn't you make this change in another of your PRs? Might want to hash this out to try and avoid breaking each other

@mheon
Copy link
Member

mheon commented Oct 1, 2018

@giuseppe Thanks for the changes looks fine now!

@giuseppe giuseppe force-pushed the fix-gvisor branch 2 times, most recently from f791437 to e09a86b Compare October 2, 2018 19:24
@rh-atomic-bot
Copy link
Collaborator

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

@giuseppe
oci: always set XDG_RUNTIME_DIR
c21e85e 
Fix an issue when using gVisor that couldn't start the container since
the XDG_RUNTIME_DIR env variable used for the "create" and "start"
commands is different.  Set the environment variable for each command
so that the OCI runtime gets always the same value.

Signed-off-by: Giuseppe Scrivano <[email protected]>
@giuseppe
oci: split the stdout and stderr pipes
c554672 
read the OCI status from stdout, not the combined stdout+stderr
stream.

Signed-off-by: Giuseppe Scrivano <[email protected]>
@rhatdan
Copy link
Member

rhatdan commented Oct 4, 2018

/retest

@AkihiroSuda
Copy link
Collaborator

did you test gvisor with rootless?

@giuseppe
Copy link
Member Author

giuseppe commented Oct 4, 2018

did you test gvisor with rootless?

I did a quick test but it seems to fail even with a simple test case

@giuseppe
Copy link
Member Author

giuseppe commented Oct 4, 2018

tests are finally passing

@mheon
Copy link
Member

mheon commented Oct 4, 2018

LGTM

@TomSweeneyRedHat
Copy link
Member

I'd love to see a blog next week posted to podman.io talking about this and showing an asciinema demo...

@rhatdan
Copy link
Member

rhatdan commented Oct 4, 2018

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 4, 2018
@openshift-merge-robot openshift-merge-robot merged commit 094b8b7 into containers:master Oct 4, 2018
@vrothberg
Copy link
Member

I'd love to see a blog next week posted to podman.io talking about this and showing an asciinema demo...

💯 percent agreed with @TomSweeneyRedHat! That's a really nice showcase and will attract attention.

@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 27, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mheon mheon mheon left review comments

@rhatdan rhatdan rhatdan left review comments

@baude baude Awaiting requested review from baude

Assignees

@rhatdan rhatdan

@mheon mheon

@vrothberg vrothberg

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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@giuseppe @vrothberg @rhatdan @openshift-ci-robot @mheon @TomSweeneyRedHat @rh-atomic-bot @AkihiroSuda @openshift-merge-robot