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.

Sign up for GitHub

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

Jump to bottom

Add support for target namespaces #5440

Merged
merged 13 commits into from Dec 16, 2021
Merged

Add support for target namespaces #5440

merged 13 commits into from Dec 16, 2021

Conversation

haircommander
Copy link
Member

What type of PR is this?

/kind feature

What this PR does / why we need it:

Adds support for TARGET namespaces, which is needed by ephemeral containers (set to go beta in 1.23)

Which issue(s) this PR fixes:

fixes #4790

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Add support for TARGET namespace mode, which enables support for ephemeral containers.

@openshift-ci openshift-ci bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. kind/feature Categorizes issue or PR as related to a new feature. labels Nov 5, 2021
@openshift-ci openshift-ci bot requested review from sameo and sboeuf November 5, 2021 18:06
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 5, 2021
@haircommander haircommander force-pushed the pin-pid-2 branch 4 times, most recently from 6039474 to f2b4676 Compare November 5, 2021 19:42
@haircommander haircommander added this to the 1.23 milestone Nov 5, 2021
@rphillips
Copy link
Contributor

rphillips commented Nov 8, 2021 •
edited

I did a review pass... just Sascha's comment needs a fix

littlejawa
littlejawa reviewed Nov 15, 2021
View reviewed changes
test/test_runner.sh Outdated Show resolved Hide resolved
saschagrunert
saschagrunert approved these changes Nov 16, 2021
View reviewed changes
Copy link
Member

@saschagrunert saschagrunert left a comment

Choose a reason for hiding this comment

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

/retest-required

@haircommander
Copy link
Member Author

/hold
unit tests and kube e2e tests (in github actions) still fail, need to investigate those

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 16, 2021
@codecov
Copy link

codecov bot commented Nov 18, 2021 •
edited

Codecov Report

Merging #5440 (2f1a115) into main (daab4d1) will increase coverage by 0.17%.
The diff coverage is 42.85%.

❗ Current head 2f1a115 differs from pull request most recent head e7aa30f. Consider uploading reports for the commit e7aa30f to get more accurate results

@@            Coverage Diff             @@
##             main    #5440      +/-   ##
==========================================
+ Coverage   43.25%   43.43%   +0.17%     
==========================================
  Files         120      121       +1     
  Lines       12001    12037      +36     
==========================================
+ Hits         5191     5228      +37     
+ Misses       6315     6306       -9     
- Partials      495      503       +8

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 24, 2021
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 30, 2021
@haircommander
Copy link
Member Author

/retest
/unhold

kolyshkin
kolyshkin reviewed Dec 13, 2021
View reviewed changes
pkg/config/config_test.go Outdated
@@ -14,6 +14,8 @@ import (
. "github.com/onsi/gomega"
)

const tmpDir = "/tmp"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: use os.TempDir() instead?

@kolyshkin
Copy link
Collaborator

It seems like a few last commits are fixups to earlier ones. Instead, they should be part of those earlier commits.

@saschagrunert
Bump Kubernetes to v1.23.0
6f4e7bf 
Signed-off-by: Sascha Grunert <sgrunert@redhat.com>
@haircommander
pkg/sandbox: take config initialization
8db9a89 
Signed-off-by: Peter Hunt <pehunt@redhat.com>
@haircommander
nsmgr: take (and rename) NamespacePathFromProc
e122cb4 
Signed-off-by: Peter Hunt <pehunt@redhat.com>
@haircommander
nsmgr: add function to pin existing namespace
440ba9f 
Signed-off-by: Peter Hunt <pehunt@redhat.com>
@haircommander
nsmgrtest: take some namespace related test code
546732e 
Signed-off-by: Peter Hunt <pehunt@redhat.com>
@haircommander
pkg/container: take container namespace configuration
1fa69c7 
Signed-off-by: Peter Hunt <pehunt@redhat.com>
@haircommander
oci: add managed pidns to container object
4b0d40a 
Signed-off-by: Peter Hunt <pehunt@redhat.com>
@haircommander
test: give testunit sudo
da0de53 
Otherwise, "Container:SpecAddNamespaces should use target PID namespace" fails

Signed-off-by: Peter Hunt <pehunt@redhat.com>
@haircommander
add support for target PID namespaces
0176d5f 
Signed-off-by: Peter Hunt <pehunt@redhat.com>
@haircommander
test: add test for target namespace
972c29c 
Signed-off-by: Peter Hunt <pehunt@redhat.com>
@haircommander
test: skip target tests for userns
9f584ca 
Signed-off-by: Peter Hunt <pehunt@redhat.com>
@haircommander
unit tests: update pinns path in case it isn't found in PATH
acc746e 
Signed-off-by: Peter Hunt <pehunt@redhat.com>
@haircommander
unit test: fix relative log test
e7aa30f 
Signed-off-by: Peter Hunt <pehunt@redhat.com>
@haircommander
Copy link
Member Author

/retest-required

@haircommander
Copy link
Member Author

/test integration_crun

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 15, 2021 •
edited

@haircommander: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/openshift-jenkins/e2e_crun_cgroupv2 e7aa30f link false /test e2e_cgroupv2
ci/openshift-jenkins/integration_crun_cgroupv2 e7aa30f link false /test integration_cgroupv2

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

mrunalp
mrunalp approved these changes Dec 16, 2021
View reviewed changes
Copy link
Member

@mrunalp mrunalp left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 16, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 16, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: haircommander, mrunalp, 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 [haircommander,mrunalp,saschagrunert]

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

@openshift-bot
Copy link

/retest-required

Please review the full test history for this PR and help us cut down flakes.

1 similar comment
@openshift-bot
Copy link

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 183d7ef into cri-o:main Dec 16, 2021
@ChristianCiach ChristianCiach mentioned this pull request Jun 20, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@littlejawa littlejawa littlejawa left review comments

@kolyshkin kolyshkin kolyshkin left review comments

@mrunalp mrunalp mrunalp approved these changes

@saschagrunert saschagrunert saschagrunert approved these changes

@runcom runcom Awaiting requested review from runcom

@sameo sameo Awaiting requested review from sameo

@sboeuf sboeuf Awaiting requested review from sboeuf

Assignees

@mrunalp mrunalp

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes.
Projects
None yet
Milestone
1.23
Development

Successfully merging this pull request may close these issues.

Support Target NamespaceMode
8 participants
@haircommander @rphillips @kolyshkin @openshift-bot @mrunalp @saschagrunert @littlejawa @openshift-merge-robot