Just declare one trigger type for file watch sensor #3398

lakshmi-kannan · 2017-05-09T02:18:16Z

(virtualenv)vagrant@st2dev /m/s/s/st2 ❯❯❯ st2 rule-enforcement list                                           file_watch_line_trigger ✭ ✱ ◼
+--------------------------+------------------------+------------------------+------------------------+------------------------+
| id                       | rule.ref               | trigger_instance_id    | execution_id           | enforced_at            |
+--------------------------+------------------------+------------------------+------------------------+------------------------+
| 59372ad6d9d7ed0fe08ff0f1 | examples.sample_rule_f | 59372ad5d9d7ed0fe08ff0 | 59372ad6d9d7ed0fe08ff0 | Tue, 06 Jun 2017       |
|                          | ile_watch              | ed                     | f0                     | 22:21:09 UTC           |
+--------------------------+------------------------+------------------------+------------------------+------------------------+
(virtualenv)vagrant@st2dev /m/s/s/st2 ❯❯❯ st2 trigger-instance get 59372ad5d9d7ed0fe08ff0ed                   file_watch_line_trigger ✭ ✱ ◼
+-----------------+-----------------------------------------------------------+
| Property        | Value                                                     |
+-----------------+-----------------------------------------------------------+
| id              | 59372ad5d9d7ed0fe08ff0ed                                  |
| trigger         | {                                                         |
|                 |     "type": "linux.file_watch.line",                      |
|                 |     "ref": "linux.5a0829e8-5135-4e8e-9482-1ecd6d5082f6",  |
|                 |     "parameters": {                                       |
|                 |         "file_path": "/tmp/st2_test"                      |
|                 |     }                                                     |
|                 | }                                                         |
| occurrence_time | 2017-06-06T22:21:09.824000Z                               |
| payload         | {                                                         |
|                 |     "file_name": "st2_test",                              |
|                 |     "line": "I am a ttiny little star",                   |
|                 |     "file_path": "/tmp/st2_test"                          |
|                 | }                                                         |
| status          | processed                                                 |
+-----------------+-----------------------------------------------------------+
(virtualenv)vagrant@st2dev /m/s/s/st2 ❯❯❯ st2 execution get 59372ad6d9d7ed0fe08ff0f0                          file_watch_line_trigger ✭ ✱ ◼
id: 59372ad6d9d7ed0fe08ff0f0
status: succeeded (1s elapsed)
parameters:
  cmd: 'echo "{''file_name'': ''st2_test'', ''line'': u''I am a ttiny little star'', ''file_path'': ''/tmp/st2_test''}"'
result:
  failed: false
  return_code: 0
  stderr: ''
  stdout: '{''file_name'': ''st2_test'', ''line'': u''I am a ttiny little star'', ''file_path'': ''/tmp/st2_test''}'
  succeeded: true
(virtualenv)vagrant@st2dev /m/s/s/st2 ❯❯❯

Kami · 2017-05-09T07:46:51Z

I dug in and tried to track down why the you never actually see corresponding trace and trigger instance being created and rule being matched and executed.

You were correct and it's indeed a more systematic issue with triggers with parameters. The problem is that corresponding TriggerDB object is not found in the database (we don't create trigger objects for triggers with parameters) and as such, trigger instance and trace creation fails inside the rules engine fail.

The reason why this worked in the past (long time ago when I created a PR with that change) is because at that time traces concept didn't exist yet.

There are probably some quick workarounds possible (I quickly looked in ignoring trigger doesn't exist error in the rules engine, but it turned out a bit in a rabbit whole and without better and more thought out approach it's possible we / I are missing something important by doing that).

So the best approach probably is to think this through and come with a real solution (which might as well be what I proposed as a quick hack, but needs more digging in). In short, we need to update rules engine, etc. to support triggers with parameters.

For demo the best short-term solution probably is to not use trigger with parameters (even if we can get a quick change / fix out, it will only be available in dev and future v2.3.0 packages).

/cc @lakshmi-kannan @dzimine

Kami · 2017-05-09T09:05:23Z

@lakshmi-kannan Did more research, I believe the change which should work is modifying rules engine and all the trigger retrieval methods so they also know how to handle triggers with parameters inside the rules engine. We already do that right now in some scenarios where we pass around TriggerDB object, but that's not the case in rules engine (right now we don't have that context there).

It's also worth nothing that this is quite tricky because the rules engine code previously wasn't designed to handle that scenario.

For it to work, we would need to make rules engine aware of not just the trigger type (ref), but also the corresponding trigger id or parameters (preferably id, because dict look ups are hacky and messy).

This would require changes to the sensor dispatch code and rules engine and trigger retrieval stuff.

One of the problems with this approach is that sensor would need to know for each payload to which actual TriggerDB it belongs to. Right now we don't have this information in the sensor code, but we didn't really use this approach before so we can update sensors which utilize triggers with parameters to internally store this info and then do something like:

self._dispatcher.dispatch_for_trigger_with_parameters(trigger_db_id='trigger_db_id, trigger='trigger ref', payload={})

And then we would manipulate rules engine and trigger retrieval code so it also works if we already have TriggerDB id around.

WDYT?

lakshmi-kannan · 2017-05-24T22:17:20Z

@Kami I verified your claim and we do create "trigger" object in db.

> db.trigger_type_d_b.find({"_id": ObjectId("592601d5d9d7ed24a5453062")})
{ "_id" : ObjectId("592601d5d9d7ed24a5453062"), "description" : "Trigger which indicates a new line has been detected", "tags" : [ ], "uid" : "trigger_type:linux:file_watch.line", "name" : "file_watch.line", "pack" : "linux", "payload_schema" : {  }, "parameters_schema" : { "additionalProperties" : false, "type" : "object", "properties" : { "file_path" : { "required" : true, "type" : "string", "description" : "Path to the file to monitor" } } } }
> db.trigger_d_b.find({"_id": ObjectId("592601efd9d7ed24a545324e")})
{ "_id" : ObjectId("592601efd9d7ed24a545324e"), "uid" : "trigger:linux:b72b9c38-708f-4872-9a52-efae449d5221:851746484aeb7ece7f72c1e68c2d91a3", "name" : "b72b9c38-708f-4872-9a52-efae449d5221", "pack" : "linux", "type" : "linux.file_watch.line", "parameters" : { "file_path" : "/tmp/st2_test" }, "ref_count" : 1 }
>
bye
(virtualenv)vagrant@st2dev /m/s/s/st2 ❯❯❯ st2 rule get examples.sample_rule_file_watch                           file_watch_line_trigger ✭ ◼
+-------------+--------------------------------------------------------------+
| Property    | Value                                                        |
+-------------+--------------------------------------------------------------+
| id          | 592601efd9d7ed24a545324f                                     |
| uid         | rule:examples:sample_rule_file_watch                         |
| ref         | examples.sample_rule_file_watch                              |
| pack        | examples                                                     |
| name        | sample_rule_file_watch                                       |
| description | Sample rule custom trigger type - add a file to be watched   |
|             | by file_watch_sensor in linux pack.                          |
| enabled     | False                                                        |
| action      | {                                                            |
|             |     "ref": "core.local",                                     |
|             |     "parameters": {                                          |
|             |         "cmd": "echo "{{trigger}}""                          |
|             |     }                                                        |
|             | }                                                            |
| criteria    |                                                              |
| tags        |                                                              |
| trigger     | {                                                            |
|             |     "type": "linux.file_watch.line",                         |
|             |     "ref": "linux.b72b9c38-708f-4872-9a52-efae449d5221",     |
|             |     "parameters": {                                          |
|             |         "file_path": "/tmp/st2_test"                         |
|             |     }                                                        |
|             | }                                                            |
| type        | {                                                            |
|             |     "ref": "standard",                                       |
|             |     "parameters": {}                                         |
|             | }                                                            |
+-------------+--------------------------------------------------------------+
(virtualenv)vagrant@st2dev /m/s/s/st2 ❯❯❯

This is as per https://github.com/StackStorm/st2/blob/master/st2common/st2common/models/api/rule.py#L241.

So we are still back to not understanding the root cause :)

Kami · 2017-05-25T07:27:44Z

@lakshmi-kannan You are talking about Trigger Type and I was talking about Trigger.

We also create Trigger object, but the problem is because the way everything is currently structured right now, rules engine doesn't know anything about it since it's trigger with parameters and it fails to match it. My proposed solution above still stands, notably something along the lines of:

For it to work, we would need to make rules engine aware of not just the trigger type (ref), but also the corresponding trigger id or parameters (preferably id, because dict look ups are hacky and messy).

In any case, I followed it through the pipeline last time - you can easily amend rules engine by add some logging / printf and you will see where it fails / gets stuck (nothing to do with Trigger Type).

If you do that, I think you will come to the same conclusion as I did.

