Skip to content

Conversation

@Kami
Copy link
Member

@Kami Kami commented Jan 7, 2019

This pull request fixes an issue with file watch sensor not working correctly since the trigger payload validation has been enabled by default a couple of releases ago.

The root cause was that validate_trigger_payload() method only worked with TriggerType reference, but not with Trigger reference.

FileWatch sensor is a bit special since it's the only sensor which works with actual triggers and not trigger types (aka issue didn't affect other sensors).

Resolves #4486

TODO

  • Test case

Kami added 5 commits January 3, 2019 17:50
1. Mitigate / avoid race when calling add_file() before tailer has been
initialized
2. Fix a bug with failing to dispatch a trigger due to code using
invalid trigger type reference (it uses trigger ref instead of trigger
type ref)
reference and not trigger type reference is passed to it.
@Kami Kami added the bug label Jan 7, 2019
@Kami Kami added this to the 2.10.2 milestone Jan 7, 2019
raise Exception('Trigger %s did not contain a ref.' % trigger)

# Wait a bit to avoid initialization race in logshipper library
eventlet.sleep(1.0)
Copy link
Member Author

Choose a reason for hiding this comment

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

There is an edge case in logshipper library.

If a trigger is added and add_file is called before the tailer is initialized, an exception will be thrown and the sensor will die / crash (this can happen because add_trigger method is called asynchronously and it could be called before run() and tailer fully initialized).

Copy link
Contributor

Choose a reason for hiding this comment

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

:(

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it's not ideal.

I could make it 100% deterministic by shuffling around the code (set initialized or similar variable to True inside run() and only proceed inside add_handler when that variable is True).

I think that's too complicated though since sleep seems to work just as fine (and it's one line of code).

It also only affects a single sensor right now so there is no point in handling that inside sensor container / base sensor class code.

@Kami Kami requested a review from bigmstone January 7, 2019 15:10
raise Exception('Trigger %s did not contain a ref.' % trigger)

# Wait a bit to avoid initialization race in logshipper library
eventlet.sleep(1.0)
Copy link
Contributor

Choose a reason for hiding this comment

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

:(

@Kami Kami merged commit 61b1de4 into master Jan 8, 2019
@Kami Kami deleted the file_watch_payload_validation_fix branch January 8, 2019 08:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

3 participants