Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor image related functions to use docker engine-api #23800

Merged
merged 3 commits into from
Apr 24, 2016
Merged

Refactor image related functions to use docker engine-api #23800

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7ecb44f
Refactor list image to use new api
resouer Apr 3, 2016
3918eee
Refactor InspectImage method
resouer Apr 6, 2016
a393947
Refactor PullImage RemoveImage methods
resouer Apr 6, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 4 additions & 5 deletions pkg/credentialprovider/keyring.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,9 @@ import (
"sort"
"strings"

docker "github.com/fsouza/go-dockerclient"
"github.com/golang/glog"

dockertypes "github.com/docker/engine-api/types"
"k8s.io/kubernetes/pkg/api"
"k8s.io/kubernetes/pkg/util/sets"
)
Expand Down Expand Up @@ -54,17 +54,17 @@ type lazyDockerKeyring struct {
Providers []DockerConfigProvider
}

// LazyAuthConfiguration wraps AuthConfiguration, potentially deferring its
// LazyAuthConfiguration wraps dockertypes.AuthConfig, potentially deferring its
// binding. If Provider is non-nil, it will be used to obtain new credentials
// by calling LazyProvide() on it.
type LazyAuthConfiguration struct {
docker.AuthConfiguration
dockertypes.AuthConfig
Provider DockerConfigProvider
}

