etcd3 #286

knisbet · 2018-05-07T14:36:43Z

Issue: https://github.com/gravitational/gravity/issues/3121

This is the planet logic to implemented etcd upgrades. Let's chat about the approach before you spend too much time looking through the PR's.

as per etcd-io/etcd#5322

a-palchikov

I've not yet completed the review - only skimmed over the first part of changes. I'll continue tomorrow.

a-palchikov · 2018-05-07T17:29:56Z

Makefile

+# This list needs to include every version of etcd that we can upgrade from + latest
+ETCD_VER := v2.3.8 v3.3.4
+# This is the version of etcd we should upgrade to (from the version list)
+ETCD_LATEST_VER := v3.3.4


I'd swap the variable definitions to make it less error-prone:

# This is the version of etcd we should upgrade to (from the version list) ETCD_LATEST_VER := v3.3.4 # ETCD Versions to include in the release # This list needs to include every version of etcd that we can upgrade from + latest ETCD_VER := v2.3.8 $(ETCD_LATEST_VER)

I'm not sure I would embed $(ETCD_LATEST_VER) in ETCD_VER, to me it adds some amount of risks, that if someone say bumps ETCD to v3.3.5, that they may forget to add it to the ETCD_VER list, everything would work but upgrades from that version may not.

The way it is now, if someone bumps ETCD_LATEST_VER to v3.3.5, but forgets to update ETCD_VER, the new release will clearly not work.

a-palchikov · 2018-05-07T17:31:26Z

build.assets/docker/buildbox.dockerfile

@@ -3,7 +3,7 @@ FROM planet/base
 ENV GOPATH /gopath
 ENV GOROOT /opt/go
 ENV PATH $PATH:$GOPATH/bin:$GOROOT/bin
-ENV GOVERSION 1.8.3
+ENV GOVERSION 1.10.1


do you need go1.10 for anything specifically or is it just a timely update?

Also, do you know if go1.10 is an umbrella binary for 1.10.x or it is the 1.10.0? If they could release from release-branch.go1.10 as capturing the latest patch (i.e. 1.10.2 atm) but not sure if they actually do that.

Etcd requires go 1.9, but I bumped to 1.10 since I was updating the version anyways: etcd-io/etcd#8548 (comment)

As for an umbrella binary, I don't know, I was sticking with the full version, as I suspect that's less likely for an upstream change to affect us.

a-palchikov · 2018-05-07T18:48:12Z

build.assets/makefiles/etcd/etcd.mk

+	ln -sf /dev/null $(ROOTFS)/etc/systemd/system/etcd-upgrade.service
+
+	# Write to the release file to indicate the latest release
+	echo PLANET_ETCD_VERSION=$(ETCD_LATEST_VER) >> $(ROOTFS)/etc/planet-release


Is appending to planet-release intentional?

Yes, right now it's only for etcd, but strictly speaking, if we were to embed version information for other dependencies, we could also include additional version information.

a-palchikov · 2018-05-07T18:48:48Z

build.assets/makefiles/etcd/etcd.mk

+
+.PHONY: $(ETCD_VER)
+$(ETCD_VER):
+	@echo "\n---> $@ - Downloading etcd\n"


echo -e to properly interpret escape sequences

Are you sure that applies to Makefile builtin @echo?

I don't think that's a builtin and what make does is it runs the echo command with the shell. The shell is /bin/sh by default but can be overridden with the SHELL envvar.
Now that I'm thinking about it, a better solution would be, for instance, to default SHELL to /bin/bash and use -e to avoid inconsistencies across environments:

With this makefile as an example:

.PHONY: all all: @echo "\n --> I'm a bad boy\n" @echo -e "\n --> I'm a good boy\n"

you get either:

10:18 $ make --> I'm a bad boy -e --> I'm a good boy

or

10:18 $ make \n --> I'm a bad boy\n --> I'm a good boy

