K8SPS-548: fix comparePrimaryPurged function
#1125
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes rare test failures in the gr-self-healing test by addressing type safety issues and improving error handling in MySQL shell operations. The changes ensure better compatibility with different MySQL shell JSON response formats that could cause intermittent test failures.
- Changed string type assertions to handle
anytypes from JSON responses - Added proper error handling and type checking for SQL result parsing
- Removed unnecessary blank lines in test files for cleaner formatting
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
cmd/bootstrap/gr/recovery_method_test.go |
Updated mock SQL runner to use any type and removed extra blank lines in tests |
cmd/bootstrap/gr/recovery_method.go |
Added type assertions and error handling for SQL results, updated function signatures |
cmd/bootstrap/gr/group_replication.go |
Changed SQL result type from string to any, added new primary partition check method |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| inPrimary, err := shell.checkIfInPrimaryPartition(ctx) | ||
| if err != nil { | ||
| return errors.Wrap(err, "check if member in primary partition") | ||
| } | ||
| if !inPrimary { | ||
| log.Printf("Instance (%s) is not in primary partition. Starting full cluster crash recovery...", localShell.host) | ||
|
|
||
| if err := handleFullClusterCrash(ctx, mysqlshVer); err != nil { | ||
| return errors.Wrap(err, "handle full cluster crash") | ||
| } | ||
|
|
||
| // force restart container | ||
| os.Exit(1) | ||
| } |
There was a problem hiding this comment.
not sure about this: handleFullClusterCrash is going to create /var/lib/mysql/full-cluster-crash in this pod and it is going to trigger recovery in operator. but do we need to reboot the whole cluster in this case? if pod is not in the primary partition it needs to be restarted but do we need to touch other pods? probably i am missing something since we have the same check in liveness probe and that would be enough if restarting the pod was enough.
There was a problem hiding this comment.
I think this is the only way to handle the situation described in the PR description. Maybe we should store a counter of pod restarts and trigger a full cluster crash if there are too many restarts?
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
pooknull
changed the title
gr-self-healing test failurescomparePrimaryPurged function
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
commit: 703ecc3 |
| sub, err := strconv.Atoi(v) | ||
| if err != nil { | ||
| return false | ||
| s, ok := v.(float64) |
There was a problem hiding this comment.
why float and not int? as far as i remember SELECT GTID_SUBTRACT('%s', '%s') = '' returns 0 or 1
https://perconadev.atlassian.net/browse/K8SPS-548
DESCRIPTION
This PR was originally tried to address rare
gr-self-healingtest failures, but the initial change did not resolve the issue and was removed.The PR still contains a small fix for
comparePrimaryPurgedfunction. Previously, the function always returnedfalsebecause the map returned byrunSQLused keys such asGTID_SUBTRACT()instead ofGTID_SUBTRACT. The function couldn't findGTID_SUBTRACTand returnedfalseCHECKLIST
Jira
Needs Doc) and QA (Needs QA)?Tests
Config/Logging/Testability