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.

Sign up for GitHub

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

Jump to bottom

Reload the ems object in the event catcher if we fail to start #14736

Merged
merged 1 commit into from
Apr 12, 2017
Merged

Reload the ems object in the event catcher if we fail to start #14736

merged 1 commit into from
Apr 12, 2017

Conversation

carbonin
Copy link
Member

When the embedded ansible role changes servers the endpoint url changes. This causes the event catcher to get http errors on the old url.

Reloading the object will pick up the new url and allow the event monitor thread to start.

https://bugzilla.redhat.com/show_bug.cgi?id=1439392

@carbonin
Reload the ems object in the event catcher if we fail to start
48f3479 
When the embedded ansible role changes servers the endpoint url
changes. This causes the event catcher to get http errors on the
old url.

Reloading the object will pick up the new url and allow the event
monitor thread to start.

https://bugzilla.redhat.com/show_bug.cgi?id=1439392
@carbonin carbonin requested a review from jameswnl April 11, 2017 20:58
@miq-bot
Copy link
Member

miq-bot commented Apr 11, 2017

Checked commit carbonin@48f3479 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks good. 🍰

jameswnl
jameswnl reviewed Apr 11, 2017
View reviewed changes
app/models/manageiq/providers/embedded_ansible/automation_manager/event_catcher/runner.rb
return tid unless tid.nil?

# Get a new copy of the ems record in case the embedded ansible role changed servers
@ems.reload
Copy link
Contributor

Choose a reason for hiding this comment

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

so I am not very familiar with the life cycle, 3 (maybe obvious to others) questions,

  • Do we need to reload before the call to super so that whatever being done in super will reflect the new state in ems?
  • The url is stored in endpoint which will be reloaded as well, right?
  • Is this behavior applicable to other managers?

Copy link
Member Author

Choose a reason for hiding this comment

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

  • Do we need to reload before the call to super so that whatever being done in super will reflect the new state in ems?

I didn't do it this way because I wanted to stay as close to the original implementation as possible which meant keeping the @ems "cache" around and only reloading when it might be necessary. This prevents us from having to go to the database every call (although I'm not sure this method is called very frequently so maybe that doesn't matter).

  • The url is stored in endpoint which will be reloaded as well, right?

It is and it definitely seems that way (I tested this out on live machines).

  • Is this behavior applicable to other managers?

I don't think so because other managers don't expect the endpoint url to change, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am wondering if we need this for the refresh worker also ... we didn't see the same problem, but maybe that's just because refreshes happen less frequently?

Copy link
Member Author

Choose a reason for hiding this comment

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

Spoke with @Fryguy and @agrare, we think this shouldn't be an issue for the refresher.

gtanzillo
gtanzillo approved these changes Apr 11, 2017
View reviewed changes
Copy link
Member

@gtanzillo gtanzillo left a comment

Choose a reason for hiding this comment

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

👍 Nice!

@bdunne bdunne merged commit 3d18b43 into ManageIQ:master Apr 12, 2017
@bdunne bdunne added this to the Sprint 59 Ending Apr 24, 2017 milestone Apr 12, 2017
simaishi pushed a commit that referenced this pull request Apr 13, 2017
@bdunne @simaishi
Merge pull request #14736 from carbonin/reload_ems_after_event_catche…
8ee9d83 
…r_failure

Reload the ems object in the event catcher if we fail to start
(cherry picked from commit 3d18b43)

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

Fine backport details:

$ git log -1
commit 8ee9d83189f0c67eb57a40dd34b4fec4d2f8f834
Author: Brandon Dunne <[email protected]>
Date:   Wed Apr 12 12:20:16 2017 -0400

    Merge pull request #14736 from carbonin/reload_ems_after_event_catcher_failure
    
    Reload the ems object in the event catcher if we fail to start
    (cherry picked from commit 3d18b43eb610cb998281a7727ca43a5859163d18)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1442172

@carbonin carbonin deleted the reload_ems_after_event_catcher_failure branch May 18, 2017 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jameswnl jameswnl jameswnl left review comments

@gtanzillo gtanzillo gtanzillo approved these changes

Assignees
No one assigned
Labels
blocker bug fine/backported providers/automation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@carbonin @miq-bot @simaishi @gtanzillo @jameswnl @blomquisg @Fryguy @bdunne