Webhook: handle error when calling wait.ExponentialBackoff

[tkashem]

What type of PR is this?

/kind bug

What this PR does / why we need it:

Inside WithExponentialBackoff function, handle error returned by wait.ExponentialBackoff

Does this PR introduce a user-facing change?:

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

[k8s-ci-robot]

@tkashem: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

[tkashem]

/assign @deads2k

[tkashem]

/retest

[tkashem]

/retest

[tkashem]

/retest

[tkashem]

/retest

[deads2k]

if the context fails, the webhookErr could be nil and we would still want to fail, right?

[tkashem]

Yes, if context fails, we should return ctx.Err()

[tkashem]

/retest

[deads2k]

switch{
// if we have a webhook error, it's the most important
case webhookErr != nil:
  return webhookErr

// if we have a deadline exceeded or cancelled, then that 's got priority over timeouts.
case ctx.Err() != nil:
  return ctx.Err()

// if the wait returned an error, but we didn't get a webhook error, it should be timeout and return that
case err != nil:
  return err

// if nothing had an error, then return nil
default:
  return nil
}

[tkashem]

@deads2k If the context gets canceled or times out then wait.ExponentialBackoff returns the context error to us via err. So either ctx error or wait.Timeout error is returned. I don't think we need to check for ctx.Err() != nil in the switch.

I added the switch statement as suggested.

Also, in the current implementation, we call the webhookFn first and then check for ctx.Error. I think checking if the context has timed out first before calling the webhookFn function is more correct.

err := wait.ExponentialBackoff(backoff, func() (bool, error) {		
		if ctx.Err() != nil {
			// we timed out or were cancelled, we should not retry
			return true, ctx.Err()
		}
                webhookErr = webhookFn()

I added two unit tests to verify:

• webhookErr is more important than ctx.Err()
• If the context is already canceled, there is no need to call the webhookFn function.

[tkashem]

/retest

[deads2k]

maybe a comment for future generations

// having a webhook error allows us to track the last actual webhook error for requests that are later cancelled or time out

[deads2k]

/lgtm
/approve

[deads2k]

/hold

let's get that nit on that comment. It may be not be super obvious.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, tkashem

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:

• staging/src/k8s.io/apimachinery/pkg/OWNERS [deads2k]
• staging/src/k8s.io/apiserver/OWNERS [deads2k]

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

[deads2k]

/lgtm
/hold cancel

[tkashem]

/priority important-soon

[tkashem]

/retest

[k8s-ci-robot]

@tkashem: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-kind-ipv6	f8e35de	link	/test pull-kubernetes-e2e-kind-ipv6

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

[tkashem]

/retest