[cli] [topo] Migrate topo flags to pflags (vitessio#11393)

* [cli] [topo] Migrate topo flags to pflags

Signed-off-by: Rameez Sajwani <[email protected]>

* fix vtorc test

Signed-off-by: Rameez Sajwani <[email protected]>

* feat: fix VTOrc test

Signed-off-by: Manan Gupta <[email protected]>

* feat: remove binaries from the list that don't need the flags

Signed-off-by: Manan Gupta <[email protected]>

* test: update flag help outputs

Signed-off-by: Manan Gupta <[email protected]>

* code review

Signed-off-by: Rameez Sajwani <[email protected]>

* fix import formatting

Signed-off-by: Rameez Sajwani <[email protected]>

Signed-off-by: Rameez Sajwani <[email protected]>
Signed-off-by: Manan Gupta <[email protected]>
Co-authored-by: Manan Gupta <[email protected]>

Author: rsajwani

go/cmd/vtctldclient/command/reparents.go

@@ -277,7 +277,7 @@ func commandTabletExternallyReparented(cmd *cobra.Command, args []string) error
 }
 
 func init() {
-	EmergencyReparentShard.Flags().DurationVar(&emergencyReparentShardOptions.WaitReplicasTimeout, "wait-replicas-timeout", *topo.RemoteOperationTimeout, "Time to wait for replicas to catch up in reparenting.")
+	EmergencyReparentShard.Flags().DurationVar(&emergencyReparentShardOptions.WaitReplicasTimeout, "wait-replicas-timeout", topo.RemoteOperationTimeout, "Time to wait for replicas to catch up in reparenting.")
 	EmergencyReparentShard.Flags().StringVar(&emergencyReparentShardOptions.NewPrimaryAliasStr, "new-primary", "", "Alias of a tablet that should be the new primary. If not specified, the vtctld will select the best candidate to promote.")
 	EmergencyReparentShard.Flags().BoolVar(&emergencyReparentShardOptions.PreventCrossCellPromotion, "prevent-cross-cell-promotion", false, "Only promotes a new primary from the same cell as the previous primary.")
 	EmergencyReparentShard.Flags().StringSliceVarP(&emergencyReparentShardOptions.IgnoreReplicaAliasStrList, "ignore-replicas", "i", nil, "Comma-separated, repeated list of replica tablet aliases to ignore during the emergency reparent.")
@@ -287,7 +287,7 @@ func init() {
 	InitShardPrimary.Flags().BoolVar(&initShardPrimaryOptions.Force, "force", false, "Force the reparent even if the provided tablet is not writable or the shard primary.")
 	Root.AddCommand(InitShardPrimary)
 
-	PlannedReparentShard.Flags().DurationVar(&plannedReparentShardOptions.WaitReplicasTimeout, "wait-replicas-timeout", *topo.RemoteOperationTimeout, "Time to wait for replicas to catch up on replication both before and after reparenting.")
+	PlannedReparentShard.Flags().DurationVar(&plannedReparentShardOptions.WaitReplicasTimeout, "wait-replicas-timeout", topo.RemoteOperationTimeout, "Time to wait for replicas to catch up on replication both before and after reparenting.")
 	PlannedReparentShard.Flags().StringVar(&plannedReparentShardOptions.NewPrimaryAliasStr, "new-primary", "", "Alias of a tablet that should be the new primary.")
 	PlannedReparentShard.Flags().StringVar(&plannedReparentShardOptions.AvoidPrimaryAliasStr, "avoid-primary", "", "Alias of a tablet that should not be the primary; i.e. \"reparent to any other tablet if this one is the primary\".")
 	Root.AddCommand(PlannedReparentShard)

go/flags/endtoend/mysqlctl.txt

@@ -103,17 +103,13 @@ Global flags:
       --port int                                                         port for the server
       --pprof strings                                                    enable profiling
       --purge_logs_interval duration                                     how often try to remove old logs (default 1h0m0s)
-      --remote_operation_timeout duration                                time to wait for a remote operation (default 30s)
       --replication_connect_retry duration                               how long to wait in between replica reconnect attempts. Only precise to the second. (default 10s)
       --security_policy string                                           the name of a registered security policy to use for controlling access to URLs - empty means allow all for anyone (built-in policies: deny-all, read-only)
       --service_map strings                                              comma separated list of services to enable (or disable if prefixed with '-') Example: grpc-queryservice
       --socket_file string                                               Local unix socket file to listen on
       --stderrthreshold severity                                         logs at or above this threshold go to stderr (default 1)
       --tablet_dir string                                                The directory within the vtdataroot to store vttablet/mysql files. Defaults to being generated by the tablet uid.
       --tablet_uid uint                                                  Tablet UID (default 41983)
-      --topo_global_root string                                          the path of the global topology data in the global topology server
-      --topo_global_server_address string                                the address of the global topology server
-      --topo_implementation string                                       the topology implementation to use
   -v, --v Level                                                          log level for V logs
       --version                                                          print binary version
       --vmodule moduleSpec                                               comma-separated list of pattern=N settings for file-filtered logging

go/flags/endtoend/mysqlctld.txt

@@ -91,17 +91,13 @@ Usage of mysqlctld:
       --port int                                                         port for the server
       --pprof strings                                                    enable profiling
       --purge_logs_interval duration                                     how often try to remove old logs (default 1h0m0s)
-      --remote_operation_timeout duration                                time to wait for a remote operation (default 30s)
       --replication_connect_retry duration                               how long to wait in between replica reconnect attempts. Only precise to the second. (default 10s)
       --security_policy string                                           the name of a registered security policy to use for controlling access to URLs - empty means allow all for anyone (built-in policies: deny-all, read-only)
       --service_map strings                                              comma separated list of services to enable (or disable if prefixed with '-') Example: grpc-queryservice
       --socket_file string                                               Local unix socket file to listen on
       --stderrthreshold severity                                         logs at or above this threshold go to stderr (default 1)
       --tablet_dir string                                                The directory within the vtdataroot to store vttablet/mysql files. Defaults to being generated by the tablet uid.
       --tablet_uid uint                                                  Tablet UID (default 41983)
-      --topo_global_root string                                          the path of the global topology data in the global topology server
-      --topo_global_server_address string                                the address of the global topology server
-      --topo_implementation string                                       the topology implementation to use
   -v, --v Level                                                          log level for V logs
       --version                                                          print binary version
       --vmodule moduleSpec                                               comma-separated list of pattern=N settings for file-filtered logging

go/flags/endtoend/vtctldclient.txt

@@ -108,7 +108,6 @@ Flags:
       --logtostderr                            log to standard error instead of files
       --mysql_server_version string            MySQL server version to advertise.
       --purge_logs_interval duration           how often try to remove old logs (default 1h0m0s)
-      --remote_operation_timeout duration      time to wait for a remote operation (default 30s)
       --security_policy string                 the name of a registered security policy to use for controlling access to URLs - empty means allow all for anyone (built-in policies: deny-all, read-only)
       --server string                          server to use for connection (required)
       --stats_backend string                   The name of the registered push-based monitoring/stats backend to use
@@ -117,9 +116,6 @@ Flags:
       --stats_drop_variables string            Variables to be dropped from the list of exported variables.
       --stats_emit_period duration             Interval between emitting stats to all registered backends (default 1m0s)
       --stderrthreshold severity               logs at or above this threshold go to stderr (default 1)
-      --topo_global_root string                the path of the global topology data in the global topology server
-      --topo_global_server_address string      the address of the global topology server
-      --topo_implementation string             the topology implementation to use
   -v, --v Level                                log level for V logs
       --version                                version for vtctldclient
       --vmodule moduleSpec                     comma-separated list of pattern=N settings for file-filtered logging

go/flags/endtoend/vttestserver.txt

@@ -88,7 +88,6 @@ Usage of vttestserver:
       --purge_logs_interval duration                                     how often try to remove old logs (default 1h0m0s)
       --queryserver-config-transaction-timeout float                     query server transaction timeout (in seconds), a transaction will be killed if it takes longer than this value
       --rdonly_count int                                                 Rdonly tablets per shard (default 1)
-      --remote_operation_timeout duration                                time to wait for a remote operation (default 30s)
       --replica_count int                                                Replica tablets per shard (includes primary) (default 2)
       --replication_connect_retry duration                               how long to wait in between replica reconnect attempts. Only precise to the second. (default 10s)
       --rng_seed int                                                     The random number generator seed to use when initializing with random data (see also --initialize_with_random_data). Multiple runs with the same seed will result with the same initial data. (default 123)
@@ -113,13 +112,6 @@ Usage of vttestserver:
       --topo_consul_lock_session_checks string                           List of checks for consul session. (default "serfHealth")
       --topo_consul_lock_session_ttl string                              TTL for consul session.
       --topo_consul_watch_poll_duration duration                         time of the long poll for watch queries. (default 30s)
-      --topo_etcd_lease_ttl int                                          Lease TTL for locks and leader election. The client will use KeepAlive to keep the lease going. (default 30)
-      --topo_etcd_tls_ca string                                          path to the ca to use to validate the server cert when connecting to the etcd topo server
-      --topo_etcd_tls_cert string                                        path to the client cert to use to connect to the etcd topo server, requires topo_etcd_tls_key, enables TLS
-      --topo_etcd_tls_key string                                         path to the client key to use to connect to the etcd topo server, enables TLS
-      --topo_global_root string                                          the path of the global topology data in the global topology server
-      --topo_global_server_address string                                the address of the global topology server
-      --topo_implementation string                                       the topology implementation to use
       --topo_zk_auth_file string                                         auth to use when connecting to the zk topo server, file contents should be <scheme>:<auth>, e.g., digest:user:pass
       --topo_zk_base_timeout duration                                    zk base timeout (see zk.Connect) (default 30s)
       --topo_zk_max_concurrency int                                      maximum number of pending requests to send to a Zookeeper server. (default 64)

go/test/endtoend/cluster/cluster_process.go

@@ -583,7 +583,6 @@ func (cluster *LocalProcessCluster) StartKeyspaceLegacy(keyspace Keyspace, shard
 // This does not start any process and user have to explicitly start all
 // the required services (ex topo, vtgate, mysql and vttablet)
 func (cluster *LocalProcessCluster) SetupCluster(keyspace *Keyspace, shards []Shard) (err error) {
-
 	log.Infof("Starting keyspace: %v", keyspace.Name)
 
 	if !cluster.ReusingVTDATAROOT {
@@ -598,7 +597,6 @@ func (cluster *LocalProcessCluster) SetupCluster(keyspace *Keyspace, shards []Sh
 	// Create shard
 	for _, shard := range shards {
 		for _, tablet := range shard.Vttablets {
-
 			// Setup MysqlctlProcess
 			tablet.MysqlctlProcess = *MysqlCtlProcessInstance(tablet.TabletUID, tablet.MySQLPort, cluster.TmpDirectory)
 			// Setup VttabletProcess

go/test/endtoend/vtorc/readtopologyinstance/main_test.go

@@ -17,20 +17,20 @@ limitations under the License.
 package readtopologyinstance
 
 import (
-	"flag"
 	"fmt"
+	"os"
 	"testing"
 	"time"
 
 	"vitess.io/vitess/go/test/endtoend/cluster"
 	"vitess.io/vitess/go/test/endtoend/vtorc/utils"
+	"vitess.io/vitess/go/vt/servenv"
 	"vitess.io/vitess/go/vt/vtorc/config"
 	"vitess.io/vitess/go/vt/vtorc/inst"
 	"vitess.io/vitess/go/vt/vtorc/server"
 
 	_ "github.com/go-sql-driver/mysql"
 	_ "github.com/mattn/go-sqlite3"
-
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -42,13 +42,19 @@ func TestReadTopologyInstanceBufferable(t *testing.T) {
 	}()
 	keyspace := &clusterInfo.ClusterInstance.Keyspaces[0]
 	shard0 := &keyspace.Shards[0]
+	oldArgs := os.Args
+	defer func() {
+		// Restore the old args after the test
+		os.Args = oldArgs
+	}()
 
-	err := flag.Set("topo_global_server_address", clusterInfo.ClusterInstance.VtctlProcess.TopoGlobalAddress)
-	require.NoError(t, err)
-	err = flag.Set("topo_implementation", clusterInfo.ClusterInstance.VtctlProcess.TopoImplementation)
-	require.NoError(t, err)
-	err = flag.Set("topo_global_root", clusterInfo.ClusterInstance.VtctlProcess.TopoGlobalRoot)
-	require.NoError(t, err)
+	// Change the args such that they match how we would invoke VTOrc
+	os.Args = []string{"vtorc",
+		"--topo_global_server_address", clusterInfo.ClusterInstance.VtctlProcess.TopoGlobalAddress,
+		"--topo_implementation", clusterInfo.ClusterInstance.VtctlProcess.TopoImplementation,
+		"--topo_global_root", clusterInfo.ClusterInstance.VtctlProcess.TopoGlobalRoot,
+	}
+	servenv.ParseFlags("vtorc")
 	config.Config.RecoveryPeriodBlockSeconds = 1
 	config.Config.InstancePollSeconds = 1
 	config.MarkConfigurationLoaded()

go/vt/discovery/tablet_picker.go

@@ -172,7 +172,7 @@ func (tp *TabletPicker) GetMatchingTablets(ctx context.Context) []*topo.TabletIn
 	// Since there is only one primary, we ignore cell and find the primary
 	aliases := make([]*topodatapb.TabletAlias, 0)
 	if len(tp.tabletTypes) == 1 && tp.tabletTypes[0] == topodatapb.TabletType_PRIMARY {
-		shortCtx, cancel := context.WithTimeout(ctx, *topo.RemoteOperationTimeout)
+		shortCtx, cancel := context.WithTimeout(ctx, topo.RemoteOperationTimeout)
 		defer cancel()
 		si, err := tp.ts.GetShard(shortCtx, tp.keyspace, tp.shard)
 		if err != nil {
@@ -185,12 +185,12 @@ func (tp *TabletPicker) GetMatchingTablets(ctx context.Context) []*topo.TabletIn
 		for _, cell := range tp.cells {
 			// check if cell is actually an alias
 			// non-blocking read so that this is fast
-			shortCtx, cancel := context.WithTimeout(ctx, *topo.RemoteOperationTimeout)
+			shortCtx, cancel := context.WithTimeout(ctx, topo.RemoteOperationTimeout)
 			defer cancel()
 			_, err := tp.ts.GetCellInfo(shortCtx, cell, false)
 			if err != nil {
 				// not a valid cell, check whether it is a cell alias
-				shortCtx, cancel := context.WithTimeout(ctx, *topo.RemoteOperationTimeout)
+				shortCtx, cancel := context.WithTimeout(ctx, topo.RemoteOperationTimeout)
 				defer cancel()
 				alias, err := tp.ts.GetCellsAlias(shortCtx, cell, false)
 				// if we get an error, either cellAlias doesn't exist or it isn't a cell alias at all. Ignore and continue
@@ -205,7 +205,7 @@ func (tp *TabletPicker) GetMatchingTablets(ctx context.Context) []*topo.TabletIn
 			}
 		}
 		for _, cell := range actualCells {
-			shortCtx, cancel := context.WithTimeout(ctx, *topo.RemoteOperationTimeout)
+			shortCtx, cancel := context.WithTimeout(ctx, topo.RemoteOperationTimeout)
 			defer cancel()
 			// match cell, keyspace and shard
 			sri, err := tp.ts.GetShardReplication(shortCtx, cell, tp.keyspace, tp.shard)
@@ -222,7 +222,7 @@ func (tp *TabletPicker) GetMatchingTablets(ctx context.Context) []*topo.TabletIn
 	if len(aliases) == 0 {
 		return nil
 	}
-	shortCtx, cancel := context.WithTimeout(ctx, *topo.RemoteOperationTimeout)
+	shortCtx, cancel := context.WithTimeout(ctx, topo.RemoteOperationTimeout)
 	defer cancel()
 	tabletMap, err := tp.ts.GetTabletMap(shortCtx, aliases)
 	if err != nil {

go/vt/mysqlctl/builtinbackupengine.go

@@ -269,7 +269,7 @@ func (be *BuiltinBackupEngine) ExecuteBackup(ctx context.Context, params BackupP
 		// the saved replicationPosition before proceeding
 		tmc := tmclient.NewTabletManagerClient()
 		defer tmc.Close()
-		remoteCtx, remoteCancel := context.WithTimeout(ctx, *topo.RemoteOperationTimeout)
+		remoteCtx, remoteCancel := context.WithTimeout(ctx, topo.RemoteOperationTimeout)
 		defer remoteCancel()
 
 		pos, err := getPrimaryPosition(remoteCtx, tmc, params.TopoServer, params.Keyspace, params.Shard)

go/vt/schemamanager/tablet_executor.go

@@ -360,7 +360,7 @@ func (exec *TabletExecutor) Execute(ctx context.Context, sqls []string) *Execute
 	}
 	providedUUID := ""
 
-	rl := timer.NewRateLimiter(*topo.RemoteOperationTimeout / 4)
+	rl := timer.NewRateLimiter(topo.RemoteOperationTimeout / 4)
 	defer rl.Stop()
 
 	syncOperationExecuted := false

go/vt/topo/cell_info.go

@@ -163,7 +163,7 @@ func (ts *Server) DeleteCellInfo(ctx context.Context, cell string, force bool) e
 			// local-down-topo scenario would mean we never can delete it.
 			// (see https://github.com/vitessio/vitess/issues/8220).
 			var cancel context.CancelFunc
-			ctx, cancel = context.WithTimeout(context.Background(), *RemoteOperationTimeout)
+			ctx, cancel = context.WithTimeout(context.Background(), RemoteOperationTimeout)
 			defer cancel()
 		default:
 			// Context still has some time left, no need to make a new one.
@@ -207,13 +207,13 @@ func (ts *Server) ExpandCells(ctx context.Context, cells string) ([]string, erro
 	}
 
 	expandCell := func(ctx context.Context, cell string) error {
-		shortCtx, cancel := context.WithTimeout(ctx, *RemoteOperationTimeout)
+		shortCtx, cancel := context.WithTimeout(ctx, RemoteOperationTimeout)
 		defer cancel()
 
 		_, err := ts.GetCellInfo(shortCtx, cell, false /* strongRead */)
 		if err != nil {
 			// Not a valid cell name. Check whether it is an alias.
-			shortCtx, cancel := context.WithTimeout(ctx, *RemoteOperationTimeout)
+			shortCtx, cancel := context.WithTimeout(ctx, RemoteOperationTimeout)
 			defer cancel()
 
 			alias, err2 := ts.GetCellsAlias(shortCtx, cell, false /* strongRead */)

go/vt/topo/consultopo/watch.go

@@ -48,7 +48,7 @@ func (s *Server) Watch(ctx context.Context, filePath string) (*topo.WatchData, <
 	nodePath := path.Join(s.root, filePath)
 	options := &api.QueryOptions{}
 
-	initialCtx, initialCancel := context.WithTimeout(ctx, *topo.RemoteOperationTimeout)
+	initialCtx, initialCancel := context.WithTimeout(ctx, topo.RemoteOperationTimeout)
 	defer initialCancel()
 
 	pair, _, err := s.kv.Get(nodePath, options.WithContext(initialCtx))

go/vt/topo/etcd2topo/lock.go

@@ -23,13 +23,12 @@ import (
 
 	"github.com/spf13/pflag"
 
-	"vitess.io/vitess/go/vt/servenv"
-
 	"go.etcd.io/etcd/api/v3/mvccpb"
 	clientv3 "go.etcd.io/etcd/client/v3"
 
 	"vitess.io/vitess/go/vt/log"
 	"vitess.io/vitess/go/vt/proto/vtrpc"
+	"vitess.io/vitess/go/vt/servenv"
 	"vitess.io/vitess/go/vt/topo"
 	"vitess.io/vitess/go/vt/vterrors"
 )
@@ -39,8 +38,7 @@ var (
 )
 
 func init() {
-	for _, cmd := range []string{"vttablet", "vtctl", "vtctld", "mysqlctl", "mysqlctld", "vttestserver", "vtcombo", "vtctldclient", "vtexplain", "vtgate",
-		"vtgr", "vtorc", "vtbackup"} {
+	for _, cmd := range topo.FlagBinaries {
 		servenv.OnParseFor(cmd, registerEtcd2TopoLockFlags)
 	}
 }

go/vt/topo/etcd2topo/server.go

@@ -40,14 +40,12 @@ import (
 	"time"
 
 	"github.com/spf13/pflag"
-
-	"vitess.io/vitess/go/vt/servenv"
-
 	"go.etcd.io/etcd/client/pkg/v3/tlsutil"
 	"google.golang.org/grpc"
 
 	clientv3 "go.etcd.io/etcd/client/v3"
 
+	"vitess.io/vitess/go/vt/servenv"
 	"vitess.io/vitess/go/vt/topo"
 )
 
@@ -82,8 +80,7 @@ type Server struct {
 }
 
 func init() {
-	for _, cmd := range []string{"vttablet", "vtctl", "vtctld", "mysqlctl", "mysqlctld", "vttestserver", "vtcombo", "vtctldclient", "vtexplain", "vtgate",
-		"vtgr", "vtorc", "vtbackup"} {
+	for _, cmd := range topo.FlagBinaries {
 		servenv.OnParseFor(cmd, registerEtcd2TopoFlags)
 	}
 	topo.RegisterFactory("etcd2", Factory{})