Skip to content
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

Add new "lock" command #619

Merged
merged 12 commits into from
Jan 20, 2015
Merged

Add new "lock" command #619

merged 12 commits into from
Jan 20, 2015

Conversation

armon
Copy link
Member

@armon armon commented Jan 20, 2015

This PR adds a new "consul lock" command. This command can be used to easy do distributed locking for N+1 deploys of highly available services.

@@ -24,6 +24,11 @@ const (
// before attempting to do the lock again. This is so that once a lock-delay
// is in affect, we do not hot loop retrying the acquisition.
DefaultLockRetryTime = 5 * time.Second

// LockFlagValue is a magic flag we set to indicate a key
// is being used for a semaphore. It is used to detect a potential
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wording just seems reversed here. NBD but confused me.

@ryanuber
Copy link
Member

@armon This looks great! Only minor nit-picks added. Otherwise this LGTM. Glad to see a SIGTERM attempt before SIGKILL in here, too!

case <-time.After(ttl / 2):
entry, _, err := s.Renew(id, q)
if err != nil {
return err
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible for a client to believe it holds the lock when it doesn't if this returns with an error? (since we're in a goroutine and not funneling errors down a channel)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both the Lock and Semaphore implementations will catch this and release the lock. e.g. eventually the session gets invalidated, and then they release the lock.

@ryanuber
Copy link
Member

@armon added one more question about RenewPeriodic().

@armon armon merged commit ce4fa17 into master Jan 20, 2015
@armon armon deleted the f-lock branch January 20, 2015 21:01
uses the [leader election algorithm](/docs/guides/leader-election.html).

If the lock holder count is more than one, then a semaphore is used instead.
A semaphore allows more than a single holder, but the is less efficient than
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/the // ?

@sean-
Copy link
Contributor

sean- commented Jan 21, 2015

It would be good to clarify the -n option: if a client A says -n1, client B says -n2, does that mean client B gets in? It'd be good if the number of lock holders is encoded in the lock such that -n2 will block until it wins the lock from an -n1. If that's not possible, then a note in the docs should exist to say that -n is the level of allowed concurrency by the client. It's easy for me to see situations where a client with -n1 is expecting to be the only instance operating in a critical section. Or, "Danger!!! Dragons exist when clients don't all agree on the level of concurrency." That would work, too.

layout: "docs"
page_title: "Commands: Lock"
sidebar_current: "docs-commands-lock"
description: |-
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think @sethvargo mentioned that the description needs to be a single line of text for SEO reasons. You can use > in place of |- to do this in YAML.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I've fixed this in d478f78

@ryanuber
Copy link
Member

@sean- the -n value actually is encoded in the key used for the semaphore! This allows additional clients trying to use the semaphore with a conflicting -n value to detect this and exit immediately. The code that does the check was actually part of a different PR here, which extended the API client with a bunch of helper methods, including both the lock and the semaphore. This PR wraps those extra methods nicely into the base consul CLI. Agreed though, we could probably add a note that using -n with a conflicting value will result in an error.

@armon
Copy link
Member Author

armon commented Jan 22, 2015

@sean- Clarified the documentation in d478f78. You will get an error if -n conflicts.

@sean-
Copy link
Contributor

sean- commented Jan 22, 2015

Thank you!, that looks great. I'm pleasantly impressed and surprised by the fact that this was coded (correctly) in advance. Good forethought and design by whomever. Consul++.

duckhan pushed a commit to duckhan/consul that referenced this pull request Oct 24, 2021
duckhan pushed a commit to duckhan/consul that referenced this pull request Oct 24, 2021
[sync-catalog] Fix NodePort register service with wrong internal IP when multiple internal IPs are reported on the node
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants