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

Exec support using libpod api #87

Merged
merged 3 commits into from
Apr 2, 2021
Merged

Exec support using libpod api #87

merged 3 commits into from
Apr 2, 2021

Conversation

towe75
Copy link
Collaborator

@towe75 towe75 commented Jan 30, 2021

Alternative implementation to #85
It uses libpod api directly, following the minimal client strategy of our 0.2.0 release.

It's pretty complete and support both interactive use and scripted health checks. The only known issue is related to terminal resize events due to containers/podman#7102 which should be fixed in the upcoming podman 3.0 release.

@towe75
Copy link
Collaborator Author

towe75 commented Jan 30, 2021

@drewbailey PTAL. I think it's low risk to merge because it adds a totally new feature without changing any of the existing functions.

api/exec_create.go Outdated Show resolved Hide resolved
api/exec_create.go Outdated Show resolved Hide resolved
api/exec_start.go Outdated Show resolved Hide resolved
api/exec_start.go Outdated Show resolved Hide resolved
api/exec_start.go Outdated Show resolved Hide resolved
driver.go Outdated Show resolved Hide resolved
Copy link
Contributor

@drewbailey drewbailey left a comment

Choose a reason for hiding this comment

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

I tried this out locally and it works great! Excellent work 🎉 Just a few comments, Some tests would be good too

Base automatically changed from master to main March 3, 2021 20:39
@towe75
Copy link
Collaborator Author

towe75 commented Mar 21, 2021

@drewbailey : i rebased this branch to include the latest changes from main. Please check my earlier reply on your review.

Are we good to go? I think it would be worth a release.

@drewbailey drewbailey closed this Mar 22, 2021
@drewbailey drewbailey reopened this Mar 22, 2021
for {
select {
case <-ctx.Done():
c.logger.Warn("Resize handler is done")
Copy link
Contributor

@drewbailey drewbailey Mar 22, 2021

Choose a reason for hiding this comment

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

I'm not sure if an operator would find this something that they need to act upon or be notified about, Trace seems better

Suggested change
c.logger.Warn("Resize handler is done")
c.logger.Trace("Resize handler is done")

api/exec_start.go Outdated Show resolved Hide resolved
@drewbailey
Copy link
Contributor

@towe75 this looks really great! A few tests would be great to ensure everything works as expected as new podman versions roll out. There should be a few good examples in the Nomad repo

@towe75
Copy link
Collaborator Author

towe75 commented Mar 22, 2021

@drewbailey : thank you for feedback. I'll try to add some tests.

@drewbailey
Copy link
Contributor

@towe75 We can also merge this and do a follow up PR with some tests before releasing if you prefer, I'll approve the PR and leave it up to you. If we can fix those two log levels that would be great

@towe75
Copy link
Collaborator Author

towe75 commented Mar 23, 2021

@drewbailey i started to derive a test from the other task driver and only two sub-tests from StreamingConformingTests fail:

--- FAIL: TestPodmanDriver_execTaskStreaming (76.20s)
--- PASS: TestPodmanDriver_execTaskStreaming/basic:_notty:_basic (0.13s)
--- PASS: TestPodmanDriver_execTaskStreaming/basic:_notty:_streaming (3.12s)
--- PASS: TestPodmanDriver_execTaskStreaming/basic:_notty:_stty_check (0.11s)
--- PASS: TestPodmanDriver_execTaskStreaming/basic:_notty:_stdin_passing (0.12s)
--- FAIL: TestPodmanDriver_execTaskStreaming/basic:_notty:_children_processes (1.09s)
--- PASS: TestPodmanDriver_execTaskStreaming/basic:_tty:_basic (0.13s)
--- PASS: TestPodmanDriver_execTaskStreaming/basic:_tty:_streaming (3.07s)
--- PASS: TestPodmanDriver_execTaskStreaming/basic:_tty:_stty_check (1.10s)
--- PASS: TestPodmanDriver_execTaskStreaming/basic:_tty:_stdin_passing (0.11s)
--- PASS: TestPodmanDriver_execTaskStreaming/basic:_tty:_children_processes (1.10s)
--- FAIL: TestPodmanDriver_execTaskStreaming/isolation (0.28s)

notty:children_processes has to be tackled, could be potential bug. But "isolation" test has a hardcoded docker assumption in exec_testing.go . Should this become a bug in the nomad repo?

I did not commit the test yet to not break the build.

@drewbailey
Copy link
Contributor

@towe75 that is great news! sounds like TestExecFSIsolation should pass in the expected freezer cgroup instead of assuming nomad or docker

@towe75
Copy link
Collaborator Author

towe75 commented Mar 23, 2021

I looked into the other failing test and it seems to be a podman problem OR wrong assumption.
Here is what i tried:

podman run -d -t busybox --name tester

podman exec tester /bin/sh -c "(( sleep 3; echo from background ) & ); echo from main; exec sleep 1"
from main

A plain run without a container acts differently:

/bin/sh -c "(( sleep 3; echo from background ) & ); echo from main; exec sleep 1"
from main
from background

Should i raise a podman issue? Seems they handle it differently compared to docker.

@drewbailey
Copy link
Contributor

@towe75 fyi we are removing this test hashicorp/nomad#10291 from nomad

@towe75
Copy link
Collaborator Author

towe75 commented Apr 2, 2021

@drewbailey thank you for keeping me posted. I will file a follow-up issue to add the tests later because there is still the TestExecFSIsolation blocker.
After that we can merge this PR, IMHO.

@towe75 towe75 merged commit d3c2186 into main Apr 2, 2021
@towe75 towe75 deleted the tw_exec branch April 2, 2021 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants