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

[3] "Deactivated Switch" for Collectors. #715

Closed
dmth opened this issue Sep 29, 2016 · 15 comments
Closed

[3] "Deactivated Switch" for Collectors. #715

dmth opened this issue Sep 29, 2016 · 15 comments
Assignees
Labels
component: configuration component: core feature Indicates new feature requests or new features usability
Milestone

Comments

@dmth
Copy link
Contributor

dmth commented Sep 29, 2016

Since Bots were split into different classes, it's possible to enhance certain types of bots. That is a very good development.

I'd like to add a "Deactivated Switch" to the CollectorBot, in order to enable the deactivation of Collectors. When a bot is starting / restarted it checks if it was deactivated, and automatically stops if this is the case.

This is useful for a temporary deactivation of collectors , for instance when feeds are providing erroneous data. Restarting the whole bot-net would not start such bots, once a "deactivated switch" is in place.

@aaronkaplan
Copy link
Member

On 29 Sep 2016, at 11:58, Dustin Demuth notifications@github.com wrote:

Since Bots were split into different classes, it's possible to enhance certain types of bots. That is a very good development.

I'd like to add a "Deactivated Switch" to the CollectorBot, in order to enable the deactivation of Collectors. When a bot is starting / restarted it checks if it was deactivated, and automatically stops if this is the case.

Excellent idea, I also thought about it before. Ideally it should be shown visually in the manager.

But please let's call this flag "enabled" [Y/N]

This is useful for a temporary deactivation of collectors , for instance when feeds are providing erroneous data. Restarting the whole bot-net would not start such bots, once a "deactivated switch" is in place.

Makes sense.

a.

@dmth
Copy link
Contributor Author

dmth commented Sep 29, 2016

But please let's call this flag "enabled" [Y/N]

That's possible. The default for enabled should be "yes", right? Or should a bot be deactivated unless it's explicitly activated?

Is there a guideline how to deal with such boolean Input (Y/N, 0/1, True/False [and string representations therof])? Because user-input might be manifold.

EDIT:

Ideally it should be shown visually in the manager.

Yes, that would be nice. But currently out of scope.

@aaronkaplan
Copy link
Member

On 29 Sep 2016, at 12:10, Dustin Demuth notifications@github.com wrote:

But please let's call this flag "enabled" [Y/N]
That's possible. The default for enabled should be "yes", right?
correct

Or should a bot be deactivated unless it's explicitly activated?

Is there a guideline how to deal with such boolean Input (Y/N, 0/1, True/False [and string representations therof])? Because user-input might be manifold.

Concerning the syntax of boolean in the config files: in JSON of course it needs to be a boolean value . That is a clearly defined syntax for JSON.
However, AFAIK the manager does not syntax check its input - in case that's what you meant (?)

Not sure if I understood the question correctly.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.

@dmth
Copy link
Contributor Author

dmth commented Sep 29, 2016

Concerning the syntax of boolean in the config files: in JSON of course it needs to be a boolean value . That is a clearly defined syntax for JSON. However, AFAIK the manager does not syntax check its input - in case that's what you meant (?)

Yes, this question does concern the manager. But it does also concern the JSON configuration, when we interpret it in a naive way, because:

j = '{"enabled" : false, "enabled_s": "false"}'
d = json.loads(j)

if d["enabled"]:
    print("Enabled")
else:
    print("Not Enabled")

=> Not Enabled

if d["enabled_s"]:
    print("Enabled")
else:
    print("Not Enabled")

=> Enabled

That's why I asked if there is a guideline on dealing with boolean's.
Which should be communicated in the manuals.

