Skip to content

Commit b5ccc66

Browse files
Tom Godkinostenbomgeorgethebeatle
committed
Do not kill all on task delete by default
 - Still KillAll if the task uses the hosts pid namespace - Test for both host pid namespace and normal cases Co-authored-by: Oliver Stenbom <[email protected]> Co-authored-by: Georgi Sabev <[email protected]> Signed-off-by: Oliver Stenbom <[email protected]>
1 parent 0649e38 commit b5ccc66

File tree

4 files changed

+83
-16
lines changed

4 files changed

+83
-16
lines changed

‎container_linux_test.go

+13-5
Original file line numberDiff line numberDiff line change
@@ -1040,7 +1040,7 @@ func TestContainerRuntimeOptionsv2(t *testing.T) {
10401040
}
10411041
}
10421042

1043-
func TestContainerKillInitPidHost(t *testing.T) {
1043+
func initContainerAndCheckChildrenDieOnKill(t *testing.T, opts ...oci.SpecOpts) {
10441044
client, err := newClient(t, address)
10451045
if err != nil {
10461046
t.Fatal(err)
@@ -1059,12 +1059,12 @@ func TestContainerKillInitPidHost(t *testing.T) {
10591059
t.Fatal(err)
10601060
}
10611061

1062+
opts = append(opts, oci.WithImageConfig(image))
1063+
opts = append(opts, withProcessArgs("sh", "-c", "sleep 42; echo hi"))
1064+
10621065
container, err := client.NewContainer(ctx, id,
10631066
WithNewSnapshot(id, image),
1064-
WithNewSpec(oci.WithImageConfig(image),
1065-
withProcessArgs("sh", "-c", "sleep 42; echo hi"),
1066-
oci.WithHostNamespace(specs.PIDNamespace),
1067-
),
1067+
WithNewSpec(opts...),
10681068
)
10691069
if err != nil {
10701070
t.Fatal(err)
@@ -1111,6 +1111,14 @@ func TestContainerKillInitPidHost(t *testing.T) {
11111111
}
11121112
}
11131113

1114+
func TestContainerKillInitPidHost(t *testing.T) {
1115+
initContainerAndCheckChildrenDieOnKill(t, oci.WithHostNamespace(specs.PIDNamespace))
1116+
}
1117+
1118+
func TestContainerKillInitKillsChildWhenNotHostPid(t *testing.T) {
1119+
initContainerAndCheckChildrenDieOnKill(t)
1120+
}
1121+
11141122
func TestUserNamespaces(t *testing.T) {
11151123
t.Parallel()
11161124
t.Run("WritableRootFS", func(t *testing.T) { testUserNamespaces(t, false) })

‎runtime/v1/linux/proc/init.go

-1
Original file line numberDiff line numberDiff line change
@@ -249,7 +249,6 @@ func (p *Init) setExited(status int) {
249249
}
250250

251251
func (p *Init) delete(context context.Context) error {
252-
p.KillAll(context)
253252
p.wg.Wait()
254253
err := p.runtime.Delete(context, p.id, nil)
255254
// ignore errors if a runtime has already deleted the process

‎runtime/v1/shim/service.go

+36-5
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,9 @@ package shim
2020

2121
import (
2222
"context"
23+
"encoding/json"
2324
"fmt"
25+
"io/ioutil"
2426
"os"
2527
"path/filepath"
2628
"sync"
@@ -41,6 +43,7 @@ import (
4143
runc "github.com/containerd/go-runc"
4244
"github.com/containerd/typeurl"
4345
ptypes "github.com/gogo/protobuf/types"
46+
specs "github.com/opencontainers/runtime-spec/specs-go"
4447
"github.com/pkg/errors"
4548
"github.com/sirupsen/logrus"
4649
"google.golang.org/grpc/codes"
@@ -507,13 +510,22 @@ func (s *Service) processExits() {
507510
func (s *Service) checkProcesses(e runc.Exit) {
508511
s.mu.Lock()
509512
defer s.mu.Unlock()
513+
514+
shouldKillAll, err := shouldKillAllOnExit(s.bundle)
515+
if err != nil {
516+
log.G(s.context).WithError(err).Error("failed to check shouldKillAll")
517+
}
518+
510519
for _, p := range s.processes {
511520
if p.Pid() == e.Pid {
512-
if ip, ok := p.(*proc.Init); ok {
513-
// Ensure all children are killed
514-
if err := ip.KillAll(s.context); err != nil {
515-
log.G(s.context).WithError(err).WithField("id", ip.ID()).
516-
Error("failed to kill init's children")
521+
522+
if shouldKillAll {
523+
if ip, ok := p.(*proc.Init); ok {
524+
// Ensure all children are killed
525+
if err := ip.KillAll(s.context); err != nil {
526+
log.G(s.context).WithError(err).WithField("id", ip.ID()).
527+
Error("failed to kill init's children")
528+
}
517529
}
518530
}
519531
p.SetExited(e.Status)
@@ -529,6 +541,25 @@ func (s *Service) checkProcesses(e runc.Exit) {
529541
}
530542
}
531543

544+
func shouldKillAllOnExit(bundlePath string) (bool, error) {
545+
var bundleSpec specs.Spec
546+
bundleConfigContents, err := ioutil.ReadFile(filepath.Join(bundlePath, "config.json"))
547+
if err != nil {
548+
return false, err
549+
}
550+
json.Unmarshal(bundleConfigContents, &bundleSpec)
551+
552+
if bundleSpec.Linux != nil {
553+
for _, ns := range bundleSpec.Linux.Namespaces {
554+
if ns.Type == specs.PIDNamespace {
555+
return false, nil
556+
}
557+
}
558+
}
559+
560+
return true, nil
561+
}
562+
532563
func (s *Service) getContainerPids(ctx context.Context, id string) ([]uint32, error) {
533564
s.mu.Lock()
534565
defer s.mu.Unlock()

‎runtime/v2/runc/service.go

+34-5
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ package runc
2020

2121
import (
2222
"context"
23+
"encoding/json"
2324
"io/ioutil"
2425
"os"
2526
"os/exec"
@@ -34,6 +35,7 @@ import (
3435
"github.com/containerd/containerd/api/types/task"
3536
"github.com/containerd/containerd/errdefs"
3637
"github.com/containerd/containerd/events"
38+
"github.com/containerd/containerd/log"
3739
"github.com/containerd/containerd/mount"
3840
"github.com/containerd/containerd/namespaces"
3941
"github.com/containerd/containerd/runtime"
@@ -45,6 +47,7 @@ import (
4547
runcC "github.com/containerd/go-runc"
4648
"github.com/containerd/typeurl"
4749
ptypes "github.com/gogo/protobuf/types"
50+
specs "github.com/opencontainers/runtime-spec/specs-go"
4851
"github.com/pkg/errors"
4952
"github.com/sirupsen/logrus"
5053
"golang.org/x/sys/unix"
@@ -638,13 +641,20 @@ func (s *service) processExits() {
638641
}
639642

640643
func (s *service) checkProcesses(e runcC.Exit) {
644+
shouldKillAll, err := shouldKillAllOnExit(s.bundle)
645+
if err != nil {
646+
log.G(s.context).WithError(err).Error("failed to check shouldKillAll")
647+
}
648+
641649
for _, p := range s.allProcesses() {
642650
if p.Pid() == e.Pid {
643-
if ip, ok := p.(*proc.Init); ok {
644-
// Ensure all children are killed
645-
if err := ip.KillAll(s.context); err != nil {
646-
logrus.WithError(err).WithField("id", ip.ID()).
647-
Error("failed to kill init's children")
651+
if shouldKillAll {
652+
if ip, ok := p.(*proc.Init); ok {
653+
// Ensure all children are killed
654+
if err := ip.KillAll(s.context); err != nil {
655+
logrus.WithError(err).WithField("id", ip.ID()).
656+
Error("failed to kill init's children")
657+
}
648658
}
649659
}
650660
p.SetExited(e.Status)
@@ -660,6 +670,25 @@ func (s *service) checkProcesses(e runcC.Exit) {
660670
}
661671
}
662672

673+
func shouldKillAllOnExit(bundlePath string) (bool, error) {
674+
var bundleSpec specs.Spec
675+
bundleConfigContents, err := ioutil.ReadFile(filepath.Join(bundlePath, "config.json"))
676+
if err != nil {
677+
return false, err
678+
}
679+
json.Unmarshal(bundleConfigContents, &bundleSpec)
680+
681+
if bundleSpec.Linux != nil {
682+
for _, ns := range bundleSpec.Linux.Namespaces {
683+
if ns.Type == specs.PIDNamespace {
684+
return false, nil
685+
}
686+
}
687+
}
688+
689+
return true, nil
690+
}
691+
663692
func (s *service) allProcesses() (o []rproc.Process) {
664693
s.mu.Lock()
665694
defer s.mu.Unlock()

0 commit comments

Comments
 (0)