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

server components should be broken out into a unique cookbook #616

Open
smcavallo opened this issue Jul 31, 2018 · 13 comments
Open

server components should be broken out into a unique cookbook #616

smcavallo opened this issue Jul 31, 2018 · 13 comments

Comments

@smcavallo
Copy link
Contributor

Proposal to break server components out to a unique cookbook.

The client install does not have many requirements/dependencies
The server components have a lot of requirements/dependencies - ex. requires rabbitmq/redis cookbooks

Consumers end up having to maintain all the dependent cookbooks and pushing them to all nodes
Compatibility/version conflict problems can arise for some of the dependent cookbooks.
Namely - the rabbitmq cookbook and/or attributes being applied to a non-sensu server which needs rabbitmq.

Proposal is to break out the server components into a unique cookbook/repo.
It could secure some secrets/config/attributes which should only apply to the server - better security.
Another added benefit is that it would make the cookbook(s)/PRs/issues easier to manage and revision.

I'm also proposing this because I'd like to contribute to this effort.

@majormoses
Copy link
Contributor

@smcavallo hmm I have been consuming (and more recently maintaining) this cookbook for about 5 years and have never had a dependency conflict with it. I have used this cookbook in various forms such as all in one, all but rabbitmq, and all but rabbitmq + redis. I'd like to better understand what kind of issues you have seen as I think it would break a lot of peoples usage and I'd be very hesitant to do so without a very compelling reason. Looking through the dependencies they are quite loose so any additional information you can provide would be helpful.

Consumers end up having to maintain all the dependent cookbooks and pushing them to all nodes

Based on this statement you are using chef-solo rather than chef-server? This is generally managed using berks.

It could secure some secrets/config/attributes which should only apply to the server - better security.

Can you elaborate on what this looks like? For example the same rabbitmq certificate is needed on both the client and server. If you are referring to your own custom secrets (such as using token substitution) you can maintain your own business logic for how to retrieve them only on the clients that you want them on, I am not sure how this community cookbook could know what that would look like.

I'm also proposing this because I'd like to contribute to this effort.

Appreciated and look forward to working with you on improving this cookbook whether that looks like splitting it out or finding additional ways to reduce dependency issues with smaller incremental change.

@tas50 what are your thoughts on this?

@smcavallo
Copy link
Contributor Author

@majormoses -
The current dependency tree for the sensu cookbook looks like this ->

- windows
- ms_dotnet
    - windows
- rabbitmq
  - dpkg_autostart
  - logrotate
  - erlang
    - yum-epel
    - yum-erlang_solutions
    - build-essential
  - yum-epel
  - yum-erlang_solutions
    - yum-epel
- redisio
  - ulimit
  - build-essential
  - selinux_policy
    - compat_resource

The downstream dependencies are for the most part limited to the rabbitmq and redisio cookbooks.
We've moved to dockerized rabbitmq and dockerized redis for sensu.
We'd like to not have to maintain all those dependent cookbooks on our chef server any longer.

We use chef-server and lock cookbooks at the environment level.
We've run into version conflicts in the past for some of the dependent cookbooks which was slightly painful, because it meant nodes wouldn't converge until those conflicts were resolved. Ex. a different cookbook requiring a different version.
Also - some of those cookbooks (ex. redisio) took a very long time to be chef 14 compatible.

We run non-sensu rabbitmq servers as part of our infrastructure.
We've had issues where an engineer would set a rabbitmq config attribute in an environment or role or cookbook and since the rabbitmq cookbook applies to both sensu rabbitmq and non-sensu rabbitmq there would be unintended consequences. It would either modify the sensu rabbitmq in the wrong way when attempting to modify the non-sensu rabbitmq (and break our monitoring), or modify the non-sensu rabbitmq in the wrong way when attempting to modify the sensu rabbitmq (and break our infrastructure.) The fact that sensu cookbook relies on the rabbitmq cookbook makes it easy to shoot yourself in the foot by setting rabbitmq attributes at the wrong level due to attribute inheritance.
It would help prevent those issues if the server/client were separate cookbooks.

@majormoses
Copy link
Contributor

majormoses commented Aug 2, 2018

