-
Notifications
You must be signed in to change notification settings - Fork 9
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
Refactor fault injection logic #307
Conversation
Signed-off-by: Pablo Chacin <pablochacin@gmail.com>
6529bc8
to
8511d60
Compare
pkg/disruptors/injector_test.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we move the Test_NewPodDisruptor
and Test_PodSelectorString
tests to a new pod_test.go
file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to keep testing concerns separated instead of one huge test file, but I'm not opposed to merging all tests in one single file, either. In any case, this is probably unrelated to the purpose of this PR, so I prefer to leave it to another PR.
pkg/disruptors/injector.go
Outdated
type ServiceHTTPFaultInjector struct { | ||
service corev1.Service | ||
fault HTTPFault | ||
duration time.Duration | ||
options HTTPDisruptionOptions | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add tests for ServiceHTTPFaultInjector
and/or ServiceGrpcFaultInjector
? If we have some already, I'm not being able to find them 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to do this in a follow-up PR closing #261
Signed-off-by: Pablo Chacin <pablochacin@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Description
The purpose of the PR is to introduce abstractions that help in the separation of concerns around fault injection with the purpose of facilitating testing and promoting reusability.
Presently, the fault injection is done in conjunction between the disruptors (Pod, Service) and the AgentController.
The AgentController encapsulates the logic for interacting with the agents in a set of targets. The disruptors encapsulate the logic for defining the list of targets and for creating the agent command for injecting a particular fault in a target.
However, the way these aspects are presently integrated difficult testing them separately. In particular, testing the generation of commands for fault injection requires setting a controller.
This problem comes primarily due to the way the interface between the controller and the disruptor is implemented. The disruptor passes an anonymous function to the controller that generates the commands for each target pod. The logic of these functions cannot be tested without setting a controller which will initiate the call to the functions.
It is important to keep in mind that this model of interacting between the disruptors and the controller (the disruptor calls the controller's visit function that calls back the disruptor) was introduced to allow a per-target validation and customization of the commands.
This PR introduces a new interface for generating these commands and the implementations for each type of fault. In this way,
the command generation can be implemented independently of the controller.
It is important to notice that in its current form, this is a naive implementation with learning purposes, to help in identifying common patterns that could be refactored into existing or new abstractions.
For example, the fault injection for each fault and target type is implemented as a different struct. Subsequent changes could introduce alternatives.
Fixes #301
Checklist:
make lint
) and all checks pass.make test
) and all tests pass.