Unintended misconfiguration is so easy here ;-(

@aaronkaplan
Copy link
Member

On 29 Sep 2016, at 13:34, Dustin Demuth notifications@github.com wrote:

Concerning the syntax of boolean in the config files: in JSON of course it needs to be a boolean value . That is a clearly defined syntax for JSON. However, AFAIK the manager does not syntax check its input - in case that's what you meant (?)

Yes, this question does concern the manager. But it does also concern the JSON configuration, when we interpret it in a naive way, because:

j = '{"enabled" : false, "enabled_s": "false"}'
d = json.loads(j)

if d["enabled"]:
print("Enabled")
else:
print("Not Enabled")

=> Not Enabled

if d["enabled_s"]:
print("Enabled")
else:
print("Not Enabled")

=> Enabled

That's why I asked if there is a guideline on dealing with boolean's.
Which should be communicated in the manuals.

Unintended misconfiguration is so easy here ;-(

Generic answer:

  1. JSON syntax check (we could specify a JSON schema [1] for our config!)
  2. semantic check on the values.

That is related to the issue of having a "intelmqctl configtest" tool for 1.x or 2.0

[1] http://json-schema.org/
http://json-schema.org/examples.html
http://json-schema.org/latest/json-schema-core.html


You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.

@sebix
Copy link
Member

sebix commented Sep 29, 2016

See also certtools/intelmq-manager#81

@sebix sebix added feature Indicates new feature requests or new features usability component: core labels Sep 29, 2016
@sebix sebix added this to the v1.1 Feature release milestone Sep 29, 2016
@dmth
Copy link
Contributor Author

dmth commented Sep 30, 2016

See also #81

Sorry, I don't get that reference. Why is the "Cymru Encoding Fix" relevant?

@sebix
Copy link
Member

sebix commented Sep 30, 2016

On 09/30/2016 10:08 AM, Dustin Demuth wrote:

See also certtools/intelmq#81
<https://github.com/certtools/intelmq/pull/81>

Sorry, I don't get that reference. Why is the "Cymru Encoding Fix"
relevant?

Wrong repo, should be certtools/intelmq-manager#81

Because you asked how booleans should be saved:

Is there a guideline how to deal with such boolean Input (Y/N, 0/1,
True/False [and string representations therof])? Because user-input
might be manifold.

The answer is: Save as boolean.
Also related: #552 which defines types for configuration values.
And #692 which can check the types.

dmth added a commit to Intevation/intelmq that referenced this issue Oct 4, 2016
This introduces the enabled-switch for all Collector-Bots.
By doing so it fixes certtools#715.

The Collectorbot was extended with an own start-function
which checks if the enabled paramter is present and was set
to a representation of False. To check the "representation of
False" the function to_bool was introduced in utils.py, which
maps well-known-text to booleans. It has a a fallbackmode parameter
which determines if the function returns true of false if no
mapping  was found.

As long as to_bool does not evaluate to False the bot will be started.
@dmth
Copy link
Contributor Author

dmth commented Oct 4, 2016

The answer is: Save as boolean.
Ok, that would be an adaptation of the intelmq-manager. I don#t want to touch that code right now.

Anyway, I just finished an implementation of this switch. See:
Intevation@9d0db8a

@sebix
Copy link
Member

sebix commented Oct 4, 2016

Why only for collectors?

Implementation looks good. Tests would be nice, but not necessary.

@dmth
Copy link
Contributor Author

dmth commented Oct 4, 2016

Why only for collectors?

I was sure that this question will come up, eventually.
I thought it might be harmful if you can disable more central bots, such as experts, which would the block the queues. The problem which we solved with this Implementation was to disable certain bots which insert data into the queues. If you disable central bots, you will jam the system. Maybe a "short-circuit" switch would be better for these bots, as well as "Trash" switch for all outputs.

If this is not the problem, the code could easily be moved to the bots base class.

BTW: Do we want to merge this while it's hot? I'd create a PR then.

@sebix
Copy link
Member

sebix commented Oct 4, 2016

I thought more of parsers. E.g. in our current setups 20 collectors and parsers are stopped.

But anyway, the functionality is not at specific to collectors, so why constrain the use case?

Yes, you can create a PR and I merge it in the next days. Or I'll just cherry pick the commits^^

@aaronkaplan
Copy link
Member

On 04 Oct 2016, at 16:10, Dustin Demuth notifications@github.com wrote:

Why only for collectors?

I was sure that this question will come up, eventually.
I thought it might be harmful if you can disable more central bots, such as experts, which would the block the queues.

That's ok if done intentionally.
When testing , actually I often stop a central expert bot in order to assemble a reasonable queue.

The problem which we solved with this Implementation was to disable certain bots which insert data into the queues. If you disable central bots, you will jam the system.
Or intentionally queue up events.

Maybe a "short-circuit" switch would be better for these bots, as well as "Trash" switch for all outputs.

If this is not the problem, the code could easily be moved to the bots base class.

IMHO I envisioned something different by "enabled". Maybe the words were not quite clear.
I would assume something like a "pause" button: if pressed in the UI, the bot would simply stop processing events. If pressed again (or toggled), it would resume.

But yes, I think that should be a property of the base class.

@sebix sebix assigned sebix and dmth Oct 5, 2016
@dmth
Copy link
Contributor Author

dmth commented Oct 5, 2016

IMHO I envisioned something different by "enabled". Maybe the words were not quite clear. I would assume something like a "pause" button: if pressed in the UI, the bot would simply stop processing events. If pressed again (or toggled), it would resume.

That's more complex. If the code I proposed here can be kept, this could be achieved by adding a "config-reload" (a ticket exists for that somewhere) option and an additional check for enabled in the processing function

@aaronkaplan
Copy link
Member

On 05 Oct 2016, at 12:51, Dustin Demuth notifications@github.com wrote:

IMHO I envisioned something different by "enabled". Maybe the words were not quite clear. I would assume something like a "pause" button: if pressed in the UI, the bot would simply stop processing events. If pressed again (or toggled), it would resume.

That's more complex. If the code I proposed here can be kept, this could be achieved by adding a "config-reload" (a ticket exists for that somewhere) option and an additional check for enabled in the processing function

Today I discussed with @sebix about this.
"Pause" is something different, agreed.

We can implement that separately.

Apart from that I agree with the above said and think an "enabled" option is a good idea.

+1

@aaronkaplan aaronkaplan changed the title "Deactivated Switch" for Collectors. [3] "Deactivated Switch" for Collectors. Oct 5, 2016
@sebix sebix closed this as completed in #718 Nov 8, 2016
@ghost ghost removed this from the v1.1 Feature release milestone Jul 5, 2017
@ghost ghost modified the milestones: v1.0 Stable Release, v1.1 Feature release Jul 5, 2017
@ghost ghost unassigned sebix Jul 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: configuration component: core feature Indicates new feature requests or new features usability
Projects
None yet
Development

No branches or pull requests

3 participants