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

Build in support for "consul lock" #224

Closed
sebest opened this issue Mar 12, 2015 · 22 comments
Closed

Build in support for "consul lock" #224

sebest opened this issue Mar 12, 2015 · 22 comments

Comments

@sebest
Copy link

sebest commented Mar 12, 2015

I know that i can use "consul lock" in command, but it would be more convenient to have a "lock" key in the JSON to lock on the command execution to make sure that there is only one service restarting at the given time.

If we could also have some kind of extra lock duration variable to be able to say that after the command exited we want to keep the lock for X seconds.

These 2 features would ease rolling updates.

@ryanuber
Copy link
Member

This makes sense to me, marking as an enhancement. Thanks!

@sethvargo
Copy link
Contributor

Hey @sebest

Thanks for suggesting this. As I mentioned in the other issue, I think there are many assumptions CT would have to make about the underlying template and its environment. For example, CT could be rendering an HTML file that is unrelated to any service registered with Consul. More importantly, the command can be arbitrary, so it would be incredibly difficult to guess the name of the service in Consul from the command the user supplied.

The restarting of a service is entirely controlled by the end user due to the extreme complexity and customization requirements. For this reason, I think the best solution is for the end-user to make these decisions inside the script CT executes. Does that make sense?

@sethvargo sethvargo changed the title feature request: built in support for "consul lock" Build in support for "consul lock" Mar 17, 2015
@ryanuber
Copy link
Member

@sethvargo I think that we actually don't need to make any assumptions here. For example, consider the following consul-template config:

template {
  source = "/var/consul/templates/httpd.ctmpl"
  destination = "/etc/httpd.conf"
  command = "restart httpd"
  lock = "services/httpd_restart_lock"
  lock_concurrent = 3
}

The two new keys would function like this:

  • lock - specifies the key to use for consul lock
  • lock_concurrent - specifies the number of times the lock can be acquired to allow the semaphore to be used.

This would not affect the current behavior of consul-template, but would allow the end user to get easy rolling upgrades. There are probably a few edge cases, but I think having this as optional behavior would allow us to just avoid them.

Does that make sense? It's possible I am missing something.

@ryanuber
Copy link
Member

Thinking about this more, the common use case would probably work by just using the consul lock command directly. Its syntax is easy enough to not require any wrapper scripts. Thats probably the way to go for now. Having it integrated might be nice but also would probably introduce more complexity than its worth.

@sethvargo
Copy link
Contributor

@ryanuber yea, I see your point, but as a counterexample, I think this is much more readable IMO:

$ consul-template -template "/templates/config:/etc/config:consul lock -n 3 service config restart"

The only disadvantage I see is that -consul and -http-addr would need to be specified if not the default (and the confusion that they are actually the same):

$ consul-template \
  -consul demo.consul.io \
  -template "/templates/config:/etc/config:consul lock -http-addr demo.consul.io -n 3 service config restart"

Even with that tradeoff, I think it will be better if we leave this decision in the hands of the user instead of having a tightly-grained integration with the lock functionality. What do you think?

@ryanuber
Copy link
Member

I agree. I think the command gets us 80% there without complicating consul-template. We can also use env vars to configure the consul lock address.

@sethvargo
Copy link
Contributor

Ah yes. Envvars. I'm going to leave this open because I think we should add an example to the README, but I think that's about it 😄

@sebest
Copy link
Author

sebest commented Mar 18, 2015

Sure, enhancing the documentation with tips is maybe the best way to go in the near future. thanx!

@sethvargo
Copy link
Contributor

Whoops. I updated the README in 662e7d1, but I forgot to close this out.

@sebest I think this is the best path moving forward for now. We can definitely revisit this issue in the future if things change though 😄.

@nhuray
Copy link

nhuray commented Jan 22, 2017

@ryanuber It's not clear for me..does the locking feature exists in Consul Template ?

The documentation does not mention any kind of lock directive for template

@sethvargo
Copy link
Contributor

It's part of the consul command.

@dadgar
Copy link
Contributor

dadgar commented Feb 1, 2017

@sethvargo Would you consider integrating this into consul-template itself? Would be nice for Nomad to be able to limit the concurrent restarts.

Referencing: hashicorp/nomad#2202

@sethvargo
Copy link
Contributor

@dadgar - not really. There's no point (imo) of copy-pasting code that's already available in consul core. I think if Nomad wants to limit concurrent restarts, it should wrap the consul lock logic.

@dadgar
Copy link
Contributor

dadgar commented Feb 2, 2017

@sethvargo Slightly disagree. You could take the sha's of the template create the lock key automatically and use Consul's API rather than CLI tool. It would be very nice!

As a library, consul-template wouldn't make implementing this that easy.

@ryancurrah
Copy link

ryancurrah commented Mar 20, 2017

This would be a great feature for us. Right now in GCP we are testing consul and consul-template. When a new service becomes available or unavailable it brings down our app because everything restarts at the same time. This makes service discovery, the main reason we want to use consul, really hard to do safely.

@dadgar
Copy link
Contributor

dadgar commented Mar 20, 2017

@ryancurrah You should look at setting splay in the meantime!

@ryancurrah
Copy link

ryancurrah commented Mar 20, 2017

@dadgar I was looking for something like splay to. I guess I did not give the docs enough of a read. It will be helpful in the meantime. Thanks!

EDIT1: Looks like its only for exec mode not templates :(

EDIT2: Look like for templates what I may want is wait

@scalp42
Copy link

scalp42 commented Mar 23, 2017

Not sure if helpful but here is an example with Chef:

consul_template_config %|nginx_upstream_#{recipe_name}| do
  templates [{
    source: %|#{node['my-consul']['consul_template']['templates']}/nginx_upstream_#{recipe_name}.ctmpl|,
    destination: %|#{node['my-loadbalancer']['consul']['include']}/nginx_upstream_#{recipe_name}.conf|,
    command: 'consul lock -n=1 locks/#{node.chef_environment}/#{cookbook_name}/#{recipe_name} "nginx -t && service nginx reload"'
  }]
  notifies :restart, 'service[consul-template]', :immediately
  action node['my-loadbalancer']['consul']['enabled'] ? :create : :delete
end

This will allow only one node at a time per Chef environment to reload nginx, just leverage consul lock directly within the consul-template command.

@ryancurrah
Copy link

@scalp42 thanks this is a nice example we will def be using it

@spanktar
Copy link

For those of us not versed in Chef, could someone help explain this usage of lock?

@alkalinecoffee
Copy link

Just to be clear on the example provided above for anyone googling this issue: if you're running multiple commands with a lock, wrap them in quotes, otherwise only the first command will run under the lock, ie:

consul lock mylocks 'curl localhost:443/status && sleep 10'

@otto-dev
Copy link

otto-dev commented Feb 20, 2023

I'm glad I found this issue. I ended up solving rolling restarts by writing an HTTP service that calls nomad alloc restart && sleep through consul lock , so that I could call that via curl on template reloads to facilitate rolling restarts. 🤯

Edit: Reason is that you cannot access the consul executable from within the docker consul driver inside the restart hook.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants