ci(tests): split container/compile-build and itests run steps in ci

[maxcao13]

Fixes #1108

[maxcao13]

This is the ci failure and it seems like podman isn't loading the cryostat.tar uploaded artifact correctly, and I'm not sure why. https://github.com/cryostatio/cryostat/actions/runs/3292962616/jobs/5429251837#step:3:11

[andrewazores]

GitHub Artifacts automatically zips files for storage and serves them back to you as a .zip when you try to download them back - take a look at the workflow run summary and scroll to the bottom:

https://github.com/cryostatio/cryostat/actions/runs/3292962616

if you click the cryostat artifact it will download it as a zip file. I bet the same thing is happening to the download-artifacts action, so you might need to run an additional step to unzip the archive before loading it into podman. ie the file you've downloaded is actually a cryostat.tar.zip.

[andrewazores]

Or maybe I'm wrong and this action automatically unzips: actions/download-artifact#143

still worth checking, I guess.

There's also a newer v3 of that action that should probably be preferred.

[andrewazores]

I don't think it's causing any problem here, but also:

[INFO] Built image to Docker daemon as quay.io/cryostat/cryostat, quay.io/cryostat/cryostat:2.3.0-snapshot

There's probably an implicit :latest on that first tag, but maybe using the fully-qualified name like podman save quay.io/cryostat/cryostat would be a good idea?

[andrewazores]

It may also be worth trying to use the action cache instead of sharing the container image between steps as an artifact. I think that should work too, and it might be faster as well as not creating a long-lived (90 days default) permalink to the intermediate container image. But let's get this flow working first with the artifact approach, and then try simplifying it to use the cache later.

[maxcao13]

Doesn't look like its getting zipped twice but this looks interesting: https://github.com/cryostatio/cryostat/actions/runs/3293293359/jobs/5429719411#step:4:11 Weird that it just looks like it's not actually downloading any data.

[andrewazores]

Looks like the artifact is still downloaded to the filename cryostat.tar, but then the unzip step is looking for cryostat.tar.zip. I think the download-action just needs to be updated to append .zip, if that's really what's happening.

[andrewazores]

Doesn't look like its getting zipped twice but this looks interesting: https://github.com/cryostatio/cryostat/actions/runs/3293293359/jobs/5429719411#step:4:11 Weird that it just looks like it's not actually downloading any data.

Oh yea, shoot...

actions/upload-artifact#181

Okay, new plan I guess, try using the cache now instead of an artifact :-/

[maxcao13]

Looks like the artifact is still downloaded to the filename cryostat.tar, but then the unzip step is looking for cryostat.tar.zip. I think the download-action just needs to be updated to append .zip, if that's really what's happening.

Well, I specified podman to output to cryostat.tar like
podman save -o cryostat.tar --format oci-archive quay.io/cryostat/cryostat
and later now
podman load -i cryostat.tar
and it works on my machine, which is why I am confused. Must be something with the actions/upload and download that is causing this.

The unzip -qq etc.. was also my commands and I think there was just no such files. I guess the cache method is the way to go then...

[andrewazores]

Wait, GitHub says you should be able to use artifacts between jobs within a workflow:

https://docs.github.com/en/actions/using-workflows/storing-workflow-data-as-artifacts

[andrewazores]

I think the artifacts route really should work:

https://docs.github.com/en/actions/using-workflows/storing-workflow-data-as-artifacts#passing-data-between-jobs-in-a-workflow

so maybe don't try to rip it out in favour of the cache just yet. If we keep it on artifacts then that also makes #1133 easier, or at least manually downloading the images to test is a possibility too which would still be neat.

[maxcao13]

I believe it should work now, but I still have a few questions.

1. What is the reason for having fetch-depth: 0 when checking out cryostat? Can that be removed when we have runners run spotless, spotbugs, shellcheck?
2. There is a dependency on xpath from the repeated-integration-tests script and it takes about 30 seconds to install, I'm not sure if that's great or not. If it's not, is there an easier way to get the POD_NAME and CONTAINER_NAME without using xpath, e.g. maybe just passing the variables directly and updating the repeated-integration-tests script?

if [ -z "${POD_NAME}" ]; then
    POD_NAME = etc.
fi

Or maybe just use mvn directly with all the correct flags and such?

