Improve `outcome-analysis.sh` script #8011

valeriosetti · 2023-07-31T13:16:47Z

outcome-analysis.sh scripts should be run on the same Docker instance that is used by the CI in order to have also tests included by ssl-opt script to be executed with the proper OpenSSL/GnuTLS versions.
Unfortunately that Docker instance have an old version of Git (2.7.4) which does not support a command used in this script:

git branch --show-current

This was added only in Git version 2.22.

As a consequence this PR:

proposes a "fall back" solution when an old Git version is used
speeds up the build time by parallelizing the work (not related to the issue above, but beneficial for testing purposes)

PR checklist

changelog not required as nothing changes from end user point of view
backport not required since this is a test enhancement
tests not required the PR itself modifies a test

The Docker container used for the CI has Git version 2.7.4 which does not support the "git branch --show-current" command since this was added in version 2.22. Therefore this commit adds an alternative version for old Git versions. Signed-off-by: Valerio Setti <[email protected]>

gilles-peskine-arm

Please don't impose make -j.

gilles-peskine-arm · 2023-07-31T14:02:01Z

docs/architecture/psa-migration/outcome-analysis.sh

@@ -34,7 +34,7 @@ record() {
    export MBEDTLS_TEST_OUTCOME_FILE="$PWD/outcome-$1.csv"
    rm -f $MBEDTLS_TEST_OUTCOME_FILE

-    make check
+    make -j check


Please don't use make -j in CI scripts. That does unbridled parallelism which creates load spikes and seems to decrease CI performance overall. Even where load isn't a problem and the sole goal is processing time for this single build, increasing parallelism has diminishing returns: -j2 is significantly faster than single threading, -jNUMBER_OF_PROCESSORS is a little faster than -j2, going beyond that is pretty much no win.

We set MAKEFLAGS to a reasonable -j in our CI environments, so it's best to call make without an explicit -j in scripts.

Ok, understood. Thanks for the explanation ;)

gilles-peskine-arm · 2023-07-31T14:03:52Z

docs/architecture/psa-migration/outcome-analysis.sh

+if dpkg --compare-versions "$GIT_VERSION" "gt" "2.22.0"; then
+    HEAD=$(git branch --show-current)
+else
+    HEAD=$(git rev-parse --abbrev-ref HEAD)


Why not always use this command? Is there a case where git branch --show-current is better in some non-critical way?

We're planning to move most of our CI from Ubuntu 16.04 to 22.04 in the next quarter. We can move outcome analysis sooner if there's a strong benefit.

I think it makes for slightly more understandable code if you can see that git rev-parse --abbrev-ref HEAD is equivalent to git branch --show-current for older versions of git. I did not personally know that before reviewing this PR for example. I think it is ok to leave it as is.

A comment explaining what the command do would serve the same purpose and would reduce the complexity of the code.

The version check has added a dependency on dpkg, so it will fail for developers who work on non-Debian-based systems (e.g. Fedora, macOS, ...). Also, it's wrong if someone has installed a more recent git in their PATH.

The version check has added a dependency on dpkg, so it will fail for developers who work on non-Debian-based systems (e.g. Fedora, macOS, ...). Also, it's wrong if someone has installed a more recent git in their PATH

Ops, sorry, I didn't thought about these limitations. I will modify it as suggested

A comment explaining what the command do would serve the same purpose and would reduce the complexity of the code.

The version check has added a dependency on dpkg, so it will fail for developers who work on non-Debian-based systems (e.g. Fedora, macOS, ...). Also, it's wrong if someone has installed a more recent git in their PATH.

Yep, fair enough on the dpkg part, I agree.

tom-daubney-arm

I agree about the make -j comment so once that is changed I will approve. Thanks

Signed-off-by: Valerio Setti <[email protected]>

tom-daubney-arm

LGTM - Pending CI. Will approve now but mark with "needs-ci" until CI completes just in case of an unforeseen issue.

gilles-peskine-arm

LGTM

gilles-peskine-arm · 2023-07-31T15:37:31Z

Incidentally, I don't understand why you need this patch. (But it's a good patch anyway, it makes the script a bit more robust in case we turn out to want it.) We currently don't run outcome-analysis.sh on the CI. If we start running it, we can pick Ubuntu 22.04 as the platform to run it, with git 2.34.

valeriosetti · 2023-07-31T17:09:32Z

We currently don't run outcome-analysis.sh on the CI

Yes, correct, but when working on the ECC/BIGNUM EPICs I used 2 scripts:

analyze_outcomes.py
- which verifies driver coverage between a reference component (without driver) and its accelerated counterpart
- this is part of the CI
outcome_analysis.sh
- which verifies that the current PR does not skip tests that were previously done in the past
- this is not part of the CI

