Revert "Merge pull request #30513 from tmrts/kubelet-rkt-cri/use-image-service"

This reverts commit aff7dfcaab, reversing
changes made to 7a4d81ea43.

pkg/kubelet/rkt/image.go

@@ -87,19 +87,40 @@ func (r *Runtime) PullImage(image kubecontainer.ImageSpec, pullSecrets []api.Sec
 }
 
 func (r *Runtime) IsImagePresent(image kubecontainer.ImageSpec) (bool, error) {
-	_, err := r.imageService.Status(image)
+	images, err := r.listImages(image.Image, false)
-
+	return len(images) > 0, err
-	return err != nil, err
 }
 
 // ListImages lists all the available appc images on the machine by invoking 'rkt image list'.
 func (r *Runtime) ListImages() ([]kubecontainer.Image, error) {
-	return r.imageService.List()
+	ctx, cancel := context.WithTimeout(context.Background(), r.requestTimeout)
+	defer cancel()
+	listResp, err := r.apisvc.ListImages(ctx, &rktapi.ListImagesRequest{})
+	if err != nil {
+		return nil, fmt.Errorf("couldn't list images: %v", err)
+	}
+
+	images := make([]kubecontainer.Image, len(listResp.Images))
+	for i, image := range listResp.Images {
+		images[i] = kubecontainer.Image{
+			ID:       image.Id,
+			RepoTags: []string{buildImageName(image)},
+			Size:     image.Size,
+		}
+	}
+	return images, nil
 }
 
 // RemoveImage removes an on-disk image using 'rkt image rm'.
 func (r *Runtime) RemoveImage(image kubecontainer.ImageSpec) error {
-	return r.imageService.Remove(image)
+	imageID, err := r.getImageID(image.Image)
+	if err != nil {
+		return err
+	}
+	if _, err := r.cli.RunCommand(nil, "image", "rm", imageID); err != nil {
+		return err
+	}
+	return nil
 }
 
 // buildImageName constructs the image name for kubecontainer.Image.

pkg/kubelet/rkt/rkt.go

