cgroupv2: fix Poststop hooks could not run when using systemd driver by lifubang · Pull Request #2329 · opencontainers/runc

lifubang · 2020-04-19T07:53:29Z

How to reproduce:

[root@localhost systemdtest]# runc --systemd-cgroup run -d test
[root@localhost systemdtest]# runc --systemd-cgroup delete -f test
ERRO[0000] cannot detect unified path

The error is in here:

runc/libcontainer/state_linux.go

Lines 63 to 76 in bf0a8e1

    
           func runPoststopHooks(c *linuxContainer) error { 
        
           	if c.config.Hooks != nil { 
        
           		s, err := c.currentOCIState() 
        
           		if err != nil { 
        
           			return err 
        
           		} 
        
           		for _, hook := range c.config.Hooks.Poststop { 
        
           			if err := hook.Run(s); err != nil { 
        
           				return err 
        
           			} 
        
           		} 
        
           	} 
        
           	return nil 
        
           }

If we got an error, the Poststop hooks could not run because the func has returned.

The reason is here:

runc/libcontainer/cgroups/utils.go

Lines 472 to 495 in bf0a8e1

    
           func RemovePaths(paths map[string]string) (err error) { 
        
           	delay := 10 * time.Millisecond 
        
           	for i := 0; i < 5; i++ { 
        
           		if i != 0 { 
        
           			time.Sleep(delay) 
        
           			delay *= 2 
        
           		} 
        
           		for s, p := range paths { 
        
           			os.RemoveAll(p) 
        
           			// TODO: here probably should be logging 
        
           			_, err := os.Stat(p) 
        
           			// We need this strange way of checking cgroups existence because 
        
           			// RemoveAll almost always returns error, even on already removed 
        
           			// cgroups 
        
           			if os.IsNotExist(err) { 
        
           				delete(paths, s) 
        
           			} 
        
           		} 
        
           		if len(paths) == 0 { 
        
           			return nil 
        
           		} 
        
           	} 
        
           	return fmt.Errorf("Failed to remove paths: %v", paths) 
        
           }

Because we delete the cgroup paths value in cgroup manager when destroy.

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

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

lifubang · 2020-04-20T07:26:55Z

libcontainer/cgroups/systemd/unified_hierarchy.go

-		return err
+	unitName := getUnitName(m.Cgroups)
+	statusChan := make(chan string, 1)
+	if _, err := dbusConnection.StopUnit(unitName, "replace", statusChan); err == nil {


@kolyshkin
I think we may not need StopUnit method. Because if the init process has been exited, the cgroup path will be deleted by systemd automatically.
WDYT

Well, I think it is still needed, otherwise it will show up in systemctl output as "killed" or something like that (I haven't checked yet, just a hunch).

We can also take a look at crun implementation -- in there the unit is removed from systemd first, then killed. Might make more sense.

kolyshkin · 2020-04-21T08:54:58Z

@lifubang can you please rebase it? (libcontainer/cgroups/systemd/unified_hierarchy.go is renamed to libcontainer/cgroups/systemd/v2.go)

lifubang · 2020-04-21T10:08:13Z

ERRO[0000] cannot detect unified path
This error has been fixed by #2299 .
I will add waiting for StopUnit in #2331 .

lifubang force-pushed the systemd branch 2 times, most recently from 554de67 to 3d9c9d3 Compare April 19, 2020 08:31

lifubang mentioned this pull request Apr 20, 2020

check that StartTransientUnit/StopUnit succeeds #2331

Merged

fix Poststop hooks could not run when using systemd driver in cgroup v2

b7bfa21

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

lifubang force-pushed the systemd branch from 3d9c9d3 to b7bfa21 Compare April 20, 2020 07:11

lifubang commented Apr 20, 2020

View reviewed changes

kolyshkin added the area/cgroupv2 label Apr 21, 2020

kolyshkin added the area/systemd label Apr 21, 2020

lifubang closed this Apr 21, 2020

lifubang deleted the systemd branch May 28, 2020 01:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

cgroupv2: fix Poststop hooks could not run when using systemd driver#2329
lifubang wants to merge 1 commit intoopencontainers:masterfrom
lifubang:systemd

lifubang commented Apr 19, 2020

Uh oh!

lifubang Apr 20, 2020

Uh oh!

kolyshkin Apr 20, 2020

Uh oh!

kolyshkin commented Apr 21, 2020

Uh oh!

lifubang commented Apr 21, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	func runPoststopHooks(c *linuxContainer) error {
	if c.config.Hooks != nil {
	s, err := c.currentOCIState()
	if err != nil {
	return err
	}
	for _, hook := range c.config.Hooks.Poststop {
	if err := hook.Run(s); err != nil {
	return err
	}
	}
	}
	return nil
	}

	func RemovePaths(paths map[string]string) (err error) {
	delay := 10 * time.Millisecond
	for i := 0; i < 5; i++ {
	if i != 0 {
	time.Sleep(delay)
	delay *= 2
	}
	for s, p := range paths {
	os.RemoveAll(p)
	// TODO: here probably should be logging
	_, err := os.Stat(p)
	// We need this strange way of checking cgroups existence because
	// RemoveAll almost always returns error, even on already removed
	// cgroups
	if os.IsNotExist(err) {
	delete(paths, s)
	}
	}
	if len(paths) == 0 {
	return nil
	}
	}
	return fmt.Errorf("Failed to remove paths: %v", paths)
	}

Conversation

lifubang commented Apr 19, 2020

Uh oh!

lifubang Apr 20, 2020

Choose a reason for hiding this comment

Uh oh!

kolyshkin Apr 20, 2020

Choose a reason for hiding this comment

Uh oh!

kolyshkin commented Apr 21, 2020

Uh oh!

lifubang commented Apr 21, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants