Issue by insomniacslk
Tuesday Feb 04, 2020 at 16:25 GMT
Originally opened as https://github.com/facebookincubator/contest/issues/5

Panic in goroutines other than the main one cannot be recovered explicitly, they have to be handled in the goroutine itself[1].
For ConTest this means that if a test step or any other plugin has a goroutine that panics, we are not able to catch it.

We should write a tool to detect (statically or dynamically) whether each spawned goroutine attempts to recover, and warn that the plugin may break the service run.

[1] https://stackoverflow.com/questions/50409011/handling-panics-in-go-routines

The ambiguity of stepState.resumeState is that it is used for both input and output and we get the final result in TestRunner.waitStepRunners which should return collected resume states as well as errors as an output parameter

The problem raises as we can't atomicly (at least without "ugly" locks in events emission) pass the target to the test step via channel and emit EventTargetIn event.

Currently we are having the following partial work-around:

https://github.com/linuxboot/contest/blob/main/pkg/runner/step_runner.go#L193

// put the target into step runner
case sr.input <- tgt:
// by the time we are hare, the test step could have already processed the target and emitted 100500 events
// test steps rarely emit events, so it is not a big issue. For consistency with TargetOut or TargetError I made this hack:

    // we should always emit TargetIn before TargetOut or TargetError
    // we have a race condition that outputLoop may receive result for this target first
    // in that case we will emit TargetIn in outputLoop and should not emit it here
    sr.mu.Lock()
   if targetInfo.acquireTargetInEmission() {
       if err := emitEvent(ctx, ev, target.EventTargetIn, tgt, nil); err != nil {
           sr.setErrLocked(ctx, fmt.Errorf("failed to report target injection: %w", err))
       }
    }
   sr.mu.Unlock()

The way how all the contents of StepState rely on TestRunner's lock is not trivial.
This should be removed or made more explicit

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.

TestRunner.Run method should not consume JobID/RunID parameters

Issue by rjoleary
Saturday Mar 07, 2020 at 02:41 GMT
Originally opened as https://github.com/facebookincubator/contest/issues/71

Each jobs has its own process. This has the following benefits:

• Failure isolation: If a test panics unexpectedly (for example it runs out of memory), only that test is affected and not the whole system.
• Cancel jobs: Allow jobs to be force cancelled simply by killing the process.
• Resource limits: Each job can have its own limits on memory, fds, cpu, etc...

Currently, test step plugins are required to have the following interface:

Load() (
    string, // plugin identifier
    test.TestStepFactory, // instance factory: func() test.TestStep
    []event.name // events allowed to be emitted
)

Run(
    ctx xcontext.Context, // cancellation context
    ch test.TestStepChannels, // input, output chans
    ev testevent.Emitter, // event emitter interface
    stepsVars test.StepsVariables, // step output variables accessor
    params test.TestStepParameters, // map[string] -> list of Param (raw json)
    resumeState json.RawMessage, // input serialized plugin state, when resuming
) (
    json.RawMessage, // output serialized plugin state, when pausing
    error
)

ValidateParameters(
    ctx xcontext.Context,
    params test.TestStepParameters // map[string] -> list of Param (raw json)
) error

// required but unused
Name() string

Most also define (but not required by contest server):

New() test.TestStep // passed in test.TestStepFactory in Load()

