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

feat: Dns reconciler queue #447

Merged
6 commits merged into from Mar 27, 2023
Merged

feat: Dns reconciler queue #447

6 commits merged into from Mar 27, 2023

Conversation

ghost
Copy link

@ghost ghost commented Mar 26, 2023

  • Moves the complete reconciler to under queues
  • Creates a queue for it with automatic repeat. It makes certain that only 1 repeat job can exist
  • Adds a new table, Uses a unique enum so only ever 1 row can exist in the table
  • Alters the reconciler main code, so that it only runs if there is no concurrency problem (because of multiple instances in the swarm), and it is requested to run
  • Introduces 'limp mode'. If bulk update fails, it will try to run it one by one, and it points the offending update out in the logs

Closes #396

@ghost ghost added this to the Milestone 0.9 milestone Mar 26, 2023
Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

This is great. I love normal vs. limp mode! Lots of great problem solving and interesting patterns in here.

I've left a bunch of questions and comments.

prisma/schema.prisma Outdated Show resolved Hide resolved
prisma/schema.prisma Outdated Show resolved Hide resolved
prisma/schema.prisma Outdated Show resolved Hide resolved
prisma/seed.ts Show resolved Hide resolved
app/models/system-state.server.ts Show resolved Hide resolved
app/queues/reconciler/reconciler-worker.server.ts Outdated Show resolved Hide resolved
app/root.tsx Outdated Show resolved Hide resolved
@ghost ghost requested a review from humphd March 26, 2023 22:33
@ghost
Copy link
Author

ghost commented Mar 26, 2023

@humphd Thank you! Updates pushed, and I'll ask for some help with the e2e tests.
My only feeling is that it might be failing because of the updated database schema, but the video shows it standing on the main screen (before login) not doing anything.

No rush, we can talk about it tomorrow! Again, thanks for the very quick reviews!

@Genne23v Genne23v assigned ghost Mar 26, 2023
app/root.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@Genne23v Genne23v left a comment

Choose a reason for hiding this comment

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

The code is well organized and comments are easy to understand. I added only one comment. Feel free to update or keep it as is.

@ghost ghost requested review from humphd and Genne23v March 27, 2023 01:16
Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

Debugging this locally, the following change gets the e2e tests to pass for me locally:

diff --git a/app/root.tsx b/app/root.tsx
index a4a8f08..89550eb 100644
--- a/app/root.tsx
+++ b/app/root.tsx
@@ -12,7 +12,6 @@ import {

 import { getUser } from './session.server';
 import theme from './theme';
-import { addReconcilerJob } from './queues/reconciler/reconciler-queue.server';

 import type { MetaFunction, LoaderArgs, LinksFunction } from '@remix-run/node';

@@ -37,8 +36,6 @@ export async function loader({ request, context }: LoaderArgs) {
   });
 }

-addReconcilerJob();
-
 function Document({ children }: { children: React.ReactNode }) {
   // We need the nonce generated by the server via helmetjs to put on scripts
   const { nonce } = useLoaderData<typeof loader>();
diff --git a/server.ts b/server.ts
index a604867..7765fd0 100644
--- a/server.ts
+++ b/server.ts
@@ -8,6 +8,7 @@ import helmet from 'helmet';
 import cors from 'cors';

 import logger from '~/lib/logger.server';
+import { addReconcilerJob } from '~/queues/reconciler/reconciler-queue.server';

 import type { Request, Response } from 'express';

@@ -81,6 +82,8 @@ const port = process.env.PORT || 8080;
 const server = app.listen(port, () => {
   // require the built app so we're ready when the first request comes in
   require(BUILD_DIR);
+  // start the DNS reconciler
+  addReconcilerJob();
   logger.info(`✅ app ready: http://localhost:${port}`);
 });

@ghost
Copy link
Author

ghost commented Mar 27, 2023

@humphd Thanks for the suggestion! It seems to have solved the issue!

@ghost ghost requested a review from humphd March 27, 2023 15:24
@ghost
Copy link
Author

ghost commented Mar 27, 2023

@humphd When you are around, can I get an approve if all is well?

Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

🚢 ship it!

@ghost ghost merged commit ca739e0 into DevelopingSpace:main Mar 27, 2023
@ghost ghost deleted the reconciler-queue branch March 27, 2023 22:52
Genne23v pushed a commit to Genne23v/starchart that referenced this pull request Mar 29, 2023
* feat: Dns Reconciler queue

* fix: init

* fix: comments

* improvements

* small fixes

* fix: error during testing Credit: @humphd
This pull request was closed.
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.

DNS reconciler
5 participants