Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds MySQL addon support and shared saga infrastructure. Introduces three new schema kinds and generated entity types: MysqlServer, MysqlDedicatedData, MysqlSharedData. Adds pkg/addon/dbsaga (AddonConfig, saga actions for shared/dedicated workflows, BaseProvider), MySQL provider and utilities (plans, shared/dedicated sagas, server counter, SQL helpers), unit and blackbox tests, and testdata for a bun-mysql app. Adds Client.CreateOrReplace, registers the MySQL provider unconditionally in the coordinator, changes registry to use CreateOrReplace, updates PostgreSQL code to use dbsaga helpers, and updates go.mod to require the MySQL driver. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)
testdata/bun-mysql/index.ts (1)
8-14: Consider handling table creation errors.The top-level
awaitfor table creation will crash the server if it fails. While acceptable for a test fixture, you may want to log failures or retry for robustness in blackbox tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testdata/bun-mysql/index.ts` around lines 8 - 14, The top-level await executing the SQL template literal that creates the visits table (the sql`CREATE TABLE IF NOT EXISTS visits ...`) can crash the process on failure; wrap that call in a try/catch around the sql`...` invocation, log the error (including the error object) and optionally implement a simple retry loop (e.g., few attempts with small backoff) before rethrowing so test setup is more robust; update the code in index.ts where the sql`CREATE TABLE IF NOT EXISTS visits ...` is invoked to perform these steps.pkg/addon/postgresql/plans_test.go (1)
31-45: Trim this matrix to PostgreSQL-specific wrapper behavior.Now that
sanitizeIdentifierdelegates to the shared helper, most of this table overlapspkg/addon/ident_test.go. Keeping just the 63-character cap and one delegation smoke case would reduce dual-maintenance.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/addon/postgresql/plans_test.go` around lines 31 - 45, The TestSanitizeIdentifier table currently duplicates cases covered by the shared helper; trim it to PostgreSQL-specific behavior by keeping only a 63-character limit case and a single delegation smoke case (e.g., one normal input that shows delegation to the shared helper), and remove the redundant inputs; update the tests in TestSanitizeIdentifier and the expected values accordingly so it asserts the identifier is truncated to 63 chars and that a simple delegation case (e.g., "my-app" -> "my_app") still passes, leaving other normalization tests to the shared ident tests.blackbox/addon_test.go (1)
138-147: Cover the shared variant’s MySQL-specific env vars too.The dedicated test later asserts
MYSQL_HOSTandMYSQL_DATABASE, but this shared-path test only waits forDATABASE_URL. If shared provisioning stops exporting the MySQL-specific vars, this still stays green because the bun app only consumesDATABASE_URL.Coverage tweak
harness.WaitForAddonReady(t, m, name, "miren-mysql", 30*time.Second) harness.WaitForEnvVar(t, m, name, "DATABASE_URL", 5*time.Minute) + harness.WaitForEnvVar(t, m, name, "MYSQL_HOST", 30*time.Second) + harness.WaitForEnvVar(t, m, name, "MYSQL_DATABASE", 30*time.Second)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@blackbox/addon_test.go` around lines 138 - 147, The shared-path addon test only waits for DATABASE_URL but misses verifying MySQL-specific env vars; update the test where harness.WaitForEnvVar is called (and around harness.WaitForAddonReady / harness.WaitForAppReady) to also wait for the MySQL-specific variables (e.g., "MYSQL_HOST" and "MYSQL_DATABASE") using harness.WaitForEnvVar so the shared variant fails if those vars stop being exported; ensure the new waits use appropriate timeouts similar to the DATABASE_URL call.pkg/addon/mysql/shared.go (2)
126-198: Local implementations duplicate dbsaga shared actions.
WaitForSharedPool,CreateSharedService, andWaitForSharedServiceare implemented locally here, whilepkg/addon/dbsaga/shared.goalso provides shared implementations of these. This appears intentional since the MySQL shared saga doesn't injectAddonConfigand uses hardcoded constants instead.Consider whether these could be consolidated in a future refactoring to reduce duplication and ensure consistent behavior across providers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/addon/mysql/shared.go` around lines 126 - 198, These MySQL-specific implementations duplicate existing shared saga actions (WaitForSharedPool, CreateSharedService, WaitForSharedService); replace the local implementations by reusing the shared implementations (call the shared WaitForSharedPool/ CreateSharedService/ WaitForSharedService functions) or refactor the shared helpers to accept the MySQL-specific inputs (e.g., hardcoded server name, port, labels or an AddonConfig) and have these functions delegate to that shared helper; update UndoCreateSharedService to remain unchanged if delegation returns the same ServiceID, and remove the duplicated local functions or mark them as thin wrappers forwarding to the shared implementations.
467-506: Association count operations lack retry on revision conflict.Both
IncrementAssociationCountand its undo usefw.EC.Patchwith revision checks but don't retry on conflict. Under concurrent load, this could cause saga failures that require external retry.This matches the pattern in
dbsaga/shared.goand is acceptable given the low likelihood of concurrent operations on the same shared server, but consider adding retry logic for robustness in a future iteration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/addon/mysql/shared.go` around lines 467 - 506, IncrementAssociationCount and UndoIncrementAssociationCount currently call fw.EC.Patch once using ent.Revision() and will fail on revision conflicts; wrap the read-update-patch sequence (GetByIdWithEntity -> compute newCount -> fw.EC.Patch with entity.Int64(addon_v1alpha.MysqlServerAssociationCountId, newCount) and ent.Revision()) in a short retry loop (e.g., 3 attempts) that on conflict re-fetches the server and recomputes newCount before retrying, include a small backoff between attempts, and return the final error if retries are exhausted; keep the same behavior of clamping newCount >= 0 in UndoIncrementAssociationCount.pkg/addon/dbsaga/shared.go (1)
119-151: Association count increment lacks retry on revision conflict.The read-modify-write pattern (get count+revision, then patch) is susceptible to revision conflicts under concurrent provisioning. If two provisions run simultaneously and both read the same revision, one will fail with a conflict error. The saga will retry, but this could be more robust with an explicit retry loop.
This is acceptable for now given the low likelihood of concurrent provisions to the same shared server, but worth noting for future hardening.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/addon/dbsaga/shared.go` around lines 119 - 151, The IncrementAssociationCount function (using saga.Get[ServerCounter]) currently does a single read (ServerCounter.GetAssociationCount) then write (ServerCounter.PatchAssociationCount) which can fail on revision conflicts under concurrency; modify IncrementAssociationCount to wrap the read-modify-write in a small retry loop that detects revision/conflict errors from PatchAssociationCount, re-reads GetAssociationCount, and retries the Patch up to a bounded number of attempts (e.g., 3) before returning an error, preserving the existing error wrapping; keep UndoIncrementAssociationCount unchanged or apply the same retry pattern if you want symmetric robustness.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/addon/dbsaga/shared.go`:
- Around line 59-76: CreateSharedService assumes cfg.AddonName always starts
with "miren-" when building serviceName via cfg.AddonName[len("miren-"):], which
can panic or produce wrong names; update CreateSharedService to defensively
handle addon names by checking strings.HasPrefix(cfg.AddonName, "miren-") and
using either the suffix (when true) or the whole cfg.AddonName (or a safe
fallback) to build serviceName, and add the "strings" import; ensure the
variable serviceName is constructed only after this check and preserve existing
label logic and error handling around fw.CreateService.
In `@pkg/addon/ident.go`:
- Around line 23-30: The fallback return of "app" bypasses the truncation logic
in SanitizeIdentifier, so change the flow to assign the fallback to result
(e.g., result = []byte("app")) instead of returning immediately, then continue
to apply the existing leading-digit check and the maxLen truncation (use the
existing result and maxLen handling) so SanitizeIdentifier("", 1) will produce a
1-byte identifier; ensure you reference the result byte slice, the maxLen
parameter, and the leading-digit check when making the change.
In `@pkg/addon/mysql/dedicated.go`:
- Around line 237-252: Update the saga data flow by adding explicit saga field
tags and ensuring VariantName is emitted/propagated: add saga:"root_password",
saga:"password", saga:"database_name", saga:"username" tags to
GenerateCredentialsOut so credentials are exported explicitly; ensure
CreateMysqlServerOut includes or forwards VariantName (or have
GenerateCredentials produce VariantName) so UpdateDedicatedServer can consume
it; add saga tags on UpdateDedicatedServerIn for ServerID, PoolID, ServiceID,
and RootPassword to bind inputs from
CreateMysqlServerOut/CreateDedicatedPool/GenerateCredentials; and add matching
saga tags on BuildDedicatedResultIn for the credential fields so
BuildDedicatedResult consumes the generated credentials. Ensure the types
referenced (GenerateCredentialsOut, CreateMysqlServerOut,
UpdateDedicatedServerIn, BuildDedicatedResultIn) are updated accordingly so saga
registry can route values.
In `@pkg/addon/mysql/myutil.go`:
- Around line 32-44: The connectMysql function currently hand-assembles the DSN
which can break on reserved characters; replace the fmt.Sprintf DSN with
github.com/go-sql-driver/mysql's mysql.Config usage: create a mysql.NewConfig(),
set User, Passwd, Net="tcp", Addr using host and port, DBName=database and
TLSConfig=nil (to preserve tls=false), call cfg.FormatDSN() to get the DSN, then
pass that to sql.Open and keep the existing PingContext/db.Close/error wrapping
logic unchanged (symbols: connectMysql, mysql.Config, FormatDSN, sql.Open,
db.PingContext).
- Around line 51-75: The DDL in createMysqlUser and createMysqlDatabase must be
made idempotent: change the CREATE USER in createMysqlUser to use "CREATE USER
IF NOT EXISTS" and then always run an "ALTER USER ... IDENTIFIED BY" (or "SET
PASSWORD") statement to update the password so retries won't fail; similarly
change the CREATE DATABASE in createMysqlDatabase to "CREATE DATABASE IF NOT
EXISTS" and keep the GRANT ALL PRIVILEGES and FLUSH PRIVILEGES following it so
repeated runs succeed. Ensure you still use
quoteIdentifier(username)/quoteIdentifier(dbname) and the same password escaping
when building the ALTER USER/SET PASSWORD statement.
---
Nitpick comments:
In `@blackbox/addon_test.go`:
- Around line 138-147: The shared-path addon test only waits for DATABASE_URL
but misses verifying MySQL-specific env vars; update the test where
harness.WaitForEnvVar is called (and around harness.WaitForAddonReady /
harness.WaitForAppReady) to also wait for the MySQL-specific variables (e.g.,
"MYSQL_HOST" and "MYSQL_DATABASE") using harness.WaitForEnvVar so the shared
variant fails if those vars stop being exported; ensure the new waits use
appropriate timeouts similar to the DATABASE_URL call.
In `@pkg/addon/dbsaga/shared.go`:
- Around line 119-151: The IncrementAssociationCount function (using
saga.Get[ServerCounter]) currently does a single read
(ServerCounter.GetAssociationCount) then write
(ServerCounter.PatchAssociationCount) which can fail on revision conflicts under
concurrency; modify IncrementAssociationCount to wrap the read-modify-write in a
small retry loop that detects revision/conflict errors from
PatchAssociationCount, re-reads GetAssociationCount, and retries the Patch up to
a bounded number of attempts (e.g., 3) before returning an error, preserving the
existing error wrapping; keep UndoIncrementAssociationCount unchanged or apply
the same retry pattern if you want symmetric robustness.
In `@pkg/addon/mysql/shared.go`:
- Around line 126-198: These MySQL-specific implementations duplicate existing
shared saga actions (WaitForSharedPool, CreateSharedService,
WaitForSharedService); replace the local implementations by reusing the shared
implementations (call the shared WaitForSharedPool/ CreateSharedService/
WaitForSharedService functions) or refactor the shared helpers to accept the
MySQL-specific inputs (e.g., hardcoded server name, port, labels or an
AddonConfig) and have these functions delegate to that shared helper; update
UndoCreateSharedService to remain unchanged if delegation returns the same
ServiceID, and remove the duplicated local functions or mark them as thin
wrappers forwarding to the shared implementations.
- Around line 467-506: IncrementAssociationCount and
UndoIncrementAssociationCount currently call fw.EC.Patch once using
ent.Revision() and will fail on revision conflicts; wrap the read-update-patch
sequence (GetByIdWithEntity -> compute newCount -> fw.EC.Patch with
entity.Int64(addon_v1alpha.MysqlServerAssociationCountId, newCount) and
ent.Revision()) in a short retry loop (e.g., 3 attempts) that on conflict
re-fetches the server and recomputes newCount before retrying, include a small
backoff between attempts, and return the final error if retries are exhausted;
keep the same behavior of clamping newCount >= 0 in
UndoIncrementAssociationCount.
In `@pkg/addon/postgresql/plans_test.go`:
- Around line 31-45: The TestSanitizeIdentifier table currently duplicates cases
covered by the shared helper; trim it to PostgreSQL-specific behavior by keeping
only a 63-character limit case and a single delegation smoke case (e.g., one
normal input that shows delegation to the shared helper), and remove the
redundant inputs; update the tests in TestSanitizeIdentifier and the expected
values accordingly so it asserts the identifier is truncated to 63 chars and
that a simple delegation case (e.g., "my-app" -> "my_app") still passes, leaving
other normalization tests to the shared ident tests.
In `@testdata/bun-mysql/index.ts`:
- Around line 8-14: The top-level await executing the SQL template literal that
creates the visits table (the sql`CREATE TABLE IF NOT EXISTS visits ...`) can
crash the process on failure; wrap that call in a try/catch around the sql`...`
invocation, log the error (including the error object) and optionally implement
a simple retry loop (e.g., few attempts with small backoff) before rethrowing so
test setup is more robust; update the code in index.ts where the sql`CREATE
TABLE IF NOT EXISTS visits ...` is invoked to perform these steps.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b4102ad3-50eb-4c4f-8f45-2e6012c289e7
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumtestdata/bun-mysql/bun.lockis excluded by!**/*.lock
📒 Files selected for processing (30)
api/addon/addon_v1alpha/schema.gen.goapi/addon/schema.ymlapi/entityserver/client.goblackbox/addon_test.gocomponents/coordinate/coordinate.gogo.modpkg/addon/dbsaga/config.gopkg/addon/dbsaga/dedicated.gopkg/addon/dbsaga/provider.gopkg/addon/dbsaga/shared.gopkg/addon/ident.gopkg/addon/ident_test.gopkg/addon/mysql/dedicated.gopkg/addon/mysql/dedicated_test.gopkg/addon/mysql/myutil.gopkg/addon/mysql/plans.gopkg/addon/mysql/plans_test.gopkg/addon/mysql/provider.gopkg/addon/mysql/shared.gopkg/addon/mysql/shared_test.gopkg/addon/postgresql/dedicated.gopkg/addon/postgresql/plans.gopkg/addon/postgresql/plans_test.gopkg/addon/postgresql/provider.gopkg/addon/postgresql/shared.gopkg/addon/registry.gotestdata/bun-mysql/.gitignoretestdata/bun-mysql/.miren/app.tomltestdata/bun-mysql/index.tstestdata/bun-mysql/package.json
phinze
approved these changes
Mar 30, 2026
Contributor
phinze
left a comment
phinze
left a comment
There was a problem hiding this comment.
Walked through this with my robo-buddy, all looking good! Clean mirror of the PostgreSQL addon pattern, solid saga structure, nice dedup with the dbsaga extraction. A few small notes inline.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/addon/postgresql/server_counter.go (1)
28-33: Wrap patch failures with operation context.Line 31 currently returns raw patch errors. Adding context here makes saga failure logs much easier to triage.
Proposed diff
func (pgServerCounter) PatchAssociationCount(ctx context.Context, serverID entity.Id, revision int64, newCount int64) error { fw := saga.Get[*addon.ProviderFramework](ctx) - return fw.EC.Patch(ctx, serverID, revision, - entity.Int64(addon_v1alpha.PostgresServerAssociationCountId, newCount), - ) + if err := fw.EC.Patch(ctx, serverID, revision, + entity.Int64(addon_v1alpha.PostgresServerAssociationCountId, newCount), + ); err != nil { + return fmt.Errorf("patching postgres server association count: %w", err) + } + return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/addon/postgresql/server_counter.go` around lines 28 - 33, The PatchAssociationCount function currently returns the raw error from fw.EC.Patch; capture the returned error and wrap it with contextual information before returning (e.g., include serverID and revision) so saga logs are clearer. Modify the call in PatchAssociationCount (which uses saga.Get[*addon.ProviderFramework] and fw.EC.Patch with addon_v1alpha.PostgresServerAssociationCountId) to assign the error to a variable and return a wrapped error (using fmt.Errorf or errors.Wrapf) such as "patching association count for server <serverID> rev <revision>: %w" including the original error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/addon/mysql/myutil.go`:
- Around line 58-73: The password escaping in createMysqlUser only replaces
single quotes and misses backslashes; add a helper (e.g., escapeMysqlString)
that first doubles backslashes then replaces single quotes (replace "\" with
"\\" then "'" with "''"), and use that helper for the password argument in both
ExecContext calls inside createMysqlUser so backslashes are preserved in MySQL
string literals.
---
Nitpick comments:
In `@pkg/addon/postgresql/server_counter.go`:
- Around line 28-33: The PatchAssociationCount function currently returns the
raw error from fw.EC.Patch; capture the returned error and wrap it with
contextual information before returning (e.g., include serverID and revision) so
saga logs are clearer. Modify the call in PatchAssociationCount (which uses
saga.Get[*addon.ProviderFramework] and fw.EC.Patch with
addon_v1alpha.PostgresServerAssociationCountId) to assign the error to a
variable and return a wrapped error (using fmt.Errorf or errors.Wrapf) such as
"patching association count for server <serverID> rev <revision>: %w" including
the original error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e88e2b76-a5ca-4f37-98fc-0d15f00c51d9
📒 Files selected for processing (10)
components/coordinate/coordinate.gopkg/addon/dbsaga/config.gopkg/addon/dbsaga/shared.gopkg/addon/ident.gopkg/addon/mysql/dedicated.gopkg/addon/mysql/myutil.gopkg/addon/mysql/server_counter.gopkg/addon/mysql/shared.gopkg/addon/postgresql/server_counter.gopkg/addon/postgresql/shared.go
✅ Files skipped from review due to trivial changes (1)
- pkg/addon/mysql/dedicated.go
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/addon/dbsaga/config.go
- pkg/addon/ident.go
- pkg/addon/postgresql/shared.go
- pkg/addon/dbsaga/shared.go
- pkg/addon/mysql/shared.go
Implement miren-mysql addon provider mirroring the PostgreSQL addon pattern, with dedicated and shared variants, provisioning/deprovisioning sagas, and MySQL-specific env vars (DATABASE_URL, MYSQL_HOST, etc.). Also fix EnsureEntities to use CreateOrReplace instead of CreateOrUpdate, preventing duplicate component entries (like variants) from accumulating on server restarts due to the entity store's Put/merge semantics. Includes blackbox tests for MySQL addon lifecycle and a bun-mysql test app that exercises end-to-end deploy with shared MySQL via app.toml.
- Generate a separate root password for dedicated MySQL servers instead of reusing the app user password for MYSQL_ROOT_PASSWORD - Cap MySQL identifiers at 32 characters (MySQL's limit for usernames and database names)
- Add 63-character limit to PostgreSQL sanitizeIdentifier (NAMEDATALEN-1) - Document that CreateOrReplace is not safe for concurrent writers on the same entity (acceptable for startup-time EnsureEntities)
Move duplicated code between PostgreSQL and MySQL addon providers into shared packages: - pkg/addon/ident.go: SanitizeIdentifier, ParseStorageGb, IsSharedVariant - pkg/addon/dbsaga/: BaseProvider, AddonConfig, and 10 shared saga actions (WaitForPool, CreateService, WaitForService, DeleteService, DeletePool, plus shared-server variants and association counting) Both providers now embed dbsaga.BaseProvider and reference shared saga actions in their saga registrations, eliminating ~400 lines of duplication. Adding a new database addon now requires only the DB-specific parts (~300 lines vs ~1400).
- Add explicit saga tags to MySQL dedicated action structs for clear data flow documentation - Make MySQL DDL operations idempotent (CREATE USER/DATABASE IF NOT EXISTS + ALTER USER for password updates) for saga crash recovery - Use mysql.Config FormatDSN() instead of hand-assembled DSN to properly escape credentials with reserved characters - Fix SanitizeIdentifier empty-name fallback to honor maxLen - Add defensive strings.TrimPrefix for AddonName in CreateSharedService - Wire up shared-path dbsaga actions (WaitForSharedPool, CreateSharedService, WaitForSharedService, IncrementAssociationCount, DecrementAssociationCount) in both PostgreSQL and MySQL providers via ServerCounter interface for association counting - Remove labs.Addons() conditional from coordinate.go (default-on)
MySQL treats backslash as an escape character in string literals by default. Add escapeMysqlString helper that doubles backslashes before single quotes, and use it in createMysqlUser for both CREATE USER and ALTER USER statements.
evanphx
force-pushed
the
mir-901-add-mysql-addon-support
branch
from
f57cd7a to
4a77c06
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/addon/mysql/myutil.go (1)
125-141: Consider logging ignored KILL errors for observability.While silently ignoring individual
KILLexecution errors is reasonable (the connection may have already closed), consider logging them at debug level for troubleshooting.♻️ Optional: Add debug logging for ignored errors
for rows.Next() { var id int64 if err := rows.Scan(&id); err != nil { return fmt.Errorf("scanning process id: %w", err) } - _, _ = db.ExecContext(ctx, fmt.Sprintf("KILL %d", id)) + if _, err := db.ExecContext(ctx, fmt.Sprintf("KILL %d", id)); err != nil { + // Connection may have already closed; log and continue + slog.Debug("ignoring KILL error", "process_id", id, "error", err) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/addon/mysql/myutil.go` around lines 125 - 141, The terminateMysqlConnections function currently ignores errors from the per-connection db.ExecContext call that runs the "KILL %d" statement; change this so that when db.ExecContext(ctx, fmt.Sprintf("KILL %d", id)) returns an error you log it at debug level (including the process id and the error) without changing the control flow (still continue to the next id). Locate terminateMysqlConnections and the db.ExecContext call and add a debug log call for non-nil errors (use the package/global logger if one exists, or accept/pass a logger into the function) so ignored KILL errors are recorded for observability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/addon/mysql/dedicated.go`:
- Around line 167-173: Update the saga to make activation wait for service
readiness by threading the service host into the UpdateDedicatedServerIn payload
(or adding an explicit edge) so the saga action depends on
dbsaga.WaitForDedicatedService; specifically add a ServiceHost (or
ServiceHostID/ServiceHostAddr) field to the UpdateDedicatedServerIn struct and
update the UpdateDedicatedServer handler to consume that field (or create a
dedicated dependency) rather than just checking ServiceID, ensuring the saga
framework infers the dependency and the server is only marked active once the
WaitForDedicatedService step has completed; apply the same pattern to the other
occurrence noted around the 248-249 area so all activation steps explicitly
depend on service readiness.
- Around line 143-152: CreateDedicatedPool creates a named persistent disk but
UndoCreateDedicatedPool only deletes the sandbox pool, leaking the disk; update
UndoCreateDedicatedPool (and the similar rollback logic around lines 162-165) to
also delete the persistent disk that was created (use the same disk
identifier/diskName used in CreateDedicatedPool) as part of the undo, making the
deletion idempotent and logging/propagating errors appropriately so failed
rollbacks don't leave orphaned disks.
---
Nitpick comments:
In `@pkg/addon/mysql/myutil.go`:
- Around line 125-141: The terminateMysqlConnections function currently ignores
errors from the per-connection db.ExecContext call that runs the "KILL %d"
statement; change this so that when db.ExecContext(ctx, fmt.Sprintf("KILL %d",
id)) returns an error you log it at debug level (including the process id and
the error) without changing the control flow (still continue to the next id).
Locate terminateMysqlConnections and the db.ExecContext call and add a debug log
call for non-nil errors (use the package/global logger if one exists, or
accept/pass a logger into the function) so ignored KILL errors are recorded for
observability.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 53b60531-84d9-4d2d-a567-04651701efed
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumtestdata/bun-mysql/bun.lockis excluded by!**/*.lock
📒 Files selected for processing (32)
api/addon/addon_v1alpha/schema.gen.goapi/addon/schema.ymlapi/entityserver/client.goblackbox/addon_test.gocomponents/coordinate/coordinate.gogo.modpkg/addon/dbsaga/config.gopkg/addon/dbsaga/dedicated.gopkg/addon/dbsaga/provider.gopkg/addon/dbsaga/shared.gopkg/addon/ident.gopkg/addon/ident_test.gopkg/addon/mysql/dedicated.gopkg/addon/mysql/dedicated_test.gopkg/addon/mysql/myutil.gopkg/addon/mysql/plans.gopkg/addon/mysql/plans_test.gopkg/addon/mysql/provider.gopkg/addon/mysql/server_counter.gopkg/addon/mysql/shared.gopkg/addon/mysql/shared_test.gopkg/addon/postgresql/dedicated.gopkg/addon/postgresql/plans.gopkg/addon/postgresql/plans_test.gopkg/addon/postgresql/provider.gopkg/addon/postgresql/server_counter.gopkg/addon/postgresql/shared.gopkg/addon/registry.gotestdata/bun-mysql/.gitignoretestdata/bun-mysql/.miren/app.tomltestdata/bun-mysql/index.tstestdata/bun-mysql/package.json
✅ Files skipped from review due to trivial changes (11)
- testdata/bun-mysql/.gitignore
- testdata/bun-mysql/.miren/app.toml
- testdata/bun-mysql/package.json
- pkg/addon/postgresql/plans_test.go
- pkg/addon/registry.go
- pkg/addon/dbsaga/provider.go
- pkg/addon/ident.go
- pkg/addon/dbsaga/config.go
- pkg/addon/ident_test.go
- api/addon/schema.yml
- testdata/bun-mysql/index.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- pkg/addon/postgresql/plans.go
- go.mod
- pkg/addon/mysql/shared_test.go
- pkg/addon/postgresql/server_counter.go
- pkg/addon/mysql/server_counter.go
- api/entityserver/client.go
- pkg/addon/mysql/dedicated_test.go
- pkg/addon/mysql/provider.go
- pkg/addon/mysql/plans.go
- pkg/addon/dbsaga/shared.go
- pkg/addon/mysql/shared.go
- UndoCreateDedicatedPool now deletes the persistent disk in addition to the sandbox pool, preventing orphaned disks on rollback - Add ServiceHost to UpdateDedicatedServerIn so the saga framework infers a dependency on WaitForDedicatedService, ensuring the server isn't marked active before the service is reachable Both fixes applied to PostgreSQL and MySQL providers.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/addon/mysql/dedicated.go`:
- Around line 352-361: Add matching saga struct tags so saga wiring works: add
`saga:"dedicatedserviceid"` on DeleteDedicatedServiceIn.DedicatedServiceID (to
match LookupDedicatedServerOut.DedicatedServiceID) and `saga:"dedicatedpoolid"`
on DeleteDedicatedPoolIn.DedicatedPoolID (to match
LookupDedicatedServerOut.DedicatedPoolID); also add a saga output tag on the
boolean in DeleteDedicatedServiceOut (e.g., `saga:"dedicated_service_deleted"`)
similar to DeleteDedicatedPoolOut.PoolDeleted so downstream edges see the
service-deleted signal.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d768ae47-cb33-419a-85d3-8ddad233eee7
📒 Files selected for processing (2)
pkg/addon/mysql/dedicated.gopkg/addon/postgresql/dedicated.go
Add saga tags to DeleteDedicatedServiceIn, DeleteDedicatedServiceOut, and DeleteDedicatedPoolIn in dbsaga for consistency with the explicit tag convention used throughout the addon saga actions.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
miren-mysqladdon provider with dedicated and shared variants, mirroring the PostgreSQL addon patternEnsureEntitiesto useCreateOrReplaceinstead ofCreateOrUpdate, preventing duplicate component entries from accumulating on server restartspkg/addon/dbsaga/to eliminate ~400 lines of duplication between providersChanges
New MySQL addon (
pkg/addon/mysql/):DATABASE_URL,MYSQL_HOST,MYSQL_PORT,MYSQL_USER,MYSQL_PASSWORD,MYSQL_DATABASEgithub.com/go-sql-driver/mysqlEntity store fix (
api/entityserver/client.go):CreateOrReplacemethod that usesReplaceinstead ofPuton update, preventing "many" component fields from appending duplicatesShared addon framework (
pkg/addon/dbsaga/,pkg/addon/ident.go):BaseProviderwith sharedLocalityModeandAdjustEnvVarsAddonConfigfor injecting addon-specific constants into shared saga actionsSanitizeIdentifier,ParseStorageGb,IsSharedVariantutilitiesBlackbox tests (
blackbox/addon_test.go,testdata/bun-mysql/):TestMysqlAddonCreateListDestroy— dedicated addon lifecycleTestMysqlAddonDeployWithAppToml— end-to-end shared addon via app.toml with DB connectivity verificationTestAddonListAvailableandTestAddonVariantsto cover MySQLCloses MIR-901
Test plan
make test— 3786 tests passmake test-blackbox— 17/17 tests pass (including new MySQL tests)golangci-lint run— no issues