Comments (12)
@lyrixx Does the recipe which makes call to "consul_service_def" includes "consul" recipe? I think if you include "consul" recipe, it will work. Meanwhile, I'll do more investigation when "consul" is applied on a node and not in the same recipe.
from consul.
Because LWRP creates its own run_content that is why we get the exception. To fix it, I can add the service resource in the recipe but we end up having 2 service definitions, in recipe and resource. Or, I can remove the service notification but caller will be responsible for restarting Consul on consul_service_def change. I'm leaning more towards the former.
from consul.
@thedebugger I actually ran into this problem myself yesterday and just hadn't gotten around to writing a PR to address the issue. I should have paid more attention before I had merged the work.
The person who is writing the consul_service_def should notify the consul service that they want it to restart. I don't think it should be the job of the LWRP itself to attempt to reach out of it's run_context and notify the service resource.
When I first read the PR I thought it was a nice optimization but didn't think it all the way through. I think we should go the path of least surprises and not try to define the service resource in two places. There are plenty of error conditions just waiting for us down that path.
from consul.
Does the recipe which makes call to "consul_service_def" includes "consul" recipe
Yes, here is my sensiolabs_consul::default
include_recipe "consul"
if node['sensiolabs_consul']['serve_ui']
include_recipe "consul::ui"
end
include_recipe "sensiolabs_consul::system_checks"
node['sensiolabs_consul']['services'].each do |name, definition|
consul_service_def name do
id definition['id'] unless definition['id'].nil?
port definition['port'] unless definition['port'].nil?
tags definition['tags'] unless definition['tags'].nil?
check definition['check'] unless definition['check'].nil?
end
end
About all your questions, I have no answer. Sorry. I'm really too junior with chef for that.
The idea to reload consul directly in your cookook is nice, because I have to write less code. Then
you can enforce the use of reload
and not restart
to avoid "downtime" of node. But if it's technically not possible, I will update my code ;)
from consul.
@reset Caller notifying service['consul'] is a leaky abstraction. In the future, if we change the implementation of consul_service_def to an HTTP call, there is no need to notify the service['consul']. We can define the service['consul'] resource in a library so that it is at a single place.
from consul.
@thedebugger that's not a leaky abstraction at all. There is a clear definition between the inner workings of the LWRP and what the recipe is defining. That's why there are two run contexts.
from consul.
@reset IMO, the implementation of consul_service_def is leaky, since the caller has to decided based on the implementation if it has to notify service['consul'] or not. Won't it make sense to have the notification inside the consul_service_def if we aren't limited by different chef run contexts?
IMO, it makes sense to do it within the consul_service_def instead of burdening the caller (plus, one can easly miss to notify service['consul']).
from consul.
@thedebugger the behaviour your describing makes perfect sense and is desirable. However, with the primitives we have the only way to properly accomplish it without coupling the internals of an LWRP to the recipe of a cookbook is to have the service resource subscribe to a wildcard of all consul_service_def
LWRPs. This is not supported in Chef as of today. I honestly am pretty surprised that I've never asked for this feature before.
Unless you are speaking about the state of the consul_service_def
LWRP after merging PR #74 (what is currently in master), I don't understand why you believe it is "leaky". Prior to that PR it was completely self contained.
from consul.
@reset I think you meant PR #70. Prior to it, IMO, no service[consul]
notification was a bug; hence the PR #70. After running into this problem (and thinking through it), if we can't add notify consul['service']
inside the LWRP because of the chef limitation, the consul_service_def
implementation would be leaky. Why? Because it can have 2 implementations – existing one, and other based on HTTP API
which doesn't require notification. So, as a caller, I'll add notify servcie[consul]
based on the implementation. Makes sense?
from consul.
@thedebugger yes 70 I mean 70, though 74 is related.
The consumer of the consul_service_def
needs to notify the consul service resource of a change, yes. This would mean that we revert PR 70/74 and leave it up to the consumer to notify their service resource. If that's what you're saying we're on the same page 😄.
from consul.
@reset We only need to revert PR #70. PR #74 fixes consumer notifications which are needed when consumer like to notify service[consul]
. I'll work on the PR tonight.
from consul.
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
from consul.
Related Issues (20)
- Dangerfile should use failure instead of fail
- Remove .rubocop.yml with Dangerfile HOT 1
- Update Changelog HOT 2
- Run latest cookstyle HOT 2
- Update builds to be parallel HOT 1
- undefined method `join_path' for PoiseArchive::Resources::PoiseArchive::Resource HOT 6
- Segment part of the config is wrong type HOT 1
- Poise dependency is abandoned HOT 3
- Rewrite Consul cookbook to use custom resources HOT 5
- `services` definition throws "no implicit conversion of Symbol into Integer" HOT 3
- Support arm64 on linux
- Diplomat gem fails on Chef 16.9.29+ HOT 4
- Error in configure_diplomat with Diplomat 2.5.0 HOT 12
- Chef 17 - Poise no longer works HOT 2
- Consul 1.9.x - ui/ui_dir deprecated HOT 1
- Why are unit tests disabled? Any plans to return them back? HOT 1
- consul_service ignores program property HOT 1
- Dependency Dashboard
- Consul 1.12.x deprecated fields
- Consul client install latest packages missing checksums
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from consul.