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

Set EmsEvent ems_ref to event's uid, to avoid same-second collision #264

Merged
merged 1 commit into from
Jun 25, 2018

Conversation

cben
Copy link
Contributor

@cben cben commented Jun 24, 2018

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1583832
@miq-bot add-label bug, events, gaprindashvili/yes

EmsEvent.create_event drops "duplicate" events, determined by (event_type, timestamp, chain_id, ems_id, ems_ref).
Container events had no ems_ref (and no chain_id), meaning events of same type in same second collided and only one EmsEvent was created.
The lost events weren't handled by Automate / Policy.

Using k8s event's uid, not involvedObject's uid which still wouldn't be unique (for repeating events on same object).

  • Tested Automate runs 4 times for scaling from 0 to 4 pods.
    4 EmsEvents created, with same timestamp and different ems_ref and.
  • Passed same test on gaprindashvili.

@elad661 @moolitayer please review

EmsEvent.create_event drops "duplicate" events, determined by
(event_type, timestamp, chain_id, ems_id, ems_ref).
Container events had no ems_ref (and no chain_id), meaning events of same
type in same second collided and only one EmsEvent was created.
The lost events weren't handled by Automate / Policy.

Using k8s event's uid, not involvedObject's uid which still wouldn't
be unique (for repeating events on same object).

https://bugzilla.redhat.com/show_bug.cgi?id=1583832
@miq-bot
Copy link
Member

miq-bot commented Jun 24, 2018

Checked commit cben@6d1cb2c with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 0 offenses detected
Everything looks fine. 👍

Copy link
Member

@yaacov yaacov 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
Copy link

LGTM 👍

@moolitayer
Copy link

@cben I see you want to test on Gaprindashvili before we merge so I'm not merging.

@cben
Copy link
Contributor Author

cben commented Jun 25, 2018 via email

@moolitayer moolitayer merged commit 9ee6118 into ManageIQ:master Jun 25, 2018
@moolitayer
Copy link

@simaishi please wait for final ok from @cben before backporting this fix.

@cben
Copy link
Contributor Author

cben commented Jun 27, 2018

@simaishi Applies cleanly to gaprindashvili, tested there too, clear for backporting.
Will update BZ soon.

simaishi pushed a commit that referenced this pull request Jun 27, 2018
Set EmsEvent ems_ref to event's uid, to avoid same-second collision
(cherry picked from commit 9ee6118)

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1595324
@simaishi
Copy link

Gaprindashvili backport details:

$ git log -1
commit 1748f9b993cf67fb053ec34673fd5080302a144f
Author: Mooli Tayer <[email protected]>
Date:   Mon Jun 25 11:46:53 2018 +0300

    Merge pull request #264 from cben/event-ems-ref
    
    Set EmsEvent ems_ref to event's uid, to avoid same-second collision
    (cherry picked from commit 9ee61185a2c739f950e8021f7dd61578199ae350)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1595324

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.

5 participants