* master: (164 commits) Minor fix to mistral itests action raises ValueError for unknown uuid type. Fixes test and added enum to meta Fix style - use 4 spaces for a tab, throw instead of returning false. Fix lint, use 4 spaces for tab. Throw exception instead of returning false. Fix typo. Bump pack version. Update changelog. Update description. Add contributors field to chatops pack metadata file. Update changelog. Add CHANGES file to chatops pack. Bump chatops pack version. added support for uuid_type for multiple types and added tests wait for an execution to complete before continuing Optimize mistral task queries for st2resultstracker Add CHANGELOG fix some linting things dont do a nested chain bug fixes and errors ...

lakshmi-kannan · 2017-06-07T04:07:11Z

@Kami I dug in further and now I am on the same page with you :). RulesEngine, Trace and some other parts of the platform do not handle triggers with parameters. I added some commits and now I have the end-to-end trigger->rule->execution working but the commits are not final. I only did the changes to see what needs to be changed to get it to work. I learnt few ugliness in our data models. Over the next day or two, I'll get the PR to a point where we can merge an initial version that's acceptable and we can iterate again on the models and UX.

lakshmi-kannan · 2017-06-07T16:00:21Z

contrib/linux/sensors/file_watch_sensor.py

    def __init__(self, sensor_service, config=None):
        super(FileWatchSensor, self).__init__(sensor_service=sensor_service,
                                              config=config)
-        self._trigger_ref = 'linux.file_watch.line'


@Kami @dzimine This turned out to the root cause of all problems. Basically, I was using the trigger type as the "ref" which is incorrect. I should either be using "type"and "parameters" or the actual ref for the trigger object we create for this instance of type and parameters. For example,

Trigger type:

{ "_id" : ObjectId("59381e4ad9d7ed39ed238337"), "description" : "Trigger which indicates a new line has been detected", "tags" : [ ], "uid" : "trigger_type:linux:file_watch.line", "name" : "file_watch.line", "pack" : "linux", "payload_schema" : { "type" : "object", "properties" : { "file_name" : { "type" : "string" }, "line" : { "type" : "string" }, "file_path" : { "type" : "string" } } }, "parameters_schema" : { "additionalProperties" : false, "type" : "object", "properties" : { "file_path" : { "required" : true, "type" : "string", "description" : "Path to the file to monitor" } } } }

Trigger:

{ "_id" : ObjectId("59381e71d9d7ed39ed238524"), "uid" : "trigger:linux:7c883eb8-376d-40f4-a0f1-0cb3baa46ab9:851746484aeb7ece7f72c1e68c2d91a3", "name" : "7c883eb8-376d-40f4-a0f1-0cb3baa46ab9", "pack" : "linux", "type" : "linux.file_watch.line", "parameters" : { "file_path" : "/tmp/st2_test" }, "ref_count" : 1 }

And if you look at the rule, it's tied to the trigger ref we created and it has no knowledge of type and parameters.

{ "_id" : ObjectId("59381e71d9d7ed39ed238525"), "tags" : [ ], "uid" : "rule:examples:sample_rule_file_watch", "name" : "sample_rule_file_watch", "ref" : "examples.sample_rule_file_watch", "description" : "Sample rule custom trigger type - add a file to be watched by file_watch_sensor in linux pack.", "pack" : "examples", "type" : { "ref" : "standard", "parameters" : { } }, "trigger" : "linux.7c883eb8-376d-40f4-a0f1-0cb3baa46ab9", "criteria" : { }, "action" : { "ref" : "core.local", "parameters" : { "cmd" : "echo \"{{trigger}}\"" } }, "enabled" : true }

So for a single trigger type, for each instance of parameters, we end up creating a rule with a different "trigger". The trigger type information is only used to verify that the parameters someone supplies matches the schema. I have to verify if we throw an error if the parameters don't match. I'll validate that part.

In essence, the sensor should either use the actual "ref" for the trigger or a dict containing "type" and "parameters" so we can do a lookup and get the actual trigger ref ourselves. Everything from trigger instance to rules engine only understands trigger ref. They don't understand type and parameters.

I have to verify if we throw an error if the parameters don't match. I'll validate that part.

I verified by swapping "file_path" to "file_route" and the rule registered successfully without throwing errors :/. See trigger below

{ "_id" : ObjectId("593823d2d9d7ed49861671ad"), "uid" : "trigger:linux:745a7d8a-cc6e-458b-987d-a6b67de56761:15c31053a00d27a5438d559253101833", "name" : "745a7d8a-cc6e-458b-987d-a6b67de56761", "pack" : "linux", "type" : "linux.file_watch.line", "parameters" : { "file_route" : "/tmp/st2_test" }, "ref_count" : 1 }

This is a bug and it should be fixed. #3453

