Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 14 commits into from Dec 4, 2020
Merged

Conversation

@benjamingeer
Copy link
Collaborator

@benjamingeer benjamingeer commented Nov 26, 2020

resolves DSP-1114

@benjamingeer benjamingeer self-assigned this Nov 26, 2020
@benjamingeer
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Nov 26, 2020

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
Copy link
Collaborator

@subotic subotic commented Nov 26, 2020

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
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Nov 26, 2020

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
Copy link
Collaborator

@subotic subotic commented Nov 26, 2020

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

@subotic
Copy link
Collaborator

@subotic subotic commented Nov 26, 2020

you can see this here:

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

@benjamingeer
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Nov 26, 2020

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
Copy link
Collaborator

@subotic subotic commented Nov 26, 2020

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
Copy link
Collaborator

@subotic subotic commented Nov 26, 2020

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

@benjamingeer
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Nov 26, 2020

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
Copy link
Collaborator

@subotic subotic commented Nov 26, 2020

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

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

@subotic
Copy link
Collaborator

@subotic subotic commented Nov 26, 2020

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

@subotic
Copy link
Collaborator

@subotic subotic commented Nov 26, 2020

"//sipi/config",

@subotic
Copy link
Collaborator

@subotic subotic commented Nov 26, 2020

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

"//sipi/config",

@benjamingeer
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Nov 26, 2020

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
Copy link
Collaborator

@subotic subotic commented Dec 1, 2020

@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
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Dec 1, 2020

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

@benjamingeer
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Dec 1, 2020

@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
Copy link
Collaborator

@subotic subotic commented Dec 1, 2020

@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.

@benjamingeer
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Dec 2, 2020

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
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Dec 2, 2020

Should it be using 127.0.0.1 or 0.0.0.0?

@benjamingeer
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Dec 2, 2020

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
Copy link
Collaborator

@subotic subotic commented Dec 2, 2020

Should it be using 127.0.0.1 or 0.0.0.0?

It should actually be your machine's IP address.

@benjamingeer
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Dec 2, 2020

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.

@@ -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 comment has been minimized.

@subotic

subotic Dec 2, 2020
Collaborator

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

This comment has been minimized.

@benjamingeer

benjamingeer Dec 2, 2020
Author Collaborator

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.

@@ -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)

This comment has been minimized.

@subotic

subotic Dec 2, 2020
Collaborator

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

@benjamingeer
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Dec 2, 2020

@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
Copy link
Collaborator

@subotic subotic commented Dec 2, 2020

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
Copy link
Collaborator

@subotic subotic commented Dec 2, 2020

I've pushed the fix for the testcontainer deprecation

@benjamingeer
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Dec 2, 2020

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
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Dec 2, 2020

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
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Dec 2, 2020

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

@benjamingeer
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Dec 2, 2020

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
Copy link
Collaborator

@subotic subotic commented Dec 2, 2020

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
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Dec 2, 2020

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

@subotic
Copy link
Collaborator

@subotic subotic commented Dec 2, 2020

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
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Dec 2, 2020

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
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Dec 2, 2020

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
Copy link
Collaborator

@subotic subotic commented Dec 2, 2020

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
Copy link
Collaborator

@subotic subotic commented Dec 2, 2020

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
Copy link
Collaborator

@subotic subotic commented Dec 2, 2020

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
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Dec 3, 2020

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.

benjamingeer added 3 commits Dec 3, 2020
@benjamingeer benjamingeer merged commit 3236d25 into main Dec 4, 2020
8 checks passed
8 checks passed
Build Everything
Details
API Unit Tests
Details
API E2E Tests
Details
API Integration Tests
Details
Upgrade Integration Tests
Details
Docs Build Test
Details
Update next release draft
Details
Publish (on release only)
Details
@benjamingeer benjamingeer deleted the fix/DSP-1114-sipi branch Dec 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants