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

Retain pause.exe as entrypoint for default pause images #1615

Merged
merged 1 commit into from
Jan 26, 2023

Conversation

kiashok
Copy link
Contributor

@kiashok kiashok commented Jan 9, 2023

Currently, the entrypoint for all pause containers is being overriden to ping.exe immaterial of the pause image used. This is causing perf issues even when the default pause image is used.
This PR retains /pause.exe as entrypoint for the default pause image and overrides to ping.exe only if the default arg/commandline of the non-default pause image being used is "c:\windows\system32\cmd.exe" . We intend to completely get rid of the ping override by containerd/1.7 and hence throw a warning log message to switch to the default pause image asap.

@kiashok kiashok requested a review from a team as a code owner January 9, 2023 18:13
@kiashok kiashok force-pushed the pauseImageFix branch 2 times, most recently from 738afec to df289c7 Compare January 9, 2023 19:25
@katiewasnothere
Copy link
Contributor

This PR addresses #1576 and is related to containerd/containerd#7752

Copy link
Contributor

@ambarve ambarve left a comment

Choose a reason for hiding this comment

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

Minor comments, but LGTM otherwise!

@kiashok kiashok force-pushed the pauseImageFix branch 2 times, most recently from f9b6b73 to 5c142d6 Compare January 18, 2023 23:13
Signed-off-by: Kirtana Ashok <Kirtana.Ashok@microsoft.com>
@kiashok
Copy link
Contributor Author

kiashok commented Jan 24, 2023

As suggested by @kevpar, I have also updated the github bug and discussion item opened for this to explicitly call out the fix.
Will attend the containerd community meeting on Thursday (01/26/2023) to see if there are any concerns about the fix before checking it in.

@kiashok
Copy link
Contributor Author

kiashok commented Jan 26, 2023

image

CI / integration-tests (windows-2019) is failing with an unrelated error "2023-01-24T23:12:15.7814225Z === CONT TestCRIImagePullTimeout
2023-01-24T23:12:15.7817308Z image_pull_timeout_test.go:59:
2023-01-24T23:12:15.7817989Z --- SKIP: TestCRIImagePullTimeout (0.00s)
2023-01-24T23:12:15.7818462Z FAIL
2023-01-24T23:12:15.9223217Z mv: cannot remove 'C:/Windows/Temp/test-integration/containerd.log': Device or resource busy
2023-01-24T23:12:15.9534349Z [SC] DeleteService SUCCESS
2023-01-24T23:12:15.9797216Z
2023-01-24T23:12:15.9799461Z SERVICE_NAME: containerd-test
2023-01-24T23:12:15.9806993Z TYPE : 10 WIN32_OWN_PROCESS
2023-01-24T23:12:15.9810141Z STATE : 4 RUNNING
2023-01-24T23:12:15.9866674Z (STOPPABLE, NOT_PAUSABLE, ACCEPTS_SHUTDOWN)
2023-01-24T23:12:15.9868624Z WIN32_EXIT_CODE : 0 (0x0)
2023-01-24T23:12:15.9874067Z SERVICE_EXIT_CODE : 0 (0x0)
2023-01-24T23:12:15.9876002Z CHECKPOINT : 0x0
2023-01-24T23:12:15.9897735Z WAIT_HINT : 0x0
2023-01-24T23:12:15.9959801Z mingw32-make: *** [Makefile:224: cri-integration] Error 1
2023-01-24T23:12:16.0138354Z ##[error]Process completed with exit code 2."

@kiashok kiashok merged commit 6cd5572 into microsoft:main Jan 26, 2023
kiashok added a commit to kiashok/hcsshim that referenced this pull request Jan 30, 2023
Signed-off-by: Kirtana Ashok <Kirtana.Ashok@microsoft.com>

Signed-off-by: Kirtana Ashok <Kirtana.Ashok@microsoft.com>
Co-authored-by: Kirtana Ashok <Kirtana.Ashok@microsoft.com>
(cherry picked from commit 6cd5572)
Signed-off-by: Kirtana Ashok <Kirtana.Ashok@microsoft.com>
kiashok added a commit that referenced this pull request Feb 1, 2023
(cherry picked from commit 6cd5572)

Signed-off-by: Kirtana Ashok <Kirtana.Ashok@microsoft.com>
Co-authored-by: Kirtana Ashok <Kirtana.Ashok@microsoft.com>
@jterry75
Copy link
Contributor

jterry75 commented Apr 5, 2023

@katiewasnothere - I know this is old news, but can we just have MS provide "halt.exe" in the root nanoserver+ compatibility sets. I feel like we have been talking about this for ages and ages lol. Here is what I want to do:

docker (or containerd) run --detach nanoserver halt

I dont want stdin/stdout to cause the reader to block. I just want a halted process that never exits until sent sigterm/kill (ctrl+c or exit) and then exits. This should be inside the containers by default IMO, and it cant take more than a few kb of space being such a small program. Can we forward on to Windows PM to see if this can be done? We shouldnt need ping to hold a container alive.. :). Ty!!

@brasmith-ms
Copy link

Hey @jterry75 what would the benefit of this be over pause.exe? If you're trying to hold a container alive this can be done from cmd or at the application level, but I don't see the benefit of holding an empty container idle? Maybe I'm missing context here.

@jterry75
Copy link
Contributor

jterry75 commented Apr 10, 2023

Its possible we are way out of the loop, but pause.exe requires that the stdin pipe be valid. Meaning you cannot run a detached instance for the process to stay alive. The reason why the pause container runs ping is to have a detached process they never exits in the container pid namespace. Thus because that pid is always alive, the shim considers the container as active. But this has adverse effect of running actual cpu runtime. It would be nice if there was a way from nanoserver up to just generically say "run this container process which consumes no cpu but hangs until ctrl+c or ctrl+exit is sent"

Kubernetes has to distribute a custom "pause" container to do just this. They wrote a pause.exe process themselves that does the hanging here because Windows does not have an in-built way to do that. There is no reason this should not be part of the base image IMO

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.

6 participants