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

Updating routes in Router #135

Merged
merged 1 commit into from Mar 11, 2021
Merged

Updating routes in Router #135

merged 1 commit into from Mar 11, 2021

Conversation

karlbaker02
Copy link
Contributor

@karlbaker02 karlbaker02 commented Feb 12, 2021

Rendered

Deadline: 2021-03-05

Copy link
Contributor

@thomasleese thomasleese left a comment

Choose a reason for hiding this comment

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

This seems like a good approach, I've left a couple of comments on the text of the RFC itself.

rfc-135-updating-routes-in-router.md Outdated Show resolved Hide resolved

Router will keep its in-memory cache of routes up-to-date via a two stage process:

1. Router will perform a simple query against MongoDB returning the state of the database with the current set of routes; ideally this will be a query which contains a checksum or timestamp of last update which can be used by Router instances to check whether their local copy of routes is up-to-date.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have any more specifics on how this query might work? I guess we don't want to come up with a solution in the RFC, but I think a little more might be helpful to decide whether the overall idea is a good idea or not. (For example, I'm imagining we might need to think a little bit about database indexes or how long it would take to generate said checksum, etc).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't sure how far to go into detail here, as like you say, we haven't done any real research into this and I imagine it'll be something we look at when it comes to implementation. If we're calling a method such as db.stats() (or using some other method which returns aggregates) then we might not need to worry about manually creating indexes etc as the queries should already be performant.

On the note of how long it takes to query, so long as the queries return results in a short (and reasonable) amount of time I feel we will be fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have added an additional paragraph about this, let us know your thoughts.

Copy link
Contributor

Choose a reason for hiding this comment

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

A couple more ideas to demonstrate that the approach is feasible: collection.dbHash, or Router API could write a timestamp when the routes collection changes and not use a hash.

Copy link
Contributor

@sengi sengi Feb 25, 2021

Choose a reason for hiding this comment

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

(Again, sorry for thread necromancy but seemed like the most relevant place to continue the discussion.)

To echo Tom's point, I think the details of the polling mechanism are quite crucial here and to defer their consideration until later as a mere implementation detail would be a mistake. The concept may be simple, but how Router API denotes that there's been a change is a different matter and I think it reduces to a non-trivial classical concurrency problem.

For example, having a "last updated" timestamp on the database would be super convenient for implementing the polling loop in Router, but how would Router API update that timestamp? Just write to a specific record in the database each time a change happens? That means all writes will contend for the same lock. Maybe that's fine, say if we're only writing at most a handful of times a second. How frequently do updates happen in the worst case? (Are there bulk republishing operations which generate a lot of changes all at once, for example?)

I'm pretty sure hashing is a red herring - reading through the entire dataset on every write to compute the hash would be infeasible, and doing something incremental involves fancy techniques which Mongo almost certainly doesn't implement. I don't think it's necessary either - a counter or a timestamp would do just fine (and even that is challenging enough!)

