linuxboot / linuxboot/contest

Simplify TestStep API

Open
#14 2 comments 0 reactions 1 assignee View on GitHub

@mimir-d is already working on this.

Since Aug 23, 2022.

Dominant language
Go
Stars
20
Forks
17
PR merge metrics
No merged PRs in 30d

Description

**Issue by [marcoguerri](https://github.com/marcoguerri)**
_Wednesday Feb 05, 2020 at 12:51 GMT_
_Originally opened as https://github.com/facebookincubator/contest/issues/23_

----

I have been thinking of how to implement single target cancellation for two reasons:

1) It's a feature we might want to support in the future
2) I believed it would be necessary when losing locks on the targets. However, after reading again the implementation based on Zookeeper, if things go wrong, we'll lose the connection pool to the whole ensemble. So we won't lose per-target-locks, but we'll lose all locks: we'll need to cancel the whole job (which we can do now).

Even though number 2) doesn't really apply, I had some ideas on how to simplify the API for TestStep. There is now a good amount of duplication. Taking a sample TestStep as example:

```
func (ts *SSHCmd) Run(cancel, pause <-chan struct{}, ch teststep.TestStepChannels, params teststep.TestStepParameters, ev eventmanager.Emitter) error {
if err := ts.validateAndPopulate(params); err != nil {
return err
}

f := func(cancel, pause <-chan struct{}, target *target.Target) error {
// DO WORK
}
return teststeps.ForEachTarget(Name, cancel, pause, ch, f)
```

* Validate Params
* Define the function object
* Call ForEachTarget

I would divide the TestStep API into high level API (supporting `ForEachTarget` and `ForAllTarget` semantics), and low level API, which uses directly the channels and can be used if more complex Target aggregations are necessary. Most of the use cases should be covered by the high level API.

With the high level API, the plugin would not implement the TestStep interface, but would define function objects and parameters that would be used to instantiate a generic SingleTargetTestStep struct. For example, myplugin.go would do something as follows:

```
var pluginName = "myplugin"

func run(cancel, pause <-chan struct{}, target *target.Target) error {
// Do work
}

func resume(cancel, pause <-chan struct{}, target *target.Target) error {
// Do work
}

func NewMyPlugin() TestStep {
return teststep.NewSingleTargetTestStep{
Run: run,
Resume: resume,
Name: pluginName
}
}
````

How does this help?

* It makes it easier to implement the API, as it removes the duplicated code coming from having to define the TestStep struct, call validateAndPopulate, call ForEachTarget, etc. A possible "regression" is that `validateAndPopulate` will change semantics, it will only be validate, there won't be anything to populate. I think it's fine, and it's actually in line with the direction we have taken to offload parameter expansion to the `Params` object.
* It will move the responsibility to schedule each Target into a `TestStep` instance to the `TestRunner`: this will allow per-Target control.
* It allows to implement the `ForAllTargets` semantics in the same way, also reducing at minimum the duplication
* It will still allow to use the "low level API", i.e. implementing directly the `TestStep` interface and use the channels directly.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.