Allow CRI-O to manage net, IPC and UTS namespaces using pinns #3048

haircommander · 2019-12-13T18:59:14Z

This includes:
change the options ManageNetworkNSLifecycle to ManageNSLifecycle
add a new set of files: internal/lib/sandbox/namespaces* that takes care of namespace related functionality
create a generic NamespaceIface interface for interacting with all three kinds of namespaces
refactor some of runPodSandbox to reduce cyclomatic complexity
use pinns for managing

haircommander · 2019-12-13T21:09:51Z

internal/lib/sandbox/sandbox_test.go

@@ -242,193 +242,4 @@ var _ = t.Describe("Sandbox", func() {
 			Expect(err).NotTo(BeNil())
 		})
 	})
-


TODO: update these tests

codecov · 2019-12-13T21:23:28Z

Codecov Report

Merging #3048 into master will decrease coverage by 0.69%.
The diff coverage is 38.24%.

@@            Coverage Diff            @@
##           master    #3048     +/-   ##
=========================================
- Coverage   46.35%   45.66%   -0.7%     
=========================================
  Files          89       90      +1     
  Lines        7346     7363     +17     
=========================================
- Hits         3405     3362     -43     
- Misses       3639     3710     +71     
+ Partials      302      291     -11

Signed-off-by: Peter Hunt <pehunt@redhat.com>

saschagrunert

LGTM

openshift-ci-robot · 2019-12-20T07:49:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: haircommander, saschagrunert

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [saschagrunert]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot · 2019-12-20T07:49:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: haircommander, saschagrunert

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [saschagrunert]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

haircommander · 2019-12-20T14:53:40Z

@giuseppe PTAL

haircommander · 2019-12-20T15:00:37Z

/hold
missed some cases

giuseppe · 2019-12-20T15:19:07Z

LGTM

Signed-off-by: Peter Hunt <pehunt@redhat.com>

rhatdan · 2019-12-20T16:34:30Z

/lgtm

as well as refactor *NsJoin Signed-off-by: Peter Hunt <pehunt@redhat.com>

instead of using the infra pid unconditionally Signed-off-by: Peter Hunt <pehunt@redhat.com>

haircommander · 2019-12-20T16:55:05Z

Added a couple more changes, as well as a TODO note that can be addressed here or in another PR.
/hold cancel

haircommander · 2019-12-20T19:11:44Z

/retest

saschagrunert · 2019-12-20T20:50:23Z

Hm, still
/lgtm
:)

haircommander · 2019-12-21T04:13:17Z

/retest

saschagrunert · 2020-01-08T14:00:01Z

internal/lib/container_server.go

+			// If we can't load the IPC namespace
+			// because it's closed, we just set the sb ipcns
+			// pointer to nil. Otherwise we return an error.
+			if nsErr != nil && nsErr != sandbox.ErrClosedNS {
+				return nsErr
+			}


@haircommander is there any use-case behind the check for closed namespaces? I'm not sure if we really need this. If the namespace is closed I assume that something went wrong and mark the sandbox as not-ready that it gets properly re-created.

hm good question. to be honest I just copied this from the old net ns managing code. There are fewer calls to close now though.

