Simplify TestStep API

[mimir-d]

Issue by 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.

[mimir-d]

Comment by tfg13
Friday Jul 16, 2021 at 08:54 GMT

---

[doing spring cleaning] Do you think we can close this? (Or maybe reword it)

I think we did most of this, but not everything. The are now better helpers that handle all the channel stuff, including job resumption - but yeah the parameter handling is still required, but not a lot of work.

[mimir-d]

can be included in #135
will review.