[cli] [tabletmanager] Migrate all tabletmanager flags to pflag (vit…

…essio#11057)

* [tabletmanager] Migrate tabletmanager's orchestrator flags to pflag

Signed-off-by: Andrew Mason <[email protected]>

* [tabletmanager] Migrate tabletmanager's restore flags to pflag

Signed-off-by: Andrew Mason <[email protected]>

* [tabletmanager] Migrate tabletmanager's lockTables flags to pflag

Signed-off-by: Andrew Mason <[email protected]>

* [tabletmanager] Migrate tabletmanager's replication flags to pflag

Signed-off-by: Andrew Mason <[email protected]>

* [tabletmanager] Migrate tabletmanager's shard_sync flags to pflag

Signed-off-by: Andrew Mason <[email protected]>

* [tabletmanager] Migrate tabletmanager's tm_init flags to pflag

Signed-off-by: Andrew Mason <[email protected]>

* [tabletmanager] Migrate tabletmanager's tm_state flags to pflag

Signed-off-by: Andrew Mason <[email protected]>

* [tabletmanager_test] tidy up imports

Signed-off-by: Andrew Mason <[email protected]>

Signed-off-by: Andrew Mason <[email protected]>

doc/SeparatingVttabletMysql.md

@@ -25,7 +25,7 @@ The following adjustments need to be made to VTTablet command line parameters:
   `-db\_socket` parameters.
 
 * Disable restores / backups, by not passing any backup command line
-  parameters. Specifically, `-restore_from_backup` and
+  parameters. Specifically, `--restore_from_backup` and
   `-backup_storage_implementation` should not be set.
 
 Since cluster management and replication are not handled by Vitess, we just need

go/flags/endtoend/vttablet.txt

@@ -25,7 +25,7 @@ Usage of vttablet:
       --binlog_ssl_ca string                                             PITR restore parameter: Filename containing TLS CA certificate to verify binlog server TLS certificate against.
       --binlog_ssl_cert string                                           PITR restore parameter: Filename containing mTLS client certificate to present to binlog server as authentication.
       --binlog_ssl_key string                                            PITR restore parameter: Filename containing mTLS client private key for use in binlog server authentication.
-      --binlog_ssl_server_name string                                    PITR restore parameter: TLS server name (common name) to verify against for the binlog server we are connecting to (If not set: use the hostname or IP supplied in -binlog_host).
+      --binlog_ssl_server_name string                                    PITR restore parameter: TLS server name (common name) to verify against for the binlog server we are connecting to (If not set: use the hostname or IP supplied in --binlog_host).
       --binlog_user string                                               PITR restore parameter: username of binlog server.
       --builtinbackup_mysqld_timeout duration                            how long to wait for mysqld to shutdown at the start of the backup (default 10m0s)
       --builtinbackup_progress duration                                  how often to send progress updates when backing up large files (default 5s)
@@ -195,7 +195,6 @@ Usage of vttablet:
       --enable_lag_throttler                                             If true, vttablet will run a throttler service, and will implicitly enable heartbeats
       --enable_query_plan_field_caching                                  (DEPRECATED) This option fetches & caches fields (columns) when storing query plans (default true)
       --enable_replication_reporter                                      Use polling to track replication lag.
-      --enable_semi_sync                                                 DEPRECATED - Set the correct durability policy on the keyspace instead.
       --enable_transaction_limit                                         If true, limit on number of transactions open at the same time will be enforced for all users. User trying to open a new transaction after exhausting their limit will receive an error immediately, regardless of whether there are available slots or not.
       --enable_transaction_limit_dry_run                                 If true, limit on number of transactions open at the same time will be tracked for all users, but not enforced.
       --enable_tx_throttler                                              If true replication-lag-based throttling on transactions will be enabled.
@@ -297,8 +296,8 @@ Usage of vttablet:
       --orc_api_password string                                          (Optional) Basic auth password to authenticate with Orchestrator's HTTP API.
       --orc_api_url string                                               Address of Orchestrator's HTTP API (e.g. http://host:port/api/). Leave empty to disable Orchestrator integration.
       --orc_api_user string                                              (Optional) Basic auth username to authenticate with Orchestrator's HTTP API. Leave empty to disable basic auth.
-      --orc_discover_interval duration                                   How often to ping Orchestrator's HTTP API endpoint to tell it we exist. 0 means never. (default 0s)
-      --orc_timeout duration                                             Timeout for calls to Orchestrator's HTTP API (default 30s)
+      --orc_discover_interval duration                                   How often to ping Orchestrator's HTTP API endpoint to tell it we exist. 0 means never.
+      --orc_timeout duration                                             Timeout for calls to Orchestrator's HTTP API. (default 30s)
       --pid_file string                                                  If set, the process will write its pid to the named file, and delete it on graceful shutdown.
       --pitr_gtid_lookup_timeout duration                                PITR restore parameter: timeout for fetching gtid from timestamp. (default 1m0s)
       --pool_hostname_resolve_interval duration                          if set force an update to all hostnames and reconnect if changed, defaults to 0 (disabled) (default 0s)
@@ -469,8 +468,8 @@ Usage of vttablet:
       --vstream_packet_size int                                          Suggested packet size for VReplication streamer. This is used only as a recommendation. The actual packet size may be more or less than this amount. (default 250000)
       --vtctld_addr string                                               address of a vtctld instance
       --vtgate_protocol string                                           how to talk to vtgate (default "grpc")
-      --vttablet_skip_buildinfo_tags string                              comma-separated list of buildinfo tags to skip from merging with -init_tags. each tag is either an exact match or a regular expression of the form '/regexp/'. (default "/.*/")
-      --wait_for_backup_interval duration                                (init restore parameter) if this is greater than 0, instead of starting up empty when no backups are found, keep checking at this interval for a backup to appear (default 0s)
+      --vttablet_skip_buildinfo_tags string                              comma-separated list of buildinfo tags to skip from merging with --init_tags. each tag is either an exact match or a regular expression of the form '/regexp/'. (default "/.*/")
+      --wait_for_backup_interval duration                                (init restore parameter) if this is greater than 0, instead of starting up empty when no backups are found, keep checking at this interval for a backup to appear
       --watch_replication_stream                                         When enabled, vttablet will stream the MySQL replication stream from the local server, and use it to update schema when it sees a DDL.
       --xbstream_restore_flags string                                    flags to pass to xbstream command during restore. These should be space separated and will be added to the end of the command. These need to match the ones used for backup e.g. --compress / --decompress, --encrypt / --decrypt
       --xtrabackup_backup_flags string                                   flags to pass to backup command. These should be space separated and will be added to the end of the command

go/flagutil/flagutil.go

@@ -93,6 +93,9 @@ func StringListVar(p *[]string, name string, defaultValue []string, usage string
 // StringMapValue is a map[string]string flag. It accepts a
 // comma-separated list of key value pairs, of the form key:value. The
 // keys cannot contain colons.
+//
+// TODO (andrew): Look into whether there's a native pflag Flag type that we can
+// use/transition to instead.
 type StringMapValue map[string]string
 
 // Set sets the value of this flag from parsing the given string.
@@ -126,6 +129,9 @@ func (value StringMapValue) String() string {
 	return strings.Join(parts, ",")
 }
 
+// Type is part of the pflag.Value interface.
+func (value StringMapValue) Type() string { return "StringMap" }
+
 // DualFormatStringListVar creates a flag which supports both dashes and underscores
 func DualFormatStringListVar(p *[]string, name string, value []string, usage string) {
 	dashes := strings.Replace(name, "_", "-", -1)

go/vt/mysqlctl/backupengine.go

@@ -72,7 +72,7 @@ type RestoreParams struct {
 	Cnf    *Mycnf
 	Mysqld MysqlDaemon
 	Logger logutil.Logger
-	// Concurrency is the value of -restore_concurrency flag (init restore parameter)
+	// Concurrency is the value of --restore_concurrency flag (init restore parameter)
 	// It determines how many files are processed in parallel
 	Concurrency int
 	// Extra env variables for pre-restore and post-restore transform hooks

go/vt/vttablet/tabletmanager/orchestrator.go

@@ -18,7 +18,6 @@ package tabletmanager
 
 import (
 	"encoding/json"
-	"flag"
 	"fmt"
 	"io"
 	"net/http"
@@ -27,22 +26,38 @@ import (
 	"strconv"
 	"time"
 
-	"vitess.io/vitess/go/vt/vterrors"
+	"github.com/spf13/pflag"
 
 	"vitess.io/vitess/go/timer"
 	"vitess.io/vitess/go/vt/log"
-	topodatapb "vitess.io/vitess/go/vt/proto/topodata"
+	"vitess.io/vitess/go/vt/servenv"
 	"vitess.io/vitess/go/vt/topo/topoproto"
+	"vitess.io/vitess/go/vt/vterrors"
+
+	topodatapb "vitess.io/vitess/go/vt/proto/topodata"
 )
 
 var (
-	orcAddr     = flag.String("orc_api_url", "", "Address of Orchestrator's HTTP API (e.g. http://host:port/api/). Leave empty to disable Orchestrator integration.")
-	orcUser     = flag.String("orc_api_user", "", "(Optional) Basic auth username to authenticate with Orchestrator's HTTP API. Leave empty to disable basic auth.")
-	orcPassword = flag.String("orc_api_password", "", "(Optional) Basic auth password to authenticate with Orchestrator's HTTP API.")
-	orcTimeout  = flag.Duration("orc_timeout", 30*time.Second, "Timeout for calls to Orchestrator's HTTP API")
-	orcInterval = flag.Duration("orc_discover_interval", 0, "How often to ping Orchestrator's HTTP API endpoint to tell it we exist. 0 means never.")
+	orcAddr     string
+	orcUser     string
+	orcPassword string
+	orcTimeout  = 30 * time.Second
+	orcInterval time.Duration
 )
 
+func init() {
+	servenv.OnParseFor("vtcombo", registerOrcFlags)
+	servenv.OnParseFor("vttablet", registerOrcFlags)
+}
+
+func registerOrcFlags(fs *pflag.FlagSet) {
+	fs.StringVar(&orcAddr, "orc_api_url", orcAddr, "Address of Orchestrator's HTTP API (e.g. http://host:port/api/). Leave empty to disable Orchestrator integration.")
+	fs.StringVar(&orcUser, "orc_api_user", orcUser, "(Optional) Basic auth username to authenticate with Orchestrator's HTTP API. Leave empty to disable basic auth.")
+	fs.StringVar(&orcPassword, "orc_api_password", orcPassword, "(Optional) Basic auth password to authenticate with Orchestrator's HTTP API.")
+	fs.DurationVar(&orcTimeout, "orc_timeout", orcTimeout, "Timeout for calls to Orchestrator's HTTP API.")
+	fs.DurationVar(&orcInterval, "orc_discover_interval", orcInterval, "How often to ping Orchestrator's HTTP API endpoint to tell it we exist. 0 means never.")
+}
+
 type orcClient struct {
 	apiRoot    *url.URL
 	httpClient *http.Client
@@ -51,32 +66,32 @@ type orcClient struct {
 // newOrcClient creates a client for the Orchestrator HTTP API.
 // It should only be called after flags have been parsed.
 func newOrcClient() (*orcClient, error) {
-	if *orcAddr == "" {
+	if orcAddr == "" {
 		// Orchestrator integration is disabled.
 		return nil, nil
 	}
-	apiRoot, err := url.Parse(*orcAddr)
+	apiRoot, err := url.Parse(orcAddr)
 	if err != nil {
-		return nil, vterrors.Wrapf(err, "can't parse -orc_api_url flag value (%v)", *orcAddr)
+		return nil, vterrors.Wrapf(err, "can't parse --orc_api_url flag value (%v)", orcAddr)
 	}
 	return &orcClient{
 		apiRoot:    apiRoot,
-		httpClient: &http.Client{Timeout: *orcTimeout},
+		httpClient: &http.Client{Timeout: orcTimeout},
 	}, nil
 }
 
 // DiscoverLoop periodically calls orc.discover() until process termination.
 // The Tablet is read from the given tm each time before calling discover().
 // Usually this will be launched as a background goroutine.
 func (orc *orcClient) DiscoverLoop(tm *TabletManager) {
-	if *orcInterval == 0 {
+	if orcInterval == 0 {
 		// 0 means never.
 		return
 	}
-	log.Infof("Starting periodic Orchestrator self-registration: API URL = %v, interval = %v", *orcAddr, *orcInterval)
+	log.Infof("Starting periodic Orchestrator self-registration: API URL = %v, interval = %v", orcAddr, orcInterval)
 
 	// Randomly vary the interval by +/- 25% to reduce the potential for spikes.
-	ticker := timer.NewRandTicker(*orcInterval, *orcInterval/4)
+	ticker := timer.NewRandTicker(orcInterval, orcInterval/4)
 
 	// Remember whether we've most recently succeeded or failed.
 	var lastErr error
@@ -189,8 +204,8 @@ func (orc *orcClient) apiGet(pathParts ...string) ([]byte, error) {
 	if err != nil {
 		return nil, err
 	}
-	if *orcUser != "" {
-		req.SetBasicAuth(*orcUser, *orcPassword)
+	if orcUser != "" {
+		req.SetBasicAuth(orcUser, orcPassword)
 	}
 	resp, err := orc.httpClient.Do(req)
 	if err != nil {