-
Notifications
You must be signed in to change notification settings - Fork 5.5k
Redis fault injection #10784
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Redis fault injection #10784
Changes from 11 commits
2894271
827f332
4d83a32
99aace9
780bd44
b67b102
9679b33
b1a08f2
6f472a1
46325db
6f0c543
abcaa1b
b1c2a08
e7afabf
ef0545a
03f8238
23ad135
62ae483
412b74c
88e2ae3
7a86b58
38bbbc7
4c80aa2
23db915
c4339c1
f8dc205
86f2289
253d6a3
375ce5d
20e52a9
7f09ce6
6f0f82b
e62cd46
aa561ab
a593eb5
b839bda
eae5c0f
76d07f9
c9483a5
0ea8739
da490cf
0aa369c
b678124
37a2901
eac80ba
a952625
73aadf4
68fb506
c310d9b
d429faa
abe5a0a
0b62a99
b7f8486
638d2d8
0667411
c963fec
b26d949
1f5bd06
03cef03
8eac747
bf3d97f
5877bb5
d0c19a2
c6a720e
8190e8c
a03c392
7603989
9d03016
b5ccc44
2db0f8a
83b278f
474af56
37db613
8805ae7
b63cbf9
5f7544d
7b4ca69
6bc4aa5
a58ef91
b4e6357
873c093
8befff3
3c14dbe
b67b1c7
0708bbd
829d8a0
0404ef0
3c33eb1
f61db6f
c29ae1a
20ad1ca
48e5c67
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| #pragma once | ||
|
|
||
| #include <string> | ||
|
|
||
| #include "envoy/api/api.h" | ||
|
|
||
| namespace Envoy { | ||
| namespace Extensions { | ||
| namespace NetworkFilters { | ||
| namespace Common { | ||
| namespace Redis { | ||
|
|
||
| /** | ||
| * Fault Type. | ||
| */ | ||
| enum class FaultType { Delay, Error }; | ||
|
|
||
| class FaultManager { | ||
| public: | ||
| virtual ~FaultManager() = default; | ||
|
|
||
| /** | ||
| * Get fault type and delay given a Redis command. | ||
| * @param command supplies the Redis command string. | ||
| */ | ||
| virtual absl::optional<std::pair<FaultType, std::chrono::milliseconds>> | ||
| getFaultForCommand(std::string command) PURE; | ||
| }; | ||
|
|
||
| using FaultManagerPtr = std::shared_ptr<FaultManager>; | ||
|
FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg marked this conversation as resolved.
Outdated
|
||
|
|
||
| } // namespace Redis | ||
| } // namespace Common | ||
| } // namespace NetworkFilters | ||
| } // namespace Extensions | ||
| } // namespace Envoy | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,141 @@ | ||
| #include "extensions/filters/network/common/redis/fault_impl.h" | ||
|
|
||
| namespace Envoy { | ||
| namespace Extensions { | ||
| namespace NetworkFilters { | ||
| namespace Common { | ||
| namespace Redis { | ||
|
|
||
| Fault::Fault(envoy::extensions::filters::network::redis_proxy::v3::RedisProxy_RedisFault base_fault) | ||
| : commands_(getCommands(base_fault)) { | ||
| // // Get delay | ||
|
FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg marked this conversation as resolved.
Outdated
|
||
| delay_ms_ = std::chrono::milliseconds(PROTOBUF_GET_MS_OR_DEFAULT(base_fault, delay, 0)); | ||
|
|
||
| // Get fault type | ||
| switch (base_fault.fault_type()) { | ||
| case envoy::extensions::filters::network::redis_proxy::v3::RedisProxy::RedisFault::DELAY: | ||
| fault_type_ = FaultType::Delay; | ||
| break; | ||
| case envoy::extensions::filters::network::redis_proxy::v3::RedisProxy::RedisFault::ERROR: | ||
| fault_type_ = FaultType::Error; | ||
| break; | ||
| default: | ||
| NOT_REACHED_GCOVR_EXCL_LINE; | ||
| break; | ||
| } | ||
|
|
||
| // Get the default value/runtime key | ||
| if (base_fault.has_fault_enabled()) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this means if fault_enabled is not set, it's disabled, right? Please update documentation for this.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Noted from earlier discussions and have an open todo. Focusing on figuring out why tests fail in CI and not local first. |
||
| has_fault_enabled_ = true; | ||
| if (base_fault.fault_enabled().has_default_value()) { | ||
| if (base_fault.fault_enabled().default_value().denominator() == | ||
| envoy::type::v3::FractionalPercent::HUNDRED) { | ||
| default_value_ = base_fault.fault_enabled().default_value().numerator(); | ||
| } else { | ||
| auto denominator = ProtobufPercentHelper::fractionalPercentDenominatorToInt( | ||
| base_fault.fault_enabled().default_value().denominator()); | ||
| default_value_ = | ||
| (base_fault.fault_enabled().default_value().numerator() * 100) / denominator; | ||
| } | ||
| } | ||
| runtime_key_ = base_fault.fault_enabled().runtime_key(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this code is needed, you should just use FractionalPercent which wraps RuntimeFractionalPercent
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| } | ||
| }; | ||
|
|
||
| std::vector<std::string> Fault::getCommands( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: I think this is a redundant value copying, is this only used for initializing the commands_ field?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
| envoy::extensions::filters::network::redis_proxy::v3::RedisProxy_RedisFault base_fault) { | ||
| std::vector<std::string> commands; | ||
| for (auto command : base_fault.commands()) { | ||
| commands.emplace_back(absl::AsciiStrToLower(command)); | ||
| } | ||
| return commands; | ||
| } | ||
|
|
||
| FaultManagerImpl::FaultManagerImpl( | ||
| Runtime::RandomGenerator& random, Runtime::Loader& runtime, | ||
| const Protobuf::RepeatedPtrField< | ||
| ::envoy::extensions::filters::network::redis_proxy::v3::RedisProxy_RedisFault> | ||
| faults) | ||
| : random_(random), runtime_(runtime) { | ||
|
|
||
| // Next, create the fault map that maps commands to pointers to Fault objects. | ||
| // Group faults by command | ||
| for (auto base_fault : faults) { | ||
| auto fault_ptr = std::make_shared<Fault>(base_fault); | ||
| if (fault_ptr->commands_.size() > 0) { | ||
| for (std::string command : fault_ptr->commands_) { | ||
| fault_map_[command].push_back(fault_ptr); | ||
|
FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg marked this conversation as resolved.
Outdated
|
||
| } | ||
| } else { | ||
| // Generic "ALL" entry in map for faults that map to all keys; also add to each command | ||
| fault_map_[ALL_KEY].push_back(fault_ptr); | ||
| } | ||
| } | ||
|
|
||
| // Add the ALL keys faults to each command too so that we can just query faults by command. | ||
| // Get all ALL_KEY faults. | ||
| FaultMap::iterator it_outer = fault_map_.find(ALL_KEY); | ||
| if (it_outer != fault_map_.end()) { | ||
| // For each ALL_KEY fault... | ||
|
FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg marked this conversation as resolved.
Outdated
|
||
| for (auto fault_ptr : it_outer->second) { | ||
| // Loop through all unique commands other than ALL_KEY and add the fault. | ||
| FaultMap::iterator it_inner; | ||
| for (it_inner = fault_map_.begin(); it_inner != fault_map_.end(); it_inner++) { | ||
| std::string command = it_inner->first; | ||
| if (command != ALL_KEY) { | ||
| fault_map_[command].push_back(fault_ptr); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Fault checking algorithm: | ||
| // | ||
| // For example, if we have an ERROR fault at 5% for all commands, and a DELAY fault at 10% for GET, | ||
| // if we receive a GET, we want 5% of GETs to get DELAY, and 5% to get ERROR. Thus, we need to | ||
|
FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg marked this conversation as resolved.
Outdated
|
||
| // amortize the percentages. | ||
| // | ||
| // 0. Get random number. | ||
| // 1. Get faults for given command.s | ||
|
FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg marked this conversation as resolved.
Outdated
|
||
| // 2. For each fault, calculate the amortized fault injection percentage. | ||
| absl::optional<std::pair<FaultType, std::chrono::milliseconds>> | ||
| FaultManagerImpl::getFaultForCommandInternal(std::string command) { | ||
| auto random_number = random_.random() % 100; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok. I was under the impression we were going to treat that as user error. I'll add that to the documentation section.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. are we checking for this user error anywhere? |
||
| int amortized_fault = 0; | ||
|
|
||
| FaultMap::iterator it_outer = fault_map_.find(command); | ||
| if (it_outer != fault_map_.end()) { | ||
| for (auto fault_ptr : it_outer->second) { | ||
| auto fault_injection_percentage = runtime_.snapshot().getInteger( | ||
| fault_ptr->runtime_key_.value(), fault_ptr->default_value_.value()); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. would calling value() on an unset optional throw exception here?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good call- I'll add a check.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll also add a test. |
||
| if (random_number < (fault_injection_percentage + amortized_fault)) { | ||
| return std::make_pair(fault_ptr->fault_type_, fault_ptr->delay_ms_); | ||
| } else { | ||
| amortized_fault += fault_injection_percentage; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return absl::nullopt; | ||
| } | ||
|
|
||
| absl::optional<std::pair<FaultType, std::chrono::milliseconds>> | ||
| FaultManagerImpl::getFaultForCommand(std::string command) { | ||
| // Check if faults exist for given command; else use ALL_KEY and search for general faults. | ||
| if (!fault_map_.empty()) { | ||
| if (fault_map_.count(command) > 0) { | ||
| return getFaultForCommandInternal(command); | ||
| } else { | ||
| return getFaultForCommandInternal(ALL_KEY); | ||
| } | ||
| } | ||
|
|
||
| return absl::nullopt; | ||
| } | ||
|
|
||
| } // namespace Redis | ||
| } // namespace Common | ||
| } // namespace NetworkFilters | ||
| } // namespace Extensions | ||
| } // namespace Envoy | ||
Uh oh!
There was an error while loading. Please reload this page.