Openjdk-tests: Re-running get.sh without cleaning workspace causes issues when downloading an sdk

[adam-thorpe]

If the same compressed sdk file exists, when curl tries to download a new one it will hang forever.

curl -OLJks  https://api.adoptopenjdk.net/v2/binary/releases/openjdk11?openjdk_impl=openj9&os=linux&arch=x64&release=latest&type=jdk&heap_size=normal
curl error code: 23. Sleep 300 secs, then retry 1...

Also if the j2sdk-image directory exists, extracting the sdk will cause the program to error:

unzip file: j2sdk-image ...
gzip: j2sdk-image is a directory -- ignored
tar: This does not look like a tar archive
tar: Exiting with failure status due to previous errors

These problems aren't detected by the grinders and jenkins machines, as they clean their workspaces after each run. However when testing locally, these problems will persist unless these directories are cleaned. I can see this causing issues for people who are unaware as the error messages given are not too helpful in describing what the actual problem is.

For the first problem, simply checking whether the compressed sdk exists before downloading one would not suffice as a newer nightly build of the same version/architecture/impl etc would be marked exactly the same. So I would propose that the file gets removed once it is extracted to j2sdk-image.

For the second problem, when extracting the sdk it should overwrite the pre-existing j2sdk-image directory, should it actually exist.

Can I get some opinions on these changes? In theory this should not effect any of the jenkins machines, but more so for local testers.

Edit: It seems that when extracting the sdk, the script just looks for every file in the directory and tries to extract it, halting if any of the files cannot be extracted.

[sophia-guo]

The error message is not because of existing sdk file. The retry is because download is not successful. If same sdk file exists the curl won't try to redownload. A newer nightly build of the same version/architecture/impl has the different filename with build date.

[adam-thorpe]

My apologies, you are correct in saying that nightly builds do have different file names. However, curl does try to redownload duplicate sdk files. It will retry 5 more times (each with a 5 minute timeout) before giving up.

[adam-thorpe]

Removing the silent mode:

curl -OLJks  https://api.adoptopenjdk.net/v2/binary/nightly/openjdk11?openjdk_impl=openj9&os=linux&arch=x64&release=latest&type=jdk&heap_size=normal
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   170  100   170    0     0    170      0  0:00:01 --:--:--  0:00:01   170
100   646    0   646    0     0    345      0 --:--:--  0:00:01 --:--:--  630k
Warning: Refusing to overwrite 
Warning: OpenJDK11U-jdk_x64_linux_openj9_2019-10-16-10-43.tar.gz: File exists
  0  190M    0 16360    0     0   6220      0  8:56:34  0:00:02  8:56:32  6220
curl: (23) Failed writing body (0 != 16360)
curl error code: 23. Sleep 300 secs, then retry 1...

[sophia-guo]

In that case we can simply limited retry mechanism to non error code 23, which means File exists or refusing to write.

https://github.com/AdoptOpenJDK/openjdk-tests/blob/master/get.sh#L172

[smlambert]

perhaps ...

if $download_exit_code == 23, print a message saying file is already downloaded and on the system and proceed with the rest of the functions of the script

[adam-thorpe]

That sounds good to me. Can I suggest also adding the -S option to the curl command. It adds an additional line of output when the command fails, which could be more useful to explain the problem than just an error code. Eg:

get jdk binary...
curl -OLJks  blah-blah.blah-blah
curl: (6) Could not resolve host: blah-blah.blah-blah
curl error code: 6. Sleep 300 secs, then retry 1...

Also what about the second problem? The script will just try to extract everything in the directory including other sdks and directories. I guess a rule could be made that stops incompatible files from being downloaded, but any old sdks could overwrite a newer one. So is there a way to pass the downloaded files from curl to be extracted since they have different names?

[llxia]

Should we add a clean script? If users want to use get.sh to download sdk, we delete binary and unzipped dirs (i.e., openjdkbinary, j2re-image, etc) first and then download the SDK. In this way, we avoid confusion of downloading and unzipping files.

[smlambert]

Sure, a clean step to protect against users not cleaning their work areas themselves.

We have had these discussion before, I almost want to separate the download of the binaries from the download of TKG and test material, as this mixing seems to confuse people. Its good for our automation, but for a local user, I would rather they took some responsibility for their work areas themselves.

In all our doc, we should encourage users to download their own binaries and set TEST_SDK_HOME to where they put them, rather than relying on get.sh ... that way they are managing their own builds and binaries and the tidying/organizing is left to the user.

[adam-thorpe]

The get.sh script is good for getting latest nightly/release builds for people running tests regularly, but I agree that users should be prompted to manage their testing themselves so they have more control.

One point I would like to make is about the documentation of get.sh. It might be worth pointing out to users that they do not need to set --jdk_version, --jdk_impl etc if they are not trying to download an sdk. There was a recent post in the testing channel where a user had set these values and --sdkdir (which is essentially pulling an sdk from adoptopenjdk). They had a local build available and got by this by manually setting TEST_SDK_HOME after the get.sh script had finished, but if the TEST_SDK_HOME variable was set prior they may have tested an incorrect version for their needs. I think some clarification in the documentation would be great here.

[smlambert]

Updated doc, most parameters were already marked as optional, and as is the standard practice the [ ] indicates optionality as well: https://github.com/AdoptOpenJDK/openjdk-tests/blob/master/doc/userGuide.md#local-testing-via-make-targets-on-the-commandline

Planning a major overhaul of test doc soon (with plan to centralize most of it to the TKG repo), but feel free to create PRs to improve doc as it stands today.

[smlambert]

After discussing with @llxia and @sophia-guo - prefer to just check if test dir has contents and warn/exit from the script "Please provide an empty sdk directory before proceeding, currently $sdkDir has content which this script will not overwrite" or some such thing.

Frankly the get.sh script has the fetching of sdk to ease automation, the expected use case for devs is to manually download or build their sdk, and set TEST_JDK_HOME, but agree we should be clear with the behaviour of this script to avoid corruption.

[adam-thorpe]

Sounds good to me, I'll put in a PR later :)

[sophia-guo]

@adam-thorpe , Just realized that #1425 will break all test builds which grab the sdk from upstream artifact - currently AdoptOpenJDK test build story ( not by artifactory url), which happens before get.sh is called.

https://ci.adoptopenjdk.net/view/Test_grinder/job/Grinder/1012/console