gergo/previews #3765

gjedlicska · 2025-01-06T13:49:18Z

Description & motivation

Limitations with the existing preview service:

The existing preview service worked by directly querying the database of the server. This blurred some segregation of responsibility between micro-services (and violated the database-per-microservice rule).
The preview service was given full read/write permissions to the database.
The preview service was limited to querying only one database, so each deployment required at least one dedicated preview-service.
As the preview-service requires significant allocation of memory and CPU, it is an expensive component. In low-traffic deployments it was not being fully utilised, resulting in unused & wasted resources. The cost-per-preview-generated was high and not optimal.
The preview service could not be run in a shared (one preview service for multiple deployment) mode because it required privileged access to each deployment.
The preview service could not easily be hosted on separate infrastructure because it required access to the database.
The preview service had to implement much of the objects API, duplicating the same in the server.
The preview service had limited or no unit tests.

This PR aims to resolve these shortcomings

Changes:

the preview frontend is now a new app, that exposes specific interface implementing functions via the window object, that we can use for easier integration testing of the viewer to the preview generation script
the preview service now does not have any access to the postgres DB, it gets a project scoped token in the job payload, and is now communicating with the preview frontend with the shared interface functions, so the app boundaries are now very clear and easy to understand
the server is responsible for storing the preview job results into the project specific regional DB-s
the server can rely on a dedicated redis queue for the preview jobs, and it specifies its own dedicated response queue. This allows us to run one dedicated cluster of preview services on a beefy gpu enabled machine, and they can generate previews for multiple server deployments securely

To-do before merge:

Screenshots:

Validation of changes:

Checklist:

My pull request follows the guidelines in the Contributing guide?
My pull request does not duplicate any other open Pull Requests for the same update/change?
My commits are related to the pull request and do not amend unrelated code or documentation.
My code follows a similar style to existing code.
I have added appropriate tests.
I have updated or added relevant documentation.

References

…gergo/previews

iainsproat · 2025-01-06T15:11:44Z

packages/preview-service/Dockerfile

@@ -125,4 +125,4 @@ RUN apt-get update && \
 # Run everything after as non-privileged user.
 USER pptruser

-ENTRYPOINT [ "tini", "--", "node", "--loader=./dist/src/aliasLoader.js", "bin/www.js" ]
+CMD [ "tini", "--", "node", "dist/main" ]


If we're changing this, and to be pedantic, entrypoint should be the binary being executed and cmd the arguments to that statement. So:

ENTRYPOINT [ "tini", "--", "node"] CMD ["dist/main"]

And is there a reason for using dist/main pattern over the bin/www.js pattern?

there is no reason to have that www.js file. For the server, there is some boostrapping that is useful, here its not needed.

iainsproat · 2025-01-06T15:14:05Z

packages/preview-service/_bak/src/aliasLoader.ts

What's the _bak directory? Is it a backup to be removed?

iainsproat · 2025-01-06T15:14:30Z

packages/preview-service/_bak/src/repositories/previews.ts

@@ -1,4 +1,4 @@
-import type { Preview } from '@/domain/domain.js'
+import type { Preview } from '../domain/domain.js'


Can we retain the alias pattern please? i.e. @/domain/domain.js would be preferable.

iainsproat · 2025-01-06T15:15:34Z

packages/preview-service/_bak/src/server/background.ts

@@ -12,7 +12,7 @@ import {
  notifyUpdateFactory,
  updatePreviewMetadataFactory
 } from '@/repositories/objectPreview.js'
-import { insertPreviewFactory } from '@/repositories/previews.js'
+import { insertPreviewFactory } from '../repositories/previews.js'


Again, the alias pattern should be used.

iainsproat · 2025-01-06T15:15:51Z

packages/preview-service/_bak/src/services/360preview.ts

-import type { InsertPreview } from '@/repositories/previews.js'
+import type { GeneratePreview } from '../clients/previewService.js'
+import type { Angle, ObjectIdentifier, PreviewId } from '../domain/domain.js'
+import type { InsertPreview } from '../repositories/previews.js'


alias pattern should be used. Is there an eslint rule missing?

iainsproat · 2025-01-06T15:52:06Z

packages/server/modules/previews/index.ts

+      }
+    }
+  }
+  const previewRequestQueue = new Bull('preview-service-jobs', opts)


The queue name should be configurable.

iainsproat · 2025-01-06T15:55:56Z

packages/preview-service/src/main.ts

