[ADMINAPI-1369] Create DbInstance Jobs - #316
Conversation
Test Results 14 files 14 suites 36s ⏱️ Results for commit 68ba2ac. ♻️ This comment has been updated with latest results. |
be9e150 to
b736c3b
Compare
There was a problem hiding this comment.
Pull request overview
This PR enhances the Admin API v2 POST /v2/dbinstances workflow by making DbInstance provisioning asynchronous via Quartz jobs, improving multi-tenant support for background execution, and tightening request validation around naming/length constraints.
Changes:
- Schedule
CreateInstanceJobafter persisting aPendingDbInstance and add a recurringCreatePendingDbInstancesDispatcherJobfor sweep-based recovery and capped retries. - Improve multi-tenant behavior by propagating tenant identity into job keys/data, enabling tenant-specific connection string resolution, and updating Docker compose defaults accordingly.
- Expand validation (allowed characters, uniqueness across
DbInstancesandOdsInstances, portable DB-name length) and update/add unit/E2E/docs to reflect the async job pipeline.
Reviewed changes
Copilot reviewed 50 out of 50 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/http/dbinstances.http | Updates HTTP examples to include Tenant usage and new request naming; adds delete examples. |
| docs/developer.md | Documents the asynchronous provisioning flow and required configuration. |
| docs/design/PLAN-D-ACCEPT-RISK.md | Adds “accept risk” remediation plan documentation for tenant context race. |
| docs/design/PLAN-C-EXPLICIT-CONNECTION-STRING-PARAM.md | Adds “explicit connection string param” remediation plan documentation. |
| docs/design/PLAN-B-HTTPACCESSOR-SPLIT-STORAGE.md | Adds “split storage” remediation plan documentation. |
| docs/design/PLAN-A-ASYNC-LOCAL-CONTEXT-STORAGE.md | Adds “AsyncLocal context storage” remediation plan documentation. |
| docs/design/DBINSTANCE-PROVISIONING-JOBS.md | Adds detailed durable design doc for provisioning jobs, identity, retries, and risks. |
| Docker/V2/Compose/pgsql/SingleTenant/compose-build-ods.yml | Adds EdFi_Ods/EdFi_Master connection strings for single-tenant pgsql. |
| Docker/V2/Compose/pgsql/SingleTenant/compose-build-idp-dev.yml | Adds EdFi_Ods/EdFi_Master connection strings for single-tenant pgsql. |
| Docker/V2/Compose/pgsql/SingleTenant/compose-build-idp-binaries.yml | Adds EdFi_Ods/EdFi_Master connection strings for single-tenant pgsql. |
| Docker/V2/Compose/pgsql/SingleTenant/compose-build-dev.yml | Adds EdFi_Ods/EdFi_Master connection strings; trims whitespace. |
| Docker/V2/Compose/pgsql/SingleTenant/compose-build-binaries.yml | Adds EdFi_Ods/EdFi_Master connection strings for single-tenant pgsql. |
| Docker/V2/Compose/pgsql/MultiTenant/compose-build-ods-multi-tenant.yml | Adds default + tenant-specific EdFi_Ods/EdFi_Master connection strings. |
| Docker/V2/Compose/pgsql/MultiTenant/compose-build-idp-dev-multi-tenant.yml | Adds default + tenant-specific EdFi_Ods/EdFi_Master connection strings. |
| Docker/V2/Compose/pgsql/MultiTenant/compose-build-idp-binaries-multi-tenant.yml | Adds default + tenant-specific EdFi_Ods/EdFi_Master connection strings. |
| Docker/V2/Compose/pgsql/MultiTenant/compose-build-dev-multi-tenant.yml | Adds default + tenant-specific EdFi_Ods/EdFi_Master connection strings. |
| Docker/V2/Compose/pgsql/MultiTenant/compose-build-binaries-multi-tenant.yml | Adds default + tenant-specific EdFi_Ods/EdFi_Master connection strings. |
| Docker/V2/Compose/mssql/SingleTenant/compose-build-ods.yml | Adds EdFi_Ods/EdFi_Master connection strings for single-tenant mssql. |
| Docker/V2/Compose/mssql/SingleTenant/compose-build-idp-dev.yml | Adds EdFi_Ods/EdFi_Master connection strings for single-tenant mssql. |
| Docker/V2/Compose/mssql/SingleTenant/compose-build-idp-binaries.yml | Adds EdFi_Ods/EdFi_Master connection strings for single-tenant mssql. |
| Docker/V2/Compose/mssql/SingleTenant/compose-build-dev.yml | Adds EdFi_Ods/EdFi_Master connection strings for single-tenant mssql. |
| Docker/V2/Compose/mssql/SingleTenant/compose-build-binaries.yml | Adds EdFi_Ods/EdFi_Master connection strings for single-tenant mssql. |
| Docker/V2/Compose/mssql/MultiTenant/compose-build-ods-multi-tenant.yml | Adds default + tenant-specific EdFi_Ods/EdFi_Master connection strings. |
| Docker/V2/Compose/mssql/MultiTenant/compose-build-idp-dev-multi-tenant.yml | Adds default + tenant-specific EdFi_Ods/EdFi_Master connection strings. |
| Docker/V2/Compose/mssql/MultiTenant/compose-build-idp-binaries-multi-tenant.yml | Adds default + tenant-specific EdFi_Ods/EdFi_Master connection strings. |
| Docker/V2/Compose/mssql/MultiTenant/compose-build-dev-multi-tenant.yml | Adds default + tenant-specific EdFi_Ods/EdFi_Master connection strings. |
| Docker/V2/Compose/mssql/MultiTenant/compose-build-binaries-multi-tenant.yml | Adds default + tenant-specific EdFi_Ods/EdFi_Master connection strings. |
| Application/EdFi.Ods.AdminApi/appsettings.json | Adds defaults for sweep interval and max retries. |
| Application/EdFi.Ods.AdminApi/appsettings.Development.json | Adds dev defaults for sweep interval and max retries. |
| Application/EdFi.Ods.AdminApi/Program.cs | Schedules recurring dispatcher (and refactors tenant initialization / scheduler retrieval). |
| Application/EdFi.Ods.AdminApi/Infrastructure/WebApplicationBuilderExtensions.cs | Registers new jobs and makes ConfigConnectionStringsProvider transient for sandbox provisioning. |
| Application/EdFi.Ods.AdminApi/Infrastructure/Services/Jobs/CreatePendingDbInstancesDispatcherJob.cs | Adds sweep-based dispatcher job implementation. |
| Application/EdFi.Ods.AdminApi/Infrastructure/Services/Jobs/CreateInstanceJob.cs | Adds worker job to provision DB + sync OdsInstance + status transitions. |
| Application/EdFi.Ods.AdminApi/Features/DbInstances/DbInstanceDatabaseNameFormatter.cs | Adds canonical DB-name generation + portable length constant. |
| Application/EdFi.Ods.AdminApi/Features/DbInstances/AddDbInstance.cs | Schedules CreateInstanceJob from the API and expands validation (chars, uniqueness, portable DB-name length). |
| Application/EdFi.Ods.AdminApi/E2E Tests/V2/Bruno Admin API E2.0 refactor/v2/DbInstances/POST - DbInstances - Sample Template.bru | Uses unique name per run; improves diagnostics; adjusts ID parsing; sets timeout. |
| Application/EdFi.Ods.AdminApi/E2E Tests/V2/Bruno Admin API E2.0 refactor/v2/DbInstances/POST - DbInstance - For Delete Test.bru | Removes obsolete pre-delete setup request file. |
| Application/EdFi.Ods.AdminApi/E2E Tests/V2/Bruno Admin API E2.0 refactor/v2/DbInstances/DELETE - DbInstance - Success.bru.disabled | Adds disabled delete-success E2E with documented CI prerequisites. |
| Application/EdFi.Ods.AdminApi/E2E Tests/V2/Bruno Admin API E2.0 refactor/v2/DbInstances/DELETE - DbInstance - Pending Status.bru | Removes pending-status delete test file. |
| Application/EdFi.Ods.AdminApi/E2E Tests/V2/Bruno Admin API E2.0 refactor/v2/DbInstances/DELETE - DbInstance - Not Found.bru | Minor formatting + timeout setting. |
| Application/EdFi.Ods.AdminApi.UnitTests/Infrastructure/Services/Jobs/CreatePendingDbInstancesDispatcherJobTests.cs | Adds unit tests for dispatcher scheduling and retry gating. |
| Application/EdFi.Ods.AdminApi.UnitTests/Infrastructure/Services/Jobs/CreateInstanceJobTests.cs | Adds unit tests for worker job behavior, tenant context, and DB-name formatting. |
| Application/EdFi.Ods.AdminApi.UnitTests/Infrastructure/Helpers/ConfigConnectionStringsProviderTests.cs | Adds unit tests for tenant override behavior in connection string provider. |
| Application/EdFi.Ods.AdminApi.UnitTests/Features/DbInstances/AddDbInstanceTests.cs | Updates tests for new handler signature and validates job scheduling + new validation rules. |
| Application/EdFi.Ods.AdminApi.InstanceManagement/Provisioners/SandboxProvisionerBase.cs | Avoids caching EdFi_Master connection string; resolves at call time for multi-tenant jobs. |
| Application/EdFi.Ods.AdminApi.Common/Settings/AppSettings.cs | Adds settings for sweep interval and max retries. |
| Application/EdFi.Ods.AdminApi.Common/Infrastructure/MultiTenancy/TenantConfigurationProvider.cs | Adds tenant-level EdFi_Ods and EdFi_Master extraction. |
| Application/EdFi.Ods.AdminApi.Common/Infrastructure/MultiTenancy/TenantConfiguration.cs | Adds OdsConnectionString and MasterConnectionString properties. |
| Application/EdFi.Ods.AdminApi.Common/Infrastructure/Jobs/JobConstants.cs | Adds job constants for DbInstance provisioning jobs. |
| Application/EdFi.Ods.AdminApi.Common/Infrastructure/Helpers/ConfigConnectionStringsProvider.cs | Adds tenant-aware overlay logic and makes mapping dynamic per call. |
| var trigger = TriggerBuilder.Create() | ||
| .StartNow() | ||
| .Build(); | ||
|
|
||
| var scheduler = await schedulerFactory.GetScheduler(); | ||
| await scheduler.ScheduleJob(jobBuilder.Build(), trigger); | ||
|
|
There was a problem hiding this comment.
This schedules the CreateInstance job directly via scheduler.ScheduleJob(...) without checking whether the same job key was already scheduled. Because the dispatcher can also schedule CreateInstanceJob for newly-inserted Pending rows, there’s a real race where the dispatcher schedules first and this call throws ObjectAlreadyExistsException, causing the API to fail even though the job is already queued. Consider using the existing QuartzJobScheduler.ScheduleJob<TJob>(...) helper (or an explicit CheckExists guard) here for idempotent scheduling.
| var trigger = TriggerBuilder.Create() | |
| .StartNow() | |
| .Build(); | |
| var scheduler = await schedulerFactory.GetScheduler(); | |
| await scheduler.ScheduleJob(jobBuilder.Build(), trigger); | |
| var job = jobBuilder.Build(); | |
| var trigger = TriggerBuilder.Create() | |
| .StartNow() | |
| .Build(); | |
| var scheduler = await schedulerFactory.GetScheduler(); | |
| try | |
| { | |
| await scheduler.ScheduleJob(job, trigger); | |
| } | |
| catch (ObjectAlreadyExistsException) | |
| { | |
| // Another path may have already scheduled the same CreateInstanceJob. | |
| // Treat duplicate scheduling as success so this endpoint remains idempotent. | |
| } |
There was a problem hiding this comment.
Fixed in this PR. Added a try/catch ObjectAlreadyExistsException guard around scheduler.ScheduleJob(...) so that if the dispatcher has already scheduled the same job key, the duplicate is silently swallowed and the endpoint still returns 202 Accepted.
- Make ConfigConnectionStringsProvider transient and tenant-aware so per-tenant EdFi_Ods/EdFi_Master connection strings are resolved at call time instead of captured once at startup - Extend TenantConfiguration with OdsConnectionString/MasterConnectionString - Change SandboxProvisionerBase.ConnectionString to a computed property so multi-tenant jobs always read the active tenant's EdFi_Master - Set and clear TenantConfiguration ambient context in CreateInstanceJob before/after provisioning each tenant's DbInstance - Add EdFi_Ods and EdFi_Master to all 20 Docker compose files (pgsql + mssql, single-tenant + multi-tenant) - Add unit tests for ConfigConnectionStringsProvider and CreateInstanceJob - Document HashtableContextStorage race condition and four remediation plans (Plan A–D) in docs/design/
fc3a7ac to
68ba2ac
Compare
Create CODE_OF_CONDUCT.md
This pull request introduces enhancements to the
AddDbInstancefeature, primarily adding job scheduling for database instance creation and improving multi-tenancy support. It also refines validation logic for instance names and updates related tests to cover the new behavior.Job Scheduling and Multi-Tenancy Enhancements:
AddDbInstance.Handlemethod now schedules a Quartz job (CreateInstanceJob) to create the database instance after the request is validated and persisted. The job is tenant-aware if multi-tenancy is enabled, ensuring jobs are scheduled with the correct tenant context.JobConstantsfor job and data map keys, supporting the new job scheduling logic.AppSettingsclass now includes configuration for the database instance creation job sweep interval and max retry attempts, supporting job execution management.Validation Improvements:
Test Coverage Updates:
AddDbInstancehave been updated to accommodate the new job scheduling logic. New tests verify that jobs are scheduled correctly, including tenant-aware scenarios, and all validation tests are updated to use the new method signature and dependencies. [1] [2] [3] [4] [5] [6] [7] [8]Summary of Most Important Changes:
Job Scheduling and Multi-Tenancy:
AddDbInstance.Handlenow schedules aCreateInstanceJobusing Quartz, including tenant information when multi-tenancy is enabled.JobConstantsand new job configuration properties toAppSettings. [1] [2]Validation:
Testing: