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

Ensure firewalld is not attempted on el6 #97

Merged
merged 1 commit into from Sep 29, 2015

Conversation

glenjamin
Copy link
Contributor

I'm seeing an issue were chef provider selection appears to be non-deterministic, depending on whether the firewalld or iptables provider is checked first.

This resulted in firewalld trying to install on EL6 machines.

The included patch ensures that there is no overlap in provider selection.

@martinb3
Copy link
Contributor

@glenjamin What version of chef was this one? I'd like to debug it further if possible.

@glenjamin
Copy link
Contributor Author

This was seen on a few versions between 12.2.1 and 12.4.1.

I've been unable to find any documentation that indicates how chef selects providers - I added some debug logging into the node block, and it appeared to be based on first-matching.

@martinb3
Copy link
Contributor

We're testing against 12.4.1, and I haven't seen this one. I'd like to fix the bug in chef proper, if that's where the problem is. There was definitely a bug in Chef 12.4.0 where the block was never called, so you would have seen the problem on that version for sure.

@martinb3
Copy link
Contributor

Could you give me a specific Chef version and distro/version you saw it on, so I could test that?

@glenjamin
Copy link
Contributor Author

I saw this on at least centos 6.6 on azure (open logic) running chef 12.4.1

My initial manual chef-client run correctly selected iptables, but future chef runs and chef-service runs across the estate started failing.

I patched the cookbook with logging of platform version in both the iptables and firewalld provider, and saw only the firewalld provider log "6.6" before beginning its install.

I applied the attached fix to our chef server by uploading the fork and am no longer seeing any issue.

@glenjamin
Copy link
Contributor Author

Reading the comment in https://github.com/chef/chef/blob/master/lib/chef/provider_resolver.rb indicates that the selected provider is load order dependent

Is there ever a scenario where firewalld should be selected when platform version is less than 7? If not then I believe this patch is valid, and ensures firewalld will not be considered for auto-selection on older systems.

@martinb3
Copy link
Contributor

Hi @glenjamin -- yes, I agree that this would be a valid fix; I was hoping we could understand why it doesn't already work though. I'd like to report the bug upstream if chef isn't behaving correctly. I'll open a separate issue for it...

martinb3 added a commit that referenced this pull request Sep 29, 2015
Ensure firewalld is not attempted on el6
@martinb3 martinb3 merged commit d24f3c9 into sous-chefs:master Sep 29, 2015
@glenjamin
Copy link
Contributor Author

I think the key phrase is this one:

The first matching Provider in the most recently registered list is selected and returned.

https://github.com/chef/chef/blob/master/lib/chef/provider_resolver.rb#L44

Unless you explicitly load the files in the order you want then you're at the mercy of whatever require_all magic chef uses - which presumably is not strictly defined to be equivalent cross-platform.

@martinb3
Copy link
Contributor

I believe chef should also choose providers with blocks before providers without those. That bug I linked was actually about 12.4.0 not doing that, and then they fixed it -- chef/chef#3593.

Specifically, check out this comment from one of the core chef developers:

Both providers match on centos, and for some reason the NodeMap stuff is not considering the filter with the block to be of a 'higher priority' and returning it. Which is a bug in core chef AFAIK (probably something that jkeiser needs to look at).

He goes on to suggest the same solution you've contributed:

But the problem can be resolved here by making the selection of the provider unambiguous. You always want to select sysvinit on centos/scientific/oracle/redhat where pv < 7.0 and always want to select systemd on those when pv >= 7.0 so that is exactly how the provides lines should be fixed to read.

But I still think there's a bug in core chef somewhere, based on that comment.

@glenjamin
Copy link
Contributor Author

Cheers for the links, definitely sounds like a bug in the more general case then!

On 29 Sep 2015, at 16:40, Martin Smith notifications@github.com wrote:

I believe chef should also choose providers with blocks before providers without those. That bug I linked was actually about 12.4.0 not doing that, and then they fixed it -- chef/chef#3593.

Specifically, check out this comment from one of the core chef developers:

Both providers match on centos, and for some reason the NodeMap stuff is not considering the filter with the block to be of a 'higher priority' and returning it. Which is a bug in core chef AFAIK (probably something that jkeiser needs to look at).

He goes on to suggest the same solution you've contributed:

But the problem can be resolved here by making the selection of the provider unambiguous. You always want to select sysvinit on centos/scientific/oracle/redhat where pv < 7.0 and always want to select systemd on those when pv >= 7.0 so that is exactly how the provides lines should be fixed to read.

But I still think there's a bug in core chef somewhere, based on that comment.


Reply to this email directly or view it on GitHub.

martinb3 added a commit that referenced this pull request Oct 6, 2015
Ensure all of the distros/versions and provides blocks yield exactly one provider, no more.

RE: #97, #98.
glenjamin pushed a commit to glenjamin/firewall that referenced this pull request Nov 12, 2015
Ensure all of the distros/versions and provides blocks yield exactly one provider, no more.

RE: sous-chefs#97, sous-chefs#98.
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.

None yet

2 participants