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

namespaces: by default create cgroupns on cgroups v2 #4374

Merged
merged 1 commit into from
Nov 5, 2019
Merged

namespaces: by default create cgroupns on cgroups v2 #4374

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cmd/podman/common.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ func getCreateFlags(c *cliconfig.PodmanCommand) {
"Drop capabilities from the container",
)
createFlags.String(
"cgroupns", "host",
"cgroupns", "",
Copy link
Member

Choose a reason for hiding this comment

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

Can't we dynamically set the default like we do for a few other things? A bit less ugly than special-casing the empty string

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking about that, but one potential issue would be, if we default the CLI based on when the container was created, and the user switched cgroups, it could cause issues with existing containers.

Copy link
Member

Choose a reason for hiding this comment

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

Existing containers are an issue either way - this is all done at spec generation time, it'll be in the final, immutable spec that we put in the DB

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'd prefer we don't add this logic to the cmd/ package if possible.

Also, we need to be able to catch errors in the cgroups v2 detection. If we move it here, we would need to either ignore the error or panic.

"cgroup namespace to use",
)
createFlags.String(
Expand Down
4 changes: 3 additions & 1 deletion docs/source/markdown/podman-create.1.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,12 +67,14 @@ Drop Linux capabilities

**--cgroupns**=*mode*

Set the cgroup namespace mode for the container, by default **host** is used.
Set the cgroup namespace mode for the container.
**host**: use the host's cgroup namespace inside the container.
**container:<NAME|ID>**: join the namespace of the specified container.
**private**: create a new cgroup namespace.
**ns:<PATH>**: join the namespace at the specified path.

If the host uses cgroups v1, the default is set to **host**. On cgroups v2 the default is **private**.

**--cgroups**=*mode*

Determines whether the container will create CGroups.
Expand Down
4 changes: 3 additions & 1 deletion docs/source/markdown/podman-run.1.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,12 +81,14 @@ Drop Linux capabilities

**--cgroupns**=*mode*

Set the cgroup namespace mode for the container, by default **host** is used.
Set the cgroup namespace mode for the container.
**host**: use the host's cgroup namespace inside the container.
**container:<NAME|ID>**: join the namespace of the specified container.
**private**: create a new cgroup namespace.
**ns:<PATH>**: join the namespace at the specified path.

If the host uses cgroups v1, the default is set to **host**. On cgroups v2 the default is **private**.

**--cgroups**=*mode*

Determines whether the container will create CGroups.
Expand Down
5 changes: 5 additions & 0 deletions pkg/namespaces/namespaces.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,11 @@ func (n CgroupMode) IsHost() bool {
return n == hostType
}

// IsDefaultValue indicates whether the cgroup namespace has the default value.
func (n CgroupMode) IsDefaultValue() bool {
return n == ""
}

// IsNS indicates a cgroup namespace passed in by path (ns:<path>)
func (n CgroupMode) IsNS() bool {
return strings.HasPrefix(string(n), nsType)
Expand Down
13 changes: 13 additions & 0 deletions pkg/spec/spec.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -631,6 +631,19 @@ func addIpcNS(config *CreateConfig, g *generate.Generator) error {

func addCgroupNS(config *CreateConfig, g *generate.Generator) error {
cgroupMode := config.CgroupMode

if cgroupMode.IsDefaultValue() {
// If the value is not specified, default to "private" on cgroups v2 and "host" on cgroups v1.
unified, err := cgroups.IsCgroup2UnifiedMode()
if err != nil {
return err
}
if unified {
cgroupMode = "private"
} else {
cgroupMode = "host"
}
}
if cgroupMode.IsNS() {
return g.AddOrReplaceLinuxNamespace(string(spec.CgroupNamespace), NS(string(cgroupMode)))
}
Expand Down
6 changes: 3 additions & 3 deletions test/e2e/run_cgroup_parent_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ var _ = Describe("Podman run with --cgroup-parent", func() {
Skip("Must be containerized to run this test.")
}
cgroup := "/zzz"
run := podmanTest.Podman([]string{"run", "--cgroup-parent", cgroup, fedoraMinimal, "cat", "/proc/self/cgroup"})
run := podmanTest.Podman([]string{"run", "--cgroupns=host", "--cgroup-parent", cgroup, fedoraMinimal, "cat", "/proc/self/cgroup"})
run.WaitWithDefaultTimeout()
Expect(run.ExitCode()).To(Equal(0))
ok, _ := run.GrepString(cgroup)
Expand All @@ -52,7 +52,7 @@ var _ = Describe("Podman run with --cgroup-parent", func() {
if !Containerized() && podmanTest.CgroupManager != "cgroupfs" {
cgroup = "/machine.slice"
}
run := podmanTest.Podman([]string{"run", fedoraMinimal, "cat", "/proc/self/cgroup"})
run := podmanTest.Podman([]string{"run", "--cgroupns=host", fedoraMinimal, "cat", "/proc/self/cgroup"})
run.WaitWithDefaultTimeout()
Expect(run.ExitCode()).To(Equal(0))
ok, _ := run.GrepString(cgroup)
Expand All @@ -64,7 +64,7 @@ var _ = Describe("Podman run with --cgroup-parent", func() {
Skip("Requires Systemd cgroup manager support")
}
cgroup := "aaaa.slice"
run := podmanTest.Podman([]string{"run", "--cgroup-parent", cgroup, fedoraMinimal, "cat", "/proc/1/cgroup"})
run := podmanTest.Podman([]string{"run", "--cgroupns=host", "--cgroup-parent", cgroup, fedoraMinimal, "cat", "/proc/1/cgroup"})
run.WaitWithDefaultTimeout()
Expect(run.ExitCode()).To(Equal(0))
ok, _ := run.GrepString(cgroup)
Expand Down