Conversation
- Replace incorrect ExternalSiteLink (mysql-mgr) with relative links since mysql-mgr site is not defined in mgr-docs sites.yaml - Add comprehensive PXC-to-MGR migration guide in how_to section - Fix upgrade.mdx warning format and internal cross-references Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughAdds a migration runbook from MySQL‑PXC 5.7 to MySQL Group Replication 8.0, expands upgrade guidance for Operator v4.3+, updates the MySQL CRD schema (removes exposed PXC state; adds s3Config and other spec fields), and removes PXC-specific FunctionResource labels and auth rules. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 6
🧹 Nitpick comments (1)
docs/shared/crds/middleware.alauda.io_mysqls.yaml (1)
10224-10227: Add enum constraint topodManagementPolicyto enforce StatefulSet-valid values.The field currently accepts any string, allowing invalid values to pass CRD validation and fail later during reconciliation. Kubernetes StatefulSet only accepts
OrderedReadyorParallelfor this field. Add an enum constraint to the schema to validate values at creation time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/shared/crds/middleware.alauda.io_mysqls.yaml` around lines 10224 - 10227, The CRD schema for podManagementPolicy currently allows any string; update the schema for podManagementPolicy to add an enum constraint restricting values to the two valid StatefulSet options (“OrderedReady” and “Parallel”) so invalid values are rejected at validation time; locate the podManagementPolicy property in the CRD definition and add an enum: ["OrderedReady","Parallel"] entry alongside the existing type: string and description to enforce Kubernetes-compatible values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/en/how_to/35-migrate-from-pxc.mdx`:
- Around line 421-437: The documentation uses an undefined MYSQL_PASSWORD
variable in several kubectl exec/verification commands; update those commands to
use the defined SOURCE_MYSQL_PASSWORD (for the source PXC pod) and keep
TARGET_MYSQL_PASSWORD for the target pod so the shell script will run as-is.
Search for occurrences of MYSQL_PASSWORD in the verification loop and other
kubectl exec invocations (e.g., the mysql -uroot -p${MYSQL_PASSWORD} calls) and
replace them with ${SOURCE_MYSQL_PASSWORD} when targeting the source
(${SOURCE_NAME}-pxc-0) and leave ${TARGET_MYSQL_PASSWORD} when targeting the
target (${TARGET_NAME}-0) to ensure variable names match the top-of-file
definitions.
- Around line 117-120: The comment incorrectly expects PXCSTATE; update the text
to check only STATE for readiness and optionally mgrState if needed: remove the
"PXCSTATE = ready" reference and replace with a note that readiness is reflected
by .status.state (printed as STATE) and, if relevant, .status.mgr.state (printed
as mgrState), so the kubectl example should only assert STATE = ready (and
mention mgrState separately if required).
- Around line 398-405: The mysqldump invocation in the kubectl exec block (the
line starting with "mysqldump -uroot -p${SOURCE_MYSQL_PASSWORD}") uses malformed
flag spacing ("-- routines", "events", "triggers"); update those flags to the
correct forms --routines, --events, and --triggers (no space after dashes and
include dashes for all three) so the export command runs correctly.
- Around line 484-492: The IN-clause is being built incorrectly from the
DATABASES variable (used inside the kubectl mysql command) producing entries
like 'db1,','db2' instead of 'db1','db2'; fix it by building a properly quoted,
comma-separated list before embedding it in the SQL: take DATABASES, wrap each
database name in single quotes, join them with commas (ensuring no stray
commas/quotes), assign that result to a new variable (e.g., QUOTED_DATABASES)
and use IN (${QUOTED_DATABASES}) in the SELECT; update the kubectl exec command
to reference that new variable (symbols to change: DATABASES usage in the mysql
-e SQL, and the SELECT/WHERE IN clause).
In `@docs/en/upgrade.mdx`:
- Line 58: Replace the broken relative link target "../functions/01-create" used
in the "[Instance Creation Guide]" link in the upgrade page with the correct
relative path "./functions/01-create" so it points to
docs/en/functions/01-create.mdx.
In `@docs/shared/crds/middleware.alauda.io_mysqls.yaml`:
- Around line 12547-12568: The s3Config schema currently marks awsAccessKeyId
and awsSecretAccessKey as required but omits backupStorage, allowing configs
that lack the required backupStorage.bucket/name/namespace; update the s3Config
object schema (the block that lists required: - awsAccessKeyId -
awsSecretAccessKey) to also include backupStorage so it reads required: -
awsAccessKeyId - awsSecretAccessKey - backupStorage, ensuring the existing
backupStorage properties (bucket, name, namespace) remain as their own required
nested fields.
---
Nitpick comments:
In `@docs/shared/crds/middleware.alauda.io_mysqls.yaml`:
- Around line 10224-10227: The CRD schema for podManagementPolicy currently
allows any string; update the schema for podManagementPolicy to add an enum
constraint restricting values to the two valid StatefulSet options
(“OrderedReady” and “Parallel”) so invalid values are rejected at validation
time; locate the podManagementPolicy property in the CRD definition and add an
enum: ["OrderedReady","Parallel"] entry alongside the existing type: string and
description to enforce Kubernetes-compatible values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d2cf7dba-b3b8-419a-9372-99a11f85c626
📒 Files selected for processing (4)
docs/en/how_to/35-migrate-from-pxc.mdxdocs/en/upgrade.mdxdocs/shared/crds/middleware.alauda.io_mysqls.yamldocs/shared/functionresources/functionresources.yaml
- Fix mysqldump flag spacing: --routines, --events, --triggers - Replace undefined MYSQL_PASSWORD with SOURCE_MYSQL_PASSWORD (for source PXC pods) and TARGET_MYSQL_PASSWORD (for target MGR pods) - Fix DATABASES IN-clause quoting bug in fragmentation query - Remove broken link to deleted pxc-docs migrate-to-mgr guide - Fix relative link path in upgrade.mdx Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
docs/en/how_to/35-migrate-from-pxc.mdx (2)
540-550: Remove-itflag from non-interactive kubectl exec commands.Lines 544 and similar commands use
kubectl exec -itfor simple SQL query execution. The-itflags (interactive + TTY) are unnecessary for non-interactive commands and may cause issues in scripted/automated contexts or CI/CD pipelines.🔧 Simplify flags
# Test database connectivity -kubectl exec -it <app-pod> -n <app-namespace> -- \ +kubectl exec <app-pod> -n <app-namespace> -- \ mysql -h mysql-8-target-read-write.${TARGET_NAMESPACE}.svc.cluster.local \ -uroot -p${TARGET_MYSQL_PASSWORD} -e "SELECT 1 AS test;"Apply the same change to line 582 in the rollback verification section.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/en/how_to/35-migrate-from-pxc.mdx` around lines 540 - 550, The kubectl exec commands shown (e.g., the mysql connectivity test using "kubectl exec -it <app-pod> -n <app-namespace> -- mysql -h ... -e \"SELECT 1 AS test;\"") should remove the "-it" flags for non-interactive execution; update those occurrences (the connectivity test and the similar command in the rollback verification section) to use "kubectl exec -- ..." so they run cleanly in scripts/CI without requesting a TTY.
121-124: Clarify the expected status field names for MGR.Line 123 references
STATUS = Running, but line 336 expectsSTATE = Ready, MGRSTATE = ready. The field names and expected values are inconsistent. Confirm the correct column names returned bykubectl get mysqlfor MGR instances and update both locations to match.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/en/how_to/35-migrate-from-pxc.mdx` around lines 121 - 124, The docs show inconsistent status field names for MGR: update the "kubectl get mysql <target-name> -n <target-namespace>" example so its expected columns match the actual output (replace "STATUS = Running" with the real column name/value), and make the other instance that expects "STATE = Ready, MGRSTATE = ready" use the same canonical field names/values; run the command against the target operator/CRD to confirm whether the fields are "STATUS", "STATE", "MGRSTATE" (and their exact casing/values) and then standardize both snippets to that confirmed output.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/en/how_to/35-migrate-from-pxc.mdx`:
- Around line 396-413: The doc's "Migrate Users and Privileges" step is
incomplete: it exports mysql to /tmp/users.sql but never imports it and only
demonstrates altering a single user (app_user); update this to (1) enumerate
users from the source mysql.user (avoid system accounts) into a list (replacing
the /tmp/users.sql step), (2) for each user run SHOW GRANTS FOR 'user'@'host' on
the SOURCE (use kubectl exec ${SOURCE_NAME}-pxc-0 ... mysql -e "SHOW
GRANTS..."), pipe those GRANTS to the TARGET (${TARGET_NAME}-0) to create the
user and grants, (3) run ALTER USER '${user}'@'${host}' IDENTIFIED WITH
mysql_native_password on the TARGET when needed to ensure auth plugin
compatibility, and (4) run FLUSH PRIVILEGES on the TARGET; reference the
commands and artifacts in the diff (/tmp/users.sql, users_list.txt, SHOW GRANTS,
ALTER USER, FLUSH PRIVILEGES, kubectl exec ${SOURCE_NAME}-pxc-0 and
${TARGET_NAME}-0) so the steps can be scripted and replace the existing
fragment.
---
Nitpick comments:
In `@docs/en/how_to/35-migrate-from-pxc.mdx`:
- Around line 540-550: The kubectl exec commands shown (e.g., the mysql
connectivity test using "kubectl exec -it <app-pod> -n <app-namespace> -- mysql
-h ... -e \"SELECT 1 AS test;\"") should remove the "-it" flags for
non-interactive execution; update those occurrences (the connectivity test and
the similar command in the rollback verification section) to use "kubectl exec
-- ..." so they run cleanly in scripts/CI without requesting a TTY.
- Around line 121-124: The docs show inconsistent status field names for MGR:
update the "kubectl get mysql <target-name> -n <target-namespace>" example so
its expected columns match the actual output (replace "STATUS = Running" with
the real column name/value), and make the other instance that expects "STATE =
Ready, MGRSTATE = ready" use the same canonical field names/values; run the
command against the target operator/CRD to confirm whether the fields are
"STATUS", "STATE", "MGRSTATE" (and their exact casing/values) and then
standardize both snippets to that confirmed output.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5290eea8-67cc-4c9c-a3ee-45bda94f2736
📒 Files selected for processing (2)
docs/en/how_to/35-migrate-from-pxc.mdxdocs/en/upgrade.mdx
✅ Files skipped from review due to trivial changes (1)
- docs/en/upgrade.mdx
Address remaining CodeRabbit review comments: - Replace incomplete user migration stub with full scripted flow - Remove unnecessary -it flags from non-interactive kubectl exec Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/en/how_to/35-migrate-from-pxc.mdx`:
- Around line 336-337: Update the expected output line that currently reads
"STATE = Ready, MGRSTATE = ready" to use the repo's consistent MGR column naming
("mgrState") so it reads "STATE = Ready, mgrState = ready"; locate and edit the
string in the docs fragment around the example (the expected output block in the
migrate-from-pxc example) to ensure consistency with the rest of the doc and
repository behavior.
- Around line 414-421: The script only runs SHOW GRANTS and never captures or
applies the CREATE USER statement, so user authentication attributes are lost;
add a step that runs SHOW CREATE USER ${user_host} on the source (store in a
variable like CREATEUSER), execute that CREATEUSER statement against the target
(via kubectl exec to ${TARGET_NAME}-0 -c mysql), and only then proceed to
retrieve GRANTS and apply them; ensure this CREATEUSER application occurs before
any ALTER USER or grant application so authentication/plugin/password details
are preserved for user_host.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d2e76354-244b-4312-b007-d309ca2bdc36
📒 Files selected for processing (1)
docs/en/how_to/35-migrate-from-pxc.mdx
SHOW CREATE USER preserves auth plugin and password hash, ensuring users can log in with existing passwords after migration. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (3)
docs/en/how_to/35-migrate-from-pxc.mdx (3)
280-280: Document the single-member cluster choice.The YAML creates a single-member MGR cluster (
members: 1), but the guide doesn't explain why a 1-member cluster is recommended for the initial migration instead of a full 3-member deployment. Consider adding a brief comment or note explaining whether this is for simplicity during migration, with intent to scale up after validation, or if there's another operational reason.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/en/how_to/35-migrate-from-pxc.mdx` at line 280, Add a short explanatory note near the YAML entry that sets members: 1 to clarify the single-member MGR cluster choice; mention that the guide uses a 1-member MGR for initial migration/testing simplicity and reduced quorum/complexity during validation, and state the intent to scale to a multi-member (e.g., 3-member) MGR deployment for production post-validation. Place this note next to the YAML block that defines members: 1 or immediately above the "single-member MGR cluster" reference so readers see the rationale and recommended follow-up step to scale.
475-476: Consider escaping view names for defensive coding.The view verification query uses
${view}directly without backticks. While MySQL view names frominformation_schema.VIEWStypically don't require escaping, using backticks provides defense against edge cases with reserved words or unusual identifiers.🛡️ Optional defensive improvement
kubectl exec ${TARGET_NAME}-0 -n ${TARGET_NAMESPACE} -c mysql -- \ - mysql -uroot -p${TARGET_MYSQL_PASSWORD} ${db} -e "SELECT 1 FROM ${view} LIMIT 1;" > /dev/null 2>&1 \ + mysql -uroot -p${TARGET_MYSQL_PASSWORD} ${db} -e "SELECT 1 FROM \`${view}\` LIMIT 1;" > /dev/null 2>&1 \ && echo "✓ View ${view} OK" \ || echo "✗ View ${view} FAILED"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/en/how_to/35-migrate-from-pxc.mdx` around lines 475 - 476, The command that verifies views injects ${view} directly into the SQL (the mysql exec line containing SELECT 1 FROM ${view} LIMIT 1;), which can break on reserved words or odd identifiers; update the command to wrap the view identifier in backticks (e.g., use SELECT 1 FROM \`${view}\` LIMIT 1;) so the ${view} substitution is quoted defensively when running the kubectl exec ... mysql -e invocation.
696-718: Document that TABLE_ROWS is an estimate.The data integrity verification uses
information_schema.TABLES.TABLE_ROWS, which provides an estimated row count (particularly for InnoDB). For critical validation, consider noting that exact counts requireSELECT COUNT(*)from each table, though that's more expensive. The estimation is usually sufficient for migration verification, but documenting this limitation helps users understand potential minor discrepancies.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/en/how_to/35-migrate-from-pxc.mdx` around lines 696 - 718, The current verification step reads information_schema.TABLES.TABLE_ROWS (used in the SELECT query) which is an estimated row count for some engines (e.g., InnoDB); update the documentation around the loop that runs the SELECT from information_schema.TABLES to explicitly state TABLE_ROWS is an estimate and add a note that for exact validation you should run a per-table SELECT COUNT(*) (acknowledging the higher cost), referencing the SELECT ... FROM information_schema.TABLES and the alternative SELECT COUNT(*) as the precise-method for users who need strict verification.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/en/how_to/35-migrate-from-pxc.mdx`:
- Line 280: Add a short explanatory note near the YAML entry that sets members:
1 to clarify the single-member MGR cluster choice; mention that the guide uses a
1-member MGR for initial migration/testing simplicity and reduced
quorum/complexity during validation, and state the intent to scale to a
multi-member (e.g., 3-member) MGR deployment for production post-validation.
Place this note next to the YAML block that defines members: 1 or immediately
above the "single-member MGR cluster" reference so readers see the rationale and
recommended follow-up step to scale.
- Around line 475-476: The command that verifies views injects ${view} directly
into the SQL (the mysql exec line containing SELECT 1 FROM ${view} LIMIT 1;),
which can break on reserved words or odd identifiers; update the command to wrap
the view identifier in backticks (e.g., use SELECT 1 FROM \`${view}\` LIMIT 1;)
so the ${view} substitution is quoted defensively when running the kubectl exec
... mysql -e invocation.
- Around line 696-718: The current verification step reads
information_schema.TABLES.TABLE_ROWS (used in the SELECT query) which is an
estimated row count for some engines (e.g., InnoDB); update the documentation
around the loop that runs the SELECT from information_schema.TABLES to
explicitly state TABLE_ROWS is an estimate and add a note that for exact
validation you should run a per-table SELECT COUNT(*) (acknowledging the higher
cost), referencing the SELECT ... FROM information_schema.TABLES and the
alternative SELECT COUNT(*) as the precise-method for users who need strict
verification.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 06d0d1a5-16ae-4b0d-a9a8-3ab9ad8bdc8e
📒 Files selected for processing (1)
docs/en/how_to/35-migrate-from-pxc.mdx
Change `####` (h4) to `###` (h3) in the PXC pre-upgrade checklist to satisfy remark-lint-heading-increment rule. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Deploying alauda-mysql-mgr with
|
| Latest commit: |
66c3c3d
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://a5ad0a42.alauda-mysql-mgr.pages.dev |
| Branch Preview URL: | https://fix-pxc-migration-docs.alauda-mysql-mgr.pages.dev |
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
1 participant
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 by CodeRabbit
Documentation
Updates