Merge pull request #38 from Random-Liu/add-not-exist-error

Return not exist error in metadata store

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

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)
 }

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

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)
 }

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
+	// Note that Get MUST return ErrNotExist if the id doesn't exist.
-	// doesn't exist.
 	Get(string) ([]byte, error)
-	// Update the data by id. Note that the update MUST be applied in
+	// Update the data by id.
-	// one transaction.
+	// 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)
 }

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)

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()
+}

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))
+}

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)
+	id, indexErr := c.sandboxIDIndex.Get(id)
-	if err != nil {
+	if indexErr != nil {
-		if err == truncindex.ErrNotExist {
+		if indexErr == truncindex.ErrNotExist {
-			return nil, nil
+			// Return the original error if sandbox id is not found.
+			return nil, err
 		}
 		return nil, fmt.Errorf("sandbox id not found: %v", err)
 	}

pkg/server/helpers_test.go

@@ -37,23 +37,32 @@ func TestGetSandbox(t *testing.T) {
 	for desc, test := range map[string]struct {
 		id        string
 		expected  *metadata.SandboxMetadata
+		expectErr bool
 	}{
 		"full id": {
 			id:        testID,
 			expected:  &testSandbox,
+			expectErr: false,
 		},
 		"partial id": {
 			id:        testID[:3],
 			expected:  &testSandbox,
+			expectErr: false,
 		},
 		"non-exist id": {
 			id:        "gfedcba",
 			expected:  nil,
+			expectErr: true,
 		},
 	} {
 		t.Logf("TestCase %q", desc)
 		sb, err := c.getSandbox(test.id)
+		if test.expectErr {
+			assert.Error(t, err)
+			assert.True(t, metadata.IsNotExistError(err))
+		} else {
 			assert.NoError(t, err)
+		}
 		assert.Equal(t, test.expected, sb)
 	}
 }

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 !metadata.IsNotExistError(err) {
+			return nil, fmt.Errorf("an error occurred when try to find sandbox %q: %v",
+				r.GetPodSandboxId(), err)
 		}
-	if sandbox == nil {
 		// 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())

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,

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)
+		return nil, fmt.Errorf("an error occurred when try to find sandbox %q: %v",
-	}
+			r.GetPodSandboxId(), err)
-	if sandbox == nil {
-		return nil, fmt.Errorf("sandbox %q does not exist", r.GetPodSandboxId())
 	}
 	// Use the full sandbox id.
 	id := sandbox.ID

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)
+		return nil, fmt.Errorf("an error occurred when try to find sandbox %q: %v",
-	}
+			r.GetPodSandboxId(), err)
-	if sandbox == nil {
-		return nil, fmt.Errorf("sandbox %q does not exist", r.GetPodSandboxId())
 	}
 	// Use the full sandbox id.
 	id := sandbox.ID