The service consul is not present and restart fail

[sous-chefs]

The service consul is not present and restart fail

[thedebugger]

@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.

[thedebugger]

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.

@reset @lyrixx thoughts?

[reset]

@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.

[lyrixx]

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 ;)

[thedebugger]

@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.

[reset]

@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.

[thedebugger]

@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']).

[reset]

@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.

[thedebugger]

@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?

[reset]

@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 😄.

[thedebugger]

@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.

[lock]

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.