Adding retries for gRPC calls

[akirillov]

What this PR does / why we need it:
This PR adds retries for gRPC calls in suggestionclient.go. One of the specific problems resolved by retries is the timing issue in Katib controller pod when running on Istio 1.5+: when Experiment deployment is ready, Envoy sidecar health check doesn't treat it as a healthy upstream yet and ValidateAlgorithmSettings call from the controller fails with no healthy upstream.

In general, retries can improve resilience and help to avoid transient failures instead of marking experiment as failed after the first and the only attempt.

[kubeflow-bot]

This change is 

[k8s-ci-robot]

Hi @akirillov. Thanks for your PR.

I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[gaocegege]

/ok-to-test

[gaocegege]

/lgtm

Thanks for your contribution! 🎉 👍

[andreyvelich]

@akirillov Thank you for doing this!
Maybe we could make this change directly in v1beta1, since v1alpha3 release was already cut?
What do you think @johnugeorge @gaocegege ?

[andreyvelich]

Should we have DefaultGRPCRetryAttempts in comment here?

[akirillov]

Good catch, thanks. Fixed that.

[akirillov]

thanks for the review, @gaocegege, @andreyvelich! Speaking of adding the changes to both alpha and beta, I've noticed that even though the image tags in this PR kubeflow/manifests#1239 were updated to 0.9.0 release, the images are still using v1alpha3 version e.g.

images:
  - name: gcr.io/kubeflow-images-public/katib/v1alpha3/katib-controller
    newTag: 917164a

Is it intentional?

[andreyvelich]

thanks for the review, @gaocegege, @andreyvelich! Speaking of adding the changes to both alpha and beta, I've noticed that even though the image tags in this PR kubeflow/manifests#1239 were updated to 0.9.0 release, the images are still using v1alpha3 version e.g.

images:
  - name: gcr.io/kubeflow-images-public/katib/v1alpha3/katib-controller
    newTag: 917164a

Is it intentional?

Yes, the current Katib release for Kubeflow 1.1 is v1alpha3 with 917164a commit. This is the cut release: https://github.com/kubeflow/katib/releases/tag/v0.9.0.
Currently, we make all new changes only for v1beta1 version and we will try to release v1beta1 in the next Kubeflow release.
We update files for v1alpha3 version only with the urgent bug fixes.

[akirillov]

Thanks for the clarification, @andreyvelich. So what's the decision, shall I revert the changes for v1alpha3 and keep them only in v1beta1? Until v1beta1 is released, the fix can be helpful for the users running Katib v1alpha3 on Istio 1.5+ because, without it, all created Experiments are marked as failed after ValidateAlgorithmSetting gRPC call fails with 503 and no retry.

[andreyvelich]

@akirillov What do you think about adding this change to v1alpha3 and v1beta1 version.
I am not sure if we can include this change to the Katib v0.9.0, which deployed with Kubeflow 1.1, because it is already cut and manifest repository is updated.

But if users come with this problem, they just need to update Katib controller image to the latest v1alpha3 version and should be able to use Istio 1.5 +. They don't need to deploy v1beta1 version.

Let's see what others say.
What do you think @gaocegege @johnugeorge ?

[akirillov]

thanks, @andreyvelich, this PR adds it to both versions and I think it is beneficial to have it that way to avoid waiting for v1beta1 release. This is the use case my team is dealing with and it would be great to have v1alpha3 image with the fix published to avoid living on the custom build from a fork until v1beta1 is out.

[andreyvelich]

@akirillov Got it.

What do you think @gaocegege @johnugeorge ?
Can we just update image tags for the master branch in manifest repository?
I believe, we have kfdef config, which is related on master branch.

[gaocegege]

I have no objection to it. If users (akirillov 's team) need it we can do it.

/cc @johnugeorge

[andreyvelich]

Sure, I will update master manifests when we merge this PR.
/lgtm
/assign @johnugeorge

[johnugeorge]

SGTM
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: johnugeorge

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [johnugeorge]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment