fix(sipi): Don't expect API v1 status code (DSP-1114) #1763

benjamingeer · 2020-11-26T11:38:41Z

resolves DSP-1114

benjamingeer · 2020-11-26T15:37:05Z

Something very strange is happening in KnoraSipiIntegrationV2ITSpec.

If I do make stack-up (on this branch) and request http://localhost:1024/0803/incunabula_0000000002.jp2/full/499,630/0/default.jpg in a web browser, the image loads. Sipi's log in the Docker dashboard says:

Sipi: pre_flight called in sipi.init-knora.lua

Now I set a breakpoint at line 305 of KnoraSipiIntegrationV2ITSpec, where it requests the same image.

https://github.com/dasch-swiss/dsp-api/pull/1763/files#diff-ccc8e1b4be7f09191193ca27b7b469d7b2e34f0ff97482fded3c02552ea27fc2R305

I run the test in IntelliJ's debugger. At the breakpoint, I can see that Sipi returned 404 Not Found. Why isn't the image there?

Now I set another breakpoint at line 438 and resume the test. At the breakpoint, the test has uploaded a file and is requesting the newly created file. I copy the IIIF URL (savedImage.iiifUrl), and I can load the image in a web browser. In the Docker dashboard, in the Sipi test container log, I can see that it didn't print pre_flight called in sipi.init-knora.lua.

So:

Images that are available when you do make stack-up are not available to the Sipi image running in the test.
The Sipi configuration used with make stack-up is not the same as the one used in the test.

I can't figure out where the Sipi configuration is coming from in the test. In org.knora.webapi.TestContainers, it says it's using sipi.knora-docker-config.lua, which loads sipi.init-knora.lua. So it should be the same, but it isn't.

Any ideas @subotic @SepidehAlassi ?

subotic · 2020-11-26T15:53:57Z

There is one small difference. In the tests, the Docker containers are started through the Testcontainers library, which does not allow mounting of volumes. When you run make stack-up then the sipi/images folder is mounted inside the Sipi container.

benjamingeer · 2020-11-26T16:24:38Z

So where does the Sipi test container get its configuration files from? I suspect that the Lua pre_flight function isn’t being called at all inside the test container. How can I find out?

subotic · 2020-11-26T16:47:16Z

The config and scripts folders are backed into the Docker image. This is why we publish a DSP-API-specific Sipi Docker image.

subotic · 2020-11-26T16:48:07Z

you can see this here:

dsp-api/docker/knora-sipi/BUILD.bazel

Line 11 in 346873d

"//sipi/scripts:sipi-scripts",

benjamingeer · 2020-11-26T16:51:35Z

The config and scripts folders are backed into the Docker image.

So I guess that means that if I need to change a Sipi script and test it, I need to publish a new Sipi Docker image. How do I do that?

subotic · 2020-11-26T16:52:53Z

hmm, something does not look right. The config folder should be added to the Sipi image, and also used when starting the container for the tests.

subotic · 2020-11-26T16:54:40Z

you recreate the docker containers locally with make docker-build. Then these images will be used when running the tests.

benjamingeer · 2020-11-26T16:58:15Z

you recreate the docker containers locally with make docker-build. Then these images will be used when running the tests.

I did that, and it had no effect on the output that I described above.

subotic · 2020-11-26T17:00:01Z

ok, it should not be necessary to recreate the docker containers. I implemented it better:

dsp-api/webapi/src/it/scala/org/knora/webapi/TestContainers.scala

Line 40 in 346873d

SipiContainer.withCommand("--config=/sipi/config/sipi.knora-docker-config.lua")

subotic · 2020-11-26T17:01:39Z

The sipi scripts are mounted into the image before the container is started. They are put on the class path.

subotic · 2020-11-26T17:03:28Z

dsp-api/webapi/BUILD.bazel

Line 140 in 346873d

"//sipi/config",

subotic · 2020-11-26T17:06:25Z

or more correctly, they are part of the "it_library" which is a dependency for all it tests:

dsp-api/webapi/BUILD.bazel

Line 199 in 346873d

"//sipi/config",

benjamingeer · 2020-11-26T17:10:57Z

The sipi scripts are mounted into the image before the container is started. They are put on the class path.

