locksmithctl/locksmithcl: fix endpoints resilience #11

tormath1 · 2021-08-04T10:34:48Z

with the recent etcd upgrade (1b5eb50),
the endpoints resilience were not ensured -> having failing endpoints
would make the locksmithcl commands fail.

in this commit, we patch the software (and not the vendor), to reproduce
a kind of resilience: we loop over the endpoints - if one of them is not
reachable we continue to the over.
The loop could have been done elsewhere in the Create command, but we
don't have access to the client.httpKeysAPI which holds the config
endpoints.

Signed-off-by: Mathieu Tortuyaux mathieu@kinvolk.io

See also: flatcar-archive/coreos-overlay#1161

invidian

@tormath1 maybe we could try upgrading to latest etcd client version, maybe there was a regression which is now fixed? I know there has been a lot of internal changes in etcd v3.5.0, which makes it much easier to consume via go modules.

invidian · 2021-08-04T10:58:37Z

Now it came to my mind, this should be quite easy to have integration tests, perhaps we can then ensure the desired behavior.

tormath1 · 2021-08-04T11:04:57Z

maybe we could try upgrading to latest etcd client version, maybe there was a regression which is now fixed? I know there has been a lot of internal changes in etcd v3.5.0, which makes it much easier to consume via go modules.

@invidian I spent quite some times to figure out the changes on the concerned files - but yeah maybe the issue has been fixed deeper elsewhere. I'll give a try to the upgrade - would be nice also in order to prepare any v3 migration.

locksmithctl/locksmithctl.go

tormath1 · 2021-08-04T16:01:58Z

For the record, I upgraded etcd to /v2 - we still have the same behavior (see: https://github.com/kinvolk/locksmith/commits/tormath1/upgrade-etcd)

invidian · 2021-08-10T11:03:25Z

For the record, I upgraded etcd to /v2 - we still have the same behavior (see: https://github.com/kinvolk/locksmith/commits/tormath1/upgrade-etcd)

And what about v3 client?

tormath1 · 2021-08-10T12:27:21Z

@invidian I started to have a look last Thursday - there are two things to considere:

etcd/v3 client does not seem compatible with v2 server - so it's not a "simple" refactoring since we could break users with etcd/v2 nodes ? I agree we should upgrade to v3 but without a test cover and a way to provide a seamless transition it sounds a bit ... perilous ><
this interface: https://github.com/kinvolk/locksmith/blob/c96ae6e242605a6bdb8969faed1bd848d724e7e1/lock/etcd.go#L36-L42 does not seem to exist anymore with v3

invidian · 2021-08-10T15:21:18Z

Thanks for checking @tormath1. You're perhaps right about migration to v3, it doesn't seem trivial, if the data is stored in a different place. Meaning upgrading only some clients to v3 can cause multiple nodes to be updated simultaneously. Perhaps we could create an issue for this.

tormath1 · 2021-08-13T08:31:17Z

@invidian I gave a deeper try to etcd/v3 upgrade - it compiles; let's run some tests to see the damages haha.

EDIT:

core@localhost ~ $ ./locksmithctl status
Available: 1
Max: 1
core@localhost ~ $ ./locksmithctl set-max 1234
Old-Max: 1
Max: 1234
core@localhost ~ $ ./locksmithctl status
Available: 1
Max: 1

works fine haha

tormath1 · 2021-08-13T15:31:37Z

closed in favor of: #12

tormath1 · 2021-08-25T13:49:51Z

@kinvolk/flatcar-maintainers @invidian let's bring back this PR to life.

Since the on-going PR (#12) to upgrade to etcd/v3 will take some time (need to properly test it, rewrite a few things); let's patch the current locksmith with the following changes to ship locksmith in the OS.

invidian

I suggested improvements to returned error messages and adding a comment to describe the situation, otherwise looks good 👍

invidian · 2021-08-25T13:57:56Z

locksmithctl/locksmithctl.go

-	}
+		ec, err := client.New(cfg)
+		if err != nil {
+			return nil, fmt.Errorf("unable to create etcd client: %w", err)


Suggested change

return nil, fmt.Errorf("unable to create etcd client: %w", err)

return nil, fmt.Errorf("creating etcd client: %w", err)

invidian · 2021-08-25T13:58:03Z

locksmithctl/locksmithctl.go

+			if errors.Is(err, syscall.ECONNREFUSED) {
+				continue
+			}
+			return nil, fmt.Errorf("unable to create etcd lock client: %w", err)


Suggested change

return nil, fmt.Errorf("unable to create etcd lock client: %w", err)

return nil, fmt.Errorf("creating etcd lock client: %w", err)

invidian · 2021-08-25T13:59:07Z

locksmithctl/locksmithctl.go

 	}
-	return lc, err
+
+	return nil, errors.New("unable to create an etcd client")


Suggested change

return nil, errors.New("unable to create an etcd client")

return nil, errors.New("no etcd endpoints available, tried: %s", strings.Join(",", globalFlags.Endpoints))

locksmithctl/locksmithctl.go

krnowak

Looks good, but the commit message is confusing. What Create command? I don't see locksmithctl having a create command. You probably meant something about etcd internal create command or something?

tormath1 · 2021-08-25T14:28:31Z

@krnowak right, the Create actually refers to the Create method in the KeysAPI interface but it's confusing and I don't remember what I wanted to say - I'll definitely reword the commit.

tormath1 · 2021-08-25T14:47:02Z

@invidian @krnowak thanks for your review - comments have been adressed.

New body's commit message will be:

with the recent etcd upgrade (1b5eb50),
the endpoints resilience were not ensured -> having failing endpoints
would make the `locksmithcl` commands fail.

in this commit, we patch the software (and not the vendor), to reproduce
a kind of resilience: we loop over the endpoints - if one of them is not
reachable we continue to the over.
this patch can be reverted once the upgrade to etcd/v3 will be done.

invidian

Looks fantastic! Great work @tormath1 🎉

invidian

One thing, can we squash commits before merging?

tormath1 · 2021-08-26T07:11:52Z

@invidian obviously I'll squash the fixup! commits before merging if this is what you're talking about. 🥳

with the recent etcd upgrade (1b5eb50), the endpoints resilience were not ensured -> having failing endpoints would make the `locksmithcl` commands fail. in this commit, we patch the software (and not the vendor), to reproduce a kind of resilience: we loop over the endpoints - if one of them is not reachable we continue to the over. this patch can be reverted once the upgrade to etcd/v3 will be done. Signed-off-by: Mathieu Tortuyaux <mathieu@kinvolk.io>

tormath1 requested review from invidian and a team August 4, 2021 10:34

tormath1 self-assigned this Aug 4, 2021

invidian reviewed Aug 4, 2021

View reviewed changes

locksmithctl/locksmithctl.go Outdated Show resolved Hide resolved

tormath1 mentioned this pull request Aug 13, 2021

app-admin/locksmith: bump commit ID flatcar-archive/coreos-overlay#1161

Merged

tormath1 closed this Aug 13, 2021

invidian deleted the tormath1/fix-side-effect-upgrade branch August 14, 2021 12:01

tormath1 restored the tormath1/fix-side-effect-upgrade branch August 25, 2021 13:46

tormath1 reopened this Aug 25, 2021

invidian suggested changes Aug 25, 2021

View reviewed changes

krnowak reviewed Aug 25, 2021

View reviewed changes

krnowak approved these changes Aug 25, 2021

View reviewed changes

invidian approved these changes Aug 25, 2021

View reviewed changes

invidian suggested changes Aug 25, 2021

View reviewed changes

tormath1 force-pushed the tormath1/fix-side-effect-upgrade branch from 0cc129b to 860103c Compare August 26, 2021 07:15

invidian approved these changes Aug 26, 2021

View reviewed changes

tormath1 merged commit 63cef2d into flatcar-master Aug 26, 2021

tormath1 deleted the tormath1/fix-side-effect-upgrade branch August 26, 2021 07:37

tormath1 mentioned this pull request Aug 27, 2021

locksmithcl: reset endpoints when we set a new one #13

Merged

tormath1 mentioned this pull request Oct 20, 2021

[WIP] Integrate Fleetlock client with Locksmith #14

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

locksmithctl/locksmithcl: fix endpoints resilience #11

locksmithctl/locksmithcl: fix endpoints resilience #11

tormath1 commented Aug 4, 2021

invidian left a comment

invidian commented Aug 4, 2021

tormath1 commented Aug 4, 2021 •

edited

Loading

tormath1 commented Aug 4, 2021

invidian commented Aug 10, 2021

tormath1 commented Aug 10, 2021 •

edited

Loading

invidian commented Aug 10, 2021 •

edited

Loading

tormath1 commented Aug 13, 2021 •

edited

Loading

tormath1 commented Aug 13, 2021

tormath1 commented Aug 25, 2021

invidian left a comment

invidian Aug 25, 2021

invidian Aug 25, 2021

invidian Aug 25, 2021

krnowak left a comment

tormath1 commented Aug 25, 2021

tormath1 commented Aug 25, 2021

invidian left a comment

invidian left a comment

tormath1 commented Aug 26, 2021 •

edited

Loading

	return nil, fmt.Errorf("unable to create etcd client: %w", err)
	return nil, fmt.Errorf("creating etcd client: %w", err)

	return nil, fmt.Errorf("unable to create etcd lock client: %w", err)
	return nil, fmt.Errorf("creating etcd lock client: %w", err)

	return nil, errors.New("unable to create an etcd client")
	return nil, errors.New("no etcd endpoints available, tried: %s", strings.Join(",", globalFlags.Endpoints))

locksmithctl/locksmithcl: fix endpoints resilience #11

locksmithctl/locksmithcl: fix endpoints resilience #11

Conversation

tormath1 commented Aug 4, 2021

invidian left a comment

Choose a reason for hiding this comment

invidian commented Aug 4, 2021

tormath1 commented Aug 4, 2021 • edited Loading

tormath1 commented Aug 4, 2021

invidian commented Aug 10, 2021

tormath1 commented Aug 10, 2021 • edited Loading

invidian commented Aug 10, 2021 • edited Loading

tormath1 commented Aug 13, 2021 • edited Loading

tormath1 commented Aug 13, 2021

tormath1 commented Aug 25, 2021

invidian left a comment

Choose a reason for hiding this comment

invidian Aug 25, 2021

Choose a reason for hiding this comment

invidian Aug 25, 2021

Choose a reason for hiding this comment

invidian Aug 25, 2021

Choose a reason for hiding this comment

krnowak left a comment

Choose a reason for hiding this comment

tormath1 commented Aug 25, 2021

tormath1 commented Aug 25, 2021

invidian left a comment

Choose a reason for hiding this comment

invidian left a comment

Choose a reason for hiding this comment

tormath1 commented Aug 26, 2021 • edited Loading

tormath1 commented Aug 4, 2021 •

edited

Loading

tormath1 commented Aug 10, 2021 •

edited

Loading

invidian commented Aug 10, 2021 •

edited

Loading

tormath1 commented Aug 13, 2021 •

edited

Loading

tormath1 commented Aug 26, 2021 •

edited

Loading