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 rocsp unit test flakes #6071

Closed
aarongable opened this issue Apr 29, 2022 · 6 comments · Fixed by #6073
Closed

Fix rocsp unit test flakes #6071

aarongable opened this issue Apr 29, 2022 · 6 comments · Fixed by #6073
Assignees
Milestone

Comments

@aarongable
Copy link
Contributor

D161252 rocsp-tool.test 88b-5wc SQL:  insert into `certificateStatus` (`id`,`serial`,`status`,`ocspLastUpdated`,`revokedDate`,`revokedReason`,`lastExpirationNagSent`,`ocspResponse`,`notAfter`,`isExpired`,`IssuerID`) values (null,?,?,?,?,?,?,?,?,?,?); [1:"1337" 2:"" 3:0001-01-01 00:00:00 +0000 UTC 4:0001-01-01 00:00:00 +0000 UTC 5:0 6:0001-01-01 00:00:00 +0000 UTC 7:[] 8:1970-01-01 12:00:00 +0000 UTC 9:false 10:0] (1.433177ms)
D161252 rocsp-tool.test qYyKowg SQL:  insert into `certificateStatus` (`id`,`serial`,`status`,`ocspLastUpdated`,`revokedDate`,`revokedReason`,`lastExpirationNagSent`,`ocspResponse`,`notAfter`,`isExpired`,`IssuerID`) values (null,?,?,?,?,?,?,?,?,?,?); [1:"1338" 2:"" 3:0001-01-01 00:00:00 +0000 UTC 4:0001-01-01 00:00:00 +0000 UTC 5:0 6:0001-01-01 00:00:00 +0000 UTC 7:[] 8:1970-01-02 12:00:00 +0000 UTC 9:false 10:0] (988.953µs)
--- FAIL: TestStoreResponse (0.12s)
    client_test.go:115: storing response: storing response: transaction failed: got 4 elements in cluster info address, expected 2 or 3
FAIL
FAIL	github.com/letsencrypt/boulder/cmd/rocsp-tool	0.543s

and

--- FAIL: TestSetAndGet (0.01s)
    rocsp_test.go:50: storing response: transaction failed: got 4 elements in cluster info address, expected 2 or 3
FAIL
FAIL	github.com/letsencrypt/boulder/rocsp	0.042s
@aarongable aarongable added this to the 2022-04-26 milestone Apr 29, 2022
@aarongable
Copy link
Contributor Author

It's failing this way in multiple PRs:
https://github.com/letsencrypt/boulder/runs/6232644947?check_suite_focus=true
https://github.com/letsencrypt/boulder/runs/6231447684?check_suite_focus=true

But I cannot reproduce it locally, neither on main nor with either of those PRs checked out.

@aarongable
Copy link
Contributor Author

The lowest error message comes from here:

err := fmt.Errorf("got %d elements in cluster info address, expected 2 or 3", n)

@aarongable
Copy link
Contributor Author

aarongable commented Apr 29, 2022

I updated my local redis docker image from aea9b698d7d1 (4 months old) to a10f849e1540 (2 days old, and the same version being run in github workflows and codespaces). I now reliably reproduce the failure. So it's something to do with the redis docker image version, not anything to do with our Boulder (or dependencies) code changing.

There is no more recent version of the go-redis/redis dependency available that might fix this.

@aarongable
Copy link
Contributor Author

It's a major version update. We've always been specifying "redis:latest" in our docker-compose.yml:

image: redis:latest

As of two days ago, the official "redis" docker image has released version 7.0.0, and updated their "latest" tag to point at it:
image

Because github workflows (and github codespaces) rebuild the environment from scratch every time, they get the true latest version every time. Most of the rest of us, whose local docker installations cache images, stick with the same version until we explicitly run a docker image pull redis:<version> to update it. This explains why we couldn't reproduce the error locally.

I ran docker image pull redis:7.0.0, and updated our docker-compose.yml to specify the same version, and was immediately able to reproduce the failures locally.

I then ran docker image pull redis:6.2.7 (the most recent non-v7 version) and did the same, and the errors went away.

I intend to fix this by pinning our redis to 6.2.7, and filing a bug against the go-redis/redis library to properly support talking to a v7 redis image.

@aarongable
Copy link
Contributor Author

Here are the Redis 7.0.0 release notes: https://raw.githubusercontent.com/redis/redis/7.0/00-RELEASENOTES

Of particular interest, they mention:

Command replies that have been extended

  • CLUSTER SLOTS hostname support (#9530)

That's the failing command, and "extending" it sounds exactly like giving it a fourth field, as the error message indicates. The referenced PR does exactly that.

@aarongable aarongable self-assigned this Apr 29, 2022
@aarongable
Copy link
Contributor Author

Upstream issue filed: redis/go-redis#2082

@jsha jsha closed this as completed in #6073 May 2, 2022
jsha pushed a commit that referenced this issue May 2, 2022
Redis recently released version 7.0.0, which has several breaking
changes. The go-redis library that we rely on does not yet support
communicating with a Redis 7.0.0 cluster.

Pin ourselves to the latest non-7.0.0 version, 6.2.7, until such time
as go-redis releases a version with support for 7.0.0.

Fixes #6071
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 a pull request may close this issue.

1 participant