The lifecycle of a plugin is: instance is created before main loop and put into a "step bundle" object (owned by test, and then job), that gets carried around until it reaches StepRunner (field of StepState, should also be removed). Step runner calls Run() once (per job, new TestRunner made in JobRunner), and leaves it running in a goroutine. Step runner is stopped in stepState.DecreaseLeftTargets, when number of targets remaining to be injected into a step plugin is 0 (note, this whole thing needs refactoring, but that's for another tracking issue).

Issue 1: The validate params method is usually implemented by another method in plugins, that also gets called at the beginning of Run(), resulting in a double parsing of the input json.
Instead, the plugin instance should be valid by construction. This implies removal of the ValidateParameters method, and plugin config structs should be created along with the plugin instance. Config param interpolation can be done in Run() on a per target basis.

Issue 2: The Run method requires a number of dependencies from inside the server core, and this list may increase later (as was the case with StepsVariables). These dependencies may also depend on other objects, which will in effect explode the signature of Run.
Instead, an interface should be presented to the plugin (we dont require ABI compat) which contains getter methods for these dependencies (called services here).
Proposal

type PluginServices interface {
    EventEmitter() testevent.Emitter
    StepsVariables() test.StepsVariables
    Logger() xcontext.Logger // with any tags needed
    // maybe Metrics() xcontext.Metrics
    // maybe Tracer() xcontext.Tracer
    // maybe something to provide state pause/resume services
    // other
}

// note that xcontext.Context is also removed, for the standard golang Context
Run(
    ctx context.Context,
    chans test.TestStepChannels,
    params test.TestStepParameters,
    svc PluginServices, // new
    resumeState json.RawMessage,
) (json.RawMessage, error)

Issue 3: The plugin currently accepts both a target input chan and an output one. It is generally advised that chans are created by the code that knows most about said channel. In this case, the input chan should be created by caller, but the output should be created by the plugin.
Proposal (provisional, may instead require Run to launch async itself and return an output chan)

Run(
    ctx xcontext.Context,
    input <-chan *target.Target,
    // ...
) (json.RawMessage, error)

Output() chan<- step.TestStepResult

one should be able to override the default variables used in http logger from the cli.

admin server storage api should have context parameter to support:

• function cancellation
• sharing contexts with logger, etc.

Issue by marcoguerri
Wednesday Feb 05, 2020 at 12:48 GMT
Originally opened as https://github.com/facebookincubator/contest/issues/21

Using []Struct is in most cases probably desirable, but in some cases []*Struct might make sense (e.g. Targets). We should re-evaluate these cases. Here is where we use []*, excluding Targets:

pkg/test/fetcher.go:16: Fetch(interface{}) (string, []*TestStepDescriptor, error)
pkg/job/reporter.go:20: Report(cancel <-chan struct{}, parameters interface{}, results []*test.TestResult, ev testevent.Fetcher) (bool, interface{}, error)
pkg/job/job.go:27:      TestDescriptors []*test.TestDescriptor
pkg/job/job.go:63:      Tests       []*test.Test
pkg/runner/job_runner.go:49:            testResults []*test.TestResult
pkg/jobmanager/jobmanager.go:88:        tests := make([]*test.Test, 0, len(jd.TestDescriptors))
plugins/testfetchers/uri/uri.go:86:func (tf *URI) Fetch(params interface{}) (string, []*test.TestStepDescriptor, error) {
plugins/testfetchers/uri/uri.go:117:            Steps []*test.TestStepDescriptor
plugins/testfetchers/literal/literal.go:29:     Steps    []*test.TestStepDescriptor
plugins/testfetchers/literal/literal.go:54:func (tf *Literal) Fetch(params interface{}) (string, []*test.TestStepDescriptor, error) 

Issue by insomniacslk
Tuesday Feb 04, 2020 at 16:31 GMT
Originally opened as https://github.com/facebookincubator/contest/issues/11

We may need to forward a user's identity so that ConTest can log with the proper key/certs. This needs support on both sshcmd and the client side (i.e. the API). This needs a proper design session

move TargetIn event emission to StepRunner

Issue by insomniacslk
Tuesday Feb 04, 2020 at 16:37 GMT
Originally opened as https://github.com/facebookincubator/contest/issues/16

The job ID is unique per reporter, it is redundant to have it in each TestResult of the results array. It also makes it more explicit to the user

Issue by xaionaro
Friday Oct 09, 2020 at 09:54 GMT
Originally opened as https://github.com/facebookincubator/contest/issues/149

This is a follow-up task of discussion: https://github.com/facebookincubator/contest/pull/148

(feel free to edit the issue)

Problem

Currently we just have a database where we store the whole history of anything ever happened. So longer we use ConTest, more data is stored and those tables. And the problem is that in long term any migration/backup/whatever of the DB might take too much time. IMO, it makes sense to consider ways to rotate/archive data or partition it.

Action required

So I'm asking to share your thoughts about the problem and possible solutions, including data rotation/archivation and data partitioning. We need to decide what and how we will do about the problem.

--- FAIL: TestJobManagerSuiteRdbmsStorage (14.48s)
common.go:389:
Error Trace: common.go:389
suite.go:166
suite.go:177
jobmanager_rdbms_test.go:35
Error: Received unexpected error:
leaked goroutines:
18 event_handler.go:187 github.com/linuxboot/contest/pkg/xcontext.(*ctxValue).addEventHandler.func1
internal/race.Acquire(0xc00051f7b0)
/usr/local/go/src/internal/race/race.go:16 +0x67
sync.(*Mutex).Lock(0xc00051f7b0)
/usr/local/go/src/sync/mutex.go:76 +0xab
github.com/linuxboot/contest/pkg/xcontext.(*ctxValue).addEventHandler.func1(0xc00027a230)
/go/src/github.com/linuxboot/contest/pkg/xcontext/event_handler.go:187 +0x7e
Test: TestJobManagerSuiteRdbmsStorage

Struct conversion between server layer and storage layer in admin server can be based on reflection in order to be generic and flexible.

I would like to do some logging on a target after each test run no matter whether it succeeds or fails.
Currently to do so, I need to implement this logic in ReleaseTargets() method that will be unique for each TargetsManager, while it looks like these actions should be common and shared across all of them.

CC: @tfg13 , @rminnich , @insomniacslk

Issue by marcoguerri
Thursday Feb 06, 2020 at 15:06 GMT
Originally opened as https://github.com/facebookincubator/contest/issues/34

Right now there is no source of truth that tells which targets were acquired at the beginning of the job, that can be references for example at events submission time. To have a normalized data model, we need to store targets acquired as well.

Remove runStepIfNeeded
injectTarget should run step (if it is not launched already) and return a future object for target result.

All members should be hided behind methods

Issue by insomniacslk
Tuesday Feb 04, 2020 at 16:35 GMT
Originally opened as https://github.com/facebookincubator/contest/issues/15

This is currently left to plugin implementations, which may or may not require their presence. We also need to reject jobs if no device is returned. The various Host and Device should become Target (i.e. MinTargets and MaxTargets) to have consistent naming.

Issue by insomniacslk
Thursday Feb 06, 2020 at 14:11 GMT
Originally opened as https://github.com/facebookincubator/contest/issues/33

Currently every reporter is responsible for result calculation and delivery of the report. However some tasks are common to many reporters (e.g. sending results over e-mail), and it would be easier on the user if they were supported out of the box. I propose to add a Methods section in the reporters that will allow to specify multiple ways of delivering a report.

This should send the JSON report via the specified methods, and also allow (optional) filtering and templating so users can format the report as desired.

    "Reporting": {
        "RunReporters": [
            {
                "Name": "TargetSuccess",
                "Parameters": {
                    "SuccessExpression": ">80%"
                },
                "Methods": [
                    {
                        "name": "email",
                        "recipient": "[email protected]"
                    }
                }
            ]
        ],
        ...
    }

This is a parent issue that includes:
#66
#67
#68
#71
#72
#74
#75

Issue by insomniacslk
Monday May 11, 2020 at 15:19 GMT
Originally opened as https://github.com/facebookincubator/contest/issues/100

There are cases where a single target manager is not enough, and we may want to connect multiple target managers in a chain, to filter results. This requires a redesign of the target manager interface (to have multiple target managers) and in the way they are called in sequence, connecting the results of the previous to the next

Issue by insomniacslk
Friday Feb 07, 2020 at 13:37 GMT
Originally opened as https://github.com/facebookincubator/contest/issues/39

Similarly to #38, we want to integrate with Gerrit, so we can test coreboot builds.

One should be always able to put result into target.resCh

StepRunner should be aware of all input targets and those that it didn't get result for

Issue by dimkup
Thursday Aug 27, 2020 at 15:47 GMT
Originally opened as https://github.com/facebookincubator/contest/issues/141

Intro

There are some cases when it would be useful to pass some information between Test Steps in the same Job/Run.

For example some step could acquire some resource (for example create a block list rule). The resource should be released by some further Test Step, so in order to do it effectively, the first step should pass resource ID to the second step.

Another example: some Test Step produces an artefact (builds a package) and another Test Step should deploy or run the artefact. It would be good to have a way to pass the artefact ID/url to the next step.

The Idea

Currently there is no way Test Steps could pass information to each other. In order to introduce it, we could try to extend the Target structure with kind of context, but there are some issues with the approach:

• The context is "per target", but the information we want to pass between steps in not necessarily "per target". Some Test Step could collect all the targets first and acquire some resource which is shared among the targets.
• The context is shared among Test Steps, so some Test step could damage the context in unexpected way
• The context has to be unstructured. Since we don't know in advance, what kind of information we need to pass, we have to make the context dynamic structure, what is error prone.

There is another approach to the information passing - reuse existing event infrastructure. Generally the only thing we need to do here is to extend Test Step's Run method signature with Fetcher parameter and change existing limitations on event queries. This way Test Steps would be able to emit and consume events from each other. How could it help?

• Event can have any context - "pre Target", "per Run", "per set of Targets", "per Job" etc
• Event is a standalone immutable structure. There is no way it can be changed after the emission.
• Events are specific and could be tied to a specific structure. (It would require additional changes)

Since most of the time Test Steps need to interact inside the same Run, as suggested by @insomniacslk, it's pretty easy to keep them in a local memory cache and heat up the cache from the DB in case of restart.

The issues with the approach:

• There is no warranty that Test Step consumes existing event. (Probably we could force teststep plugins to register as event consumers and check producer/consumer match on start)
• Missing/duplicated events. It would be hard to enforce produce/consume logic in a way each instance of produced event of certain type should be consumed specific number of times. But may be it's an overkill :)
• Cross plugin dependency. Probably we could restrict producer/consumer code to be in one teststep plugin. Of course it would not work for all scenarios, but could be a tradeoff.
• TBC :)

We should not use context as structure members

Issue by insomniacslk
Thursday Dec 03, 2020 at 21:20 GMT
Originally opened as https://github.com/facebookincubator/contest/issues/192

As noted in https://github.com/facebookincubator/contest/pull/188 our CI doesn't detect correctly when non-go files are missing the copyright header. Specifically, despite adding *.sql to the configuration of checklicenses, it always reports success, even when the license header is missing or wrong.

One may use instead of readingLoopRunning + conditional variable WaitGroup.

admin server db should have indexes over log data and log dates to support execute the queries more efficiently

Currently StepRunner may panic if one adds new target after close.
This behaviour was inherited from TestRunner (to keep the change small) which avoids it using its internal state.

When step finishes processing of a target and at the same time pause happens, we loose information that the target was already processed

Logs:

2022-06-21T21:44:01.501Z D teststeps.go:158] ForEachTargetWithResume: target T1 completed successfully attempt=%!s(uint32=0) job_id=1 run_id=2 step_index=1 step_label=Test1Step2
[2022-06-21T21:44:01.501Z I server.go:297] Signal "terminated", pausing jobs
[2022-06-21T21:44:01.503Z D httplistener.go:236] Received server shut down request
[2022-06-21T21:44:01.503Z I step_runner.go:318] Obtained '{Target{ID: "T1", TMS: "{"token":"1-T1"}"} }' for target 'T1' attempt=%!s(uint32=0) job_id=1 run_id=2 step_index=1 step_label=Test1Step2
[2022-06-21T21:44:01.505Z I jobmanager.go:165] Listener shut down successfully.
[2022-06-21T21:44:01.505Z D step_runner.go:415] TestStep finished 'job is paused', rs: '{"V":1}' attempt=%!s(uint32=0) job_id=1 run_id=2 step_index=1 step_label=Test1Step2
[2022-06-21T21:44:01.506Z I jobmanager.go:189] Paused
[2022-06-21T21:44:01.507Z D jobmanager.go:306] JobManager: pausing job 1
[2022-06-21T21:44:01.506Z E step_runner.go:434] err: job is paused attempt=%!s(uint32=0) job_id=1 run_id=2 step_index=1 step_label=Test1Step2
[2022-06-21T21:44:01.508Z I jobmanager.go:266] Waiting for 1 jobs
[2022-06-21T21:44:01.509Z I jobmanager.go:207] Paused
[2022-06-21T21:44:01.510Z D jobmanager.go:306] JobManager: pausing job 1
[2022-06-21T21:44:01.509Z D step_runner.go:395] output channel closed attempt=%!s(uint32=0) job_id=1 run_id=2 step_index=1 step_label=Test1Step2
[2022-06-21T21:44:01.510Z I test_runner.go:575] step runner results in err: attempt=%!s(uint32=0) job_id=1 run_id=2 step_index=1 step_label=Test1Step2
[2022-06-21T21:44:01.512Z D step_runner.go:128] Running loop finished attempt=%!s(uint32=0) job_id=1 run_id=2 step_index=1 step_label=Test1Step2
[2022-06-21T21:44:01.512Z E test_runner.go:602] setErrLocked: job is paused attempt=%!s(uint32=0) job_id=1 run_id=2 step_index=1 step_label=Test1Step2
[2022-06-21T21:44:01.513Z D step_runner.go:292] Output channel closed attempt=%!s(uint32=0) job_id=1 run_id=2 step_index=1 step_label=Test1Step2
[2022-06-21T21:44:01.514Z D test_runner.go:589] step context was cancelled attempt=%!s(uint32=0) job_id=1 run_id=2 step_index=1 step_label=Test1Step2
[2022-06-21T21:44:01.514Z D step_runner.go:134] Reading loop finished attempt=%!s(uint32=0) job_id=1 run_id=2 step_index=1 step_label=Test1Step2
[2022-06-21T21:44:01.515Z D test_runner.go:429] [Target{ID: "T1", TMS: "{"token":"1-T1"}"} 1 run false ]: canceled 2 attempt=%!s(uint32=0) job_id=1 run_id=2 target=T1
[2022-06-21T21:44:01.515Z D step_runner.go:119] StepRunner finished attempt=%!s(uint32=0) job_id=1 run_id=2 step_index=1 step_label=Test1Step2
[2022-06-21T21:44:01.517Z E test_runner.go:509] context canceled attempt=%!s(uint32=0) job_id=1 run_id=2 target=T1

