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.

Sign up for GitHub

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(server): new projects are placed in a configurable region #3801

Merged
merged 6 commits into from
Jan 13, 2025
+67 −8
Merged

feat(server): new projects are placed in a configurable region #3801

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
65c4a8b
feat(server): log subscription started messages with info
gjedlicska Jan 10, 2025
f30f143
Merge branch 'main' of github.com:specklesystems/speckle-server into …
gjedlicska Jan 10, 2025
7aebe10
Merge branch 'main' of github.com:specklesystems/speckle-server into …
gjedlicska Jan 11, 2025
a9b73fb
feat(server): create projects in a default region
gjedlicska Jan 13, 2025
b94c302
feat(server): allow project default region config
gjedlicska Jan 13, 2025
6d49b83
feat(server): load project region from multi region config
gjedlicska Jan 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 28 additions & 3 deletions packages/server/modules/core/graph/resolvers/projects.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,13 @@ import {
insertCommitsFactory,
insertStreamCommitsFactory
} from '@/modules/core/repositories/commits'
import { storeModelFactory } from '@/modules/core/repositories/models'
import {
deleteProjectFactory,
getProjectFactory,
storeProjectFactory,
storeProjectRoleFactory
} from '@/modules/core/repositories/projects'
import { getServerInfoFactory } from '@/modules/core/repositories/server'
import {
getStreamFactory,
Expand All @@ -50,6 +57,7 @@ import {
getUserStreamsCountFactory
} from '@/modules/core/repositories/streams'
import { getUserFactory, getUsersFactory } from '@/modules/core/repositories/users'
import { createNewProjectFactory } from '@/modules/core/services/projects'
import {
getRateLimitResult,
isRateLimitBreached
Expand All @@ -69,7 +77,11 @@ import {
} from '@/modules/core/services/streams/management'
import { createOnboardingStreamFactory } from '@/modules/core/services/streams/onboarding'
import { getOnboardingBaseProjectFactory } from '@/modules/cross-server-sync/services/onboardingProject'
import { getProjectDbClient } from '@/modules/multiregion/utils/dbSelector'
import {
getDb,
getProjectDbClient,
getValidDefaultProjectRegionKey
} from '@/modules/multiregion/utils/dbSelector'
import {
deleteAllResourceInvitesFactory,
findUserByTargetFactory,
Expand Down Expand Up @@ -284,10 +296,23 @@ export = {
throw new RateLimitError(rateLimitResult)
}

const project = await createStreamReturnRecord({
const regionKey = await getValidDefaultProjectRegionKey()
const projectDb = await getDb({ regionKey })

const createNewProject = createNewProjectFactory({
Copy link
Contributor

Choose a reason for hiding this comment

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

Why use this over createStreamReturnRecord? What's the purpose of createStreamReturnRecord and where should it be used, and can we add comments to document that? i.e. why was it being used here previously, and why is it no longer required?
I think this is the bit of context I was missing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

createStreamReturnRecord is the old implementation, if you check how its written, its not compatible for creating multi region projects. That's why i had to write a new service for creating projects. The old one should not be used any more, but I did not have a need to replace it so far.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does createStreamReturnRecord need to be replaced everywhere it's currently referenced? e.g. will we continue to have onboarding streams created in the incorrect region if it's not replaced?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably we'll replace it there too, but that is not a big deal right now, so i did not want this PR to balloon.

storeProject: storeProjectFactory({ db: projectDb }),
getProject: getProjectFactory({ db }),
deleteProject: deleteProjectFactory({ db: projectDb }),
storeModel: storeModelFactory({ db: projectDb }),
// THIS MUST GO TO THE MAIN DB
storeProjectRole: storeProjectRoleFactory({ db }),
projectsEventsEmitter: ProjectsEmitter.emit
})

const project = await createNewProject({
...(args.input || {}),
ownerId: context.userId!,
ownerResourceAccessRules: context.resourceAccessRules
regionKey
Copy link
Contributor

Choose a reason for hiding this comment

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

what does regionKey do here? This should have a more explicit name for this object parameter.

Copy link
Contributor

Choose a reason for hiding this comment

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

It somehow seems like a duplication that we have to pass it to getDb and here. It seems like this could be the source of error (that we mistakenly pass in a regionKey here but different projectDb to the factory)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is a property of the project. Given, that all functions are only implementing a domain operation, no DB clients are exposed to the business logic functions, so explicitly entering the region key is the appropriate solution on the right level.

})

return project
Expand Down
7 changes: 6 additions & 1 deletion packages/server/modules/multiregion/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
import { moduleLogger } from '@/logging/logging'
import { initializeRegisteredRegionClients as initDb } from '@/modules/multiregion/utils/dbSelector'
import {
getValidDefaultProjectRegionKey,
initializeRegisteredRegionClients as initDb
} from '@/modules/multiregion/utils/dbSelector'
import { isMultiRegionEnabled } from '@/modules/multiregion/helpers'
import { SpeckleModule } from '@/modules/shared/helpers/typeHelper'
import {
Expand All @@ -18,6 +21,8 @@ const multiRegion: SpeckleModule = {

// Init registered region clients
await initDb()
// validate default project region key
await getValidDefaultProjectRegionKey()

const isBlobStorageEnabled = isMultiRegionBlobStorageEnabled()
if (isBlobStorageEnabled) {
Expand Down
5 changes: 5 additions & 0 deletions packages/server/modules/multiregion/regionConfig.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,3 +58,8 @@ export const getMainRegionConfig = async (): Promise<MainRegionConfig> => {
export const getAvailableRegionConfig: GetAvailableRegionConfig = async () => {
return (await getMultiRegionConfig()).regions
}

export const getDefaultProjectRegionKey = async (): Promise<string | null> => {
const defaultRegionKey = (await getMultiRegionConfig()).defaultProjectRegionKey
return defaultRegionKey ?? null
}
21 changes: 20 additions & 1 deletion packages/server/modules/multiregion/utils/dbSelector.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,12 @@ import {
} from '@/modules/multiregion/services/projectRegion'
import { Knex } from 'knex'
import { getRegionFactory } from '@/modules/multiregion/repositories'
import { DatabaseError } from '@/modules/shared/errors'
import { DatabaseError, MisconfiguredEnvironmentError } from '@/modules/shared/errors'
import { configureClient } from '@/knexfile'
import { InitializeRegion } from '@/modules/multiregion/domain/operations'
import {
getAvailableRegionConfig,
getDefaultProjectRegionKey,
getMainRegionConfig
} from '@/modules/multiregion/regionConfig'
import { ensureError, MaybeNullOrUndefined } from '@speckle/shared'
Expand Down Expand Up @@ -72,6 +73,24 @@ export const getProjectDbClient: GetProjectDb = async ({ projectId }) => {
return await getter({ projectId })
}

// the default region key is a config value, we're caching this globally
let defaultRegionKeyCache: string | null | undefined = undefined

export const getValidDefaultProjectRegionKey = async (): Promise<string | null> => {
if (defaultRegionKeyCache !== undefined) return defaultRegionKeyCache
const defaultRegionKey = await getDefaultProjectRegionKey()

if (!defaultRegionKey) return defaultRegionKey
const registeredRegionClients = await getRegisteredRegionClients()
if (!(defaultRegionKey in registeredRegionClients))
throw new MisconfiguredEnvironmentError(
`There is no region client registered for the default region key ${defaultRegionKey} `
)

defaultRegionKeyCache = defaultRegionKey
return defaultRegionKey
}

type RegionClients = Record<string, Knex>
let registeredRegionClients: RegionClients | undefined = undefined

Expand Down
8 changes: 6 additions & 2 deletions packages/server/modules/workspaces/services/projects.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,10 @@ import {
} from '@/modules/core/domain/streams/operations'
import { ProjectNotFoundError } from '@/modules/core/errors/projects'
import { WorkspaceProjectCreateInput } from '@/test/graphql/generated/graphql'
import { getDb } from '@/modules/multiregion/utils/dbSelector'
import {
getDb,
getValidDefaultProjectRegionKey
} from '@/modules/multiregion/utils/dbSelector'
import { createNewProjectFactory } from '@/modules/core/services/projects'
import {
deleteProjectFactory,
Expand Down Expand Up @@ -265,7 +268,8 @@ export const createWorkspaceProjectFactory =
const workspaceDefaultRegion = await deps.getDefaultRegion({
workspaceId: input.workspaceId
})
const regionKey = workspaceDefaultRegion?.key
const regionKey =
workspaceDefaultRegion?.key ?? (await getValidDefaultProjectRegionKey())
const projectDb = await getDb({ regionKey })
const db = mainDb

Expand Down
3 changes: 2 additions & 1 deletion packages/shared/src/environment/multiRegionConfig.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,8 @@ const regionConfigSchema = z.object({

const multiRegionConfigSchema = z.object({
main: regionConfigSchema,
regions: z.record(z.string(), regionConfigSchema)
regions: z.record(z.string(), regionConfigSchema),
defaultProjectRegionKey: z.string().min(3).nullish()
iainsproat marked this conversation as resolved.
Show resolved Hide resolved
})

export type MultiRegionConfig = z.infer<typeof multiRegionConfigSchema>
Expand Down
Loading