-
Notifications
You must be signed in to change notification settings - Fork 43
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
support multiple shas #88
Conversation
Also the /graphql sticks in the first commit, which is undesired. There are also some debugging messages that need to be cleaned up.
The ttl is not currently enforced (no cache invalidation yet). Additionally some methods have been fixed in order to obtain data from the latest bundles.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from just reading the description i understand that you thought of every scenario i could think of.
this is amazing work.
LGTM
qontract-reconcile PR: app-sre/qontract-reconcile#1135 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an impressive job. A few comments and suggestions here and there to the best of my knowledge (which is extremely limited in the js world)
One thing I don't particularly like is breaking changes... Especially since we're doing GET
s with query
params, e.g. https://github.com/openshift/aws-account-operator/pull/493/files, and it makes sense that we're able to do a select with a query string via GET
(on top of the POST
support, I'd do both)
As a request, I'd add part of this great and very helpful description in the documentation of the repository :)
Reimplemented and updated PR description. |
And don't hate me too much, but please convert the relevant bits of the description of this PR into proper documentation. |
@rporres I have updated the README.md, it has been neglected so thanks for pointing it out. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great stuff
Part of https://issues.redhat.com/browse/APPSRE-2608
This is a major PR that introduces a major feature and modifies the internal mechanics of the server.
For consistent access, qontract-server clients can connect to it via a specific
<sha>
using this url:/graphqlsha/<sha>
. If the data is reloaded thesha
will also be renewed, and the server will start returning409
indicating that there is a conflict and that it needs to fetch a newsha
.This has a negative impact on the reconciliation times for the integrations that query this server. Every time the data is reloaded, the integration is interrupted and has to start afresh. When the data is changed very frequently, the state may never be reconciled.
The goal of this PR is to add support for multiple shas. Each time the data is reloaded, the new bundle is exposed on its new
<sha>
(with the same well-known url/graphqlsha/<sha>
thus maintaining backwards compatibility), but previous shas should continue to work. Note that/graphql
will consistently point at the latest bundle, which is useful for casual access to the server, not for integrations.Cache Invalidation
In order to guarantee memory efficiency, the shas will expire after a certain amount of time:
BUNDLE_SHA_TTL
environment variable).BUNDLE_SHA_TTL
in the future again. This means that shas can be kept available forever by querying them before theBUNDLE_SHA_TTL
has passed./graphql
will never expire./reload
is queried./reload
is called and there is no new data available, then no shas will be expired.Method changes
GET /sha256
: (existed previously) Now it specifically points to the latest bundle. No backwards compatibility conflicts.GET /git-commit
: (existed previously) Now it specifically points to the latest bundle. No backwards compatibility conflicts.GET /git-commit/:sha
: (new method) It is possible to obtain now the git-commit for a specific sha.GET /cache
: (new method) Prints the cache state.Metrics
The metrics have been refactored to a separate file
metrics.ts
.There are also new metrics introduced:
These new metrics are useful in order to ensure that there are no memory leaks. Note that
qontract_server_cache_bundle
andqontract_server_cache_bundle_cache
should always coincide in number.Breaking changes
There are no breaking changes. This PR preserves backwards compatibility. However, there are two minor changes that are worth mentioning.
The server will no longer send
409
on any events.409
indicates conflict, which is something that no longer applies as the shas will continue to be available. If the client queries a non-existing sha, or an expired one, it will receive404
..GET /graphql
which was undocumented and just a corner case of the poorly defined middleware that existed previously will no longer redirect toPOST /graphql
. This was never the intention, so it has been removed. The tests however have been rewritten to properly send POST data, instead of performing GET queries as they were incorrectly doing beforeTesting
The functionality of the previously existing tests has remained untouched (except for renaming the
clusters
directory toschemas
which is the proper name).A new set of tests have been included in this PR which test the multiple shas functionality, and the bundle sha expiration and invalidation:
Things to look out for
Memory leaks. An important part of this PR resides in the logic that removes the expired shas. There is a new set of tests that verify that data is properly purged when the bundle shas are removed. However, this needs to be carefully reviewed.
Every time that a new bundle is added it is mounted into the express server with this directive:
app.use(serverMiddleware);
. This is anexpress
framework method call that pushes the middleware to the internal router stack. The challenge is that there is no documented / supported way to remove a middleware instance, which is precisely what we want to do when bundles expire. What we have done in this case is to access the private router stack (app._router.stack
) and splice it in order to remove the relevant middleware. This is somewhat dangerous and may break if theexpress
framework is upgraded (regardless if it's a major or minor update).The ApolloServer package has been upgraded. This is because the schema hot-reload internals have changed. This specifically affects the GraphQL inline fragments. However, this was a minor version upgrade, and it didn't affect the tests, so it is of minor importance.
EDIT: Reimplemented GET /graphql -> POST /graphql