This loop is used to know that targets processing is finished and also detecting ErrTestStepLostTargets.
After #67 will be implemented, one can just wait for all target processing goroutines to finish using "targetsWg"

There are just two places left where this variable is used.

Issue by xaionaro
Tuesday Mar 03, 2020 at 16:46 GMT
Originally opened as https://github.com/facebookincubator/contest/issues/61

I guess we should also add fuzz tests to all methods of the api.API. It may help to improve both:

• Security.
• Reliability.

If TargetsManager fails to acquire targets (which can be a temporary problem), the test will not be retries.

The reason of this problem is that we treat all acquiring errors as fatal and do not retry.
The acquiring errors could be put into the following categories:

• Target manager failed (could be retried)
• Target locking failed (could be a temporary problem if the target is used by someone else)
• Context cancellation + Deadline

All problems except context cancellation that means that the test as cancelled could be retried

Issue by insomniacslk
Monday Feb 24, 2020 at 18:32 GMT
Originally opened as https://github.com/facebookincubator/contest/issues/53

Almost every plugin will have to replicate certain steps to validate its parameters, e.g.:

• ensure that all and only the supported parameters are passed
• ensure that the passed parameters have the right types
• ensure that a parameters receives the correct number of parameters (e.g. zero, more-than-zero, at-least-one, between-2-and-3, etc)