3. I would think that the skjolber/maven-cache github action would cache the dependencies needed for the mvn lifecycle but it seems like plugin dependecies are downloaded regardless.
   e.g. https://github.com/cryostatio/cryostat/actions/runs/3294011272/jobs/5431161849#step:8:63
   and https://github.com/cryostatio/cryostat/actions/runs/3293985967/jobs/5431057884#step:5:8
   Is there any way around this?

[maxcao13]

And on a side note, I can't seem to get the rerunFailedTests config to work with maven failsafe, and I think it has something to do with providers: https://maven.apache.org/surefire/maven-failsafe-plugin/examples/providers.html. There is a note at the top of the link https://maven.apache.org/surefire/maven-failsafe-plugin/examples/rerun-failing-tests.html NOTE : This feature is supported for JUnit 4.x, and (since 3.0.0-M4) JUnit 5.x..

[andrewazores]

And on a side note, I can't seem to get the rerunFailedTests config to work with maven failsafe,

With a separated out integration test run I guess this is not so important. Re-running the flaky tests is not such an inconvenience anymore. We should still know about the flaky test runs and see that the failures are occurring, so even if rerunFailedTests was working as expected I think we would want to capture that some number of failures did occur and maybe have an automated comment detailing the results.

What is the reason for having fetch-depth: 0 when checking out cryostat? Can that be removed when we have runners run spotless, spotbugs, shellcheck?

I believe this is to do with the git clone and how many commits' worth of history are cloned. I think it makes sense to clone shallowly, but doing the clone once and caching the sources across jobs would be ideal. I don't think a shallow clone across each of the jobs is too bad, at least not right now, since the repo isn't too large and shouldn't take very long to clone.

There is a dependency on xpath from the repeated-integration-tests script and it takes about 30 seconds to install, I'm not sure if that's great or not. If it's not, is there an easier way to get the POD_NAME and CONTAINER_NAME without using xpath, e.g. maybe just passing the variables directly and updating the repeated-integration-tests script?

Good insight. Yep, passing variables directly or just invoking mvn directly in the same way that the script does makes sense. Those are read from the pom.xml to make it easier and more centralized to make experimental changes across different setups, but for CI all these things should just be constant values.

I was thinking that the repeated-integration-tests.bash and smoketest.sh could use some additional capability to pass in variables already, too, and just read from the pom.xml using xpath if the env var isn't set. This would be used for specifying things like the jfr-datasource image to use in smoketest.sh for example.

I would think that the skjolber/maven-cache github action would cache the dependencies needed for the mvn lifecycle but it seems like plugin dependecies are downloaded regardless. Is there any way around this?

I guess this comes down to what paths are cached by that action and where the dependencies are being downloaded to. Maybe the action isn't caching as much as we think it should.

https://github.com/marketplace/actions/maven-cache

You could try using the more generic cache action:

https://github.com/actions/cache

and cache the whole $HOME/.m2 perhaps.

[ebaron]

1. What is the reason for having fetch-depth: 0 when checking out cryostat? Can that be removed when we have runners run spotless, spotbugs, shellcheck?

This was added to fix https://github.com/cryostatio/cryostat/issues/1007 when we started syncing the web-client submodule.

[maxcao13]

Gonna say it's ready now for actual review now. The caching should be fine for the integration tests, it doesn't take that long anyways only 10 seconds or so, and the caching for the shellcheck, spotless, spotbugs, doesn't really matter since its concurrent. The existing caching action does cache the entire m2 folder, but it only detects changes in the pom.xml files.

I think there is a need to update the mergability Required label thing so that all the jobs are required. I don't think I'm able to do that.

[andrewazores]

I think there is a need to update the mergability Required label thing so that all the jobs are required. I don't think I'm able to do that.

I can do that but I think this PR needs to be merged first, and then I can update the repo settings to make those required.

[andrewazores]

@maxcao13 looks like push-to-quay might be broken:

https://github.com/cryostatio/cryostat/actions/runs/3300643166/jobs/5445469164

[maxcao13]

@maxcao13 looks like push-to-quay might be broken:

https://github.com/cryostatio/cryostat/actions/runs/3300643166/jobs/5445469164

Uh oh, I'll see what's wrong.

[maxcao13]

Oh, I just forgot to checkout the branch again I think.