lakshmi-kannan · 2017-06-07T19:04:38Z

st2common/st2common/models/db/trigger.py

    RESOURCE_TYPE = ResourceType.TRIGGER_TYPE
    UID_FIELDS = ['pack', 'name']

+    ref = me.StringField(required=False)


I made it required=False so people upgrading don't have issues. At some point, we should make it required.

Also see why this "ref" change is required. #3454

dzimine · 2017-06-08T20:22:11Z

+1
Thanks for digging to the bottom of it. (and bunch of extra improvements in process).

Also, looks like we need good example on how to write a "proper" sensor.

Kami · 2017-06-12T07:20:15Z

st2common/st2common/services/rules.py

+        if trigger_ref:
+            return get_rules_with_trigger_ref(trigger_ref=trigger_ref)
+        else:
+            raise ValueError('Trigger dict %s is missing ``ref``.' % trigger)


Parenthesis around % (trigger) would make it a bit less error prone in the future.

Kami · 2017-06-12T07:23:29Z

LGTM.

I didn't see any additional tests, but it would be nice if we could write tests at least for all the edge cases for get_matching_rules_for_trigger method.

Kami · 2017-06-12T07:24:06Z

And we should also update changelog :)

Lakshmi Kannan added 7 commits May 8, 2017 19:17

Just declare one trigger type for file watch sensor

8a1525d

Deprecate linux.file_watch.line trigger

1a55bcb

Remove unneeded file_paths variable

d377362

Fix README.md

5a22501

Change path of the default file to tail

7de681c

Change trigger name to linux.file_watch.line

2c01e66

Add debug lines to file_watch_sensor

506cd55

Kami approved these changes May 9, 2017

View reviewed changes

Kami mentioned this pull request May 9, 2017

Update rules engine so it knows how to handle triggers with parameters #3399

Open

Lakshmi Kannan added 4 commits June 5, 2017 07:25

Checkpoint

a1b9f79

Checkpoint #2

09ca593

Checkpoint #3

36df9fd

Lakshmi Kannan added 4 commits June 7, 2017 08:42

Revert back trigger to StringField in trigger instance

c997969

Log debug rule query

f2e2b1f

Cleanup log changes

a0c95eb

Revert unneeded changes

ad31b4e

lakshmi-kannan commented Jun 7, 2017

View reviewed changes

Lakshmi Kannan added 2 commits June 7, 2017 11:59

Trigger and TriggerType models should have ref field

0128377

Name is required, ref is optional

b738eef

lakshmi-kannan commented Jun 7, 2017

View reviewed changes

Lakshmi Kannan added 2 commits June 8, 2017 08:34

Enable trigger parameters validation check

0096682

Lint and unit tests fixes

c555786

lakshmi-kannan changed the title ~~WIP: Just declare one trigger type for file watch sensor~~ Just declare one trigger type for file watch sensor Jun 8, 2017

lakshmi-kannan merged commit 019a6df into StackStorm:master Jun 9, 2017

lakshmi-kannan deleted the file_watch_line_trigger branch June 9, 2017 14:36

Kami reviewed Jun 12, 2017

View reviewed changes

arm4b added this to the 2.3.0 milestone Jun 12, 2017

cognifloyd mentioned this pull request Jan 30, 2024

Refactor FileWatchSensor to remove logshipper #5096

Draft

Uh oh!

Just declare one trigger type for file watch sensor #3398

Just declare one trigger type for file watch sensor #3398

Uh oh!

Conversation

lakshmi-kannan commented May 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kami commented May 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kami commented May 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lakshmi-kannan commented May 24, 2017

Uh oh!

Kami commented May 25, 2017

Uh oh!

lakshmi-kannan commented Jun 7, 2017

Uh oh!

lakshmi-kannan Jun 7, 2017

Choose a reason for hiding this comment

Uh oh!

lakshmi-kannan Jun 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lakshmi-kannan Jun 7, 2017

Choose a reason for hiding this comment

Uh oh!

lakshmi-kannan Jun 7, 2017

Choose a reason for hiding this comment

Uh oh!

dzimine commented Jun 8, 2017

Uh oh!

Kami Jun 12, 2017

Choose a reason for hiding this comment

Uh oh!

Kami commented Jun 12, 2017

Uh oh!

Kami commented Jun 12, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lakshmi-kannan commented May 9, 2017 •

edited

Loading

Kami commented May 9, 2017 •

edited

Loading

Kami commented May 9, 2017 •

edited

Loading

lakshmi-kannan Jun 7, 2017 •

edited

Loading