From b1ee54346347f3675f285d24e302d36980983804 Mon Sep 17 00:00:00 2001 From: Euan Kemp Date: Fri, 9 Sep 2016 13:35:22 -0700 Subject: [PATCH] Revert "Merge pull request #30513 from tmrts/kubelet-rkt-cri/use-image-service" This reverts commit aff7dfcaab593903d79687b82541f5103c86938c, reversing changes made to 7a4d81ea43d262221a1cf7bb7eec391307ea7302. --- pkg/kubelet/rkt/image.go | 31 +++- pkg/kubelet/rkt/rkt.go | 20 --- pkg/kubelet/rkt/rkt_test.go | 1 - pkg/kubelet/rktshim/cli.go | 152 ------------------ pkg/kubelet/rktshim/imagestore.go | 85 ++-------- pkg/kubelet/rktshim/imagestore_test.go | 210 +++++++++++++++++++++++++ 6 files changed, 248 insertions(+), 251 deletions(-) delete mode 100644 pkg/kubelet/rktshim/cli.go create mode 100644 pkg/kubelet/rktshim/imagestore_test.go diff --git a/pkg/kubelet/rkt/image.go b/pkg/kubelet/rkt/image.go index 56b8e43d638..5487a07f6fb 100644 --- a/pkg/kubelet/rkt/image.go +++ b/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) - - return err != nil, err + images, err := r.listImages(image.Image, false) + return len(images) > 0, 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. diff --git a/pkg/kubelet/rkt/rkt.go b/pkg/kubelet/rkt/rkt.go index 130fde20108..31b2bc6a0b6 100644 --- a/pkg/kubelet/rkt/rkt.go +++ b/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) diff --git a/pkg/kubelet/rkt/rkt_test.go b/pkg/kubelet/rkt/rkt_test.go index 239ac47a9f9..3dd7a773065 100644 --- a/pkg/kubelet/rkt/rkt_test.go +++ b/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} diff --git a/pkg/kubelet/rktshim/cli.go b/pkg/kubelet/rktshim/cli.go deleted file mode 100644 index 1b03bb1f435..00000000000 --- a/pkg/kubelet/rktshim/cli.go +++ /dev/null @@ -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} -} diff --git a/pkg/kubelet/rktshim/imagestore.go b/pkg/kubelet/rktshim/imagestore.go index 428f166a65f..bba1fc4ddb6 100644 --- a/pkg/kubelet/rktshim/imagestore.go +++ b/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 { - rkt CLI - requestTimeout time.Duration -} +type ImageStore struct{} // TODO(tmrts): fill the image store configuration fields. -type ImageStoreConfig struct { - CLI CLI - RequestTimeout time.Duration -} +type ImageStoreConfig struct{} // NewImageStore creates an image storage that allows CRUD operations for images. -func NewImageStore(cfg ImageStoreConfig) (*ImageStore, error) { - return &ImageStore{rkt: cfg.CLI, requestTimeout: cfg.RequestTimeout}, nil +func NewImageStore(ImageStoreConfig) (*ImageStore, error) { + return &ImageStore{}, nil } // List lists the images residing in the image store. -func (s *ImageStore) List() ([]container.Image, error) { - list, err := s.rkt.RunCommand("image", "list", - "--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 +func (*ImageStore) List() ([]runtimeApi.Image, error) { + panic("not implemented") } // 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 { - panic("not implemented yet!") +func (*ImageStore) Pull(runtimeApi.ImageSpec, runtimeApi.AuthConfig, *runtimeApi.PodSandboxConfig) error { + panic("not implemented") } // Remove removes the image from the image store. -func (s *ImageStore) Remove(imgSpec container.ImageSpec) error { - img, err := s.Status(imgSpec) - 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 +func (*ImageStore) Remove(runtimeApi.ImageSpec) error { + panic("not implemented") } // Status returns the status of the image. -func (s *ImageStore) Status(spec container.ImageSpec) (container.Image, error) { - images, err := s.List() - 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) +func (*ImageStore) Status(runtimeApi.ImageSpec) (runtimeApi.Image, error) { + panic("not implemented") } diff --git a/pkg/kubelet/rktshim/imagestore_test.go b/pkg/kubelet/rktshim/imagestore_test.go new file mode 100644 index 00000000000..3eb966ec7ec --- /dev/null +++ b/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) + } + } +}