From 9f60034de2681fa2b78ac2569e478cccd8bc00de Mon Sep 17 00:00:00 2001 From: Julio Montes Date: Tue, 3 Apr 2018 16:09:44 -0500 Subject: [PATCH 1/2] reaper: implement reaper interface All reaper implementations must override its functions. This new interface allows the creation of mock reapers that are useful for unit testing. Signed-off-by: Julio Montes --- agent.go | 9 +++++---- grpc.go | 4 ++-- reaper.go | 38 +++++++++++++++++++++++++++++++------- 3 files changed, 38 insertions(+), 13 deletions(-) diff --git a/agent.go b/agent.go index 109b2b7eeb..6881d4f31c 100644 --- a/agent.go +++ b/agent.go @@ -69,7 +69,7 @@ type sandbox struct { grpcListener net.Listener sharedPidNs namespace mounts []string - subreaper *reaper + subreaper reaper } type namespace struct { @@ -575,6 +575,9 @@ func main() { os.Exit(exitSuccess) }() + r := &agentReaper{} + r.init() + // Initialize unique sandbox structure. s := &sandbox{ containers: make(map[string]*container), @@ -582,9 +585,7 @@ func main() { // pivot_root won't work for init, see // Documention/filesystem/ramfs-rootfs-initramfs.txt noPivotRoot: os.Getpid() == 1, - subreaper: &reaper{ - exitCodeChans: make(map[int]chan<- int), - }, + subreaper: r, } if err = s.initLogger(); err != nil { diff --git a/grpc.go b/grpc.go index 54907641f7..028c301d94 100644 --- a/grpc.go +++ b/grpc.go @@ -206,8 +206,8 @@ func (a *agentGRPC) execProcess(ctr *container, proc *process, createContainer b // miss the opportunity to get the exit code, leading WaitProcess() to // wait forever on the new channel. // This lock has to be taken before we run the new process. - a.sandbox.subreaper.RLock() - defer a.sandbox.subreaper.RUnlock() + a.sandbox.subreaper.lock() + defer a.sandbox.subreaper.unlock() if createContainer { err = ctr.container.Start(&proc.process) diff --git a/reaper.go b/reaper.go index 12ad0f70f0..5289c8d3a4 100644 --- a/reaper.go +++ b/reaper.go @@ -18,7 +18,19 @@ import ( grpcStatus "google.golang.org/grpc/status" ) -type reaper struct { +type reaper interface { + init() + getExitCodeCh(pid int) (chan<- int, error) + setExitCodeCh(pid int, exitCodeCh chan<- int) + deleteExitCodeCh(pid int) + reap() error + start(c *exec.Cmd) (<-chan int, error) + wait(exitCodeCh <-chan int, proc waitProcess) (int, error) + lock() + unlock() +} + +type agentReaper struct { sync.RWMutex chansLock sync.RWMutex @@ -33,7 +45,19 @@ func exitStatus(status unix.WaitStatus) int { return status.ExitStatus() } -func (r *reaper) getExitCodeCh(pid int) (chan<- int, error) { +func (r *agentReaper) init() { + r.exitCodeChans = make(map[int]chan<- int) +} + +func (r *agentReaper) lock() { + r.RLock() +} + +func (r *agentReaper) unlock() { + r.RUnlock() +} + +func (r *agentReaper) getExitCodeCh(pid int) (chan<- int, error) { r.chansLock.RLock() defer r.chansLock.RUnlock() @@ -45,21 +69,21 @@ func (r *reaper) getExitCodeCh(pid int) (chan<- int, error) { return exitCodeCh, nil } -func (r *reaper) setExitCodeCh(pid int, exitCodeCh chan<- int) { +func (r *agentReaper) setExitCodeCh(pid int, exitCodeCh chan<- int) { r.chansLock.Lock() defer r.chansLock.Unlock() r.exitCodeChans[pid] = exitCodeCh } -func (r *reaper) deleteExitCodeCh(pid int) { +func (r *agentReaper) deleteExitCodeCh(pid int) { r.chansLock.Lock() defer r.chansLock.Unlock() delete(r.exitCodeChans, pid) } -func (r *reaper) reap() error { +func (r *agentReaper) reap() error { var ( ws unix.WaitStatus rus unix.Rusage @@ -119,7 +143,7 @@ func (r *reaper) reap() error { // start starts the exec command and registers the process to the reaper. // This function is a helper for exec.Cmd.Start() since this needs to be // in sync with exec.Cmd.Wait(). -func (r *reaper) start(c *exec.Cmd) (<-chan int, error) { +func (r *agentReaper) start(c *exec.Cmd) (<-chan int, error) { // This lock is very important to avoid any race with reaper.reap(). // We don't want the reaper to reap a process before we have added // it to the exit code channel list. @@ -143,7 +167,7 @@ func (r *reaper) start(c *exec.Cmd) (<-chan int, error) { // from the subreaper, the exit code is sent through the provided channel. // This function is a helper for exec.Cmd.Wait() and os.Process.Wait() since // both cannot be used directly, because of the subreaper. -func (r *reaper) wait(exitCodeCh <-chan int, proc waitProcess) (int, error) { +func (r *agentReaper) wait(exitCodeCh <-chan int, proc waitProcess) (int, error) { // Wait for the subreaper to receive the SIGCHLD signal. Once it gets // it, this channel will be notified by receiving the exit code of the // corresponding process. From ee7850d766c22ae22b0bbac3848f8d1669af729b Mon Sep 17 00:00:00 2001 From: Julio Montes Date: Wed, 4 Apr 2018 09:33:22 -0500 Subject: [PATCH 2/2] mockreaper: implement mock reaper mockreaper is a helpful struct to write unit tests fixes #208 Signed-off-by: Julio Montes --- mockreaper.go | 43 +++++++++++++++++++++++++++++ mockreaper_test.go | 69 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 112 insertions(+) create mode 100644 mockreaper.go create mode 100644 mockreaper_test.go diff --git a/mockreaper.go b/mockreaper.go new file mode 100644 index 0000000000..67178d61f3 --- /dev/null +++ b/mockreaper.go @@ -0,0 +1,43 @@ +// +// Copyright (c) 2018 Intel Corporation +// +// SPDX-License-Identifier: Apache-2.0 +// + +package main + +import "os/exec" + +type mockreaper struct { +} + +func (r *mockreaper) init() { +} + +func (r *mockreaper) getExitCodeCh(pid int) (chan<- int, error) { + return nil, nil +} + +func (r *mockreaper) setExitCodeCh(pid int, exitCodeCh chan<- int) { +} + +func (r *mockreaper) deleteExitCodeCh(pid int) { +} + +func (r *mockreaper) reap() error { + return nil +} + +func (r *mockreaper) start(c *exec.Cmd) (<-chan int, error) { + return nil, nil +} + +func (r *mockreaper) wait(exitCodeCh <-chan int, proc waitProcess) (int, error) { + return 0, nil +} + +func (r *mockreaper) lock() { +} + +func (r *mockreaper) unlock() { +} diff --git a/mockreaper_test.go b/mockreaper_test.go new file mode 100644 index 0000000000..a2eb61875f --- /dev/null +++ b/mockreaper_test.go @@ -0,0 +1,69 @@ +// +// Copyright (c) 2018 Intel Corporation +// +// SPDX-License-Identifier: Apache-2.0 +// + +package main + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestMockReaperInit(t *testing.T) { + m := &mockreaper{} + m.init() +} + +func TestMockReaperGetExitCodeCh(t *testing.T) { + assert := assert.New(t) + m := &mockreaper{} + c, e := m.getExitCodeCh(0) + assert.Nil(c) + assert.NoError(e) +} + +func TestMockReaperSetExitCodeCh(t *testing.T) { + m := &mockreaper{} + m.setExitCodeCh(0, nil) +} + +func TestMockReaperDeleteExitCodeCh(t *testing.T) { + m := &mockreaper{} + m.deleteExitCodeCh(0) +} + +func TestMockReaperReap(t *testing.T) { + assert := assert.New(t) + m := &mockreaper{} + err := m.reap() + assert.NoError(err) +} + +func TestMockReaperStart(t *testing.T) { + assert := assert.New(t) + m := &mockreaper{} + c, e := m.start(nil) + assert.Nil(c) + assert.NoError(e) +} + +func TestMockReaperWait(t *testing.T) { + assert := assert.New(t) + m := &mockreaper{} + e, err := m.wait(nil, &reaperOSProcess{}) + assert.Equal(0, e) + assert.NoError(err) +} + +func TestMockReaperLock(t *testing.T) { + m := &mockreaper{} + m.lock() +} + +func TestMockReaperUnlock(t *testing.T) { + m := &mockreaper{} + m.unlock() +}