introduce util.Retry2 with cleaner func signature

pkg/kapp/resources/resources.go

@@ -89,17 +89,13 @@ func (c *Resources) All(resTypes []ResourceType, opts ResourcesAllOpts) ([]Resou
 
 			client := c.dynamicClient.Resource(resType.GroupVersionResource)
 
-			err = util.Retry(time.Second, 5*time.Second, func() (bool, error) {
-				var err error
+			err = util.Retry2(time.Second, 5*time.Second, c.isServerRescaleErr, func() error {
 				if resType.Namespaced() {
 					list, err = client.Namespace("").List(*opts.ListOpts)
 				} else {
 					list, err = client.List(*opts.ListOpts)
 				}
-				if err != nil {
-					return doneServerRescaleErr(err), err
-				}
-				return true, nil
+				return err
 			})
 
 			if err != nil {
@@ -222,15 +218,12 @@ func (c *Resources) Create(resource Resource) (Resource, error) {
 
 	var createdUn *unstructured.Unstructured
 
-	err = util.Retry(time.Second, 5*time.Second, func() (bool, error) {
+	err = util.Retry2(time.Second, 5*time.Second, c.isGeneralRetryableErr, func() error {
 		createdUn, err = resClient.Create(resource.unstructuredPtr())
-		if err != nil {
-			return doneResourceChangeBlockedOrServerRescaleErr(err), c.resourceErr(err, "Creating", resource)
-		}
-		return true, nil
+		return err
 	})
 	if err != nil {
-		return nil, err
+		return nil, c.resourceErr(err, "Creating", resource)
 	}
 
 	return NewResourceUnstructured(*createdUn, resType), nil
@@ -252,15 +245,12 @@ func (c *Resources) Update(resource Resource) (Resource, error) {
 
 	var updatedUn *unstructured.Unstructured
 
-	err = util.Retry(time.Second, 5*time.Second, func() (bool, error) {
+	err = util.Retry2(time.Second, 5*time.Second, c.isGeneralRetryableErr, func() error {
 		updatedUn, err = resClient.Update(resource.unstructuredPtr())
-		if err != nil {
-			return doneResourceChangeBlockedOrServerRescaleErr(err), c.resourceErr(err, "Updating", resource)
-		}
-		return true, nil
+		return err
 	})
 	if err != nil {
-		return nil, err
+		return nil, c.resourceErr(err, "Updating", resource)
 	}
 
 	return NewResourceUnstructured(*updatedUn, resType), nil
@@ -279,15 +269,12 @@ func (c *Resources) Patch(resource Resource, patchType types.PatchType, data []b
 
 	var patchedUn *unstructured.Unstructured
 
-	err = util.Retry(time.Second, 5*time.Second, func() (bool, error) {
+	err = util.Retry2(time.Second, 5*time.Second, c.isGeneralRetryableErr, func() error {
 		patchedUn, err = resClient.Patch(resource.Name(), patchType, data)
-		if err != nil {
-			return doneResourceChangeBlockedOrServerRescaleErr(err), c.resourceErr(err, "Patching", resource)
-		}
-		return true, nil
+		return err
 	})
 	if err != nil {
-		return nil, err
+		return nil, c.resourceErr(err, "Patching", resource)
 	}
 
 	return NewResourceUnstructured(*patchedUn, resType), nil
@@ -349,17 +336,16 @@ func (c *Resources) Get(resource Resource) (Resource, error) {
 	if err != nil {
 		return nil, err
 	}
+
 	var item *unstructured.Unstructured
-	err = util.Retry(time.Second, 5*time.Second, func() (bool, error) {
+
+	err = util.Retry2(time.Second, 5*time.Second, c.isServerRescaleErr, func() error {
 		var err error
 		item, err = resClient.Get(resource.Name(), metav1.GetOptions{})
-		if err != nil {
-			return doneServerRescaleErr(err), c.resourceErr(err, "Getting", resource)
-		}
-		return true, nil
+		return err
 	})
 	if err != nil {
-		return nil, err
+		return nil, c.resourceErr(err, "Getting", resource)
 	}
 
 	return NewResourceUnstructured(*item, resType), nil
@@ -394,7 +380,7 @@ func (c *Resources) Exists(resource Resource) (bool, error) {
 				found = false
 				return true, nil
 			}
-			if isServerRescaleErr(err) {
+			if c.isServerRescaleErr(err) {
 				return false, nil
 			}
 			// No point in waiting if we are not allowed to get it
@@ -431,7 +417,11 @@ func (c *Resources) isPodMetrics(resource Resource, err error) bool {
 	return false
 }
 
-func isServerRescaleErr(err error) bool {
+func (c *Resources) isGeneralRetryableErr(err error) bool {
+	return IsResourceChangeBlockedErr(err) || c.isServerRescaleErr(err)
+}
+
+func (*Resources) isServerRescaleErr(err error) bool {
 	switch err := err.(type) {
 	case *http2.GoAwayError:
 		return true
@@ -443,14 +433,6 @@ func isServerRescaleErr(err error) bool {
 	return false
 }
 
-func doneServerRescaleErr(err error) bool {
-	return !isServerRescaleErr(err)
-}
-
-func doneResourceChangeBlockedOrServerRescaleErr(err error) bool {
-	return !(IsResourceChangeBlockedErr(err) || isServerRescaleErr(err))
-}
-
 func (c *Resources) resourceErr(err error, action string, resource Resource) error {
 	if typedErr, ok := err.(errors.APIStatus); ok {
 		return resourceStatusErr{resourcePlainErr{err, action, resource}, typedErr.Status()}

pkg/kapp/util/retry.go

@@ -29,3 +29,25 @@ func Retry(interval, timeout time.Duration, condFunc wait.ConditionFunc) error {
 
 	return nil
 }
+
+// Retry is different from wait.Poll because
+// it does not stop retrying when error is encountered
+func Retry2(interval, timeout time.Duration, shouldRetryFunc func(error) bool, performFunc func() error) error {
+	var lastErr error
+
+	wait.PollImmediate(interval, timeout, func() (bool, error) {
+		err := performFunc()
+		lastErr = err
+		done := err == nil || shouldRetryFunc(err) == false
+		return done, nil
+	})
+
+	if lastErr != nil {
+		// TODO should not wrap error as it may lose necessary type info
+		// eg resources.Update needs to return status info
+		// return fmt.Errorf("Retried %d times: %s", times, lastErr)
+		return lastErr
+	}
+
+	return nil
+}