All of the above checks can be offloaded from the plugin developer, and done in a consistent and declarative manner

Not if we should reject them right away, or run them and just call them a success.
No strong opinions either way, but at the moment we crash the server:

panic: runtime error: index out of range [0] with length 0

goroutine 116 [running]:
github.com/facebookincubator/contest/pkg/runner.(*TestRunner).run(0xc000737720, 0x99d4f8, 0xc000737c20, 0xc0008b4b00, 0xc000720180, 0x3, 0x3, 0x740, 0x1, 0x0, ...)
	github.com/facebookincubator/contest/pkg/runner/test_runner.go:257 +0x1d16
github.com/facebookincubator/contest/pkg/runner.(*TestRunner).Run(0xc000737720, 0x99d4f8, 0xc000737810, 0xc0008b4b00, 0xc000720180, 0x3, 0x3, 0x740, 0x1, 0x0, ...)
	github.com/facebookincubator/contest/pkg/runner/test_runner.go:133 +0x486
github.com/facebookincubator/contest/pkg/runner.(*JobRunner).Run(0xc000722690, 0x99d4f8, 0xc0000caeb0, 0xc000a1e140, 0x0, 0x0, 0x0, 0x0)
	github.com/facebookincubator/contest/pkg/runner/job_runner.go:232 +0x11d3
