oci_conmon: do not create a cgroup under systemd #4861

giuseppe · 2020-01-14T14:37:42Z

Detect whether we are running under systemd (if the INVOCATION_ID is set). If Podman is running under a systemd service, we do not need to create a cgroup for conmon.

The variable is set only on new systemd versions (the version on RHEL8 seems fine and sets it), so for older versions add a --cgroups=no-conmon option to disable cgroups for conmon.

Closes: #4833

Signed-off-by: Giuseppe Scrivano gscrivan@redhat.com

openshift-ci-robot · 2020-01-14T14:37:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [giuseppe]

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

mheon · 2020-01-14T14:39:13Z

docs/source/markdown/podman-run.1.md

@@ -92,8 +92,9 @@ If the host uses cgroups v1, the default is set to **host**.  On cgroups v2 the
 **--cgroups**=*mode*

 Determines whether the container will create CGroups.
-Valid values are *enabled* and *disabled*, which the default being *enabled*.
+Valid values are *enabled*, *disabled*, *disabled-conmon*, which the default being *enabled*.


I think no-conmon would be a better name, but I'd like other folks to agree to that before we change it.

@vrothberg do you have any preference on the naming?

I don't have a preference.

libpod/container.go

giuseppe · 2020-01-15T13:31:57Z

how to move forward? I rename the new option to no-conmon and make it a string instead of a bool?

vrothberg · 2020-01-15T13:42:35Z

how to move forward? I rename the new option to no-conmon and make it a string instead of a bool?

SGTM

rhatdan · 2020-01-15T17:18:17Z

Can you add ("enabled"|"disabled"|"no-conmon") to the command line option? @edsantiago uses this for generation of the completion flags.

libpod/oci_conmon_linux.go

mheon · 2020-01-15T17:35:51Z

pkg/spec/namespaces.go

