From b9829c15e1973a5cbd8a02161b6652ce4809ca65 Mon Sep 17 00:00:00 2001 From: Ante Gulin Date: Fri, 15 May 2026 13:46:55 +0200 Subject: [PATCH 1/7] PMM-14858 Skip built-in ClickHouse scrape for external instances --- managed/cmd/pmm-managed/main.go | 21 ++-- managed/models/clickhouse_params.go | 74 +++++++++++++ managed/models/clickhouse_params_test.go | 101 ++++++++++++++++++ .../services/victoriametrics/prometheus.go | 14 +++ .../victoriametrics/victoriametrics.go | 9 +- .../victoriametrics/victoriametrics_test.go | 60 ++++++++++- 6 files changed, 268 insertions(+), 11 deletions(-) create mode 100644 managed/models/clickhouse_params.go create mode 100644 managed/models/clickhouse_params_test.go diff --git a/managed/cmd/pmm-managed/main.go b/managed/cmd/pmm-managed/main.go index 9854238af15..acec0bd4582 100644 --- a/managed/cmd/pmm-managed/main.go +++ b/managed/cmd/pmm-managed/main.go @@ -726,6 +726,7 @@ func main() { //nolint:gocognit,maintidx,cyclop clickhouseAddrF := kingpin.Flag("clickhouse-addr", "Clickhouse database address").Default("127.0.0.1:9000").Envar("PMM_CLICKHOUSE_ADDR").String() clickhouseUsernameF := kingpin.Flag("clickhouse-username", "Clickhouse database user").Default("default").Envar("PMM_CLICKHOUSE_USER").String() clickhousePasswordF := kingpin.Flag("clickhouse-password", "Clickhouse database user password").Default("clickhouse").Envar("PMM_CLICKHOUSE_PASSWORD").String() + clickhouseBuiltinDisabledF := kingpin.Flag("clickhouse-disable-builtin", "Disable the built-in ClickHouse").Envar("PMM_DISABLE_BUILTIN_CLICKHOUSE").Bool() watchtowerHostF := kingpin.Flag("watchtower-host", "Watchtower host").Default("http://watchtower:8080").Envar("PMM_WATCHTOWER_HOST").URL() @@ -821,15 +822,19 @@ func main() { //nolint:gocognit,maintidx,cyclop grafanadb.DSN.DB = "grafana" grafanadb.DSN.Params = q.Encode() - chURI := url.URL{ - Scheme: "tcp", - User: url.UserPassword(*clickhouseUsernameF, *clickhousePasswordF), - Host: *clickhouseAddrF, - Path: *clickHouseDatabaseF, + chParams, err := models.NewClickHouseParams( + *clickhouseAddrF, + *clickHouseDatabaseF, + *clickhouseUsernameF, + *clickhousePasswordF, + *clickhouseBuiltinDisabledF, + ) + if err != nil { + l.Panicf("cannot load clickhouse params: %+v", err) } qanDB := ds.QanDBSelect - qanDB.DSN = chURI.String() + qanDB.DSN = chParams.URL().String() ds.VM.Address = *victoriaMetricsURLF @@ -879,7 +884,7 @@ func main() { //nolint:gocognit,maintidx,cyclop cleaner := clean.New(db) externalRules := vmalert.NewExternalRules() - vmdb, err := victoriametrics.NewVictoriaMetrics(*victoriaMetricsConfigF, db, vmParams, haService) + vmdb, err := victoriametrics.NewVictoriaMetrics(*victoriaMetricsConfigF, db, vmParams, chParams, haService) if err != nil { l.Panicf("VictoriaMetrics service problem: %+v", err) } @@ -1019,7 +1024,7 @@ func main() { //nolint:gocognit,maintidx,cyclop versionCache := versioncache.New(db, versioner) dumpService := dump.New(db, &dump.URLs{ - ClickhouseURL: chURI.String(), + ClickhouseURL: chParams.URL().String(), VMURL: *victoriaMetricsURLF, }) diff --git a/managed/models/clickhouse_params.go b/managed/models/clickhouse_params.go new file mode 100644 index 00000000000..dcb17139121 --- /dev/null +++ b/managed/models/clickhouse_params.go @@ -0,0 +1,74 @@ +// Copyright (C) 2023 Percona LLC +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . + +package models + +import ( + "fmt" + "net" + "net/url" + "strconv" +) + +// ClickHouseParams represents ClickHouse server params. +type ClickHouseParams struct { + BuiltinDisabled bool + url *url.URL +} + +// ExternalClickHouse returns true if ClickHouse is configured externally. +func (p *ClickHouseParams) ExternalClickHouse() bool { + return p.url.Hostname() != "127.0.0.1" +} + +// URL returns the ClickHouse DSN. +func (p *ClickHouseParams) URL() *url.URL { + u := *p.url + return &u +} + +// NewClickHouseParams returns validated ClickHouse configuration params, +// or an error if any required field is missing or malformed. +func NewClickHouseParams(addr, dbName, dbUsername, dbPassword string, builtinDisabled bool) (*ClickHouseParams, error) { + if addr == "" { + return nil, fmt.Errorf("addr is required") + } + host, port, err := net.SplitHostPort(addr) + if err != nil { + return nil, fmt.Errorf("invalid addr %q: %w", addr, err) + } + if host == "" { + return nil, fmt.Errorf("invalid addr %q: empty host", addr) + } + if _, err := strconv.ParseUint(port, 10, 16); err != nil { + return nil, fmt.Errorf("invalid port in addr %q: %w", addr, err) + } + if dbName == "" { + return nil, fmt.Errorf("database name is required") + } + if dbUsername == "" { + return nil, fmt.Errorf("username is required") + } + + return &ClickHouseParams{ + BuiltinDisabled: builtinDisabled, + url: &url.URL{ + Scheme: "tcp", + User: url.UserPassword(dbUsername, dbPassword), + Host: addr, + Path: dbName, + }, + }, nil +} diff --git a/managed/models/clickhouse_params_test.go b/managed/models/clickhouse_params_test.go new file mode 100644 index 00000000000..cabbc4ee710 --- /dev/null +++ b/managed/models/clickhouse_params_test.go @@ -0,0 +1,101 @@ +// Copyright (C) 2023 Percona LLC +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . + +package models + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestNewClickHouseParams(t *testing.T) { + t.Run("valid", func(t *testing.T) { + p, err := NewClickHouseParams("127.0.0.1:9000", "pmm", "default", "clickhouse", false) + require.NoError(t, err) + assert.Equal(t, "tcp://default:clickhouse@127.0.0.1:9000/pmm", p.URL().String()) + assert.False(t, p.BuiltinDisabled) + }) + + t.Run("valid builtin disabled", func(t *testing.T) { + p, err := NewClickHouseParams("127.0.0.1:9000", "pmm", "default", "clickhouse", true) + require.NoError(t, err) + assert.True(t, p.BuiltinDisabled) + }) + + t.Run("valid empty password", func(t *testing.T) { + _, err := NewClickHouseParams("127.0.0.1:9000", "pmm", "default", "", false) + require.NoError(t, err) + }) + + errCases := []struct { + name string + addr string + dbName string + dbUsername string + dbPassword string + wantErrSub string + }{ + {"empty addr", "", "pmm", "default", "clickhouse", "addr is required"}, + {"missing port", "127.0.0.1", "pmm", "default", "clickhouse", "invalid addr"}, + {"empty host", ":9000", "pmm", "default", "clickhouse", "empty host"}, + {"non numeric port", "localhost:abc", "pmm", "default", "clickhouse", "invalid port"}, + {"port out of range", "localhost:99999", "pmm", "default", "clickhouse", "invalid port"}, + {"empty db name", "127.0.0.1:9000", "", "default", "clickhouse", "database name is required"}, + {"empty username", "127.0.0.1:9000", "pmm", "", "clickhouse", "username is required"}, + } + for _, tc := range errCases { + t.Run(tc.name, func(t *testing.T) { + _, err := NewClickHouseParams(tc.addr, tc.dbName, tc.dbUsername, tc.dbPassword, false) + require.Error(t, err) + assert.ErrorContains(t, err, tc.wantErrSub) + }) + } +} + +func TestCHParamsExternalClickHouse(t *testing.T) { + cases := []struct { + name string + addr string + want bool + }{ + {"loopback", "127.0.0.1:9000", false}, + {"localhost", "localhost:9000", true}, + {"external host", "ch-01.test.net:9000", true}, + {"wildcard", "0.0.0.0:9000", true}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + p, err := NewClickHouseParams(tc.addr, "pmm", "default", "clickhouse", false) + require.NoError(t, err) + assert.Equal(t, tc.want, p.ExternalClickHouse()) + }) + } +} + +func TestCHParamsURL(t *testing.T) { + t.Run("simple", func(t *testing.T) { + p, err := NewClickHouseParams("127.0.0.1:9000", "pmm", "default", "clickhouse", false) + require.NoError(t, err) + assert.Equal(t, "tcp://default:clickhouse@127.0.0.1:9000/pmm", p.URL().String()) + }) + + t.Run("password with special chars", func(t *testing.T) { + p, err := NewClickHouseParams("127.0.0.1:9000", "pmm", "default", "p@ss/word", false) + require.NoError(t, err) + assert.Equal(t, "tcp://default:p%40ss%2Fword@127.0.0.1:9000/pmm", p.URL().String()) + }) +} diff --git a/managed/services/victoriametrics/prometheus.go b/managed/services/victoriametrics/prometheus.go index 80266458812..7388cbbdaf9 100644 --- a/managed/services/victoriametrics/prometheus.go +++ b/managed/services/victoriametrics/prometheus.go @@ -273,6 +273,20 @@ func addInternalServicesToScrape(s models.MetricsResolutions, svc *Service, pmmS } if svc.params.ExternalVM() { + svc.l.Infof("Skip scrape config for ClickHouse, VictoriaMetrics is configured to run externally.") + return cfg + } + + if svc.chParams.BuiltinDisabled { + svc.l.Infof("Skip scrape config for ClickHouse, scraping has been disabled for builtin clickhouse.") + return cfg + } + + if svc.chParams.ExternalClickHouse() { + svc.l.Warnf( + "External ClickHouse detected (%s); skipping built-in ClickHouse scrape (127.0.0.1:9363). Set PMM_DISABLE_BUILTIN_CLICKHOUSE=true to make this explicit.", + svc.chParams.URL().Redacted(), + ) return cfg } diff --git a/managed/services/victoriametrics/victoriametrics.go b/managed/services/victoriametrics/victoriametrics.go index ce4277027d9..d13343509d2 100644 --- a/managed/services/victoriametrics/victoriametrics.go +++ b/managed/services/victoriametrics/victoriametrics.go @@ -60,7 +60,8 @@ type Service struct { baseURL *url.URL client *http.Client - params *models.VictoriaMetricsParams + params *models.VictoriaMetricsParams + chParams *models.ClickHouseParams l *logrus.Entry reloadCh chan struct{} @@ -68,11 +69,14 @@ type Service struct { } // NewVictoriaMetrics creates new VictoriaMetrics service. -func NewVictoriaMetrics(scrapeConfigPath string, db *reform.DB, params *models.VictoriaMetricsParams, haService haService) (*Service, error) { +func NewVictoriaMetrics(scrapeConfigPath string, db *reform.DB, params *models.VictoriaMetricsParams, chParams *models.ClickHouseParams, haService haService) (*Service, error) { u, err := url.Parse(params.URL()) if err != nil { return nil, err } + if chParams == nil { + return nil, fmt.Errorf("ClickHouseParams is required") + } return &Service{ scrapeConfigPath: scrapeConfigPath, @@ -80,6 +84,7 @@ func NewVictoriaMetrics(scrapeConfigPath string, db *reform.DB, params *models.V baseURL: u, client: &http.Client{}, // TODO instrument with utils/irt; see vmalert package https://jira.percona.com/browse/PMM-7229 params: params, + chParams: chParams, l: logrus.WithField("component", "victoriametrics"), reloadCh: make(chan struct{}, 1), haService: haService, diff --git a/managed/services/victoriametrics/victoriametrics_test.go b/managed/services/victoriametrics/victoriametrics_test.go index f21ccdf75fc..29cd38995f2 100644 --- a/managed/services/victoriametrics/victoriametrics_test.go +++ b/managed/services/victoriametrics/victoriametrics_test.go @@ -45,10 +45,13 @@ func setup(t *testing.T) (*reform.DB, *Service, []byte) { vmParams, err := models.NewVictoriaMetricsParams(models.BasePrometheusConfigPath, models.VMBaseURL) check.NoError(err) + chParams, err := models.NewClickHouseParams("127.0.0.1:9000", "pmm", "default", "clickhouse", false) + check.NoError(err) + mockHaService := newMockHaService(t) mockHaService.On("Params").Return(&models.HAParams{Enabled: false, NodeID: "pmm-ha-service-0"}).Maybe() mockHaService.On("IsLeader").Return(true).Maybe() - svc, err := NewVictoriaMetrics(configPath, db, vmParams, mockHaService) + svc, err := NewVictoriaMetrics(configPath, db, vmParams, chParams, mockHaService) check.NoError(err) original, err := os.ReadFile(configPath) @@ -991,3 +994,58 @@ scrape_configs: assert.NoError(t, err) assert.Equal(t, expected, string(newcfg), "actual:\n%s", newcfg) } + +func TestVMConfig_OmitsClickhouseScrape(t *testing.T) { + newSvc := func(t *testing.T, chParams *models.ClickHouseParams, vmURL string) (*reform.DB, *Service) { + t.Helper() + sqlDB := testdb.Open(t, models.SkipFixtures, nil) + db := reform.NewDB(sqlDB, postgresql.Dialect, reform.NewPrintfLogger(t.Logf)) + vmParams, err := models.NewVictoriaMetricsParams(models.BasePrometheusConfigPath, vmURL) + require.NoError(t, err) + + mockHaService := newMockHaService(t) + mockHaService.On("Params").Return(&models.HAParams{Enabled: false, NodeID: "pmm-ha-service-0"}).Maybe() + mockHaService.On("IsLeader").Return(true).Maybe() + + svc, err := NewVictoriaMetrics(configPath, db, vmParams, chParams, mockHaService) + require.NoError(t, err) + require.NoError(t, svc.IsReady(t.Context())) + t.Cleanup(func() { _ = db.DBInterface().(*sql.DB).Close() }) + return db, svc + } + + newCHParams := func(t *testing.T, addr string, builtinDisabled bool) *models.ClickHouseParams { + t.Helper() + chp, err := models.NewClickHouseParams(addr, "pmm", "default", "clickhouse", builtinDisabled) + require.NoError(t, err) + return chp + } + + cases := []struct { + name string + addr string + builtinDisabled bool + vmURL string + wantClickhouse bool + }{ + {"internal enabled positive control", "127.0.0.1:9000", false, models.VMBaseURL, true}, + {"external addr, builtin not disabled (auto-skip)", "ch.external:9000", false, models.VMBaseURL, false}, + {"builtin disabled", "127.0.0.1:9000", true, models.VMBaseURL, false}, + {"external addr, builtin disabled", "ch.external:9000", true, models.VMBaseURL, false}, + {"external VM short-circuits CH scrape", "127.0.0.1:9000", false, "http://vm.external:9090/prometheus/", false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + _, svc := newSvc(t, newCHParams(t, tc.addr, tc.builtinDisabled), tc.vmURL) + cfg, err := svc.marshalConfig(svc.loadBaseConfig()) + require.NoError(t, err) + if tc.wantClickhouse { + assert.Contains(t, string(cfg), "127.0.0.1:9363") + assert.Contains(t, string(cfg), "job_name: clickhouse") + } else { + assert.NotContains(t, string(cfg), "127.0.0.1:9363") + assert.NotContains(t, string(cfg), "job_name: clickhouse") + } + }) + } +} From 3861407b28019acbb3231a70f573824f5e135a7b Mon Sep 17 00:00:00 2001 From: Ante Gulin Date: Fri, 15 May 2026 14:49:56 +0200 Subject: [PATCH 2/7] PMM-14858 Improve formatting --- managed/services/victoriametrics/victoriametrics.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/managed/services/victoriametrics/victoriametrics.go b/managed/services/victoriametrics/victoriametrics.go index d13343509d2..5434dd6837c 100644 --- a/managed/services/victoriametrics/victoriametrics.go +++ b/managed/services/victoriametrics/victoriametrics.go @@ -69,7 +69,13 @@ type Service struct { } // NewVictoriaMetrics creates new VictoriaMetrics service. -func NewVictoriaMetrics(scrapeConfigPath string, db *reform.DB, params *models.VictoriaMetricsParams, chParams *models.ClickHouseParams, haService haService) (*Service, error) { +func NewVictoriaMetrics( + scrapeConfigPath string, + db *reform.DB, + params *models.VictoriaMetricsParams, + chParams *models.ClickHouseParams, + haService haService, +) (*Service, error) { u, err := url.Parse(params.URL()) if err != nil { return nil, err From e39e7c2f8b53379804ef3c6ea66920d9a7c4e19b Mon Sep 17 00:00:00 2001 From: Ante Gulin Date: Fri, 15 May 2026 14:55:55 +0200 Subject: [PATCH 3/7] PMM-14858 Extract `127.0.0.1` literal into localhost constant --- managed/services/victoriametrics/prometheus.go | 2 +- managed/services/victoriametrics/scrape_configs.go | 10 +++++----- managed/services/victoriametrics/victoriametrics.go | 4 +++- 3 files changed, 9 insertions(+), 7 deletions(-) diff --git a/managed/services/victoriametrics/prometheus.go b/managed/services/victoriametrics/prometheus.go index 7388cbbdaf9..1aeb44f249e 100644 --- a/managed/services/victoriametrics/prometheus.go +++ b/managed/services/victoriametrics/prometheus.go @@ -87,7 +87,7 @@ func AddScrapeConfigs(l *logrus.Entry, cfg *config.Config, q *reform.Querier, // } switch { case pushMetrics: - paramsHost = "127.0.0.1" + paramsHost = localhost case agent.PMMAgentID != nil: pmmAgentNode = &models.Node{NodeID: pointer.GetString(pmmAgent.RunsOnNodeID)} if err = q.Reload(pmmAgentNode); err != nil { diff --git a/managed/services/victoriametrics/scrape_configs.go b/managed/services/victoriametrics/scrape_configs.go index 32ff7570679..5887620d5c5 100644 --- a/managed/services/victoriametrics/scrape_configs.go +++ b/managed/services/victoriametrics/scrape_configs.go @@ -55,7 +55,7 @@ func scrapeConfigForClickhouse(mr time.Duration, pmmServerNodeName string) *conf MetricsPath: "/metrics", ServiceDiscoveryConfig: config.ServiceDiscoveryConfig{ StaticConfigs: []*config.Group{{ - Targets: []string{"127.0.0.1:9363"}, + Targets: []string{localhost + ":9363"}, Labels: map[string]string{"instance": pmmServerNodeName}, }}, }, @@ -70,7 +70,7 @@ func scrapeConfigForGrafana(interval time.Duration, pmmServerNodeName string) *c MetricsPath: "/graph/metrics", ServiceDiscoveryConfig: config.ServiceDiscoveryConfig{ StaticConfigs: []*config.Group{{ - Targets: []string{"127.0.0.1:3000"}, + Targets: []string{localhost + ":3000"}, Labels: map[string]string{"instance": pmmServerNodeName}, }}, }, @@ -85,7 +85,7 @@ func scrapeConfigForPMMManaged(interval time.Duration, pmmServerNodeName string) MetricsPath: "/debug/metrics", ServiceDiscoveryConfig: config.ServiceDiscoveryConfig{ StaticConfigs: []*config.Group{{ - Targets: []string{"127.0.0.1:7773"}, + Targets: []string{localhost + ":7773"}, Labels: map[string]string{"instance": pmmServerNodeName}, }}, }, @@ -100,7 +100,7 @@ func scrapeConfigForQANAPI2(interval time.Duration, pmmServerNodeName string) *c MetricsPath: "/debug/metrics", ServiceDiscoveryConfig: config.ServiceDiscoveryConfig{ StaticConfigs: []*config.Group{{ - Targets: []string{"127.0.0.1:9933"}, + Targets: []string{localhost + ":9933"}, Labels: map[string]string{"instance": pmmServerNodeName}, }}, }, @@ -116,7 +116,7 @@ func scrapeConfigForNomadServer(resolution time.Duration, pmmServerNodeName stri Scheme: "https", ServiceDiscoveryConfig: config.ServiceDiscoveryConfig{ StaticConfigs: []*config.Group{{ - Targets: []string{"127.0.0.1:4646"}, + Targets: []string{localhost + ":4646"}, Labels: map[string]string{"instance": pmmServerNodeName}, }}, }, diff --git a/managed/services/victoriametrics/victoriametrics.go b/managed/services/victoriametrics/victoriametrics.go index 5434dd6837c..b28902a5b3d 100644 --- a/managed/services/victoriametrics/victoriametrics.go +++ b/managed/services/victoriametrics/victoriametrics.go @@ -49,6 +49,8 @@ const ( victoriametricsDir = "/srv/victoriametrics" victoriametricsDataDir = "/srv/victoriametrics/data" dirPerm = os.FileMode(0o775) + + localhost = "127.0.0.1" ) var checkFailedRE = regexp.MustCompile(`(?s)cannot unmarshal data: (.+)`) @@ -446,7 +448,7 @@ func scrapeConfigForVMAlert(interval time.Duration, pmmServerNodeName string) *c ServiceDiscoveryConfig: config.ServiceDiscoveryConfig{ StaticConfigs: []*config.Group{ { - Targets: []string{"127.0.0.1:8880"}, + Targets: []string{localhost + ":8880"}, Labels: map[string]string{"instance": pmmServerNodeName}, }, }, From 9db24f9d269d42d7115d1b1d02c2266a0b89f1a5 Mon Sep 17 00:00:00 2001 From: Ante Gulin Date: Fri, 15 May 2026 15:16:13 +0200 Subject: [PATCH 4/7] PMM-14858 Test NewVictoriaMetrics with nil CH params --- .../services/victoriametrics/victoriametrics_test.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/managed/services/victoriametrics/victoriametrics_test.go b/managed/services/victoriametrics/victoriametrics_test.go index 29cd38995f2..449a113a079 100644 --- a/managed/services/victoriametrics/victoriametrics_test.go +++ b/managed/services/victoriametrics/victoriametrics_test.go @@ -1049,3 +1049,13 @@ func TestVMConfig_OmitsClickhouseScrape(t *testing.T) { }) } } + +func TestNewVictoriaMetrics_NilClickHouseParams(t *testing.T) { + vmParams, err := models.NewVictoriaMetricsParams(models.BasePrometheusConfigPath, models.VMBaseURL) + require.NoError(t, err) + + svc, err := NewVictoriaMetrics(configPath, nil, vmParams, nil, newMockHaService(t)) + require.Error(t, err) + assert.Nil(t, svc) + assert.Contains(t, err.Error(), "ClickHouseParams is required") +} From d6f828a4c8820c04661fbd331eadb489d58aa53b Mon Sep 17 00:00:00 2001 From: Ante Gulin <8070595+4nte@users.noreply.github.com> Date: Mon, 18 May 2026 10:46:12 +0200 Subject: [PATCH 5/7] PMM-14858 Fix godoc on `URL()` Co-authored-by: Maxim Kondratenko --- managed/models/clickhouse_params.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/managed/models/clickhouse_params.go b/managed/models/clickhouse_params.go index dcb17139121..3b920f33730 100644 --- a/managed/models/clickhouse_params.go +++ b/managed/models/clickhouse_params.go @@ -33,7 +33,7 @@ func (p *ClickHouseParams) ExternalClickHouse() bool { return p.url.Hostname() != "127.0.0.1" } -// URL returns the ClickHouse DSN. +// URL returns the ClickHouse URL. func (p *ClickHouseParams) URL() *url.URL { u := *p.url return &u From 532ddae93742632536d1374384f36d7cb56286b1 Mon Sep 17 00:00:00 2001 From: Ante Gulin Date: Tue, 19 May 2026 09:55:54 +0200 Subject: [PATCH 6/7] PMM-14858 Drop redundant `PMM_DISABLE_BUILTIN_CLICKHOUSE` flag check --- managed/cmd/pmm-managed/main.go | 3 --- managed/models/clickhouse_params.go | 6 ++--- managed/models/clickhouse_params_test.go | 19 +++++--------- .../services/victoriametrics/prometheus.go | 22 ++++++---------- .../victoriametrics/victoriametrics_test.go | 25 ++++++++----------- 5 files changed, 27 insertions(+), 48 deletions(-) diff --git a/managed/cmd/pmm-managed/main.go b/managed/cmd/pmm-managed/main.go index b940bb3f2ab..eac52e54a46 100644 --- a/managed/cmd/pmm-managed/main.go +++ b/managed/cmd/pmm-managed/main.go @@ -729,8 +729,6 @@ func main() { //nolint:gocognit,maintidx,cyclop clickhouseAddrF := kingpin.Flag("clickhouse-addr", "Clickhouse database address").Default("127.0.0.1:9000").Envar("PMM_CLICKHOUSE_ADDR").String() clickhouseUsernameF := kingpin.Flag("clickhouse-username", "Clickhouse database user").Default("default").Envar("PMM_CLICKHOUSE_USER").String() clickhousePasswordF := kingpin.Flag("clickhouse-password", "Clickhouse database user password").Default("clickhouse").Envar("PMM_CLICKHOUSE_PASSWORD").String() - clickhouseBuiltinDisabledF := kingpin.Flag("clickhouse-disable-builtin", "Disable the built-in ClickHouse").Envar("PMM_DISABLE_BUILTIN_CLICKHOUSE").Bool() - watchtowerHostF := kingpin.Flag("watchtower-host", "Watchtower host").Default("http://watchtower:8080").Envar("PMM_WATCHTOWER_HOST").URL() // Nomad garbage collection flags @@ -830,7 +828,6 @@ func main() { //nolint:gocognit,maintidx,cyclop *clickHouseDatabaseF, *clickhouseUsernameF, *clickhousePasswordF, - *clickhouseBuiltinDisabledF, ) if err != nil { l.Panicf("cannot load clickhouse params: %+v", err) diff --git a/managed/models/clickhouse_params.go b/managed/models/clickhouse_params.go index 3b920f33730..4da6892474b 100644 --- a/managed/models/clickhouse_params.go +++ b/managed/models/clickhouse_params.go @@ -24,8 +24,7 @@ import ( // ClickHouseParams represents ClickHouse server params. type ClickHouseParams struct { - BuiltinDisabled bool - url *url.URL + url *url.URL } // ExternalClickHouse returns true if ClickHouse is configured externally. @@ -41,7 +40,7 @@ func (p *ClickHouseParams) URL() *url.URL { // NewClickHouseParams returns validated ClickHouse configuration params, // or an error if any required field is missing or malformed. -func NewClickHouseParams(addr, dbName, dbUsername, dbPassword string, builtinDisabled bool) (*ClickHouseParams, error) { +func NewClickHouseParams(addr, dbName, dbUsername, dbPassword string) (*ClickHouseParams, error) { if addr == "" { return nil, fmt.Errorf("addr is required") } @@ -63,7 +62,6 @@ func NewClickHouseParams(addr, dbName, dbUsername, dbPassword string, builtinDis } return &ClickHouseParams{ - BuiltinDisabled: builtinDisabled, url: &url.URL{ Scheme: "tcp", User: url.UserPassword(dbUsername, dbPassword), diff --git a/managed/models/clickhouse_params_test.go b/managed/models/clickhouse_params_test.go index cabbc4ee710..879e500a3fa 100644 --- a/managed/models/clickhouse_params_test.go +++ b/managed/models/clickhouse_params_test.go @@ -24,20 +24,13 @@ import ( func TestNewClickHouseParams(t *testing.T) { t.Run("valid", func(t *testing.T) { - p, err := NewClickHouseParams("127.0.0.1:9000", "pmm", "default", "clickhouse", false) + p, err := NewClickHouseParams("127.0.0.1:9000", "pmm", "default", "clickhouse") require.NoError(t, err) assert.Equal(t, "tcp://default:clickhouse@127.0.0.1:9000/pmm", p.URL().String()) - assert.False(t, p.BuiltinDisabled) - }) - - t.Run("valid builtin disabled", func(t *testing.T) { - p, err := NewClickHouseParams("127.0.0.1:9000", "pmm", "default", "clickhouse", true) - require.NoError(t, err) - assert.True(t, p.BuiltinDisabled) }) t.Run("valid empty password", func(t *testing.T) { - _, err := NewClickHouseParams("127.0.0.1:9000", "pmm", "default", "", false) + _, err := NewClickHouseParams("127.0.0.1:9000", "pmm", "default", "") require.NoError(t, err) }) @@ -59,7 +52,7 @@ func TestNewClickHouseParams(t *testing.T) { } for _, tc := range errCases { t.Run(tc.name, func(t *testing.T) { - _, err := NewClickHouseParams(tc.addr, tc.dbName, tc.dbUsername, tc.dbPassword, false) + _, err := NewClickHouseParams(tc.addr, tc.dbName, tc.dbUsername, tc.dbPassword) require.Error(t, err) assert.ErrorContains(t, err, tc.wantErrSub) }) @@ -79,7 +72,7 @@ func TestCHParamsExternalClickHouse(t *testing.T) { } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - p, err := NewClickHouseParams(tc.addr, "pmm", "default", "clickhouse", false) + p, err := NewClickHouseParams(tc.addr, "pmm", "default", "clickhouse") require.NoError(t, err) assert.Equal(t, tc.want, p.ExternalClickHouse()) }) @@ -88,13 +81,13 @@ func TestCHParamsExternalClickHouse(t *testing.T) { func TestCHParamsURL(t *testing.T) { t.Run("simple", func(t *testing.T) { - p, err := NewClickHouseParams("127.0.0.1:9000", "pmm", "default", "clickhouse", false) + p, err := NewClickHouseParams("127.0.0.1:9000", "pmm", "default", "clickhouse") require.NoError(t, err) assert.Equal(t, "tcp://default:clickhouse@127.0.0.1:9000/pmm", p.URL().String()) }) t.Run("password with special chars", func(t *testing.T) { - p, err := NewClickHouseParams("127.0.0.1:9000", "pmm", "default", "p@ss/word", false) + p, err := NewClickHouseParams("127.0.0.1:9000", "pmm", "default", "p@ss/word") require.NoError(t, err) assert.Equal(t, "tcp://default:p%40ss%2Fword@127.0.0.1:9000/pmm", p.URL().String()) }) diff --git a/managed/services/victoriametrics/prometheus.go b/managed/services/victoriametrics/prometheus.go index 1aeb44f249e..d3e86a6d661 100644 --- a/managed/services/victoriametrics/prometheus.go +++ b/managed/services/victoriametrics/prometheus.go @@ -266,27 +266,21 @@ func AddScrapeConfigs(l *logrus.Entry, cfg *config.Config, q *reform.Querier, // // AddInternalServicesToScrape adds internal services metrics to scrape targets. func addInternalServicesToScrape(s models.MetricsResolutions, svc *Service, pmmServerNodeName string) []*config.ScrapeConfig { - cfg := []*config.ScrapeConfig{ - scrapeConfigForGrafana(s.MR, pmmServerNodeName), - scrapeConfigForPMMManaged(s.MR, pmmServerNodeName), - scrapeConfigForQANAPI2(s.MR, pmmServerNodeName), - } + cfg := []*config.ScrapeConfig{} if svc.params.ExternalVM() { - svc.l.Infof("Skip scrape config for ClickHouse, VictoriaMetrics is configured to run externally.") + svc.l.Infof("Skip all scrape configs, VictoriaMetrics is configured to run externally.") return cfg } - if svc.chParams.BuiltinDisabled { - svc.l.Infof("Skip scrape config for ClickHouse, scraping has been disabled for builtin clickhouse.") - return cfg - } + cfg = append(cfg, + scrapeConfigForGrafana(s.MR, pmmServerNodeName), + scrapeConfigForPMMManaged(s.MR, pmmServerNodeName), + scrapeConfigForQANAPI2(s.MR, pmmServerNodeName), + ) if svc.chParams.ExternalClickHouse() { - svc.l.Warnf( - "External ClickHouse detected (%s); skipping built-in ClickHouse scrape (127.0.0.1:9363). Set PMM_DISABLE_BUILTIN_CLICKHOUSE=true to make this explicit.", - svc.chParams.URL().Redacted(), - ) + svc.l.Warnf("Skip ClickHouse scrape config, ClickHouse is configured to run externally.") return cfg } diff --git a/managed/services/victoriametrics/victoriametrics_test.go b/managed/services/victoriametrics/victoriametrics_test.go index 449a113a079..6998b2aebaa 100644 --- a/managed/services/victoriametrics/victoriametrics_test.go +++ b/managed/services/victoriametrics/victoriametrics_test.go @@ -45,7 +45,7 @@ func setup(t *testing.T) (*reform.DB, *Service, []byte) { vmParams, err := models.NewVictoriaMetricsParams(models.BasePrometheusConfigPath, models.VMBaseURL) check.NoError(err) - chParams, err := models.NewClickHouseParams("127.0.0.1:9000", "pmm", "default", "clickhouse", false) + chParams, err := models.NewClickHouseParams("127.0.0.1:9000", "pmm", "default", "clickhouse") check.NoError(err) mockHaService := newMockHaService(t) @@ -1014,29 +1014,26 @@ func TestVMConfig_OmitsClickhouseScrape(t *testing.T) { return db, svc } - newCHParams := func(t *testing.T, addr string, builtinDisabled bool) *models.ClickHouseParams { + newCHParams := func(t *testing.T, addr string) *models.ClickHouseParams { t.Helper() - chp, err := models.NewClickHouseParams(addr, "pmm", "default", "clickhouse", builtinDisabled) + chp, err := models.NewClickHouseParams(addr, "pmm", "default", "clickhouse") require.NoError(t, err) return chp } cases := []struct { - name string - addr string - builtinDisabled bool - vmURL string - wantClickhouse bool + name string + addr string + vmURL string + wantClickhouse bool }{ - {"internal enabled positive control", "127.0.0.1:9000", false, models.VMBaseURL, true}, - {"external addr, builtin not disabled (auto-skip)", "ch.external:9000", false, models.VMBaseURL, false}, - {"builtin disabled", "127.0.0.1:9000", true, models.VMBaseURL, false}, - {"external addr, builtin disabled", "ch.external:9000", true, models.VMBaseURL, false}, - {"external VM short-circuits CH scrape", "127.0.0.1:9000", false, "http://vm.external:9090/prometheus/", false}, + {"internal enabled positive control", "127.0.0.1:9000", models.VMBaseURL, true}, + {"external addr skips scrape", "ch.external:9000", models.VMBaseURL, false}, + {"external VM short-circuits CH scrape", "127.0.0.1:9000", "http://vm.external:9090/prometheus/", false}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - _, svc := newSvc(t, newCHParams(t, tc.addr, tc.builtinDisabled), tc.vmURL) + _, svc := newSvc(t, newCHParams(t, tc.addr), tc.vmURL) cfg, err := svc.marshalConfig(svc.loadBaseConfig()) require.NoError(t, err) if tc.wantClickhouse { From 09179cdfa52364f50b3c84c93d74b6338368aacd Mon Sep 17 00:00:00 2001 From: Ante Gulin Date: Tue, 19 May 2026 10:02:09 +0200 Subject: [PATCH 7/7] PMM-14858 Improve log clarity --- managed/services/victoriametrics/prometheus.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/managed/services/victoriametrics/prometheus.go b/managed/services/victoriametrics/prometheus.go index d3e86a6d661..c0db7ac8aeb 100644 --- a/managed/services/victoriametrics/prometheus.go +++ b/managed/services/victoriametrics/prometheus.go @@ -269,7 +269,7 @@ func addInternalServicesToScrape(s models.MetricsResolutions, svc *Service, pmmS cfg := []*config.ScrapeConfig{} if svc.params.ExternalVM() { - svc.l.Infof("Skip all scrape configs, VictoriaMetrics is configured to run externally.") + svc.l.Infof("Skip all internal-service configs, VictoriaMetrics is configured to run externally.") return cfg } @@ -280,7 +280,7 @@ func addInternalServicesToScrape(s models.MetricsResolutions, svc *Service, pmmS ) if svc.chParams.ExternalClickHouse() { - svc.l.Warnf("Skip ClickHouse scrape config, ClickHouse is configured to run externally.") + svc.l.Warnf("Skip internal ClickHouse scrape config, ClickHouse is configured to run externally.") return cfg }