Additional test checks

[happz]

As I think I have all I need for the plugin aspect of the implementation of additional checks, I'd like to get a better picture WRT what checks we're thinking about, when, and what kind of effect they might have, to summarize our current understanding, before I start hacking. And I'd like to find out answers for some aspects don't see clearly enough yet.

Issues

• Add additional test checks #216 - the original issue, mostly motivated by AVC denials
• Kernel aborts / panics detection #1235
• collect core dump files #1464
• Teach tmt to reboot unresponsive machines #1523

Pull requests

• Add additional test checks #216 - specification for L1/L2 keys
• Bootstrap before/after test checks #2210 - happz's stash and a dummy prototype implementation
• preparations for more plugin types
  • Adds "bootstrap logger" for logging before CLI options are recognized #1839
  • Refactor logging during plugin discovery, using tmt's logging #1840
  • Define a "plugin registry" class #1873

Checks

What's interesting

So far the following checks were identified as "interesting":

• detect AVC denials: AVC denials check for tests #2331
• detect kernel panic
• detect core dumps/ABRT
• test-inspector

Where?

• L1 metadata, i.e. a test gets a new key, check:

/test/one:
  ...
  check: abrt

/test/two:
  ...
  check:
    - abrt
    - save-dmesg
    - name: kernel-panic
      foo: bar
    # See questions at the end, but imagine L2 checks propagated to L1, can we disable checks on a per-test basis?
    - name: test-inspector
      enable: false

• L2 metadata, i.e. a plan gets a new key, check - same as the above.
• results.yaml gets new key, check, partially mirroring what's saved for a test itself:

/test/one:
    ...
    result: "pass"
    log:
        - /log1
        - /log2
    check:
        - name: abrt
          result: "pass"
          log:
            - /log3
            - /log4

/test/two:
    ...
    result: "pass"
    log:
        - /log1
        - /log2
    check:
        - name: abrt
          result: "pass"
          log:
            - /log3
            - /log4
        - name: save-dmesg
          result: "pass"
          log:
            - /dmesg.txt
        - name: kernel-panic
          result: "fail"
          log: []

When?

