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

Enable integration tests with Daphne Helper #3030

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

cjpatton
Copy link
Contributor

@cjpatton cjpatton commented Apr 18, 2024

Partially addresses #2389.

Also, remove code for testing against Daphne running locally.

Also, remove code for testing against Daphne running locally.
@cjpatton cjpatton requested a review from a team as a code owner April 18, 2024 21:31
@@ -10,11 +10,11 @@ use serde_json::json;
use testcontainers::{clients::Cli, GenericImage, RunnableImage};
use url::Url;

const DAPHNE_HELPER_IMAGE_NAME_AND_TAG: &str = "cloudflare/daphne-worker-helper:sha-f6b3ef1";
const DAPHNE_HELPER_IMAGE_NAME_AND_TAG: &str = "cloudflare/daphne-worker-helper:sha-4c612db";
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewer note: This tag was created manually. It matches the commit that was checked out when the image was built. In the future we will add publishing the container to CI.

@cjpatton
Copy link
Contributor Author

@branlwyd can you take a look at the CI failure? It's a bit cryptic:

image

@inahga
Copy link
Contributor

inahga commented Apr 19, 2024

It actually failed a test further above--this job step just fails the job if a failure as detected earlier. The true error is here https://github.com/divviup/janus/actions/runs/8744950821/job/23999640943?pr=3030#step:14:411, where it complains:

Error response from daemon: Could not find the file /logs in container 99f04f6724ae4b56b30a8a6185505212813810d3c83e31fbc0ccb1d92821b183
thread 'daphne::janus_in_process_daphne' panicked at library/core/src/panicking.rs:144:5:
thread 'daphne::janus_in_process_daphne' panicked at /home/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/testcontainers-0.15.0/src/core/container.rs:143:17:
container 99f04f6724ae4b56b30a8a6185505212813810d3c83e31fbc0ccb1d92821b183 does not expose port 8788

n.b. I think the second does not expose port 8788 is the true error, and the container logs guard is inducing a double-panic because the container doesn't actually exist at the time of drop.

@divergentdave
Copy link
Collaborator

The actual error message is a couple steps above, container <hash> does not expose port 8788, but that's misleading. I tried running the container, and I get the following:

WARNING: The requested image's platform (linux/arm64) does not match the detected host platform (linux/amd64/v4) and no specific platform was requested
exec /bin/bash: exec format error

Unpacking the image, I see that both /bin/bash and /service are indeed ELF executables for ARM aarch64, so this is just an issue with missing images for Intel architectures.

@inahga
Copy link
Contributor

inahga commented Apr 19, 2024

Ah, nice find. Yeah if you built it locally with docker build on an ARM macbook it's going to yield a linux/arm64 image. Perhaps the best way around that is to set an explicit target in Daphne's build step cargo build --target x86_64-unknown-linux-gnu.

@divergentdave
Copy link
Collaborator

I don't think it would be worth hardcoding a target in the Dockerfile, since the whole base image needs to match the architecture as well, and whatever (docker-)machine is used for building needs to be able to run that architecture.

@inahga
Copy link
Contributor

inahga commented Apr 19, 2024

Ah right, Rust cross-compilation is not as fool-proof as I had hoped, you still need a platform-specific linker.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants