Added enable flag for chef and set it to true by default #20

ghost · 2018-03-29T19:35:06Z

Added the enable configuration flag for chef and set it to true by default to ensure backward compatibility.

…ckward compatability

eheydrick

Hi @sestary thanks for the contribution! A few comments. In order to maintain backwards compatibility with existing configs we should have chef enabled by default like here. Also note that it would still use chef to determine the node name even if chef support were disabled. We should document that in the README.

I'd love to support integrations with other systems - right now it's tied to chef but there's no reason it has to be. We're you looking to make chef support entirely optional?

eheydrick · 2018-03-30T21:39:32Z

Part of #18.

ghost · 2018-04-04T15:57:43Z

Thanks Eric, Yes I was looking to make chef completely optional. The follow up commits add the following:

Backwards compatibility with older config files.
Using sensu as a lookup for client name (also with backwards compatibility).
Updated the sample config file with the new values.
Moved the logic for the deletion of messages out of the individual functions into the main loop.
Fixes a bug where if the node was not found in chef the message would never get deleted.

Also fixed up my code so rubycop is happy.

Hope this works for you.

Thanks,

Robbert

majormoses

Overall I think this looks good. I have a couple of comments/questions to address.

majormoses · 2018-04-04T16:11:30Z

config.yml.sample

@@ -1,3 +1,4 @@
+:lookup: chef


I assume this should be quoted.

Added that.

majormoses · 2018-04-04T16:12:43Z

lib/aws-cleaner.rb

@@ -30,15 +30,15 @@ def self.client(config)
    # call the Chef API to get the node name of the instance
    def self.get_chef_node_name(instance_id, config)
      chef = client(config)
-      results = chef.search.query(:node, "ec2_instance_id:#{instance_id} OR chef_provisioning_reference_server_id:#{instance_id}")
+      results = chef.search.query(:node, "ec2_instance_id:#{instance_id[2..-1]} OR chef_provisioning_reference_server_id:#{instance_id[2..-1]}")


I don't have a chef server currently to test this against can you show me the output of this or explain this change?

This removes the "i-" part from the instance ID in case you don't have that. I do the same on the sensu side since we don't have that part in our client names.

I tested it on a quick chef install and it seemed to work but don't have a full chef install to test against. I'm ok omitting this change.

I'd like to address this separately and will be starting a new job next week so I will have a chef and sensu server to test with.

I think that should be removed since it's going to break lookups for the instance. Ohai populates that attribute with the actual instance-id and it will always include i-.

I have reverted that, I figured since it was the "search" api that was being called it would also search partial strings it seemed to work on my quick test with chef but since I don't use it I'm fine with reverting it.

majormoses · 2018-04-04T16:15:19Z

bin/aws_cleaner.rb

-        webhook(id, instance_id)
-        chef(id, instance_id, chef_node)
-        sensu(id, instance_id, chef_node)
+        node = case @config[:lookup]


This option name seems too ambiguous to me, short variables are good but maybe making it a bit more verbose will do some good here. maybe something along the lines of lookup_type?

Have updated it to lookup_type. That is a bit more clear.

eheydrick · 2018-04-04T17:13:38Z

Thanks @sestary will review soon.

ghost · 2018-04-04T20:51:14Z

I added another option to listen for multiple states and completely ignore messages that don't match these states, this means other programs will be able to listen to the same queue.

Let me know what you think.

Thanks,

Robbert

ghost

Let me know how you want to handle this. I did make the change in the last commit.

ghost · 2018-04-04T19:28:07Z

bin/aws_cleaner.rb

      end
+
+      AwsCleaner.new.delete_message(id, @config)


I was thinking about this and discussing with a colleague, right now the message is deleted when it is not deemed relevant like when the state is not "terminated", however if you have another process looking at the same SQS queue for "running" events this could cause messages to get lost. Should this be moved to before the else statement so it looks like this? That way the message is only deleted once processed and not when it is ignored, the other side affect is that if you have nothing else listening you will have extra messages queuing up that will never be processed.

