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

Remove pause containers for process isolated containers #1973

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kiashok
Copy link
Contributor

@kiashok kiashok commented Nov 29, 2023

PR does the following:

  • Introduces new HostComputeNamespace.ReadyOnCreate field and sets it for HNS versions that support pause container removal
  • Removes pause container creation while creating process isolated pods for HNS versions that support removal of pause containers

The changes have been manually tested with the supported HNS versions.

@kiashok kiashok requested a review from a team as a code owner November 29, 2023 18:50
@kevpar
Copy link
Member

kevpar commented Nov 29, 2023

This seems like it will break port forwarding in containerd, as it depends on execing wincat in the pause container (code). Is there a plan for addressing that?

@kiashok
Copy link
Contributor Author

kiashok commented Nov 29, 2023

This seems like it will break port forwarding in containerd, as it depends on execing wincat in the pause container (code). Is there a plan for addressing that?

yes I already have changes for the same. Waiting to complete the k8s e2e local testing with the supported HNS version. Plan to send out the PR for this early next week

@kiashok kiashok marked this pull request as draft January 17, 2024 17:49
@kiashok kiashok marked this pull request as ready for review March 27, 2024 17:14
@kiashok
Copy link
Contributor Author

kiashok commented Mar 27, 2024

This seems like it will break port forwarding in containerd, as it depends on execing wincat in the pause container (code). Is there a plan for addressing that?

yes I already have changes for the same. Waiting to complete the k8s e2e local testing with the supported HNS version. Plan to send out the PR for this early next week

port forwarding for windows was checked in yesterday containerd/containerd@b97ef91 . We should now be good to remove pause containers when supported.

@kiashok
Copy link
Contributor Author

kiashok commented Mar 27, 2024

@kevpar @ambarve could you please take a look when you have some time? Thanks!

hcn/hcnnamespace.go Outdated Show resolved Hide resolved
hcn/hcnnamespace.go Outdated Show resolved Hide resolved
This commit does the following:
- Introduces new HostComputeNamespace.ReadyOnCreate field and set it
for HNS versions that support pause container removal
- Remove pause container creation while creating process
isolated pods for HNS versions that support pause container creation

Signed-off-by: Kirtana Ashok <kiashok@microsoft.com>
@kevpar
Copy link
Member

kevpar commented Apr 24, 2024

Is the assumption that the shim version with this change will only be used with a containerd version that also supports pause container removal? Is that something we can rely on?

If not, do we need some way to configure that pause containers should/shouldn't be used?

@kevpar
Copy link
Member

kevpar commented Apr 24, 2024

We should have someone from Sravanth's team review for the hcn change. Other than that and my other comment, this looks good.

@kiashok
Copy link
Contributor Author

kiashok commented Apr 24, 2024

We should have someone from Sravanth's team review for the hcn change. Other than that and my other comment, this looks good.

I think Sravanth has already taken a look at these changes. @sbangari can you confirm please? Thanks!

@kiashok
Copy link
Contributor Author

kiashok commented Apr 24, 2024

Is the assumption that the shim version with this change will only be used with a containerd version that also supports pause container removal? Is that something we can rely on?

If not, do we need some way to configure that pause containers should/shouldn't be used?

On upstream, containerd/2.0 (currently the main branch) is the only one that will support pause container removal as port forwarding feature changes cannot be backported to release branches. Once this PR is merged, we should cherry-pick to hcsshim/release/0.12 branch and vendor in the changes to containerd/main.
Windows port forwarding is the only thing on containerd that depends on pause container containers and the pause image as of today.

@kiashok kiashok marked this pull request as draft April 24, 2024 17:31
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.

None yet

3 participants