fix path error in systemd when stopped by lifubang · Pull Request #2338 · opencontainers/runc

lifubang · 2020-04-21T10:49:35Z

fix #2337

Because when the container is in stopped state, the cgroup path has been deleted by systemd driver.

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

lifubang · 2020-04-21T10:50:30Z

~~This pr only fix cgroup v2.~~
~~For cgroup v1, it also has this problem, shall we need to fix it in cgroup v1?~~

libcontainer/cgroups/systemd/v2.go

kolyshkin

I'd rather check if path exists in callers, whenever deemed necessary (or ignore ENOENT in some cases)

lifubang · 2020-04-23T13:48:43Z

I'd rather check if path exists in callers

Force pushed.
And the label is "area/systemd", not "area/cgroupv2". @kolyshkin

lifubang · 2020-04-24T09:29:09Z

travis-ci green now!

libcontainer/container_linux.go

kolyshkin

I think it's better to add Exists() method to cgroupManager and use it.

thaJeztah · 2020-05-01T08:30:05Z

libcontainer/container_linux.go

+		}
+	}
+
+	pids, err = c.cgroupManager.GetAllPids()


wouldn't this already be able to have a check for os.IsNotExist(err) ? (Also wondering if c.cgroupManager.GetAllPids() should ignore os.IsNotExist(err) by default (if the path doesn't exist, just return an empty list of pids ?

wouldn't this already be able to have a check for os.IsNotExist(err) ?

Yes, but in fs cgroup driver, there is no error when the container is in stopped state. And I think we should not ignore IsNotExist error when it is not in stopped state or it ~~has joined a new pid namespace without path~~ is not in a private pid namespace.

