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

ci: add a test to check that the Docker image config works #5968

Merged
merged 25 commits into from
Jan 23, 2023
Merged

Conversation

gustavovalverde
Copy link
Member

@gustavovalverde gustavovalverde commented Jan 16, 2023

Motivation

We want to test that our Docker build scripts configure Zebra correctly:

  • check that $ZEBRA_CONF_PATH works
  • make sure the default Docker config works with the latest Zebra version

Fixes #5168

Solution

  • Add a Docker test for the default config
  • Add a Docker test for $ZEBRA_CONF_PATH

Review

Anyone can review this, validating the test is using the right configuration

Reviewer Checklist

  • Will the PR name make sense to users?
    • Does it need extra CHANGELOG info? (new features, breaking changes, large changes)
  • Are the PR labels correct?
  • Does the code do what the ticket and PR says?
    • Does it change concurrent code, unsafe code, or consensus rules?
  • How do you know it works? Does it have tests?

Follow Up Work

Use a custom file instead of the existing one living inside common/configs

@gustavovalverde gustavovalverde requested a review from a team as a code owner January 16, 2023 12:40
@gustavovalverde gustavovalverde requested review from teor2345 and removed request for a team January 16, 2023 12:40
@github-actions github-actions bot added C-feature Category: New features C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG labels Jan 16, 2023
@gustavovalverde gustavovalverde added A-devops Area: Pipelines, CI/CD and Dockerfiles C-enhancement Category: This is an improvement P-Medium ⚡ I-build-fail Zebra fails to build labels Jan 16, 2023
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Looks good, let's see how it goes!

Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

I don't think the test commands are quite right yet.

We also need to add the new test to the branch protection rules, did you want to do that after this PR merges?

.github/workflows/continous-integration-docker.yml Outdated Show resolved Hide resolved
@gustavovalverde gustavovalverde marked this pull request as draft January 17, 2023 11:12
@gustavovalverde
Copy link
Member Author

In the test Docker stage we use cargo test --locked --release --features lightwalletd-grpc-tests --workspace --no-run which doesn't have zebrad as a binary

This is different from the runtime stage as it uses cargo build --locked --release --features sentry --package zebrad --bin zebrad

How should we run zebrad from the test stage @teor2345 ?

@teor2345
Copy link
Contributor

I'm not sure I fully understand the question, but I'll try to say some helpful things:

In the test Docker stage we use cargo test --locked --release --features lightwalletd-grpc-tests --workspace --no-run which doesn't have zebrad as a binary

--no-run builds the binaries for the tests, without running any test binaries or the zebrad binary.

This is different from the runtime stage as it uses cargo build --locked --release --features sentry --package zebrad --bin zebrad

This builds the zebrad release binary.

How should we run zebrad from the test stage @teor2345 ?

The test stage should create a zebrad binary for the acceptance tests. It should be in a similar place to the release binary:

COPY --from=release /opt/zebrad/target/release/zebrad /usr/local/bin

(It won't be in the debug directory, because we build tests with --release.)

I think one of the goals of the ticket is automating this manual check from the Zebra release process:

- [ ] Test the Docker image using `docker run --tty --interactive zfnd/zebra:1.0.0-rc.<version>` <!-- TODO: replace with `zfnd/zebra` when we release 1.0.0 -->

Is it possible to run this test on the release Docker image instead?
If it's not, or it's a lot of effort, that's ok. The test will still cover most of what we need.

@teor2345
Copy link
Contributor

teor2345 commented Jan 20, 2023

If you run docker run zfnd/zebra does it generate output or starts syncing? (I'm in a Mac with M1 and it doesn't, and the ones deployed on GCP are not syncing either)

Since this PR changes the entry point script, I'm not sure if running the main branch image will help us here.

Here is the output from Run tests using the $ZEBRA_CONF_PATH:

+ exec zebrad -c zebra/zebrad/tests/common/configs/v1.0.0-rc.2.toml start
error: Zebra could not parse the provided config file. This might mean you are using a deprecated format of the file. You can generate a valid config by running "zebrad generate", and diff it against yours to examine any format inconsistencies.
error: zebrad fatal error: config error: path error: zebra/zebrad/tests/common/configs/v1.0.0-rc.2.toml

https://github.com/ZcashFoundation/zebra/actions/runs/3963766811/jobs/6792068479#step:3:174

It looks like the config file isn't at the expected path - where is the Zebra git checkout in the Docker image?

Also, tee isn't echoing the output before it exits - since tee works with longer "docker logs" output and with other tools, it could be a bug in "docker logs" where it doesn't send an end of file or flush its internal buffers before finishing. But printing all the logs at the end seems like enough for now. I don't think we need any further fixes for the log output.

Co-authored-by: teor <teor@riseup.net>
@gustavovalverde
Copy link
Member Author

Since this PR changes the entry point script, I'm not sure if running the main branch image will help us here.

The main branch does not build the Docker image with the entrypoint (is not included in the runtime stage), so this PR won't have an impact there, even after merged.

https://github.com/ZcashFoundation/zebra/actions/runs/3963766811/jobs/6792068479#step:3:174

It looks like the config file isn't at the expected path - where is the Zebra git checkout in the Docker image?

Right here https://github.com/ZcashFoundation/zebra/blob/feat-conf-test/docker/Dockerfile#L105

This are the result of a pwd and ls
image

@gustavovalverde gustavovalverde marked this pull request as ready for review January 23, 2023 01:05
@teor2345 teor2345 dismissed their stale review January 23, 2023 01:15

Requested changes were made

teor2345
teor2345 previously approved these changes Jan 23, 2023
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

This looks like it should work.

Did you want to add the new jobs to the branch protection rules after it merges?

@teor2345
Copy link
Contributor

We'll also need to update the patch workflows with the new jobs 🙂

@gustavovalverde
Copy link
Member Author

@teor2345 sorry that I dismissed your review with this change

Please have a look at it, as this will ensure we test the Dockerfile on both stages: tests and runtime, as the build process is different and this ensures it works in both cases.

@gustavovalverde
Copy link
Member Author

Manual deployment to test runtime stage https://github.com/ZcashFoundation/zebra/actions/runs/3982611624

Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for adding that extra test!

We'll have to manually check the test status in the CD workflow, but that's ok, that's what we're doing for everything else in that workflow anyway.

mergify bot added a commit that referenced this pull request Jan 23, 2023
mergify bot added a commit that referenced this pull request Jan 23, 2023
@mergify mergify bot merged commit 85bcbbd into main Jan 23, 2023
@mergify mergify bot deleted the feat-conf-test branch January 23, 2023 06:42
@teor2345
Copy link
Contributor

I added these new jobs to the main branch protection rules:

  • Test Zebra default Docker config file
  • Test Zebra custom Docker config file

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-devops Area: Pipelines, CI/CD and Dockerfiles C-feature Category: New features C-testing Category: These are tests C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG I-usability Zebra is hard to understand or use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Docker tests for default config and $ZEBRA_CONF_PATH
2 participants