I don't know if it's worth to add also the 2nd script to the CI since:

it takes a lot of time to execute (especially when ssl-opt tests are included)
maybe it's more specific for my activity. Perhaps most of the PRs of the mbedTLS library don't need this kind of test

However the combination of the 2 scripts was/is useful for my activity in order to ensure that I don't break some support from the past.

mpg · 2023-08-01T08:13:37Z

To add to what Valerio said, I'm not sure outcome_analysis.sh (sorry for the name, way too similar to analyze_outcomes.py, I didn't realise) is a good fit for running in the CI:

it would have false positives every time we modify the name of an existing test;
it has little value for most PRs - we mainly find it useful for PRs that change a lot of dependency declarations in tests, but that's not the case of a lot of PRs. And IMO it will have even less value once Check that all test cases are executed at least once #2691 will be done - which is IMO so much more desirable to have in the CI.

valeriosetti added needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review priority-high High priority - will be reviewed soon labels Jul 31, 2023

valeriosetti requested a review from mpg July 31, 2023 13:16

valeriosetti self-assigned this Jul 31, 2023

valeriosetti requested a review from tom-daubney-arm July 31, 2023 13:24

gilles-peskine-arm requested changes Jul 31, 2023

View reviewed changes

tom-daubney-arm removed the needs-reviewer This PR needs someone to pick it up for review label Jul 31, 2023

tom-daubney-arm reviewed Jul 31, 2023

View reviewed changes

test: use only rev-parse for getting the current branch

ab02d39

Signed-off-by: Valerio Setti <[email protected]>

valeriosetti force-pushed the improve-outcome-analysis branch from c030ace to ab02d39 Compare July 31, 2023 14:48

valeriosetti requested a review from tom-daubney-arm July 31, 2023 14:48

tom-daubney-arm approved these changes Jul 31, 2023

View reviewed changes

tom-daubney-arm added the needs-ci Needs to pass CI tests label Jul 31, 2023

gilles-peskine-arm approved these changes Jul 31, 2023

View reviewed changes

gilles-peskine-arm added approved Design and code approved - may be waiting for CI or backports size-xs Estimated task size: extra small (a few hours at most) and removed needs-review Every commit must be reviewed by at least two team members, labels Jul 31, 2023

mpg added this pull request to the merge queue Aug 1, 2023

tom-daubney-arm removed the needs-ci Needs to pass CI tests label Aug 1, 2023

Merged via the queue into Mbed-TLS:development with commit cbc495e Aug 1, 2023

valeriosetti mentioned this pull request Aug 1, 2023

TLS: Clean up ECDSA dependencies #7921

Merged

3 tasks

valeriosetti deleted the improve-outcome-analysis branch December 6, 2024 06:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve `outcome-analysis.sh` script #8011

Improve `outcome-analysis.sh` script #8011

valeriosetti commented Jul 31, 2023 •

edited by minosgalanakis

Loading

gilles-peskine-arm left a comment

gilles-peskine-arm Jul 31, 2023

valeriosetti Jul 31, 2023

gilles-peskine-arm Jul 31, 2023

tom-daubney-arm Jul 31, 2023

gilles-peskine-arm Jul 31, 2023

valeriosetti Jul 31, 2023

tom-daubney-arm Jul 31, 2023

tom-daubney-arm left a comment

tom-daubney-arm left a comment

gilles-peskine-arm left a comment

gilles-peskine-arm commented Jul 31, 2023

valeriosetti commented Jul 31, 2023

mpg commented Aug 1, 2023

Improve outcome-analysis.sh script #8011

Improve outcome-analysis.sh script #8011

Conversation

valeriosetti commented Jul 31, 2023 • edited by minosgalanakis Loading

PR checklist

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

gilles-peskine-arm Jul 31, 2023

Choose a reason for hiding this comment

valeriosetti Jul 31, 2023

Choose a reason for hiding this comment

gilles-peskine-arm Jul 31, 2023

Choose a reason for hiding this comment

tom-daubney-arm Jul 31, 2023

Choose a reason for hiding this comment

gilles-peskine-arm Jul 31, 2023

Choose a reason for hiding this comment

valeriosetti Jul 31, 2023

Choose a reason for hiding this comment

tom-daubney-arm Jul 31, 2023

Choose a reason for hiding this comment

tom-daubney-arm left a comment

Choose a reason for hiding this comment

tom-daubney-arm left a comment

Choose a reason for hiding this comment

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

gilles-peskine-arm commented Jul 31, 2023

valeriosetti commented Jul 31, 2023

mpg commented Aug 1, 2023

Improve `outcome-analysis.sh` script #8011

Improve `outcome-analysis.sh` script #8011

valeriosetti commented Jul 31, 2023 •

edited by minosgalanakis

Loading