But in that case, Sipi should print the following when run inside the test container:

Sipi: pre_flight called in sipi.init-knora.lua

But it doesn't. That's what makes me think that the pre_flight script isn't being called inside the test container, and that this could be because it's not using the same configuration file.

subotic · 2020-12-01T13:49:23Z

@benjamingeer I found the error. I made a mistake when "mounting" the sipi.init-knora.lua file. I mounted sipi.knora-docker-config.lua twice. Once as itself and another time as sipi.init-knora.lua. I'm not sure why sipi didn't complain. Those files don't look anything alike.

benjamingeer · 2020-12-01T14:22:05Z

@subotic No worries, I'm glad it turns out to be easy to fix. :)

benjamingeer · 2020-12-01T14:29:02Z

@subotic But now I get this error in the Sipi log when I do bazel test //webapi/src/it/scala/org/knora/webapi:KnoraSipiIntegrationV2ITSpec:

BUILD_TIMESTAMP: 2020-11-06 10:22
BUILD_SCM_TAG: v3.0.0-rc.9
BUILD_SCM_REVISION: 453fd94601e0e732a65839e5fbde55bcc94bf72f
Sipi: BUILD_TIMESTAMP: 2020-11-06 10:22
Sipi: BUILD_SCM_TAG: v3.0.0-rc.9
Sipi: BUILD_SCM_REVISION: 453fd94601e0e732a65839e5fbde55bcc94bf72f
Sipi: Cache at "/sipi/cache" cachesize=104857600 nfiles=0 hysteresis=0.100000
Sipi: Closing cache...
Error at [/sipi/shttps/Server.h: 433]: initscript "/sipi/scripts/sipi.init-knora.lua" not found!

subotic · 2020-12-01T14:49:55Z

@subotic But now I get this error in the Sipi log when I do bazel test //webapi/src/it/scala/org/knora/webapi:KnoraSipiIntegrationV2ITSpec:

BUILD_TIMESTAMP: 2020-11-06 10:22
BUILD_SCM_TAG: v3.0.0-rc.9
BUILD_SCM_REVISION: 453fd94601e0e732a65839e5fbde55bcc94bf72f
Sipi: BUILD_TIMESTAMP: 2020-11-06 10:22
Sipi: BUILD_SCM_TAG: v3.0.0-rc.9
Sipi: BUILD_SCM_REVISION: 453fd94601e0e732a65839e5fbde55bcc94bf72f
Sipi: Cache at "/sipi/cache" cachesize=104857600 nfiles=0 hysteresis=0.100000
Sipi: Closing cache...
Error at [/sipi/shttps/Server.h: 433]: initscript "/sipi/scripts/sipi.init-knora.lua" not found!

If you use Bazel directly, then you first need to manually publish the new Docker image locally: make docker-build.

subotic · 2020-12-02T10:58:05Z

did you run make docker-build?

benjamingeer · 2020-12-02T11:09:24Z

I've just done:

make docker-build
bazel test //webapi/src/it/scala/org/knora/webapi:KnoraSipiIntegrationV2ITSpec

I stopped the Ryuk container as soon as it appeared. The test failed with:

