Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Abstract storage error handling #18163

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@
.idea/
*.iml

# Vscode files
.vscode/**

# This is where the result of the go build goes
/output/**
/output
Expand Down
32 changes: 16 additions & 16 deletions pkg/api/errors/etcd/etcd.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,68 +18,68 @@ package etcd

import (
"k8s.io/kubernetes/pkg/api/errors"
etcdutil "k8s.io/kubernetes/pkg/storage/etcd/util"
"k8s.io/kubernetes/pkg/storage"
)

// InterpretListError converts a generic etcd error on a retrieval
// InterpretListError converts a generic error on a retrieval
// operation into the appropriate API error.
func InterpretListError(err error, kind string) error {
switch {
case etcdutil.IsEtcdNotFound(err):
case storage.IsNotFound(err):
return errors.NewNotFound(kind, "")
case etcdutil.IsEtcdUnreachable(err):
case storage.IsUnreachable(err):
return errors.NewServerTimeout(kind, "list", 2) // TODO: make configurable or handled at a higher level
default:
return err
}
}

// InterpretGetError converts a generic etcd error on a retrieval
// InterpretGetError converts a generic error on a retrieval
// operation into the appropriate API error.
func InterpretGetError(err error, kind, name string) error {
switch {
case etcdutil.IsEtcdNotFound(err):
case storage.IsNotFound(err):
return errors.NewNotFound(kind, name)
case etcdutil.IsEtcdUnreachable(err):
case storage.IsUnreachable(err):
return errors.NewServerTimeout(kind, "get", 2) // TODO: make configurable or handled at a higher level
default:
return err
}
}

// InterpretCreateError converts a generic etcd error on a create
// InterpretCreateError converts a generic error on a create
// operation into the appropriate API error.
func InterpretCreateError(err error, kind, name string) error {
switch {
case etcdutil.IsEtcdNodeExist(err):
case storage.IsNodeExist(err):
return errors.NewAlreadyExists(kind, name)
case etcdutil.IsEtcdUnreachable(err):
case storage.IsUnreachable(err):
return errors.NewServerTimeout(kind, "create", 2) // TODO: make configurable or handled at a higher level
default:
return err
}
}

// InterpretUpdateError converts a generic etcd error on a update
// InterpretUpdateError converts a generic error on a update
// operation into the appropriate API error.
func InterpretUpdateError(err error, kind, name string) error {
switch {
case etcdutil.IsEtcdTestFailed(err), etcdutil.IsEtcdNodeExist(err):
case storage.IsTestFailed(err), storage.IsNodeExist(err):
return errors.NewConflict(kind, name, err)
case etcdutil.IsEtcdUnreachable(err):
case storage.IsUnreachable(err):
return errors.NewServerTimeout(kind, "update", 2) // TODO: make configurable or handled at a higher level
default:
return err
}
}

// InterpretDeleteError converts a generic etcd error on a delete
// InterpretDeleteError converts a generic error on a delete
// operation into the appropriate API error.
func InterpretDeleteError(err error, kind, name string) error {
switch {
case etcdutil.IsEtcdNotFound(err):
case storage.IsNotFound(err):
return errors.NewNotFound(kind, name)
case etcdutil.IsEtcdUnreachable(err):
case storage.IsUnreachable(err):
return errors.NewServerTimeout(kind, "delete", 2) // TODO: make configurable or handled at a higher level
default:
return err
Expand Down
4 changes: 2 additions & 2 deletions pkg/apiserver/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import (
"net/http"

"k8s.io/kubernetes/pkg/api/unversioned"
etcdutil "k8s.io/kubernetes/pkg/storage/etcd/util"
"k8s.io/kubernetes/pkg/storage"
"k8s.io/kubernetes/pkg/util"
)

Expand Down Expand Up @@ -52,7 +52,7 @@ func errToAPIStatus(err error) *unversioned.Status {
status := http.StatusInternalServerError
switch {
//TODO: replace me with NewConflictErr
case etcdutil.IsEtcdTestFailed(err):
case storage.IsTestFailed(err):
status = http.StatusConflict
}
// Log errors that were not converted to an error status
Expand Down
1 change: 1 addition & 0 deletions pkg/master/master.go
Original file line number Diff line number Diff line change
Expand Up @@ -836,6 +836,7 @@ func (m *Master) getServersToValidate(c *Config) map[string]apiserver.Server {
addr = etcdUrl.Host
port = 4001
}
// TODO: etcd health checking should be abstracted in the storage tier
serversToValidate[fmt.Sprintf("etcd-%d", ix)] = apiserver.Server{Addr: addr, Port: port, Path: "/health", Validate: etcdutil.EtcdHealthCheck}
}
return serversToValidate
Expand Down
5 changes: 2 additions & 3 deletions pkg/master/master_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@ import (
etcdstorage "k8s.io/kubernetes/pkg/storage/etcd"
"k8s.io/kubernetes/pkg/storage/etcd/etcdtest"
etcdtesting "k8s.io/kubernetes/pkg/storage/etcd/testing"
etcdutil "k8s.io/kubernetes/pkg/storage/etcd/util"
"k8s.io/kubernetes/pkg/util"
"k8s.io/kubernetes/pkg/util/intstr"

Expand Down Expand Up @@ -789,7 +788,7 @@ func testInstallThirdPartyAPIDeleteVersion(t *testing.T, version string) {
thirdPartyObj := extensions.ThirdPartyResourceData{}
err = master.thirdPartyStorage.Get(
context.TODO(), expectedDeletedKey, &thirdPartyObj, false)
if !etcdutil.IsEtcdNotFound(err) {
if !storage.IsNotFound(err) {
t.Errorf("expected deletion didn't happen: %v", err)
}
}
Expand Down Expand Up @@ -876,7 +875,7 @@ func testInstallThirdPartyResourceRemove(t *testing.T, version string) {
for _, key := range expectedDeletedKeys {
thirdPartyObj := extensions.ThirdPartyResourceData{}
err := master.thirdPartyStorage.Get(context.TODO(), key, &thirdPartyObj, false)
if !etcdutil.IsEtcdNotFound(err) {
if !storage.IsNotFound(err) {
t.Errorf("expected deletion didn't happen: %v", err)
}
}
Expand Down
3 changes: 1 addition & 2 deletions pkg/registry/service/allocator/etcd/etcd.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ import (
"k8s.io/kubernetes/pkg/registry/service/allocator"
"k8s.io/kubernetes/pkg/runtime"
"k8s.io/kubernetes/pkg/storage"
etcdutil "k8s.io/kubernetes/pkg/storage/etcd/util"

"golang.org/x/net/context"
)
Expand Down Expand Up @@ -174,7 +173,7 @@ func (e *Etcd) Refresh() (*api.RangeAllocation, error) {

existing := &api.RangeAllocation{}
if err := e.storage.Get(context.TODO(), e.baseKey, existing, false); err != nil {
if etcdutil.IsEtcdNotFound(err) {
if storage.IsNotFound(err) {
return nil, nil
}
return nil, etcderr.InterpretGetError(err, e.kind, "")
Expand Down
45 changes: 45 additions & 0 deletions pkg/storage/errors.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
/*
Copyright 2015 The Kubernetes Authors All rights reserved.

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 storage

import (
etcdutil "k8s.io/kubernetes/pkg/storage/etcd/util"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering whether we should completely get rid of etcdutil.IsEtcd* functions. But I guess it makes some sense to have them...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the 1st push to abstract via 1 layer of indirection, as time progresses I'm guessing it will reduce further.

)

// IsNotFound returns true if and only if err is "key" not found error.
func IsNotFound(err error) bool {
// TODO: add alternate storage error here
return etcdutil.IsEtcdNotFound(err)
}

// IsNodeExist returns true if and only if err is an node already exist error.
func IsNodeExist(err error) bool {
// TODO: add alternate storage error here
return etcdutil.IsEtcdNodeExist(err)
}

// IsUnreachable returns true if and only if err indicates the server could not be reached.
func IsUnreachable(err error) bool {
// TODO: add alternate storage error here
return etcdutil.IsEtcdUnreachable(err)
}

// IsTestFailed returns true if and only if err is a write conflict.
func IsTestFailed(err error) bool {
// TODO: add alternate storage error here
return etcdutil.IsEtcdTestFailed(err)
}