diff --git a/pkg/metadata/image_metadata.go b/pkg/metadata/image_metadata.go index a4df99e2d..5cff42fb0 100644 --- a/pkg/metadata/image_metadata.go +++ b/pkg/metadata/image_metadata.go @@ -108,11 +108,6 @@ func (s *imageMetadataStore) Get(digest string) (*ImageMetadata, error) { if err != nil { return nil, err } - // Return nil without error if the corresponding metadata - // does not exist. - if data == nil { - return nil, nil - } imageMetadata := &ImageMetadata{} if err := json.Unmarshal(data, imageMetadata); err != nil { return nil, err diff --git a/pkg/metadata/image_metadata_test.go b/pkg/metadata/image_metadata_test.go index 3c43b5151..17b49cc7a 100644 --- a/pkg/metadata/image_metadata_test.go +++ b/pkg/metadata/image_metadata_test.go @@ -80,8 +80,8 @@ func TestImageMetadataStore(t *testing.T) { assert.NoError(err) assert.Len(imgs, 2) - t.Logf("get should return nil without error after deletion") + t.Logf("get should return nil not exist error after deletion") meta, err := s.Get(testID) - assert.NoError(err) + assert.Error(store.ErrNotExist, err) assert.Nil(meta) } diff --git a/pkg/metadata/sandbox.go b/pkg/metadata/sandbox.go index b7b308933..31e29c320 100644 --- a/pkg/metadata/sandbox.go +++ b/pkg/metadata/sandbox.go @@ -115,11 +115,6 @@ func (s *sandboxStore) Get(sandboxID string) (*SandboxMetadata, error) { if err != nil { return nil, err } - // Return nil without error if the corresponding metadata - // does not exist. - if data == nil { - return nil, nil - } sandbox := &SandboxMetadata{} if err := json.Unmarshal(data, sandbox); err != nil { return nil, err diff --git a/pkg/metadata/sandbox_test.go b/pkg/metadata/sandbox_test.go index 8eb1cab86..133554f0a 100644 --- a/pkg/metadata/sandbox_test.go +++ b/pkg/metadata/sandbox_test.go @@ -113,8 +113,8 @@ func TestSandboxStore(t *testing.T) { assert.NoError(err) assert.Len(sbs, 2) - t.Logf("get should return nil without error after deletion") + t.Logf("get should return nil with not exist error after deletion") meta, err := s.Get(testID) - assert.NoError(err) + assert.Error(store.ErrNotExist, err) assert.Nil(meta) } diff --git a/pkg/metadata/store/metadata_store.go b/pkg/metadata/store/metadata_store.go index 19b5ec02b..7449afe5b 100644 --- a/pkg/metadata/store/metadata_store.go +++ b/pkg/metadata/store/metadata_store.go @@ -17,12 +17,21 @@ limitations under the License. package store import ( - "fmt" + "errors" "sync" "github.com/golang/glog" ) +var ( + // ErrNotExist is the error returned when specified id does + // not exist. + ErrNotExist = errors.New("does not exist") + // ErrAlreadyExist is the error returned when specified id already + // exists. + ErrAlreadyExist = errors.New("already exists") +) + // All byte arrays are expected to be read-only. User MUST NOT modify byte // array element directly!! @@ -46,11 +55,12 @@ type MetadataStore interface { // * The id and data MUST be added in one transaction to the store. Create(string, []byte) error // Get the data by id. - // Note that Get MUST return nil without error if the id - // doesn't exist. + // Note that Get MUST return ErrNotExist if the id doesn't exist. Get(string) ([]byte, error) - // Update the data by id. Note that the update MUST be applied in - // one transaction. + // Update the data by id. + // Note: + // * Update MUST return ErrNotExist is the id doesn't exist. + // * The update MUST be applied in one transaction. Update(string, UpdateFunc) error // List returns entire array of data from the store. List() ([][]byte, error) @@ -135,7 +145,7 @@ func (m *metadataStore) createMetadata(id string, meta *metadata) error { m.Lock() defer m.Unlock() if _, found := m.metas[id]; found { - return fmt.Errorf("id %q already exists", id) + return ErrAlreadyExist } m.metas[id] = meta return nil @@ -172,7 +182,7 @@ func (m *metadataStore) getMetadata(id string) (*metadata, bool) { func (m *metadataStore) Get(id string) ([]byte, error) { meta, found := m.getMetadata(id) if !found { - return nil, nil + return nil, ErrNotExist } return meta.get(), nil } @@ -181,7 +191,7 @@ func (m *metadataStore) Get(id string) ([]byte, error) { func (m *metadataStore) Update(id string, u UpdateFunc) error { meta, found := m.getMetadata(id) if !found { - return fmt.Errorf("id %q doesn't exist", id) + return ErrNotExist } return meta.update(u) } diff --git a/pkg/metadata/store/metadata_store_test.go b/pkg/metadata/store/metadata_store_test.go index 2b8491dac..c507d0804 100644 --- a/pkg/metadata/store/metadata_store_test.go +++ b/pkg/metadata/store/metadata_store_test.go @@ -97,9 +97,10 @@ func TestMetadataStore(t *testing.T) { assert.Equal(testMeta[testIds[1]], meta) t.Logf("update should take effect") - m.Update(testIds[1], func(in []byte) ([]byte, error) { + err = m.Update(testIds[1], func(in []byte) ([]byte, error) { return []byte("updated-metadata-1"), nil }) + assert.NoError(err) newMeta, err := m.Get(testIds[1]) assert.NoError(err) assert.Equal([]byte("updated-metadata-1"), newMeta) @@ -111,9 +112,15 @@ func TestMetadataStore(t *testing.T) { assert.Len(metas, 1) assert.Equal(testMeta[testIds[0]], metas[0]) meta, err = m.Get(testIds[1]) - assert.NoError(err) + assert.Equal(ErrNotExist, err) assert.Nil(meta) + t.Logf("update should return not exist error after metadata got deleted") + err = m.Update(testIds[1], func(in []byte) ([]byte, error) { + return in, nil + }) + assert.Equal(ErrNotExist, err) + t.Logf("existing reference should not be affected by delete") assert.Equal([]byte("updated-metadata-1"), newMeta) @@ -181,9 +188,14 @@ func TestMultithreadAccess(t *testing.T) { assert.NoError(err) got, err = m.Get(id) - assert.NoError(err) + assert.Equal(ErrNotExist, err) assert.Nil(got) + err = m.Update(id, func(in []byte) ([]byte, error) { + return in, nil + }) + assert.Equal(ErrNotExist, err) + gotList, err = m.List() assert.NoError(err) assert.NotContains(gotList, expect) diff --git a/pkg/metadata/util.go b/pkg/metadata/util.go new file mode 100644 index 000000000..00b5be66e --- /dev/null +++ b/pkg/metadata/util.go @@ -0,0 +1,25 @@ +/* +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 metadata + +import "github.com/kubernetes-incubator/cri-containerd/pkg/metadata/store" + +// IsNotExistError is a helper function to check whether the error returned +// by metadata store is not exist error. +func IsNotExistError(err error) bool { + return err.Error() == store.ErrNotExist.Error() +} diff --git a/pkg/metadata/util_test.go b/pkg/metadata/util_test.go new file mode 100644 index 000000000..999ddd1b7 --- /dev/null +++ b/pkg/metadata/util_test.go @@ -0,0 +1,35 @@ +/* +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 metadata + +import ( + "errors" + "testing" + + "github.com/stretchr/testify/assert" + + "github.com/kubernetes-incubator/cri-containerd/pkg/metadata/store" +) + +func TestIsNotExistError(t *testing.T) { + err := store.ErrNotExist + assert.True(t, IsNotExistError(err)) + err = errors.New(store.ErrNotExist.Error()) + assert.True(t, IsNotExistError(err)) + err = errors.New("random error") + assert.False(t, IsNotExistError(err)) +} diff --git a/pkg/server/helpers.go b/pkg/server/helpers.go index 30e0e717b..54cb7074a 100644 --- a/pkg/server/helpers.go +++ b/pkg/server/helpers.go @@ -107,17 +107,18 @@ func isContainerdContainerNotExistError(grpcError error) bool { // retry if the sandbox metadata is not found with the initial id. func (c *criContainerdService) getSandbox(id string) (*metadata.SandboxMetadata, error) { sandbox, err := c.sandboxStore.Get(id) - if err != nil { + if err != nil && !metadata.IsNotExistError(err) { return nil, fmt.Errorf("sandbox metadata not found: %v", err) } - if sandbox != nil { + if err == nil { return sandbox, nil } // sandbox is not found in metadata store, try to extract full id. - id, err = c.sandboxIDIndex.Get(id) - if err != nil { - if err == truncindex.ErrNotExist { - return nil, nil + id, indexErr := c.sandboxIDIndex.Get(id) + if indexErr != nil { + if indexErr == truncindex.ErrNotExist { + // Return the original error if sandbox id is not found. + return nil, err } return nil, fmt.Errorf("sandbox id not found: %v", err) } diff --git a/pkg/server/helpers_test.go b/pkg/server/helpers_test.go index b14a9a8fb..b1ccb40cc 100644 --- a/pkg/server/helpers_test.go +++ b/pkg/server/helpers_test.go @@ -35,25 +35,34 @@ func TestGetSandbox(t *testing.T) { assert.NoError(t, c.sandboxIDIndex.Add(testID)) for desc, test := range map[string]struct { - id string - expected *metadata.SandboxMetadata + id string + expected *metadata.SandboxMetadata + expectErr bool }{ "full id": { - id: testID, - expected: &testSandbox, + id: testID, + expected: &testSandbox, + expectErr: false, }, "partial id": { - id: testID[:3], - expected: &testSandbox, + id: testID[:3], + expected: &testSandbox, + expectErr: false, }, "non-exist id": { - id: "gfedcba", - expected: nil, + id: "gfedcba", + expected: nil, + expectErr: true, }, } { t.Logf("TestCase %q", desc) sb, err := c.getSandbox(test.id) - assert.NoError(t, err) + if test.expectErr { + assert.Error(t, err) + assert.True(t, metadata.IsNotExistError(err)) + } else { + assert.NoError(t, err) + } assert.Equal(t, test.expected, sb) } } diff --git a/pkg/server/sandbox_remove.go b/pkg/server/sandbox_remove.go index 13ed8882f..eb11cf292 100644 --- a/pkg/server/sandbox_remove.go +++ b/pkg/server/sandbox_remove.go @@ -25,6 +25,8 @@ import ( "github.com/containerd/containerd/api/services/execution" "k8s.io/kubernetes/pkg/kubelet/api/v1alpha1/runtime" + + "github.com/kubernetes-incubator/cri-containerd/pkg/metadata" ) // RemovePodSandbox removes the sandbox. If there are running containers in the @@ -39,9 +41,10 @@ func (c *criContainerdService) RemovePodSandbox(ctx context.Context, r *runtime. sandbox, err := c.getSandbox(r.GetPodSandboxId()) if err != nil { - return nil, fmt.Errorf("failed to find sandbox %q: %v", r.GetPodSandboxId(), err) - } - if sandbox == nil { + if !metadata.IsNotExistError(err) { + return nil, fmt.Errorf("an error occurred when try to find sandbox %q: %v", + r.GetPodSandboxId(), err) + } // Do not return error if the id doesn't exist. glog.V(5).Infof("RemovePodSandbox called for sandbox %q that does not exist", r.GetPodSandboxId()) diff --git a/pkg/server/sandbox_remove_test.go b/pkg/server/sandbox_remove_test.go index 7820919af..ee0b42ab4 100644 --- a/pkg/server/sandbox_remove_test.go +++ b/pkg/server/sandbox_remove_test.go @@ -111,7 +111,8 @@ func TestRemovePodSandbox(t *testing.T) { _, err = c.sandboxIDIndex.Get(testID) assert.Error(t, err, "sandbox id should be removed") meta, err := c.sandboxStore.Get(testID) - assert.NoError(t, err) + assert.Error(t, err) + assert.True(t, metadata.IsNotExistError(err)) assert.Nil(t, meta, "sandbox metadata should be removed") res, err = c.RemovePodSandbox(context.Background(), &runtime.RemovePodSandboxRequest{ PodSandboxId: testID, diff --git a/pkg/server/sandbox_status.go b/pkg/server/sandbox_status.go index e1ecb5ce1..a54c4a608 100644 --- a/pkg/server/sandbox_status.go +++ b/pkg/server/sandbox_status.go @@ -41,10 +41,8 @@ func (c *criContainerdService) PodSandboxStatus(ctx context.Context, r *runtime. sandbox, err := c.getSandbox(r.GetPodSandboxId()) if err != nil { - return nil, fmt.Errorf("failed to find sandbox %q: %v", r.GetPodSandboxId(), err) - } - if sandbox == nil { - return nil, fmt.Errorf("sandbox %q does not exist", r.GetPodSandboxId()) + return nil, fmt.Errorf("an error occurred when try to find sandbox %q: %v", + r.GetPodSandboxId(), err) } // Use the full sandbox id. id := sandbox.ID diff --git a/pkg/server/sandbox_stop.go b/pkg/server/sandbox_stop.go index c45f1cff6..53285634a 100644 --- a/pkg/server/sandbox_stop.go +++ b/pkg/server/sandbox_stop.go @@ -39,10 +39,8 @@ func (c *criContainerdService) StopPodSandbox(ctx context.Context, r *runtime.St sandbox, err := c.getSandbox(r.GetPodSandboxId()) if err != nil { - return nil, fmt.Errorf("failed to find sandbox %q: %v", r.GetPodSandboxId(), err) - } - if sandbox == nil { - return nil, fmt.Errorf("sandbox %q does not exist", r.GetPodSandboxId()) + return nil, fmt.Errorf("an error occurred when try to find sandbox %q: %v", + r.GetPodSandboxId(), err) } // Use the full sandbox id. id := sandbox.ID