Add additional test checks

[thrix]

This PR introduces additional test checks that make sense
to run while runing the tests.

The checks are by default controlled on the test level,
but can be overridden in plans.

Signed-off-by: Miroslav Vadkerti [email protected]

[t184256]

I feel like AVC and test-inspector represent two distinct classes of checks, actually. Checking for AVCs is a property of the test, and the decision to either do that manually or default to making the execution environment do that relies on the test itself. test-inspector is more of a runtime choice. I mean, yeah, enabling this in the test can mean "it's checked to be non-destructive and I don't want to regress on that", but I'd say we need a way to enable-disable checks at runtime too.

[thrix]

I feel like AVC and test-inspector represent two distinct classes of checks, actually. Checking for AVCs is a property of the test, and the decision to either do that manually or default to making the execution environment do that relies on the test itself. test-inspector is more of a runtime choice. I mean, yeah, enabling this in the test can mean "it's checked to be non-destructive and I don't want to regress on that", but I'd say we need a way to enable-disable checks at runtime too.

hmm I wonder, isn't that runtime disabling then more a details of the check itself? is it something we need to care here?

[t184256]

I hope we'll be fine with non-parametrized checks and boolean enable/disable for a long time.

[t184256]

Looks fine, would benefit from a command-line override example.

[psss]

I am not completely sure about the granularity. Is it really necessary to track check per individual test case? Especially for the test-inspector this does not make sense to me. I'd say you either want to enable it for the whole plan or completely disable.

@milosmalik, what should be the granularity for the AVC check opt-out feature for your use case? Would it be enough to enable/disable the AVC check for the whole set of tests selected in a plan (L2 metadata) or would it be useful to be able to enable/disable checks per individual test case (L1 metadata)?

[milosmalik]

This option is ideal for our purposes:

• enable/disable AVC checks per individual test case (L1 metadata)

The following option is too broad for our purposes:

• enable/disable the AVC check for the whole set of tests selected in a plan (L2 metadata)

[thrix]

@psss ok seems so we need it in L1, at least for avc

[psss]

I see, thanks for the feedback, @milosmalik. So it seems for the selinux team there would have to be a new L1 attribute. Just want to make sure we do not introduce a new field for a single team and for a single check: Are there any other check examples to support the use case? Are there other teams or components for which it would make sense to enable additional checks per test case?

[kkaarreell]

I would argue for a different word with more clear semantics. 'check' sounds too unclear to me.
I don't have truly better proposal though. :-( runtime_test, runtime_subtest, runtime_check? No idea.

[thrix]

So after discussion, seems we will drop this idea because:

1. there is not other example here than 'avc' ...

2. keep it similar to what we have now in downstream, control 'avc' via an environment variable

--taskparam='AVC_ERROR=+no_avc_check'

[psss]

I would wait a bit, just to make sure more similar use cases do not pop up from other team members.

[martinky82]

Hi, introducing ABRT check would be nice, too. It could work simply by checking, if there's anyting present in /var/spool/abrt and/or by grepping specific patterns in syslog (journal, /var/log/messages) and fail if there's anything found. For proper functionality it would of course need to have ABRT up and running, but since it is preferrable to run jobs with --abrt to have crashes reported to QE FAF, should an arise, it shouldn't be a problem. If ABRT is not installed, the check would do nothing and simply pass.

[thrix]

Hi, introducing ABRT check would be nice, too. It could work simply by checking, if there's anyting present in /var/spool/abrt and/or by grepping specific patterns in syslog (journal, /var/log/messages) and fail if there's anything found. For proper functionality it would of course need to have ABRT up and running, but since it is preferable to run jobs with --abrt to have crashes reported to QE FAF, should an arise, it shouldn't be a problem. If ABRT is not installed, the check would do nothing and simply pass.

@martinky82 --abrt where? if we would use the same approach in TMT/FMF world, we would need to explicitly enable and start abrt in prepare phase preferably. Maybe abrt check then could be part of report phase instead @psss ?

[martinky82]

@martinky82 --abrt where? if we would use the same approach in TMT/FMF world, we would need to explicitly enable and start abrt in prepare phase preferably. Maybe abrt check then could be part of report phase instead @psss ?

In wow command line. What it does is that it puts enable-abrt task at the beginning and check-abrt task at the end of a job (enable-abrt being much more important in our workflow).

[martinky82]

Here's how it works:

1. enable-abrt changes abrt configuration and starts abrtd + friends (this could be done in prepare phase)
2. every crash gets reported to FAF server immediately
3. check-abrt in the end checks if there is anything in /var/spool/abrt and does rlPass / rlFail accordingly (this could be done in report phase).

[psss]

This has been stalled for too long. I believe this will need some more discussion before moving forward. Also the proposed key name checks should be singular to be consistent with other test keys and the content seems much more naturally to be a list rather then a string. Resetting the reviews and marking for discussion.

[packit-as-a-service]

Failed to load packit config file:

Cannot parse package config: ValidationError({'jobs': {0: {'current_version_command': ['Unknown field.']}}, 'current_version_command': ['Unknown field.']}).

For more info, please check out the documentation or contact the Packit team.

[happz]

Other related issues that appeared over time: #1235 #1523 #1464

[packit-as-a-service]

Failed to load packit config file:

Cannot parse package config: ValidationError({'jobs': {0: {'current_version_command': ['Unknown field.']}}, 'current_version_command': ['Unknown field.']}).

For more info, please check out the documentation or contact the Packit team.

[thrix]

@happz @psss I updated it to the latest code, and left out avc as the default check. I believe the default AVC check enabled should be part of the planned profile, rather than a global default.

I am, although, SUPER confused why validation does not fail here :(

[kkaarreell]

Where is the actual AVC chech implemented? I do not see such a code in this PR.

[psss]

This was just a kick-off of the specification.

[Koncpa]

After yesterday's discussion, we agreed that the proposed solution should be more generic and a framework should be created in tmt where the user can use a prepared module or add their own to check various information on the system (AVC, out-of-memory, kernel-panic, logs) .

Probably check will be applied after every test, but was also proposed check after each beakerLib phase or after run of the whole test plan.

[happz]

I'm not sure how changes in this file are related to adding the check L1 key.

[thrix]

not sure now, feel free to drop it @happz

[happz]

I'd start hacking on this because it's a feature we'd have use for in downstream, but I'd rather wait till #1679 gets resolved & its components merged. Supporting custom checks, together with custom export plugins, sounds like we do need to generalize plugin loading a bit - it was handling step plugins only until very recently, here already a possible third type of plugin spawned, let's make it a cleaner and less step-ish. I can imagine having a shell-like plugin, making decisions based on a script exit code as well as a plugin built out of hundreds of Python lines, and these need to register somehow, just like step and now the export plugins do.

[thrix]

@happz thanks, I changed the assignee to you

[martinhoyer]

Can I somehow help to move this forward?
Have there been any discussions about the subject since February, apart from in #1875 ?

[happz]

Can I somehow help to move this forward?

If there’s a tmt meeting tomorrow and you’d be able to join, please, remind the developers of the discussion mentioned below.

Have there been any discussions about the subject since February, apart from in #1875 ?

None that I know if. I’m waiting for literally anyone to comment and help me address the blank spots.

[psss]

@happz, I guess this one has been obsoleted by #2210, right?

[happz]

@happz, I guess this one has been obsoleted by #2210, right?

@psss I think so, yes. There are few lines here and there #2210 does not add, namely the plan-level checks, but I avoided those on purpose. I will raise the topic of plan-level test checks in the #1875, and file issues as needed.