search

Netuitive / chef-netuitive

Installs Native Netuitive Agent
MIT License
4 stars 6 forks source link

Added a docker_collector_metrics_whitelist #61

Closed rclagett closed 5 years ago

rclagett commented 5 years ago

Pull Request Checklist

Is this in reference to an existing issue? No

General

New Features

Purpose

Adds the ability to filter docker container metrics

Known Compatibility Issues

N/A

rclagett commented 5 years ago

I'm not sure what to make of the errors causing the build to fail.

Would you mind lending an expert hand or two @majormoses @ziggythehamster?

majormoses commented 5 years ago

Hmm I feel like its because of some changes in chef 13 around the apt cookbook but it should still be present. I am gonna take a quick peak and if I dont see whats going on then I will give it some more attention over the weekend.

majormoses commented 5 years ago

This is incredibly strange, I have so far been unsuccessful in reproducing this locally. Here are some interesting tidbits. Locally I see it resolving the cookbook dependencies

-----> Creating <netuitive-ubuntu-1604>...
       Creating kitchen sandbox at /home/babrams/.dokken/kitchen_sandbox/ad44ddee27-netuitive-ubuntu-1604
       Creating verifier sandbox at /home/babrams/.dokken/verifier_sandbox/ad44ddee27-netuitive-ubuntu-1604
       Building work image..
       Creating container ad44ddee27-netuitive-ubuntu-1604
       Finished creating <netuitive-ubuntu-1604> (0m21.31s).
-----> Converging <netuitive-ubuntu-1604>...
       Creating kitchen sandbox in /home/babrams/.dokken/kitchen_sandbox/ad44ddee27-netuitive-ubuntu-1604
       Preparing dna.json
       Resolving cookbook dependencies with Berkshelf 5.6.5...
       Removing non-cookbook files before transfer
       Preparing validation.pem
       Preparing client.rb
Starting Chef Client, version 13.8.13
Creating a new client identity for netuitive-ubuntu-1604 using the validator key.
resolving cookbooks for run list: ["netuitive::add_repo", "netuitive::install_agent", "netuitive::configure"]
Synchronizing Cookbooks:
  - netuitive (0.21.6)
  - apt (3.0.0)
  - yum (3.10.0)
Installing Cookbook Gems:

However in travis we see

-----> Creating <netuitive-ubuntu-1804>...
       Creating container chef-13.8.13
       Creating kitchen sandbox at /home/travis/.dokken/kitchen_sandbox/6c1504d6e9-netuitive-ubuntu-1804
       Creating verifier sandbox at /home/travis/.dokken/verifier_sandbox/6c1504d6e9-netuitive-ubuntu-1804
       Building work image..
       Creating container 6c1504d6e9-netuitive-ubuntu-1804
       Finished creating <netuitive-ubuntu-1804> (0m26.31s).
-----> Converging <netuitive-ubuntu-1804>...
       Creating kitchen sandbox in /home/travis/.dokken/kitchen_sandbox/6c1504d6e9-netuitive-ubuntu-1804
       Preparing dna.json
       Resolving cookbook dependencies with Berkshelf 5.6.5...
       Removing non-cookbook files before transfer
       Preparing validation.pem
       Preparing client.rb
Starting Chef Client, version 13.8.13
Creating a new client identity for netuitive-ubuntu-1804 using the validator key.
resolving cookbooks for run list: ["netuitive::add_repo", "netuitive::install_agent", "netuitive::configure"]
================================================================================
Error Resolving Cookbooks for Run List:
================================================================================
Missing Cookbooks:
------------------
No such cookbook: apt
Expanded Run List:
------------------
* netuitive::add_repo
* netuitive::install_agent
* netuitive::configure

I will need to take a closer look and make sure there is parity of tooling (versions).

ziggythehamster commented 5 years ago

We ran into this, too.

In your fork, you depend on the APT cookbook (https://github.com/Netuitive/chef-netuitive/blob/master/metadata.rb#L17), which they deprecated/removed in 13.3 and up. In our fork, we ran into this problem and fixed it by removing the dependency. The trade-off is that the cookbook would then be completely broken in <13.3, and you apparently can't do anything in metadata.rb which doesn't actually end up compiling to a JSON file or something.

Here's the PR which fixed this for us: https://github.com/art19/chef-netuitive/pull/6

If you're happy breaking backcompat for Chef <13.3, this should be an easy one to resolve :)

majormoses commented 5 years ago

@ziggythehamster I suspected it was related but the behavior is odd. Here is my thought on a game plan:

  1. I think we should accept this contribution and ignore the CI issue for the moment.
  2. Cut a minor version to release the functionality, this only affects people who it already effects in a sense.
  3. Cut a major version (1.0.0 in this case) and break backwards compatibility supporting only chef 13, I think this is very reasonable given that chef 12 support ended quite a while ago.

If this sounds good I will do this over the weekend.

rclagett commented 5 years ago

Sounds good to me. Appreciate the input!

ziggythehamster commented 5 years ago

Yeah I'm happy with that too

rclagett commented 5 years ago

@majormoses Will you have some time this week to cut new releases?

majormoses commented 5 years ago

@rclagett I had my sister unexpectedly pop into town to visit me this weekend so I didnt have time to work on it. I will find some time this week, probably over the next few days.

majormoses commented 5 years ago

released: https://supermarket.chef.io/cookbooks/netuitive/versions/0.22.0

majormoses commented 5 years ago

I have created https://github.com/Netuitive/chef-netuitive/issues/62 to track the CI and support issues.

ziggythehamster commented 5 years ago

I missed this in the review: the resource doesn't define an docker_collector_metrics_whitelist option, so the library won't work

ziggythehamster commented 5 years ago

https://github.com/Netuitive/chef-netuitive/blob/master/libraries/resource_configure.rb#L17 <- needs a configurable below it

rclagett commented 5 years ago

I submitted a new PR to address this - https://github.com/Netuitive/chef-netuitive/pull/64