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

Add ADR for porting router and router-api to PostgreSQL #353

Merged
merged 1 commit into from
May 19, 2023

Conversation

chao-xian
Copy link
Contributor

@chao-xian chao-xian commented Apr 17, 2023

* Spin up a completely separate stack of router and router-api (govuk-docker first) that will talk to a PostgreSQL db only (as forks)
* Provision in EKS a new PostgreSQL db
* Provision the new p-router/p-router-api apps
* Amend messages to router-api to be duplicated to p-router-api within the gds-api-adapters gem

Choose a reason for hiding this comment

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

This should be in content-store. The gds-api-adapters gem is just helper methods, it shouldn't have "business" logic in it.

Copy link

@leenagupte leenagupte Apr 18, 2023

Choose a reason for hiding this comment

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

So I'd split this into two steps. One to add helper methods to gds-api-adapter to call p-router-api (this still needs a better name!) and the next step to call the new helper methods from content store.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We did consider doing it in content-store, but felt it much easier to implement and manage (temporarily) in the gem.

Good point on the splitting of steps.

Copy link
Contributor

@Tetrino Tetrino Apr 18, 2023

Choose a reason for hiding this comment

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

@chao-xian the suggestion from me to do the logic in the API (as small as that logic is) was based on the theory that it's one solid point of entry for everything. @leenagupte if content store is the only location that actually accesses these helper calls, we could do it there.

However to clarify, there is no business logic as such in the API adaptor in this plan, we simply append a call to another new psql helper method inside the existing one (as is the plan).

Copy link

@leenagupte leenagupte Apr 18, 2023

Choose a reason for hiding this comment

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

Content Store is the only place that calls router / router-api. You'd be making a bit of a mess in gds-api-adapters if you try to have the router helpers call two places as you'd need to do some work to get it to figure out what the endpoint is.

Copy link
Contributor

@KludgeKML KludgeKML Apr 18, 2023

Choose a reason for hiding this comment

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

My opinion is that either the API or the content store way of doing it would be fine, but the question of where/when to report errors probably makes the decision.

Basically, if we do this splitting we need to be able to know about the following error cases:

  • Neither router-api nor p-router-api got the update (PANIC)
  • both got it (FINE)
  • router-api got it but not p-router-api (FINE, WE HAVEN'T SPUN UP P-ROUTER / OH, WE HAVE TO INVESTIGATE)
  • p-router-api got it but not router-api (UH, WHAT? / FINE, WE'VE TURNED OFF OLD ROUTER)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great point @KludgeKML thanks!

@chao-xian
Copy link
Contributor Author

@aldavidson has suggested running a proxy to duplicate the requests and to be able to log - which is a good idea it seems

@leenagupte
Copy link

@aldavidson has suggested running a proxy to duplicate the requests and to be able to log - which is a good idea it seems

So a new app that routes requests to both versions of router-api? And the proxy would be called by content store?

@chao-xian
Copy link
Contributor Author

@leenagupte yes he's suggesting a new app, and we can use that same proxy to duplicate our calls too

@chao-xian
Copy link
Contributor Author

Link to @aldavidson 's spike alphagov/content-store#1062

@aldavidson
Copy link

Hi both - yes, we're looking at a simple proxy in front of two copies of content-store (one running Mongo, one running Postgres) that would forward all requests to both, and log any differences in the responses.

That will let us dual-run for a while, and build up a decent history of database backups before switching. Also lets us manage the switchover in phases (Mongo primary, PG secondary -> PG primary, Mongo secondary -> just PG -> no more proxy). It's based on the approach taken by The Guardian when they ported their CMS from Mongo to PG.

If you're considering the same, we'll hopefully be able to write the proxy in such a way that it's usable for both migrations

@chao-xian
Copy link
Contributor Author

See also alphagov/govuk-rfcs#158

@chao-xian
Copy link
Contributor Author

@chao-xian
Copy link
Contributor Author

chao-xian commented May 15, 2023

See also spike from Al alphagov/content-store#1062

@chao-xian
Copy link
Contributor Author

@chao-xian chao-xian marked this pull request as ready for review May 17, 2023 14:37
Copy link

@leenagupte leenagupte left a comment

Choose a reason for hiding this comment

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

This sounds good. I think the only thing missing is any risks or consequences and any other approaches considered.

docs/ADR/001-porting-to-postgresql.md Outdated Show resolved Hide resolved
docs/ADR/001-porting-to-postgresql.md Outdated Show resolved Hide resolved
@leenagupte
Copy link

@chao-xian Sorry, it occurred to me later that the Decision and the outline were mislabelled.

@chao-xian chao-xian merged commit 045953c into main May 19, 2023
@chao-xian chao-xian deleted the add-postgresql-adr branch May 19, 2023 15:05
@chao-xian
Copy link
Contributor Author

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.

5 participants