CI: Add scripts to run samples automatically #829

AntonioND · 2020-09-15T14:14:03Z

The scripts are a mix of bash and a bit of GNU expect for flexibility, as each sample needs to be tested in slightly different ways.

Each sample must have a test.sh script in its folder so that the CI detects it and runs it. They are executed by run_sample.sh script, created using run_test.sh as an example.

This PR adds several samples to the CI, but not all of them:

The ml folder samples, for example, take far too long to be part of a regular CI run, and need caching of docker images, which is a task on its own.
The openmp and nodejs samples seem to be broken.

This PR also fixes the following samples:

languages/dotnet: The following error happened when trying to run it:

FailFast:
Couldn't find a valid ICU package installed on the system. Set the configuration flag
System.Globalization.Invariant to true if you want to run with no globalization support.

By adding a new file with this configuration flag set to true, the sample works again. dotnet/core#2186 (comment)

basic/helloworld: This PR also simplifies the sample so that it matches the helloworld test.
languages/java: When the number of ethreads is 1 the java app hangs after printing the message. With a higher number of ethreads it exits as expected.

This is a partial fix for #231, and is intended to make #332 easier by adding this new bash/expect framework.

AntonioND · 2020-09-15T16:26:37Z

I need to make this work in the CI, it looks like the build output isn't saved where I expected it to be.

AntonioND · 2020-09-16T15:05:29Z

So now the 8 samples I've added to the CI pass (the CI failures are not this PR's fault).

They also pass in my VM (with run-hw and run-sw):

run_mode=run-sw \
SGXLKL_ETHREADS=1 \
SGXLKL_BUILD_MODE=debug \
SGXLKL_ROOT=~/sgx-lkl \
.azure-pipelines/scripts/test_runner.sh samples

SeanTAllen

Would it be reasonable to say based on the number of things with 20 minute time outs that if 1 or 2 or these hang that not all of them will be run before the CI run is terminated?

samples/basic/attack/read_memory.sh

samples/containers/encrypted/test.sh

samples/containers/redis/README.md

samples/languages/dotnet/README.md

samples/languages/java/README.md

samples/languages/java/test.sh

SeanTAllen · 2020-09-16T15:45:37Z

@AntonioND rather than using expect, would it be reasonable to merely check the output value like we do with existing tests?

AntonioND · 2020-09-16T16:01:11Z

@AntonioND rather than using expect, would it be reasonable to merely check the output value like we do with existing tests?

That's not possible in all cases. For example, the language/java sample doesn't exit after it's done printing the demo message, I have to wait until it's printed and then send a "^C" for the test to finish. For the basic/attack sample you have to wait until the initial process is running and it prints "I'm ready to be attacked", attack it from a second process, and send a "\n" to the initial one to end. I tried doing this by having a countdown in the initial process and launching the second one at the right time, but it wasn't reliable at all. It is pretty reliable with expect.

EDIT: Maybe that behaviour of the java sample is a bug that could be added to the issue I'm going to create to fix the other samples...

SeanTAllen · 2020-09-16T16:06:08Z

It is pretty reliable with expect.

That sounds like it is flakey. Are you saying it should be expected to fail sometimes with expect?

AntonioND · 2020-09-16T16:15:46Z

That sounds like it is flakey. Are you saying it should be expected to fail sometimes with expect?

I can't be 100% sure it is always going to work, specially after seeing that some samples (like the java one) don't exit when I would expect them to.

Regarding the ones I'm testing with expect, I've never seen them fail so far. Of course, if someone changes the text output of the demo code the expect script will break, that's what I was thinking about when I wrote "pretty reliable".

In general I agree that expect is not a nice thing to use, but in the case of basic/attack I don't know how to do it in a different way, and in the case of languages/java, assuming that the hangup is not a bug (I don't know the expected behaviour after printing the test output), I don't know how to make the test end either.

AntonioND · 2020-09-16T16:17:00Z

Would it be reasonable to say based on the number of things with 20 minute time outs that if 1 or 2 or these hang that not all of them will be run before the CI run is terminated?

I've also fixed this, now there is only one sample that has that timeout. It's not needed when I run the CI, but it is needed when I run the test in my VM.

AntonioND · 2020-09-17T09:16:49Z