There are a couple of things in our favour. Firstly, Mongo already implements replication, so we may be able to leverage that (for example perhaps the replication counters are suitable for detecting when there's been an update). Secondly, we only need eventual consistency and we don't care about linearizability (I'm fairly sure we don't, anyway).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are some options here:

  • db.stats doesn't return any timestamp or serial number or anything as you rightly suggest. If we were to use this method to poll then the most likely candidate property that we'd be able to hook into to check for changes would probably be objects, which gives us the total number of objects. This could stay consistent whilst underlying data changes, say if we remove a bunch of routes and add a bunch of routes simultaneously, though how likely this is in reality I'm not 100% certain.

  • db.collection_name.stats() returns pretty much the same as db.stats() but limited to a particular collection, so again the most likely candidate from here would probably be objects, with the same caveats as mentioned previously.

  • db.runCommand ( { dbHash: 1 } ) returns the hash values of the collections in a database and an MD5 value for these collections. dbHash is useful to compare databases across mongod instances, such as across members of replica sets. This gives us a useful detail to hook into, and we can either do so on the individual collections we're interested in or for the database as a whole via the MD5 value for all the collections.

Example output of:db.runCommand ( { dbHash: 1 } ) for the Router database:

{
	"numCollections" : 7,
	"host" : "...",
	"collections" : {
		"backends" : "6c6fe3e7c7cc830d8f9bd557d7778b79",
		"data_migrations" : "de19151119786e79f1449e4cd458324a",
		"routes" : "d7922096b260e5490e26523016a79ee2",
		"users" : "6a0a8ca16bd2970a853750b29f08c32e"
	},
	"md5" : "b6c592489cd1e99e1262e6d84617e5e8",
	"timeMillis" : 1039,
	"fromCache" : [ ],
	"ok" : 1
}
  • rs.status() returns the status of the [nodes of the] replica set and doesn't really feel useful in any way to determine whether there have been any updates to the actual data within the database (vs the secondaries that we have in place mirroring the primary).

Example of rs.status() on the Router database (partly redacted):

...
{
	"name" : "router-backend",
	"health" : 1,
	"state" : 2,
	"stateStr" : "SECONDARY",
	"uptime" : 9755060,
	"optime" : Timestamp(1615302470, 4),
	"optimeDate" : ISODate("2021-03-09T15:07:50Z"),
	"lastHeartbeat" : ISODate("2021-03-09T15:29:38Z"),
	"lastHeartbeatRecv" : ISODate("2021-03-09T15:29:37Z"),
	"pingMs" : 0,
	"syncingTo" : "another-router-backend"
}
...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another option we have is to use the primary machine's oplog. From the docs...

  • The oplog (operations log) is a special capped collection that keeps a rolling record of all operations that modify the data stored in your databases.

  • MongoDB applies database operations on the primary and then records the operations on the primary’s oplog. The secondary members then copy and apply these operations in an asynchronous process. All replica set members contain a copy of the oplog, in the local.oplog.rs collection, which allows them to maintain the current state of the database.

  • Each operation in the oplog is idempotent.

From the oplog, we can call the method rs.printReplicationInfo() to get the timestamp of the last update to the primary (oplog last event time) which can then be used to poll for changes in Router instances.

Example output of rs.printReplicationInfo() on the Router database:

rs.printReplicationInfo()
configured oplog size:   990MB
log length start to end: 130748secs (36.32hrs)
oplog first event time:  Mon Mar 08 2021 03:24:12 GMT+0000 (UTC)
oplog last event time:   Tue Mar 09 2021 15:43:20 GMT+0000 (UTC)
now:                     Tue Mar 09 2021 16:21:54 GMT+0000 (UTC)

Importantly, as the oplog is a log of all operations (modifications) performed on the primary, there is no need to perform a table scan or other high-workload operation on the primary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Diving more deeply into the the possible concerns presented earlier around race conditions, I've looked into the updates happening on prod, using MongoDB's oplog (described above).

The oplog contains a special internal BSON timestamp reserved for use by MongoDB, which contains both a timestamp of an entry in the oplog and an ordinal, which auto-increments for operations within a given second. This means that MongoDB does keep track of all operations that are applied at the same "wall-clock" second and the order in which they're applied. This can be seen in the small snippet below:

{ "ts" : Timestamp(1615368615, 1), "h" : NumberLong("-8363688210986405942"), "v" : 2, "op" : "i", "ns" : "router.routes", "o" : { "_id" : ObjectId("604891a7f482b26f5519ad86"), "incoming_path" : "/government/statistics/port-freight-quarterly-statistics-october-to-december-2020/port-freight-quarterly-statistics-october-to-december-2020", "disabled" : false, "route_type" : "exact", "handler" : "backend", "backend_id" : "government-frontend", "updated_at" : ISODate("2021-03-10T09:30:15.166Z"), "created_at" : ISODate("2021-03-10T09:30:15.166Z") } }
{ "ts" : Timestamp(1615368614, 2), "h" : NumberLong("-8522173943813702296"), "v" : 2, "op" : "i", "ns" : "router.routes", "o" : { "_id" : ObjectId("604891a63888ad51c2a5f11c"), "incoming_path" : "/government/publications/intensive-personalised-employment-support-provider-guidance/covid19-chapter-6-pre-work-and-in-work-support", "disabled" : false, "route_type" : "exact", "handler" : "backend", "backend_id" : "government-frontend", "updated_at" : ISODate("2021-03-10T09:30:14.936Z"), "created_at" : ISODate("2021-03-10T09:30:14.936Z") } }
{ "ts" : Timestamp(1615368614, 1), "h" : NumberLong("-3842222477273027033"), "v" : 2, "op" : "i", "ns" : "router.routes", "o" : { "_id" : ObjectId("604891a63888ad51c1b1fdf8"), "incoming_path" : "/government/publications/intensive-personalised-employment-support-provider-guidance/covid-19-chapter-5-working-with-third-parties-and-participant-support", "disabled" : false, "route_type" : "exact", "handler" : "backend", "backend_id" : "government-frontend", "updated_at" : ISODate("2021-03-10T09:30:14.431Z"), "created_at" : ISODate("2021-03-10T09:30:14.431Z") } }

The concern described earlier around race conditions was that if a Router instance were to use the oplog last event time timestamp as the basis to decide whether or not to update it's cache of in-memory routes from MongoDB, it could in theory be out of sync with the most up-to-date data if multiple updates were applied within the same "wall-clock" second by MongoDB but at the time of polling, the Router instance only saw some of those updates (which were the latest at the time of read). This would mean that the remaining updates applied at that time would not be pulled down by the Router instance when it next polled, as the timestamp it holds would be the same as the timestamp returned from rs.printReplicationInfo() for oplog last event time.

Such a scenario, should it occur, would really only be noticeable in times of lower frequency updates to Routes, which is generally outside of the 9-5 working day. A possible workaround for said scenario could be to force an update of the cache of routes within Router, once, after five or so minutes of polling without seeing any updates; the idea behind this proposal is that if routes aren't going to be updated for several hours, then performing an update after five minutes of inactivity ensures everything is up-to-date and actually consistent. During the working day, it's unlikely we'd ever force the reload as routes are updated at least several times per minute.

It's worth mentioning that there is probably some inherent delay with the current set-up of MongoDB too. We have a primary MongoDB instance and a number of secondaries, working together as part of a replica set. When updates are processed by the primary, the primary writes to it's oplog and it is this oplog which is copied across to the secondaries and which are used by those secondaries to ensure that they're consistent with the primary. A small lag between different secondaries updating from the oplog, as well as the small lag in copying the updates from the primary oplog to the secondary oplogs, could in theory mean that a request to one Router instance returns a slightly different result to the same request to another Router instance at a particular point in time. The effects of this lag are most likely negligible due to how we host MongoDB but are worth calling out nonetheless.

Copy link
Contributor

Choose a reason for hiding this comment

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

Aha, so I was talking nonsense about the 1-second precision being a problem. We can just compare Timestamp objects from rs.status() and we should be fine?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to clarify what's being said here, we could use the optime timestamp that's returned when calling rs.status(), which is the internal MongoDB BSON timestamp. Importantly though, it does give us the actual timestamp and the ordinal which together form the most recent update to a given database instance, whether the primary or one of the secondaries. The format it returns this in is:

...
"optime" : Timestamp(1615383267, 4),
...

We are given this timestamp for all members of the replica set, so as long as we're consistent about which replica we're reading from, Router instances should be able to use this value when polling to determine whether there are any new route updates to fetch and if so, to reload it's routes from MongoDB. This timestamp gives us confidence that there will be no race conditions of the kind described previously, so there's also no need to do any force refreshes of routes in Router.

Copy link
Contributor

@bilbof bilbof left a comment

Choose a reason for hiding this comment

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

I think this approach will work, thanks Karl 👍

Confusingly, we have both Router API calling Router instances, and Router instances calling Router API's DB, so it'll be good to remove one of those requests. It's a simpler design you've proposed 👏

It would be nice to know how many router instances concurrently refreshing routes (a full reload) it'll take to overload the MongoDB instance. But we can tweak this design in future if we need to deal with scale mismatch. For example, Router API could push the config to an S3 bucket. There's a good article on scale mismatch in designs like this here.

I'd also be interested if you have thoughts on how Router polling MongoDB for changes might work?

rfc-135-updating-routes-in-router.md Outdated Show resolved Hide resolved
rfc-135-updating-routes-in-router.md Outdated Show resolved Hide resolved
rfc-135-updating-routes-in-router.md Outdated Show resolved Hide resolved
@karlbaker02 karlbaker02 changed the title Add RFC for updating routes in Router [NOT YET READY FOR DISCUSSION] Add RFC for updating routes in Router Feb 17, 2021
@karlbaker02 karlbaker02 marked this pull request as ready for review February 18, 2021 12:09
@karlbaker02 karlbaker02 changed the title [NOT YET READY FOR DISCUSSION] Add RFC for updating routes in Router Add RFC for updating routes in Router Feb 18, 2021
@karlbaker02 karlbaker02 changed the title Add RFC for updating routes in Router Updating routes in Router Feb 18, 2021
@timblair
Copy link
Member

We have an existing tech debt card and a (non-accepted) RFC about doing away with router-api entirely. How does this concept relate to what you're proposing?

@richardTowers
Copy link
Contributor

We have an existing tech debt card and a (non-accepted) RFC about doing away with router-api entirely. How does this concept relate to what you're proposing?

It's mentioned in the other options we considered.

We still think that's a good thing to be doing, but it doesn't actually solve this problem. It just moves the problem from "router-api needs to contact individual router instances" to "content-store needs to contact individual instances".

We could fix both things at the same time (i.e. poll content store's DB instead of router's DB, remove router-api and its DB entirely). But I think it's quite a bit more work. Still worth considering though.

@timblair
Copy link
Member

🤦 My brain completely skipped over that part of the proposal.

I agree that it doesn't solve the current problem, but I was wondering if it's worth considering as part of this to remove that extra bit of infrastructure (router-api) which is essentially only there to provide an intermediary between the content store and the router.

@richardTowers
Copy link
Contributor

Let's do a quick 👍🏻 👎🏻 poll to see who prefers which option:

👍🏻 - clean up the architecture. Remove router-api, have router instances poll content-store's DB
👎🏻 - keep it simple for now. Keep router-api, have router instances poll router-api's DB.

@timblair
Copy link
Member

I've voted for "clean up the architecture," but I'm not convinced that we actually know what that means, or if it's the right thing to do. What I would like to see is some further thinking as to what the implications would be here. If we're doing significant work on the Router / Router API, I want to make sure that we're making the best use of that effort.

Questions to answer would include:

  • Is it reasonable / desirable for the Content Store to be the source of truth for Router, and what would be the impact if it was?
  • Are there any edge cases where an app talks directly to the Router API, or other times where something isn't registered in the Content Store in a way which would allow suitable, complete view of routes.

Copy link
Contributor

@ChrisBAshton ChrisBAshton left a comment

Choose a reason for hiding this comment

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

LGTM 👍 nice write-up!

rfc-135-updating-routes-in-router.md Outdated Show resolved Hide resolved
rfc-135-updating-routes-in-router.md Outdated Show resolved Hide resolved
rfc-135-updating-routes-in-router.md Outdated Show resolved Hide resolved
This commit adds an RFC for updating routes in Router. This RFC is necessary as we're updating how we host Router, such that the current method of updating routes will no longer work, so need to propose an alternative.
@karlbaker02 karlbaker02 merged commit 876965f into main Mar 11, 2021
@karlbaker02 karlbaker02 deleted the updating-routes-in-router branch March 11, 2021 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants