Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add unmanaged tablet flag at vttablet level #14871

Merged
merged 11 commits into from
Mar 14, 2024
7 changes: 7 additions & 0 deletions changelog/20.0/20.0.0/summary.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
- **[Major Changes](#major-changes)**
- **[Breaking changes](#breaking-changes)**
- [`shutdown_grace_period` Default Change](#shutdown-grace-period-default)
- [New `unmanaged` Flag](#unmanaged-flag)
- **[Query Compatibility](#query-compatibility)**
- [Vindex Hints](#vindex-hints)
- [Update with Limit Support](#update-limit)
Expand All @@ -28,6 +29,12 @@ This makes reparenting in Vitess resilient to client errors, and prevents Planne

In order to preserve the old behaviour, the users can set the flag back to `0 seconds` causing open transactions to never be shutdown, but in that case, they run the risk of PlannedReparentShard calls timing out.

#### <a id="unmanaged-tablet"/> New `unmanaged` Flag

New flag `--unmanaged` has been introduced in this release to make it easier to flag unmanaged tablets. It also runs validations to make sure the unmanaged tablets are configured properly.

Starting this release, all unmanaged tablets should specify this flag.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

People can still not specify this, and just specify the existing set of flags, correct?
We should make this language stronger, and say that we recommend setting this flag so that we can validate all the related flags.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking about this again, and I believe we should deprecate --disable_active_reparents along with adding this flag.
For backwards compatibility, we should deprecate --disable_active_reparents in v20, and delete it in v21.
The internal value of mysqlctl.DisableActiveReparents should be the OR of --unmanaged and --disable_active_reparents in v20, and just the value of --unmanaged in v21.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

disable_active_reparents is used in vtctldclient as well to prevent that specific instance from running reparent commands. I don't think we can get rid of that. We could rename the flag on vtctld too?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be fine to leave that flag on vtctld. Having the same flag name on vttablet and vtctld was always confusing. I'm not sure if the two are sharing code which might make this separation more complicated, though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done 😎 I made the required changes and deprecated the flag only on the required binaries.


### <a id="query-compatibility"/>Query Compatibility

#### <a id="vindex-hints"/> Vindex Hints
Expand Down
1 change: 1 addition & 0 deletions go/flags/endtoend/vtcombo.txt
Original file line number Diff line number Diff line change
Expand Up @@ -397,6 +397,7 @@ Flags:
--tx_throttler_config string The configuration of the transaction throttler as a text-formatted throttlerdata.Configuration protocol buffer message. (default "target_replication_lag_sec:2 max_replication_lag_sec:10 initial_rate:100 max_increase:1 emergency_decrease:0.5 min_duration_between_increases_sec:40 max_duration_between_increases_sec:62 min_duration_between_decreases_sec:20 spread_backlog_across_sec:20 age_bad_rate_after_sec:180 bad_rate_increase:0.1 max_rate_approach_threshold:0.9")
--tx_throttler_healthcheck_cells strings A comma-separated list of cells. Only tabletservers running in these cells will be monitored for replication lag by the transaction throttler.
--unhealthy_threshold duration replication lag after which a replica is considered unhealthy (default 2h0m0s)
--unmanaged Indicates an unmanaged tablet, i.e. using an external mysql-compatible database
--v Level log level for V logs
-v, --version print binary version
--vmodule vModuleFlag comma-separated list of pattern=N settings for file-filtered logging
Expand Down
1 change: 1 addition & 0 deletions go/flags/endtoend/vttablet.txt
Original file line number Diff line number Diff line change
Expand Up @@ -399,6 +399,7 @@ Flags:
--tx_throttler_config string The configuration of the transaction throttler as a text-formatted throttlerdata.Configuration protocol buffer message. (default "target_replication_lag_sec:2 max_replication_lag_sec:10 initial_rate:100 max_increase:1 emergency_decrease:0.5 min_duration_between_increases_sec:40 max_duration_between_increases_sec:62 min_duration_between_decreases_sec:20 spread_backlog_across_sec:20 age_bad_rate_after_sec:180 bad_rate_increase:0.1 max_rate_approach_threshold:0.9")
--tx_throttler_healthcheck_cells strings A comma-separated list of cells. Only tabletservers running in these cells will be monitored for replication lag by the transaction throttler.
--unhealthy_threshold duration replication lag after which a replica is considered unhealthy (default 2h0m0s)
--unmanaged Indicates an unmanaged tablet, i.e. using an external mysql-compatible database
--v Level log level for V logs
-v, --version print binary version
--vmodule vModuleFlag comma-separated list of pattern=N settings for file-filtered logging
Expand Down
55 changes: 55 additions & 0 deletions go/vt/vttablet/tabletserver/tabletenv/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
package tabletenv

import (
"context"
"encoding/json"
"errors"
"fmt"
Expand All @@ -27,9 +28,11 @@
"google.golang.org/protobuf/encoding/prototext"

"vitess.io/vitess/go/flagutil"
"vitess.io/vitess/go/mysql"
"vitess.io/vitess/go/streamlog"
"vitess.io/vitess/go/vt/dbconfigs"
"vitess.io/vitess/go/vt/log"
"vitess.io/vitess/go/vt/mysqlctl"
"vitess.io/vitess/go/vt/servenv"
"vitess.io/vitess/go/vt/sqlparser"
"vitess.io/vitess/go/vt/throttler"
Expand Down Expand Up @@ -213,6 +216,8 @@
fs.BoolVar(&currentConfig.EnableViews, "queryserver-enable-views", false, "Enable views support in vttablet.")

fs.BoolVar(&currentConfig.EnablePerWorkloadTableMetrics, "enable-per-workload-table-metrics", defaultConfig.EnablePerWorkloadTableMetrics, "If true, query counts and query error metrics include a label that identifies the workload")

fs.BoolVar(&currentConfig.UnmanagedTablet, "unmanaged", false, "Indicates an unmanaged tablet, i.e. using an external mysql-compatible database")
}

var (
Expand Down Expand Up @@ -298,6 +303,8 @@
type TabletConfig struct {
DB *dbconfigs.DBConfigs `json:"db,omitempty"`

UnmanagedTablet bool `json:"unmanaged,omitempty"`
GuptaManan100 marked this conversation as resolved.
Show resolved Hide resolved

OltpReadPool ConnPoolConfig `json:"oltpReadPool,omitempty"`
OlapReadPool ConnPoolConfig `json:"olapReadPool,omitempty"`
TxPool ConnPoolConfig `json:"txPool,omitempty"`
Expand Down Expand Up @@ -861,6 +868,9 @@

// Verify checks for contradicting flags.
func (c *TabletConfig) Verify() error {
if err := c.verifyUnmanagedTabletConfig(); err != nil {
return err

Check warning on line 872 in go/vt/vttablet/tabletserver/tabletenv/config.go

View check run for this annotation

Codecov / codecov/patch

go/vt/vttablet/tabletserver/tabletenv/config.go#L871-L872

Added lines #L871 - L872 were not covered by tests
}
if err := c.verifyTransactionLimitConfig(); err != nil {
return err
}
Expand All @@ -882,6 +892,51 @@
return nil
}

// verifyUnmanagedTabletConfig checks unmanaged tablet related config for sanity
func (c *TabletConfig) verifyUnmanagedTabletConfig() error {
// Skip checks if tablet is not unmanaged
if !c.UnmanagedTablet {
return nil
}

// Throw error if both host and socket are null
if !c.DB.HasGlobalSettings() {
return errors.New("no connection parameters specified but unmanaged mode specified")
}
if c.DB.App.User == "" {
return errors.New("database app user not specified")
}
if c.DB.App.Password == "" {
return errors.New("database app user password not specified")
}
GuptaManan100 marked this conversation as resolved.
Show resolved Hide resolved
GuptaManan100 marked this conversation as resolved.
Show resolved Hide resolved
if !mysqlctl.DisableActiveReparents {
return errors.New("fixing replication should be disabled on unmanaged tablets")
}

return c.checkConnectionForExternalMysql()
}

// Test connectivity of external mysql
func (c *TabletConfig) checkConnectionForExternalMysql() error {
params := mysql.ConnParams{
Host: c.DB.Host,
Port: c.DB.Port,
DbName: c.DB.DBName,
Uname: c.DB.App.User,
Pass: c.DB.App.Password,
UnixSocket: c.DB.Socket,
}

conn, err := mysql.Connect(context.Background(), &params)
if err != nil {
return err

Check warning on line 932 in go/vt/vttablet/tabletserver/tabletenv/config.go

View check run for this annotation

Codecov / codecov/patch

go/vt/vttablet/tabletserver/tabletenv/config.go#L932

Added line #L932 was not covered by tests
}
GuptaManan100 marked this conversation as resolved.
Show resolved Hide resolved

defer conn.Close()

return conn.Ping()
}

// verifyTransactionLimitConfig checks TransactionLimitConfig for sanity
func (c *TabletConfig) verifyTransactionLimitConfig() error {
actual, dryRun := c.EnableTransactionLimit, c.EnableTransactionLimitDryRun
Expand Down
42 changes: 42 additions & 0 deletions go/vt/vttablet/tabletserver/tabletenv/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,10 @@ import (
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

"vitess.io/vitess/go/mysql/fakesqldb"
"vitess.io/vitess/go/test/utils"
"vitess.io/vitess/go/vt/dbconfigs"
"vitess.io/vitess/go/vt/mysqlctl"
"vitess.io/vitess/go/vt/throttler"
"vitess.io/vitess/go/vt/topo/topoproto"
"vitess.io/vitess/go/vt/vterrors"
Expand Down Expand Up @@ -444,3 +446,43 @@ func TestVerifyTxThrottlerConfig(t *testing.T) {
})
}
}

func TestVerifyUnmanagedTabletConfig(t *testing.T) {
oldDisableActiveReparents := mysqlctl.DisableActiveReparents
defer func() {
mysqlctl.DisableActiveReparents = oldDisableActiveReparents
}()

config := defaultConfig

db := fakesqldb.New(t)
defer db.Close()

params := db.ConnParams()
config.DB = dbconfigs.NewTestDBConfigs(*params, *params, "")

// By default, unmanaged mode should be false
err := config.verifyUnmanagedTabletConfig()
assert.Nil(t, err)

config.UnmanagedTablet = true
err = config.verifyUnmanagedTabletConfig()
assert.EqualError(t, err, "no connection parameters specified but unmanaged mode specified")

config.DB.Socket = db.ConnParams().UnixSocket
err = config.verifyUnmanagedTabletConfig()
assert.EqualError(t, err, "database app user not specified")

config.DB.App.User = "testUser"
err = config.verifyUnmanagedTabletConfig()
assert.EqualError(t, err, "database app user password not specified")

config.DB.App.Password = "testPassword"
mysqlctl.DisableActiveReparents = false
err = config.verifyUnmanagedTabletConfig()
assert.EqualError(t, err, "fixing replication should be disabled on unmanaged tablets")

mysqlctl.DisableActiveReparents = true
err = config.verifyUnmanagedTabletConfig()
assert.Nil(t, err)
}
Loading