github.com/facebookincubator/contest/pkg/jobmanager.(*JobManager).runJob(0xc000139720, 0x99d4f8, 0xc0000ca2d0, 0xc000a1e140, 0x0)
	github.com/facebookincubator/contest/pkg/jobmanager/start.go:98 +0x2b3
created by github.com/facebookincubator/contest/pkg/jobmanager.(*JobManager).startJob
	github.com/facebookincubator/contest/pkg/jobmanager/start.go:80 +0x245

Issue by walterchris
Thursday Nov 05, 2020 at 12:31 GMT
Originally opened as https://github.com/facebookincubator/contest/issues/166

Currently after every run, the RunReport function from every registered reporting plugin will be invoked. After all runs, the FinalReport function will be invoked. It's not possible right now to access the RunReport report's from the FinalReport plugin. This way I can not access intermediate RunReports to generate a FinalReport out of this. So it would be nice if we can add the functionality such that I am able to access the RunReport reports (at least from the same plugin) during the FinalReport function.

time="2021-12-19T01:33:58Z" level=debug msg="monitor pass 2: [#0 Step 1]: no more targets, closing input channel" file="test_runner.go:702"
time="2021-12-19T01:33:58Z" level=debug msg="monitor: waiting for targets to finish" file="test_runner.go:707"
time="2021-12-19T01:33:58Z" level=debug msg="monitor pass 3: [Target{ID: "T1"} 0 run false ]" file="test_runner.go:715"
time="2021-12-19T01:33:58Z" level=debug msg="TestStep finished '', rs " file="step_runner.go:253" step_index=0 step_label="Step 1"
time="2021-12-19T01:33:58Z" level=error msg="err: test step Step 1 closed output channels (api violation)" file="step_runner.go:272" step_index=0 step_label="Step 1"
time="2021-12-19T01:33:58Z" level=debug msg="output channel closed" file="step_runner.go:233" step_index=0 step_label="Step 1"
time="2021-12-19T01:33:58Z" level=debug msg="Running loop finished" file="step_runner.go:99" step_index=0 step_label="Step 1"
time="2021-12-19T01:33:58Z" level=error msg="step runner results an err: " file="test_runner.go:541" step_index=0 step_label="Step 1"
time="2021-12-19T01:33:58Z" level=error msg="setErrLocked: test step Step 1 closed output channels (api violation)" file="test_runner.go:568" step_index=0 step_label="Step 1"
time="2021-12-19T01:33:58Z" level=debug msg="monitor: finished, test step Step 1 closed output channels (api violation)" file="test_runner.go:743"
time="2021-12-19T01:33:58Z" level=error msg="monitor returned error: "test step Step 1 closed output channels (api violation)", canceling" file="test_runner.go:200"
time="2021-12-19T01:33:58Z" level=debug msg="waiting for step runners to finish" file="test_runner.go:290"
time="2021-12-19T01:33:58Z" level=info msg="Obtained '{Target{ID: "T1"} }' for target 'T1'" file="step_runner.go:191" step_index=0 step_label="Step 1"
time="2021-12-19T01:33:58Z" level=debug msg="reading loop detected context canceled" file="step_runner.go:211" step_index=0 step_label="Step 1"
time="2021-12-19T01:33:58Z" level=debug msg="step context was cancelled" file="test_runner.go:555" step_index=0 step_label="Step 1"
time="2021-12-19T01:33:58Z" level=debug msg="Reading loop finished" file="step_runner.go:105" step_index=0 step_label="Step 1"
time="2021-12-19T01:33:58Z" level=debug msg="waiting for target handlers to finish" file="test_runner.go:212"
time="2021-12-19T01:33:58Z" level=debug msg="StepRunner finished" file="step_runner.go:91" step_index=0 step_label="Step 1"
time="2021-12-19T01:33:58Z" level=debug msg="[Target{ID: "T1"} 0 run false ]: canceled 2" file="test_runner.go:417" target=T1
time="2021-12-19T01:33:58Z" level=error msg="context canceled" file="test_runner.go:477" target=T1
time="2021-12-19T01:33:58Z" level=debug msg="[Target{ID: "T1"} 0 run false ]: canceled 1" file="test_runner.go:482" target=T1
time="2021-12-19T01:33:58Z" level=debug msg="[Target{ID: "T1"} 0 run false ]: target handler finished" file="test_runner.go:501" target=T1
time="2021-12-19T01:33:58Z" level=debug msg="leaving, err test step Step 1 closed output channels (api violation), target states:" file="test_runner.go:224"
time="2021-12-19T01:33:58Z" level=debug msg=" 0 [Target{ID: "T1"} 0 run true ] test step Step 1 closed output channels (api violation) false" file="test_runner.go:241"
time="2021-12-19T01:33:58Z" level=debug msg="- 1 in flight, ok to resume? false" file="test_runner.go:243"
time="2021-12-19T01:33:58Z" level=debug msg="step states:" file="test_runner.go:244"
time="2021-12-19T01:33:58Z" level=debug msg=" 0 [#0 Step 1] true false false test step Step 1 closed output channels (api violation) " file="test_runner.go:246"
--- FAIL: TestTestRunnerSuite (2.28s)
--- FAIL: TestTestRunnerSuite/TestStepClosesChannels (0.01s)
test_runner_test.go:345:
Error Trace: test_runner_test.go:345
Error: Not equal:
expected: "\n{[1 1 SimpleTest 0 Step 1][Target{ID: "T1"} TargetIn]}\n{[1 1 SimpleTest 0 Step 1][Target{ID: "T1"} TargetOut]}\n"
actual : "\n{[1 1 SimpleTest 0 Step 1][Target{ID: "T1"} TargetIn]}\n"

        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -2,3 +2,2 @@
        	            	 {[1 1 SimpleTest 0 Step 1][Target{ID: "T1"} TargetIn]}
        	            	-{[1 1 SimpleTest 0 Step 1][Target{ID: "T1"} TargetOut]}
        	            	 
        	Test:       	TestTestRunnerSuite/TestStepClosesChannels