This might be the root cause that we tear down the local loopback interface when the namespace is not correctly set-up (referring to openshift/containernetworking-plugins#15 and https://bugzilla.redhat.com/show_bug.cgi?id=1754154#c11).

Ref: #3086

haircommander requested review from mrunalp and runcom as code owners December 13, 2019 18:59

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Dec 13, 2019

openshift-ci-robot requested review from saschagrunert and vrothberg December 13, 2019 18:59

openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Dec 13, 2019

openshift-ci-robot requested a review from nalind December 13, 2019 19:00

haircommander mentioned this pull request Dec 13, 2019

Allow CRI-O to manage IPC and UTS namespaces #2918

Closed

haircommander force-pushed the manage-ns-pinns branch 2 times, most recently from 6ef1bf2 to 937a9ee Compare December 13, 2019 19:59

haircommander changed the title ~~WIP Allow CRI-O to manage net, IPC and UTS namespaces using pinns~~ Allow CRI-O to manage net, IPC and UTS namespaces using pinns Dec 13, 2019

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 13, 2019

haircommander changed the title ~~Allow CRI-O to manage net, IPC and UTS namespaces using pinns~~ WIP Allow CRI-O to manage net, IPC and UTS namespaces using pinns Dec 13, 2019

haircommander commented Dec 13, 2019

View reviewed changes

haircommander force-pushed the manage-ns-pinns branch from 613b50a to 1f67ccb Compare December 13, 2019 21:10

haircommander force-pushed the manage-ns-pinns branch from 7a1a86d to 4965f03 Compare December 14, 2019 01:42

openshift-ci-robot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. and removed dco-signoff: no Indicates the PR's author has not DCO signed all their commits. labels Dec 14, 2019

haircommander force-pushed the manage-ns-pinns branch from 4965f03 to c8a32d3 Compare December 14, 2019 01:44

haircommander added the release-1.17 label Dec 16, 2019

haircommander force-pushed the manage-ns-pinns branch from c8a32d3 to 598ee9c Compare December 16, 2019 17:29

openshift-ci-robot added dco-signoff: no Indicates the PR's author has not DCO signed all their commits. and removed dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Dec 16, 2019

manage ns: Add unit tests

63f4d04

Signed-off-by: Peter Hunt <pehunt@redhat.com>

haircommander force-pushed the manage-ns-pinns branch from 7e2ecae to 63f4d04 Compare December 20, 2019 03:04

saschagrunert approved these changes Dec 20, 2019

View reviewed changes

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 20, 2019

remove container level NetNsPath()

db9236b

Signed-off-by: Peter Hunt <pehunt@redhat.com>

openshift-ci-robot assigned rhatdan Dec 20, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 20, 2019

haircommander added 2 commits December 20, 2019 11:34

remove *Ns functions, as they're unneeded

8f88d53

as well as refactor *NsJoin Signed-off-by: Peter Hunt <pehunt@redhat.com>

Use sandbox managed namespaces in container create call

69cf730

instead of using the infra pid unconditionally Signed-off-by: Peter Hunt <pehunt@redhat.com>

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Dec 20, 2019

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 20, 2019

openshift-ci-robot assigned saschagrunert Dec 20, 2019

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. labels Dec 20, 2019

openshift-merge-robot merged commit d7b886c into cri-o:master Dec 21, 2019

saschagrunert reviewed Jan 8, 2020

View reviewed changes

sysrich mentioned this pull request Jan 10, 2020

[1.16] Don't check for closed namespaces #3097

Merged

openshift-ci-robot mentioned this pull request Apr 2, 2020

Release 1.16 centos install #3512

Closed

PaulFurtado mentioned this pull request Mar 30, 2021

CNI: loopback adapter gets state set to down due to PID recycling kubernetes/kubernetes#100648

Closed

mikebrow mentioned this pull request Jun 21, 2021

CRI PID Recycling containerd/containerd#5630

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow CRI-O to manage net, IPC and UTS namespaces using pinns #3048

Allow CRI-O to manage net, IPC and UTS namespaces using pinns #3048

haircommander commented Dec 13, 2019

haircommander Dec 13, 2019

codecov bot commented Dec 13, 2019 •

edited

saschagrunert left a comment •

edited

openshift-ci-robot commented Dec 20, 2019

openshift-ci-robot commented Dec 20, 2019

haircommander commented Dec 20, 2019

haircommander commented Dec 20, 2019

giuseppe commented Dec 20, 2019

rhatdan commented Dec 20, 2019

haircommander commented Dec 20, 2019

haircommander commented Dec 20, 2019

saschagrunert commented Dec 20, 2019

haircommander commented Dec 21, 2019

saschagrunert Jan 8, 2020

haircommander Jan 8, 2020

saschagrunert Jan 9, 2020

Allow CRI-O to manage net, IPC and UTS namespaces using pinns #3048

Allow CRI-O to manage net, IPC and UTS namespaces using pinns #3048

Conversation

haircommander commented Dec 13, 2019

haircommander Dec 13, 2019

Choose a reason for hiding this comment

codecov bot commented Dec 13, 2019 • edited

Codecov Report

saschagrunert left a comment • edited

Choose a reason for hiding this comment

openshift-ci-robot commented Dec 20, 2019

openshift-ci-robot commented Dec 20, 2019

haircommander commented Dec 20, 2019

haircommander commented Dec 20, 2019

giuseppe commented Dec 20, 2019

rhatdan commented Dec 20, 2019

haircommander commented Dec 20, 2019

haircommander commented Dec 20, 2019

saschagrunert commented Dec 20, 2019

haircommander commented Dec 21, 2019

saschagrunert Jan 8, 2020

Choose a reason for hiding this comment

haircommander Jan 8, 2020

Choose a reason for hiding this comment

saschagrunert Jan 9, 2020

Choose a reason for hiding this comment

codecov bot commented Dec 13, 2019 •

edited

saschagrunert left a comment •

edited