• for sure, after a test (Add additional test checks #216 (comment))
• checks may need to perform some of their magic before a test starts, e.g. the AVC denial detection might reset the audit log or store a timestamp
• some checks might even be interested in actions before running the first step/after running the last. Smells like prepare/finish phase, but an implicit one, injected by the check implementation rather than the one user needs to include. Suggested by Add additional test checks #216 (comment): ABRT check preparing the environment by enabling abrtd (before running tests), then collecting data in finish.

What?

• the basic functionality checks need is running commands on a guest
• checks may affect the result outcome of a test
• tests have "logs", besides their output they also produce files with extra information related to their function (e.g. dmesg output in dmesg.txt, or saved code dumps)

There seems to be the need for "during the test" type of checks as well, see #1523. Something that is executed while the test is still running, and might take action. While there is an overlap with what we're building for checks - implementation in plugins, with clear internal API & clear specification of how it interacts with tests (see "points" below) - it looks like something more watchdog-like in nature, and while definitely requested and needed, it does not fit the "additional check" concept discussed here & in the original issue. I'd implement it as a standalone type of action.

Implementation

• plugin-based, with plugins discovered like any other plugins we already have, re-using the established infrastructure
• we'd define "points" in which the execute plugin would call into (enabled) checks - before-all-tests, before-test, after-test, after-all-tests, (or [before|after]-all-tests could be implemented by (maybe) Plan calling checks to generate their prepare and finish phases).
• same approach as provides_method() but for "point" & name combination

Things that are unclear (to me)

• how shall checks affect the result of a test? E.g. a failed check means the test can't ever reach pass level, warn would be the best it can ever hope for? Or not at all, leave the evaluation to whoever runs tmt?
• what relationship do we want between L1 and L2 metadata? L2 overrides L1 completely? Does L1 inherit from L2? How about some non-trivial use cases, like "same checks for all tests in a plan", "same checks but test /foo needs this one extra", "same checks but test /foo needs this one extra and /bar needs one of the defaults disabled"?
• how about those "before the first starts" & "after the last test finishes" - do we want to implement them as prepare/finish phases, i.e. call plugins to add their own phases? This is what require does, BTW, it's converted into a prepare/install phase. prepare sounds harmless, but finish might be complicated as it might be way too late for checks to affect test results. Or we may support both, have two distinct check plugin entry points, "now it's time to add your prepare phase if you want to do so" and "we're in execute now, if you want to do something before we start running tests, do it now", and same for the "finish" actions ("we're done with tests, here are their results" & "now it's time to add your finish phase, if you want to do so".

Feel free to chime in, I'd like to know answers to the questions above - the implementation itself would be fairly quick and straightforward as soon as we resolve the unresolved.

---

So, what did I miss? Any obvious checks, points where it must influence or inspect the test process, or artifacts it might produce? Looking forward to hearing from you.

[martinhoyer]

how shall checks affect the result of a test? E.g. a failed check means the test can't ever reach pass level, warn would be the best it can ever hope for? Or not at all, leave the evaluation to whoever runs tmt?

I think it would be good if users could define in what check failures are acceptable/expected. For example matching ABRT "reason" and "package".
If there is only a "xfail" for these checks, it could mask failures for tests that trigger errors on purpose, or those testing something with long-term unfixed bugs.

A really nice feature imho would be to be able to specify a list of log files/dirs that should be included in case of the check fail. For example, when running iscsi tests, save contents of /var/lib/iscsi dir in addition to the dmesg, etc.

[martinhoyer]

@happz Do you know if there was any development in this area?
I'm planning to rewrite the part of my tests, which is doing basically what you described - detecting errors, panics, avc denials, etc and am wondering if some or all of it wouldn't be a duplication of what's being done (or was already done) as part of tmt.

[happz]

Not much, so far. I raised a couple of questions above before I dive in and start hacking. So far, I did some preparation steps and cleanups, and I'm willing to work on this once it's clear what are the expectations. At the end, at least AVC denials and kernel panic detection would be supported by tmt, I'm less sure about "detecting errors" as that is a pretty generic point.

[martinhoyer]

Right, let me know if I can help in any way.

[smwltr]

Hi, the RHIVOS QE team is also interested in this now for the kernel panic detection. In their case, it would not be intentional kernel panics. So they would be reported as test failure. Though, IIUC, intentional kernel panics would also need to be supported.

[psss]

Here's a couple of thoughts from myself:

• If we expect some of the checks to be executed during test execution, shouldn't there be three "points", e.g. before, during and after?

• The prepare and finish methods sound a bit more straightforward and clearly distinct rather than before-test and before-tests which could be confusing.

• Regarding the evaluation: I think the test result itself should not be affected (e.g. not changed). Various ways how we present results (e.g. html report or oculus in testing farm) should clearly show failing tests and include them in the aggregated "result", that is for example bring user attention to a failed avc check.

• Relationship between L1 and L2 should most probably me similar to what we do with the environment key, that is plan variables override individual values in test metadata. But, on the other hand I can imagine a situation when there is an exception for a single test. So not 100 % sure here.

• Regarding the before-all-tests and after-all-tests actions: What about performing these at the very beginning or end of the execute step? Would it make the implementation a bit more clear? Would it provide sufficient flexibility? Or we really need to perform special actions during the prepare step itself?

All in all, the outline looks very good to me. Thanks for summarizing the existing issues and putting it all together, @happz!

[happz]

• If we expect some of the checks to be executed during test execution, shouldn't there be three "points", e.g. before, during and after?

Depends on the wording, I suppose. Yes, there would be things happening while the test is running, and we may call it a point in the sense discussed above, although it's more of a period of time rather than an event.

• The prepare and finish methods sound a bit more straightforward and clearly distinct rather than before-test and before-tests which could be confusing.

Actually, I would stay away from prepare and finish here, to avoid confusing them with prepare and finish steps. Since there will be several such steps, I wanted to make the distinction between "before" and "after", and include the relationship (a test or all tests). "setup" and "teardown" are also commonly used for similar functionality, but these checks don't really setup/teardown stuff, we're talking about moments in the workflow when they trigger, therefore I tend towards before-tests, before-test, after-test, after-tests.

Note that these names are not necessarily to appear in user docs, it's just a concept for workflow diagrams, plugin methods, and docstrings, tmt specification may be much more user-friendly, and it already is:

/test/two:
  ...
  check:
    - abrt
    - save-dmesg
    - name: kernel-panic
      foo: bar

We just need clear spots for plugins to connect to the workflow, and for these and their developers we would say "here's a point called before-tests, and if you defined a method called before_tests, it would be called before any test starts on the guest".

• Regarding the evaluation: I think the test result itself should not be affected (e.g. not changed). Various ways how we present results (e.g. html report or oculus in testing farm) should clearly show failing tests and include them in the aggregated "result", that is for example bring user attention to a failed avc check.

Ok, makes sense, +1.

• Regarding the before-all-tests and after-all-tests actions: What about performing these at the very beginning or end of the execute step? Would it make the implementation a bit more clear? Would it provide sufficient flexibility? Or we really need to perform special actions during the prepare step itself?

Yes, calling these by execute step would be my preferred approach. I mentioned prepare step to illustrate we do have before-all-tests-ish actions that materialize as distinct prepare phases. I wouldn't implement checks like this.

[psss]

I wanted to make the distinction between "before" and "after", and include the relationship (a test or all tests). "setup" and "teardown" are also commonly used for similar functionality, but these checks don't really setup/teardown stuff, we're talking about moments in the workflow when they trigger, therefore I tend towards before-tests, before-test, after-test, after-tests.

I see, understood. Still, I'd say the one-letter difference might be a bit confusing (when reading the code, not for the user). What about before-all-tests, before-test, after-test, after-all-tests?

Yes, calling these by execute step would be my preferred approach. I mentioned prepare step to illustrate we do have before-all-tests-ish actions that materialize as distinct prepare phases. I wouldn't implement checks like this.

Ack.

[happz]

Including -all- sounds very good, will do so.

[thrix]

@happz seems it is still not updated in the description

[happz]

Updated the description above. Note that #2210 does not implement [before|after]-all-tests yet, that's left for later.

[kkaarreell]

Hello, I have few comments to this feature:

1. Regarding plugable infra. Basically it assumes that these checks would be "hardcoded" to tmt/infra and the user would be only able to choose from the existing checks. Personally, I find it quite limiting as it requires the respective check to be available in all the infra that a user would be using and there won't be an option to "customize" the check or enable a custom one.
2. Regarding the reporting, I think it is reasonable to add those details to results.yaml but it should be clarified what would be the impact on the original test result and how the test result (and checks) would be reflected e.g. in resutls.xml or when imported to RP. To me, it would make sense to make the impact configurable on test plan level (with some reasonable tmt defaults).

[kkaarreell]

Are we talking about some kind of hints, e.g. the following in my test metadata?

Yes. I think it would be nice to be able to configure if whether a specific check should have an impact on the previous test result. From a reporting perspective, people do not have a need to look at passing results so personally I would rather expect the test to fail and have the check log uploaded with other test artifacts and available for a review.

[psss]

I think it would be nice to be able to configure if whether a specific check should have an impact on the previous test result.

I'm not sure about overwriting the test results (if that's what you mean by "have an impact on"). I'd say it would be best to have the full information in the results.yaml, without any modifications or adjustments but allow to present this information to the user in a good way.

