Add retries to policy checks on failed transport error

[duderino]

What this PR does / why we need it:

On transport error only, retry a policy/quota check with exponential backoff and jitter

Which issue this PR fixes

istio/istio#9992

Special notes for your reviewer:

This PR takes advantage of the newly introduced CheckContext with its memory scoped by a smart pointer. This lets us more easily implement retry - each retry lambda just increments the smart pointer.

Note that I also moved the mixer client cancellation lambda into the CheckContext to make sure it too stays valid as long as the CheckContext is referenced.

A new retry lambda was also introduced and handled similarly to the cancellation lambda.

The semantic difference between the two: the cancellation lambda is invoked if the downstream client goes away. the retry lambda is invoked if a transport error occurs and the mixer filter wants to wait a bit before retrying against another mixer upstream.

1. I recommend reviewing the changes with ignore whitespace because clang-format varies the indentation on the lambda inside mixer_client_impl.cc quite a bit. See https://github.com/istio/proxy/pull/2113/files?w=1

2. After this is merged I will update Adapt mixer client tests to new mixer filter counters istio#11591 to import the new istio/proxy. That pull request makes the istio/mixerclient unit tests pass against the new mixer filter counters.

[mandarjog]

Is cancel check fn replaced by reset cancel? A comment would help.

[duderino]

See explanation in #2113 (comment)

[lambdai]

Talked offline. Stop reviewing until the rebase.

[lambdai]

nit: We may adopt the macro PURE in envoy/common/pure.h

[lambdai]

nit :

virtual ~CheckResponseInfo() = default;

[duderino]

Remaining work:

1. add tests for new policy check retry and cancellation counters
2. add test for exponential backoff
3. address code reviewer comments
4. rebase

[duderino]

@JimmyCYJ and @silentdai could you review this?

[duderino]

I'll fix the tsan and asan errors.

[lambdai]

Intentionally skip the multithread issue. You guys may have deep understanding than me.

[lambdai]

Question: What is the value of options.retries if the branch is not hit?

[duderino]

As long as the protobuf defines it as a uint32, then the value returned will always be >= 0. So currently this if statement is useless. It could be seen as an extreme form of defensive programming in case the return type ever changed to a signed int... or I could just remove it and unconditionally clobber options.retries. Thoughts?

[lambdai]

The temporary context of context->policyStatus().ToString().c_str() could be invalid. I am not a language lawyer. I need to verify.

[duderino]

I'm impressed you're reading this so thoroughly. Thank you for the diligent code review :)

  // Return a combination of the error code name and message.
  string ToString() const;

So it returns a copy that isn't stored in a variable... then I call c_str() on it. Will it be destroyed before or after I call c_str()?

Interestingly, because the log threshold is warn in these tests, asan won't actually see this.

@jplevyak do you know what will happen here?

[lambdai]

nit:

Suggested change

	void operator=(const CheckContext&) = delete;
	CheckContext& operator=(const CheckContext&) = delete;

[lambdai]

I am confused by this cancel stuff.

• When this lambda is invoked, the resetCancel invalid future cancel attempt
• this lambda doesn't always succeed. A retry may occurs.

In case there is a retr, does the retry retore the cancel? If so, where is it? If not, is it a big issue?

[duderino]

I'll try to explain....

Transactions are cancelled when the downstream client goes away and when the overall transaction times out.

In the original code there was a cancel lambda that is invoked when the transaction is cancelled. This cancel lambda tells the grpc library to cancel any outstanding grpc request sent to the mixer server. If a request is not outstanding, then the cancel lambda will be null and won't be called.

In the new code, it's also possible for requests to be in a retry wait when the transaction is cancelled. In this case we don't tell the grpc library to cancel the request because no request has been passed to it. Instead we tell envoy to cancel the retry timer.

In check_context.h you'll find this:

  void cancel() {
    if (cancel_func_) {
      MIXER_DEBUG("Cancelling check call");
      cancel_func_();
      cancel_func_ = nullptr;
    }

    if (retry_timer_) {
      MIXER_DEBUG("Cancelling retry");
      retry_timer_->Stop();
      retry_timer_ = nullptr;
    }
  }

[duderino]

And yes, once the retry lambda is invoked, it will restore the cancel lambda assuming it can successfully pass a request to the grpc library

[lambdai]

Is the completeCheck always invoked after callCheck(so that handler_ is not nullptr)?

[duderino]

I think callCheck is always invoked before completeCheck. I'm pretty unfamiliar with this code, but other code in the same function calls handler_ without checking for null.

[lambdai]

@duderino
WTR asan,

!std::equal(prefix.begin(), prefix.begin() + prefix.size(),
                      counter->name().begin()))

Is the asan error happens if the prefix is longer than counter->name() ?
If so, absl::StartsWith(counter->name(), prefix) could be a good replacement.

[duderino]

I followed @silentdai's suggestion and used absl::StartsWith() and that took care of the asan warning.

However, I had no luck getting the mixer_fault_tests passing under tsan and asan. In both cases they ran for 5 minutes and are cancelled by bazel or gtest perhaps 30% through. So I excluded the mixer fault test and only the mixer fault test from the sanitizers.

[duderino]

I am working on the macos failure. After creating a macos dev env to debug this, I'm pretty sure the failures are in the test case itself, not the code under test. The mixer filter periodically syncs stats from the mixer client impls and the envoy stats store. Something about the timing of that periodic sync is causing the stats to appear to be 0 to the test case when diagnostic log statements say they actually have the expected values. I'll fix this, but I don't want to delay rc0 because of it.

[JimmyCYJ]

Could you add a short comment for RetryDelay?

[JimmyCYJ]

/lgtm

/approve

[istio-testing]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: duderino, JimmyCYJ

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [JimmyCYJ]

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

[JimmyCYJ]

Could you please add a short comment describing the test case here and below?

[duderino]

Will fix the failing macos build and add commentary to the mixer_fault_test.cc in a subsequent PR. Thanks @JimmyCYJ