Skip to content
This repository has been archived by the owner on Nov 22, 2021. It is now read-only.

BZ#1184630 - deployments run forever #410

Merged
merged 1 commit into from Jan 23, 2015
Merged

Conversation

jistr
Copy link
Member

@jistr jistr commented Jan 22, 2015

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

Implement timeouts for actions which do polling. Note that if DynFlow
executor stops working, the timeout checking will stop working too,
and the deployment will still run forever. This patch mitigates issues
like Puppet not being triggered, provisioning failures etc., but not
DynFlow failures.

@jistr
Copy link
Member Author

jistr commented Jan 22, 2015

Might need discussion with mburns re which BZ we'd associate it with and if we want to merge it now. (The linked BZ's objective was to mitigate the IOError issue, however this issue cannot be mitigated from Staypuft. The timeouts still would catch quite a bit of other "deployment runs forever" problems -- e.g. failed provisioning or deadlocked puppet runs.)

@jistr
Copy link
Member Author

jistr commented Jan 23, 2015

Ok, ready for review :)

@mburns72h
Copy link
Contributor

I've update the bz information and added a comment that we're including this anyway.

ACK on timeouts as set. Functionality looks good visually.

@sseago
Copy link
Contributor

sseago commented Jan 23, 2015

I'm not totally clear on the under-the-covers dynflow stuff that this interacts with, but it looks reasonable on the surface here. visual ACK.

@mburns72h
Copy link
Contributor

One thing i might like to see is a helpful message saying something like:

You've reached the timeout set for this action. If the action is still ongoing, you can click on the "Resume Deployment" button to continue.

@knowncitizen
Copy link
Member

Would love to see a message, but ACK otherwise.

@jistr jistr force-pushed the timeouts branch 2 times, most recently from f95cb1b to 65bc051 Compare January 23, 2015 16:57
https://bugzilla.redhat.com/show_bug.cgi?id=1184630

Implement timeouts for actions which do polling. Note that if DynFlow
executor stops working, the timeout checking will stop working too,
and the deployment will still run forever. This patch mitigates issues
like Puppet not being triggered, provisioning failures etc., but not
DynFlow failures.
@jistr
Copy link
Member Author

jistr commented Jan 23, 2015

Updated with the message. Didn't redeploy to test it, but tested by copy pasting the changed part to irb. On the first try i forgot one + sign but all should be fine now :)

2.0.0-p598 :001 > module Staypuft
2.0.0-p598 :002?>   class Exception
2.0.0-p598 :003?>     end
2.0.0-p598 :004?>   end
 => nil 
2.0.0-p598 :005 > def fail(exc, text)
2.0.0-p598 :006?>   puts text
2.0.0-p598 :007?>   end
 => nil 
2.0.0-p598 :008 >             fail(::Staypuft::Exception,
2.0.0-p598 :009 >                      "You've reached the timeout set for this action. If the " +
2.0.0-p598 :010 >                      "action is still ongoing, you can click on the "
2.0.0-p598 :011?>                    "\"Resume Deployment\" button to continue.")
SyntaxError: (irb):11: syntax error, unexpected tSTRING_BEG, expecting ')'
                 "\"Resume Deployment\" button to continue.")
                  ^
(irb):11: syntax error, unexpected ')', expecting end-of-input
    from /home/jistr/.rvm/rubies/ruby-2.0.0-p598/bin/irb:12:in `<main>'
2.0.0-p598 :012 >             fail(::Staypuft::Exception,
2.0.0-p598 :013 >                      "You've reached the timeout set for this action. If the " +
2.0.0-p598 :014 >                      "action is still ongoing, you can click on the " +
2.0.0-p598 :015 >                      "\"Resume Deployment\" button to continue.")
You've reached the timeout set for this action. If the action is still ongoing, you can click on the "Resume Deployment" button to continue.
 => nil 
2.0.0-p598 :016 > 

@knowncitizen
Copy link
Member

NICE! 2nd ACK

@mburns72h
Copy link
Contributor

ack

sseago added a commit that referenced this pull request Jan 23, 2015
BZ#1184630 - deployments run forever
@sseago sseago merged commit b42c6da into theforeman:master Jan 23, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants