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

Updating apiserver to return 202 when resource is being deleted asynchronously via cascading deletion #41165

Merged
merged 4 commits into from
Feb 26, 2017
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
2 changes: 1 addition & 1 deletion federation/registry/cluster/registry.go
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,6 @@ func (s *storage) UpdateCluster(ctx genericapirequest.Context, cluster *federati
}

func (s *storage) DeleteCluster(ctx genericapirequest.Context, name string) error {
_, err := s.Delete(ctx, name, nil)
_, _, err := s.Delete(ctx, name, nil)
return err
}
2 changes: 1 addition & 1 deletion pkg/master/thirdparty/thirdparty.go
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ func (m *ThirdPartyResourceServer) removeAllThirdPartyResources(registry *thirdp
}
for ix := range list.Items {
item := &list.Items[ix]
if _, err := registry.Delete(ctx, item.Name, nil); err != nil {
if _, _, err := registry.Delete(ctx, item.Name, nil); err != nil {
return err
}
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/registry/certificates/certificates/registry.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,6 @@ func (s *storage) GetCSR(ctx genericapirequest.Context, name string, options *me
}

func (s *storage) DeleteCSR(ctx genericapirequest.Context, name string) error {
_, err := s.Delete(ctx, name, nil)
_, _, err := s.Delete(ctx, name, nil)
return err
}
3 changes: 1 addition & 2 deletions pkg/registry/core/configmap/registry.go
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,6 @@ func (s *storage) UpdateConfigMap(ctx genericapirequest.Context, cfg *api.Config
}

func (s *storage) DeleteConfigMap(ctx genericapirequest.Context, name string) error {
_, err := s.Delete(ctx, name, nil)

_, _, err := s.Delete(ctx, name, nil)
return err
}
2 changes: 1 addition & 1 deletion pkg/registry/core/endpoint/registry.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,6 @@ func (s *storage) UpdateEndpoints(ctx genericapirequest.Context, endpoints *api.
}

func (s *storage) DeleteEndpoints(ctx genericapirequest.Context, name string) error {
_, err := s.Delete(ctx, name, nil)
_, _, err := s.Delete(ctx, name, nil)
return err
}
2 changes: 1 addition & 1 deletion pkg/registry/core/namespace/registry.go
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,6 @@ func (s *storage) UpdateNamespace(ctx genericapirequest.Context, namespace *api.
}

func (s *storage) DeleteNamespace(ctx genericapirequest.Context, namespaceID string) error {
_, err := s.Delete(ctx, namespaceID, nil)
_, _, err := s.Delete(ctx, namespaceID, nil)
return err
}
14 changes: 7 additions & 7 deletions pkg/registry/core/namespace/storage/storage.go
Original file line number Diff line number Diff line change
Expand Up @@ -82,10 +82,10 @@ func NewREST(optsGetter generic.RESTOptionsGetter) (*REST, *StatusREST, *Finaliz
}

// Delete enforces life-cycle rules for namespace termination
func (r *REST) Delete(ctx genericapirequest.Context, name string, options *metav1.DeleteOptions) (runtime.Object, error) {
func (r *REST) Delete(ctx genericapirequest.Context, name string, options *metav1.DeleteOptions) (runtime.Object, bool, error) {
nsObj, err := r.Get(ctx, name, &metav1.GetOptions{})
if err != nil {
return nil, err
return nil, false, err
}

namespace := nsObj.(*api.Namespace)
Expand All @@ -105,15 +105,15 @@ func (r *REST) Delete(ctx genericapirequest.Context, name string, options *metav
name,
fmt.Errorf("Precondition failed: UID in precondition: %v, UID in object meta: %v", *options.Preconditions.UID, namespace.UID),
)
return nil, err
return nil, false, err
}

// upon first request to delete, we switch the phase to start namespace termination
// TODO: enhance graceful deletion's calls to DeleteStrategy to allow phase change and finalizer patterns
if namespace.DeletionTimestamp.IsZero() {
key, err := r.Store.KeyFunc(ctx, name)
if err != nil {
return nil, err
return nil, false, err
}

preconditions := storage.Preconditions{UID: options.Preconditions.UID}
Expand Down Expand Up @@ -159,16 +159,16 @@ func (r *REST) Delete(ctx genericapirequest.Context, name string, options *metav
if _, ok := err.(*apierrors.StatusError); !ok {
err = apierrors.NewInternalError(err)
}
return nil, err
return nil, false, err
}

return out, nil
return out, false, nil
}

// prior to final deletion, we must ensure that finalizers is empty
if len(namespace.Spec.Finalizers) != 0 {
err = apierrors.NewConflict(api.Resource("namespaces"), namespace.Name, fmt.Errorf("The system is ensuring all content is removed from this namespace. Upon completion, this namespace will automatically be purged by the system."))
return nil, err
return nil, false, err
}
return r.Store.Delete(ctx, name, options)
}
Expand Down
4 changes: 2 additions & 2 deletions pkg/registry/core/namespace/storage/storage_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ func TestDeleteNamespaceWithIncompleteFinalizers(t *testing.T) {
if err := storage.Storage.Create(ctx, key, namespace, nil, 0); err != nil {
t.Fatalf("unexpected error: %v", err)
}
if _, err := storage.Delete(ctx, "foo", nil); err == nil {
if _, _, err := storage.Delete(ctx, "foo", nil); err == nil {
t.Errorf("unexpected error: %v", err)
}
}
Expand All @@ -182,7 +182,7 @@ func TestDeleteNamespaceWithCompleteFinalizers(t *testing.T) {
if err := storage.Storage.Create(ctx, key, namespace, nil, 0); err != nil {
t.Fatalf("unexpected error: %v", err)
}
if _, err := storage.Delete(ctx, "foo", nil); err != nil {
if _, _, err := storage.Delete(ctx, "foo", nil); err != nil {
t.Errorf("unexpected error: %v", err)
}
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/registry/core/node/registry.go
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,6 @@ func (s *storage) GetNode(ctx genericapirequest.Context, name string, options *m
}

func (s *storage) DeleteNode(ctx genericapirequest.Context, name string) error {
_, err := s.Delete(ctx, name, nil)
_, _, err := s.Delete(ctx, name, nil)
return err
}
2 changes: 1 addition & 1 deletion pkg/registry/core/pod/storage/eviction.go
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ func (r *EvictionREST) Create(ctx genericapirequest.Context, obj runtime.Object)
// At this point there was either no PDB or we succeded in decrementing

// Try the delete
_, err = r.store.Delete(ctx, eviction.Name, eviction.DeleteOptions)
_, _, err = r.store.Delete(ctx, eviction.Name, eviction.DeleteOptions)
if err != nil {
return nil, err
}
Expand Down
4 changes: 2 additions & 2 deletions pkg/registry/core/pod/storage/storage_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ func TestIgnoreDeleteNotFound(t *testing.T) {
defer registry.Store.DestroyFunc()

// should fail if pod A is not created yet.
_, err := registry.Delete(testContext, pod.Name, nil)
_, _, err := registry.Delete(testContext, pod.Name, nil)
if !errors.IsNotFound(err) {
t.Errorf("Unexpected error: %v", err)
}
Expand All @@ -191,7 +191,7 @@ func TestIgnoreDeleteNotFound(t *testing.T) {
// registry shouldn't get any error since we ignore the NotFound error.
zero := int64(0)
opt := &metav1.DeleteOptions{GracePeriodSeconds: &zero}
obj, err := registry.Delete(testContext, pod.Name, opt)
obj, _, err := registry.Delete(testContext, pod.Name, opt)
if err != nil {
t.Fatalf("Unexpected error: %v", err)
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/registry/core/replicationcontroller/registry.go
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,6 @@ func (s *storage) UpdateController(ctx genericapirequest.Context, controller *ap
}

func (s *storage) DeleteController(ctx genericapirequest.Context, controllerID string) error {
_, err := s.Delete(ctx, controllerID, nil)
_, _, err := s.Delete(ctx, controllerID, nil)
return err
}
2 changes: 1 addition & 1 deletion pkg/registry/core/secret/registry.go
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,6 @@ func (s *storage) UpdateSecret(ctx genericapirequest.Context, secret *api.Secret
}

func (s *storage) DeleteSecret(ctx genericapirequest.Context, name string) error {
_, err := s.Delete(ctx, name, nil)
_, _, err := s.Delete(ctx, name, nil)
return err
}
2 changes: 1 addition & 1 deletion pkg/registry/core/service/registry.go
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ func (s *storage) GetService(ctx genericapirequest.Context, name string, options
}

func (s *storage) DeleteService(ctx genericapirequest.Context, name string) error {
_, err := s.Delete(ctx, name, nil)
_, _, err := s.Delete(ctx, name, nil)
return err
}

Expand Down
2 changes: 1 addition & 1 deletion pkg/registry/core/serviceaccount/registry.go
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,6 @@ func (s *storage) UpdateServiceAccount(ctx genericapirequest.Context, serviceAcc
}

func (s *storage) DeleteServiceAccount(ctx genericapirequest.Context, name string) error {
_, err := s.Delete(ctx, name, nil)
_, _, err := s.Delete(ctx, name, nil)
return err
}
2 changes: 1 addition & 1 deletion pkg/registry/extensions/deployment/registry.go
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,6 @@ func (s *storage) UpdateDeployment(ctx genericapirequest.Context, deployment *ex
}

func (s *storage) DeleteDeployment(ctx genericapirequest.Context, deploymentID string) error {
_, err := s.Delete(ctx, deploymentID, nil)
_, _, err := s.Delete(ctx, deploymentID, nil)
return err
}
2 changes: 1 addition & 1 deletion pkg/registry/extensions/replicaset/registry.go
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,6 @@ func (s *storage) UpdateReplicaSet(ctx genericapirequest.Context, replicaSet *ex
}

func (s *storage) DeleteReplicaSet(ctx genericapirequest.Context, replicaSetID string) error {
_, err := s.Delete(ctx, replicaSetID, nil)
_, _, err := s.Delete(ctx, replicaSetID, nil)
return err
}
2 changes: 1 addition & 1 deletion pkg/registry/extensions/thirdpartyresourcedata/registry.go
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,6 @@ func (s *storage) UpdateThirdPartyResourceData(ctx genericapirequest.Context, Th
}

func (s *storage) DeleteThirdPartyResourceData(ctx genericapirequest.Context, name string) error {
_, err := s.Delete(ctx, name, nil)
_, _, err := s.Delete(ctx, name, nil)
return err
}
2 changes: 1 addition & 1 deletion pkg/registry/rbac/clusterrole/registry.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ func (s *storage) GetClusterRole(ctx genericapirequest.Context, name string, opt
}

func (s *storage) DeleteClusterRole(ctx genericapirequest.Context, name string) error {
_, err := s.Delete(ctx, name, nil)
_, _, err := s.Delete(ctx, name, nil)
return err
}

Expand Down
2 changes: 1 addition & 1 deletion pkg/registry/rbac/clusterrolebinding/registry.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ func (s *storage) GetClusterRoleBinding(ctx genericapirequest.Context, name stri
}

func (s *storage) DeleteClusterRoleBinding(ctx genericapirequest.Context, name string) error {
_, err := s.Delete(ctx, name, nil)
_, _, err := s.Delete(ctx, name, nil)
return err
}

Expand Down
2 changes: 1 addition & 1 deletion pkg/registry/rbac/role/registry.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ func (s *storage) GetRole(ctx genericapirequest.Context, name string, options *m
}

func (s *storage) DeleteRole(ctx genericapirequest.Context, name string) error {
_, err := s.Delete(ctx, name, nil)
_, _, err := s.Delete(ctx, name, nil)
return err
}

Expand Down
2 changes: 1 addition & 1 deletion pkg/registry/rbac/rolebinding/registry.go
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ func (s *storage) GetRoleBinding(ctx genericapirequest.Context, name string, opt
}

func (s *storage) DeleteRoleBinding(ctx genericapirequest.Context, name string) error {
_, err := s.Delete(ctx, name, nil)
_, _, err := s.Delete(ctx, name, nil)
return err
}

Expand Down
2 changes: 1 addition & 1 deletion pkg/registry/registrytest/etcd.go
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,7 @@ func (t *Tester) emitObject(obj runtime.Object, action string) error {
if err != nil {
return err
}
_, err = t.storage.Delete(ctx, accessor.GetName(), nil)
_, _, err = t.storage.Delete(ctx, accessor.GetName(), nil)
default:
err = fmt.Errorf("unexpected action: %v", action)
}
Expand Down
9 changes: 5 additions & 4 deletions staging/src/k8s.io/apiserver/pkg/endpoints/apiserver_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -457,19 +457,19 @@ func (storage *SimpleRESTStorage) checkContext(ctx request.Context) {
storage.actualNamespace, storage.namespacePresent = request.NamespaceFrom(ctx)
}

func (storage *SimpleRESTStorage) Delete(ctx request.Context, id string, options *metav1.DeleteOptions) (runtime.Object, error) {
func (storage *SimpleRESTStorage) Delete(ctx request.Context, id string, options *metav1.DeleteOptions) (runtime.Object, bool, error) {
storage.checkContext(ctx)
storage.deleted = id
storage.deleteOptions = options
if err := storage.errors["delete"]; err != nil {
return nil, err
return nil, false, err
}
var obj runtime.Object = &metav1.Status{Status: metav1.StatusSuccess}
var err error
if storage.injectedFunction != nil {
obj, err = storage.injectedFunction(&genericapitesting.Simple{ObjectMeta: metav1.ObjectMeta{Name: id}})
}
return obj, err
return obj, true, err
}

func (storage *SimpleRESTStorage) New() runtime.Object {
Expand Down Expand Up @@ -605,7 +605,8 @@ type LegacyRESTStorage struct {
}

func (storage LegacyRESTStorage) Delete(ctx request.Context, id string) (runtime.Object, error) {
return storage.SimpleRESTStorage.Delete(ctx, id, nil)
obj, _, err := storage.SimpleRESTStorage.Delete(ctx, id, nil)
return obj, err
}

type MetadataRESTStorage struct {
Expand Down
19 changes: 16 additions & 3 deletions staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest.go
Original file line number Diff line number Diff line change
Expand Up @@ -857,21 +857,34 @@ func DeleteResource(r rest.GracefulDeleter, allowsOptions bool, scope RequestSco
}

trace.Step("About do delete object from database")
wasDeleted := true
result, err := finishRequest(timeout, func() (runtime.Object, error) {
return r.Delete(ctx, name, options)
obj, deleted, err := r.Delete(ctx, name, options)
wasDeleted = deleted
return obj, err
})
if err != nil {
scope.err(err, res.ResponseWriter, req.Request)
return
}
trace.Step("Object deleted from database")

status := http.StatusOK
// Return http.StatusAccepted if the resource was not deleted immediately and
// user requested cascading deletion by setting OrphanDependents=false.
// Note: We want to do this always if resource was not deleted immediately, but
// that will break existing clients.
// Other cases where resource is not instantly deleted are: namespace deletion
// and pod graceful deletion.
if !wasDeleted && options.OrphanDependents != nil && *options.OrphanDependents == false {
status = http.StatusAccepted
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a test case in apiserver for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@smarterclayton Any recommendation on what is the right file to put that test in?
I see test/integration/auth/auth_test.go being abused for status code testing. I can add a test there if there is no better place.

// if the rest.Deleter returns a nil object, fill out a status. Callers may return a valid
// object with the response.
if result == nil {
result = &metav1.Status{
Status: metav1.StatusSuccess,
Code: http.StatusOK,
Code: int32(status),
Details: &metav1.StatusDetails{
Name: name,
Kind: scope.Kind.Kind,
Expand All @@ -886,7 +899,7 @@ func DeleteResource(r rest.GracefulDeleter, allowsOptions bool, scope RequestSco
}
}
}
responsewriters.WriteObject(http.StatusOK, scope.Kind.GroupVersion(), scope.Serializer, result, w, req.Request)
responsewriters.WriteObject(status, scope.Kind.GroupVersion(), scope.Serializer, result, w, req.Request)
}
}

Expand Down