orc: more changes #6714

sougou · 2020-09-13T17:04:24Z

Since we're moving iteratively, the individual commits are not necessarily holistic. So, I'm combining everything done so far into one PR:

Added support for a a "durability" plugin. It allows you to specify who are the eligible masters, how many semi-sync acks the master should be configured to, and who can ack semi-syncs. The promotion rules now consult this module instead of config variables. This will evolve as we move forward.
Added more failure detections: MasterSemiSyncMustBeSet, MasterSemiSyncMustNotBeSet, ReplicaSemiSyncMustBeSet, ReplicaSemiSyncMustNotBeSet
Proactively change the tablet types in vitess for a smoother user experience.
LockShard has improved, but enough. More will come later.
Added support for Operator in orchestrator: initial support planetscale/vitess-operator#130. Made corresponding changes here.
Added operator to docker images: this required disabling of CGO, because of our new dependency with sqlite.
Deleted discovery through mysql topology. This is not needed because there is authoritative info coming from the vitess topo.
Disabled hostname resolutions: This was not working inside kubernetes. Again, the vitess topo provides authoritative info for hostnames alreay.
Removed CLI mode from orchestrator main. Now, the only mode is http. So, that argument is now unnecessary.
Added a new flag "orc_web_dir" in order to point orchestrator at its webdir to the right path in the vitess/lite container.
The default values for some configs have been changed, and will likely be removed from the user's control eventually. The hard-coded values are:
- "BackendDB": "sqlite"
- "SQLite3DataFile": ":memory:"
- "RecoverMasterClusterFilters": ["*"]
- "DelayMasterPromotionIfSQLThreadNotUpToDate": true
- "MySQLHostnameResolveMethod": "none"
Added some additional demo scripts/files in examples/local, and examples/operator.

shlomi-noach

change from :memory: to file::memory:?mode=memory&cache=shared, because :memory: will open a new snapshot of the database for any new sqlite connection. The only reason you didn't notice a problem is that the orchestrator code forces a single connection in the pool; that's because I've ran into deadlocks in the past that I couldn't solve. Ideally these can be solved sometime in the future, but then file::memory:?mode=memory&cache=shared must be used or ese everything breaks.
Do you still need help with CGO and Docker? I can look into that.

Removed CLI mode from orchestrator main. Now, the only mode is http. So, that argument is now unnecessary.

(iterating inline comment) CLI is used in integration tests; a but like vitess' endtoend tests run command line vtctl. In that sense it's useful to keep. Unless you convert all integrated tests to run orchestrator-client, which is some undertaking.

Disabled hostname resolutions: This was not working inside kubernetes. Again, the vitess topo provides authoritative info for hostnames alreay.

Is it possible, though, that when you SHOW PROCESSLIST or SHOW SLAVE STATUS on some user's MySQL server, the hostname from the master/replicas does not match the way Vitess thinks it should show? If that happens, then orchestrator will not build the topology view correctly. I'm concerned that we are unable to verify that, because who knows what setups different users may have.
I'd suggest restoring hostname resolutions to be on the safe side.

go/vt/orchestrator/config/config.go

shlomi-noach · 2020-09-14T06:25:05Z

Makefile

@@ -61,7 +61,7 @@ endif
 install: build
 	# binaries
 	mkdir -p "$${PREFIX}/bin"
-	cp "$${VTROOT}/bin/"{mysqlctld,vtctld,vtctlclient,vtgate,vttablet,vtworker,vtbackup} "$${PREFIX}/bin/"
+	cp "$${VTROOT}/bin/"{mysqlctld,orchestrator,vtctld,vtctlclient,vtgate,vttablet,vtworker,vtbackup} "$${PREFIX}/bin/"


shlomi-noach · 2020-09-14T06:25:30Z

docker/lite/Dockerfile.alpine

@@ -19,9 +19,6 @@
 # Use a temporary layer for the build stage.
 FROM vitess/bootstrap:mariadb103 AS builder

-# Allows some docker builds to disable CGO
-ARG CGO_ENABLED=0
-


shlomi-noach · 2020-09-14T06:25:59Z

docker/lite/Dockerfile.alpine

@@ -48,6 +45,7 @@ ENV MYSQL_FLAVOR MariaDB103

 # Copy artifacts from builder layer.
 COPY --from=builder --chown=vitess:vitess /vt/install /vt
+COPY --from=builder --chown=vitess:vitess /vt/src/vitess.io/vitess/web/orchestrator /vt/web/orchestrator


shlomi-noach · 2020-09-14T06:26:26Z

examples/local/orc_test.sh

@@ -0,0 +1,57 @@
+#!/bin/bash
+
+# Copyright 2019 The Vitess Authors.


Suggested change

# Copyright 2019 The Vitess Authors.

# Copyright 2020 The Vitess Authors.

shlomi-noach · 2020-09-14T06:45:16Z

go/vt/orchestrator/logic/orchestrator.go

-		if !inst.RegexpMatchPatterns(instance.MasterKey.StringCode(), config.Config.DiscoveryIgnoreMasterHostnameFilters) {
-			discoveryQueue.Push(instance.MasterKey)
-		}
-	}


oh wow 🤞 hope this works 😅

shlomi-noach · 2020-09-14T06:45:58Z

go/vt/orchestrator/logic/tablet_discovery.go

-	_, unlock, err := ts.LockShard(context.TODO(), tablet.Keyspace, tablet.Shard, "Orc Recovery")
+	ctx, cancel := context.WithTimeout(context.TODO(), 1*time.Second)
+	defer cancel()
+	_, unlock, err := ts.LockShard(ctx, tablet.Keyspace, tablet.Shard, "Orc Recovery")


shlomi-noach · 2020-09-14T06:46:27Z

go/vt/orchestrator/logic/tablet_discovery.go

 	} else {
-		err = tmc.UndoDemoteMaster(context.TODO(), tablet)
+		err = tmc.UndoDemoteMaster(ctx, tablet)


ah, nice! I like the Undo!

shlomi-noach · 2020-09-14T06:46:59Z

go/vt/orchestrator/logic/topology_recovery.go

@@ -1535,9 +1559,10 @@ func getCheckAndRecoverFunction(analysisCode inst.AnalysisCode, analyzedInstance
 		return electNewMaster, true
 	case inst.MasterHasMaster:
 		return fixClusterAndMaster, true
-	case inst.MasterIsReadOnly:
+	case inst.MasterIsReadOnly, inst.MasterSemiSyncMustBeSet, inst.MasterSemiSyncMustNotBeSet:


go/vt/orchestrator/logic/topology_recovery.go

sougou

I'll answer the main comments separately.

examples/local/scripts/ovttablet-up.sh

sougou · 2020-09-15T01:14:02Z

go/cmd/orchestrator/main.go

 	}

 	switch {
 	case helpTopic != "":
 		app.HelpCommand(helpTopic)
-	case len(flag.Args()) == 0 || flag.Arg(0) == "cli":
-		app.CliWrapper(*command, *strict, *instance, *destination, *owner, *reason, *duration, *pattern, *clusterAlias, *pool, *hostnameFlag)


We will have to convert. I'll explain in the main answer.

sougou · 2020-09-15T01:16:15Z

go/vt/orchestrator/app/cli.go

-				log.Fatale(err)
-			}
-			fmt.Println(instanceKey.DisplayString())
-		}


These were removed because they conflict with the new durability plugin. Even you set it here, the plugin will undo the change if it notices that the setting doesn't agree with its expectations.

sougou · 2020-09-15T01:19:17Z

go/vt/orchestrator/inst/tablet_dao.go

+		si.MasterTermStartTime = newMasterTablet.MasterTermStartTime
+		return nil
+	})
+	// Don't proceed if shard record could not be updated.


The comment applies to the error check and nil return. It says that we should not ignore this error. LMK if there is a better way to phrase it.

go/vt/orchestrator/logic/topology_recovery.go

sougou · 2020-09-15T01:36:27Z

change from :memory: to file::memory:?mode=memory&cache=shared, because :memory: will open a new snapshot of the database for any new sqlite connection. The only reason you didn't notice a problem is that the orchestrator code forces a single connection in the pool; that's because I've ran into deadlocks in the past that I couldn't solve. Ideally these can be solved sometime in the future, but then file::memory:?mode=memory&cache=shared must be used or ese everything breaks.

Done

Do you still need help with CGO and Docker? I can look into that.

CGO works now.

Removed CLI mode from orchestrator main. Now, the only mode is http. So, that argument is now unnecessary.

(iterating inline comment) CLI is used in integration tests; a but like vitess' endtoend tests run command line vtctl. In that sense it's useful to keep. Unless you convert all integrated tests to run orchestrator-client, which is some undertaking.

I had to remove the CLI because there was no easy way to change the operator framework to add Http as a non-flag argument. So, removing that argument cascaded into these dependencies. I'm not worried about losing CLI for tests because we've managed to test vitess without command line versions of the servers. vtctl continues to exist mainly because of inertia. It is deprecated. We just haven't gotten around to deleting it.

