From eca0b9f33c3d75fc80168bfcb885b00eaecc22c0 Mon Sep 17 00:00:00 2001 From: Jakub Pliszka Date: Mon, 7 Sep 2026 13:02:11 +0100 Subject: [PATCH] Add fallback cluster config option --- doc/http.md | 4 +- doc/mysql.md | 4 +- pkg/config/config_test.go | 27 +++++++ pkg/config/mysql_config.go | 9 ++- pkg/http/api.go | 16 ++-- pkg/throttle/api_fallback_test.go | 45 +++++++++++ pkg/throttle/check.go | 38 ++++++--- pkg/throttle/throttler_test.go | 126 ++++++++++++++++++++++++++++++ 8 files changed, 246 insertions(+), 23 deletions(-) create mode 100644 pkg/throttle/api_fallback_test.go diff --git a/doc/http.md b/doc/http.md index d060ca0..b2e06ac 100644 --- a/doc/http.md +++ b/doc/http.md @@ -98,9 +98,9 @@ Notes: However this check is known to be useful, at least in one common scenario: a monitoring of a MySQL cluster based on replication lag. In such case, we may have write requests followed by read requests. We may happen to know the elapsed time between write & read. As an example, say `2.5s` have passed between the write and read. The check `/check-read/archive/mysql/main1/2.5` confirms or denies that relevant replicas are up-to-date for the `2.5s` elapsed time. We can therefore read from the replicas and safely expect to find the data we wrote `2.5s` ago on the master. -- `/check-if-exists///`: like `/check`, but if the metric is unknown (e.g. `` not in `freno`'s configuration), return `200 OK`. This is useful for hybrid systems where some metrics need to be strictly controlled, and some not. `freno` would probe the important stores, and still can serve requests for all stores. +- `/check-if-exists///`: like `/check`, but if the metric is unknown (e.g. `` not in `freno`'s configuration), return `200 OK`. Unknown names do not use MySQL's configured `FallbackCluster`. This is useful for hybrid systems where some metrics need to be strictly controlled, and some not. `freno` would probe the important stores, and still can serve requests for all stores. -- `/check-read-if-exists////`: like `/check-read`, but if the metric is unknown (e.g. `` not in `freno`'s configuration), return `200 OK`. This is useful for hybrid systems where some metrics need to be strictly controlled, and some not. `freno` would probe the important stores, and still can serve requests for all stores. +- `/check-read-if-exists////`: like `/check-read`, but if the metric is unknown (e.g. `` not in `freno`'s configuration), return `200 OK`. Unknown names do not use MySQL's configured `FallbackCluster`. This is useful for hybrid systems where some metrics need to be strictly controlled, and some not. `freno` would probe the important stores, and still can serve requests for all stores. - `/skip-host//ttl/`: skip host when aggregating metrics for specified number of minutes. If host is already skipped, update the TTL. - `/skip-host/`: same as `/skip-host//ttl/60` diff --git a/doc/mysql.md b/doc/mysql.md index 0b651ce..9815050 100644 --- a/doc/mysql.md +++ b/doc/mysql.md @@ -45,6 +45,7 @@ You will find the top-level configuration: "us-east-1", "us-east-2" ], + "FallbackCluster": "", "Clusters": { } } @@ -75,6 +76,7 @@ These params apply in general to all MySQL clusters, unless specified differentl You may override `HttpCheckPath` on specific clusters. - `IgnoreHosts`: array of substrings. A host is completely ignored by `freno` if it contains a substring listed in `IgnoreHosts`. Like other values, this value can be overridden per-cluster. A non-empty `IgnoreHosts` in a specific cluster will replace the `MySQL` scope definition, for that cluster. An empty `IgnoreHosts` in a cluster scope will not un-ignore the patterns specified in `MySQL` scope. If you want to un-ignore the `MySQL` scope use some thing like `"IgnoreHosts": ["--no-such-pattern--"],`, known to never match any of your hosts. +- `FallbackCluster`: optional configured cluster used by `/check` and `/check-read` when the requested MySQL cluster name is not configured. The value must exactly match a key in `Clusters`. Exact configured names always take precedence, including when their metric or threshold is unavailable. Looking at clusters configuration: @@ -112,7 +114,7 @@ Looking at clusters configuration: } ``` -This introduces two clusters: `prod4` and `local`. `freno` will only serve requests for these two clusters. Any other request (e.g. `/check/archive/mysql/prod7`) will be answered with `HTTP 500` -- an unknown metric +This introduces the `prod4`, `sharded`, and `local` clusters. Without `FallbackCluster`, `freno` only serves requests for configured clusters; any other request (e.g. `/check/archive/mysql/prod7`) is answered with `HTTP 404`. With `"FallbackCluster": "prod4"`, ordinary `/check` and `/check-read` requests for unknown MySQL cluster names use `prod4`'s metric, threshold, and throttling state. Noteworthy: diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index 0fea668..21216e0 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -85,6 +85,33 @@ func TestReload(t *testing.T) { } } +func TestMySQLFallbackCluster(t *testing.T) { + tests := []struct { + name string + fallbackCluster string + clusters map[string]*MySQLClusterConfigurationSettings + wantErr string + }{ + {name: "omitted"}, + {name: "empty", clusters: map[string]*MySQLClusterConfigurationSettings{"primary": {}}}, + {name: "configured", fallbackCluster: "primary", clusters: map[string]*MySQLClusterConfigurationSettings{"primary": {}}}, + {name: "missing", fallbackCluster: "missing", clusters: map[string]*MySQLClusterConfigurationSettings{"primary": {}}, wantErr: `Stores.MySQL.FallbackCluster "missing" does not name a configured cluster`}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + settings := MySQLConfigurationSettings{FallbackCluster: test.fallbackCluster, Clusters: test.clusters} + err := settings.postReadAdjustments() + if test.wantErr == "" && err != nil { + t.Fatalf("unexpected error: %v", err) + } + if test.wantErr != "" && (err == nil || err.Error() != test.wantErr) { + t.Fatalf("error = %v, want %q", err, test.wantErr) + } + }) + } +} + func dump(path string, contents *ConfigurationSettings) error { json, _ := json.Marshal(contents) err := ioutil.WriteFile(path, json, 0644) diff --git a/pkg/config/mysql_config.go b/pkg/config/mysql_config.go index ed1e968..8e5a7bf 100644 --- a/pkg/config/mysql_config.go +++ b/pkg/config/mysql_config.go @@ -5,6 +5,7 @@ package config // import ( + "fmt" "os" ) @@ -65,7 +66,8 @@ type MySQLConfigurationSettings struct { VitessCells []string // Name of the Vitess cells for polling tablet hosts Collation string // MySQL collation to use for stores, replaces charset if specified - Clusters map[string](*MySQLClusterConfigurationSettings) // cluster name -> cluster config + FallbackCluster string + Clusters map[string](*MySQLClusterConfigurationSettings) // cluster name -> cluster config } // Hook to implement adjustments after reading each configuration file. @@ -73,6 +75,11 @@ func (settings *MySQLConfigurationSettings) postReadAdjustments() error { if settings.Port == 0 { settings.Port = DefaultMySQLPort } + if settings.FallbackCluster != "" { + if _, ok := settings.Clusters[settings.FallbackCluster]; !ok { + return fmt.Errorf("Stores.MySQL.FallbackCluster %q does not name a configured cluster", settings.FallbackCluster) + } + } // Username & password may be given as plaintext in the config file, or can be delivered // via environment variables. We accept user & password in the form "${SOME_ENV_VARIABLE}" // in which case we get the value from this process' invoking environment. diff --git a/pkg/http/api.go b/pkg/http/api.go index 2b163a7..d0bd34d 100644 --- a/pkg/http/api.go +++ b/pkg/http/api.go @@ -49,7 +49,6 @@ type API interface { var endpoints = []string{} // known API URIs -var okIfNotExistsFlags = &throttle.CheckFlags{OKIfNotExists: true} var metricsHandler = exp.ExpHandler(metrics.DefaultRegistry) type GeneralResponse struct { @@ -167,11 +166,14 @@ func (api *APIImpl) check(w http.ResponseWriter, r *http.Request, ps httprouter. remoteAddr = r.RemoteAddr remoteAddr = strings.Split(remoteAddr, ":")[0] } - flags.LowPriority = (r.URL.Query().Get("p") == "low") - - checkResult := api.throttlerCheck.Check(appName, storeType, storeName, remoteAddr, flags) - if checkResult.StatusCode == http.StatusNotFound && flags.OKIfNotExists { - checkResult.StatusCode = http.StatusOK // 200 + requestFlags := *flags + requestFlags.LowPriority = (r.URL.Query().Get("p") == "low") + + checkResult := api.throttlerCheck.Check(appName, storeType, storeName, remoteAddr, &requestFlags) + if checkResult.StatusCode == http.StatusNotFound && requestFlags.OKIfNotExists { + result := *checkResult + result.StatusCode = http.StatusOK // 200 + checkResult = &result } api.respondToCheckRequest(w, r, checkResult) @@ -185,7 +187,7 @@ func (api *APIImpl) WriteCheck(w http.ResponseWriter, r *http.Request, ps httpro // WriteCheckIfExists checks for a metric, but reports an OK if the metric does not exist. // If the metric does exist, then all usual checks are made. func (api *APIImpl) WriteCheckIfExists(w http.ResponseWriter, r *http.Request, ps httprouter.Params) { - api.check(w, r, ps, okIfNotExistsFlags) + api.check(w, r, ps, &throttle.CheckFlags{OKIfNotExists: true}) } func (api *APIImpl) readCheck(w http.ResponseWriter, r *http.Request, ps httprouter.Params, flags *throttle.CheckFlags) { diff --git a/pkg/throttle/api_fallback_test.go b/pkg/throttle/api_fallback_test.go new file mode 100644 index 0000000..4175eb0 --- /dev/null +++ b/pkg/throttle/api_fallback_test.go @@ -0,0 +1,45 @@ +package throttle_test + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/github/freno/pkg/config" + frenohttp "github.com/github/freno/pkg/http" + "github.com/github/freno/pkg/throttle" +) + +func TestFallbackHTTPCheckEndpoints(t *testing.T) { + defer config.Reset() + config.Settings().Stores.MySQL = config.MySQLConfigurationSettings{ + FallbackCluster: "fallback", + Clusters: map[string]*config.MySQLClusterConfigurationSettings{ + "fallback": {}, + }, + } + + throttler := throttle.NewThrottler() + throttle.SetTestMySQLClusterMetric(throttler, "fallback", 10.0, 5.0) + router := frenohttp.ConfigureRoutes(frenohttp.NewAPIImpl(throttle.NewThrottlerCheck(throttler), nil)) + + tests := []struct { + path string + want int + }{ + {path: "/check/test-app/mysql/unknown", want: http.StatusTooManyRequests}, + {path: "/check-read/test-app/mysql/unknown/20", want: http.StatusOK}, + {path: "/check-if-exists/test-app/mysql/unknown", want: http.StatusOK}, + {path: "/check-read-if-exists/test-app/mysql/unknown/5", want: http.StatusOK}, + } + + for _, test := range tests { + t.Run(test.path, func(t *testing.T) { + recorder := httptest.NewRecorder() + router.ServeHTTP(recorder, httptest.NewRequest(http.MethodGet, test.path, nil)) + if recorder.Code != test.want { + t.Fatalf("status = %d, want %d; body: %s", recorder.Code, test.want, recorder.Body.String()) + } + }) + } +} diff --git a/pkg/throttle/check.go b/pkg/throttle/check.go index 069845e..0c1cc7b 100644 --- a/pkg/throttle/check.go +++ b/pkg/throttle/check.go @@ -1,15 +1,14 @@ package throttle import ( + "errors" + "fmt" "net/http" "strings" "time" - "fmt" - - "errors" - "github.com/github/freno/pkg/base" + "github.com/github/freno/pkg/config" metrics "github.com/rcrowley/go-metrics" ) @@ -101,12 +100,27 @@ func (check *ThrottlerCheck) checkAppMetricResult(appName string, storeType stri // CheckAppStoreMetric func (check *ThrottlerCheck) Check(appName string, storeType string, storeName string, remoteAddr string, flags *CheckFlags) (checkResult *CheckResult) { + requestedStoreName := storeName var metricResultFunc base.MetricResultFunc switch storeType { case "mysql": { - metricResultFunc = func() (metricResult base.MetricResult, threshold float64) { - return check.throttler.getMySQLClusterMetrics(storeName) + mysqlSettings := config.Settings().Stores.MySQL + _, configured := mysqlSettings.Clusters[storeName] + if !configured && !flags.OKIfNotExists { + if fallbackCluster := mysqlSettings.FallbackCluster; fallbackCluster != "" { + storeName = fallbackCluster + _, configured = mysqlSettings.Clusters[storeName] + } + } + if configured { + metricResultFunc = func() (metricResult base.MetricResult, threshold float64) { + return check.throttler.getMySQLClusterMetrics(storeName) + } + } else { + metricResultFunc = func() (metricResult base.MetricResult, threshold float64) { + return base.NoSuchMetric, 0 + } } } } @@ -120,22 +134,22 @@ func (check *ThrottlerCheck) Check(appName string, storeType string, storeName s metrics.GetOrRegisterCounter("check.any.total", nil).Inc(1) metrics.GetOrRegisterCounter(fmt.Sprintf("check.%s.total", appName), nil).Inc(1) - metrics.GetOrRegisterCounter(fmt.Sprintf("check.any.%s.%s.total", storeType, storeName), nil).Inc(1) - metrics.GetOrRegisterCounter(fmt.Sprintf("check.%s.%s.%s.total", appName, storeType, storeName), nil).Inc(1) + metrics.GetOrRegisterCounter(fmt.Sprintf("check.any.%s.%s.total", storeType, requestedStoreName), nil).Inc(1) + metrics.GetOrRegisterCounter(fmt.Sprintf("check.%s.%s.%s.total", appName, storeType, requestedStoreName), nil).Inc(1) if statusCode != http.StatusOK { metrics.GetOrRegisterCounter("check.any.error", nil).Inc(1) metrics.GetOrRegisterCounter(fmt.Sprintf("check.%s.error", appName), nil).Inc(1) - metrics.GetOrRegisterCounter(fmt.Sprintf("check.any.%s.%s.error", storeType, storeName), nil).Inc(1) - metrics.GetOrRegisterCounter(fmt.Sprintf("check.%s.%s.%s.error", appName, storeType, storeName), nil).Inc(1) + metrics.GetOrRegisterCounter(fmt.Sprintf("check.any.%s.%s.error", storeType, requestedStoreName), nil).Inc(1) + metrics.GetOrRegisterCounter(fmt.Sprintf("check.%s.%s.%s.error", appName, storeType, requestedStoreName), nil).Inc(1) if statusCode == http.StatusInternalServerError { metrics.GetOrRegisterCounter("check.any.internal-error", nil).Inc(1) metrics.GetOrRegisterCounter(fmt.Sprintf("check.%s.internal-error", appName), nil).Inc(1) - metrics.GetOrRegisterCounter(fmt.Sprintf("check.any.%s.%s.internal-error", storeType, storeName), nil).Inc(1) - metrics.GetOrRegisterCounter(fmt.Sprintf("check.%s.%s.%s.internal-error", appName, storeType, storeName), nil).Inc(1) + metrics.GetOrRegisterCounter(fmt.Sprintf("check.any.%s.%s.internal-error", storeType, requestedStoreName), nil).Inc(1) + metrics.GetOrRegisterCounter(fmt.Sprintf("check.%s.%s.%s.internal-error", appName, storeType, requestedStoreName), nil).Inc(1) } } diff --git a/pkg/throttle/throttler_test.go b/pkg/throttle/throttler_test.go index 479fc7c..49826c8 100644 --- a/pkg/throttle/throttler_test.go +++ b/pkg/throttle/throttler_test.go @@ -3,8 +3,10 @@ package throttle import ( "net/http" "testing" + "time" "github.com/github/freno/pkg/base" + "github.com/github/freno/pkg/config" "github.com/stretchr/testify/assert" ) @@ -53,3 +55,127 @@ func Test_checkAppMetricResult(t *testing.T) { }) } + +func setTestMySQLClusterMetric(throttler *Throttler, clusterName string, value, threshold float64) { + throttler.mysqlClusterThresholds.SetDefault(clusterName, threshold) + throttler.aggregatedMetrics.SetDefault("mysql/"+clusterName, base.NewSimpleMetricResult(value)) +} + +func SetTestMySQLClusterMetric(throttler *Throttler, clusterName string, value, threshold float64) { + setTestMySQLClusterMetric(throttler, clusterName, value, threshold) +} + +func newTestMySQLFallbackCheck(fallbackCluster string) (*ThrottlerCheck, *Throttler) { + config.Settings().Stores.MySQL = config.MySQLConfigurationSettings{ + FallbackCluster: fallbackCluster, + Clusters: map[string]*config.MySQLClusterConfigurationSettings{ + "fallback": {}, + "exact": {}, + }, + } + + throttler := NewThrottler() + setTestMySQLClusterMetric(throttler, "fallback", 1.0, 10.0) + return NewThrottlerCheck(throttler), throttler +} + +func TestCheckMySQLFallback(t *testing.T) { + originalSettings := config.Settings().Stores.MySQL + defer func() { + config.Settings().Stores.MySQL = originalSettings + }() + + t.Run("exact name wins", func(t *testing.T) { + check, throttler := newTestMySQLFallbackCheck("fallback") + setTestMySQLClusterMetric(throttler, "exact", 2.0, 20.0) + result := check.Check("test-app", "mysql", "exact", "", &CheckFlags{}) + assert.Equal(t, http.StatusOK, result.StatusCode) + assert.Equal(t, 2.0, result.Value) + assert.Equal(t, 20.0, result.Threshold) + }) + + t.Run("unknown name falls back", func(t *testing.T) { + check, _ := newTestMySQLFallbackCheck("fallback") + result := check.Check("test-app", "mysql", "unknown", "", &CheckFlags{}) + assert.Equal(t, http.StatusOK, result.StatusCode) + assert.Equal(t, 1.0, result.Value) + assert.Equal(t, 10.0, result.Threshold) + }) + + t.Run("exact name with missing runtime metric does not fall back", func(t *testing.T) { + check, throttler := newTestMySQLFallbackCheck("fallback") + throttler.mysqlClusterThresholds.SetDefault("exact", 20.0) + result := check.Check("test-app", "mysql", "exact", "", &CheckFlags{}) + assert.Equal(t, http.StatusNotFound, result.StatusCode) + assert.Equal(t, 20.0, result.Threshold) + }) + + t.Run("strict metric lookup does not fall back", func(t *testing.T) { + _, throttler := newTestMySQLFallbackCheck("fallback") + metric, threshold := throttler.getMySQLClusterMetrics("unknown") + _, err := metric.Get() + assert.Equal(t, base.NoSuchMetricError, err) + assert.Equal(t, 0.0, threshold) + }) + + t.Run("no fallback remains not found", func(t *testing.T) { + check, throttler := newTestMySQLFallbackCheck("") + setTestMySQLClusterMetric(throttler, "unknown", 1.0, 10.0) + result := check.Check("test-app", "mysql", "unknown", "", &CheckFlags{}) + assert.Equal(t, http.StatusNotFound, result.StatusCode) + }) + + t.Run("unsupported store remains not found", func(t *testing.T) { + check, _ := newTestMySQLFallbackCheck("fallback") + result := check.Check("test-app", "redis", "unknown", "", &CheckFlags{}) + assert.Equal(t, http.StatusNotFound, result.StatusCode) + }) +} + +func TestCheckMySQLFallbackUsesCanonicalState(t *testing.T) { + originalSettings := config.Settings().Stores.MySQL + defer func() { + config.Settings().Stores.MySQL = originalSettings + }() + + tests := []struct { + name string + flags *CheckFlags + prepare func(*Throttler) + want int + }{ + { + name: "per-store app throttle", + flags: &CheckFlags{}, + prepare: func(throttler *Throttler) { + throttler.ThrottleApp("test-app/fallback", time.Time{}, 1) + }, + want: http.StatusExpectationFailed, + }, + { + name: "low priority state", + flags: &CheckFlags{LowPriority: true}, + prepare: func(throttler *Throttler) { + throttler.nonLowPriorityAppRequestsThrottled.SetDefault("mysql/fallback", true) + }, + want: http.StatusExpectationFailed, + }, + { + name: "share domain health", + flags: &CheckFlags{}, + prepare: func(throttler *Throttler) { + throttler.shareDomainMetricHealth.SetDefault("mysql/fallback", &base.MetricHealth{SecondsSinceLastHealthy: 1}) + }, + want: http.StatusTooManyRequests, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + check, throttler := newTestMySQLFallbackCheck("fallback") + test.prepare(throttler) + result := check.Check("test-app", "mysql", "unknown", "", test.flags) + assert.Equal(t, test.want, result.StatusCode) + }) + } +}