Skip to content
This repository has been archived by the owner on Dec 7, 2023. It is now read-only.

Create/use a runtime interface instead of direct calls to Docker #211

Merged
merged 12 commits into from
Jul 18, 2019
Merged

Create/use a runtime interface instead of direct calls to Docker #211

merged 12 commits into from
Jul 18, 2019

Conversation

twelho
Copy link
Contributor

@twelho twelho commented Jul 17, 2019 •
edited
Loading

This removes all direct calls to the docker binary in favor of the pkg/runtime interface.

  • pkg refactored
  • cmd refactored (excluding attach)
  • Global providers
  • Rebased on top of master

@luxas luxas self-assigned this Jul 17, 2019
@luxas luxas added the do-not-merge/wip The PR is still work in progress label Jul 17, 2019
luxas
luxas reviewed Jul 18, 2019
View reviewed changes
Copy link
Contributor

@luxas luxas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good direction! some small comments
lmk when it's ready to review (for real)

pkg/operations/start.go Outdated
@@ -20,8 +21,14 @@ import (
)

func StartVM(vm *vmmd.VM, debug bool) error {
// Get the Docker client
dc, err := docker.GetDockerClient()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make all these calls use the global provider

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

pkg/runtime/docker/client.go Outdated
StdinOnce: false,
Env: nil,
Cmd: nil,
Healthcheck: nil,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove the unset options here to make sure the default values are used, and make it explicit what we set

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, they were just temporarily there while I figured out which ones to set.

pkg/source/docker.go Outdated
reader, err := ds.exportCmd.StdoutPipe()
func (ds *DockerSource) Reader() (rc io.ReadCloser, err error) {
// Get the Docker client
dc, err := docker.GetDockerClient()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

global provider

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@luxas luxas added this to the v0.5.0 milestone Jul 18, 2019
@twelho
Copy link
Contributor Author

twelho commented Jul 18, 2019

Docker client's ContainerAttach and its HijackedResponse have quite lacking documentation, it seems like we need to implement a pseudo-TTY from scrach with signal handling, which is out of scope for this refactor. https://github.com/fsouza/go-dockerclient has an implementation for attach stream hijacking, but we'll stick to calling docker attach for now.

luxas
luxas approved these changes Jul 18, 2019
View reviewed changes
Copy link
Contributor

@luxas luxas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This looks really good 👍
Rebase, fix the nits and we're ready to go!

cmd/ignite/ignite.go Outdated Show resolved Hide resolved
cmd/ignite-spawn/ignite-spawn.go Outdated Show resolved Hide resolved
cmd/ignite/run/create.go Outdated Show resolved Hide resolved
cmd/ignite/run/rmi.go Outdated Show resolved Hide resolved
cmd/ignite/run/rm.go Outdated Show resolved Hide resolved
pkg/providers/network.go
var NetworkPlugin cni.NetworkPlugin

func SetCNINetworkPlugin() (err error) {
NetworkPlugin, err = cni.GetCNINetworkPlugin(Runtime)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is ok, but a bit misleading, as by default we don't use this provider in the codepath at all

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's currently only used on one place when using CNI networking, but at least it has its own provider now. Might rename it later if it causes extra confusion.

pkg/source/docker.go Outdated Show resolved Hide resolved
pkg/runtime/types.go Outdated
type Interface interface {
InspectImage(image string) (*ImageInspectResult, error)
PullImage(image string) (io.ReadCloser, error)
ExportImage(image string) (io.ReadCloser, string, error)
GetNetNS(containerID string) (string, error)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rename this to GetContainerNetns() and move to the container section?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 6e60eba.

pkg/runtime/types.go Outdated
type Interface interface {
InspectImage(image string) (*ImageInspectResult, error)
PullImage(image string) (io.ReadCloser, error)
ExportImage(image string) (io.ReadCloser, string, error)
GetNetNS(containerID string) (string, error)
RawClient() interface{}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now that we do grouping here, move this last in its own section

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 6e60eba.

pkg/runtime/types.go
GetNetNS(containerID string) (string, error)
RawClient() interface{}

RunContainer(image string, config *ContainerConfig, name string) (string, error)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add // TODO: AttachContainer()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 6e60eba.

@twelho
Merge branch 'master' into pkg-runtime, fix blank lines in all imports
6478c65 
# Conflicts:
#	cmd/ignite/run/create.go
#	cmd/ignite/run/images.go
#	cmd/ignite/run/inspect.go
#	cmd/ignite/run/kernels.go
#	cmd/ignite/run/ps.go
#	pkg/metadata/imgmd/imgmd.go
#	pkg/metadata/kernmd/kernmd.go
#	pkg/metadata/metadata.go
#	pkg/operations/start.go
#	pkg/source/docker.go
@twelho
Copy link
Contributor Author

twelho commented Jul 18, 2019

FYI: made this regex: import \([^\)]*\n\n[^\)]*\n\n[^\)]*^\) and fixed all excess blank lines in imports in the entire code in 6478c65.

@twelho twelho removed the do-not-merge/wip The PR is still work in progress label Jul 18, 2019
luxas
luxas approved these changes Jul 18, 2019
View reviewed changes
Copy link
Contributor

@luxas luxas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very well!

@luxas luxas merged commit 7c44a3e into weaveworks:master Jul 18, 2019
@luxas luxas mentioned this pull request Jul 18, 2019
Closed
@twelho twelho deleted the pkg-runtime branch July 19, 2019 10:45
@luxas luxas changed the title Refactor to use pkg/runtime instead of direct calls to Docker Create/use a runtime interface instead of direct calls to Docker Aug 6, 2019
@luxas luxas added the kind/enhancement Categorizes issue or PR as related to improving an existing feature. label Aug 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@luxas luxas luxas approved these changes

Assignees

@luxas luxas

Labels
kind/enhancement Categorizes issue or PR as related to improving an existing feature.
Projects
None yet
Milestone
v0.5.0
Development

Successfully merging this pull request may close these issues.
2 participants
@twelho @luxas