@@ -52,7 +52,6 @@ import (
 	"k8s.io/kubernetes/pkg/kubelet/network"
 	"k8s.io/kubernetes/pkg/kubelet/network/hairpin"
 	proberesults "k8s.io/kubernetes/pkg/kubelet/prober/results"
-	"k8s.io/kubernetes/pkg/kubelet/rktshim"
 	"k8s.io/kubernetes/pkg/kubelet/types"
 	"k8s.io/kubernetes/pkg/kubelet/util/format"
 	"k8s.io/kubernetes/pkg/securitycontext"
@@ -158,8 +157,6 @@ type Runtime struct {
 	execer              utilexec.Interface
 	os                  kubecontainer.OSInterface
 
-	imageService *rktshim.ImageStore
-
 	// Network plugin.
 	networkPlugin network.NetworkPlugin
 
@@ -267,28 +264,11 @@ func New(
 		requestTimeout:      requestTimeout,
 	}
 
-	// TODO(tmrts): transform from being a method to function
 	rkt.config, err = rkt.getConfig(rkt.config)
 	if err != nil {
 		return nil, fmt.Errorf("rkt: cannot get config from rkt api service: %v", err)
 	}
 
-	rkt.imageService, err = rktshim.NewImageStore(rktshim.ImageStoreConfig{
-		RequestTimeout: requestTimeout,
-		// TODO(tmrts): use the new CLI api throught the rkt pkg
-		CLI: rktshim.NewRktCLI(config.Path, execer, rktshim.CLIConfig{
-			Debug:           config.Debug,
-			Dir:             config.Dir,
-			LocalConfigDir:  config.LocalConfigDir,
-			UserConfigDir:   config.UserConfigDir,
-			SystemConfigDir: config.SystemConfigDir,
-			InsecureOptions: config.InsecureOptions,
-		}),
-	})
-	if err != nil {
-		return nil, fmt.Errorf("rkt: failed to create ImageService: %v", err)
-	}
-
 	rkt.runner = lifecycle.NewHandlerRunner(httpClient, rkt, rkt)
 
 	rkt.imagePuller = images.NewImageManager(recorder, rkt, imageBackOff, serializeImagePulls)

pkg/kubelet/rkt/rkt_test.go

@@ -262,7 +262,6 @@ func TestCheckVersion(t *testing.T) {
 }
 
 func TestListImages(t *testing.T) {
-	t.SkipNow()
 	fr := newFakeRktInterface()
 	fs := newFakeSystemd()
 	r := &Runtime{apisvc: fr, systemd: fs}

pkg/kubelet/rktshim/cli.go

@@ -1,152 +0,0 @@
-/*
-Copyright 2016 The Kubernetes Authors.
-
-Licensed under the Apache License, Version 2.0 (the "License");
-you may not use this file except in compliance with the License.
-You may obtain a copy of the License at
-
-    http://www.apache.org/licenses/LICENSE-2.0
-
-Unless required by applicable law or agreed to in writing, software
-distributed under the License is distributed on an "AS IS" BASIS,
-WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-See the License for the specific language governing permissions and
-limitations under the License.
-*/
-
-package rktshim
-
-import (
-	"errors"
-	"fmt"
-	"reflect"
-	"strings"
-
-	utilexec "k8s.io/kubernetes/pkg/util/exec"
-)
-
-var (
-	errFlagTagNotFound           = errors.New("arg: given field doesn't have a `flag` tag")
-	errStructFieldNotInitialized = errors.New("arg: given field is unitialized")
-)
-
-// TODO(tmrts): refactor these into an util pkg
-// Uses reflection to retrieve the `flag` tag of a field.
-// The value of the `flag` field with the value of the field is
-// used to construct a POSIX long flag argument string.
-func getLongFlagFormOfField(fieldValue reflect.Value, fieldType reflect.StructField) (string, error) {
-	flagTag := fieldType.Tag.Get("flag")
-	if flagTag == "" {
-		return "", errFlagTagNotFound
-	}
-
-	if fieldValue.IsValid() {
-		return "", errStructFieldNotInitialized
-	}
-
-	switch fieldValue.Kind() {
-	case reflect.Array:
-		fallthrough
-	case reflect.Slice:
-		var args []string
-		for i := 0; i < fieldValue.Len(); i++ {
-			args = append(args, fieldValue.Index(i).String())
-		}
-
-		return fmt.Sprintf("--%v=%v", flagTag, strings.Join(args, ",")), nil
-	}
-
-	return fmt.Sprintf("--%v=%v", flagTag, fieldValue), nil
-}
-
-// Uses reflection to transform a struct containing fields with `flag` tags
-// to a string slice of POSIX compliant long form arguments.
-func getArgumentFormOfStruct(strt interface{}) (flags []string) {
-	numberOfFields := reflect.ValueOf(strt).NumField()
-
-	for i := 0; i < numberOfFields; i++ {
-		fieldValue := reflect.ValueOf(strt).Field(i)
-		fieldType := reflect.TypeOf(strt).Field(i)
-
-		flagFormOfField, err := getLongFlagFormOfField(fieldValue, fieldType)
-		if err != nil {
-			continue
-		}
-
-		flags = append(flags, flagFormOfField)
-	}
-
-	return
-}
-
-func getFlagFormOfStruct(strt interface{}) (flags []string) {
-	return getArgumentFormOfStruct(strt)
-}
-
-type CLIConfig struct {
-	Debug bool `flag:"debug"`
-
-	Dir             string `flag:"dir"`
-	LocalConfigDir  string `flag:"local-config"`
-	UserConfigDir   string `flag:"user-config"`
-	SystemConfigDir string `flag:"system-config"`
-
-	InsecureOptions string `flag:"insecure-options"`
-}
-
-func (cfg *CLIConfig) Merge(newCfg CLIConfig) {
-	newCfgVal := reflect.ValueOf(newCfg)
-	newCfgType := reflect.TypeOf(newCfg)
-
-	numberOfFields := newCfgVal.NumField()
-
-	for i := 0; i < numberOfFields; i++ {
-		fieldValue := newCfgVal.Field(i)
-		fieldType := newCfgType.Field(i)
-
-		if !fieldValue.IsValid() {
-			continue
-		}
-
-		newCfgVal.FieldByName(fieldType.Name).Set(fieldValue)
-	}
-}
-
-type CLI interface {
-	With(CLIConfig) CLI
-	RunCommand(string, ...string) ([]string, error)
-}
-
-type cli struct {
-	rktPath string
-	config  CLIConfig
-	execer  utilexec.Interface
-}
-
-func (c *cli) With(cfg CLIConfig) CLI {
-	copyCfg := c.config
-
-	copyCfg.Merge(cfg)
-
-	return NewRktCLI(c.rktPath, c.execer, copyCfg)
-}
-
-func (c *cli) RunCommand(subcmd string, args ...string) ([]string, error) {
-	globalFlags := getFlagFormOfStruct(c.config)
-
-	args = append(globalFlags, args...)
-
-	cmd := c.execer.Command(c.rktPath, append([]string{subcmd}, args...)...)
-
-	out, err := cmd.CombinedOutput()
-	if err != nil {
-		return nil, fmt.Errorf("failed to run %v: %v\noutput: %v", args, err, out)
-	}
-
-	return strings.Split(strings.TrimSpace(string(out)), "\n"), nil
-}
-
-// TODO(tmrts): implement CLI with timeout
-func NewRktCLI(rktPath string, exec utilexec.Interface, cfg CLIConfig) CLI {
-	return &cli{rktPath: rktPath, config: cfg, execer: exec}
-}

pkg/kubelet/rktshim/imagestore.go

@@ -18,14 +18,8 @@ package rktshim
 
 import (
 	"errors"
-	"fmt"
-	"strconv"
-	"strings"
-	"time"
 
 	runtimeApi "k8s.io/kubernetes/pkg/kubelet/api/v1alpha1/runtime"
-
-	"k8s.io/kubernetes/pkg/kubelet/container"
 )
 
 // TODO(tmrts): Move these errors to the container API for code re-use.
@@ -36,87 +30,32 @@ var (
 // var _ kubeletApi.ImageManagerService = (*ImageStore)(nil)
 
 // ImageStore supports CRUD operations for images.
-type ImageStore struct {
+type ImageStore struct{}
-	rkt            CLI
-	requestTimeout time.Duration
-}
 
 // TODO(tmrts): fill the image store configuration fields.
-type ImageStoreConfig struct {
+type ImageStoreConfig struct{}
-	CLI            CLI
-	RequestTimeout time.Duration
-}
 
 // NewImageStore creates an image storage that allows CRUD operations for images.
-func NewImageStore(cfg ImageStoreConfig) (*ImageStore, error) {
+func NewImageStore(ImageStoreConfig) (*ImageStore, error) {
-	return &ImageStore{rkt: cfg.CLI, requestTimeout: cfg.RequestTimeout}, nil
+	return &ImageStore{}, nil
 }
 
 // List lists the images residing in the image store.
-func (s *ImageStore) List() ([]container.Image, error) {
+func (*ImageStore) List() ([]runtimeApi.Image, error) {
-	list, err := s.rkt.RunCommand("image", "list",
+	panic("not implemented")
-		"--no-legend",
-		"--fields=id,name,size",
-		"--sort=importtime",
-	)
-	if err != nil {
-		return nil, fmt.Errorf("couldn't list images: %v", err)
-	}
-
-	images := make([]container.Image, len(list))
-	for i, image := range list {
-		tokens := strings.Fields(image)
-
-		id, name := tokens[0], tokens[1]
-
-		size, err := strconv.ParseInt(tokens[2], 10, 0)
-		if err != nil {
-			return nil, fmt.Errorf("invalid image size format: %v", err)
-		}
-
-		images[i] = container.Image{
-			ID:       id,
-			RepoTags: []string{name},
-			Size:     size,
-		}
-	}
-
-	return images, nil
 }
 
 // Pull pulls an image into the image store and uses the given authentication method.
-func (s *ImageStore) Pull(container.ImageSpec, runtimeApi.AuthConfig, *runtimeApi.PodSandboxConfig) error {
+func (*ImageStore) Pull(runtimeApi.ImageSpec, runtimeApi.AuthConfig, *runtimeApi.PodSandboxConfig) error {
-	panic("not implemented yet!")
+	panic("not implemented")
 }
 
 // Remove removes the image from the image store.
-func (s *ImageStore) Remove(imgSpec container.ImageSpec) error {
+func (*ImageStore) Remove(runtimeApi.ImageSpec) error {
-	img, err := s.Status(imgSpec)
+	panic("not implemented")
-	if err != nil {
-		return err
-	}
-
-	if _, err := s.rkt.RunCommand("image", "rm", img.ID); err != nil {
-		return fmt.Errorf("failed to remove the image: %v", err)
-	}
-
-	return nil
 }
 
 // Status returns the status of the image.
-func (s *ImageStore) Status(spec container.ImageSpec) (container.Image, error) {
+func (*ImageStore) Status(runtimeApi.ImageSpec) (runtimeApi.Image, error) {
-	images, err := s.List()
+	panic("not implemented")
-	if err != nil {
-		return container.Image{}, err
-	}
-
-	for _, img := range images {
-		for _, tag := range img.RepoTags {
-			if tag == spec.Image {
-				return img, nil
-			}
-		}
-	}
-
-	return container.Image{}, fmt.Errorf("couldn't to find the image %v", spec.Image)
 }

pkg/kubelet/rktshim/imagestore_test.go

@@ -0,0 +1,210 @@
+/*
+Copyright 2016 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package rktshim
+
+import (
+	"fmt"
+	"reflect"
+	"testing"
+
+	runtimeApi "k8s.io/kubernetes/pkg/kubelet/api/v1alpha1/runtime"
+)
+
+var (
+	emptyImgStoreConfig = ImageStoreConfig{}
+	// TODO(tmrts): fill the pod configuration
+	testPodConfig *runtimeApi.PodSandboxConfig = nil
+)
+
+type imageTestCase struct {
+	Spec           *runtimeApi.ImageSpec
+	ExpectedStatus *runtimeApi.Image
+}
+
+func compareContainerImages(got, expected runtimeApi.Image) error {
+	if got.Id != expected.Id {
+		return fmt.Errorf("mismatching Ids -> expected %q, got %q", got.Id, expected.Id)
+	}
+
+	if !reflect.DeepEqual(got.RepoTags, expected.RepoTags) {
+		return fmt.Errorf("mismatching RepoTags -> expected %q, got %q", got.Id, expected.Id)
+	}
+
+	if !reflect.DeepEqual(got.RepoDigests, expected.RepoDigests) {
+		return fmt.Errorf("mismatching RepoDigests -> expected %q, got %q", got.Id, expected.Id)
+	}
+
+	if got.Size_ != expected.Size_ {
+		return fmt.Errorf("mismatching Sizes -> expected %q, got %q", got.Id, expected.Id)
+	}
+
+	return nil
+}
+
+var (
+	busyboxStr   = "busybox"
+	gibberishStr = "XXXX_GIBBERISH_XXXX"
+)
+
+var testImgSpecs = map[string]imageTestCase{
+	"non-existent-image": {
+		&runtimeApi.ImageSpec{
+			Image: &gibberishStr,
+		},
+		nil,
+	},
+	"busybox": {
+		&runtimeApi.ImageSpec{
+			Image: &busyboxStr,
+		},
+		&runtimeApi.Image{
+			Id:          nil,
+			RepoTags:    []string{},
+			RepoDigests: []string{},
+			Size_:       nil,
+		},
+	},
+}
+
+var testAuthConfig = map[string]runtimeApi.AuthConfig{
+	"no-auth": {},
+}
+
+func testNewImageStore(t *testing.T, cfg ImageStoreConfig) *ImageStore {
+	s, err := NewImageStore(cfg)
+	if err != nil {
+		// TODO(tmrts): Implement stringer for rktshim.ImageStoreConfig for test readability.
+		t.Fatalf("rktshim.NewImageStore(%s) got error %q", cfg, err)
+	}
+
+	return s
+}
+
+func TestPullsImageWithoutAuthentication(t *testing.T) {
+	t.SkipNow()
+
+	imgStore := testNewImageStore(t, emptyImgStoreConfig)
+
+	testImg := "busybox"
+	testImgSpec := *testImgSpecs[testImg].Spec
+
+	if err := imgStore.Pull(testImgSpec, testAuthConfig["no-auth"], testPodConfig); err != nil {
+		t.Fatalf("rktshim.ImageStore.PullImage(%q) got error %q", testImg, err)
+	}
+}
+
+func TestQueriesNonExistentImage(t *testing.T) {
+	t.SkipNow()
+
+	imgStore := testNewImageStore(t, emptyImgStoreConfig)
+
+	// New store shouldn't contain this image
+	testImg := "non-existent-image"
+	testImgSpec := *testImgSpecs[testImg].Spec
+
+	if _, err := imgStore.Status(testImgSpec); err != ErrImageNotFound {
+		t.Errorf("rktshim.ImageStore.Status(%q) expected error %q, got %q", testImg, ErrImageNotFound, err)
+	}
+}
+
+func TestQueriesExistentImage(t *testing.T) {
+	t.SkipNow()
+
+	imgStore := testNewImageStore(t, emptyImgStoreConfig)
+
+	testImg := "busybox"
+	testImgSpec := *testImgSpecs[testImg].Spec
+	expectedStatus := *testImgSpecs[testImg].ExpectedStatus
+
+	imgStatus, err := imgStore.Status(testImgSpec)
+	if err != nil {
+		t.Fatalf("rktshim.ImageStore.Status(%q) got error %q", testImg, err)
+	}
+
+	if err := compareContainerImages(imgStatus, expectedStatus); err != nil {
+		t.Errorf("rktshim.ImageStore.Status(%q) %v", testImg, err)
+	}
+}
+
+func TestRemovesImage(t *testing.T) {
+	t.SkipNow()
+
+	imgStore := testNewImageStore(t, emptyImgStoreConfig)
+
+	testImg := "busybox"
+	testImgSpec := *testImgSpecs[testImg].Spec
+
+	if err := imgStore.Pull(testImgSpec, testAuthConfig["no-auth"], testPodConfig); err != nil {
+		t.Fatalf("rktshim.ImageStore.Pull(%q) got error %q", testImg, err)
+	}
+
+	if _, err := imgStore.Status(testImgSpec); err != nil {
+		t.Fatalf("rktshim.ImageStore.Status(%q) got error %q", testImg, err)
+	}
+
+	if err := imgStore.Remove(testImgSpec); err != nil {
+		t.Fatalf("rktshim.ImageStore.Remove(%q) got error %q", testImg, err)
+	}
+
+	if _, err := imgStore.Status(testImgSpec); err != ErrImageNotFound {
+		t.Fatalf("rktshim.ImageStore.Status(%q) expected error %q, got error %q", testImg, ErrImageNotFound, err)
+	}
+}
+
+func TestRemovesNonExistentImage(t *testing.T) {
+	t.SkipNow()
+
+	imgStore := testNewImageStore(t, emptyImgStoreConfig)
+
+	testImg := "non-existent-image"
+	testImgSpec := *testImgSpecs[testImg].Spec
+
+	if err := imgStore.Remove(testImgSpec); err != ErrImageNotFound {
+		t.Fatalf("rktshim.ImageStore.Remove(%q) expected error %q, got error %q", testImg, ErrImageNotFound, err)
+	}
+}
+
+func TestListsImages(t *testing.T) {
+	t.SkipNow()
+
+	imgStore := testNewImageStore(t, emptyImgStoreConfig)
+
+	busyboxImg := "busybox"
+	busyboxImgSpec := *testImgSpecs[busyboxImg].Spec
+	if err := imgStore.Pull(busyboxImgSpec, testAuthConfig["no-auth"], testPodConfig); err != nil {
+		t.Fatalf("rktshim.ImageStore.Pull(%q) got error %q", busyboxImg, err)
+	}
+
+	alpineImg := "alpine"
+	alpineImgSpec := *testImgSpecs[alpineImg].Spec
+	if err := imgStore.Pull(alpineImgSpec, testAuthConfig["no-auth"], testPodConfig); err != nil {
+		t.Fatalf("rktshim.ImageStore.Pull(%q) got error %q", alpineImg, err)
+	}
+
+	imgs, err := imgStore.List()
+	if err != nil {
+		t.Fatalf("rktshim.ImageStore.List() got error %q", err)
+	}
+
+	for _, img := range imgs {
+		expectedImg := *testImgSpecs[*img.Id].ExpectedStatus
+
+		if err := compareContainerImages(img, expectedImg); err != nil {
+			t.Errorf("rktshim.ImageStore.List() for %q, %v", img.Id, err)
+		}
+	}
+}