Conversation
8ba47ac to
055feb0
Compare
055feb0 to
fb73983
Compare
fb73983 to
feb81e8
Compare
The two test30-numa-disabled tests boot the node with a kernel which
has NUMA support compiled out, and boot it back at the end by calling
enable-numa as their last statement. A failure anywhere in between
never gets there: error exits the test, so the node keeps running the
kernel without NUMA. If we are running multiple tests, every test
after the failure sees a machine with a single node... and will fail.
Restore the kernel from an EXIT trap instead, the way the tests which
have state to put back already do it, and let enable-numa return
without doing anything if NUMA is enabled, so that calling it when
there is nothing to restore is free.
Looking into a failure of these tests is easier on a node which still
has NUMA disabled, so take keep_numa_disabled=1 to leave it that way:
keep_numa_disabled=1 ./run_tests.sh \
policies.test-suite/balloons/n4c16/test30-numa-disabled
Assisted-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
feb81e8 to
8b195da
Compare
| return 0 | ||
| ;; | ||
| esac | ||
| [ -d "$dir" ] || return 0 |
There was a problem hiding this comment.
Can we always expect $dir/$vm_coverage_report_dir to exist, too?
I may be paranoid, but deleting files from host makes me wish to use all the checks possible to make sure that we're operating only on a known directory structure. So I was thinking if here [ -d "$dir/$vm_coverage_report_dir" ] || return 0 would work, or would it be too strict?
There was a problem hiding this comment.
Narrowed it instead, matching both the filename and the directory holding it. In the discard itself and in report-coverage.sh's find. So stray covmeta.* elsewhere under ${2:-$(pwd)} (normally test/e2e) is now left alone.
Your paranoid extra test wouldn't have helped. That check passes on any repeat run, because coverage-report is there by then. So it would have deleted just as much. It only stops the first run, where there's nothing to delete anyway. And it would have made an interrupted run silent, as data left with no report would have caused reset_coverage=1 to quietly do nothing in those cases.
| reset_coverage=1 ./run_tests.sh policies.test-suite | ||
| ``` | ||
|
|
||
| `1`, `true` and `yes` all enable it. It discards everything collected earlier |
There was a problem hiding this comment.
On/off variables in the test framework (provision, vagrant_debug, use_host_images, expect_error,...) are usually 1=enabled, empty or 0=disabled. There's one nasty exception, though, with skip_long_tests being yes/no, and it is in the same level (run_tests.sh) as this one. So there is a tempatation to support yes/no here, too. But still...
I'd prefer limiting the range of accepted values in on/off variables rather than starting to allow new values to some of them. So maybe we could support only reset_coverage=1 or empty/0, and perhaps later switch the only exception (skip_long_lists) to support 1/0, too.
There was a problem hiding this comment.
I am consistently inconsistent and I think that is the primary reason why I usually aim for a liberal yes/1/true and no/0/false on FP2M and M2FP interfaces. But I understand your point and am fine with being strict instead and allowing only 1|0. I'll update the PR accordingly. We can file separate corrective PRs for the existing divergence(s).
|
|
||
| # Drop whatever the plugin covered while starting up, so that the coverage | ||
| # data we collect is about what the test does. | ||
| vm-coverage-clear |
There was a problem hiding this comment.
As tests quite often that call helm-launch with a test-specific configuration, it would be reasonable to count a plugin applying its initial configuration as something that is covered by the test. So I'm wondering if it would be ok to drop vm-coverage-clear from here?
And on the other hand, many tests do helm-terminate and helm-launch many times with different configurations. Does the current SIGTERM behavior ensure that coverage is collectable. In some earlier experiments SIGTERM was racy and a custom handler failed to do the finishing it was supposed to. Do you think that helm-terminate could be extended with coverage collection to ensure that what was covered from first plugin launches would not be lost?
There was a problem hiding this comment.
Yes, we could extend it. We have 2 methods of collecting coverage data: 1) let the runtime golang coverage infra dump collected data, and 2) retrieve the same data from an dedicated HTTP server endpoint running inside the plugin, started only if test APIs are enabled in the plugin. We could use the latter one here to always dump coverage data when we helm-terminate, so we wouldn't need to rely on the golang runtime getting around and successfully dumping everything. The only thing this requires is a working port redirection, which I think we anyway always have and already need in some cases.
There was a problem hiding this comment.
Dropped the clear from helm-launch too, as you suggested. helm install starts a fresh plugin, so its counters are zero anyway, and cross-test isolation comes from wiping the VM's coverage directory before
each test.
And we now dump coverage data when we helm-terminate a plugin.
| # Start the run from scratch if asked to. This is done once here, for all the | ||
| # tests, and never per test, as each test only ever overwrites the data of its | ||
| # own earlier run. | ||
| vm-coverage-discard-collected "${2:-$(pwd)}" |
There was a problem hiding this comment.
Could we give a name to $2, like we have
TESTS_DIR="$1"
There was a problem hiding this comment.
Good point. Named it: OUTPUT_DIR_ARG="${2:-}" and OUTPUT_ROOT="${OUTPUT_DIR_ARG:-$(pwd)}" next to TESTS_DIR="$1". So no bare $2 is left. Naming it also made it visible that $2 meant two things, which now should be fixed. It was also missing from the usage message, and that is now fixed, too.
Print a single pod in vm-wait-pod-regexp, and never one which is
terminating. Otherwise port forwarding got two names while one pod was
terminating and another starting, and kubectl refused:
error: TYPE/NAME and list of ports are required for port-forward
Assisted-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
Use ansible_facts to disable swap during provisioning. Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
Add a coverage package which serves the coverage data of a plugin built with go build -cover over the instrumentation HTTP server: the meta-data, a snapshot of the counters, and the ID of the binary tying the two together. The ID is the one the go runtime names the files it writes to $GOCOVERDIR with, dug out of the meta-data header, so that data served over HTTP and data dumped at exit merge as they are. This is how we get the coverage of a plugin which is still running, or which never gets to exit gracefully. Serve it from the resource manager, so from the nri-resource-policy-* plugins, and only with the test APIs enabled. runtime/coverage works in a binary built with -cover and never in a test binary, so the tests build a helper to check that what we serve is what go tool covdata expects. Assisted-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
COVER=1 builds the resource-manager-based plugins with coverage instrumentation. Instrument our own packages only, and with atomic counters: the plugins are concurrent, and clearing the counters at runtime does not work without them. Assisted-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
Collect the coverage data of the plugins the tests exercise, and report the coverage of each plugin and the total at the end of a run. The tests clear the counters when they launch a plugin and ask it for a dump once done. make e2e-tests builds the plugins with COVER=1 so that a run has something to collect; collecting never fails a test. go tool covdata percent cannot report per plugin, and prints a package which has no statements without a percentage and without a line break, running the next package into it, so calculate the numbers from the profile instead, weighted by statements the way go tool cover does. Assisted-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
8b195da to
61bba6f
Compare
|
Closing in favor of #792. |
2 participants
Make test/e2e/run_tests.sh collect go coverage data of the plugins the tests
exercise, and report per-plugin and total coverage at the end of a run.
make e2e-testsbuilds the resource-manager-based plugins withgo build -cover(make COVER=1 build images).instrumentation HTTP server, so a plugin which never exits gracefully is
covered too. What is served and what a plugin dumps at exit merge as they are.
HTML report and summary.json.
reset_coverage=1discards earlier data first.Verified on a full run of both suites: 55/55 PASS, balloons 78.6%,
topology-aware 68.0%, 59% of all instrumented packages.
The first commit is the standalone vm-wait-pod-regexp fix, needed for port
forwarding to the plugin; drop it if that lands first.