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

Grab all of the containers.conf settings for namespaces #3637

Merged
merged 2 commits into from
Nov 30, 2021
Merged

Grab all of the containers.conf settings for namespaces #3637

merged 2 commits into from
Nov 30, 2021

Conversation

rhatdan
Copy link
Member

@rhatdan rhatdan commented Nov 18, 2021

What type of PR is this?

/kind api-change
/kind bug
/kind cleanup
/kind deprecation
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake
/kind other

What this PR does / why we need it:

How to verify it

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 18, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhatdan

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:

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

nalind
nalind reviewed Nov 18, 2021
View reviewed changes
run_linux.go Outdated
{Name: string(specs.MountNamespace), Host: true},
{Name: string(specs.NetworkNamespace), Host: true},
{Name: string(specs.PIDNamespace), Host: true},
{Name: string(specs.NetworkNamespace), Host: cfg.PidNS() == "host"},
Copy link
Member

Choose a reason for hiding this comment

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

Copy/paste error.

nalind
nalind reviewed Nov 18, 2021
View reviewed changes
tests/bud.bats Show resolved Hide resolved
@rhatdan rhatdan force-pushed the cgroup branch 3 times, most recently from 91b6ff1 to 82bb93b Compare November 19, 2021 13:29
@rhatdan rhatdan mentioned this pull request Nov 19, 2021
Merged
nalind
nalind reviewed Nov 19, 2021
View reviewed changes
run_linux.go
{Name: string(specs.MountNamespace), Host: true},
{Name: string(specs.NetworkNamespace), Host: true},
{Name: string(specs.PIDNamespace), Host: true},
{Name: string(specs.NetworkNamespace), Host: cfg.NetNS() == "host" || cfg.NetNS() == "container"},
Copy link
Member

Choose a reason for hiding this comment

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

Why is "container" special-cased here here, for only this one namespace?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure what container even means for Buildah, but this matches one of the tests expectations.

Copy link
Member Author

Choose a reason for hiding this comment

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

container is not defined in man pages, I think it is just a compatibility mode thing.

@TomSweeneyRedHat
Copy link
Member

More test redness here for you @rhatdan

@rhatdan rhatdan force-pushed the cgroup branch 4 times, most recently from f207212 to f465185 Compare November 24, 2021 13:45
@rhatdan
Copy link
Member Author

rhatdan commented Nov 25, 2021

@giuseppe PTAL. I am curious what is going on with the two tests I had to add --cgroupns=host. Basically these tests work differently in a private cgroupns.

@giuseppe
Copy link
Member

@giuseppe PTAL. I am curious what is going on with the two tests I had to add --cgroupns=host. Basically these tests work differently in a private cgroupns.

I guess that is expected:

run_buildah run $cid /bin/sh -c 'cat /proc/$$/cgroup'
expect_output --substring "test-cgroup"

The test above would fail when running in a new cgroupns because the container cannot see its group path, but would see just /:

$ podman run --rm --cgroupns=host -ti alpine cat /proc/self/cgroup
0::/user.slice/user-1000.slice/user@1000.service/user.slice/libpod-01e557c5a1e9f53f661b067ff7972f2fef49c7765e72a28eb1ce8f101820cde9.scope/container
$ podman run --rm --cgroupns=private -ti alpine cat /proc/self/cgroup
0::/

@nalind
Copy link
Member

nalind commented Nov 29, 2021

This should be exercising --cgroupns in combination with the other options that control what we do with namespaces in the "combination-namespaces" test in namespaces.bats. Looks fine otherwise.

@rhatdan
Grab all of the containers.conf settings for namespaces.
05d5d22 
Buildah is mainly building with Host Namespaces, this changes most
namespaces to be private matching Podman and using containers.conf

Fixes: containers#3634

Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
@rhatdan
Turn on rootless cgroupv2 tests
b480ce8 
Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
nalind
nalind reviewed Nov 30, 2021
View reviewed changes
tests/namespaces.bats
[ "$output" != "" ]
run_buildah run --terminal=false $ctr pwd
[ "$output" != "" ]
for cgroupns in host container private; do
Copy link
Member

Choose a reason for hiding this comment

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

Got a mixture of tabs and spaces here and on line 428.

@nalind
Copy link
Member

nalind commented Nov 30, 2021

Got a tabs/spaces mixture in namespaces.bats that makes the diff look weird, but LGTM.

@rhatdan rhatdan added the lgtm label Nov 30, 2021
@openshift-merge-robot openshift-merge-robot merged commit 6d01253 into containers:main Nov 30, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@nalind nalind nalind left review comments

Assignees
No one assigned
Labels
approved lgtm locked - please file new issue/PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@rhatdan @TomSweeneyRedHat @giuseppe @nalind @openshift-merge-robot