-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
locksmithctl/locksmithcl: fix endpoints resilience #11
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Now it came to my mind, this should be quite easy to have integration tests, perhaps we can then ensure the desired behavior. |
@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. |
For the record, I upgraded |
And what about v3 client? |
@invidian I started to have a look last Thursday - there are two things to considere:
|
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. |
@invidian I gave a deeper try to etcd/v3 upgrade - it compiles; let's run some tests to see the damages haha. EDIT:
works fine haha |
closed in favor of: #12 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggested improvements to returned error messages and adding a comment to describe the situation, otherwise looks good 👍
locksmithctl/locksmithctl.go
Outdated
} | ||
ec, err := client.New(cfg) | ||
if err != nil { | ||
return nil, fmt.Errorf("unable to create etcd client: %w", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return nil, fmt.Errorf("unable to create etcd client: %w", err) | |
return nil, fmt.Errorf("creating etcd client: %w", err) |
locksmithctl/locksmithctl.go
Outdated
if errors.Is(err, syscall.ECONNREFUSED) { | ||
continue | ||
} | ||
return nil, fmt.Errorf("unable to create etcd lock client: %w", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return nil, fmt.Errorf("unable to create etcd lock client: %w", err) | |
return nil, fmt.Errorf("creating etcd lock client: %w", err) |
locksmithctl/locksmithctl.go
Outdated
} | ||
return lc, err | ||
|
||
return nil, errors.New("unable to create an etcd client") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return nil, errors.New("unable to create an etcd client") | |
return nil, errors.New("no etcd endpoints available, tried: %s", strings.Join(",", globalFlags.Endpoints)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
@krnowak right, the |
@invidian @krnowak thanks for your review - comments have been adressed. New body's commit message will be:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fantastic! Great work @tormath1 🎉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing, can we squash commits before merging?
@invidian obviously I'll squash the |
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>
0cc129b
to
860103c
Compare
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 wedon't have access to the
client.httpKeysAPI
which holds the configendpoints.
Signed-off-by: Mathieu Tortuyaux mathieu@kinvolk.io
See also: flatcar-archive/coreos-overlay#1161