So, for example, for a failed avc check the user would see an extra column in the html report, or an extra line in the display report. Also the overall aggregated summary would clearly state both test and check results:

1 test passed, 1 test failed, 1 warn and 1 error
10 checks passed, 2 checks failed

So that user is clearly notified about something weird happening. For the checking hinting we could possibly use the same (or similar) approach with the result key to ignore/xfail checks when desired.

[happz]

My two cents: there can be plenty of different answers, and all could be correct for some users. Many places might evaluate check results, some we can control, some we would never be able to, therefore we're probably not supposed to decide everything:

• tmt core, i.e. what's in results.yaml - let's just save everything as it got out of execute plugin.

• in-tree report plugins: discuss and decide, then describe nicely in their docs how they work with check results. Do checks affect the whole result outcome? Would they be just a hint in the reports? Do all in-tree plugins have to have the same policy?

  How about plugins that are sort of pass-through like reportportal or polarion, or even junit, is it even up to tmt developers to decide how these plugins evaluate checks, if ever? For me, these plugins transport results somewhere else, and this "somewhere else" may have very different views on how to evaluate results depending on the day of the week and the team running it. Deciding the rules feels more like a job for teams using these plugins for their tests, tmt role would remain to provide tools like --strict or --ignore-all-check-failures-we-dont-care-about-them-at-all, leaving their usage on whoever would consume reports in RP.

• out of tree plugins and users, like Testing Farm - are out of our reach? Are tmt developers in a position to tell how TF should evaluate check results?

The best we could do, sort of a middle road, would be to provide as much info as possible, keeping results.yaml a solid source for consumers; we should define how display and html evaluate them, but the rest... For the rest, tmt is just the beginning, not the final resting place of results, I wouldn't touch them then.

The good thing is, we can work on implementation and stop at the boundary between the results.yaml and report plugins until we get to some sort of agreement. We would get tests, in a dormant state, and we could build upon that.

[psss]

Very nice summary! Completely agree.

[thrix]

You have also my agreement @happz on the above, looks like a reasonable step forward here

[martinhoyer]

I've briefly looked into Abrt. Quick summary:

abrt and python-abrt packages available on el-7, el-8, while on el9 it is not being included in rhel/centos/epel repos.
Ubuntu uses something called apport, that also comes with python api, apparently it's also using libreport?

python3-abrt

>>> import problem
>>> problem.list()
[<problem.Python3 (init.py:176:initialise:EOFError: EOF when reading a line)>, <problem.Python3 (__init__.py:399:from_name:importlib.metadata.PackageNotFoundError: No package metadata was found for targetcli)>, <problem.Ccpp (python3.12 killed by SIGSEGV)>, <problem.Ccpp (python3.12 killed by SIGSEGV)>, <problem.Ccpp (gimp-2.10 killed by SIGTRAP)>, <problem.Ccpp (python3.12 killed by SIGSEGV)>]

Ubuntu container:

>>> import problem_report
>>> pr = problem_report.ProblemReport()
>>> problem.items()
ItemsView({'ProblemType': 'Crash', 'Date': 'Tue Mar 12 11:27:21 2024'})

Will try to investigate further if there is any hope for interoperability.

[happz]

FTR, the eventual product does not have to be one or the other, we definitely can switch between implementations depending on guest.facts. We do have experience with a tiered approach to checks, dmesg report will depend on what capabilities the user has, for example. We may live happily with a check backed by different distribution-specific packages, and even turning to a no-op on unknown ones.