From 1b8f24c9a851fe90abd9d3856d73eded19bc86d3 Mon Sep 17 00:00:00 2001
From: Clayton Coleman <ccoleman@redhat.com>
Date: Wed, 26 Jul 2017 21:49:29 -0400
Subject: [PATCH] Return a status cause for disruption budget that contains
 more details

Also uses the standard error constructor for TooManyRequests and
clarifies *why* a disruption is rejected.
---
 pkg/registry/core/pod/storage/eviction.go | 40 ++++++++++-------------
 1 file changed, 17 insertions(+), 23 deletions(-)

diff --git a/pkg/registry/core/pod/storage/eviction.go b/pkg/registry/core/pod/storage/eviction.go
index 2c2d522bb91..468a25330ef 100644
--- a/pkg/registry/core/pod/storage/eviction.go
+++ b/pkg/registry/core/pod/storage/eviction.go
@@ -101,24 +101,9 @@ func (r *EvictionREST) Create(ctx genericapirequest.Context, obj runtime.Object,
 
 			// If it was false already, or if it becomes false during the course of our retries,
 			// raise an error marked as a 429.
-			ok, err := r.checkAndDecrement(pod.Namespace, pod.Name, pdb)
-			if err != nil {
+			if err := r.checkAndDecrement(pod.Namespace, pod.Name, pdb); err != nil {
 				return err
 			}
-
-			if !ok {
-				rtStatus = &metav1.Status{
-					Status: metav1.StatusFailure,
-					// TODO(mml): Include some more details about why the eviction is disallowed.
-					// Ideally any such text is generated by the DisruptionController (offline).
-					Message: "Cannot evict pod as it would violate the pod's disruption budget.",
-					Code:    429,
-					// TODO(mml): Add a Retry-After header.  Once there are time-based
-					// budgets, we can sometimes compute a sensible suggested value.  But
-					// even without that, we can give a suggestion (10 minutes?) that
-					// prevents well-behaved clients from hammering us.
-				}
-			}
 		}
 		return nil
 	})
@@ -146,19 +131,28 @@ func (r *EvictionREST) Create(ctx genericapirequest.Context, obj runtime.Object,
 }
 
 // checkAndDecrement checks if the provided PodDisruptionBudget allows any disruption.
-func (r *EvictionREST) checkAndDecrement(namespace string, podName string, pdb policy.PodDisruptionBudget) (ok bool, err error) {
+func (r *EvictionREST) checkAndDecrement(namespace string, podName string, pdb policy.PodDisruptionBudget) error {
 	if pdb.Status.ObservedGeneration < pdb.Generation {
-		return false, nil
+		// TODO(mml): Add a Retry-After header.  Once there are time-based
+		// budgets, we can sometimes compute a sensible suggested value.  But
+		// even without that, we can give a suggestion (10 minutes?) that
+		// prevents well-behaved clients from hammering us.
+		err := errors.NewTooManyRequests("Cannot evict pod as it would violate the pod's disruption budget.", 0)
+		err.ErrStatus.Details.Causes = append(err.ErrStatus.Details.Causes, metav1.StatusCause{Type: "DisruptionBudget", Message: fmt.Sprintf("The disruption budget %s is still being processed by the server.", pdb.Name)})
+		return err
 	}
 	if pdb.Status.PodDisruptionsAllowed < 0 {
-		return false, errors.NewForbidden(policy.Resource("poddisruptionbudget"), pdb.Name, fmt.Errorf("pdb disruptions allowed is negative"))
+		return errors.NewForbidden(policy.Resource("poddisruptionbudget"), pdb.Name, fmt.Errorf("pdb disruptions allowed is negative"))
 	}
 	if len(pdb.Status.DisruptedPods) > MaxDisruptedPodSize {
-		return false, errors.NewForbidden(policy.Resource("poddisruptionbudget"), pdb.Name, fmt.Errorf("DisrputedPods map too big - too many evictions not confirmed by PDB controller"))
+		return errors.NewForbidden(policy.Resource("poddisruptionbudget"), pdb.Name, fmt.Errorf("DisruptedPods map too big - too many evictions not confirmed by PDB controller"))
 	}
 	if pdb.Status.PodDisruptionsAllowed == 0 {
-		return false, nil
+		err := errors.NewTooManyRequests("Cannot evict pod as it would violate the pod's disruption budget.", 0)
+		err.ErrStatus.Details.Causes = append(err.ErrStatus.Details.Causes, metav1.StatusCause{Type: "DisruptionBudget", Message: fmt.Sprintf("The disruption budget %s needs %d healthy pods and has %d currently", pdb.Name, pdb.Status.DesiredHealthy, pdb.Status.CurrentHealthy)})
+		return err
 	}
+
 	pdb.Status.PodDisruptionsAllowed--
 	if pdb.Status.DisruptedPods == nil {
 		pdb.Status.DisruptedPods = make(map[string]metav1.Time)
@@ -169,10 +163,10 @@ func (r *EvictionREST) checkAndDecrement(namespace string, podName string, pdb p
 	// be deleted at all and remove it from DisruptedPod map.
 	pdb.Status.DisruptedPods[podName] = metav1.Time{Time: time.Now()}
 	if _, err := r.podDisruptionBudgetClient.PodDisruptionBudgets(namespace).UpdateStatus(&pdb); err != nil {
-		return false, err
+		return err
 	}
 
-	return true, nil
+	return nil
 }
 
 // getPodDisruptionBudgets returns any PDBs that match the pod or err if there's an error.