Skip to content
This repository has been archived by the owner on May 12, 2021. It is now read-only.

shimv2 : Remove workaround for sharedPidNs #2794

Merged

Conversation

amshinde
Copy link
Member

Removing code that existed as a workaround for a bug in
how shared process namespaces were handled in the agent.
That has been long fixed in the agent.
With this, sharedPidNs will now work with shimv2.

Fixes #2788

Signed-off-by: Archana Shinde archana.m.shinde@intel.com

Removing code that existed as a workaround for a bug in
how shared process namespaces were handled in the agent.
That has been long fixed in the agent.
With this, sharedPidNs will now work with shimv2.

Fixes kata-containers#2788

Signed-off-by: Archana Shinde <archana.m.shinde@intel.com>
@auto-comment
Copy link

auto-comment bot commented Jun 25, 2020

Thank you for raising your pull request. Please note that the main development of Kata Containers has moved to the 2.0-dev branch of https://github.com/kata-containers/kata-containers repository. The kata-containers/runtime repository is kept for 1.x release maintenance. Please check twice if your change should go to the 2.0-dev branch directly.

If it is strongly required for adding the change to Kata Containers 1.x releases, please ping @kata-containers/runtime to assign a dedicated developer to be responsible for porting the change to 2.0-dev branch. Thanks!

@amshinde
Copy link
Member Author

@lifupan @bergwolf PTAL

Function removeNamespace is no longer used. Get rid of
it.

Signed-off-by: Archana Shinde <archana.m.shinde@intel.com>
@amshinde amshinde force-pushed the remove-workaround-sharedpid branch from 94c2e25 to e0dc806 Compare June 25, 2020 23:28
@amshinde
Copy link
Member Author

/test

@codecov
Copy link

codecov bot commented Jun 26, 2020

Codecov Report

Merging #2794 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2794      +/-   ##
==========================================
- Coverage   50.61%   50.60%   -0.02%     
==========================================
  Files         118      118              
  Lines       17310    17312       +2     
==========================================
- Hits         8762     8761       -1     
- Misses       7474     7477       +3     
  Partials     1074     1074              

Copy link

@LinShuicheng LinShuicheng left a comment

Choose a reason for hiding this comment

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

LGTM. Verified in my environment, shareProcessNamespace could work with this patch.

@lifupan
Copy link
Member

lifupan commented Jun 28, 2020

It's LGTM to this modification, but I think there is an issue with sharedPidNs in the guest, and the init process shared by the containers wouldn't reap the zombie child process which would cause resources leak.

@amshinde
Copy link
Member Author

there is an issue with sharedPidNs in the guest, and the init process shared by the containers wouldn't reap the zombie child process which would cause resources leak.

Is there as easy way to reproduce this? Do you know what needs to be done to fix this. I can take a look at it.
It would be best if we could address that in a separate issue, can you open an issue for that.
Meanwhile, I think we should merge this patch.
cc @bergwolf

@amshinde amshinde requested review from devimc and jcvenegas June 29, 2020 18:36
@amshinde
Copy link
Member Author

amshinde commented Jul 6, 2020

ping @kata-containers/runtime

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[question] does kata support shareProcessNamespace?
5 participants