Add "-enable-monitoring" option to GPU plugin + testing for its options

[eero-t]

Commits in this PR do following changes for the GPU plugin:

• Disable "i915_monitoring" resource by default, and add "-enable-monitoring" option so that this resource (exposing all node GPUs) can be enabled only when needed
• Move options to their own struct, so their use is more explicit (no magic values) and they can be more easily passed around
• Add testing for the new option, and also for the device sharing option that was missing testing support

---

These changes were dropped from latest versions of the PR:

• WIP/untested operator support for the new option (I will test it properly after it's clear whether rest of the changes are going to be accepted)
• Because option combinations change how GPU plugin should interpret the test environment, they and the expected resulting values are an additional testing dimension. Therefore, each test-case can have an array of them, and there's an additional loop going through these combinations

Alternatives for the last item are:

• Having the option combinations array separate from sysfs/devfs setup array. Advantage of this is the best testing coverage (all options combos are tested with all test setups) with least amount of lines, but it requires few additional ifs to the test loop. When this was discussed in Gpu plugin testing improvements #615, people seemed to prefer slightly the approach I've taken now, but I'm fine with either of them. Example of how this could look (with additional options) is here: eero-t@1901af8
• Flattening the 2-dimensional test-case data to 1 dimension. This avoids the need for additional loop, but has the downside of needing larger amount of test-case duplication to provide as good coverage

[mythi]

This avoids the need for additional loop, but has the huge downside of needing large amount of test-case duplication, so I'm not going do it

I don't see how "needing large amount of test-case duplication" can be justified. Since monitor count is independent from how many devices there are, it can be tested separately:

name: "two devices, enable monitoring"

Further, since expectedDevs is defined by a scalar multiplier sharedDevNum only one check for it is sufficient:

name: "two devices, sharedDevNum=4"

(or how testing 2 and 4 makes math different?)

Finally, what could be added still is the check that if there are no devices, also expectedMonitors gives zero even when monitoring is enabled:

name: "no device installed, enable monitoring"

[eero-t]

I don't see how "needing large amount of test-case duplication" can be justified. Since monitor count is independent from how many devices there are, it can be tested separately

Current expectation is that options are independent. Combining the options is testing for that.

But combined options testing works also if a dependent option like this is added later on: eero-t@3fb02c0

Further, since expectedDevs is defined by a scalar multiplier sharedDevNum only one check for it is sufficient:
...
(or how testing 2 and 4 makes math different?)

I can think of some (contrived) code changes after which two out of 1, 2 & 4 cases would pass, but not all. While such changes might not pass manual review, I think it's good if unit tests catch them before one considers sending changes for review.

But I'll change 2 & 4 to a single, larger prime number, that should catch even those contrived examples well enough. :-)

In general, I think it's good practice to add few guards also against "impossible" happening, because unit tests aren't supposed to test only current code, but detect also when (potentially much) later changes break expectations in (supposedly) unrelated earlier functionality.

Finally, what could be added still is the check that if there are no devices, also expectedMonitors gives zero even when monitoring is enabled:

name: "no device installed, enable monitoring"

Good point, testing that is definitely needed! I'll add options to test that for one of the no-dev cases.

[eero-t]

Did the changes mentioned above and pushed rebased version. That dang unrelated "opae-nlb-demo" was broken again.

Test update commit message lists what testing it is supposed to add: eero-t@11609a3

[mythi]

Combining the options is testing for that.

my feedback is that we should drop []optionsCheck looping but others can decide

[uniemimu]

The cmd-line flag itself looks ok. I do not have an opinion about testing.

[eero-t]

After discussing with Ukri about windmills, I replaced []optionsCheck looping[1] with minimal testing adding just "cliOptions" to test-cases: eero-t@6cec11e

(CI failed as "opae-nlb-demo" breakage seems now to be permanent.)

[1] it moved to "gpu-plugin-proper-option-testing" branch from where suitable bits can be resurrected after GPU plugin test code has acquired more sysfs/devfs setups it needs to test, and more options they need to be tested against.

[rojkov]

Looks good to me. Added a couple of nits.

[eero-t]

Besides getting ClearLinux testing fixed for CI, only thing still missing before this could be merged, is testing the operator support (last, trivial WIP commit).

Do CI checks include any kind of operator testing, or is that something that needs to be done completely manually?

According to Ukri, testing operator support is a major undertaking / pain (especially when one does it for the first time), so I was wondering could it be dropped from this PR, or would somebody more familiar with it be willing to review / test the commit that tries to add operator support? :-)

[mythi]

so I was wondering could it be dropped from this PR,

It's OK to drop from this PR.

[eero-t]

Dropped the WIP/operator commit and updated PR description to reflect current state, so that what gets in will be properly documented.

[eero-t]

I fixed the conflicts with main updates, but the result with previous PR changes to GPU plugin scan() function pushed it over the CI lint complexity limit => I added commit moving GPU device sysfs compatibility checks to its own function.

Do you want a separate PR for that?

[mythi]

no need to split to another PR

[uniemimu]

lgtm