end AwsCleaner.new.delete_message(id, @config) else @logger.info('Message not relevant, ignoring...') end```

I think the expectation has been that this queue is only used for aws-cleaner I would not want to mix this and say application queues there is no real cost to using a seperate queue. I'd like to hear more of a real world justification before wanting to go down this route.

majormoses

Overall I am 👍 but I think we are seeing too much scope creep for my comfort. Let's get the minimum required changes in within the scope of the issue and then discuss and enhance further afterwards.

majormoses · 2018-04-06T21:10:00Z

README.md

-By default aws-cleaner assumes that removing from sensu is desired as this was one of its core intentions. To allow people to leverage this without sensu you can disable it via config:
+By default aws-cleaner assumes that the lookup will be done against chef as this was one of its core intentions. This can be configured for sensu via config:
+
+You can disable sensu via config as well to skip removing the client from sensu:


This does not indicate that you can disable removing from chef.

Updated the read me a little, but the section of chef is a little further down in the read me, but I have clarified the README a bit in the next commit.

majormoses · 2018-04-06T21:12:30Z

bin/aws_cleaner.rb

+        node = case @config[:general][:lookup_type]
+               when 'sensu'
+                 AwsCleaner::Sensu.get_sensu_node_name(instance_id, @config)
+               else


Should this not be when 'chef'?

No this should be when chef, since the variable set to "chef" plus any invalid value would default to use chef.

majormoses · 2018-04-06T21:17:29Z

lib/aws-cleaner.rb

-    return false if message_body['detail']['instance-id'].nil? &&
-                    message_body['detail']['state'] != 'terminated'
+  def process_message(message_body, config)
+    return false if message_body['detail']['instance-id'].nil? ||


lets use the && to keep the existing behavior.

This one is my only problem, since we use one queue to monitor the "running" state notifications with aws-watcher and prefer not to add multiple queues since it's more to manage. We already have a queue/process for every AWS region.

Would it be ok to add a config parameter to delete irelevant messages that is set to true by default, that way by default the existing behavior is kept but it can be overwritten. I was thinking I can add that either in the main loop or the process_message function.

Awesome to see others using that project and the use case makes sense to me.

I'd still prefer it to be in 2 separate PRs though personally @eheydrick what do you think?

I have separated the changes. I will submit another pull request for the message handling.

eheydrick · 2018-04-09T17:05:33Z

lib/aws-cleaner.rb

@@ -58,6 +58,37 @@ def self.remove_from_chef(node_name, chef, config)
  end

  module Sensu
+    # call the Chef API to get the node name of the instance


update comment to Sensu.

eheydrick · 2018-04-09T17:06:34Z

config.yml.sample

@@ -1,3 +1,7 @@
+:general:
+  :lookup_type: 'chef'


thinking this variable should node_name_provider or something like that to be clear what exactly is being looked up.

Done in the last commit.

eheydrick · 2018-04-12T21:15:42Z

👍 Thanks for splitting that out. I'm going to test out the changes and get this merged in if all looks good.

majormoses · 2018-06-06T14:32:35Z

@eheydrick looks like the user is no longer on github so we should test it and if we need any other changes we are gonna need to make them ourselves.

eheydrick · 2018-06-06T21:00:38Z

Sounds good. I'll have some time to test and review soon.

Added enable flag for chef and set it to true by default to ensure ba…

e3c1681

…ckward compatability

eheydrick requested changes Mar 30, 2018

View reviewed changes

Robbert Rijkse added 5 commits April 3, 2018 09:47

First attempt at getting client from sensu

fd253f5

Finished client lookup in sensu

8ac38a7

Cleanup rubycop violations

9681bad

Cleanup some more rubycop issues

0a5e4f0

More rubycop cleanup

7577f11

majormoses reviewed Apr 4, 2018

View reviewed changes

Robbert Rijkse added 2 commits April 4, 2018 13:07

Change lookup var name, added quotes in config file.

253d254

Whoops forgot to indent

b0c74b9

Added state option and updated the README file.

9410437

Rubycop code cleanup

009084d

ghost commented Apr 4, 2018

View reviewed changes

majormoses requested changes Apr 6, 2018

View reviewed changes

eheydrick reviewed Apr 9, 2018

View reviewed changes

Robbert Rijkse added 4 commits April 9, 2018 14:02

Fixes to feedback from @eheydrick and @majormoses

60865cb

Moved to elsif

0d763a2

Reverted state termination changes, only has changes for adding sensu

a3388be

Removing ruboCop error

304b011

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added enable flag for chef and set it to true by default #20

Added enable flag for chef and set it to true by default #20

ghost commented Mar 29, 2018

eheydrick left a comment

eheydrick commented Mar 30, 2018

ghost commented Apr 4, 2018 •

edited by ghost

Loading

majormoses left a comment

majormoses Apr 4, 2018

ghost Apr 4, 2018

majormoses Apr 4, 2018

ghost Apr 4, 2018

majormoses Apr 6, 2018

eheydrick Apr 9, 2018

ghost Apr 9, 2018

majormoses Apr 4, 2018

ghost Apr 4, 2018

eheydrick commented Apr 4, 2018

ghost commented Apr 4, 2018

ghost left a comment

ghost Apr 4, 2018

majormoses Apr 6, 2018

majormoses left a comment

majormoses Apr 6, 2018

ghost Apr 9, 2018

majormoses Apr 6, 2018

ghost Apr 9, 2018

majormoses Apr 6, 2018

ghost Apr 9, 2018 •

edited by ghost

Loading

majormoses Apr 11, 2018

ghost Apr 11, 2018

eheydrick Apr 9, 2018

eheydrick Apr 9, 2018

ghost Apr 11, 2018

eheydrick commented Apr 12, 2018

majormoses commented Jun 6, 2018

eheydrick commented Jun 6, 2018

Added enable flag for chef and set it to true by default #20

Are you sure you want to change the base?

Added enable flag for chef and set it to true by default #20

Conversation

ghost commented Mar 29, 2018

eheydrick left a comment

Choose a reason for hiding this comment

eheydrick commented Mar 30, 2018

ghost commented Apr 4, 2018 • edited by ghost Loading

majormoses left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eheydrick commented Apr 4, 2018

ghost commented Apr 4, 2018

ghost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

majormoses left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Apr 9, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eheydrick commented Apr 12, 2018

majormoses commented Jun 6, 2018

eheydrick commented Jun 6, 2018

ghost commented Apr 4, 2018 •

edited by ghost

Loading

ghost Apr 9, 2018 •

edited by ghost

Loading