diff --git a/pkg/cluster/manager/scale_in.go b/pkg/cluster/manager/scale_in.go index 1b18251f34..d243015c31 100644 --- a/pkg/cluster/manager/scale_in.go +++ b/pkg/cluster/manager/scale_in.go @@ -106,6 +106,8 @@ func (m *Manager) ScaleIn( return err } + scaledInPD := isScaledInPD(topo, nodes) + b, err := m.sshTaskBuilder(name, topo, base.User, gOpt) if err != nil { return err @@ -161,6 +163,9 @@ func (m *Manager) ScaleIn( } m.logger.Infof("Scaled cluster `%s` in successfully", name) + if warning := dashboardScaleInWarning(name, scaledInPD, topo); warning != "" { + m.logger.Warnf("%s", warning) + } return nil } @@ -186,3 +191,29 @@ func checkAsyncComps(topo spec.Topology, nodes []string) error { } return nil } + +// isScaledInPD reports whether any of the given node IDs belongs to a PD instance. +func isScaledInPD(topo spec.Topology, nodes []string) bool { + deletedNodes := set.NewStringSet(nodes...) + scaledInPD := false + topo.IterInstance(func(inst spec.Instance) { + if deletedNodes.Exist(inst.ID()) && inst.ComponentName() == spec.ComponentPD { + scaledInPD = true + } + }) + return scaledInPD +} + +// dashboardScaleInWarning returns restart guidance when PD was scaled in and a +// standalone Dashboard remains in the updated topology. +func dashboardScaleInWarning(name string, scaledInPD bool, topo spec.Topology) string { + dash := spec.FindComponent(topo, spec.ComponentDashboard) + if !scaledInPD || dash == nil || len(dash.Instances()) == 0 { + return "" + } + return color.YellowString( + "\nSince PD node(s) were scaled in, the standalone tidb-dashboard connects to a single PD endpoint "+ + "that may have been removed. If it can no longer reach PD, restart it to pick up a new endpoint:\n\t%s", + color.GreenString("%s restart %s -R %s", tui.OsArgs0(), name, spec.ComponentDashboard), + ) +} diff --git a/pkg/cluster/manager/scale_in_test.go b/pkg/cluster/manager/scale_in_test.go new file mode 100644 index 0000000000..98e639945a --- /dev/null +++ b/pkg/cluster/manager/scale_in_test.go @@ -0,0 +1,73 @@ +// Copyright 2020 PingCAP, Inc. +// +// 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, +// See the License for the specific language governing permissions and +// limitations under the License. + +package manager + +import ( + "testing" + + "github.com/pingcap/tiup/pkg/cluster/spec" + "github.com/pingcap/tiup/pkg/tui" + "github.com/stretchr/testify/require" +) + +func TestIsScaledInPD(t *testing.T) { + topo := &spec.Specification{ + PDServers: []*spec.PDSpec{ + {Host: "10.0.0.1", ClientPort: 2379}, + {Host: "10.0.0.2", ClientPort: 2379}, + }, + DashboardServers: []*spec.DashboardSpec{ + {Host: "10.0.0.50", Port: 12333}, + }, + } + + require.True(t, isScaledInPD(topo, []string{"10.0.0.1:2379"})) + require.True(t, isScaledInPD(topo, []string{"10.0.0.2:2379", "10.0.0.50:12333"})) + require.False(t, isScaledInPD(topo, []string{"10.0.0.50:12333"})) + require.False(t, isScaledInPD(topo, nil)) +} + +func TestDashboardScaleInWarning(t *testing.T) { + for _, tt := range []struct { + name string + nodes []string + dashboard bool + wantWarning bool + }{ + {"PD with standalone Dashboard", []string{"10.0.0.1:2379"}, true, true}, + {"non-PD with standalone Dashboard", []string{"10.0.0.10:4000"}, true, false}, + {"PD without standalone Dashboard", []string{"10.0.0.1:2379"}, false, false}, + {"non-PD without standalone Dashboard", []string{"10.0.0.10:4000"}, false, false}, + } { + t.Run(tt.name, func(t *testing.T) { + before := &spec.Specification{ + PDServers: []*spec.PDSpec{{Host: "10.0.0.1", ClientPort: 2379}}, + TiDBServers: []*spec.TiDBSpec{{Host: "10.0.0.10", Port: 4000}}, + } + // ScaleIn records PD removal before executing tasks, then checks + // for Dashboard in the refreshed topology after successful scale-in. + after := &spec.Specification{} + if tt.dashboard { + after.DashboardServers = []*spec.DashboardSpec{{Host: "10.0.0.50", Port: 12333}} + } + warning := dashboardScaleInWarning("test-cluster", isScaledInPD(before, tt.nodes), after) + if tt.wantWarning { + require.Contains(t, warning, "Since PD node(s) were scaled in") + require.Contains(t, warning, tui.OsArgs0()+" restart test-cluster -R tidb-dashboard") + } else { + require.Empty(t, warning) + } + }) + } +} diff --git a/pkg/cluster/spec/dashboard.go b/pkg/cluster/spec/dashboard.go index e29f68c990..fc70806645 100644 --- a/pkg/cluster/spec/dashboard.go +++ b/pkg/cluster/spec/dashboard.go @@ -18,7 +18,6 @@ import ( "crypto/tls" "fmt" "path/filepath" - "strings" "time" "github.com/pingcap/tiup/pkg/cluster/ctxt" @@ -198,10 +197,6 @@ func (i *DashboardInstance) InitConfig( enableTLS := topo.GlobalOptions.TLSEnabled spec := i.InstanceSpec.(*DashboardSpec) - pds := []string{} - for _, pdspec := range topo.PDServers { - pds = append(pds, pdspec.GetAdvertiseClientURL(enableTLS)) - } cfg := &scripts.DashboardScript{ // -h, --host string listen host of the Dashboard Server Host: i.GetListenHost(), @@ -211,7 +206,7 @@ func (i *DashboardInstance) InitConfig( LogDir: paths.Log, Port: spec.Port, NumaNode: spec.NumaNode, - PD: strings.Join(pds, ","), + PD: dashboardPDEndpoint(topo.PDServers, enableTLS), TLSEnabled: enableTLS, } @@ -243,3 +238,17 @@ func (i *DashboardInstance) InitConfig( func (i *DashboardInstance) setTLSConfig(ctx context.Context, enableTLS bool, configs map[string]any, paths meta.DirPaths) (map[string]any, error) { return nil, nil } + +// dashboardPDEndpoint returns the PD URL for standalone tidb-dashboard. +// +// Dashboard treats --pd as a single endpoint and does not split a +// comma-separated list, so passing all PD endpoints breaks its PD +// connection. See https://github.com/pingcap/tidb-dashboard/issues/1920. +// Pass only the first PD until Dashboard supports multiple PD HTTP API +// endpoints, then restore joining all PD URLs. +func dashboardPDEndpoint(pdServers []*PDSpec, enableTLS bool) string { + if len(pdServers) == 0 { + return "" + } + return pdServers[0].GetAdvertiseClientURL(enableTLS) +} diff --git a/pkg/cluster/spec/dashboard_test.go b/pkg/cluster/spec/dashboard_test.go new file mode 100644 index 0000000000..9a36aa4318 --- /dev/null +++ b/pkg/cluster/spec/dashboard_test.go @@ -0,0 +1,117 @@ +// Copyright 2020 PingCAP, Inc. +// +// 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, +// See the License for the specific language governing permissions and +// limitations under the License. + +package spec + +import ( + "context" + "errors" + "os" + "path/filepath" + "regexp" + "strings" + "testing" + "time" + + "github.com/pingcap/tiup/pkg/cluster/ctxt" + logprinter "github.com/pingcap/tiup/pkg/logger/printer" + "github.com/pingcap/tiup/pkg/meta" + "github.com/stretchr/testify/require" +) + +func TestDashboardPDEndpoint(t *testing.T) { + t.Run("selects the first PD endpoint", func(t *testing.T) { + pds := []*PDSpec{ + {Host: "10.0.0.1", ClientPort: 2379}, + {Host: "10.0.0.2", ClientPort: 2379}, + } + require.Equal(t, "http://10.0.0.1:2379", dashboardPDEndpoint(pds, false)) + }) + + t.Run("returns empty for no PD servers", func(t *testing.T) { + require.Equal(t, "", dashboardPDEndpoint(nil, false)) + require.Equal(t, "", dashboardPDEndpoint([]*PDSpec{}, false)) + }) +} + +// TestDashboardScriptSinglePD verifies that the generated startup script passes +// a single --pd endpoint even when the cluster has multiple PD servers, which +// is the standalone tidb-dashboard connectivity fix. +func TestDashboardScriptSinglePD(t *testing.T) { + tests := []struct { + name string + pds []*PDSpec + enableTLS bool + wantPD string + }{ + { + name: "uses only the first PD", + pds: []*PDSpec{{Host: "10.0.0.1", ClientPort: 2379}, {Host: "10.0.0.2", ClientPort: 2379}}, + wantPD: "http://10.0.0.1:2379", + }, + { + name: "uses https when TLS is enabled", + pds: []*PDSpec{{Host: "10.0.0.1", ClientPort: 2379}}, + enableTLS: true, + wantPD: "https://10.0.0.1:2379", + }, + { + name: "preserves an explicit advertised client address", + pds: []*PDSpec{{Host: "10.0.0.1", ClientPort: 2379, AdvertiseClientAddr: "https://pd.example.com:443"}, {Host: "10.0.0.2", ClientPort: 2379}}, + wantPD: "https://pd.example.com:443", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + topo := &Specification{ + GlobalOptions: GlobalOptions{User: "tidb", SystemdMode: UserMode, TLSEnabled: tt.enableTLS}, + PDServers: tt.pds, + DashboardServers: []*DashboardSpec{{Host: "10.0.0.50", Port: 12333}}, + } + deployDir := t.TempDir() + paths := meta.DirPaths{ + Deploy: deployDir, + Cache: t.TempDir(), + Data: []string{filepath.Join(deployDir, "data")}, + Log: filepath.Join(deployDir, "log"), + } + comp := DashboardComponent{Topology: topo} + instance := comp.Instances()[0].(*DashboardInstance) + ctx := ctxt.New(context.Background(), 0, logprinter.NewLogger("")) + // Stop after the script transfer, before config validation looks up + // binaries in the global TiUP repository. No mirror or installation + // is needed to verify the production script-generation path. + scriptTransferred := errors.New("stop after startup script transfer") + executor := &mockExecutor{executeFunc: func(_ context.Context, cmd string, _ bool, _ ...time.Duration) ([]byte, []byte, error) { + if cmd == "chmod +x "+filepath.Join(deployDir, "scripts", "run_tidb-dashboard.sh") { + return nil, nil, scriptTransferred + } + // The mock copies the unit file but does not execute mv. + // Remove that temporary source instead of leaking it in /tmp. + if strings.HasPrefix(cmd, "mv /tmp/tidb-dashboard_") { + require.NoError(t, os.Remove(strings.Fields(cmd)[1])) + } + return nil, nil, nil + }} + require.ErrorIs(t, instance.InitConfig(ctx, executor, "test-cluster", "v8.5.0", "tidb", paths), scriptTransferred) + + // Inspect the transferred script, so regressions in InitConfig wiring fail. + body, err := os.ReadFile(filepath.Join(deployDir, "scripts", "run_tidb-dashboard.sh")) + require.NoError(t, err) + endpoints := regexp.MustCompile(`--pd="([^"\n]*)"`).FindAllStringSubmatch(string(body), -1) + require.Len(t, endpoints, 1) + require.Equal(t, tt.wantPD, endpoints[0][1]) + }) + } +}