From f69c942b6f30b5b97c83380366edc50f4be1dc1f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Canna=C3=B2?= Date: Tue, 28 Jul 2026 00:34:50 +0200 Subject: [PATCH 1/2] feat: optional fail recovery when post-failover hooks fail (#110) Add FailRecoveryOnFailedPostFailoverProcesses (default false). When enabled, non-zero exits from PostFailoverProcesses, PostMasterFailoverProcesses, PostIntermediateMasterFailoverProcesses, and PostGracefulTakeoverProcesses mark the recovery unsuccessful, persist that state, return an error, and count metrics as failed. Topology changes are not rolled back. Async hooks (&) remain best-effort. Legacy behavior unchanged when the flag is false. Includes unit tests for executeProcesses failOnError and the policy wrapper, sample config, and docs. --- conf/orchestrator-sample-sqlite.conf.json | 1 + conf/orchestrator-sample.conf.json | 1 + docs/configuration-recovery.md | 11 +- go/config/config.go | 6 + go/config/config_post_failover_test.go | 10 + go/logic/post_failover_hooks_test.go | 217 ++++++++++++++++++++++ go/logic/topology_recovery.go | 120 +++++++++--- go/logic/topology_recovery_postgresql.go | 12 +- 8 files changed, 351 insertions(+), 27 deletions(-) create mode 100644 go/config/config_post_failover_test.go create mode 100644 go/logic/post_failover_hooks_test.go diff --git a/conf/orchestrator-sample-sqlite.conf.json b/conf/orchestrator-sample-sqlite.conf.json index 4357b3d23..c7700a6cc 100644 --- a/conf/orchestrator-sample-sqlite.conf.json +++ b/conf/orchestrator-sample-sqlite.conf.json @@ -107,6 +107,7 @@ "PreFailoverProcesses": [ "echo 'Will recover from {failureType} on {failureCluster}' >> /tmp/recovery.log" ], + "FailRecoveryOnFailedPostFailoverProcesses": false, "PostFailoverProcesses": [ "echo '(for all types) Recovered from {failureType} on {failureCluster}. Failed: {failedHost}:{failedPort}; Successor: {successorHost}:{successorPort}' >> /tmp/recovery.log" ], diff --git a/conf/orchestrator-sample.conf.json b/conf/orchestrator-sample.conf.json index 4752148a6..6d30a8665 100644 --- a/conf/orchestrator-sample.conf.json +++ b/conf/orchestrator-sample.conf.json @@ -116,6 +116,7 @@ "PreFailoverProcesses": [ "echo 'Will recover from {failureType} on {failureCluster}' >> /tmp/recovery.log" ], + "FailRecoveryOnFailedPostFailoverProcesses": false, "PostFailoverProcesses": [ "echo '(for all types) Recovered from {failureType} on {failureCluster}. Failed: {failedHost}:{failedPort}; Successor: {successorHost}:{successorPort}' >> /tmp/recovery.log" ], diff --git a/docs/configuration-recovery.md b/docs/configuration-recovery.md index a96dfcbbe..03e8e7915 100644 --- a/docs/configuration-recovery.md +++ b/docs/configuration-recovery.md @@ -76,8 +76,15 @@ These hooks are available for recoveries: - `PostFailoverProcesses`: executed at the end of any successful recovery (including and adding to the above two). - `PostUnsuccessfulFailoverProcesses`: executed at the end of any unsuccessful recovery. - `PostGracefulTakeoverProcesses`: executed on planned, graceful master takeover, after the old master is positioned under the newly promoted master. - -Any process command that ends with `"&"` will be executed asynchronously, and a failure for such process is ignored. +- `FailRecoveryOnFailedPostFailoverProcesses`: when `true` (default `false`), a non-zero exit from the successful-path + post hooks above (`PostFailoverProcesses`, `PostMasterFailoverProcesses`, `PostIntermediateMasterFailoverProcesses`, + `PostGracefulTakeoverProcesses`) marks the recovery as **unsuccessful** in orchestrator's recovery history, returns + an error to the caller, and counts the recovery as failed in metrics. Topology changes already performed are **not** + rolled back. Use this when external hooks (for example service discovery updates) are part of your recovery SLO. + When `false`, hook failures are audited/logged but the recovery remains successful (legacy behavior). + +Any process command that ends with `"&"` will be executed asynchronously, and a failure for such process is ignored +(including when `FailRecoveryOnFailedPostFailoverProcesses` is `true`). All of the above are lists of commands which `orchestrator` executes sequentially, in order of definition. diff --git a/go/config/config.go b/go/config/config.go index fc126c791..c9e18b13a 100644 --- a/go/config/config.go +++ b/go/config/config.go @@ -232,6 +232,11 @@ type Configuration struct { PostIntermediateMasterFailoverProcesses []string // Processes to execute after doing a master failover (order of execution undefined). Uses same placeholders as PostFailoverProcesses PostGracefulTakeoverProcesses []string // Processes to execute after running a graceful master takeover. Uses same placeholders as PostFailoverProcesses PostTakeMasterProcesses []string // Processes to execute after a successful Take-Master event has taken place + // FailRecoveryOnFailedPostFailoverProcesses, when true, treats a non-zero exit from + // PostFailoverProcesses / PostMasterFailoverProcesses / PostIntermediateMasterFailoverProcesses / + // PostGracefulTakeoverProcesses as a failed recovery (IsSuccessful=false, error returned). + // Topology changes already performed are not rolled back. Default false preserves legacy behavior. + FailRecoveryOnFailedPostFailoverProcesses bool RecoverNonWriteableMaster bool // When 'true', orchestrator treats a read-only master as a failure scenario and attempts to make the master writeable CoMasterRecoveryMustPromoteOtherCoMaster bool // When 'false', anything can get promoted (and candidates are preferred over others). When 'true', orchestrator will promote the other co-master or else fail DetachLostSlavesAfterMasterFailover bool // synonym to DetachLostReplicasAfterMasterFailover @@ -433,6 +438,7 @@ func newConfiguration() *Configuration { PostUnsuccessfulFailoverProcesses: []string{}, PostGracefulTakeoverProcesses: []string{}, PostTakeMasterProcesses: []string{}, + FailRecoveryOnFailedPostFailoverProcesses: false, RecoverNonWriteableMaster: false, CoMasterRecoveryMustPromoteOtherCoMaster: true, DetachLostSlavesAfterMasterFailover: true, diff --git a/go/config/config_post_failover_test.go b/go/config/config_post_failover_test.go new file mode 100644 index 000000000..3e0e1f100 --- /dev/null +++ b/go/config/config_post_failover_test.go @@ -0,0 +1,10 @@ +package config + +import "testing" + +func TestFailRecoveryOnFailedPostFailoverProcessesDefault(t *testing.T) { + c := newConfiguration() + if c.FailRecoveryOnFailedPostFailoverProcesses { + t.Fatal("default must be false for backward compatibility") + } +} diff --git a/go/logic/post_failover_hooks_test.go b/go/logic/post_failover_hooks_test.go new file mode 100644 index 000000000..6fe28eae1 --- /dev/null +++ b/go/logic/post_failover_hooks_test.go @@ -0,0 +1,217 @@ +/* + Copyright 2026 ProxySQL Authors + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +*/ + +package logic + +import ( + "errors" + "os/exec" + "strings" + "testing" + + "github.com/proxysql/orchestrator/go/config" + "github.com/proxysql/orchestrator/go/inst" +) + +func stubAuditNoBackend(t *testing.T) { + t.Helper() + orig := auditTopologyRecoveryFn + auditTopologyRecoveryFn = func(tr *TopologyRecovery, message string) error { + return nil + } + t.Cleanup(func() { auditTopologyRecoveryFn = orig }) +} + +func TestFailOnPostFailoverHookErrorFollowsConfig(t *testing.T) { + orig := config.Config.FailRecoveryOnFailedPostFailoverProcesses + defer func() { config.Config.FailRecoveryOnFailedPostFailoverProcesses = orig }() + + config.Config.FailRecoveryOnFailedPostFailoverProcesses = false + if failOnPostFailoverHookError() { + t.Fatal("expected false when config disabled") + } + config.Config.FailRecoveryOnFailedPostFailoverProcesses = true + if !failOnPostFailoverHookError() { + t.Fatal("expected true when config enabled") + } +} + +func TestMarkRecoveryUnsuccessfulDueToHooks(t *testing.T) { + stubAuditNoBackend(t) + tr := NewTopologyRecovery(inst.ReplicationAnalysis{}) + tr.IsSuccessful = true + + markRecoveryUnsuccessfulDueToHooks(tr, errors.New("hook boom")) + if tr.IsSuccessful { + t.Fatal("expected IsSuccessful=false") + } + if len(tr.AllErrors) == 0 { + t.Fatal("expected error recorded on topology recovery") + } + if !strings.Contains(tr.AllErrors[0], "post-failover hooks failed") { + t.Fatalf("unexpected error text: %q", tr.AllErrors[0]) + } + + // nil-safe + markRecoveryUnsuccessfulDueToHooks(nil, errors.New("x")) + markRecoveryUnsuccessfulDueToHooks(tr, nil) +} + +func TestExecuteProcessesFailOnError(t *testing.T) { + stubAuditNoBackend(t) + if _, err := exec.LookPath("false"); err != nil { + t.Skip("false binary not available") + } + tr := NewTopologyRecovery(inst.ReplicationAnalysis{}) + + err := executeProcesses([]string{"false"}, "TestHooks", tr, true) + if err == nil { + t.Fatal("expected error when failOnError=true and command fails") + } + + err = executeProcesses([]string{"false", "true"}, "TestHooks", tr, false) + // failOnError=false: returns first error but continues (true still runs) + if err == nil { + t.Fatal("expected first command error to be returned even when failOnError=false") + } + + err = executeProcesses([]string{"true"}, "TestHooks", tr, true) + if err != nil { + t.Fatalf("true should succeed: %v", err) + } + + err = executeProcesses(nil, "TestHooks", tr, true) + if err != nil { + t.Fatalf("empty hooks should succeed: %v", err) + } +} + +func TestRunPostSuccessFailoverProcessesPolicyOff(t *testing.T) { + stubAuditNoBackend(t) + if _, err := exec.LookPath("false"); err != nil { + t.Skip("false binary not available") + } + orig := config.Config.FailRecoveryOnFailedPostFailoverProcesses + defer func() { config.Config.FailRecoveryOnFailedPostFailoverProcesses = orig }() + config.Config.FailRecoveryOnFailedPostFailoverProcesses = false + + tr := NewTopologyRecovery(inst.ReplicationAnalysis{}) + tr.IsSuccessful = true + key := inst.InstanceKey{Hostname: "successor.example", Port: 3306} + tr.SuccessorKey = &key + + err := runPostSuccessFailoverProcesses([]string{"false"}, "PostFailoverProcesses", tr) + if err != nil { + t.Fatalf("policy off should not return error: %v", err) + } + if !tr.IsSuccessful { + t.Fatal("policy off must not flip IsSuccessful") + } +} + +func TestRunPostSuccessFailoverProcessesPolicyOn(t *testing.T) { + stubAuditNoBackend(t) + if _, err := exec.LookPath("false"); err != nil { + t.Skip("false binary not available") + } + orig := config.Config.FailRecoveryOnFailedPostFailoverProcesses + defer func() { config.Config.FailRecoveryOnFailedPostFailoverProcesses = orig }() + config.Config.FailRecoveryOnFailedPostFailoverProcesses = true + + origPersist := persistResolvedRecoveryFn + persistCalls := 0 + persistResolvedRecoveryFn = func(tr *TopologyRecovery) error { + persistCalls++ + if tr.IsSuccessful { + t.Error("persist should see IsSuccessful=false") + } + return nil + } + defer func() { persistResolvedRecoveryFn = origPersist }() + + tr := NewTopologyRecovery(inst.ReplicationAnalysis{}) + tr.IsSuccessful = true + key := inst.InstanceKey{Hostname: "successor.example", Port: 3306} + tr.SuccessorKey = &key + + err := runPostSuccessFailoverProcesses([]string{"false"}, "PostFailoverProcesses", tr) + if err == nil { + t.Fatal("policy on should return hook error") + } + if tr.IsSuccessful { + t.Fatal("policy on must set IsSuccessful=false") + } + if persistCalls != 1 { + t.Fatalf("expected 1 persist call, got %d", persistCalls) + } +} + +func TestRunPostSuccessFailoverProcessesPolicyOnPersistError(t *testing.T) { + stubAuditNoBackend(t) + if _, err := exec.LookPath("false"); err != nil { + t.Skip("false binary not available") + } + orig := config.Config.FailRecoveryOnFailedPostFailoverProcesses + defer func() { config.Config.FailRecoveryOnFailedPostFailoverProcesses = orig }() + config.Config.FailRecoveryOnFailedPostFailoverProcesses = true + + origPersist := persistResolvedRecoveryFn + persistResolvedRecoveryFn = func(tr *TopologyRecovery) error { + return errors.New("db down") + } + defer func() { persistResolvedRecoveryFn = origPersist }() + + tr := NewTopologyRecovery(inst.ReplicationAnalysis{}) + tr.IsSuccessful = true + + err := runPostSuccessFailoverProcesses([]string{"false"}, "PostFailoverProcesses", tr) + if err == nil { + t.Fatal("expected combined error") + } + if !strings.Contains(err.Error(), "db down") { + t.Fatalf("expected persist error in message: %v", err) + } + if !strings.Contains(err.Error(), "post-failover hooks failed") { + t.Fatalf("expected hook failure in message: %v", err) + } +} + +func TestRunPostSuccessFailoverProcessesSuccess(t *testing.T) { + stubAuditNoBackend(t) + if _, err := exec.LookPath("true"); err != nil { + t.Skip("true binary not available") + } + orig := config.Config.FailRecoveryOnFailedPostFailoverProcesses + defer func() { config.Config.FailRecoveryOnFailedPostFailoverProcesses = orig }() + config.Config.FailRecoveryOnFailedPostFailoverProcesses = true + + origPersist := persistResolvedRecoveryFn + persistResolvedRecoveryFn = func(tr *TopologyRecovery) error { + t.Fatal("persist should not run on success") + return nil + } + defer func() { persistResolvedRecoveryFn = origPersist }() + + tr := NewTopologyRecovery(inst.ReplicationAnalysis{}) + tr.IsSuccessful = true + + if err := runPostSuccessFailoverProcesses([]string{"true"}, "PostFailoverProcesses", tr); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !tr.IsSuccessful { + t.Fatal("success should keep IsSuccessful") + } +} diff --git a/go/logic/topology_recovery.go b/go/logic/topology_recovery.go index 0c56775ff..a2ebb26e2 100644 --- a/go/logic/topology_recovery.go +++ b/go/logic/topology_recovery.go @@ -234,7 +234,7 @@ func initializeTopologyRecoveryPostConfiguration() { } // AuditTopologyRecovery audits a single step in a topology recovery process. -func AuditTopologyRecovery(topologyRecovery *TopologyRecovery, message string) error { +func auditTopologyRecoveryDefault(topologyRecovery *TopologyRecovery, message string) error { log.Infof("topology_recovery: %s", message) if topologyRecovery == nil { return nil @@ -244,9 +244,16 @@ func AuditTopologyRecovery(topologyRecovery *TopologyRecovery, message string) e if orcraft.IsRaftEnabled() { _, err := orcraft.PublishCommand("write-recovery-step", recoveryStep) return err - } else { - return writeTopologyRecoveryStep(recoveryStep) } + return writeTopologyRecoveryStep(recoveryStep) +} + +// auditTopologyRecoveryFn persists recovery audit steps. Overridable in unit tests +// to avoid requiring a backend database. +var auditTopologyRecoveryFn = auditTopologyRecoveryDefault + +func AuditTopologyRecovery(topologyRecovery *TopologyRecovery, message string) error { + return auditTopologyRecoveryFn(topologyRecovery, message) } func resolveRecovery(topologyRecovery *TopologyRecovery, successorInstance *inst.Instance) error { @@ -407,6 +414,60 @@ func executeProcesses(processes []string, description string, topologyRecovery * return err } +// failOnPostFailoverHookError reports whether post-success failover hooks should +// fail the overall recovery when a hook exits non-zero. +func failOnPostFailoverHookError() bool { + return config.Config.FailRecoveryOnFailedPostFailoverProcesses +} + +// markRecoveryUnsuccessfulDueToHooks flips a previously successful recovery to +// unsuccessful in memory after post-failover hooks failed. Caller persists state. +func markRecoveryUnsuccessfulDueToHooks(topologyRecovery *TopologyRecovery, hookErr error) { + if topologyRecovery == nil || hookErr == nil { + return + } + topologyRecovery.IsSuccessful = false + _ = topologyRecovery.AddError(fmt.Errorf("post-failover hooks failed: %w", hookErr)) + AuditTopologyRecovery(topologyRecovery, fmt.Sprintf("Marking recovery unsuccessful due to post-failover hook failure: %+v", hookErr)) +} + +// persistResolvedRecovery writes the current IsSuccessful/successor fields for a recovery. +func persistResolvedRecovery(topologyRecovery *TopologyRecovery) error { + if topologyRecovery == nil { + return nil + } + if orcraft.IsRaftEnabled() { + _, err := orcraft.PublishCommand("resolve-recovery", topologyRecovery) + return err + } + return writeResolveRecovery(topologyRecovery) +} + +// persistResolvedRecoveryFn is the persistence hook used after post-failover hooks +// fail the recovery. Overridable in unit tests. +var persistResolvedRecoveryFn = persistResolvedRecovery + +// runPostSuccessFailoverProcesses runs hooks used after a successful promotion. +// When FailRecoveryOnFailedPostFailoverProcesses is true, a hook failure marks the +// recovery unsuccessful, persists that state, and returns the hook error. +// Async hooks (commands ending with &) never fail the recovery. +func runPostSuccessFailoverProcesses(processes []string, description string, topologyRecovery *TopologyRecovery) error { + failOnError := failOnPostFailoverHookError() + err := executeProcesses(processes, description, topologyRecovery, failOnError) + if err == nil { + return nil + } + if !failOnError { + AuditTopologyRecovery(topologyRecovery, fmt.Sprintf("%s completed with errors (ignored for recovery success): %+v", description, err)) + return nil + } + markRecoveryUnsuccessfulDueToHooks(topologyRecovery, err) + if persistErr := persistResolvedRecoveryFn(topologyRecovery); persistErr != nil { + return fmt.Errorf("post-failover hooks failed (%v); also failed to persist recovery state: %w", err, persistErr) + } + return err +} + func recoverDeadMasterInBinlogServerTopology(topologyRecovery *TopologyRecovery) (promotedReplica *inst.Instance, err error) { failedMasterKey := &topologyRecovery.AnalysisEntry.AnalyzedInstanceKey @@ -1016,7 +1077,9 @@ func checkAndRecoverDeadMaster(analysisEntry inst.ReplicationAnalysis, candidate if !skipProcesses { // Execute post master-failover processes - executeProcesses(config.Config.PostMasterFailoverProcesses, "PostMasterFailoverProcesses", topologyRecovery, false) + if hookErr := runPostSuccessFailoverProcesses(config.Config.PostMasterFailoverProcesses, "PostMasterFailoverProcesses", topologyRecovery); hookErr != nil { + return true, topologyRecovery, hookErr + } } } else { recoverDeadMasterFailureCounter.Inc(1) @@ -1324,7 +1387,9 @@ func checkAndRecoverDeadIntermediateMaster(analysisEntry inst.ReplicationAnalysi // Execute post intermediate-master-failover processes topologyRecovery.SuccessorKey = &promotedReplica.Key topologyRecovery.SuccessorAlias = promotedReplica.InstanceAlias - executeProcesses(config.Config.PostIntermediateMasterFailoverProcesses, "PostIntermediateMasterFailoverProcesses", topologyRecovery, false) + if hookErr := runPostSuccessFailoverProcesses(config.Config.PostIntermediateMasterFailoverProcesses, "PostIntermediateMasterFailoverProcesses", topologyRecovery); hookErr != nil { + return true, topologyRecovery, hookErr + } } } else { recoverDeadIntermediateMasterFailureCounter.Inc(1) @@ -1486,10 +1551,12 @@ func checkAndRecoverDeadCoMaster(analysisEntry inst.ReplicationAnalysis, candida _, _ = inst.SetReadOnly(&promotedReplica.Key, false) } if !skipProcesses { - // Execute post intermediate-master-failover processes + // Execute post master-failover processes (co-master promotion) topologyRecovery.SuccessorKey = &promotedReplica.Key topologyRecovery.SuccessorAlias = promotedReplica.InstanceAlias - executeProcesses(config.Config.PostMasterFailoverProcesses, "PostMasterFailoverProcesses", topologyRecovery, false) + if hookErr := runPostSuccessFailoverProcesses(config.Config.PostMasterFailoverProcesses, "PostMasterFailoverProcesses", topologyRecovery); hookErr != nil { + return true, topologyRecovery, hookErr + } } } else { recoverDeadCoMasterFailureCounter.Inc(1) @@ -1631,7 +1698,9 @@ func checkAndRecoverDeadGroupMemberWithReplicas(analysisEntry inst.ReplicationAn topologyRecovery.SuccessorKey = &recoveredToGroupMember.Key topologyRecovery.SuccessorAlias = recoveredToGroupMember.InstanceAlias // For the same reasons that were mentioned above, we re-use the post intermediate master fail-over hooks - executeProcesses(config.Config.PostIntermediateMasterFailoverProcesses, "PostIntermediateMasterFailoverProcesses", topologyRecovery, false) + if hookErr := runPostSuccessFailoverProcesses(config.Config.PostIntermediateMasterFailoverProcesses, "PostIntermediateMasterFailoverProcesses", topologyRecovery); hookErr != nil { + return true, topologyRecovery, hookErr + } } } else { recoverDeadReplicationGroupMemberFailureCounter.Inc(1) @@ -1930,24 +1999,13 @@ func executeCheckAndRecoverFunction(analysisEntry inst.ReplicationAnalysis, cand startTime := time.Now() recoveryAttempted, topologyRecovery, err = checkAndRecoverFunction(analysisEntry, candidateInstanceKey, forceInstanceRecovery, skipProcesses) - if recoveryAttempted && topologyRecovery != nil { - duration := time.Since(startTime).Seconds() - ometrics.RecoveryDurationSeconds.Observe(duration) - - result := "success" - if !topologyRecovery.IsSuccessful { - result = "failed" - } - ometrics.RecoveriesTotal.WithLabelValues(string(analysisEntry.Analysis), result).Inc() - } - if !recoveryAttempted { return recoveryAttempted, topologyRecovery, err } if topologyRecovery == nil { return recoveryAttempted, topologyRecovery, err } - if b, err := json.Marshal(topologyRecovery); err == nil { + if b, marshalErr := json.Marshal(topologyRecovery); marshalErr == nil { log.Infof("Topology recovery: %+v", string(b)) } else { log.Infof("Topology recovery: %+v", topologyRecovery) @@ -1959,9 +2017,23 @@ func executeCheckAndRecoverFunction(analysisEntry inst.ReplicationAnalysis, cand } else { // Execute general post failover processes _, _ = inst.EndDowntime(topologyRecovery.SuccessorKey) - executeProcesses(config.Config.PostFailoverProcesses, "PostFailoverProcesses", topologyRecovery, false) + if hookErr := runPostSuccessFailoverProcesses(config.Config.PostFailoverProcesses, "PostFailoverProcesses", topologyRecovery); hookErr != nil { + if err == nil { + err = hookErr + } + } } } + // Record metrics after post-failover hooks so FailRecoveryOnFailedPostFailoverProcesses + // can still flip IsSuccessful before counters are emitted. + duration := time.Since(startTime).Seconds() + ometrics.RecoveryDurationSeconds.Observe(duration) + result := "success" + if !topologyRecovery.IsSuccessful { + result = "failed" + } + ometrics.RecoveriesTotal.WithLabelValues(string(analysisEntry.Analysis), result).Inc() + AuditTopologyRecovery(topologyRecovery, fmt.Sprintf("Waiting for %d postponed functions", topologyRecovery.Len())) topologyRecovery.Wait() AuditTopologyRecovery(topologyRecovery, fmt.Sprintf("Executed %d postponed functions", topologyRecovery.Len())) @@ -2317,7 +2389,11 @@ func GracefulMasterTakeover(clusterName string, designatedKey *inst.InstanceKey, AuditTopologyRecovery(topologyRecovery, fmt.Sprintf("ProxySQL post-graceful-takeover failed: %v", err)) } } - executeProcesses(config.Config.PostGracefulTakeoverProcesses, "PostGracefulTakeoverProcesses", topologyRecovery, false) + if hookErr := runPostSuccessFailoverProcesses(config.Config.PostGracefulTakeoverProcesses, "PostGracefulTakeoverProcesses", topologyRecovery); hookErr != nil { + if err == nil { + err = hookErr + } + } return topologyRecovery, promotedMasterCoordinates, err } diff --git a/go/logic/topology_recovery_postgresql.go b/go/logic/topology_recovery_postgresql.go index 48583c217..18a6c293f 100644 --- a/go/logic/topology_recovery_postgresql.go +++ b/go/logic/topology_recovery_postgresql.go @@ -99,8 +99,12 @@ func checkAndRecoverDeadPrimary(analysisEntry inst.ReplicationAnalysis, candidat AuditTopologyRecovery(topologyRecovery, fmt.Sprintf("RecoverDeadPrimary: successfully promoted %+v", promotedInstance.Key)) if !skipProcesses { topologyRecovery.SuccessorKey = &promotedInstance.Key - executeProcesses(config.Config.PostMasterFailoverProcesses, "PostMasterFailoverProcesses", topologyRecovery, false) - executeProcesses(config.Config.PostFailoverProcesses, "PostFailoverProcesses", topologyRecovery, false) + if hookErr := runPostSuccessFailoverProcesses(config.Config.PostMasterFailoverProcesses, "PostMasterFailoverProcesses", topologyRecovery); hookErr != nil { + return true, topologyRecovery, hookErr + } + if hookErr := runPostSuccessFailoverProcesses(config.Config.PostFailoverProcesses, "PostFailoverProcesses", topologyRecovery); hookErr != nil { + return true, topologyRecovery, hookErr + } } } else { if !skipProcesses { @@ -246,7 +250,9 @@ func PostgreSQLGracefulPrimarySwitchover(clusterName string, designatedKey *inst resolveRecovery(topologyRecovery, promotedInstance) // --- Execute PostGracefulTakeoverProcesses --- - executeProcesses(config.Config.PostGracefulTakeoverProcesses, "PostGracefulTakeoverProcesses", topologyRecovery, false) + if hookErr := runPostSuccessFailoverProcesses(config.Config.PostGracefulTakeoverProcesses, "PostGracefulTakeoverProcesses", topologyRecovery); hookErr != nil { + return topologyRecovery, hookErr + } AuditTopologyRecovery(topologyRecovery, fmt.Sprintf("PostgreSQLGracefulPrimarySwitchover: completed. New primary: %+v", promotedInstance.Key)) return topologyRecovery, nil From 63ed903cb79ff9483b84decbf1fd7d8d2a5864fd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Canna=C3=B2?= Date: Tue, 28 Jul 2026 12:06:02 +0200 Subject: [PATCH 2/2] fix(lint): errcheck AuditTopologyRecovery and gofmt config alignment --- go/config/config.go | 100 +++++++++++++++++----------------- go/logic/topology_recovery.go | 4 +- 2 files changed, 52 insertions(+), 52 deletions(-) diff --git a/go/config/config.go b/go/config/config.go index c9e18b13a..77bd1393d 100644 --- a/go/config/config.go +++ b/go/config/config.go @@ -237,56 +237,56 @@ type Configuration struct { // PostGracefulTakeoverProcesses as a failed recovery (IsSuccessful=false, error returned). // Topology changes already performed are not rolled back. Default false preserves legacy behavior. FailRecoveryOnFailedPostFailoverProcesses bool - RecoverNonWriteableMaster bool // When 'true', orchestrator treats a read-only master as a failure scenario and attempts to make the master writeable - CoMasterRecoveryMustPromoteOtherCoMaster bool // When 'false', anything can get promoted (and candidates are preferred over others). When 'true', orchestrator will promote the other co-master or else fail - DetachLostSlavesAfterMasterFailover bool // synonym to DetachLostReplicasAfterMasterFailover - DetachLostReplicasAfterMasterFailover bool // Should replicas that are not to be lost in master recovery (i.e. were more up-to-date than promoted replica) be forcibly detached - ApplyMySQLPromotionAfterMasterFailover bool // Should orchestrator take upon itself to apply MySQL master promotion: set read_only=0, detach replication, etc. - PreventCrossDataCenterMasterFailover bool // When true (default: false), cross-DC master failover are not allowed, orchestrator will do all it can to only fail over within same DC, or else not fail over at all. - PreventCrossRegionMasterFailover bool // When true (default: false), cross-region master failover are not allowed, orchestrator will do all it can to only fail over within same region, or else not fail over at all. - MasterFailoverLostInstancesDowntimeMinutes uint // Number of minutes to downtime any server that was lost after a master failover (including failed master & lost replicas). 0 to disable - MasterFailoverDetachSlaveMasterHost bool // synonym to MasterFailoverDetachReplicaMasterHost - MasterFailoverDetachReplicaMasterHost bool // Should orchestrator issue a detach-replica-master-host on newly promoted master (this makes sure the new master will not attempt to replicate old master if that comes back to life). Defaults 'false'. Meaningless if ApplyMySQLPromotionAfterMasterFailover is 'true'. - FailMasterPromotionOnLagMinutes uint // when > 0, fail a master promotion if the candidate replica is lagging >= configured number of minutes. - FailMasterPromotionIfSQLThreadNotUpToDate bool // when true, and a master failover takes place, if candidate master has not consumed all relay logs, promotion is aborted with error - DelayMasterPromotionIfSQLThreadNotUpToDate bool // when true, and a master failover takes place, if candidate master has not consumed all relay logs, delay promotion until the sql thread has caught up - PostponeSlaveRecoveryOnLagMinutes uint // Synonym to PostponeReplicaRecoveryOnLagMinutes - PostponeReplicaRecoveryOnLagMinutes uint // On crash recovery, replicas that are lagging more than given minutes are only resurrected late in the recovery process, after master/IM has been elected and processes executed. Value of 0 disables this feature - OSCIgnoreHostnameFilters []string // OSC replicas recommendation will ignore replica hostnames matching given patterns - GraphiteAddr string // Optional; address of graphite port. If supplied, metrics will be written here - GraphitePath string // Prefix for graphite path. May include {hostname} magic placeholder - GraphiteConvertHostnameDotsToUnderscores bool // If true, then hostname's dots are converted to underscores before being used in graphite path - GraphitePollSeconds int // Graphite writes interval. 0 disables. - URLPrefix string // URL prefix to run orchestrator on non-root web path, e.g. /orchestrator to put it behind nginx. - DiscoveryIgnoreReplicaHostnameFilters []string // Regexp filters to apply to prevent auto-discovering new replicas. Usage: unreachable servers due to firewalls, applications which trigger binlog dumps - DiscoveryIgnoreMasterHostnameFilters []string // Regexp filters to apply to prevent auto-discovering a master. Usage: pointing your master temporarily to replicate some data from external host - DiscoveryIgnoreHostnameFilters []string // Regexp filters to apply to prevent discovering instances of any kind - DiscoveryIgnoreReplicationUsernameFilters []string // Regexp filters to apply to prevent discovering instances that use a matching replication username - EnableDiscoveryFiltersLogs bool // Should Orchestrator log the fact of filtered instance during discovery - ConsulAddress string // Address where Consul HTTP api is found. Example: 127.0.0.1:8500 - ConsulScheme string // Scheme (http or https) for Consul - ConsulAclToken string // ACL token used to write to Consul KV - ConsulCrossDataCenterDistribution bool // should orchestrator automatically auto-deduce all consul DCs and write KVs in all DCs - ConsulKVStoreProvider string // Consul KV store provider (consul or consul-txn), default: "consul" - ConsulMaxKVsPerTransaction int // Maximum number of KV operations to perform in a single Consul Transaction. Requires the "consul-txn" ConsulKVStoreProvider - ZkAddress string // UNSUPPERTED YET. Address where (single or multiple) ZooKeeper servers are found, in `srv1[:port1][,srv2[:port2]...]` format. Default port is 2181. Example: srv-a,srv-b:12181,srv-c - KVClusterMasterPrefix string // Prefix to use for clusters' masters entries in KV stores (internal, consul, ZK), default: "mysql/master" - WebMessage string // If provided, will be shown on all web pages below the title bar - MaxConcurrentReplicaOperations int // Maximum number of concurrent operations on replicas - EnforceExactSemiSyncReplicas bool // If true, semi-sync replicas will be enabled/disabled to match the wait count in the desired priority order; this applies to LockedSemiSyncMaster and MasterWithTooManySemiSyncReplicas - RecoverLockedSemiSyncMaster bool // If true, orchestrator will recover from a LockedSemiSync state by enabling semi-sync on replicas to match the wait count; this behavior can be overridden by EnforceExactSemiSyncReplicas - ReasonableLockedSemiSyncMasterSeconds uint // Time to evaluate the LockedSemiSyncHypothesis before triggering the LockedSemiSync analysis; falls back to ReasonableReplicationLagSeconds if not set - PrependMessagesWithOrcIdentity string // use FQDN/hostname/custom to prefix error message returned to the client. Empty string (default)/none skips prefixing. - CustomOrcIdentity string // use if PrependMessagesWithOrcIdentity is 'custom' - ProxySQLAdminAddress string // Address of ProxySQL Admin interface. Example: 127.0.0.1 - ProxySQLAdminPort int // Port of ProxySQL Admin interface. Default: 6032 - ProxySQLAdminUser string // Username for ProxySQL Admin. Default: admin - ProxySQLAdminPassword string // Password for ProxySQL Admin - ProxySQLAdminUseTLS bool // Use TLS for ProxySQL Admin connection - ProxySQLWriterHostgroup int // ProxySQL hostgroup ID for the writer (master). 0 means unconfigured. - ProxySQLReaderHostgroup int // ProxySQL hostgroup ID for readers (replicas). 0 means unconfigured. - ProxySQLPreFailoverAction string // Action on old master before failover: "offline_soft" (default), "weight_zero", "none" - PrometheusEnabled bool // When true (default), expose Prometheus metrics on /metrics endpoint + RecoverNonWriteableMaster bool // When 'true', orchestrator treats a read-only master as a failure scenario and attempts to make the master writeable + CoMasterRecoveryMustPromoteOtherCoMaster bool // When 'false', anything can get promoted (and candidates are preferred over others). When 'true', orchestrator will promote the other co-master or else fail + DetachLostSlavesAfterMasterFailover bool // synonym to DetachLostReplicasAfterMasterFailover + DetachLostReplicasAfterMasterFailover bool // Should replicas that are not to be lost in master recovery (i.e. were more up-to-date than promoted replica) be forcibly detached + ApplyMySQLPromotionAfterMasterFailover bool // Should orchestrator take upon itself to apply MySQL master promotion: set read_only=0, detach replication, etc. + PreventCrossDataCenterMasterFailover bool // When true (default: false), cross-DC master failover are not allowed, orchestrator will do all it can to only fail over within same DC, or else not fail over at all. + PreventCrossRegionMasterFailover bool // When true (default: false), cross-region master failover are not allowed, orchestrator will do all it can to only fail over within same region, or else not fail over at all. + MasterFailoverLostInstancesDowntimeMinutes uint // Number of minutes to downtime any server that was lost after a master failover (including failed master & lost replicas). 0 to disable + MasterFailoverDetachSlaveMasterHost bool // synonym to MasterFailoverDetachReplicaMasterHost + MasterFailoverDetachReplicaMasterHost bool // Should orchestrator issue a detach-replica-master-host on newly promoted master (this makes sure the new master will not attempt to replicate old master if that comes back to life). Defaults 'false'. Meaningless if ApplyMySQLPromotionAfterMasterFailover is 'true'. + FailMasterPromotionOnLagMinutes uint // when > 0, fail a master promotion if the candidate replica is lagging >= configured number of minutes. + FailMasterPromotionIfSQLThreadNotUpToDate bool // when true, and a master failover takes place, if candidate master has not consumed all relay logs, promotion is aborted with error + DelayMasterPromotionIfSQLThreadNotUpToDate bool // when true, and a master failover takes place, if candidate master has not consumed all relay logs, delay promotion until the sql thread has caught up + PostponeSlaveRecoveryOnLagMinutes uint // Synonym to PostponeReplicaRecoveryOnLagMinutes + PostponeReplicaRecoveryOnLagMinutes uint // On crash recovery, replicas that are lagging more than given minutes are only resurrected late in the recovery process, after master/IM has been elected and processes executed. Value of 0 disables this feature + OSCIgnoreHostnameFilters []string // OSC replicas recommendation will ignore replica hostnames matching given patterns + GraphiteAddr string // Optional; address of graphite port. If supplied, metrics will be written here + GraphitePath string // Prefix for graphite path. May include {hostname} magic placeholder + GraphiteConvertHostnameDotsToUnderscores bool // If true, then hostname's dots are converted to underscores before being used in graphite path + GraphitePollSeconds int // Graphite writes interval. 0 disables. + URLPrefix string // URL prefix to run orchestrator on non-root web path, e.g. /orchestrator to put it behind nginx. + DiscoveryIgnoreReplicaHostnameFilters []string // Regexp filters to apply to prevent auto-discovering new replicas. Usage: unreachable servers due to firewalls, applications which trigger binlog dumps + DiscoveryIgnoreMasterHostnameFilters []string // Regexp filters to apply to prevent auto-discovering a master. Usage: pointing your master temporarily to replicate some data from external host + DiscoveryIgnoreHostnameFilters []string // Regexp filters to apply to prevent discovering instances of any kind + DiscoveryIgnoreReplicationUsernameFilters []string // Regexp filters to apply to prevent discovering instances that use a matching replication username + EnableDiscoveryFiltersLogs bool // Should Orchestrator log the fact of filtered instance during discovery + ConsulAddress string // Address where Consul HTTP api is found. Example: 127.0.0.1:8500 + ConsulScheme string // Scheme (http or https) for Consul + ConsulAclToken string // ACL token used to write to Consul KV + ConsulCrossDataCenterDistribution bool // should orchestrator automatically auto-deduce all consul DCs and write KVs in all DCs + ConsulKVStoreProvider string // Consul KV store provider (consul or consul-txn), default: "consul" + ConsulMaxKVsPerTransaction int // Maximum number of KV operations to perform in a single Consul Transaction. Requires the "consul-txn" ConsulKVStoreProvider + ZkAddress string // UNSUPPERTED YET. Address where (single or multiple) ZooKeeper servers are found, in `srv1[:port1][,srv2[:port2]...]` format. Default port is 2181. Example: srv-a,srv-b:12181,srv-c + KVClusterMasterPrefix string // Prefix to use for clusters' masters entries in KV stores (internal, consul, ZK), default: "mysql/master" + WebMessage string // If provided, will be shown on all web pages below the title bar + MaxConcurrentReplicaOperations int // Maximum number of concurrent operations on replicas + EnforceExactSemiSyncReplicas bool // If true, semi-sync replicas will be enabled/disabled to match the wait count in the desired priority order; this applies to LockedSemiSyncMaster and MasterWithTooManySemiSyncReplicas + RecoverLockedSemiSyncMaster bool // If true, orchestrator will recover from a LockedSemiSync state by enabling semi-sync on replicas to match the wait count; this behavior can be overridden by EnforceExactSemiSyncReplicas + ReasonableLockedSemiSyncMasterSeconds uint // Time to evaluate the LockedSemiSyncHypothesis before triggering the LockedSemiSync analysis; falls back to ReasonableReplicationLagSeconds if not set + PrependMessagesWithOrcIdentity string // use FQDN/hostname/custom to prefix error message returned to the client. Empty string (default)/none skips prefixing. + CustomOrcIdentity string // use if PrependMessagesWithOrcIdentity is 'custom' + ProxySQLAdminAddress string // Address of ProxySQL Admin interface. Example: 127.0.0.1 + ProxySQLAdminPort int // Port of ProxySQL Admin interface. Default: 6032 + ProxySQLAdminUser string // Username for ProxySQL Admin. Default: admin + ProxySQLAdminPassword string // Password for ProxySQL Admin + ProxySQLAdminUseTLS bool // Use TLS for ProxySQL Admin connection + ProxySQLWriterHostgroup int // ProxySQL hostgroup ID for the writer (master). 0 means unconfigured. + ProxySQLReaderHostgroup int // ProxySQL hostgroup ID for readers (replicas). 0 means unconfigured. + ProxySQLPreFailoverAction string // Action on old master before failover: "offline_soft" (default), "weight_zero", "none" + PrometheusEnabled bool // When true (default), expose Prometheus metrics on /metrics endpoint } // ToJSONString will marshal this configuration as JSON diff --git a/go/logic/topology_recovery.go b/go/logic/topology_recovery.go index a2ebb26e2..e912a5c2d 100644 --- a/go/logic/topology_recovery.go +++ b/go/logic/topology_recovery.go @@ -428,7 +428,7 @@ func markRecoveryUnsuccessfulDueToHooks(topologyRecovery *TopologyRecovery, hook } topologyRecovery.IsSuccessful = false _ = topologyRecovery.AddError(fmt.Errorf("post-failover hooks failed: %w", hookErr)) - AuditTopologyRecovery(topologyRecovery, fmt.Sprintf("Marking recovery unsuccessful due to post-failover hook failure: %+v", hookErr)) + _ = AuditTopologyRecovery(topologyRecovery, fmt.Sprintf("Marking recovery unsuccessful due to post-failover hook failure: %+v", hookErr)) } // persistResolvedRecovery writes the current IsSuccessful/successor fields for a recovery. @@ -458,7 +458,7 @@ func runPostSuccessFailoverProcesses(processes []string, description string, top return nil } if !failOnError { - AuditTopologyRecovery(topologyRecovery, fmt.Sprintf("%s completed with errors (ignored for recovery success): %+v", description, err)) + _ = AuditTopologyRecovery(topologyRecovery, fmt.Sprintf("%s completed with errors (ignored for recovery success): %+v", description, err)) return nil } markRecoveryUnsuccessfulDueToHooks(topologyRecovery, err)