Extract container management into interface
Overview
At the moment executor/docker
is responsible for handling the life cycle of the containers and interacting directly with the docker client, plus other thinks like network management. This is fairly messy and when you need to change behavior it's quite hard to find out what needs to be changed and how, and can lead to some regression. This is blocking important features like #1042 (closed).
Proposal
Extract the container management into its own interface, similar to what !1261 (merged) did for volumes. This interface will be responsible for starting/stoping/killing/waiting for containers. The main goal of executor/docker
should be that it's just an orchestrator of multiple interfaces (volume/container/network) and it ties everything together, but the actual implementation is all abstracted into interfaces. We cab have a new interface Container
where it has methods like Labels
, Wait
, Kill
, Start
and then have the cacheContainerManager interface as a dependency, instead of creating a new containerClient
interface. An implementation of this interface can be using the Docker.
The above proposal will solve all the discussions below:
Original Follow Up description
The following discussions from !1261 (merged) should be addressed:
-
@steveazz started a discussion: (+2 comments) This interface is mostly a duplicate of Client interface, with the exception of
LabelContainer
, which I don't it's a step in the right direction if we update the docker client we have to look at two places to update the signature. I think it's better if remove this interface entirely. HavedefaultContainerManager
us the Client interface instead and extendscontainerManager
to have a methodDefaultLabels
The way we can implement this is by creating a new interface
Container
where it has methods likeLabels
,Wait
,Kill
,Start
and then have the cacheContainerManager interface as a dependency, instead of creating a newcontainerClient
interface. An implementation of this interface can be using the Docker. -
@ayufan started a discussion: (+1 comment) I don't think that we need it after recent changes, as we do not distuinguish
builds_dir
. I would expect that we have single interface for all volume creations, that accepts a volume specification from/builds
, and provides an error message if such volume is already created, for example, to discover if such volume is shared or not (it seems that we abuse it today).type Scope int const ( ScopeGlobal ScopeLocal ) type Manager interface { Create(scope Scope, volumeSpec string) error ContainerIDs() []string Binds() []string # or whatever format it is Cleanup() }
I would propose
volumeSpec
to support syntax something likenone:/cache
to create ephemeral volumes, and follow the same convention as creating regular volumes. It would create a disk-based ephemeral volume. The same could be applied to usetmpfs:/cache
.One example is
/builds
folder. We can have an overrided/builds
, so we could:for _, volumeSpec := range e.RunnerConfig.Docker.Volumes { err := e.volumeManager.CreateVolume(volumeSpec) if err != nil { # do something with error } } err := e.volumeManager.CreateVolume(e.RootDir) if err == ErrAlreadyCreated { SharedBuildsDir = true } else { # do something with error }
I would also expect that interface for this class would accept a set of parameters with all labels and filters and do not care about actual paths, but rather use that to figure out the affinity. Then it would make it possible to have a generic interface for volume management to live in
helpers/docker/volumes
, and use simple interface, or something like that:NewWindowsVolumeManager(client DockerClient, globalScope, localScope string, labels []string, dynamicVolumeImageID, dynamicVolumeImageCommand string) # globalScope: the container or volume name when using globalScope # localScope: the container or volume name when using localScope
I'm worried that current interface and implementation takes a crazy amount of assumptions about system behavior, I would rather make this as generic as possible, it seems possible.
-
@ayufan started a discussion: (+1 comment) Can we make this part of
docker_helpers.Client
?This is part of the first discussion
-
@ayufan started a discussion: (+1 comment) Can we make this part of
docker_helpers.Client
?This is part of the first discussion
Blocking
This issue is blocking issues like #1042 (closed)