Passing information between Test Steps

[mimir-d]

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 :)

[walterchris]

This totally makes sense. I like the approach that we emit events to pass information around. Maybe it's worth thinking about binding this to a specific key within the emit event? i.e. runtime_#event is accessible during runtime? Or does this makes things more complicated?

[rihter007]

I would go further and introduce variables, each test step will populate some set of dictionary, for example ssh can generate
{ stdout: "aa", stderr: "sss", result_code: ""}.
These variables could be populated within the test steps like we currently do for the target like.

[walterchris]

So we can access every variable populated by a test step?

[rihter007]

Yes!
Also on top of that we can develop simple assertion test steps that will just check these variables

[walterchris]

I can try to implement this - can you give me some starting point here? Or is that something that you're pushing internally already? :)

[rihter007]

I haven't started doing it, I was just musing about how the feature could look like, so you can start doing it :)
But it would be nice to have more opinions @mimir-d @xaionaro @tfg13 @dimkup

[xaionaro]

Let's schedule a meeting to discuss this (and announce it on the contest channel in the OSF workspace in Slack)?

My only concern is: it feels like we need to redesign test steps API a bit (writing asynchronous resumeable configurable interruptable test steps becomes a real pain). And if we want to use context to pass-through information, it feels like we add more complexity to the current not-yet-improved API (and potentially to a tech debt). Before making a decision how to make this specific feature I would love to hear if @mimir-d have any opinion on what API we should have in the long term.

In other words, it would be nice to implement this feature in a way which aligns the best with the envisioned long term teststep API development.

[mimir-d]

This is gonna sound bad, but I'll be honest with you all. I think any kind of implementation can go ahead right now regarding this feature, and it will be refined (or redesigned) in the future refactoring. There is a slight desire to have this close to a possible final form (as xaionaro's saying) but I don't think that should be a hard blocker right now. I mean... just don't use globals or something like that :)

[xaionaro]

Yeah, I agree, it is not a blocker. It is just a concern. And basically any way is good :)

[rihter007]

The internal implementation can always be reworked, so I'm not worried about it.
I'm more concerned about the "interface" part, the way people should write the actual tests, so I would go with a short meeting as @xaionaro proposed

[walterchris]

Meeting sounds good.

I do agree that we are increasing complexity here - however for this to work in a broader scope - I think that's important. Maybe we can brainstorm during the meeting :)

[rihter007]

Sorry, for delay, had some internal discussion on how to make the change in job descriptor more clear:

So, the raw proposal of "public interface" change is the following:

1. Each test step should be able to generate a fixed set of variables that could be of type string, int, double, boolean
   For example:

• sshcmd plugin can generate stdout: string, stderr: string, result_code: string
• waitport plugin can generate: status: bool (whether it succeed or not)

2. Each generated by a test step variable can be mapped into some string name:
   In the example below, we map "stdout" (generated by plugin sshcmd) to "stdout_sshcmd" which can later be referred in other test steps:

{
    "label": "output",
    "name": "sshcmd",
    "parameters": {
        "args": [
            "Blah blah"
        ],
        "executable": [
            "echo"
        ],
        "target": [
            "{{ .FQDN }}"
        ],
        "user": [
            "root"
        ],
        "connection_timeout": [
            "5m"
        ],
        "timeout": [
            "5m"
        ]
    },
    "variables": {
        "stdout": "stdout_sshcmd"
    }
}

Here one should also check that sshcmd produces stdout variable and if not, fail early

3. Produced variables should be used in other test steps, for example in the same manner as with Target:

{
    "label": "echo",
    "name": "echo",
    "parameters": {
        "text": "{{ echo_sshcmd }}"
    }
}

4. Variables should be also checked in "assertion" test step: (similar like we do for success rate in Reporter)

{
    "label": "check that string is expected",
    "name": "assertion",
    "parameters": {
        "check": "{{ echo_sshcmd }} == \"Blah blah\""
    }
}

Feel free to comment, brainstorm. @walterchris Can we discuss the meeting time in slack?

[mimir-d]

Few comments.

• I would name that "variables" as "outputs" or "outvars", something to show it's an output.
• the usage syntax probably needs adjusting, like "{{vars.stdout_sshcmd}}", or some command form? (this command might also deal with the state resumption problem, rather than encode that in the template expander)
• the usage in no 4 should be dropped; implementing expressions is not trivial and looks like scope creep here. The success expr in reporter is not easy to use, neither do I see a lot of value in this check happening in the job descriptor. It can be moved inside the teststep plugin, only ingesting the variable value in the descriptor

[rihter007]

Few comments.

• I would name that "variables" as "outputs" or "outvars", something to show it's an output.
• the usage syntax probably needs adjusting, like "{{vars.stdout_sshcmd}}", or some command form? (this command might also deal with the state resumption problem, rather than encode that in the template expander)
• the usage in no 4 should be dropped; implementing expressions is not trivial and looks like scope creep here. The success expr in reporter is not easy to use, neither do I see a lot of value in this check happening in the job descriptor. It can be moved inside the teststep plugin, only ingesting the variable value in the descriptor

Number 4 has definitely less priority and could be omitted, but it makes tests more clear and removes the very same checks from each and every test step