Disabled hostname resolutions: This was not working inside kubernetes. Again, the vitess topo provides authoritative info for hostnames alreay.

Is it possible, though, that when you SHOW PROCESSLIST or SHOW SLAVE STATUS on some user's MySQL server, the hostname from the master/replicas does not match the way Vitess thinks it should show? If that happens, then orchestrator will not build the topology view correctly. I'm concerned that we are unable to verify that, because who knows what setups different users may have.
I'd suggest restoring hostname resolutions to be on the safe side.

I wasn't too sure about this, which is why the core code is still there and we can resurrect it if we run into issues. But enabling hostname resolution doesn't work in kubernetes. Those pod names masquerading as hostnames was causing confusion. I'm hoping we won't have to do this because Vitess has some cloud-friendly way of resolving hostnames. So, blindly trusting the ones provided by vitess should just work. 🤞

shlomi-noach · 2020-09-15T06:12:45Z

blindly trusting the ones provided by vitess should just work. crossed_fingers

The issue is not that; I'm happy to trust the name resolving provided by vitess, that's fine. But, do we absolutely know for sure that if I SHOW SLAVE STATUS than the Master_host shows up in the same way Vitess resolves it? e.g. could I just see an IP address there? Or is this impossible because it's vitess who set up the replication in the first place?

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

sougou · 2020-09-16T04:15:24Z

blindly trusting the ones provided by vitess should just work. crossed_fingers

The issue is not that; I'm happy to trust the name resolving provided by vitess, that's fine. But, do we absolutely know for sure that if I SHOW SLAVE STATUS than the Master_host shows up in the same way Vitess resolves it? e.g. could I just see an IP address there? Or is this impossible because it's vitess who set up the replication in the first place?

This seems to work fine. I just brought up a cluster and looked at the replica info. They show up as IP in the UI also. I think it woks fine because we also use the same IP while wiring up the replicas using change master to.

sougou requested review from derekperkins and dkhenry as code owners September 13, 2020 17:04

sougou requested review from deepthi, shlomi-noach and harshit-gangal September 13, 2020 17:04

shlomi-noach requested changes Sep 14, 2020

View reviewed changes

sougou commented Sep 15, 2020

View reviewed changes

sougou added 7 commits September 15, 2020 13:48

orc: durability pluging initial cut

431687d

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

orc: durability plugin: monitor settings

cb926e9

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

orc: cross_cell durability

26c2dd3

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

orc: improved shard locking

0863c2e

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

orc: improved SwitchMaster

1059d46

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

orc: operator support

f2f7b26

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

orc: address review comments

93d7efe

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

sougou force-pushed the ss-oc5-operator branch from 752a5de to 93d7efe Compare September 15, 2020 20:51

shlomi-noach approved these changes Sep 16, 2020

View reviewed changes

sougou merged commit ba71ee6 into vitessio:master Sep 17, 2020

sougou deleted the ss-oc5-operator branch September 20, 2020 21:38

askdba added this to the v8.0 milestone Oct 6, 2020

frouioui mentioned this pull request Nov 3, 2020

Addition of the Orchestrator frouioui/tagenal#5

Open

setassociative mentioned this pull request Mar 5, 2021

Vitess v8.0 Release branch tinyspeck/vitess#194

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

orc: more changes #6714

orc: more changes #6714

sougou commented Sep 13, 2020

shlomi-noach left a comment •

edited

Loading

shlomi-noach Sep 14, 2020

shlomi-noach Sep 14, 2020

shlomi-noach Sep 14, 2020

shlomi-noach Sep 14, 2020

shlomi-noach Sep 14, 2020

shlomi-noach Sep 14, 2020

shlomi-noach Sep 14, 2020

shlomi-noach Sep 14, 2020

sougou left a comment

sougou Sep 15, 2020

sougou Sep 15, 2020

sougou Sep 15, 2020

sougou commented Sep 15, 2020

shlomi-noach commented Sep 15, 2020

sougou commented Sep 16, 2020

		@@ -0,0 +1,57 @@
		#!/bin/bash

		# Copyright 2019 The Vitess Authors.

	# Copyright 2019 The Vitess Authors.
	# Copyright 2020 The Vitess Authors.

orc: more changes #6714

orc: more changes #6714

Conversation

sougou commented Sep 13, 2020

shlomi-noach left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sougou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sougou commented Sep 15, 2020

shlomi-noach commented Sep 15, 2020

sougou commented Sep 16, 2020

shlomi-noach left a comment •

edited

Loading