Redis fault injection

[FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg]

Description:
This PR implements fault injection for Redis; specifically delay and error faults (which themselves can have delays added). I chose not to implement a separate filter after discussing with Henry; we concluded that the faults we felt were useful didn't need many levels- just a delay on top of the original fault, if any. In addition, as the Redis protocol doesn't support headers that makes it a bit different again from Envoy's http fault injection.

Faults record metrics on the original request- and the delay fault adds extra latency which is included in the command latency for that request. Also, faults can apply only to certain commands.
Future work: Add several other faults, including cache misses and connection failures.

Risk Level: Medium
Testing:

• Unit tests on fault manager and command splitter.
• Testing on local machine. For 3 faults, the faults/requests after a few dozen requests reflected the desired configuration.
  Docs Changes: yes- updated Redis configuration section with notes on metrics and fault injection proper in docs/root/configuration/listeners/network_filters/redis_proxy_filter.rst .
  Release Notes: yes, under 1.15 (pending)

@HenryYYang for first pass.

[repokitteh-read-only]

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #10784 was opened by FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg.

see: more, trace.

[FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg]

Waiting on #10794 so that I can push my branch with latest master/updated version history. Not sure why the converage build fails; looks unrelated to my changes and presumably will work on latest master.

[mattklein123]

Please use --no-verify to bypass the checks. I'm not sure why the pre-push checks are checking thirdparty. I think @yanavlasov was looking to this.

[zhjdenislyft]

sorry, the logic looks good but I don't understand C++.

[FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg]

sorry, the logic looks good but I don't understand C++.

The best thing for you to review is the logic in the fault manager where we get the command for a fault- specifically, the amortization strategy.

[HenryYYang]

Some initial comments

[HenryYYang]

why are we doing the (100 - amortized_fault) here? This doesn't seem right to me. If we have 3 faults: 30%, 30%, 40% each. and the random number is r, we'll be doing these checks:
r % 100 < 30
r % 70 < 30
r % 40 < 40
those are different from the correct checks, which are:
r % 100 < 30
r % 100 < 60
r % 100 < 100
or
r % 100 < 30
(r % 100) - 30 < 30
(r % 100) - 60 < 40

[FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg]

yeah, that was wrong. I've fixed and updated test.

[HenryYYang]

It seems like this is duplicating methods already in snapshot class

[FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg]

Good call, I was worried about null/empty inputs to the snapshot method, but if I pre-calculate the adjusted numerator on fault manager construction I don't need to worry about this.

[HenryYYang]

why do we need these new interface on the base class instead of just in the delay fault?

[FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg]

The request that is being delayed records its latency before calling the onResponse() callback. That means unless the request is somehow aware that it should not calculate it and whatever is on the other side of the callback will, we will generate the wrong latency data.

In a request the updateStats() call always precedes callbacks.onResponse():

void SplitRequestBase::updateStats(const bool success) {
  if (success) {
    command_stats_.success_.inc();
  } else {
    command_stats_.error_.inc();
  }
  if (!delay_command_latency_) {
    command_latency_->complete();
  }
}

Does that make sense?

[HenryYYang]

Thanks

[HenryYYang]

I don't think this code is needed, you should just use FractionalPercent which wraps RuntimeFractionalPercent

[FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg]

runtime_.snapshot().getInteger(...) takes the default value as int, like most of the other featureEnabled(...) methods. The featureEnabled(...) methods that take a FractionalPercent default value return a boolean, which doesn't tell us what our amortized probability is. Then, we'd need to do the calculation for the amortized percentage with a check on the denominator.

[HenryYYang]

nit: I think this is a redundant value copying, is this only used for initializing the commands_ field?

[FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg]

Correct, this only for initialization. We want to make all the commands lowercase, hence the method. Would you prefer the lowercasing be a linter check?

[HenryYYang]

create the random number only if we have a matching command. Also what do we do if the fault_injection_percentage adds up higher than 100?

[FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg]

Ok. I was under the impression we were going to treat that as user error. I'll add that to the documentation section.

[HenryYYang]

are we checking for this user error anywhere?

[HenryYYang]

would calling value() on an unset optional throw exception here?

[FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg]

Good call- I'll add a check.

[FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg]

I'll also add a test.

[HenryYYang]

what do we do with unset default_value_ and runtime_key_?

[FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg]

I'm changing it for default_value_ to not be optional and be zero by default.

[HenryYYang]

use using instead of typedef, also should the faultmap be immutable?

[FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg]

Fault map gets built at runtime, so not immutable. Using noted.

[HenryYYang]

I don't think it can be changed post constructor, so it should be immutable. Also the getFaultForCommand method should be marked const.

[FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg]

Ok done

[HenryYYang]

just noticed these fields are public, this feels like an antipattern

[FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg]

I'll add getters

[repokitteh-read-only]

CC @envoyproxy/api-watchers: FYI only for changes made to api/.

🐱

Caused by: #10784 was synchronize by FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg.

see: more, trace.

[FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg]

Master branch format check is broken due to some http codec issue, so I've removed the master merge.

[mattklein123]

@FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg please never force push. It makes reviews very difficult. Thank you!

[FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg]

Sorry! I was trying to get further back on master and broke the cardinal rule!

[mattklein123]

Thanks!