- should change a still image file value *** FAILED *** (1 second, 227 milliseconds)
  org.knora.webapi.exceptions.AssertionException: Got HTTP 401
 REQUEST: HttpRequest(HttpMethod(GET),http://localhost:33993/0001/LX0BguiymCH-EWjz5MVO8Od.jp2/full/499,630/0/default.jpg,List(),HttpEntity.Strict(none/none,0 bytes total),HttpProtocol(HTTP/1.1)),
 RESPONSE: Unauthorized: Unauthorized access

The log in the Sipi test container says:

Sipi: Server.http() failed: Error #1368 HTTP GET request to http://127.0.0.1:3333/admin/files/0001/LX0BguiymCH-EWjz5MVO8Od.jp2 failed:
Failed to connect to 127.0.0.1 port 3333: Connection refused
Sipi: GET /0001/LX0BguiymCH-EWjz5MVO8Od.jp2/full/499,630/0/default.jpg failed (Unauthorized): Unauthorized access

benjamingeer · 2020-12-02T11:11:02Z

Should it be using 127.0.0.1 or 0.0.0.0?

benjamingeer · 2020-12-02T11:15:46Z

The very strange thing is that if I comment out the test reject an image file with the wrong file extension, the test change a still image file value passes.

subotic · 2020-12-02T11:25:16Z

Should it be using 127.0.0.1 or 0.0.0.0?

It should actually be your machine's IP address.

benjamingeer · 2020-12-02T11:26:50Z

It should actually be your machine's IP address.

Is there something I need to configure to fix that? Sipi says it's using 127.0.0.1.

subotic · 2020-12-02T11:30:41Z

webapi/src/it/scala/org/knora/webapi/TestContainers.scala

@@ -27,6 +28,10 @@ import org.testcontainers.containers.{BindMode, GenericContainer}
  */
 object TestContainers {

+    // get local IP address, which we need for SIPI
+    val localhost: InetAddress = InetAddress.getLocalHost
+    val localIpAddress: String = localhost.getHostAddress


This should be your machine's IP. Could you maybe run it in debug mode and see what you get here?

I think it's a static initialiser bug: if so, these vals need to be lazy because otherwise they might not be initialised before they're needed. Will confirm after lunch.

subotic · 2020-12-02T11:31:16Z

webapi/src/it/scala/org/knora/webapi/TestContainers.scala

@@ -35,14 +40,12 @@ object TestContainers {

    val SipiContainer = new GenericContainer("bazel/docker/knora-sipi:image")
    SipiContainer.withExposedPorts(1024)
-    SipiContainer.withEnv("SIPI_WEBAPI_HOSTNAME", "api")
+    SipiContainer.withEnv("SIPI_WEBAPI_HOSTNAME", localIpAddress)


Here your machine's IP should be used to configure Sipi.

benjamingeer · 2020-12-02T13:44:49Z

@subotic

% scala
Welcome to Scala 2.13.3 (OpenJDK 64-Bit Server VM, Java 14.0.1).
Type in expressions for evaluation. Or try :help.

scala> import java.net._
import java.net._

scala> val localhost: InetAddress = InetAddress.getLocalHost
val localhost: java.net.InetAddress = euler.local/127.0.0.1

scala> val localIpAddress: String = localhost.getHostAddress
val localIpAddress: String = 127.0.0.1

subotic · 2020-12-02T14:52:33Z

scala
Welcome to Scala 2.13.4 (OpenJDK 64-Bit Server VM, Java 11.0.9.1).
Type in expressions for evaluation. Or try :help.

scala> import java.net._
import java.net._

scala> val localhost: InetAddress = InetAddress.getLocalHost
val localhost: java.net.InetAddress = iml-stargazer.localdomain/192.168.1.100

scala> val localIpAddress: String = localhost.getHostAddress
val localIpAddress: String = 192.168.1.100

This is what I get.

Also, I see that you have Java 14 installed.

subotic · 2020-12-02T14:53:42Z

I've pushed the fix for the testcontainer deprecation

benjamingeer · 2020-12-02T14:56:41Z

Also, I see that you have Java 14 installed.

Maybe Homebrew installed it with Scala. The only JVM I've installed is AdoptOpenJDK 11:

% java -version
openjdk version "11.0.5" 2019-10-15
OpenJDK Runtime Environment AdoptOpenJDK (build 11.0.5+10)
OpenJDK 64-Bit Server VM AdoptOpenJDK (build 11.0.5+10, mixed mode)

benjamingeer · 2020-12-02T15:01:12Z

public class Test {
    public static void main(String[] args) throws Exception {
        System.out.println(java.net.InetAddress.getLocalHost().getHostAddress());
    }   
}

% java -version
openjdk version "11.0.5" 2019-10-15
OpenJDK Runtime Environment AdoptOpenJDK (build 11.0.5+10)
OpenJDK 64-Bit Server VM AdoptOpenJDK (build 11.0.5+10, mixed mode)
% javac Test.java
% java -cp . Test
127.0.0.1

benjamingeer · 2020-12-02T15:03:24Z

Maybe this is the solution: https://stackoverflow.com/a/16444863

benjamingeer · 2020-12-02T16:06:31Z

OK I found the problem! It actually was a static initialisation problem, but not the kind I expected. The problem is that there are several different TestContainers objects:

webapi/src/it/scala/org/knora/webapi/TestContainers.scala
webapi/src/test/scala/org/knora/webapi/TestContainers.scala
salsah1/src/test/scala/org/knora/salsah/TestContainers.scala

Which one is actually used seems to be random.

Could you clean this up so there is only one TestContainers object? To work on my machine, it needs to contain this:

import java.net.NetworkInterface
import scala.collection.JavaConverters._

val localIpAddress: String = NetworkInterface.getNetworkInterfaces
    .asScala.toSeq.filter(!_.isLoopback)
    .flatMap(_.getInetAddresses.asScala.toSeq.filter(_.getAddress.length == 4).map(_.toString))
    .headOption.getOrElse(throw TestConfigurationException(s"No suitable network interface found"))

subotic · 2020-12-02T16:49:36Z

OK I found the problem! It actually was a static initialisation problem, but not the kind I expected. The problem is that there are several different TestContainers objects:

webapi/src/it/scala/org/knora/webapi/TestContainers.scala

webapi/src/test/scala/org/knora/webapi/TestContainers.scala

salsah1/src/test/scala/org/knora/salsah/TestContainers.scala

Which one is actually used seems to be random.

Could you clean this up so there is only one TestContainers object? To work on my machine, it needs to contain this:
import java.net.NetworkInterface
import scala.collection.JavaConverters._

val localIpAddress: String = NetworkInterface.getNetworkInterfaces
    .asScala.toSeq.filter(!_.isLoopback)
    .flatMap(_.getInetAddresses.asScala.toSeq.filter(_.getAddress.length == 4).map(_.toString))
    .headOption.getOrElse(throw TestConfigurationException(s"No suitable network interface found"))

Strange. There should be only one of course. I will try to find out what is happening.

benjamingeer · 2020-12-02T17:00:27Z

You added the ones in test and it in PR #1653, and you added the one in salsah1 in PR #1480.

subotic · 2020-12-02T21:46:07Z

Yes, it is three times in the codebase, but these hierarchies are completely separate. A test should only ever see one of those implementations. If it doesn't, then it is a configuration error in some Bazel build file.

benjamingeer · 2020-12-02T21:56:14Z

I think that having three classes with the same class name, in the same package, in different parts of the codebase, is asking for trouble with the class loader, which is what has happened here (as I can see because, if I change all three, it works). And it creates a maintenance problem, because if the three classes are redundant and are supposed to be identical, it is too easy to change one and forget to change the others, which also just happened here. Each source file should exist only in one place.

benjamingeer · 2020-12-02T22:00:15Z

And I would like to emphasise that I have now wasted several days debugging this problem, which would have been avoided if good basic software development practices had been followed, i.e. no redundant code.

subotic · 2020-12-02T22:09:29Z

I agree. We have the same problem with other test classes, too. Most base classes under it and test need to be updated simultaneously in both places. One way would be to merge the test and it hierarchies. Salsah is completely separate, so I'm not really sure, why the classloader sees all three implementations. How did you find it out?

subotic · 2020-12-02T22:18:36Z

And I would like to emphasise that I have now wasted several days debugging this problem, which would have been avoided if good basic software development practices had been followed, i.e. no redundant code.

The problems you are facing and I am trying to help you with is probably local to your system, as the test pass on my machine locally and on Github-CI. As for the redundant code, with SBT it wasn't possible to do it otherwise. If you want to clean up, please go ahead and merge the different test code hierarchies and eliminate the redundant code.

subotic · 2020-12-02T22:34:56Z

The problem is in the build file, where the test is defined. There, both the it and test libraries are added as dependencies. This was done in the PR #1724. Then of course, both implementations will be on the classpath.

benjamingeer · 2020-12-03T06:43:32Z

One way would be to merge the test and it hierarchies.

That's what I was thinking, too.

The problems you are facing and I am trying to help you with is probably local to your system, as the test pass on my machine locally and on Github-CI.

This problem started because the admin API depended on API v1. That dependency shouldn't have been there.

Then there was the misconfiguration of the test containers, which apparently caused Sipi to run with the wrong configuration. As a result, tests passed when they should have failed.

As for the problem on my machine: after looking at https://stackoverflow.com/a/16444863, I think it's just luck that it worked on your system and on GitHub-CI. It seems that you can't assume that InetAddress.getLocalHost.getHostAddress won't return the loopback interface. If you don't want the loopback interface, it seems that you have to get all the available interfaces, and choose the one you want.

As for the redundant code, with SBT it wasn't possible to do it otherwise. If you want to clean up, please go ahead and merge the different test code hierarchies and eliminate the redundant code.

The different test code hierarchies could have been merged under SBT, too. So it was possible not to copy and paste redundant code with SBT.

The problem is in the build file, where the test is defined. There, both the it and test libraries are added as dependencies.

This is an example of how difficult it is to manage dependencies with Bazel. I think this is a good reason to get rid of Bazel and go back to SBT.

fix(sipi): Don't expect API v1 status code.

be75de1

benjamingeer self-assigned this Nov 26, 2020

benjamingeer added bug something isn't working Sipi labels Nov 26, 2020

benjamingeer requested review from kilchenmann and SepidehAlassi November 26, 2020 11:42

kilchenmann approved these changes Nov 26, 2020

View reviewed changes

fix(sipi): Add some debugging.

c1146df

style(sipi): Remove newline.

b311ee1

Merge branch 'main' into fix/DSP-1114-sipi

47baf52

fix: move sipi init scripts to the scripts folder

bd5401a

test(sipi): Try to fix test.

8af166a

subotic reviewed Dec 2, 2020

View reviewed changes

fix: testcontainer deprecation

f345669

Benjamin Geer added 3 commits December 3, 2020 21:25

Merge branch 'main' into fix/DSP-1114-sipi

9fbecd0

fix(test): Don't pass loopback interface to Sipi.

2aab29c

test: Fix build errors.

0c285ae

benjamingeer merged commit 3236d25 into main Dec 4, 2020

benjamingeer deleted the fix/DSP-1114-sipi branch December 4, 2020 10:05

fix(sipi): Don't expect API v1 status code (DSP-1114) #1763

fix(sipi): Don't expect API v1 status code (DSP-1114) #1763

Conversation

benjamingeer commented Nov 26, 2020

benjamingeer commented Nov 26, 2020 • edited

subotic commented Nov 26, 2020

benjamingeer commented Nov 26, 2020

subotic commented Nov 26, 2020

subotic commented Nov 26, 2020

benjamingeer commented Nov 26, 2020

subotic commented Nov 26, 2020

subotic commented Nov 26, 2020

benjamingeer commented Nov 26, 2020

subotic commented Nov 26, 2020

subotic commented Nov 26, 2020

subotic commented Nov 26, 2020

subotic commented Nov 26, 2020

benjamingeer commented Nov 26, 2020

subotic commented Dec 1, 2020

benjamingeer commented Dec 1, 2020

benjamingeer commented Dec 1, 2020

subotic commented Dec 1, 2020

subotic commented Dec 2, 2020

benjamingeer commented Dec 2, 2020 • edited

benjamingeer commented Dec 2, 2020

benjamingeer commented Dec 2, 2020

subotic commented Dec 2, 2020

benjamingeer commented Dec 2, 2020

subotic Dec 2, 2020

Choose a reason for hiding this comment

benjamingeer Dec 2, 2020

Choose a reason for hiding this comment

subotic Dec 2, 2020

Choose a reason for hiding this comment

benjamingeer commented Dec 2, 2020

subotic commented Dec 2, 2020

subotic commented Dec 2, 2020

benjamingeer commented Dec 2, 2020 • edited

benjamingeer commented Dec 2, 2020

benjamingeer commented Dec 2, 2020

benjamingeer commented Dec 2, 2020

subotic commented Dec 2, 2020

benjamingeer commented Dec 2, 2020

subotic commented Dec 2, 2020

benjamingeer commented Dec 2, 2020

benjamingeer commented Dec 2, 2020

subotic commented Dec 2, 2020

subotic commented Dec 2, 2020

subotic commented Dec 2, 2020

benjamingeer commented Dec 3, 2020

benjamingeer commented Nov 26, 2020 •

edited

benjamingeer commented Dec 2, 2020 •

edited

benjamingeer commented Dec 2, 2020 •

edited