I've left a note in the languages/java sample that mentions that the sample isn't supposed to need expect scripts, and to remove them when the issue is fixed. I also mention in the comment the GitHub issue that needs to be fixed.

wintersteiger

I agree, it would be nice if we could get of expect, but I don't have a good alternative solution either.

samples/basic/helloworld/enclave_config.json

When trying to run the dotnet sample the following error happened: ``` FailFast: Couldn't find a valid ICU package installed on the system. Set the configuration flag System.Globalization.Invariant to true if you want to run with no globalization support. ``` By adding this file, the configuration flag mentioned in the error message is set to true, and the sample works again. [1] [1] dotnet/core#2186 (comment)

AntonioND · 2020-09-18T09:15:02Z

So @prp mentioned to me that the java sample is failing because the number of ethreads was 1. I've increased the number of ethreads in that sample and now it's working fine and it doesn't need expect scripts. However, the basic/attack one still needs them (but it's the only sample that does).

SeanTAllen · 2020-09-21T11:59:21Z

LGTM other than the CI failures which I believe are unrelated.

vtikoo

LGTM

hukoyu

@AntonioND overall looks good. I provided some feedback. After addressing them this PR is good to be merged.

.azure-pipelines/scripts/run_sample.sh

hukoyu · 2020-09-22T17:02:52Z

samples/basic/helloworld/test.sh

+    make "$run_mode"
+elif [[ "$test_mode" == "gettimeout" ]]; then
+    # Default
+    exit 1


This is 1 second and it is too short to be default value. May be 60 seconds is better.

No, this means "default". This is how it works in the case of the Makefiles of the tests. If there is no target called gettimeout, make will return 1. If the target is there, it prints the timeout length.

Take a look at tests/ltp/batch.mk, target gettimeout, for example.

samples/common.mk

hukoyu · 2020-09-22T17:10:04Z

samples/common.sh

+samples_dir=$(dirname $(realpath "$BASH_SOURCE"))
+SGXLKL_ROOT=$(realpath "${samples_dir}/..")
+
+if [[ -z "${SGXLKL_PREFIX}" ]]; then


These are already exported in Makefile. Do we need to export again? Also SGXLKL_SETUP_TOOL exists in common.sh and not in common.mk just FYI

Some samples need this in the bash script because not all of the commands are run in the Makefile. For example, SGXLKL_SETUP_TOOL is needed in containers/redis/test.sh. A couple of them are needed in languages/python/test.sh.

I only added SGXLKL_SETUP_TOOL to the bash script because it's not needed in the Makefiles.

samples/containers/redis/run-redis-client.sh

samples/containers/redis/test.sh

samples/languages/dotnet/test.sh

samples/languages/java/test.sh

samples/languages/python/test.sh

The helloworld demo used a really complicated system to build the elf file on the host and then transfer it to the docker image. It's better to simply build it inside docker, as the test does. This commit copies Dockerfile, Makefile and helloworld.c from the test to the sample. The Makefile has been modified to still use enclave_config.json (the test wasn't using it).

When the number of ethreads is 1 the java app hangs after printing the message. With a higher number of ethreads it exits as expected.

The scripts are a mix of bash and a bit of GNU expect for flexibility, as each sample needs to be tested in slightly different ways. Each sample must have a test.sh script in its folder so that the CI detects it and runs it. They are executed by run_sample.sh script, created using run_test.sh as an example. This commit adds several samples to the CI, but not all of them: - The ml folder samples, for example, take far too long to be part of a regular CI run, and need caching of docker images, which is a task on its own. - The openmp and nodejs samples seem to be broken.

AntonioND · 2020-09-23T12:11:05Z

I'm going to merge this PR now after addressing the last few comments. I'm still going to be working on the samples for a while, so I'm happy to push a PR with more changes if needed.

AntonioND force-pushed the anninodi/samples branch 2 times, most recently from 31eb4b8 to 31143f5 Compare September 15, 2020 14:30

AntonioND force-pushed the anninodi/samples branch 7 times, most recently from 7c674b3 to 04e98b9 Compare September 16, 2020 14:06

AntonioND requested review from davidchisnall, wintersteiger, letmaik, prp and SeanTAllen September 16, 2020 15:06

