Skip to content

Fault injection v2#12551

Merged
mattklein123 merged 8 commits intoenvoyproxy:masterfrom
HenryYYang:fault-injection-v2
Aug 12, 2020
Merged

Fault injection v2#12551
mattklein123 merged 8 commits intoenvoyproxy:masterfrom
HenryYYang:fault-injection-v2

Conversation

@FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg
Copy link
Copy Markdown
Contributor

Commit Message:
Un-Reverts 048583b, with fix for high cpu consumption.

Additional 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. This PR fixes the issue previously seen with redis fault injection where excessive cpu was used on connection creation.

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.
  • Specific testing for high cpu usage on local machine. Used rpc-perf with various combinations of connections and clients to simulate higher load than normal testing with single requests.

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.16 (pending)

This reverts commit 77e9ed7.

Signed-off-by: FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg <nflacco@lyft.com>
Signed-off-by: FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg <nflacco@lyft.com>
@repokitteh-read-only
Copy link
Copy Markdown

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

🐱

Caused by: #12551 was opened by FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg.

see: more, trace.

@mattklein123 mattklein123 self-assigned this Aug 7, 2020
Signed-off-by: FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg <nflacco@lyft.com>
Signed-off-by: FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg <nflacco@lyft.com>
Signed-off-by: FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg <nflacco@lyft.com>
Signed-off-by: FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg <nflacco@lyft.com>
Signed-off-by: FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg <nflacco@lyft.com>
Copy link
Copy Markdown
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working through this! The fix LGTM.

@mattklein123 mattklein123 merged commit e319b7c into envoyproxy:master Aug 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants