From 3ffe259ac773f5c9ec91d6e8b5751020a05af56b Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Fri, 5 Sep 2014 15:30:35 -0400 Subject: [PATCH] Return standard API errors from etcd registry by operation Adds pkg/api/errors/etcd, which defines default conversions for common CRUD operations from etcd to api. --- pkg/api/errors/etcd/doc.go | 18 +++++++++ pkg/api/errors/etcd/etcd.go | 66 +++++++++++++++++++++++++++++++ pkg/registry/etcd/etcd.go | 71 ++++++++++++---------------------- pkg/registry/etcd/etcd_test.go | 24 ++++++------ 4 files changed, 121 insertions(+), 58 deletions(-) create mode 100644 pkg/api/errors/etcd/doc.go create mode 100644 pkg/api/errors/etcd/etcd.go diff --git a/pkg/api/errors/etcd/doc.go b/pkg/api/errors/etcd/doc.go new file mode 100644 index 00000000000..fc9b7f917f8 --- /dev/null +++ b/pkg/api/errors/etcd/doc.go @@ -0,0 +1,18 @@ +/* +Copyright 2014 Google Inc. 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 etcd provides conversion of etcd errors to API errors. +package etcd diff --git a/pkg/api/errors/etcd/etcd.go b/pkg/api/errors/etcd/etcd.go new file mode 100644 index 00000000000..dee022828f5 --- /dev/null +++ b/pkg/api/errors/etcd/etcd.go @@ -0,0 +1,66 @@ +/* +Copyright 2014 Google Inc. 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 etcd + +import ( + "github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors" + "github.com/GoogleCloudPlatform/kubernetes/pkg/tools" +) + +// InterpretGetError converts a generic etcd error on a retrieval +// operation into the appropriate API error. +func InterpretGetError(err error, kind, name string) error { + switch { + case tools.IsEtcdNotFound(err): + return errors.NewNotFound(kind, name) + default: + return err + } +} + +// InterpretCreateError converts a generic etcd error on a create +// operation into the appropriate API error. +func InterpretCreateError(err error, kind, name string) error { + switch { + case tools.IsEtcdNodeExist(err): + return errors.NewAlreadyExists(kind, name) + default: + return err + } +} + +// InterpretUpdateError converts a generic etcd error on a create +// operation into the appropriate API error. +func InterpretUpdateError(err error, kind, name string) error { + switch { + case tools.IsEtcdTestFailed(err), tools.IsEtcdNodeExist(err): + return errors.NewConflict(kind, name, err) + default: + return err + } +} + +// InterpretDeleteError converts a generic etcd error on a create +// operation into the appropriate API error. +func InterpretDeleteError(err error, kind, name string) error { + switch { + case tools.IsEtcdNotFound(err): + return errors.NewNotFound(kind, name) + default: + return err + } +} diff --git a/pkg/registry/etcd/etcd.go b/pkg/registry/etcd/etcd.go index 0647f382475..33a279d7272 100644 --- a/pkg/registry/etcd/etcd.go +++ b/pkg/registry/etcd/etcd.go @@ -20,7 +20,7 @@ import ( "fmt" "github.com/GoogleCloudPlatform/kubernetes/pkg/api" - "github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors" + etcderr "github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors/etcd" "github.com/GoogleCloudPlatform/kubernetes/pkg/constraint" "github.com/GoogleCloudPlatform/kubernetes/pkg/labels" "github.com/GoogleCloudPlatform/kubernetes/pkg/runtime" @@ -96,7 +96,7 @@ func (r *Registry) WatchPods(resourceVersion uint64, filter func(*api.Pod) bool) func (r *Registry) GetPod(podID string) (*api.Pod, error) { var pod api.Pod if err := r.ExtractObj(makePodKey(podID), &pod, false); err != nil { - return nil, err + return nil, etcderr.InterpretGetError(err, "pod", podID) } // TODO: Currently nothing sets CurrentState.Host. We need a feedback loop that sets // the CurrentState.Host and Status fields. Here we pretend that reality perfectly @@ -117,12 +117,13 @@ func (r *Registry) CreatePod(pod *api.Pod) error { // DesiredState.Host == "" is a signal to the scheduler that this pod needs scheduling. pod.DesiredState.Status = api.PodRunning pod.DesiredState.Host = "" - return r.CreateObj(makePodKey(pod.ID), pod) + err := r.CreateObj(makePodKey(pod.ID), pod) + return etcderr.InterpretCreateError(err, "pod", pod.ID) } // ApplyBinding implements binding's registry func (r *Registry) ApplyBinding(binding *api.Binding) error { - return r.assignPod(binding.PodID, binding.Host) + return etcderr.InterpretCreateError(r.assignPod(binding.PodID, binding.Host), "binding", "") } // setPodHostTo sets the given pod's host to 'machine' iff it was previously 'oldMachine'. @@ -183,20 +184,14 @@ func (r *Registry) DeletePod(podID string) error { var pod api.Pod podKey := makePodKey(podID) err := r.ExtractObj(podKey, &pod, false) - if tools.IsEtcdNotFound(err) { - return errors.NewNotFound("pod", podID) - } if err != nil { - return err + return etcderr.InterpretDeleteError(err, "pod", podID) } // First delete the pod, so a scheduler doesn't notice it getting removed from the // machine and attempt to put it somewhere. err = r.Delete(podKey, true) - if tools.IsEtcdNotFound(err) { - return errors.NewNotFound("pod", podID) - } if err != nil { - return err + return etcderr.InterpretDeleteError(err, "pod", podID) } machine := pod.DesiredState.Host if machine == "" { @@ -248,11 +243,8 @@ func (r *Registry) GetController(controllerID string) (*api.ReplicationControlle var controller api.ReplicationController key := makeControllerKey(controllerID) err := r.ExtractObj(key, &controller, false) - if tools.IsEtcdNotFound(err) { - return nil, errors.NewNotFound("replicationController", controllerID) - } if err != nil { - return nil, err + return nil, etcderr.InterpretGetError(err, "replicationController", controllerID) } return &controller, nil } @@ -260,25 +252,20 @@ func (r *Registry) GetController(controllerID string) (*api.ReplicationControlle // CreateController creates a new ReplicationController. func (r *Registry) CreateController(controller *api.ReplicationController) error { err := r.CreateObj(makeControllerKey(controller.ID), controller) - if tools.IsEtcdNodeExist(err) { - return errors.NewAlreadyExists("replicationController", controller.ID) - } - return err + return etcderr.InterpretCreateError(err, "replicationController", controller.ID) } // UpdateController replaces an existing ReplicationController. func (r *Registry) UpdateController(controller *api.ReplicationController) error { - return r.SetObj(makeControllerKey(controller.ID), controller) + err := r.SetObj(makeControllerKey(controller.ID), controller) + return etcderr.InterpretUpdateError(err, "replicationController", controller.ID) } // DeleteController deletes a ReplicationController specified by its ID. func (r *Registry) DeleteController(controllerID string) error { key := makeControllerKey(controllerID) err := r.Delete(key, false) - if tools.IsEtcdNotFound(err) { - return errors.NewNotFound("replicationController", controllerID) - } - return err + return etcderr.InterpretDeleteError(err, "replicationController", controllerID) } func makeServiceKey(name string) string { @@ -295,10 +282,7 @@ func (r *Registry) ListServices() (*api.ServiceList, error) { // CreateService creates a new Service. func (r *Registry) CreateService(svc *api.Service) error { err := r.CreateObj(makeServiceKey(svc.ID), svc) - if tools.IsEtcdNodeExist(err) { - return errors.NewAlreadyExists("service", svc.ID) - } - return err + return etcderr.InterpretCreateError(err, "service", svc.ID) } // GetService obtains a Service specified by its name. @@ -306,11 +290,8 @@ func (r *Registry) GetService(name string) (*api.Service, error) { key := makeServiceKey(name) var svc api.Service err := r.ExtractObj(key, &svc, false) - if tools.IsEtcdNotFound(err) { - return nil, errors.NewNotFound("service", name) - } if err != nil { - return nil, err + return nil, etcderr.InterpretGetError(err, "service", name) } return &svc, nil } @@ -320,11 +301,8 @@ func (r *Registry) GetEndpoints(name string) (*api.Endpoints, error) { key := makeServiceEndpointsKey(name) var endpoints api.Endpoints err := r.ExtractObj(key, &endpoints, false) - if tools.IsEtcdNotFound(err) { - return nil, errors.NewNotFound("endpoints", name) - } if err != nil { - return nil, err + return nil, etcderr.InterpretGetError(err, "endpoints", name) } return &endpoints, nil } @@ -337,23 +315,23 @@ func makeServiceEndpointsKey(name string) string { func (r *Registry) DeleteService(name string) error { key := makeServiceKey(name) err := r.Delete(key, true) - if tools.IsEtcdNotFound(err) { - return errors.NewNotFound("service", name) - } if err != nil { - return err + return etcderr.InterpretDeleteError(err, "service", name) } + + // TODO: can leave dangling endpoints, and potentially return incorrect + // endpoints if a new service is created with the same name key = makeServiceEndpointsKey(name) - err = r.Delete(key, true) - if !tools.IsEtcdNotFound(err) { - return err + if err := r.Delete(key, true); err != nil && !tools.IsEtcdNotFound(err) { + return etcderr.InterpretDeleteError(err, "endpoints", name) } return nil } // UpdateService replaces an existing Service. func (r *Registry) UpdateService(svc *api.Service) error { - return r.SetObj(makeServiceKey(svc.ID), svc) + err := r.SetObj(makeServiceKey(svc.ID), svc) + return etcderr.InterpretUpdateError(err, "service", svc.ID) } // WatchServices begins watching for new, changed, or deleted service configurations. @@ -380,11 +358,12 @@ func (r *Registry) ListEndpoints() (*api.EndpointsList, error) { // UpdateEndpoints update Endpoints of a Service. func (r *Registry) UpdateEndpoints(e *api.Endpoints) error { // TODO: this is a really bad misuse of AtomicUpdate, need to compute a diff inside the loop. - return r.AtomicUpdate(makeServiceEndpointsKey(e.ID), &api.Endpoints{}, + err := r.AtomicUpdate(makeServiceEndpointsKey(e.ID), &api.Endpoints{}, func(input runtime.Object) (runtime.Object, error) { // TODO: racy - label query is returning different results for two simultaneous updaters return e, nil }) + return etcderr.InterpretUpdateError(err, "endpoints", e.ID) } // WatchEndpoints begins watching for new, changed, or deleted endpoint configurations. diff --git a/pkg/registry/etcd/etcd_test.go b/pkg/registry/etcd/etcd_test.go index 1f61ea085fc..d55c0582d38 100644 --- a/pkg/registry/etcd/etcd_test.go +++ b/pkg/registry/etcd/etcd_test.go @@ -63,8 +63,8 @@ func TestEtcdGetPodNotFound(t *testing.T) { } registry := NewTestEtcdRegistry(fakeClient) _, err := registry.GetPod("foo") - if err == nil { - t.Errorf("Unexpected non-error.") + if !errors.IsNotFound(err) { + t.Errorf("Unexpected error returned: %#v", err) } } @@ -144,8 +144,8 @@ func TestEtcdCreatePodAlreadyExisting(t *testing.T) { ID: "foo", }, }) - if err == nil { - t.Error("Unexpected non-error") + if !errors.IsAlreadyExists(err) { + t.Errorf("Unexpected error returned: %#v", err) } } @@ -162,7 +162,7 @@ func TestEtcdCreatePodWithContainersError(t *testing.T) { R: &etcd.Response{ Node: nil, }, - E: tools.EtcdErrorValueRequired, + E: tools.EtcdErrorNodeExist, // validate that ApplyBinding is translating Create errors } registry := NewTestEtcdRegistry(fakeClient) err := registry.CreatePod(&api.Pod{ @@ -176,8 +176,8 @@ func TestEtcdCreatePodWithContainersError(t *testing.T) { // Suddenly, a wild scheduler appears: err = registry.ApplyBinding(&api.Binding{PodID: "foo", Host: "machine"}) - if err == nil { - t.Fatalf("Unexpected non error.") + if !errors.IsAlreadyExists(err) { + t.Fatalf("Unexpected error returned: %#v", err) } existingPod, err := registry.GetPod("foo") @@ -185,7 +185,7 @@ func TestEtcdCreatePodWithContainersError(t *testing.T) { t.Fatalf("Unexpected error: %v", err) } if existingPod.DesiredState.Host == "machine" { - t.Fatal("Pod's host changed in response to an unappliable binding.") + t.Fatal("Pod's host changed in response to an non-apply-able binding.") } } @@ -568,8 +568,8 @@ func TestEtcdGetControllerNotFound(t *testing.T) { if ctrl != nil { t.Errorf("Unexpected non-nil controller: %#v", ctrl) } - if err == nil { - t.Error("Unexpected non-error.") + if !errors.IsNotFound(err) { + t.Errorf("Unexpected error returned: %#v", err) } } @@ -745,8 +745,8 @@ func TestEtcdGetServiceNotFound(t *testing.T) { } registry := NewTestEtcdRegistry(fakeClient) _, err := registry.GetService("foo") - if err == nil { - t.Errorf("Unexpected non-error.") + if !errors.IsNotFound(err) { + t.Errorf("Unexpected error returned: %#v", err) } }