+
+const opts = {
+  // redisOpts here will contain at least a property of connectionName which will identify the queue based on its name
+  createClient: function (type: string, redisOpts: RedisOptions) {


In the case of one worker for multiple deployments, how do we expect the infrastructure to be configured?

A new Redis cluster with one preview service queue? And all deployments publish to the cluster & queue.
Or the preview-service connects to all the existing Redis clusters, and reads each of their queues?

The gpu enabled preview service deployment will have its own dedicated redis queue.

For servers, we want to enable the gpu preview service, we just point the server's preview service queue config, to point to the shared preview queue.

iainsproat · 2025-01-06T15:56:22Z

packages/server/modules/previews/index.ts

-
-  if (isInitial) {
-    listenFor('preview_generation_update', messageProcessor)
+    // listenFor('preview_generation_update', messageProcessor)


should remove all commented out code. ☠️

iainsproat · 2025-01-06T15:59:23Z

packages/server/modules/previews/rest/router.ts

+
+  app.options('/preview/:streamId/:angle?', cors())
+  app.get('/preview/:streamId/:angle?', cors(), async (req, res) => {
+    const projectDb = await getProjectDbClient({ projectId: req.params.streamId })


How do we feel about refactoring these to named functions (possibly exported from the services directory?), so they can be called from other parts of the server codebase without an http call (or tested without starting an Express server)?

iainsproat · 2025-01-06T16:01:09Z

packages/server/package.json

@@ -85,6 +86,7 @@
    "graphql-subscriptions": "^2.0.0",
    "graphql-tag": "^2.12.6",
    "ioredis": "^5.2.2",
+    "join-images": "^1.1.5",


I don't think the server should be responsible for joining images.

maybe we can make that change later, for now, since we both store the 1st image as the single preview and the 360 image, its better to join server side. Its not a big deal tbf, the processing is done async, based on job triggers. If the memory cost of this is big, we can look into optimizing.

…gergo/previews

iainsproat · 2025-01-07T21:03:38Z

.github/workflows/preview-service-acceptance.yml should also be deleted if we're (temporarily, hopefully) removing the acceptance test.

…gergo/previews

gjedlicska added 10 commits November 26, 2024 22:27

feat(preview-generator): add new preview generator webapp

aa2756e

wip(preview-service): reworking the preview service backend

8a7e4fa

Merge branch 'main' of github.com:specklesystems/speckle-server into …

c4a4533

…gergo/previews

feat(previews): logging

3d27c06

feat(preview-service): streamline payloads

f29b78d

Merge branch 'main' of github.com:specklesystems/speckle-server into …

0cf172e

…gergo/previews

fix(preview-service): do not log the full payload

1804940

feat(preview-service): build new preview service

e4ef524

feat(preview-service): add separate response queue

314cf64

feat(previews): integrate preview queues with the server

9590220

gjedlicska requested a review from iainsproat January 6, 2025 13:50

iainsproat requested changes Jan 6, 2025

View reviewed changes

Merge branch 'main' of github.com:specklesystems/speckle-server into …

febb0bf

…gergo/previews

Merge branch 'main' of github.com:specklesystems/speckle-server into …

34335e1

…gergo/previews

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gergo/previews #3765

gergo/previews #3765

gjedlicska commented Jan 6, 2025 •

edited by iainsproat

Loading

iainsproat Jan 6, 2025

gjedlicska Jan 28, 2025

iainsproat Jan 6, 2025

gjedlicska Jan 28, 2025

iainsproat Jan 6, 2025

iainsproat Jan 6, 2025

iainsproat Jan 6, 2025

iainsproat Jan 6, 2025

iainsproat Jan 6, 2025

gjedlicska Jan 28, 2025

iainsproat Jan 6, 2025

iainsproat Jan 6, 2025

iainsproat Jan 6, 2025

gjedlicska Jan 28, 2025

iainsproat commented Jan 7, 2025

		@@ -1,4 +1,4 @@
		import type { Preview } from '@/domain/domain.js'
		import type { Preview } from '../domain/domain.js'

gergo/previews #3765

Are you sure you want to change the base?

gergo/previews #3765

Conversation

gjedlicska commented Jan 6, 2025 • edited by iainsproat Loading

Description & motivation

Changes:

To-do before merge:

Screenshots:

Validation of changes:

Checklist:

References

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iainsproat commented Jan 7, 2025

gjedlicska commented Jan 6, 2025 •

edited by iainsproat

Loading