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

Add a setting to only send DELETED notices #154

Merged

Conversation

agrare
Copy link
Member

@agrare agrare commented Nov 1, 2017

This adds a setting to the InventoryCollectorWorker which allows for only DELETED pod notices to be sent, thus reducing the traffic hitting MiqQueue.

@moolitayer
Copy link

@agrare can you please explain why/when :deleted_notices_only: true is useful?
(does not seem to give what you originally made the watch for)

@@ -70,6 +70,7 @@
:poll: 20.seconds
:ems_inventory_collector_worker:
:ems_inventory_collector_worker_kubernetes:
:deleted_notices_only: false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should rather expose what types to store? filter_notice_types: ['DELETED', ...] . As it might be ok to do DELETED + MODIFIED for g-release?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought about that too, I'm good with making it more complex when you have use for it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@agrare
Copy link
Member Author

agrare commented Nov 2, 2017

@moolitayer the content of the delete notice also still has the full spec of the pod so we can create the pod record from just the deletes. If we only care about not missing a pod then this will satisfy that.
I left it default to false so that we can get better testing of how the queue reacts to a large number of targets then we can switch it back before release if we want.

@moolitayer
Copy link

moolitayer commented Nov 2, 2017

I left it default to false so that we can get better testing of how the queue reacts to a large number of targets then we can switch it back before release if we want.

If we plan to release with true It's seems desired to test if the delete really is enough for the metrics collection too. If that should be enough for us maybe we don't need to stress + test the queue any more then that?

@Ladas
Copy link
Contributor

Ladas commented Nov 2, 2017

@agrare @moolitayer makes sense to switch on true by default and test mainly that. We can test with false on some big env later.

@agrare agrare force-pushed the setting_to_only_send_deleted_notices branch from 25d45b3 to a210725 Compare November 2, 2017 12:03
@miq-bot
Copy link
Member

miq-bot commented Nov 2, 2017

Checked commit agrare@a210725 with ruby 2.3.3, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks fine. 🍪

@agrare
Copy link
Member Author

agrare commented Nov 2, 2017

Okay defaulted to true

Copy link
Contributor

@Ladas Ladas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link

@moolitayer moolitayer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@moolitayer moolitayer merged commit 41a8bbe into ManageIQ:master Nov 2, 2017
@moolitayer moolitayer added this to the Sprint 73 Ending Nov 13, 2017 milestone Nov 2, 2017
@agrare agrare deleted the setting_to_only_send_deleted_notices branch November 2, 2017 13:50
simaishi pushed a commit that referenced this pull request Nov 6, 2017
Add a setting to only send DELETED notices
(cherry picked from commit 41a8bbe)
@simaishi
Copy link

simaishi commented Nov 6, 2017

Gaprindashvili backport details:

$ git log -1
commit b947c1aa7af2ef31aad6a0a831157dd40629793d
Author: Mooli Tayer <mtayer@redhat.com>
Date:   Thu Nov 2 15:49:17 2017 +0200

    Merge pull request #154 from agrare/setting_to_only_send_deleted_notices
    
    Add a setting to only send DELETED notices
    (cherry picked from commit 41a8bbeb45e6849b418a3f78d0377cb23f45528e)

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

Successfully merging this pull request may close these issues.

6 participants