check that StartTransientUnit/StopUnit succeeds by lifubang · Pull Request #2331 · opencontainers/runc

lifubang · 2020-04-20T00:47:12Z

To fix #2313
We can check whether StartTransientUnit succeeds or not with the value from the channel.
If we got failed, we should run ResetFailedUnit and return the error.

[root@localhost systemdtest]# runc --systemd-cgroup run -d test
WARN[0000] signal: killed                               
ERRO[0000] cannot detect unified path                   
ERRO[0000] container_linux.go:349: starting container process caused "process_linux.go:306: applying cgroup configuration for process caused \"StartTransientUnit job is not done: failed\"" 
container_linux.go:349: starting container process caused "process_linux.go:306: applying cgroup configuration for process caused \"StartTransientUnit job is not done: failed\""

The error ERRO[0000] cannot detect unified path relates to #2329 .

To fix #2344
We should wait StopUnit succeeds.

Signed-off-by: lifubang lifubang@acmcoder.com

lifubang · 2020-04-20T00:58:23Z

@kolyshkin PTAL

libcontainer/cgroups/systemd/unified_hierarchy.go

kolyshkin

Thank you for working on that! Left a couple of nits.

Can you also add the same code (wait for the operation to complete, check it's "done", return an error if not) to the Destroy() method. Currently it does not even wait.

Oh, finally, could you please wait until #2299 is merged and then rebase? It is mostly code refactoring in there.

kolyshkin · 2020-04-20T01:46:15Z

@lifubang have you tested that the fix works? I have described the repro in details here: containers/crun#331 (comment), should take 10-15 minutes to check it (mostly waiting for vagrant up).

lifubang · 2020-04-20T01:50:01Z

Can you also add the same code (wait for the operation to complete, check it's "done", return an error if not) to the Destroy() method. Currently it does not even wait.

Oh, finally, could you please wait until #2299 is merged and then rebase? It is mostly code refactoring in there.

OK

lifubang · 2020-04-20T01:52:45Z

have you tested that the fix works?

I have tested it in vmware.

lifubang · 2020-04-20T07:15:18Z

Can you also add the same code (wait for the operation to complete, check it's "done", return an error if not) to the Destroy() method. Currently it does not even wait.

I add it in #2329

kolyshkin · 2020-04-21T09:22:45Z

Needs rebase

libcontainer/cgroups/systemd/v2.go

kolyshkin · 2020-04-22T02:32:23Z

libcontainer/cgroups/systemd/v2.go

+		select {
+		case s := <-statusChan:
+			if DbusJobCompletionResultType(s) != DbusJobDone {
+				logrus.Warnf("error stopping unit `%s`: got `%s`. Continuing...", unitName, s)


I think s/stopping/removing/ is better.

kolyshkin · 2020-04-22T02:33:07Z

libcontainer/cgroups/systemd/common.go

+
+const (
+	DbusJobDone       DbusJobCompletionResultType = "done"
+	DbusJobCanceled   DbusJobCompletionResultType = "canceled"


Are you using any of those (I mean, except for DbusJobDone)?

Removed now!

kolyshkin · 2020-04-22T02:34:27Z

libcontainer/cgroups/systemd/common.go

 )

+// Refer to https://github.com/coreos/go-systemd/blob/5a0db84d3dc459ccdc6ffcc44b1c452bf9f171cb/dbus/methods.go#L78-L101
+// or https://godoc.org/github.com/coreos/go-systemd/dbus#Conn.StartUnit


these two are the same: one is the source code, another is godoc extracted from it.

And we only need "done", let's not overcomplicate things.

In v1, shall we need to add this?
If yes, I'll add a const string DbusJobDone.

lifubang · 2020-04-22T07:11:07Z

Can you also add the same code (wait for the operation to complete, check it's "done", return an error if not) to the Destroy() method. Currently it does not even wait.

@kolyshkin this code also can fix #2344

AkihiroSuda · 2020-04-22T16:12:39Z

libcontainer/cgroups/systemd/v2.go

+			// Please refer to https://godoc.org/github.com/coreos/go-systemd/dbus#Conn.StartUnit
+			if s != "done" {
+				logrus.Warnf("error removing unit `%s`: got `%s`. Continuing...", unitName, s)
+			}


The channel can be closed here?

I think we should not close the channel, because if someone send string to this closed channel, it will cause goroutine panic. Although I guess there is nobody would send sth to this channel.

I checked the github.com/coreos/go-systemd/dbus code.

Looks like once it sends a string over the channel it forgets it. So yes we should close the channel -- unless we hit the timeout!

finished now

kolyshkin · 2020-04-24T00:57:53Z

this code also can fix #2344

Nice! @lifubang please update the description to say so (and about the StopUnit, too).

kolyshkin · 2020-04-24T01:33:56Z

So, I guess this code should be moved to libcontainer/cgroups/systemd/common.go and used from both v1 and v2.

lifubang · 2020-04-24T09:31:34Z

Both v1 and v2 are using the same StartTransientUnit/StopUnit method now, and close the channel when finished now.

kolyshkin

~~LGTM~~ too early :)

kolyshkin · 2020-04-24T18:46:54Z

libcontainer/cgroups/systemd/common.go

 	return false
 }
+
+func startJob(unitName string, properties []systemdDbus.Property) error {


s/startJob/startUnit/

Job is a very misleading term here.

kolyshkin · 2020-04-24T18:47:12Z

libcontainer/cgroups/systemd/common.go

+	return nil
+}
+
+func stopJob(unitName string) error {


s/stopJob/stopUnit/

kolyshkin

Nit wrt functions naming. Otherwise, it's perfecto!

AkihiroSuda · 2020-04-24T23:51:28Z

LGTM

AkihiroSuda · 2020-04-25T19:23:14Z

needs rebase

AkihiroSuda · 2020-04-25T23:27:13Z

LGTM

AkihiroSuda · 2020-04-28T04:54:15Z

@kolyshkin LGTY?

libcontainer/cgroups/systemd/v1.go

libcontainer/cgroups/systemd/v2.go

Signed-off-by: lifubang <lifubang@acmcoder.com>

AkihiroSuda · 2020-04-28T08:01:49Z

LGTM

kolyshkin

LGTM

kolyshkin · 2020-04-28T17:44:19Z

LGTM

kolyshkin reviewed Apr 20, 2020

View reviewed changes

libcontainer/cgroups/systemd/unified_hierarchy.go Outdated Show resolved Hide resolved

lifubang force-pushed the StartTransientUnit branch from 613c712 to 28fc3f3 Compare April 20, 2020 01:38

kolyshkin requested changes Apr 20, 2020

View reviewed changes

lifubang force-pushed the StartTransientUnit branch from bb47d24 to c0b435d Compare April 20, 2020 07:13

kolyshkin mentioned this pull request Apr 20, 2020

cgroupv2 support meta issue #2315

Closed

kolyshkin added the kind/bug label Apr 21, 2020

lifubang force-pushed the StartTransientUnit branch from c0b435d to 3edc556 Compare April 21, 2020 10:04

lifubang mentioned this pull request Apr 21, 2020

cgroupv2: fix Poststop hooks could not run when using systemd driver #2329

Closed

lifubang force-pushed the StartTransientUnit branch 2 times, most recently from 77d1669 to b47cad6 Compare April 21, 2020 14:38

mrunalp reviewed Apr 21, 2020

View reviewed changes

libcontainer/cgroups/systemd/v2.go Outdated Show resolved Hide resolved

lifubang force-pushed the StartTransientUnit branch 2 times, most recently from 2aa85d6 to a5d5d89 Compare April 22, 2020 01:40

kolyshkin reviewed Apr 22, 2020

View reviewed changes

lifubang force-pushed the StartTransientUnit branch 3 times, most recently from 8fcd201 to ac366bc Compare April 22, 2020 02:56

lifubang mentioned this pull request Apr 22, 2020

cgroup2/systemd: remove /sys/fs/cgroup/system.slice/runc-test.scope: device or resource busy #2344

Closed

AkihiroSuda reviewed Apr 22, 2020

View reviewed changes

lifubang changed the title ~~check that StartTransientUnit succeeds~~ check StartTransientUnit/StopUnit succeeds Apr 24, 2020

lifubang changed the title ~~check StartTransientUnit/StopUnit succeeds~~ check that StartTransientUnit/StopUnit succeeds Apr 24, 2020

lifubang force-pushed the StartTransientUnit branch 2 times, most recently from 0da17ed to 59912cd Compare April 24, 2020 07:42

kolyshkin approved these changes Apr 24, 2020

View reviewed changes

kolyshkin reviewed Apr 24, 2020

View reviewed changes

kolyshkin requested changes Apr 24, 2020

View reviewed changes

lifubang force-pushed the StartTransientUnit branch from 59912cd to 2b91a2d Compare April 24, 2020 23:46

AkihiroSuda added the area/systemd label Apr 24, 2020

lifubang force-pushed the StartTransientUnit branch from 2b91a2d to b0fffaa Compare April 25, 2020 23:24

AkihiroSuda reviewed Apr 28, 2020

View reviewed changes

libcontainer/cgroups/systemd/v1.go Outdated Show resolved Hide resolved

AkihiroSuda reviewed Apr 28, 2020

View reviewed changes

libcontainer/cgroups/systemd/v2.go Outdated Show resolved Hide resolved

AkihiroSuda mentioned this pull request Apr 28, 2020

cgroup v2: support rootless systemd #2281

Merged

AkihiroSuda marked this pull request as draft April 28, 2020 07:26

AkihiroSuda marked this pull request as ready for review April 28, 2020 07:28

check that StartTransientUnit and StopUnit succeeds

bfa1b2a

Signed-off-by: lifubang <lifubang@acmcoder.com>

lifubang force-pushed the StartTransientUnit branch from b0fffaa to bfa1b2a Compare April 28, 2020 07:46

kolyshkin approved these changes Apr 28, 2020

View reviewed changes

kolyshkin merged commit 0a4dcc0 into opencontainers:master Apr 28, 2020

kolyshkin mentioned this pull request Apr 28, 2020

Check the error return from StopUnit() in UnifiedManager#Destroy() #2309

Closed

kolyshkin mentioned this pull request Jul 6, 2020

cgroup, systemd: cleanup cgroups #2503

Merged

Conversation

lifubang commented Apr 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lifubang commented Apr 20, 2020

Uh oh!

Uh oh!

kolyshkin left a comment

Choose a reason for hiding this comment

Uh oh!

kolyshkin commented Apr 20, 2020

Uh oh!

lifubang commented Apr 20, 2020

Uh oh!

lifubang commented Apr 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lifubang commented Apr 20, 2020

Uh oh!

kolyshkin commented Apr 21, 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!

lifubang Apr 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

lifubang commented Apr 22, 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!

Choose a reason for hiding this comment

Uh oh!

kolyshkin commented Apr 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kolyshkin commented Apr 24, 2020

Uh oh!

lifubang commented Apr 24, 2020

Uh oh!

kolyshkin left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

kolyshkin Apr 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kolyshkin left a comment

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda commented Apr 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AkihiroSuda commented Apr 25, 2020

Uh oh!

AkihiroSuda commented Apr 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AkihiroSuda commented Apr 28, 2020

Uh oh!

lifubang commented Apr 20, 2020 •

edited

Loading

lifubang commented Apr 20, 2020 •

edited

Loading

lifubang Apr 22, 2020 •

edited

Loading

kolyshkin commented Apr 24, 2020 •

edited

Loading

kolyshkin left a comment •

edited

Loading

kolyshkin Apr 24, 2020 •

edited

Loading

AkihiroSuda commented Apr 24, 2020 •

edited

Loading

AkihiroSuda commented Apr 25, 2020 •

edited

Loading

AkihiroSuda commented Apr 28, 2020 •

edited

Loading

kolyshkin commented Apr 28, 2020 •

edited by AkihiroSuda

Loading