Skip to content

Commit 4bafaa0

Browse files
corherecorhere
committed
Refactor libcontainerd to minimize c8d RPCs
The containerd client is very chatty at the best of times. Because the libcontained API is stateless and references containers and processes by string ID for every method call, the implementation is essentially forced to use the containerd client in a way which amplifies the number of redundant RPCs invoked to perform any operation. The libcontainerd remote implementation has to reload the containerd container, task and/or process metadata for nearly every operation. This in turn amplifies the number of context switches between dockerd and containerd to perform any container operation or handle a containerd event, increasing the load on the system which could otherwise be allocated to workloads. Overhaul the libcontainerd interface to reduce the impedance mismatch with the containerd client so that the containerd client can be used more efficiently. Split the API out into container, task and process interfaces which the consumer is expected to retain so that libcontainerd can retain state---especially the analogous containerd client objects---without having to manage any state-store inside the libcontainerd client. Signed-off-by: Cory Snider <[email protected]>
1 parent 57d2d6e commit 4bafaa0

36 files changed

+1156
-1111
lines changed

‎container/container.go‎

Lines changed: 44 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@ import (
1919
mounttypes "github.com/docker/docker/api/types/mount"
2020
swarmtypes "github.com/docker/docker/api/types/swarm"
2121
"github.com/docker/docker/container/stream"
22-
"github.com/docker/docker/daemon/exec"
2322
"github.com/docker/docker/daemon/logger"
2423
"github.com/docker/docker/daemon/logger/jsonfilelog"
2524
"github.com/docker/docker/daemon/logger/local"
@@ -28,6 +27,7 @@ import (
2827
"github.com/docker/docker/errdefs"
2928
"github.com/docker/docker/image"
3029
"github.com/docker/docker/layer"
30+
libcontainerdtypes "github.com/docker/docker/libcontainerd/types"
3131
"github.com/docker/docker/pkg/containerfs"
3232
"github.com/docker/docker/pkg/idtools"
3333
"github.com/docker/docker/pkg/ioutils"
@@ -86,7 +86,7 @@ type Container struct {
8686
HasBeenManuallyRestarted bool `json:"-"` // used to distinguish restart caused by restart policy from the manual one
8787
MountPoints map[string]*volumemounts.MountPoint
8888
HostConfig *containertypes.HostConfig `json:"-"` // do not serialize the host config in the json, otherwise we'll make the container unportable
89-
ExecCommands *exec.Store `json:"-"`
89+
ExecCommands *ExecStore `json:"-"`
9090
DependencyStore agentexec.DependencyGetter `json:"-"`
9191
SecretReferences []*swarmtypes.SecretReference
9292
ConfigReferences []*swarmtypes.ConfigReference
@@ -121,7 +121,7 @@ func NewBaseContainer(id, root string) *Container {
121121
return &Container{
122122
ID: id,
123123
State: NewState(),
124-
ExecCommands: exec.NewStore(),
124+
ExecCommands: NewExecStore(),
125125
Root: root,
126126
MountPoints: make(map[string]*volumemounts.MountPoint),
127127
StreamConfig: stream.NewConfig(),
@@ -752,6 +752,47 @@ func (container *Container) CreateDaemonEnvironment(tty bool, linkedEnv []string
752752
return env
753753
}
754754

755+
// RestoreTask restores the containerd container and task handles and reattaches
756+
// the IO for the running task. Container state is not synced with containerd's
757+
// state.
758+
//
759+
// An errdefs.NotFound error is returned if the container does not exist in
760+
// containerd. However, a nil error is returned if the task does not exist in
761+
// containerd.
762+
func (container *Container) RestoreTask(ctx context.Context, client libcontainerdtypes.Client) error {
763+
container.Lock()
764+
defer container.Unlock()
765+
var err error
766+
container.ctr, err = client.LoadContainer(ctx, container.ID)
767+
if err != nil {
768+
return err
769+
}
770+
container.task, err = container.ctr.AttachTask(ctx, container.InitializeStdio)
771+
if err != nil && !errdefs.IsNotFound(err) {
772+
return err
773+
}
774+
return nil
775+
}
776+
777+
// GetRunningTask asserts that the container is running and returns the Task for
778+
// the container. An errdefs.Conflict error is returned if the container is not
779+
// in the Running state.
780+
//
781+
// A system error is returned if container is in a bad state: Running is true
782+
// but has a nil Task.
783+
//
784+
// The container lock must be held when calling this method.
785+
func (container *Container) GetRunningTask() (libcontainerdtypes.Task, error) {
786+
if !container.Running {
787+
return nil, errdefs.Conflict(fmt.Errorf("container %s is not running", container.ID))
788+
}
789+
tsk, ok := container.Task()
790+
if !ok {
791+
return nil, errdefs.System(errors.WithStack(fmt.Errorf("container %s is in Running state but has no containerd Task set", container.ID)))
792+
}
793+
return tsk, nil
794+
}
795+
755796
type rio struct {
756797
cio.IO
757798

daemon/exec/exec.go renamed to container/exec.go

Lines changed: 26 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -1,20 +1,20 @@
1-
package exec // import "github.com/docker/docker/daemon/exec"
1+
package container // import "github.com/docker/docker/container"
22

33
import (
4-
"context"
54
"runtime"
65
"sync"
76

87
"github.com/containerd/containerd/cio"
98
"github.com/docker/docker/container/stream"
9+
"github.com/docker/docker/libcontainerd/types"
1010
"github.com/docker/docker/pkg/stringid"
1111
"github.com/sirupsen/logrus"
1212
)
1313

14-
// Config holds the configurations for execs. The Daemon keeps
14+
// ExecConfig holds the configurations for execs. The Daemon keeps
1515
// track of both running and finished execs so that they can be
1616
// examined both during and after completion.
17-
type Config struct {
17+
type ExecConfig struct {
1818
sync.Mutex
1919
Started chan struct{}
2020
StreamConfig *stream.Config
@@ -25,7 +25,7 @@ type Config struct {
2525
OpenStderr bool
2626
OpenStdout bool
2727
CanRemove bool
28-
ContainerID string
28+
Container *Container
2929
DetachKeys []byte
3030
Entrypoint string
3131
Args []string
@@ -34,39 +34,22 @@ type Config struct {
3434
User string
3535
WorkingDir string
3636
Env []string
37-
Pid int
37+
Process types.Process
3838
ConsoleSize *[2]uint
3939
}
4040

41-
// NewConfig initializes the a new exec configuration
42-
func NewConfig() *Config {
43-
return &Config{
41+
// NewExecConfig initializes the a new exec configuration
42+
func NewExecConfig(c *Container) *ExecConfig {
43+
return &ExecConfig{
4444
ID: stringid.GenerateRandomID(),
45+
Container: c,
4546
StreamConfig: stream.NewConfig(),
4647
Started: make(chan struct{}),
4748
}
4849
}
4950

50-
type rio struct {
51-
cio.IO
52-
53-
sc *stream.Config
54-
}
55-
56-
func (i *rio) Close() error {
57-
i.IO.Close()
58-
59-
return i.sc.CloseStreams()
60-
}
61-
62-
func (i *rio) Wait() {
63-
i.sc.Wait(context.Background())
64-
65-
i.IO.Wait()
66-
}
67-
6851
// InitializeStdio is called by libcontainerd to connect the stdio.
69-
func (c *Config) InitializeStdio(iop *cio.DirectIO) (cio.IO, error) {
52+
func (c *ExecConfig) InitializeStdio(iop *cio.DirectIO) (cio.IO, error) {
7053
c.StreamConfig.CopyToPipe(iop)
7154

7255
if c.StreamConfig.Stdin() == nil && !c.Tty && runtime.GOOS == "windows" {
@@ -81,32 +64,32 @@ func (c *Config) InitializeStdio(iop *cio.DirectIO) (cio.IO, error) {
8164
}
8265

8366
// CloseStreams closes the stdio streams for the exec
84-
func (c *Config) CloseStreams() error {
67+
func (c *ExecConfig) CloseStreams() error {
8568
return c.StreamConfig.CloseStreams()
8669
}
8770

8871
// SetExitCode sets the exec config's exit code
89-
func (c *Config) SetExitCode(code int) {
72+
func (c *ExecConfig) SetExitCode(code int) {
9073
c.ExitCode = &code
9174
}
9275

93-
// Store keeps track of the exec configurations.
94-
type Store struct {
95-
byID map[string]*Config
76+
// ExecStore keeps track of the exec configurations.
77+
type ExecStore struct {
78+
byID map[string]*ExecConfig
9679
mu sync.RWMutex
9780
}
9881

99-
// NewStore initializes a new exec store.
100-
func NewStore() *Store {
101-
return &Store{
102-
byID: make(map[string]*Config),
82+
// NewExecStore initializes a new exec store.
83+
func NewExecStore() *ExecStore {
84+
return &ExecStore{
85+
byID: make(map[string]*ExecConfig),
10386
}
10487
}
10588

10689
// Commands returns the exec configurations in the store.
107-
func (e *Store) Commands() map[string]*Config {
90+
func (e *ExecStore) Commands() map[string]*ExecConfig {
10891
e.mu.RLock()
109-
byID := make(map[string]*Config, len(e.byID))
92+
byID := make(map[string]*ExecConfig, len(e.byID))
11093
for id, config := range e.byID {
11194
byID[id] = config
11295
}
@@ -115,29 +98,29 @@ func (e *Store) Commands() map[string]*Config {
11598
}
11699

117100
// Add adds a new exec configuration to the store.
118-
func (e *Store) Add(id string, Config *Config) {
101+
func (e *ExecStore) Add(id string, Config *ExecConfig) {
119102
e.mu.Lock()
120103
e.byID[id] = Config
121104
e.mu.Unlock()
122105
}
123106

124107
// Get returns an exec configuration by its id.
125-
func (e *Store) Get(id string) *Config {
108+
func (e *ExecStore) Get(id string) *ExecConfig {
126109
e.mu.RLock()
127110
res := e.byID[id]
128111
e.mu.RUnlock()
129112
return res
130113
}
131114

132115
// Delete removes an exec configuration from the store.
133-
func (e *Store) Delete(id string, pid int) {
116+
func (e *ExecStore) Delete(id string) {
134117
e.mu.Lock()
135118
delete(e.byID, id)
136119
e.mu.Unlock()
137120
}
138121

139122
// List returns the list of exec ids in the store.
140-
func (e *Store) List() []string {
123+
func (e *ExecStore) List() []string {
141124
var IDs []string
142125
e.mu.RLock()
143126
for id := range e.byID {

‎container/state.go‎

Lines changed: 35 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import (
88
"time"
99

1010
"github.com/docker/docker/api/types"
11+
libcontainerdtypes "github.com/docker/docker/libcontainerd/types"
1112
units "github.com/docker/go-units"
1213
)
1314

@@ -36,6 +37,14 @@ type State struct {
3637

3738
stopWaiters []chan<- StateStatus
3839
removeOnlyWaiters []chan<- StateStatus
40+
41+
// The libcontainerd reference fields are unexported to force consumers
42+
// to access them through the getter methods with multi-valued returns
43+
// so that they can't forget to nil-check: the code won't compile unless
44+
// the nil-check result is explicitly consumed or discarded.
45+
46+
ctr libcontainerdtypes.Container
47+
task libcontainerdtypes.Task
3948
}
4049

4150
// StateStatus is used to return container wait results.
@@ -260,7 +269,7 @@ func (s *State) SetExitCode(ec int) {
260269
}
261270

262271
// SetRunning sets the state of the container to "running".
263-
func (s *State) SetRunning(pid int, initial bool) {
272+
func (s *State) SetRunning(ctr libcontainerdtypes.Container, tsk libcontainerdtypes.Task, initial bool) {
264273
s.ErrorMsg = ""
265274
s.Paused = false
266275
s.Running = true
@@ -269,7 +278,13 @@ func (s *State) SetRunning(pid int, initial bool) {
269278
s.Paused = false
270279
}
271280
s.ExitCodeValue = 0
272-
s.Pid = pid
281+
s.ctr = ctr
282+
s.task = tsk
283+
if tsk != nil {
284+
s.Pid = int(tsk.Pid())
285+
} else {
286+
s.Pid = 0
287+
}
273288
s.OOMKilled = false
274289
if initial {
275290
s.StartedAt = time.Now().UTC()
@@ -404,3 +419,21 @@ func (s *State) notifyAndClear(waiters *[]chan<- StateStatus) {
404419
}
405420
*waiters = nil
406421
}
422+
423+
// C8dContainer returns a reference to the libcontainerd Container object for
424+
// the container and whether the reference is valid.
425+
//
426+
// The container lock must be held when calling this method.
427+
func (s *State) C8dContainer() (_ libcontainerdtypes.Container, ok bool) {
428+
return s.ctr, s.ctr != nil
429+
}
430+
431+
// Task returns a reference to the libcontainerd Task object for the container
432+
// and whether the reference is valid.
433+
//
434+
// The container lock must be held when calling this method.
435+
//
436+
// See also: (*Container).GetRunningTask().
437+
func (s *State) Task() (_ libcontainerdtypes.Task, ok bool) {
438+
return s.task, s.task != nil
439+
}

‎container/state_test.go‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"time"
77

88
"github.com/docker/docker/api/types"
9+
libcontainerdtypes "github.com/docker/docker/libcontainerd/types"
910
)
1011

1112
func TestIsValidHealthString(t *testing.T) {
@@ -28,6 +29,13 @@ func TestIsValidHealthString(t *testing.T) {
2829
}
2930
}
3031

32+
type mockTask struct {
33+
libcontainerdtypes.Task
34+
pid uint32
35+
}
36+
37+
func (t *mockTask) Pid() uint32 { return t.pid }
38+
3139
func TestStateRunStop(t *testing.T) {
3240
s := NewState()
3341

@@ -60,7 +68,7 @@ func TestStateRunStop(t *testing.T) {
6068

6169
// Set the state to "Running".
6270
s.Lock()
63-
s.SetRunning(i, true)
71+
s.SetRunning(nil, &mockTask{pid: uint32(i)}, true)
6472
s.Unlock()
6573

6674
// Assert desired state.
@@ -125,7 +133,7 @@ func TestStateTimeoutWait(t *testing.T) {
125133
s := NewState()
126134

127135
s.Lock()
128-
s.SetRunning(0, true)
136+
s.SetRunning(nil, nil, true)
129137
s.Unlock()
130138

131139
// Start a wait with a timeout.
@@ -174,7 +182,7 @@ func TestCorrectStateWaitResultAfterRestart(t *testing.T) {
174182
s := NewState()
175183

176184
s.Lock()
177-
s.SetRunning(0, true)
185+
s.SetRunning(nil, nil, true)
178186
s.Unlock()
179187

180188
waitC := s.Wait(context.Background(), WaitConditionNotRunning)
@@ -185,7 +193,7 @@ func TestCorrectStateWaitResultAfterRestart(t *testing.T) {
185193
s.Unlock()
186194

187195
s.Lock()
188-
s.SetRunning(0, true)
196+
s.SetRunning(nil, nil, true)
189197
s.Unlock()
190198

191199
got := <-waitC

‎daemon/checkpoint.go‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -57,8 +57,11 @@ func (daemon *Daemon) CheckpointCreate(name string, config types.CheckpointCreat
5757
return err
5858
}
5959

60-
if !container.IsRunning() {
61-
return fmt.Errorf("Container %s not running", name)
60+
container.Lock()
61+
tsk, err := container.GetRunningTask()
62+
container.Unlock()
63+
if err != nil {
64+
return err
6265
}
6366

6467
if !validCheckpointNamePattern.MatchString(config.CheckpointID) {
@@ -70,7 +73,7 @@ func (daemon *Daemon) CheckpointCreate(name string, config types.CheckpointCreat
7073
return fmt.Errorf("cannot checkpoint container %s: %s", name, err)
7174
}
7275

73-
err = daemon.containerd.CreateCheckpoint(context.Background(), container.ID, checkpointDir, config.Exit)
76+
err = tsk.CreateCheckpoint(context.Background(), checkpointDir, config.Exit)
7477
if err != nil {
7578
os.RemoveAll(checkpointDir)
7679
return fmt.Errorf("Cannot checkpoint container %s: %s", name, err)

0 commit comments

Comments
 (0)