(Also wondering if c.cgroupManager.GetAllPids() should ignore os.IsNotExist(err) by default (if the path doesn't exist, just return an empty list of pids ?

I think we should not ignore IsNotExist error when it is not in stopped state or it ~~has joined a new pid namespace without path~~ is not in a private pid namespace

kolyshkin · 2020-05-14T23:47:05Z

@lifubang this needs a rebase

kolyshkin · 2020-05-20T16:26:30Z

libcontainer/container_linux_test.go

+	pid := 1
+	stat, err := system.Stat(pid)
+	if err != nil {
+		t.Fatalf("can't state pid %d", pid)


nit: s/state/stat/
nit: include the actual error as well

kolyshkin · 2020-05-20T16:41:45Z

libcontainer/container_linux_test.go

+		cgroupManager: &mockCgroupManager{
+			allPids: []int{1, 2, 3},
+			paths: map[string]string{
+				"device": "/proc/self/cgroups",


this looks kind of weird.

It's a mock data, I don't have any idea to improve it.

kolyshkin · 2020-05-20T16:48:47Z

libcontainer/cgroups/fs/apply_raw.go

+
+func (m *manager) Exists() bool {
+	paths := m.GetPaths()
+	return cgroups.PathExists(paths["devices"])


I think you can just do

return cgroups.PathExists(m.Path("devices"))

kolyshkin · 2020-05-20T16:54:51Z

libcontainer/cgroups/systemd/v1.go

 }
+
+func (m *legacyManager) Exists() bool {
+	path, err := getSubsystemPath(m.cgroups, "devices")


getSubsystemPath is super expensive, it parses the whole /proc/self/mountinfo.

I think you should be using m.Path("devices") here.

AkihiroSuda · 2020-06-02T05:45:10Z

@lifubang PTAL at Kir's comments. Also, can we have an integration test?

When we use cgroup with systemd driver, the cgroup path will be auto removed by systemd when all processes exited. So we should check cgroup path exists when we access the cgroup path, for example in `kill/ps`, or else we will got an error. Signed-off-by: lifubang <lifubang@acmcoder.com>

lifubang · 2020-06-02T10:57:50Z

@lifubang PTAL at Kir's comments. Also, can we have an integration test?

finished now, PTAL.

AkihiroSuda · 2020-06-02T11:50:40Z

tests/integration/ps.bats

+
+@test "ps after the container stopped" {
+  # ps is not supported, it requires cgroups
+  requires root


should suppprt rootless cgroup

AkihiroSuda · 2020-06-04T03:35:19Z

CI failing

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

AkihiroSuda

Thanks

kolyshkin · 2020-06-08T23:26:12Z

libcontainer/container_linux_test.go

+func (m *mockCgroupManager) Exists() bool {
+	paths := m.GetPaths()
+	if paths != nil {
+		_, err := os.Lstat(paths["devices"])


While it is technically OK to use m.GetPaths() here, and it's a mock code so it doesn't really matter, I'd still like to have m.Path("devices") used here, because since commit 714c91e we're not supposed to use GetPaths() for anything other than state save/restore.

kolyshkin · 2020-06-08T23:28:09Z

libcontainer/cgroups/fs/apply_raw.go

 }
+
+func (m *manager) Exists() bool {
+	return cgroups.PathExists(m.paths["devices"])


It probably doesn't matter in practice, but we're not supposed to access a map without holding a lock. This is why I have suggested using m.Path("devices") earlier -- it takes a lock before accessing m.paths.

Alternatively, you can leave this code as is but add taking a lock (same as Path()).

kolyshkin · 2020-06-08T23:28:20Z

libcontainer/cgroups/systemd/v1.go

 }
+
+func (m *legacyManager) Exists() bool {
+	return cgroups.PathExists(m.paths["devices"])


libcontainer/container_linux.go

kolyshkin

couple of nits about using Path() vs GetPaths() and locking when accessing the m.paths map in v1 code.

mrunalp · 2020-06-11T20:12:32Z

@lifubang could you update the PR?

AkihiroSuda · 2020-06-15T04:17:00Z

@kolyshkin @mrunalp
Let's merge this as-is and cover the nits in a follow-up PR.

mrunalp · 2020-06-15T04:24:20Z

I am fine with that.

AkihiroSuda · 2020-06-15T23:52:35Z

@kolyshkin WDYT?

kolyshkin reviewed Apr 21, 2020

View reviewed changes

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

kolyshkin requested changes Apr 21, 2020

View reviewed changes

kolyshkin added the area/cgroupv2 label Apr 22, 2020

lifubang force-pushed the systemdcgroupv2 branch 3 times, most recently from 0e410f7 to 157518f Compare April 23, 2020 13:45

lifubang force-pushed the systemdcgroupv2 branch 4 times, most recently from f90a6ec to cd9408b Compare April 24, 2020 08:47

kolyshkin reviewed May 1, 2020

View reviewed changes

libcontainer/container_linux.go Outdated Show resolved Hide resolved

kolyshkin reviewed May 1, 2020

View reviewed changes

libcontainer/container_linux.go Outdated Show resolved Hide resolved

kolyshkin reviewed May 1, 2020

View reviewed changes

libcontainer/container_linux.go Outdated Show resolved Hide resolved

kolyshkin requested changes May 1, 2020

View reviewed changes

thaJeztah reviewed May 1, 2020

View reviewed changes

lifubang force-pushed the systemdcgroupv2 branch 4 times, most recently from 146afb1 to 9e6d835 Compare May 3, 2020 12:02

lifubang force-pushed the systemdcgroupv2 branch 2 times, most recently from d946961 to b565295 Compare May 15, 2020 22:57

kolyshkin added the area/systemd label May 20, 2020

kolyshkin reviewed May 20, 2020

View reviewed changes

AkihiroSuda mentioned this pull request Jun 2, 2020

[rootless+cgroup2+CRI] runc did not terminate successfully: lstat /sys/fs/cgroup/user.slice/user-1001.slice/user@1001.service/user.slice/cri-containerd-<id>.scope: no such file or directory\n: unknown" #2447

Closed

AkihiroSuda added this to the 1.0.0-rc91 (nee rc11) milestone Jun 2, 2020

AkihiroSuda added the kind/bug label Jun 2, 2020

lifubang force-pushed the systemdcgroupv2 branch from b565295 to 03fd932 Compare June 2, 2020 10:55

AkihiroSuda requested changes Jun 2, 2020

View reviewed changes

lifubang force-pushed the systemdcgroupv2 branch from 03fd932 to 9a8f2f2 Compare June 2, 2020 22:50

AkihiroSuda mentioned this pull request Jun 5, 2020

[failing test] "Pod Container Status should never report success for a pending container" hangs and times out KIND jobs kubernetes/kubernetes#91579

Closed

lifubang force-pushed the systemdcgroupv2 branch from 9a8f2f2 to 92e2c29 Compare June 5, 2020 23:50

add integration test for ps/kill after the container stopped

4be5435

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

lifubang force-pushed the systemdcgroupv2 branch from 92e2c29 to 4be5435 Compare June 6, 2020 00:57

AkihiroSuda approved these changes Jun 6, 2020

View reviewed changes

dims mentioned this pull request Jun 8, 2020

Kubernetes failures with latest runc #2461

Closed

kolyshkin reviewed Jun 8, 2020

View reviewed changes

libcontainer/container_linux.go Show resolved Hide resolved

kolyshkin requested changes Jun 8, 2020

View reviewed changes

mrunalp approved these changes Jun 15, 2020

View reviewed changes

kolyshkin approved these changes Jun 16, 2020

View reviewed changes

kolyshkin merged commit 5b247e7 into opencontainers:master Jun 16, 2020

kolyshkin mentioned this pull request Jun 16, 2020

Paths nits #2472

Merged

kolyshkin mentioned this pull request Apr 5, 2023

libct: fix a race with systemd removal #3812

Merged

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

kolyshkin left a comment

Choose a reason for hiding this comment

Uh oh!

lifubang commented Apr 23, 2020

Uh oh!

lifubang commented Apr 24, 2020

Uh oh!

Uh oh!

Uh oh!

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!

lifubang May 2, 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 commented May 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

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

Uh oh!

Uh oh!

lifubang commented Jun 2, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda commented Jun 4, 2020

Uh oh!

AkihiroSuda left a comment

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!

Uh oh!

kolyshkin left a comment

Choose a reason for hiding this comment

Uh oh!

mrunalp commented Jun 11, 2020

Uh oh!

AkihiroSuda commented Jun 15, 2020

Uh oh!

mrunalp commented Jun 15, 2020

Uh oh!

AkihiroSuda commented Jun 15, 2020

Uh oh!

Reviewers

Assignees

Labels

lifubang commented Apr 21, 2020 •

edited

Loading

lifubang commented Apr 21, 2020 •

edited

Loading

kolyshkin left a comment •

edited

Loading

lifubang May 2, 2020 •

edited

Loading

kolyshkin commented May 14, 2020 •

edited

Loading

AkihiroSuda commented Jun 2, 2020 •

edited

Loading