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

M1 compatibility #581

Merged
merged 4 commits into from
Apr 14, 2022
Merged

M1 compatibility #581

merged 4 commits into from
Apr 14, 2022

Conversation

kevindew
Copy link
Member

@kevindew kevindew commented Mar 17, 2022

This PR tries to pull together all the fixes we know are needed for using GOV.UK docker on an M1 mac using information from: #561

This is marked as a draft as I'm not sure if we'll merge these things in directly, I expect we'd probably do some more testing and then consider adding some explanatory comments to code.

I've done some testing now and other M1 mac users are making use of this branch. I've also confirmed it allows database restores for MySQL, MongoDB and Elasticsearch.

@@ -41,7 +41,7 @@ services:
- "28017:28017"

mysql-8:
image: mysql:8
image: mysql:8-oracle
Copy link
Contributor

Choose a reason for hiding this comment

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

For anyone who uses this branch, you'll need to make sure you use Docker's debug feature to purge all the images before you attempt using MySQL. Without doing so, you will have odd behaviour in regards to Whitehall connectivity and replicating data.

@DilwoarH
Copy link
Contributor

Just tested Whitehall with this, ran fine 👍

docker-compose.yml Outdated Show resolved Hide resolved
@KludgeKML
Copy link
Contributor

KludgeKML commented Apr 5, 2022 via email

KludgeKML and others added 4 commits April 11, 2022 16:11
Unlike mysql:8 the 8-oracle tag has support for AMD64 and Arm64 chipsets
so can work with an M1 mac.

I've not tested all MySQL functionality (db restore etc) but I assume
it'll work since it isn't very different at all.
This replaces all usages of elasticsearch-6 with elasticsearch-7, as per
the suggestion Keith made in: #561 (comment)

I've not actually tested any of this functionality so can't guarantee it
works.
This leaves comments to record the cases where we've made an exceptions
in configuration in order to stick to using images that are suitable
with ARM64 machines (typically M1 Macs).
@kevindew
Copy link
Member Author

So, in short (because this is something I've been on a deep dive into recently): Router currently uses a feature called OpTime to find out whether it has to refresh its internal representation of the routing tree. The way Router is written it requires a replicaset (the optime calls basically open up the replicaset, dig down into it to find the primary, then use that as the check). The replicaset can be only 1 service (the primary), but it has to be there - router doesn't have any fallback. So if you're using router in a way on your local machine that needs you never to update the routes, it would work (although it would be chucking out warning messages about not finding the replica primary). If you wanted to update routes, or you wanted to run the router test suite, it would fail without the replicaset. At the moment I'm working on moving router in production to a DocumentDB database, which will require me to get rid of the replicaset requirement. But it's not quite done yet (and has just had a week+ delay due to me getting covid).

Do hope you're feeling better now Keith

Hmm I see, so for that aspect of router to work it needs to be running as a replica set. Whereas for all other apps they don't care if replicaset or not?

From what I can tell just starting mongo with --replSet <replica-set-name> doesn't seem to be enough anymore. You need to also run rs.initiate() in MongoDB directly to start the replicaset (I imagine there's perhaps other ways to do this, such as config files) - so it seems quite the pain.

Do you think we need to wait until(/if) we resolve this before we merge this PR for M1 compatibility?

I feel like, aside from this, all the other bits work.

@KludgeKML
Copy link
Contributor

Short story: I think you should merge this for the moment and we keep a separate branch for working on router until I can update router enough that that's unnecessary.

Long story: I would say that this affects:

  • people actively working on router (AFAIK only me + occasionally some people doing dependabot version bumps)
  • people who want to test things locally that require route creation going all the way through to router

People in group 1 won't be able to run the test suite locally. But it'll still run on Jenkins, and it can be made to run locally with the small change to their local govuk-docker. So I think they will live with that.

People in group 2 won't see routes get updated locally. But - that might not be a thing people care too much about in local dev? I'm also not 100% sure whether this is true, either - is the endpoint on router that triggers an update completely unused now? If some things still use it, maybe that is fine. You have a better idea of how frequent this need comes up, but I guess maybe not a lot?

Either way, the impact of merging this as is seems minimal. It might only ever affect me, and I already know how to work around it. So I'm happy for this to get merged.

Copy link
Contributor

@KludgeKML KludgeKML 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 to me.

docker-compose.yml Show resolved Hide resolved
docker-compose.yml Outdated Show resolved Hide resolved
@kevindew kevindew marked this pull request as ready for review April 13, 2022 16:37
@kevindew kevindew merged commit 6642262 into main Apr 14, 2022
@kevindew kevindew deleted the m1-compatibility branch April 14, 2022 11:11
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.

4 participants