@smcavallo thanks for taking the time to help me understand. As there is no sensu-client, sensu-api, and sensu-server packages the server and the client make sense to stay in the same cookbook. At my last org we did not use the rabbimq or redis recipes in this cookbook and had our own wrapper around those respective cookbooks. I am thinking the best compromise is to change the dependencies from depends "rabbitmq", ">= 2.0.0" to suggests "rabbitmq", ">= 2.0.0" (and same for redis) so that for those that do not rely on this cookbook to manage those components of the sensu stack can be unblocked while avoiding a breaking change.

I think there are a lot of people who use this as an all in one and I'd hate to break even 50% of use cases when there is a middle ground.

@tas50 any thoughts?

@smcavallo
Copy link
Contributor Author

@majormoses - yes i'd love for those dependencies to be 'suggests' instead of 'depends' and would give us the flexibility to include/exclude those dependencies in our wrapper cookbooks.

afaik the 'suggests' is deprecated which is why i didn't advocate for that solution, although simply changing those dependencies to 'suggests' would solve all our problems.
http://www.foodcritic.io/#FC052
although this may just mean ignored/not-fully-implemented.

@smcavallo
Copy link
Contributor Author

If acceptable, i've created a PR for the change from depends to suggests.
This is a breaking change. Since the default is now opt-out, consumers would need to implement the dependency requirements in their wrapper cookbooks. It's an ideal solution for consumers who'd like to opt out. It's not an ideal solution for consumers who'd like to stay opted in. Especially since in addition to implementing their own dependencies on those cookbooks, they'd need to implement their own VERSION dependencies. Ex.

suggests "rabbitmq", ">= 2.0.0"
suggests "redisio", ">= 2.7.0"

If you ever bump the minimum versions of these cookbooks, there's no mechanism for consumers to know that they need to update the version requirements in their wrapper cookbooks.

@tas50
Copy link
Contributor

tas50 commented Aug 3, 2018

Please don't use suggests in metadata. It does nothing at all and we're going to be removing that code entirely in the next version of Chef.

@smcavallo
Copy link
Contributor Author

@tas50 and @majormoses - agreed - closed that PR

@majormoses
Copy link
Contributor

In an ideal world if we were starting from scratch what I would probably do is this:

  1. create sensu-chef that only knows about installing and configuring sensu (may populate rmq+ redis config in sensu context but nothing more.
  2. create a sensu-stack-chef that would depend on ☝️, use all the upstream custom resources/providers, have nothing more than a wrapper around ☝️ and include the two recipes for rmq + redis as they currently exist

I need to think about this more and poll the community to get a better understanding on the impact to existing users. I imagine the number of users this would affect is quite high in comparison to the number of people who would benefit from such a split. Additionally I will have a discussion with sensu inc about it to get their two cents.

@smcavallo
Copy link
Contributor Author

@majormoses - agreed with the above, as a consumer, we can live with this 'as-is' for now as sensu 2.0 is coming down the pipeline. would it make sense for the sensu 2.0 cookbooks to follow that design?

@majormoses
Copy link
Contributor

majormoses commented Aug 9, 2018

The sensu 2.x cookbook is something we have already been working on here: https://github.com/sensu/sensu-go-chef and its only current dependency is on packagecloud: https://github.com/sensu/sensu-go-chef/blob/master/metadata.rb#L30. As sensu 2.0 arch is vastly different we sidestep this kind of issue.

Sensu Deployment now consists of two golang compiled binaries from the various OS packages:

  • backend (has an embedded etcd and eventually will support external etcd clusters), includes dashboard
  • agent: this is a client
@majormoses
Copy link
Contributor

If someone decides to use an external etcd cluster we will not be including that in our cookbook and will only support making sure that you give the sensu processes the right config and thats it and anything external would go into something like sensu-go-stack-chef.

@oscar123mendoza
Copy link

oscar123mendoza commented Jan 8, 2019

I hope the next sensu cookbook does follow this pattern. Let's simply take Rabbitmq for example. If I want to set up sensu to monitor my production rabbitmq cluster I now have to use the same version of the rabbitmq community cookbook on both my sensu server and rabbitmq nodes when in reality they should not be correlated what so ever just because they use the same sensu cookbook. If sensu-client had its own cookbook I can remove those dependencies. Why do I have to have redis as a dependency on my production rabbit cluster?

@majormoses
Copy link
Contributor

@oscar123mendoza yup the sensu-go cookbook only has packagecloud as the only direct dependency. If you plan on running an external etcd cluster we will not be handling this in that cookbook.

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