-
Notifications
You must be signed in to change notification settings - Fork 15
Conversation
The logic of the test runner is not modular enough to accomodate the need for implementing a cleanup pipeline. This revision refactors the test runner so that the concept of pipeline can be re-used. The eventual goal is to deploy two pipelines, one for testing and one for cleanup.
This revision further simplifies the routing logic and removes the dependency of the pipeline over the list of targets. The latter is important to implement pipelines that might have a variable number of targets coming in that cannot be predicted beforehand.
After extracting the pipeline logic, the routing logic was still part of the pipeline and very monolithic. This patch extracts the routing logic into a dedicated structure and splits it into routeIn logic and routeOut logic, which makes it unit-testable.
Pipeline and routing logic are now split into different files. Unit tests for the routing logic have been added.
This patch adds support for implementing a cleanup pipeline, which runs cleanup steps, after the main test pipeline. One of the requirements of ConTest has always been that it should support a "cleanup" phase, and this adds support for it. The cleanup pipeline is described in the same was test steps have been described so far: and additional "cleanup" section in the test descriptor is deserialized to implement a pipeline which runs the required cleanup steps.
The interface of TestSteps so far has dictated that targets should be forwarded to two different channels for success and failure. This makes several parts of ConTest logic relatively complicated. In order to simplify the framework, this braking change reduces the number out output channels that a test step needs to handle to only one. The object returned by the step might include error information for targets which failed in the test step.
Codecov Report
@@ Coverage Diff @@
## master #122 +/- ##
==========================================
+ Coverage 63.14% 64.12% +0.98%
==========================================
Files 51 58 +7
Lines 2846 3097 +251
==========================================
+ Hits 1797 1986 +189
- Misses 813 865 +52
- Partials 236 246 +10
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
The pipeline code was affected by some duplication in the shutdown control path. This patch removes the duplication, and introduces only two wait routines in the shutdown control path: waitTargets and waitControl.
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'll continue later.
storage2345,10.10.10.123 | ||
machinelearning1234,192.168.123.231 | ||
uselesshost10,10.0.0.10 |
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.
out of curiosity:
Why have you removed this lines? :)
}, | ||
{ | ||
"TargetManagerName": "CSVFileTargetManager", | ||
"TargetManagerAcquireParameters": { | ||
"FileURI": "hosts02.csv", | ||
"MinNumberDevices": 2, | ||
"MaxNumberDevices": 4, | ||
"HostPrefixes": [ | ||
] | ||
}, | ||
"TargetManagerReleaseParameters": { | ||
}, | ||
"TestFetcherName": "URI", | ||
"TestFetcherFetchParameters": { | ||
"TestName": "RackProvisioning", | ||
"URI": "test_samples/randecho.json" | ||
} |
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.
out of curiosity:
Why have you removed this lines? :)
} | ||
} | ||
] | ||
] |
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.
extreme nitpicking:
An extra space.
// error. Termination is signalled via terminate channel. | ||
func (p *pipeline) waitTargets(terminate <-chan struct{}, completedCh chan<- *target.Target) error { | ||
// emitStepEvent emits a failure event if a step fail | ||
func (p *pipeline) emitStepEvent(result *stepResult) error { |
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 interpret this name differently from what it does: I interpret that it sends an event on either success or failure, while actually it sends an event only on failure. BTW, why we do not send successes? To reduce amount of rows in DB?
pkg/runner/test_runner_pipeline.go
Outdated
completedTargetError error | ||
) | ||
// waitControl reads results coming from result channels (for steps and routing blocks) | ||
// until a timeout occurrs. The error handling is different depending on whether |
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.
nitpicking:
A typo: occurrs
→ occurs
.
Further simplification in the test runner. Routing blocks can become responsible for waiting for targets directly, so that the pipeline itself can just wait on routing blocks and their results.
This was supposed to be closed for the reason mentioned in #207 |
The pipeline code was affected by some duplication in the shutdown
control path. This patch removes the duplication, and introduces
only two wait routines in the shutdown control path: waitTargets
and waitControl.