(the last being also the result of using /bin/bash as a shell since bash only interprets escape sequence provided -e).
The results are different when I make locally or on jenkins (on jenkins, /bin/sh has been symlinked to /bin/bash)

a-palchikov · 2018-05-07T18:51:45Z

tool/planet/cfg.go

@@ -70,6 +70,8 @@ type Config struct {
 	// EtcdInitialCluster configures the value of ETCD_INITIAL_CLUSTER environment variable
 	// inside the container
 	EtcdInitialCluster string
+	//EtcdGatewayList is a list of etcd endpoints that the etcd gateway can use to reach the cluster


// EtcdGatewayList (a whitespace between commant and the sentence start)

a-palchikov · 2018-05-07T19:55:42Z

tool/planet/etcd.go

+	return nil
+}
+
+// etcdDisable disabled etcd on this machine


a-palchikov · 2018-05-07T19:59:13Z

tool/planet/etcd.go

+
+// etcdEnable enables a disabled etcd node
+func etcdEnable(upgradeService bool) error {
+	if upgradeService {


better to test for reversed condition to keep code outdented:

func etcdEnable(upgradeService bool) error { if !upgradeService { return trace.Wrap(enableService(ETCDServiceName)) } // don't actually enable the service if this is a proxy env, err := box.ReadEnvironment(ContainerEnvironmentFile) if err != nil { return trace.Wrap(err) } if env.Get(EnvEtcdProxy) == EtcdProxyOn { log.Infof("etcd is in proxy mode, nothing to do") return nil } return trace.Wrap(enableService(ETCDUpgradeServiceName)) }

a-palchikov · 2018-05-07T20:00:30Z

tool/planet/etcd.go

+	}
+
+	if env.Get(EnvEtcdProxy) == EtcdProxyOn {
+		log.Infof("etcd is in proxy mode, nothing to do")


no need for formatting version Infof w/o placeholders

a-palchikov · 2018-05-07T20:07:15Z

tool/planet/etcd.go

+	// this only happens if the service is already running
+	status, err := getServiceStatus(APIServerServiceName)
+	if err != nil {
+		return trace.Wrap(err)


Do you want to fail the upgrade if you failed to query the status of the API server - it does not look like it's critical to the upgrade operation?
Otherwise you might need to undo a few things to be able to retry (though I'm not sure yet this is possible).

a-palchikov · 2018-05-07T20:08:22Z

tool/planet/etcd.go

+}
+
+func etcdRestore(file string) error {
+	ctx := context.TODO()


same here: I'd accept the context from the outside.

So I'm not completely sure I agree with you here in this specific case. etcdBackup / etcdRestore come from the dispatch from main based on the kingpin function, where we may not want to use the same context based on which cli command we're in.

However, this comes more from the philosophy of cobra, where the library does the dispatch for you, and the function for the command is really the entrypoint. Example: https://github.com/gravitational/etcd-backup/blob/master/cmd/etcd-backup/backup.go

r0mant · 2018-05-07T20:15:33Z

build.assets/makefiles/etcd/etcd-gateway.dropin

+# instead of the etcd service
+
+[Service]
+ExecStart=


Extra ExecStart?

This is to reset the previous value (here is a faint hint at resetting the values of a list attribute before setting a new one - look at the end of the document).

r0mant · 2018-05-08T02:51:33Z

tool/planet/etcd.go

+func readEtcdVersion(path string) (string, error) {
+	inFile, err := os.Open(path)
+	if err != nil {
+		return "", trace.Wrap(err)


trace.ConvertSystemError

r0mant · 2018-05-08T02:52:11Z

tool/planet/etcd.go

+func writeEtcdEnvironment(path string, version string) error {
+	err := os.MkdirAll(filepath.Dir(path), 644)
+	if err != nil {
+		return trace.Wrap(err)


trace.ConvertSystemError

r0mant · 2018-05-08T02:52:22Z

tool/planet/etcd.go

+
+	f, err := os.Create(path)
+	if err != nil {
+		return trace.Wrap(err)


trace.ConvertSystemError

r0mant · 2018-05-08T02:52:35Z

tool/planet/etcd.go

+}
+
+func writeEtcdEnvironment(path string, version string) error {
+	err := os.MkdirAll(filepath.Dir(path), 644)


Use a constant for 644?

r0mant · 2018-05-08T03:07:50Z

tool/planet/etcd.go

+	out, err := exec.CommandContext(ctx, "/bin/systemctl", "--no-block", operation, service).CombinedOutput()
+	log.Infof("%v %v: %v", operation, service, string(out))
+	if err != nil {
+		return trace.Wrap(err, fmt.Sprintf("failed to %v %v: %v", operation, service, string(out)))


You don't need fmt.Sprintf here.

r0mant · 2018-05-08T03:22:26Z

tool/planet/etcd.go

+		from = DefaultEtcdStoreBackup
+		to = DefaultEtcdStoreCurrent
+	}
+	err = os.RemoveAll(to)


That's dangerous. I think this logic needs a better protection against accidental screw-ups - otherwise if "upgrade" somehow runs twice without "rollback", it will nuke the only backup on the second run.

Maybe it can append some ID (e.g. upgrade operation ID) to the backup dir to make it per-operation unique and instead of happily deleting it, refuse to proceed if it's not empty (thus, mandating rollback first).

Hmm, if you run the step twice, it will early exit before this is hit, due to CurrentVersion == Desired version.

However, if it fails part way through the step, I think the filesystem could be inconsistent that would allow it to continue and delete the data. I'm not sure appending an ID really helps here, I think it would be susceptible to similar problems or would just leave a bunch of data lying around (which some customers are having trouble with as is). That said, I think it should be possible to make this code re-entrant, so that if it's run multiple times it will skip steps already completed and continue where it left off.

r0mant · 2018-05-08T03:24:09Z

tool/planet/etcd.go

+	// symlink /usr/bin/etcd to the version we expect to be running
+	for _, path := range []string{"/usr/bin/etcd", "/usr/bin/etcdctl"} {
+		// ignore the error from os.Remove, since we don't care if it fails
+		_ = os.Remove(path)


Get rid of _ = then?

I thought using _ = was a pattern for telling linters (and other devs) I'm intentionally swallowing the error.

r0mant · 2018-05-08T03:35:41Z

tool/planet/etcd.go

+	if _, err := os.Stat(path); err == nil {
+		return readEtcdVersion(path)
+	}
+	return AssumeEtcdVersion, nil


I'm looking at how this function is used and TBH the fact that it falls back to this "assumed" version makes the logic a bit harder to follow (for example, in etcdInit()). Also, IIUC it will still return 2.3.8 initially for new installs even though they're running 3.x? I would just have it return NotFound and handle on the caller side.

r0mant · 2018-05-08T03:37:08Z

tool/planet/etcd.go

+	}
+	log.Info("Desired etcd version: ", desiredVersion)
+
+	if currentVersion == AssumeEtcdVersion {


This is what I was talking about in the currentEtcdVersion() method comment (see somewhere below). It returned 2.3.8 while in fact it's not.

a-palchikov · 2018-05-08T08:31:07Z

tool/planet/etcd_test.go

+func (*EtcdSuite) TestEtcdAssumedVersion(c *check.C) {
+	ver, err := currentEtcdVersion("/this/file/doesnt/exist")
+	if err != nil {
+		c.Error(err)


This looks like a longer version of:

c.Assert(err, IsNil)

a-palchikov · 2018-05-08T08:32:51Z

tool/planet/etcd_test.go

+func (*EtcdSuite) TestEtcdParseFile(c *check.C) {
+	file, err := ioutil.TempFile(os.TempDir(), "prefix")
+	if err != nil {
+		c.Fatal(err)


c.Assert(err, IsNil)? This will also mark the test as failed.

a-palchikov · 2018-05-08T08:34:14Z

tool/planet/etcd_test.go

+
+	// reading an empty file should return an error
+	ver, err := currentEtcdVersion(file.Name())
+	c.Assert(err, check.NotNil)


Check for specific error expectation (type or a message).

a-palchikov · 2018-05-08T08:34:47Z

tool/planet/etcd_test.go

+	// reading a missing file should return an error
+	ver, err = readEtcdVersion("/this/file/doesnt/exist")
+	c.Assert(err, check.NotNil)
+	c.Assert(ver, check.Equals, "")


Not sure if it's worth testing.

a-palchikov · 2018-05-08T08:35:15Z

tool/planet/etcd_test.go

+
+	// write a version file then check it
+	version := "1.1.1"
+	file.WriteString(fmt.Sprintf("%v=%v", EnvEtcdVersion, version))


should not ignore errors here.
There's also a helper in fmt:

fmt.Fprintf(file, "%v=%v", EnvEtcdVersion, version)

a-palchikov · 2018-05-08T08:37:56Z

tool/planet/main.go

@@ -161,6 +161,23 @@ func run() error {
 		cetcdPromoteInitialCluster      = cetcdPromote.Flag("initial-cluster", "Initial cluster, as output by 'member add' command").Required().String()
 		cetcdPromoteInitialClusterState = cetcdPromote.Flag("initial-cluster-state", "Initial cluster state, as output by 'member add' command").Required().String()

+		cetcdInit = cetcd.Command("init", "setup etcd to run the correct version").Hidden()


Setup etcd... - Capitalized as in other descriptions.

a-palchikov · 2018-05-08T08:39:13Z

tool/planet/main.go

+		cetcdEnable        = cetcd.Command("enable", "Enable etcd on this node")
+		cetcdEnableUpgrade = cetcdEnable.Flag("upgrade", "enable the upgrade service").Bool()
+
+		cetcdUpgrade  = cetcd.Command("upgrade", "Upgrade etcd to latest available in this container")


... latest version ...

a-palchikov · 2018-05-08T08:41:09Z

tool/planet/main.go

+		cetcdRollback = cetcd.Command("rollback", "Rollback etcd to the previous release")
+
+		cetcdRestore     = cetcd.Command("restore", "Restore etcd backup as part of the upgrade")
+		cetcdRestoreFile = cetcdRestore.Arg("file", "A previously taken backup file to use during upgrade").Required().String()


You could use kingpin's ExistingFile mixin to enfocre this:

cetcdRestoreFile = cetcdRestore.Arg("file", "A previously taken backup file to use during upgrade").Required().ExistingFile()

a-palchikov · 2018-05-08T08:43:20Z

tool/planet/start.go

@@ -430,6 +437,35 @@ func addEtcdOptions(config *Config) {
 	}
 }

+// setupEtcd runs setup tasks for etcd


nit: Use dots to end sentences.

a-palchikov · 2018-05-08T08:47:56Z

tool/planet/start.go

+			return trace.Wrap(err)
+		}
+	} else {
+		err := os.Remove(dropinPath)


Test for etcdproxy != "on" instead and don't use the else block as it is redundant:

func setupEtcd(config *Config) error { dropinPath := path.Join(config.Rootfs, ETCDGatewayDropinPath) if strings.ToLower(config.EtcdProxy) != "on" { err := os.Remove(dropinPath) if err != nil && !os.IsNotExist(err) { return trace.Wrap(err) } return nil } err := os.MkdirAll(path.Join(config.Rootfs, "etc/systemd/system/etcd.service.d/"), 0755) if err != nil { return trace.Wrap(err) } err = os.Symlink( "/lib/systemd/system/etcd-gateway.dropin", dropinPath, ) if err != nil && !os.IsExist(err) { return trace.Wrap(err) } return nil }

…e of container

r0mant · 2018-05-23T17:41:19Z

tool/planet/etcd.go

+	for _, service := range services {
+		status, err := getServiceStatus(service)
+		if err != nil {
+			log.Warnf("Failed to query status of service %v. Continuing upgrade. Error: %v", service, err)


Is this expected scenario? When a service status can't be queried?

r0mant · 2018-05-23T17:42:41Z

tool/planet/etcd.go

+			}
+		}
+	} else {
+		// in order to upgrade, write the new version to to disk with the current version as backup


"to to" -> "to"

r0mant · 2018-05-23T17:48:36Z

tool/planet/etcd.go

+			if err != nil {
+				return trace.Wrap(err)
+			}
+		}


Don't we also need to clean up data that can possibly be left after an upgrade attempt on rollback?

This happens on the next upgrade attempt:

planet/tool/planet/etcd.go

Lines 315 to 320 in 182dff9

// wipe data directory of any previous upgrade attempt

path := path.Join(getBaseEtcdDir(desiredVersion), "member")

err = os.RemoveAll(path)

if err != nil && !os.IsNotExist(err) {

return trace.ConvertSystemError(err)

}

r0mant · 2018-05-23T17:51:26Z

tool/planet/etcd.go

+		// wipe data directory of any previous upgrade attempt
+		path := path.Join(getBaseEtcdDir(desiredVersion), "member")
+		err = os.RemoveAll(path)
+		if err != nil && !os.IsNotExist(err) {


IsNotExist check is redundant. From the docs: If the path does not exist, RemoveAll returns nil (no error)..

r0mant · 2018-05-23T18:03:11Z

tool/planet/etcd.go

+
+			// wipe old backups leftover from previous upgrades
+			// Note: if this fails, but previous steps were successfull, the backups won't get cleaned up
+			if backupVersion != "" {


So this is a version of a previous backup, not backup for this upgrade?

that's correct.

r0mant · 2018-05-23T18:17:10Z

tool/planet/etcd.go

+}
+
+func writeEtcdEnvironment(path string, version string, prevVersion string) error {
+	err := os.MkdirAll(filepath.Dir(path), 644)


644 -> constants.SharedReadMask

r0mant · 2018-05-23T18:20:25Z

tool/planet/etcd.go

+// the problem is, when shutting down etcd, systemd will respond when the process has been told to shutdown
+// but this leaves a race, where we might be continuing while etcd is still cleanly shutting down
+func waitForEtcdStopped(ctx context.Context) error {
+	tick := time.Tick(WaitInterval)


Use ticker := time.NewTicker instead and defer ticker.Close() it. From the docs:

While Tick is useful for clients that have no need to shut down the Ticker, be aware that without a way to shut it down the underlying Ticker cannot be recovered by the garbage collector; it "leaks".

r0mant · 2018-05-23T18:23:29Z

tool/planet/etcd.go

+}
+
+func etcdRestore(file string) error {
+	ctx := context.TODO()


I would have all etcd* functions accept context as a parameter, even if it'd be passed as context.TODO() from main.go for now. This way it will be much easier to, for example, add a "--timeout" flag to all these commands so client can control it. Right now these internal context are pretty much useless.

r0mant · 2018-05-23T18:24:51Z

tool/planet/etcd.go

 	}

 	log.Infof("removing %v", ETCDProxyDir)
-	if err := os.RemoveAll(ETCDProxyDir); err != nil {
+	if err := os.RemoveAll(ETCDProxyDir); err != nil && !os.IsNotExist(err) {


IsNotExist check is redundant.

r0mant · 2018-05-23T18:26:29Z

tool/planet/etcd.go

+			CAFile:    DefaultEtcdctlCAFile,
+		},
+		Prefix:        []string{"/"},         // Restore all etcd data
+		MigratePrefix: []string{"/registry"}, // migrate kubernetes data to etcd3 datastore


There's a constant for /registry as far as I can see.

Kevin Nisbet added 30 commits April 10, 2018 15:43

port etcd3 branch to latest master

bca28fa

dep ensure

42bc200

dep update etcd, grpc, and use go 1.10.1 for builds

02f65e1

use systemctl instead of dbus which appears to be unreliable

5925c83

use systemctl instead of dbus which appears to be unreliable

ecbd282

ignore error when trying to restart service

f8b1e69

ignore error when trying to restart service

372b7d3

have etcd-upgrade service closer to full etcd configuration

c6d664c

re-organize upgrade steps to make it easier to call from gravity

0052cd4

have wait-for-etcd.sh optionally take a parameter for the etcd url

95721e4

run etcd based on symlink and not env variable

1aa0668

run execstartpre as root

b900c10

Set systemctl to not block on operations

d7ad830

use TLS when connecting to the upgrade service

0b805d1

have apiserver select backend based on variable

473d527

use systemd-dropin for setting up etcd gateway during upgrade

515ce4b

actually add etcd gateway

dd30793

etcd gateway can only bind to a single port

b8a1134

consistently use port 2379 for etcd

2f5fa44

as per etcd-io/etcd#5322

set execute bit on etcdctl3 script

62c9592

unify master/proxy upgrade process, as proxy doesn't need upgrade

a722b5c

Don't start etcd-upgrade service on proxy nodes

83e86a0

Use latest etcd on first run

5115a8b

create etcd directory if it doesn't exist

4c553a8

create etcd directory if it doesn't exist

738582b

Use planet to initialize etcd version

ee01273

only write dropin symlink if it doesn't already exist

a84ea72

fix symlink for systemd droping on repeat starts

6bfe435

store version state on persisteant etcd volume

8fa291f

update etcd environment variables

5262773

Kevin Nisbet added 3 commits May 4, 2018 16:47

restore --no-block behaviour on systemctl commands

092d61f

Merge remote-tracking branch 'origin/master' into kevin/etcd3-2

7cdb6b1

write etcd latest version to orbit manifest

713d441

knisbet requested review from r0mant and a-palchikov May 7, 2018 14:36

Kevin Nisbet added 3 commits May 7, 2018 10:46

cleanup

8c80eef

debug planet build error

925dc17

revert makefil debug

bec3770

a-palchikov reviewed May 7, 2018

View reviewed changes

r0mant reviewed May 8, 2018

View reviewed changes

a-palchikov reviewed May 8, 2018

View reviewed changes

Kevin Nisbet added 16 commits May 9, 2018 11:30

start addressing review comments

7d97789

adjust etcd upgrade process to be resilient against repeated runs

3135aa9

fix assumed etcd version during init; and path selection during upgrade

0799cc3

clear etcd latest symlink, so chown -r doesn't follow bad path outsid…

ca0fd82

…e of container

ignore service in failed status

a6c5f4d

ensure upgrade service also use new data directory path

c9e8832

set etcd directory ownership during init

b7f5c4d

use env variable to locate etcd data directory

e6834af

also add environment to etcd-upgrade service

3478ef6

use octal permissions

8f0168a

fix unit tests

f607dc9

address review comments

1159a90

don't write etcd version env variable when rolling back to 2.3.8

a3c669e

testing, set latest version to 3.3.3

8196fb6

use notfound error when failing to get etcd version

182dff9

revert testing etcd versions

5a36748

r0mant reviewed May 23, 2018

View reviewed changes

address review comments

2634907

knisbet merged commit 5376dcb into master May 23, 2018

	// wipe data directory of any previous upgrade attempt
	path := path.Join(getBaseEtcdDir(desiredVersion), "member")
	err = os.RemoveAll(path)
	if err != nil && !os.IsNotExist(err) {
	return trace.ConvertSystemError(err)
	}

etcd3 #286

etcd3 #286

Conversation

knisbet commented May 7, 2018

a-palchikov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a-palchikov May 7, 2018 • 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

a-palchikov May 9, 2018 • 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

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

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

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

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

a-palchikov May 7, 2018 •

edited

Loading

a-palchikov May 9, 2018 •

edited

Loading