AntonioND force-pushed the anninodi/samples branch from 04e98b9 to 7364b21 Compare September 16, 2020 15:09

SeanTAllen reviewed Sep 16, 2020

View reviewed changes

AntonioND force-pushed the anninodi/samples branch from 7364b21 to 98679ef Compare September 16, 2020 16:16

AntonioND force-pushed the anninodi/samples branch 3 times, most recently from 4d9e471 to 1cb8976 Compare September 17, 2020 09:10

wintersteiger approved these changes Sep 17, 2020

View reviewed changes

samples/basic/helloworld/enclave_config.json Outdated Show resolved Hide resolved

AntonioND force-pushed the anninodi/samples branch from 1cb8976 to d0933c0 Compare September 17, 2020 15:51

AntonioND requested a review from vtikoo September 17, 2020 15:52

AntonioND force-pushed the anninodi/samples branch from d0933c0 to dcc3394 Compare September 18, 2020 09:07

AntonioND force-pushed the anninodi/samples branch from dcc3394 to d891c94 Compare September 18, 2020 09:13

AntonioND mentioned this pull request Sep 18, 2020

Fix broken samples #831

Closed

AntonioND requested a review from paulcallen September 21, 2020 09:08

vtikoo requested a review from hukoyu September 21, 2020 14:54

vtikoo approved these changes Sep 21, 2020

View reviewed changes

hukoyu approved these changes Sep 22, 2020

View reviewed changes

AntonioND added 3 commits September 23, 2020 09:53

samples: Fix java sample

46430b4

When the number of ethreads is 1 the java app hangs after printing the message. With a higher number of ethreads it exits as expected.

AntonioND force-pushed the anninodi/samples branch 2 times, most recently from fed1ece to 570c78a Compare September 23, 2020 09:58

AntonioND merged commit f8f09a3 into oe_port Sep 23, 2020

AntonioND deleted the anninodi/samples branch September 23, 2020 12:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CI: Add scripts to run samples automatically #829

CI: Add scripts to run samples automatically #829

AntonioND commented Sep 15, 2020 •

edited

Loading

AntonioND commented Sep 15, 2020

AntonioND commented Sep 16, 2020

SeanTAllen left a comment

SeanTAllen commented Sep 16, 2020

AntonioND commented Sep 16, 2020 •

edited

Loading

SeanTAllen commented Sep 16, 2020

AntonioND commented Sep 16, 2020

AntonioND commented Sep 16, 2020

AntonioND commented Sep 17, 2020

wintersteiger left a comment

AntonioND commented Sep 18, 2020

SeanTAllen commented Sep 21, 2020

vtikoo left a comment

hukoyu left a comment

hukoyu Sep 22, 2020

AntonioND Sep 23, 2020 •

edited

Loading

hukoyu Sep 22, 2020

AntonioND Sep 23, 2020

AntonioND commented Sep 23, 2020

CI: Add scripts to run samples automatically #829

CI: Add scripts to run samples automatically #829

Conversation

AntonioND commented Sep 15, 2020 • edited Loading

AntonioND commented Sep 15, 2020

AntonioND commented Sep 16, 2020

SeanTAllen left a comment

Choose a reason for hiding this comment

SeanTAllen commented Sep 16, 2020

AntonioND commented Sep 16, 2020 • edited Loading

SeanTAllen commented Sep 16, 2020

AntonioND commented Sep 16, 2020

AntonioND commented Sep 16, 2020

AntonioND commented Sep 17, 2020

wintersteiger left a comment

Choose a reason for hiding this comment

AntonioND commented Sep 18, 2020

SeanTAllen commented Sep 21, 2020

vtikoo left a comment

Choose a reason for hiding this comment

hukoyu left a comment

Choose a reason for hiding this comment

hukoyu Sep 22, 2020

Choose a reason for hiding this comment

AntonioND Sep 23, 2020 • edited Loading

Choose a reason for hiding this comment

hukoyu Sep 22, 2020

Choose a reason for hiding this comment

AntonioND Sep 23, 2020

Choose a reason for hiding this comment

AntonioND commented Sep 23, 2020

AntonioND commented Sep 15, 2020 •

edited

Loading

AntonioND commented Sep 16, 2020 •

edited

Loading

AntonioND Sep 23, 2020 •

edited

Loading