From 5169cc85cfd3803b36c27c243b0bdab4997b3989 Mon Sep 17 00:00:00 2001 From: Yu-Ju Hong Date: Fri, 13 Oct 2017 09:05:52 -0700 Subject: [PATCH 1/5] Add a file store utility package in kubelet More and more components checkpoints (i.e., persist their states) in kubelet. Refurbish and move the implementation in dockershim to a utility package to improve code reusability. --- pkg/kubelet/util/store/doc.go | 18 +++ pkg/kubelet/util/store/filestore.go | 152 +++++++++++++++++++++++ pkg/kubelet/util/store/filestore_test.go | 116 +++++++++++++++++ pkg/kubelet/util/store/store.go | 53 ++++++++ pkg/kubelet/util/store/store_test.go | 55 ++++++++ 5 files changed, 394 insertions(+) create mode 100644 pkg/kubelet/util/store/doc.go create mode 100644 pkg/kubelet/util/store/filestore.go create mode 100644 pkg/kubelet/util/store/filestore_test.go create mode 100644 pkg/kubelet/util/store/store.go create mode 100644 pkg/kubelet/util/store/store_test.go diff --git a/pkg/kubelet/util/store/doc.go b/pkg/kubelet/util/store/doc.go new file mode 100644 index 00000000000..d9f1cedfd8f --- /dev/null +++ b/pkg/kubelet/util/store/doc.go @@ -0,0 +1,18 @@ +/* +Copyright 2015 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. +*/ + +// A store interface. +package store // import "k8s.io/kubernetes/pkg/kubelet/util/store" diff --git a/pkg/kubelet/util/store/filestore.go b/pkg/kubelet/util/store/filestore.go new file mode 100644 index 00000000000..94fe222ae32 --- /dev/null +++ b/pkg/kubelet/util/store/filestore.go @@ -0,0 +1,152 @@ +/* +Copyright 2017 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 store + +import ( + "fmt" + "os" + "path/filepath" + "strings" + + utilfs "k8s.io/kubernetes/pkg/util/filesystem" +) + +const ( + // Name prefix for the temporary files. + tmpPrefix = "." +) + +// FileStore is an implementation of the Store interface which stores data in files. +type FileStore struct { + // Absolute path to the base directory for storing data files. + directoryPath string + + // filesystem to use. + filesystem utilfs.Filesystem +} + +func NewFileStore(path string, fs utilfs.Filesystem) (Store, error) { + if err := ensureDirectory(fs, path); err != nil { + return nil, err + } + return &FileStore{directoryPath: path, filesystem: fs}, nil +} + +func (f *FileStore) Write(key string, data []byte) error { + if err := ValidateKey(key); err != nil { + return err + } + if err := ensureDirectory(f.filesystem, f.directoryPath); err != nil { + return err + } + + return writeFile(f.filesystem, f.getPathByKey(key), data) +} + +func (f *FileStore) Read(key string) ([]byte, error) { + if err := ValidateKey(key); err != nil { + return nil, err + } + bytes, err := f.filesystem.ReadFile(f.getPathByKey(key)) + if os.IsNotExist(err) { + return bytes, KeyNotFoundError + } + return bytes, err +} + +func (f *FileStore) Delete(key string) error { + if err := ValidateKey(key); err != nil { + return err + } + return removePath(f.filesystem, f.getPathByKey(key)) +} + +func (f *FileStore) List() ([]string, error) { + keys := make([]string, 0) + files, err := f.filesystem.ReadDir(f.directoryPath) + if err != nil { + return keys, err + } + for _, f := range files { + if !strings.HasPrefix(f.Name(), tmpPrefix) { + keys = append(keys, f.Name()) + } + } + return keys, nil +} + +func (f *FileStore) getPathByKey(key string) string { + return filepath.Join(f.directoryPath, key) +} + +// ensureDirectory creates the directory if it does not exist. +func ensureDirectory(fs utilfs.Filesystem, path string) error { + if _, err := fs.Stat(path); err != nil { + // MkdirAll returns nil if directory already exists. + return fs.MkdirAll(path, 0755) + } + return nil +} + +// writeFile writes data to path in a single transaction. +func writeFile(fs utilfs.Filesystem, path string, data []byte) (retErr error) { + // Create a temporary file in the base directory of `path` with a prefix. + tmpFile, err := fs.TempFile(filepath.Dir(path), tmpPrefix) + if err != nil { + return err + } + + defer func() { + // Close the file. + if err := tmpFile.Close(); err != nil { + if retErr == nil { + retErr = err + } else { + retErr = fmt.Errorf("failed to close temp file after error %v", retErr) + } + } + }() + + tmpPath := tmpFile.Name() + defer func() { + // Clean up the temp file on error. + if retErr != nil && tmpPath != "" { + if err := removePath(fs, tmpPath); err != nil { + retErr = fmt.Errorf("failed to remove the temporary file after error %v", retErr) + } + } + }() + + // Write data. + if _, err := tmpFile.Write(data); err != nil { + return err + } + + // Sync file. + if err := tmpFile.Sync(); err != nil { + return err + } + + return fs.Rename(tmpPath, path) +} + +func removePath(fs utilfs.Filesystem, path string) error { + if err := fs.Remove(path); err != nil && !os.IsNotExist(err) { + return err + } + return nil +} diff --git a/pkg/kubelet/util/store/filestore_test.go b/pkg/kubelet/util/store/filestore_test.go new file mode 100644 index 00000000000..567fb253d83 --- /dev/null +++ b/pkg/kubelet/util/store/filestore_test.go @@ -0,0 +1,116 @@ +/* +Copyright 2017 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 store + +import ( + "io/ioutil" + "sort" + "testing" + + "github.com/stretchr/testify/assert" + "k8s.io/kubernetes/pkg/util/filesystem" +) + +func TestFileStore(t *testing.T) { + path, err := ioutil.TempDir("", "FileStore") + assert.NoError(t, err) + store, err := NewFileStore(path, filesystem.NewFakeFs()) + assert.NoError(t, err) + + testData := []struct { + key string + data string + expectErr bool + }{ + { + "id1", + "data1", + false, + }, + { + "id2", + "data2", + false, + }, + { + "/id1", + "data1", + true, + }, + { + ".id1", + "data1", + true, + }, + { + " ", + "data2", + true, + }, + { + "___", + "data2", + true, + }, + } + + // Test add data. + for _, c := range testData { + _, err = store.Read(c.key) + assert.Error(t, err) + + err = store.Write(c.key, []byte(c.data)) + if c.expectErr { + assert.Error(t, err) + continue + } else { + assert.NoError(t, err) + } + + // Test read data by key. + data, err := store.Read(c.key) + assert.NoError(t, err) + assert.Equal(t, string(data), c.data) + } + + // Test list keys. + keys, err := store.List() + assert.NoError(t, err) + sort.Strings(keys) + assert.Equal(t, keys, []string{"id1", "id2"}) + + // Test Delete data + for _, c := range testData { + if c.expectErr { + continue + } + + err = store.Delete(c.key) + assert.NoError(t, err) + _, err = store.Read(c.key) + assert.EqualValues(t, KeyNotFoundError, err) + } + + // Test delete non-existent key. + err = store.Delete("id1") + assert.NoError(t, err) + + // Test list keys. + keys, err = store.List() + assert.NoError(t, err) + assert.Equal(t, len(keys), 0) +} diff --git a/pkg/kubelet/util/store/store.go b/pkg/kubelet/util/store/store.go new file mode 100644 index 00000000000..62072643563 --- /dev/null +++ b/pkg/kubelet/util/store/store.go @@ -0,0 +1,53 @@ +/* +Copyright 2017 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 store + +import ( + "fmt" + "regexp" +) + +const keyMaxLength = 250 + +var keyRegex = regexp.MustCompile("^[a-zA-Z0-9]+$") + +var ( + KeyNotFoundError = fmt.Errorf("key is not found.") +) + +// Store provides the interface for storing keyed data. +// Store must be thread-safe +type Store interface { + // key must contain one or more characters in [A-Za-z0-9] + // Write writes data with key. + Write(key string, data []byte) error + // Read retrieves data with key + // Read must return KeyNotFoundError if key is not found. + Read(key string) ([]byte, error) + // Delete deletes data by key + // Delete must not return error if key does not exist + Delete(key string) error + // List lists all existing keys. + List() ([]string, error) +} + +func ValidateKey(key string) error { + if len(key) <= keyMaxLength && keyRegex.MatchString(key) { + return nil + } + return fmt.Errorf("invalid key: %q", key) +} diff --git a/pkg/kubelet/util/store/store_test.go b/pkg/kubelet/util/store/store_test.go new file mode 100644 index 00000000000..e19afd3ad85 --- /dev/null +++ b/pkg/kubelet/util/store/store_test.go @@ -0,0 +1,55 @@ +/* +Copyright 2017 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 store + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestIsValidKey(t *testing.T) { + testcases := []struct { + key string + valid bool + }{ + { + " ", + false, + }, + { + "/foo/bar", + false, + }, + { + ".foo", + false, + }, + { + "a78768279290d33d0b82eaea43cb8346f500057cb5bd250e88c97a5585385d66", + true, + }, + } + + for _, tc := range testcases { + if tc.valid { + assert.NoError(t, ValidateKey(tc.key)) + } else { + assert.Error(t, ValidateKey(tc.key)) + } + } +} From 2fd59d9960c87c32910fe9d275d835d979cbc553 Mon Sep 17 00:00:00 2001 From: Yu-Ju Hong Date: Mon, 16 Oct 2017 14:25:29 -0700 Subject: [PATCH 2/5] Change dockershim to use the common store package --- pkg/kubelet/dockershim/checkpoint_store.go | 156 ----------------- .../dockershim/checkpoint_store_test.go | 158 ------------------ pkg/kubelet/dockershim/docker_checkpoint.go | 6 +- 3 files changed, 4 insertions(+), 316 deletions(-) delete mode 100644 pkg/kubelet/dockershim/checkpoint_store.go delete mode 100644 pkg/kubelet/dockershim/checkpoint_store_test.go diff --git a/pkg/kubelet/dockershim/checkpoint_store.go b/pkg/kubelet/dockershim/checkpoint_store.go deleted file mode 100644 index b8160bbd9bd..00000000000 --- a/pkg/kubelet/dockershim/checkpoint_store.go +++ /dev/null @@ -1,156 +0,0 @@ -/* -Copyright 2017 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 dockershim - -import ( - "fmt" - "io" - "io/ioutil" - "os" - "path/filepath" - "regexp" - "strings" - - "k8s.io/kubernetes/pkg/kubelet/dockershim/errors" -) - -const ( - tmpPrefix = "." - tmpSuffix = ".tmp" - keyMaxLength = 250 -) - -var keyRegex = regexp.MustCompile("^[a-zA-Z0-9]+$") - -// CheckpointStore provides the interface for checkpoint storage backend. -// CheckpointStore must be thread-safe -type CheckpointStore interface { - // key must contain one or more characters in [A-Za-z0-9] - // Write persists a checkpoint with key - Write(key string, data []byte) error - // Read retrieves a checkpoint with key - // Read must return CheckpointNotFoundError if checkpoint is not found - Read(key string) ([]byte, error) - // Delete deletes a checkpoint with key - // Delete must not return error if checkpoint does not exist - Delete(key string) error - // List lists all keys of existing checkpoints - List() ([]string, error) -} - -// FileStore is an implementation of CheckpointStore interface which stores checkpoint in files. -type FileStore struct { - // path to the base directory for storing checkpoint files - path string -} - -func NewFileStore(path string) (CheckpointStore, error) { - if err := ensurePath(path); err != nil { - return nil, err - } - return &FileStore{path: path}, nil -} - -// writeFileAndSync is copied from ioutil.WriteFile, with the extra File.Sync -// at the end to ensure file is written on the disk. -func writeFileAndSync(filename string, data []byte, perm os.FileMode) error { - f, err := os.OpenFile(filename, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, perm) - if err != nil { - return err - } - n, err := f.Write(data) - if err == nil && n < len(data) { - err = io.ErrShortWrite - } - if err == nil { - // Only sync if the Write completed successfully. - err = f.Sync() - } - if err1 := f.Close(); err == nil { - err = err1 - } - return err -} - -func (fstore *FileStore) Write(key string, data []byte) error { - if err := validateKey(key); err != nil { - return err - } - if err := ensurePath(fstore.path); err != nil { - return err - } - tmpfile := filepath.Join(fstore.path, fmt.Sprintf("%s%s%s", tmpPrefix, key, tmpSuffix)) - if err := writeFileAndSync(tmpfile, data, 0644); err != nil { - return err - } - return os.Rename(tmpfile, fstore.getCheckpointPath(key)) -} - -func (fstore *FileStore) Read(key string) ([]byte, error) { - if err := validateKey(key); err != nil { - return nil, err - } - bytes, err := ioutil.ReadFile(fstore.getCheckpointPath(key)) - if os.IsNotExist(err) { - return bytes, errors.CheckpointNotFoundError - } - return bytes, err -} - -func (fstore *FileStore) Delete(key string) error { - if err := validateKey(key); err != nil { - return err - } - if err := os.Remove(fstore.getCheckpointPath(key)); err != nil && !os.IsNotExist(err) { - return err - } - return nil -} - -func (fstore *FileStore) List() ([]string, error) { - keys := make([]string, 0) - files, err := ioutil.ReadDir(fstore.path) - if err != nil { - return keys, err - } - for _, f := range files { - if !strings.HasPrefix(f.Name(), tmpPrefix) { - keys = append(keys, f.Name()) - } - } - return keys, nil -} - -func (fstore *FileStore) getCheckpointPath(key string) string { - return filepath.Join(fstore.path, key) -} - -// ensurePath creates input directory if it does not exist -func ensurePath(path string) error { - if _, err := os.Stat(path); err != nil { - // MkdirAll returns nil if directory already exists - return os.MkdirAll(path, 0755) - } - return nil -} - -func validateKey(key string) error { - if len(key) <= keyMaxLength && keyRegex.MatchString(key) { - return nil - } - return fmt.Errorf("checkpoint key %q is not valid.", key) -} diff --git a/pkg/kubelet/dockershim/checkpoint_store_test.go b/pkg/kubelet/dockershim/checkpoint_store_test.go deleted file mode 100644 index 97d7a5a239d..00000000000 --- a/pkg/kubelet/dockershim/checkpoint_store_test.go +++ /dev/null @@ -1,158 +0,0 @@ -/* -Copyright 2017 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 dockershim - -import ( - "io/ioutil" - "os" - "sort" - "testing" - - "github.com/stretchr/testify/assert" - "k8s.io/kubernetes/pkg/kubelet/dockershim/errors" -) - -func TestFileStore(t *testing.T) { - path, err := ioutil.TempDir("", "FileStore") - assert.NoError(t, err) - defer cleanUpTestPath(t, path) - store, err := NewFileStore(path) - assert.NoError(t, err) - - Checkpoints := []struct { - key string - data string - expectErr bool - }{ - { - "id1", - "data1", - false, - }, - { - "id2", - "data2", - false, - }, - { - "/id1", - "data1", - true, - }, - { - ".id1", - "data1", - true, - }, - { - " ", - "data2", - true, - }, - { - "___", - "data2", - true, - }, - } - - // Test Add Checkpoint - for _, c := range Checkpoints { - _, err = store.Read(c.key) - assert.Error(t, err) - - err = store.Write(c.key, []byte(c.data)) - if c.expectErr { - assert.Error(t, err) - continue - } else { - assert.NoError(t, err) - } - - // Test Read Checkpoint - data, err := store.Read(c.key) - assert.NoError(t, err) - assert.Equal(t, string(data), c.data) - } - - // Test list checkpoints. - keys, err := store.List() - assert.NoError(t, err) - sort.Strings(keys) - assert.Equal(t, keys, []string{"id1", "id2"}) - - // Test Delete Checkpoint - for _, c := range Checkpoints { - if c.expectErr { - continue - } - - err = store.Delete(c.key) - assert.NoError(t, err) - _, err = store.Read(c.key) - assert.EqualValues(t, errors.CheckpointNotFoundError, err) - } - - // Test delete non existed checkpoint - err = store.Delete("id1") - assert.NoError(t, err) - - // Test list checkpoints. - keys, err = store.List() - assert.NoError(t, err) - assert.Equal(t, len(keys), 0) -} - -func TestIsValidKey(t *testing.T) { - testcases := []struct { - key string - valid bool - }{ - { - " ", - false, - }, - { - "/foo/bar", - false, - }, - { - ".foo", - false, - }, - { - "a78768279290d33d0b82eaea43cb8346f500057cb5bd250e88c97a5585385d66", - true, - }, - } - - for _, tc := range testcases { - if tc.valid { - assert.NoError(t, validateKey(tc.key)) - } else { - assert.Error(t, validateKey(tc.key)) - } - } -} - -func cleanUpTestPath(t *testing.T, path string) { - if _, err := os.Stat(path); !os.IsNotExist(err) { - if err := os.RemoveAll(path); err != nil { - assert.NoError(t, err, "Failed to delete test directory: %v", err) - } - } -} diff --git a/pkg/kubelet/dockershim/docker_checkpoint.go b/pkg/kubelet/dockershim/docker_checkpoint.go index 6ad1d794169..0b384d037dd 100644 --- a/pkg/kubelet/dockershim/docker_checkpoint.go +++ b/pkg/kubelet/dockershim/docker_checkpoint.go @@ -24,6 +24,8 @@ import ( "github.com/golang/glog" "k8s.io/kubernetes/pkg/kubelet/dockershim/errors" + utilstore "k8s.io/kubernetes/pkg/kubelet/util/store" + utilfs "k8s.io/kubernetes/pkg/util/filesystem" hashutil "k8s.io/kubernetes/pkg/util/hash" ) @@ -82,11 +84,11 @@ type CheckpointHandler interface { // PersistentCheckpointHandler is an implementation of CheckpointHandler. It persists checkpoint in CheckpointStore type PersistentCheckpointHandler struct { - store CheckpointStore + store utilstore.Store } func NewPersistentCheckpointHandler(dockershimRootDir string) (CheckpointHandler, error) { - fstore, err := NewFileStore(filepath.Join(dockershimRootDir, sandboxCheckpointDir)) + fstore, err := utilstore.NewFileStore(filepath.Join(dockershimRootDir, sandboxCheckpointDir), utilfs.DefaultFs{}) if err != nil { return nil, err } From 9f2e29f0f0779a54a5a7a297ee2c643a783803a1 Mon Sep 17 00:00:00 2001 From: Yu-Ju Hong Date: Tue, 17 Oct 2017 10:34:07 -0700 Subject: [PATCH 3/5] Update bazel file --- pkg/kubelet/dockershim/BUILD | 5 ++--- pkg/kubelet/util/BUILD | 1 + pkg/kubelet/util/store/BUILD | 39 ++++++++++++++++++++++++++++++++++++ 3 files changed, 42 insertions(+), 3 deletions(-) create mode 100644 pkg/kubelet/util/store/BUILD diff --git a/pkg/kubelet/dockershim/BUILD b/pkg/kubelet/dockershim/BUILD index a2c566599aa..e58756a2290 100644 --- a/pkg/kubelet/dockershim/BUILD +++ b/pkg/kubelet/dockershim/BUILD @@ -9,7 +9,6 @@ load( go_library( name = "go_default_library", srcs = [ - "checkpoint_store.go", "convert.go", "doc.go", "docker_checkpoint.go", @@ -61,7 +60,9 @@ go_library( "//pkg/kubelet/types:go_default_library", "//pkg/kubelet/util/cache:go_default_library", "//pkg/kubelet/util/ioutils:go_default_library", + "//pkg/kubelet/util/store:go_default_library", "//pkg/security/apparmor:go_default_library", + "//pkg/util/filesystem:go_default_library", "//pkg/util/hash:go_default_library", "//pkg/util/parsers:go_default_library", "//vendor/github.com/blang/semver:go_default_library", @@ -82,7 +83,6 @@ go_library( go_test( name = "go_default_test", srcs = [ - "checkpoint_store_test.go", "convert_test.go", "docker_checkpoint_test.go", "docker_container_test.go", @@ -109,7 +109,6 @@ go_test( "//pkg/kubelet/apis/cri/v1alpha1/runtime:go_default_library", "//pkg/kubelet/container:go_default_library", "//pkg/kubelet/container/testing:go_default_library", - "//pkg/kubelet/dockershim/errors:go_default_library", "//pkg/kubelet/dockershim/libdocker:go_default_library", "//pkg/kubelet/dockershim/testing:go_default_library", "//pkg/kubelet/network:go_default_library", diff --git a/pkg/kubelet/util/BUILD b/pkg/kubelet/util/BUILD index fdb78bddfba..0ab75084df0 100644 --- a/pkg/kubelet/util/BUILD +++ b/pkg/kubelet/util/BUILD @@ -57,6 +57,7 @@ filegroup( "//pkg/kubelet/util/ioutils:all-srcs", "//pkg/kubelet/util/queue:all-srcs", "//pkg/kubelet/util/sliceutils:all-srcs", + "//pkg/kubelet/util/store:all-srcs", ], tags = ["automanaged"], ) diff --git a/pkg/kubelet/util/store/BUILD b/pkg/kubelet/util/store/BUILD new file mode 100644 index 00000000000..33bb61d6c2a --- /dev/null +++ b/pkg/kubelet/util/store/BUILD @@ -0,0 +1,39 @@ +load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test") + +go_library( + name = "go_default_library", + srcs = [ + "doc.go", + "filestore.go", + "store.go", + ], + visibility = ["//visibility:public"], + deps = ["//pkg/util/filesystem:go_default_library"], +) + +go_test( + name = "go_default_test", + srcs = [ + "filestore_test.go", + "store_test.go", + ], + library = ":go_default_library", + deps = [ + "//pkg/util/filesystem:go_default_library", + "//vendor/github.com/stretchr/testify/assert:go_default_library", + ], +) + +filegroup( + name = "package-srcs", + srcs = glob(["**"]), + tags = ["automanaged"], + visibility = ["//visibility:private"], +) + +filegroup( + name = "all-srcs", + srcs = [":package-srcs"], + tags = ["automanaged"], + visibility = ["//visibility:public"], +) From 5cbca87add6bb2f0ac84cd007b199c2b7421a1d3 Mon Sep 17 00:00:00 2001 From: Yu-Ju Hong Date: Wed, 18 Oct 2017 16:11:21 -0700 Subject: [PATCH 4/5] Address comments, fix lint failures, and bazel complaints --- pkg/kubelet/util/store/BUILD | 2 ++ pkg/kubelet/util/store/doc.go | 2 +- pkg/kubelet/util/store/filestore.go | 13 +++++++++---- pkg/kubelet/util/store/filestore_test.go | 8 ++++---- pkg/kubelet/util/store/store.go | 19 +++++++++++++++---- pkg/kubelet/util/store/store_test.go | 8 ++++++++ 6 files changed, 39 insertions(+), 13 deletions(-) diff --git a/pkg/kubelet/util/store/BUILD b/pkg/kubelet/util/store/BUILD index 33bb61d6c2a..8f4e66a5230 100644 --- a/pkg/kubelet/util/store/BUILD +++ b/pkg/kubelet/util/store/BUILD @@ -7,6 +7,7 @@ go_library( "filestore.go", "store.go", ], + importpath = "k8s.io/kubernetes/pkg/kubelet/util/store", visibility = ["//visibility:public"], deps = ["//pkg/util/filesystem:go_default_library"], ) @@ -17,6 +18,7 @@ go_test( "filestore_test.go", "store_test.go", ], + importpath = "k8s.io/kubernetes/pkg/kubelet/util/store", library = ":go_default_library", deps = [ "//pkg/util/filesystem:go_default_library", diff --git a/pkg/kubelet/util/store/doc.go b/pkg/kubelet/util/store/doc.go index d9f1cedfd8f..b4d8523a097 100644 --- a/pkg/kubelet/util/store/doc.go +++ b/pkg/kubelet/util/store/doc.go @@ -14,5 +14,5 @@ See the License for the specific language governing permissions and limitations under the License. */ -// A store interface. +// Package store hosts a Store interface and its implementations. package store // import "k8s.io/kubernetes/pkg/kubelet/util/store" diff --git a/pkg/kubelet/util/store/filestore.go b/pkg/kubelet/util/store/filestore.go index 94fe222ae32..fa4765fd353 100644 --- a/pkg/kubelet/util/store/filestore.go +++ b/pkg/kubelet/util/store/filestore.go @@ -39,6 +39,7 @@ type FileStore struct { filesystem utilfs.Filesystem } +// NewFileStore returns an instance of FileStore. func NewFileStore(path string, fs utilfs.Filesystem) (Store, error) { if err := ensureDirectory(fs, path); err != nil { return nil, err @@ -46,6 +47,7 @@ func NewFileStore(path string, fs utilfs.Filesystem) (Store, error) { return &FileStore{directoryPath: path, filesystem: fs}, nil } +// Write writes the given data to a file named key. func (f *FileStore) Write(key string, data []byte) error { if err := ValidateKey(key); err != nil { return err @@ -57,17 +59,19 @@ func (f *FileStore) Write(key string, data []byte) error { return writeFile(f.filesystem, f.getPathByKey(key), data) } +// Read reads the data from the file named key. func (f *FileStore) Read(key string) ([]byte, error) { if err := ValidateKey(key); err != nil { return nil, err } bytes, err := f.filesystem.ReadFile(f.getPathByKey(key)) if os.IsNotExist(err) { - return bytes, KeyNotFoundError + return bytes, ErrKeyNotFound } return bytes, err } +// Delete deletes the key file. func (f *FileStore) Delete(key string) error { if err := ValidateKey(key); err != nil { return err @@ -75,6 +79,7 @@ func (f *FileStore) Delete(key string) error { return removePath(f.filesystem, f.getPathByKey(key)) } +// List returns all keys in the store. func (f *FileStore) List() ([]string, error) { keys := make([]string, 0) files, err := f.filesystem.ReadDir(f.directoryPath) @@ -89,6 +94,7 @@ func (f *FileStore) List() ([]string, error) { return keys, nil } +// getPathByKey returns the full path of the file for the key. func (f *FileStore) getPathByKey(key string) string { return filepath.Join(f.directoryPath, key) } @@ -110,6 +116,8 @@ func writeFile(fs utilfs.Filesystem, path string, data []byte) (retErr error) { return err } + tmpPath := tmpFile.Name() + defer func() { // Close the file. if err := tmpFile.Close(); err != nil { @@ -119,10 +127,7 @@ func writeFile(fs utilfs.Filesystem, path string, data []byte) (retErr error) { retErr = fmt.Errorf("failed to close temp file after error %v", retErr) } } - }() - tmpPath := tmpFile.Name() - defer func() { // Clean up the temp file on error. if retErr != nil && tmpPath != "" { if err := removePath(fs, tmpPath); err != nil { diff --git a/pkg/kubelet/util/store/filestore_test.go b/pkg/kubelet/util/store/filestore_test.go index 567fb253d83..f5dcccc5a10 100644 --- a/pkg/kubelet/util/store/filestore_test.go +++ b/pkg/kubelet/util/store/filestore_test.go @@ -31,7 +31,7 @@ func TestFileStore(t *testing.T) { store, err := NewFileStore(path, filesystem.NewFakeFs()) assert.NoError(t, err) - testData := []struct { + testCases := []struct { key string data string expectErr bool @@ -69,7 +69,7 @@ func TestFileStore(t *testing.T) { } // Test add data. - for _, c := range testData { + for _, c := range testCases { _, err = store.Read(c.key) assert.Error(t, err) @@ -94,7 +94,7 @@ func TestFileStore(t *testing.T) { assert.Equal(t, keys, []string{"id1", "id2"}) // Test Delete data - for _, c := range testData { + for _, c := range testCases { if c.expectErr { continue } @@ -102,7 +102,7 @@ func TestFileStore(t *testing.T) { err = store.Delete(c.key) assert.NoError(t, err) _, err = store.Read(c.key) - assert.EqualValues(t, KeyNotFoundError, err) + assert.EqualValues(t, ErrKeyNotFound, err) } // Test delete non-existent key. diff --git a/pkg/kubelet/util/store/store.go b/pkg/kubelet/util/store/store.go index 62072643563..e797fd812b4 100644 --- a/pkg/kubelet/util/store/store.go +++ b/pkg/kubelet/util/store/store.go @@ -21,12 +21,21 @@ import ( "regexp" ) -const keyMaxLength = 250 +const ( + keyMaxLength = 250 -var keyRegex = regexp.MustCompile("^[a-zA-Z0-9]+$") + keyCharFmt string = "[A-Za-z0-9]" + keyExtCharFmt string = "[-A-Za-z0-9_.]" + qualifiedKeyFmt string = "(" + keyCharFmt + keyExtCharFmt + "*)?" + keyCharFmt +) var ( - KeyNotFoundError = fmt.Errorf("key is not found.") + // Key must consist of alphanumeric characters, '-', '_' or '.', and must start + // and end with an alphanumeric character. + keyRegex = regexp.MustCompile("^" + qualifiedKeyFmt + "$") + + // ErrKeyNotFound is the error returned if key is not found in Store. + ErrKeyNotFound = fmt.Errorf("key is not found") ) // Store provides the interface for storing keyed data. @@ -36,7 +45,7 @@ type Store interface { // Write writes data with key. Write(key string, data []byte) error // Read retrieves data with key - // Read must return KeyNotFoundError if key is not found. + // Read must return ErrKeyNotFound if key is not found. Read(key string) ([]byte, error) // Delete deletes data by key // Delete must not return error if key does not exist @@ -45,6 +54,8 @@ type Store interface { List() ([]string, error) } +// ValidateKey returns an error if the given key does not meet the requirement +// of the key format and length. func ValidateKey(key string) error { if len(key) <= keyMaxLength && keyRegex.MatchString(key) { return nil diff --git a/pkg/kubelet/util/store/store_test.go b/pkg/kubelet/util/store/store_test.go index e19afd3ad85..a9c6e9c14b7 100644 --- a/pkg/kubelet/util/store/store_test.go +++ b/pkg/kubelet/util/store/store_test.go @@ -43,6 +43,14 @@ func TestIsValidKey(t *testing.T) { "a78768279290d33d0b82eaea43cb8346f500057cb5bd250e88c97a5585385d66", true, }, + { + "a7.87-6_8", + true, + }, + { + "a7.87-677-", + false, + }, } for _, tc := range testcases { From 4b5a3ee2e57738b01314cafd97dc05f23dff8e5f Mon Sep 17 00:00:00 2001 From: Yu-Ju Hong Date: Wed, 25 Oct 2017 10:07:32 -0700 Subject: [PATCH 5/5] Address more comments --- pkg/kubelet/util/store/filestore.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/kubelet/util/store/filestore.go b/pkg/kubelet/util/store/filestore.go index fa4765fd353..a78afa17739 100644 --- a/pkg/kubelet/util/store/filestore.go +++ b/pkg/kubelet/util/store/filestore.go @@ -124,14 +124,14 @@ func writeFile(fs utilfs.Filesystem, path string, data []byte) (retErr error) { if retErr == nil { retErr = err } else { - retErr = fmt.Errorf("failed to close temp file after error %v", retErr) + retErr = fmt.Errorf("failed to close temp file after error %v; close error: %v", retErr, err) } } // Clean up the temp file on error. if retErr != nil && tmpPath != "" { if err := removePath(fs, tmpPath); err != nil { - retErr = fmt.Errorf("failed to remove the temporary file after error %v", retErr) + retErr = fmt.Errorf("failed to remove the temporary file (%q) after error %v; remove error: %v", tmpPath, retErr, err) } } }()