FAIL
FAIL github.com/linuxboot/contest/pkg/runner 2.405s
FAIL

TestRunner.targetsWg is used only in Run() method, it could be made a local variable

One can make decision that an input channel for a step should be close in targetHandler

Issue by rjoleary
Friday Mar 06, 2020 at 23:45 GMT
Originally opened as https://github.com/facebookincubator/contest/issues/66

The json sent between server and client should have a version number. The client/server should check the version number to ensure they are still compatible.

Issue by insomniacslk
Wednesday Jun 24, 2020 at 12:31 GMT
Originally opened as https://github.com/facebookincubator/contest/issues/124

Job spec changes cannot be properly validated without running an actual job. Adding a Validate API method that runs the plugin validation without actually starting a job might help.

Issue by marcoguerri
Wednesday Jun 17, 2020 at 09:09 GMT
Originally opened as https://github.com/facebookincubator/contest/issues/123

At the moment we have two type of events, testevents and frameworkevents. The former are meant to be used when emitted by a Step, the latter from anywhere in the framework outside of a Step. The intention was to enforce non-null on most of the fields of the Step.

Eventually we settled on the fact that this is too restrictive. FrameworkEvents are barely used, and lots of places of the framework need to emit events that include a subset of the information of testevents.

So, we agree on deprecating frameworkevents, and using one single concept of event, where fields might be null. In addition, this event should be associated to a "source" in the header (most likely split into SourceType and SourceName), which will indicate who emitted the event.

This will allow to create a precise check of output targets and move that check to StepRunner