Skip to content

Remove pause containers for process isolated containers - #1973

Draft
Kirtana Ashok (kiashok) wants to merge 1 commit into
microsoft:mainfrom
kiashok:pauseContainerRemoval-main
Draft

Remove pause containers for process isolated containers#1973
Kirtana Ashok (kiashok) wants to merge 1 commit into
microsoft:mainfrom
kiashok:pauseContainerRemoval-main

Conversation

@kiashok

@kiashok Kirtana Ashok (kiashok) commented Nov 29, 2023

Copy link
Copy Markdown
Contributor

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
Kirtana Ashok (kiashok) requested a review from a team as a code owner November 29, 2023 18:50
@kevpar

Kevin Parsons (kevpar) commented Nov 29, 2023

Copy link
Copy Markdown
Member

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

Kirtana Ashok (kiashok) commented Nov 29, 2023

Copy link
Copy Markdown
Contributor Author

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
Kirtana Ashok (kiashok) marked this pull request as draft January 17, 2024 17:49
@kiashok
Kirtana Ashok (kiashok) marked this pull request as ready for review March 27, 2024 17:14
@kiashok

Kirtana Ashok (kiashok) commented Mar 27, 2024

Copy link
Copy Markdown
Contributor Author

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

Kirtana Ashok (kiashok) commented Mar 27, 2024

Copy link
Copy Markdown
Contributor Author

Kevin Parsons (@kevpar) Amit Barve (@ambarve) could you please take a look when you have some time? Thanks!

ambarve
Amit Barve (ambarve) reviewed Apr 2, 2024
View reviewed changes
Comment thread cmd/containerd-shim-runhcs-v1/pod.go Outdated
ambarve
Amit Barve (ambarve) reviewed Apr 2, 2024
View reviewed changes
Comment thread hcn/hcnnamespace.go Outdated
kevpar
Kevin Parsons (kevpar) reviewed Apr 4, 2024
View reviewed changes
Comment thread hcn/hcnnamespace.go Outdated
kevpar
Kevin Parsons (kevpar) reviewed Apr 4, 2024
View reviewed changes
Comment thread hcn/hcnnamespace.go
ambarve
Amit Barve (ambarve) reviewed Apr 16, 2024
View reviewed changes
Comment thread cmd/containerd-shim-runhcs-v1/pod.go Outdated
@kiashok
Remove pause container creation for process isolated containers
a527acc 
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

Kevin Parsons (kevpar) commented Apr 24, 2024

Copy link
Copy Markdown
Member

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

Kevin Parsons (kevpar) commented Apr 24, 2024

Copy link
Copy Markdown
Member

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

@kiashok

Kirtana Ashok (kiashok) commented Apr 24, 2024

Copy link
Copy Markdown
Contributor Author

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

Kirtana Ashok (kiashok) commented Apr 24, 2024

Copy link
Copy Markdown
Contributor Author

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
Kirtana Ashok (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

Reviewers

2 more reviewers
@kevpar Kevin Parsons (kevpar) kevpar left review comments
@ambarve Amit Barve (ambarve) ambarve approved these changes
Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

@kevpar Kevin Parsons (kevpar)

@ambarve Amit Barve (ambarve)

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@kiashok @kevpar @ambarve