func DockerConfigEntryToLazyAuthConfiguration(ident DockerConfigEntry) LazyAuthConfiguration {
return LazyAuthConfiguration{
AuthConfiguration: docker.AuthConfiguration{
AuthConfig: dockertypes.AuthConfig{
Username: ident.Username,
Password: ident.Password,
Email: ident.Email,
Expand Down Expand Up @@ -291,7 +291,6 @@ type unionDockerKeyring struct {

func (k *unionDockerKeyring) Lookup(image string) ([]LazyAuthConfiguration, bool) {
authConfigs := []LazyAuthConfiguration{}

for _, subKeyring := range k.keyrings {
if subKeyring == nil {
continue
Expand Down
8 changes: 4 additions & 4 deletions pkg/credentialprovider/provider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import (
"sync"
"time"

docker "github.com/fsouza/go-dockerclient"
dockertypes "github.com/docker/engine-api/types"
"github.com/golang/glog"
)

Expand All @@ -36,12 +36,12 @@ type DockerConfigProvider interface {
LazyProvide() *DockerConfigEntry
}

func LazyProvide(creds LazyAuthConfiguration) docker.AuthConfiguration {
func LazyProvide(creds LazyAuthConfiguration) dockertypes.AuthConfig {
if creds.Provider != nil {
entry := *creds.Provider.LazyProvide()
return DockerConfigEntryToLazyAuthConfiguration(entry).AuthConfiguration
return DockerConfigEntryToLazyAuthConfiguration(entry).AuthConfig
} else {
return creds.AuthConfiguration
return creds.AuthConfig
}

}
Expand Down
5 changes: 2 additions & 3 deletions pkg/kubelet/dockertools/convert.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import (
"strings"

dockertypes "github.com/docker/engine-api/types"
docker "github.com/fsouza/go-dockerclient"
kubecontainer "k8s.io/kubernetes/pkg/kubelet/container"
)

Expand Down Expand Up @@ -70,8 +69,8 @@ func toRuntimeContainer(c *dockertypes.Container) (*kubecontainer.Container, err
}, nil
}

// Converts docker.APIImages to kubecontainer.Image.
func toRuntimeImage(image *docker.APIImages) (*kubecontainer.Image, error) {
// Converts dockertypes.Image to kubecontainer.Image.
func toRuntimeImage(image *dockertypes.Image) (*kubecontainer.Image, error) {
if image == nil {
return nil, fmt.Errorf("unable to convert a nil pointer to a runtime image")
}
Expand Down
3 changes: 1 addition & 2 deletions pkg/kubelet/dockertools/convert_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import (
"testing"

dockertypes "github.com/docker/engine-api/types"
docker "github.com/fsouza/go-dockerclient"
kubecontainer "k8s.io/kubernetes/pkg/kubelet/container"
)

Expand Down Expand Up @@ -70,7 +69,7 @@ func TestToRuntimeContainer(t *testing.T) {
}

func TestToRuntimeImage(t *testing.T) {
original := &docker.APIImages{
original := &dockertypes.Image{
ID: "aeeea",
RepoTags: []string{"abc", "def"},
VirtualSize: 1234,
Expand Down
28 changes: 13 additions & 15 deletions pkg/kubelet/dockertools/docker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ import (
"github.com/docker/docker/pkg/jsonmessage"
dockerapi "github.com/docker/engine-api/client"
dockertypes "github.com/docker/engine-api/types"
docker "github.com/fsouza/go-dockerclient"
"github.com/golang/glog"
"k8s.io/kubernetes/pkg/api"
"k8s.io/kubernetes/pkg/credentialprovider"
Expand Down Expand Up @@ -64,10 +63,10 @@ type DockerInterface interface {
StartContainer(id string) error
StopContainer(id string, timeout int) error
RemoveContainer(id string, opts dockertypes.ContainerRemoveOptions) error
InspectImage(image string) (*docker.Image, error)
ListImages(opts docker.ListImagesOptions) ([]docker.APIImages, error)
PullImage(opts docker.PullImageOptions, auth docker.AuthConfiguration) error
RemoveImage(image string) error
InspectImage(image string) (*dockertypes.ImageInspect, error)
ListImages(opts dockertypes.ImageListOptions) ([]dockertypes.Image, error)
PullImage(image string, auth dockertypes.AuthConfig, opts dockertypes.ImagePullOptions) error
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question as by others below.

Why are we adding an image argument when the opts contain the image information.

Copy link
Member

Choose a reason for hiding this comment

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

@MHBauer We do this, because:

  1. Engine-api has moved image out of ImagePullOptions Make required arguments required. docker/engine-api#162, it will be easier to bump up the engine-api version later.
  2. This is more intuitive Make required arguments required - optional arguments should be optional docker/engine-api#137. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

okay.

RemoveImage(image string, opts dockertypes.ImageRemoveOptions) ([]dockertypes.ImageDelete, error)
Logs(string, dockertypes.ContainerLogsOptions, StreamOptions) error
Version() (*dockertypes.Version, error)
Info() (*dockertypes.Info, error)
Expand Down Expand Up @@ -146,23 +145,22 @@ func filterHTTPError(err error, image string) error {

func (p dockerPuller) Pull(image string, secrets []api.Secret) error {
// If no tag was specified, use the default "latest".
repoToPull, tag := parsers.ParseImageName(image)

opts := docker.PullImageOptions{
Repository: repoToPull,
Tag: tag,
}
imageID, tag := parsers.ParseImageName(image)

keyring, err := credentialprovider.MakeDockerKeyring(secrets, p.keyring)
if err != nil {
return err
}

creds, haveCredentials := keyring.Lookup(repoToPull)
opts := dockertypes.ImagePullOptions{
Tag: tag,
}

creds, haveCredentials := keyring.Lookup(imageID)
if !haveCredentials {
glog.V(1).Infof("Pulling image %s without credentials", image)

err := p.client.PullImage(opts, docker.AuthConfiguration{})
err := p.client.PullImage(imageID, dockertypes.AuthConfig{}, opts)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have to pass imageID both as an argument and as a filed in the options? The client should be able to write the imageID to the options.

Copy link
Member

Choose a reason for hiding this comment

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

Agree.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So do we keep imageID here? Seems two opinions?

if err == nil {
// Sometimes PullImage failed with no error returned.
exist, ierr := p.IsImagePresent(image)
Expand All @@ -189,7 +187,7 @@ func (p dockerPuller) Pull(image string, secrets []api.Secret) error {

var pullErrs []error
for _, currentCreds := range creds {
err := p.client.PullImage(opts, credentialprovider.LazyProvide(currentCreds))
err = p.client.PullImage(imageID, credentialprovider.LazyProvide(currentCreds), opts)
// If there was no error, return success
if err == nil {
return nil
Expand All @@ -213,7 +211,7 @@ func (p dockerPuller) IsImagePresent(image string) (bool, error) {
if err == nil {
return true, nil
}
if err == docker.ErrNoSuchImage {
if _, ok := err.(imageNotFoundError); ok {
return false, nil
}
return false, err
Expand Down
10 changes: 4 additions & 6 deletions pkg/kubelet/dockertools/docker_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ import (
"github.com/docker/docker/pkg/jsonmessage"
dockertypes "github.com/docker/engine-api/types"
dockernat "github.com/docker/go-connections/nat"
docker "github.com/fsouza/go-dockerclient"
cadvisorapi "github.com/google/cadvisor/info/v1"
"k8s.io/kubernetes/cmd/kubelet/app/options"
"k8s.io/kubernetes/pkg/api"
Expand Down Expand Up @@ -352,17 +351,16 @@ func TestDockerKeyringLookupFails(t *testing.T) {
}

func TestDockerKeyringLookup(t *testing.T) {

ada := credentialprovider.LazyAuthConfiguration{
AuthConfiguration: docker.AuthConfiguration{
AuthConfig: dockertypes.AuthConfig{
Username: "ada",
Password: "smash",
Email: "ada@example.com",
},
}

grace := credentialprovider.LazyAuthConfiguration{
AuthConfiguration: docker.AuthConfiguration{
AuthConfig: dockertypes.AuthConfig{
Username: "grace",
Password: "squash",
Email: "grace@example.com",
Expand Down Expand Up @@ -425,7 +423,7 @@ func TestDockerKeyringLookup(t *testing.T) {
// NOTE: the above covers the case of a more specific match trumping just hostname.
func TestIssue3797(t *testing.T) {
rex := credentialprovider.LazyAuthConfiguration{
AuthConfiguration: docker.AuthConfiguration{
AuthConfig: dockertypes.AuthConfig{
Username: "rex",
Password: "tiny arms",
Email: "rex@example.com",
Expand Down Expand Up @@ -471,7 +469,7 @@ type imageTrackingDockerClient struct {
imageName string
}

func (f *imageTrackingDockerClient) InspectImage(name string) (image *docker.Image, err error) {
func (f *imageTrackingDockerClient) InspectImage(name string) (image *dockertypes.ImageInspect, err error) {
image, err = f.FakeDockerClient.InspectImage(name)
f.imageName = name
return
Expand Down
22 changes: 9 additions & 13 deletions pkg/kubelet/dockertools/fake_docker_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ import (

dockertypes "github.com/docker/engine-api/types"
dockercontainer "github.com/docker/engine-api/types/container"
docker "github.com/fsouza/go-dockerclient"

"k8s.io/kubernetes/pkg/api"
"k8s.io/kubernetes/pkg/util/sets"
Expand All @@ -40,11 +39,12 @@ type FakeDockerClient struct {
RunningContainerList []dockertypes.Container
ExitedContainerList []dockertypes.Container
ContainerMap map[string]*dockertypes.ContainerJSON
Image *docker.Image
Images []docker.APIImages
Image *dockertypes.ImageInspect
Images []dockertypes.Image
Errors map[string]error
called []string
pulled []string

// Created, Stopped and Removed all container docker ID
Created []string
Stopped []string
Expand Down Expand Up @@ -281,7 +281,7 @@ func (f *FakeDockerClient) InspectContainer(id string) (*dockertypes.ContainerJS

// InspectImage is a test-spy implementation of DockerInterface.InspectImage.
// It adds an entry "inspect" to the internal method call record.
func (f *FakeDockerClient) InspectImage(name string) (*docker.Image, error) {
func (f *FakeDockerClient) InspectImage(name string) (*dockertypes.ImageInspect, error) {
f.Lock()
defer f.Unlock()
f.called = append(f.called, "inspect_image")
Expand Down Expand Up @@ -421,18 +421,14 @@ func (f *FakeDockerClient) Logs(id string, opts dockertypes.ContainerLogsOptions

// PullImage is a test-spy implementation of DockerInterface.PullImage.
// It adds an entry "pull" to the internal method call record.
func (f *FakeDockerClient) PullImage(opts docker.PullImageOptions, auth docker.AuthConfiguration) error {
func (f *FakeDockerClient) PullImage(imageID string, auth dockertypes.AuthConfig, opts dockertypes.ImagePullOptions) error {
f.Lock()
defer f.Unlock()
f.called = append(f.called, "pull")
err := f.popError("pull")
if err == nil {
registry := opts.Registry
if len(registry) != 0 {
registry = registry + "/"
}
authJson, _ := json.Marshal(auth)
f.pulled = append(f.pulled, fmt.Sprintf("%s%s:%s using %s", registry, opts.Repository, opts.Tag, string(authJson)))
f.pulled = append(f.pulled, fmt.Sprintf("%s:%s using %s", imageID, opts.Tag, string(authJson)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Get rid of imageID and pull the info out of opts.

Copy link
Member

Choose a reason for hiding this comment

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

I think it would be better to just use the imageID, because we'll bump up the engine-api version right after this gets merged. In newer version of engine-api, there is no imageID in PullImageOptions https://github.com/kubernetes/kubernetes/pull/23800/files#r60776433

Copy link
Contributor

Choose a reason for hiding this comment

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

yes. Sorry. I had not read your explanation in the other comment yet.

}
return err
}
Expand Down Expand Up @@ -473,17 +469,17 @@ func (f *FakeDockerClient) InspectExec(id string) (*dockertypes.ContainerExecIns
return f.ExecInspect, f.popError("inspect_exec")
}

func (f *FakeDockerClient) ListImages(opts docker.ListImagesOptions) ([]docker.APIImages, error) {
func (f *FakeDockerClient) ListImages(opts dockertypes.ImageListOptions) ([]dockertypes.Image, error) {
err := f.popError("list_images")
return f.Images, err
}

func (f *FakeDockerClient) RemoveImage(image string) error {
func (f *FakeDockerClient) RemoveImage(image string, opts dockertypes.ImageRemoveOptions) ([]dockertypes.ImageDelete, error) {
err := f.popError("remove_image")
if err == nil {
f.RemovedImages.Insert(image)
}
return err
return []dockertypes.ImageDelete{{Deleted: image}}, err
}

func (f *FakeDockerClient) updateContainerStatus(id, status string) {
Expand Down
16 changes: 7 additions & 9 deletions pkg/kubelet/dockertools/instrumented_docker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import (
"time"

dockertypes "github.com/docker/engine-api/types"
docker "github.com/fsouza/go-dockerclient"
"k8s.io/kubernetes/pkg/kubelet/metrics"
)

Expand Down Expand Up @@ -103,7 +102,7 @@ func (in instrumentedDockerInterface) RemoveContainer(id string, opts dockertype
return err
}

func (in instrumentedDockerInterface) InspectImage(image string) (*docker.Image, error) {
func (in instrumentedDockerInterface) InspectImage(image string) (*dockertypes.ImageInspect, error) {
const operation = "inspect_image"
defer recordOperation(operation, time.Now())

Expand All @@ -112,7 +111,7 @@ func (in instrumentedDockerInterface) InspectImage(image string) (*docker.Image,
return out, err
}

func (in instrumentedDockerInterface) ListImages(opts docker.ListImagesOptions) ([]docker.APIImages, error) {
func (in instrumentedDockerInterface) ListImages(opts dockertypes.ImageListOptions) ([]dockertypes.Image, error) {
const operation = "list_images"
defer recordOperation(operation, time.Now())

Expand All @@ -121,22 +120,21 @@ func (in instrumentedDockerInterface) ListImages(opts docker.ListImagesOptions)
return out, err
}

func (in instrumentedDockerInterface) PullImage(opts docker.PullImageOptions, auth docker.AuthConfiguration) error {
func (in instrumentedDockerInterface) PullImage(imageID string, auth dockertypes.AuthConfig, opts dockertypes.ImagePullOptions) error {
const operation = "pull_image"
defer recordOperation(operation, time.Now())

err := in.client.PullImage(opts, auth)
err := in.client.PullImage(imageID, auth, opts)
recordError(operation, err)
return err
}

func (in instrumentedDockerInterface) RemoveImage(image string) error {
func (in instrumentedDockerInterface) RemoveImage(image string, opts dockertypes.ImageRemoveOptions) ([]dockertypes.ImageDelete, error) {
const operation = "remove_image"
defer recordOperation(operation, time.Now())

err := in.client.RemoveImage(image)
imageDelete, err := in.client.RemoveImage(image, opts)
recordError(operation, err)
return err
return imageDelete, err
}

func (in instrumentedDockerInterface) Logs(id string, opts dockertypes.ContainerLogsOptions, sopts StreamOptions) error {
Expand Down