@@ -216,6 +216,9 @@ func (c *CgroupConfig) ToCreateOptions(runtime *libpod.Runtime) ([]libpod.CtrCre
 	if c.Cgroups == "disabled" {


IMO we should disable this and remove WithNoCgroups, and move everything to use WithCgroupsMode

We'll need to retain NoCgroups in the database and keep supporting it there for legacy reasons, but we should move all new containers over to use the new version

If we are depracatng it, then remove it from the docs and hide the option. No reason for people to continue to use it.

It should continue working - we're just changing the way we store it in the database

But I thought we were discouraging an option?

mheon · 2020-01-15T23:59:14Z

Just the way we used to store it in the DB - want to move from a bool to a string

…

On Wed, Jan 15, 2020, 18:13 Daniel J Walsh ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pkg/spec/namespaces.go <#4861 (comment)>: > @@ -216,6 +216,9 @@ func (c *CgroupConfig) ToCreateOptions(runtime *libpod.Runtime) ([]libpod.CtrCre if c.Cgroups == "disabled" { But I thought we were discouraging an option? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#4861>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB3AOCDERFFA4AFMA6JTG5LQ56J7ZANCNFSM4KGUCQ2A> .

rh-atomic-bot · 2020-01-16T17:33:22Z

☔ The latest upstream changes (presumably #4884) made this pull request unmergeable. Please resolve the merge conflicts.

it allows to disable cgroups creation only for the conmon process. A new cgroup is created for the container payload. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

Detect whether we are running under systemd (if the INVOCATION_ID is set). If Podman is running under a systemd service, we do not need to create a cgroup for conmon. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

giuseppe · 2020-01-16T21:29:25Z

tests are passing

rhatdan · 2020-01-16T21:41:08Z

LGTM
But I thought that @mheon still has an issue?

giuseppe · 2020-01-17T10:49:38Z

But I thought that @mheon still has an issue?

I think I took care of all the comments. @mheon is there still anything pending?

giuseppe · 2020-01-20T13:37:29Z

@mheon are you fine with the current version?

mheon · 2020-01-20T15:22:09Z

Looking...

mheon · 2020-01-20T15:23:00Z

/lgtm

mheon · 2020-01-20T15:23:08Z

/hold cancel

rhatdan · 2020-01-20T19:07:03Z

/test images

vrothberg · 2020-01-21T10:07:31Z

/test images

giuseppe · 2020-01-22T15:14:54Z

/retest

giuseppe · 2020-01-22T15:28:29Z

/test images

openshift-ci-robot requested review from TomSweeneyRedHat and rhatdan January 14, 2020 14:37

openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M labels Jan 14, 2020

mheon reviewed Jan 14, 2020

View reviewed changes

libpod/container.go Outdated Show resolved Hide resolved

giuseppe requested a review from vrothberg January 15, 2020 10:36

giuseppe force-pushed the add-cgroups-disabled-conmon branch from c912932 to ba1160c Compare January 15, 2020 14:34

giuseppe added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 15, 2020

giuseppe force-pushed the add-cgroups-disabled-conmon branch 2 times, most recently from 2db6dc1 to 0c270d7 Compare January 15, 2020 16:16

mheon reviewed Jan 15, 2020

View reviewed changes

libpod/oci_conmon_linux.go Outdated Show resolved Hide resolved

mheon reviewed Jan 15, 2020

View reviewed changes

giuseppe force-pushed the add-cgroups-disabled-conmon branch from 0c270d7 to 7324b39 Compare January 15, 2020 19:29

giuseppe force-pushed the add-cgroups-disabled-conmon branch 2 times, most recently from 5778fc3 to b71b77c Compare January 16, 2020 14:52

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 16, 2020

giuseppe added 2 commits January 16, 2020 18:56

podman: add new option --cgroups=no-conmon

ba0a6f3

it allows to disable cgroups creation only for the conmon process. A new cgroup is created for the container payload. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

systemdgen: specify --cgroups=disabled-conmon for --new

70236f5

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

giuseppe force-pushed the add-cgroups-disabled-conmon branch from b71b77c to 433a8c2 Compare January 16, 2020 17:59

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 16, 2020

oci_conmon: do not create a cgroup under systemd

1951ff1

Detect whether we are running under systemd (if the INVOCATION_ID is set). If Podman is running under a systemd service, we do not need to create a cgroup for conmon. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

giuseppe force-pushed the add-cgroups-disabled-conmon branch from 433a8c2 to 1951ff1 Compare January 16, 2020 19:29

mheon added the Release Notes 1.8.0 label Jan 20, 2020

openshift-ci-robot assigned mheon Jan 20, 2020

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jan 20, 2020

openshift-merge-robot merged commit 9f146b1 into containers:master Jan 22, 2020

mheon removed the Release Notes 1.8.0 label Jan 30, 2020

cognition9144 mentioned this pull request Mar 25, 2020

Use --new flag when generating systemd units for containers in pod #5605

Closed

vrothberg mentioned this pull request Jun 15, 2020

containers killed when restarting a systemd service #6549

Closed

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 25, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 25, 2023

		@@ -216,6 +216,9 @@ func (c CgroupConfig) ToCreateOptions(runtime libpod.Runtime) ([]libpod.CtrCre
		if c.Cgroups == "disabled" {

oci_conmon: do not create a cgroup under systemd #4861

oci_conmon: do not create a cgroup under systemd #4861

Uh oh!

Conversation

giuseppe commented Jan 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Jan 14, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

giuseppe commented Jan 15, 2020

Uh oh!

vrothberg commented Jan 15, 2020

Uh oh!

rhatdan commented Jan 15, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mheon commented Jan 15, 2020 via email

Uh oh!

rh-atomic-bot commented Jan 16, 2020

Uh oh!

giuseppe commented Jan 16, 2020

Uh oh!

rhatdan commented Jan 16, 2020

Uh oh!

giuseppe commented Jan 17, 2020

Uh oh!

giuseppe commented Jan 20, 2020

Uh oh!

mheon commented Jan 20, 2020

Uh oh!

mheon commented Jan 20, 2020

Uh oh!

mheon commented Jan 20, 2020

Uh oh!

rhatdan commented Jan 20, 2020

Uh oh!

vrothberg commented Jan 21, 2020

Uh oh!

giuseppe commented Jan 22, 2020

Uh oh!

giuseppe commented Jan 22, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

giuseppe commented Jan 14, 2020 •

edited

Loading