fixes #11987 - fix checking exit code of deployment scripts #2760

machalekj · 2015-09-25T12:44:57Z

This commit fixes checking exit codes returned from provisioning templates, which were ignored
because of sending results through pipe to command tee. Also adds deleting hosts which deployment
fails because of non zero exit code.

theforeman-bot · 2015-09-25T12:44:58Z

There were the following issues with the commit message:

91e579b must be in the format fixes #redmine_number - brief description.

If you don't have a ticket number, please create an issue in Redmine, selecting the appropriate project.

More guidelines are available on the Foreman wiki.

This message was auto-generated by Foreman's prprocessor

domcleal · 2015-09-25T12:58:36Z

Also adds deleting hosts which deployment
fails because of non zero exit code.

My only hesitation about doing this is that it might make it harder to debug what's gone on, since the VM will get destroyed too.

machalekj · 2015-09-29T06:08:40Z

@domcleal: The original exception in this case was: "Provision script had a non zero exit, removing instance" - but removing wasn't done. In my case removing badly deployed instances would make it easier to re-run the failed deployment via API. What about making this deletion optional?

domcleal · 2015-09-29T07:09:53Z

Ah interesting, that makes sense then. Optional would be great if you'd be interested in doing it - perhaps a global setting would be enough for debugging purposes - a new entry under app/models/setting/provisioning.rb can be easily accessed as Setting[:example].

theforeman-bot · 2015-09-29T13:05:49Z

There were the following issues with the commit message:

91e579b must be in the format fixes #redmine_number - brief description.
7adb469 must be in the format fixes #redmine_number - brief description.

If you don't have a ticket number, please create an issue in Redmine, selecting the appropriate project.

More guidelines are available on the Foreman wiki.

This message was auto-generated by Foreman's prprocessor

machalekj · 2015-09-29T13:25:35Z

@domcleal Hi, I implemented deleting hosts as optional with default value keeping current state. Hope this can be easily used.

domcleal · 2015-09-30T07:44:22Z

[test]

dLobatog · 2015-10-02T08:32:22Z

app/models/concerns/orchestration/ssh_provision.rb

@@ -82,6 +82,10 @@ def setSSHProvision
      Host.find(id).built
      respond_to?(:initialize_puppetca,true) && initialize_puppetca && delAutosign if puppetca?
    else
+      if Setting[:clean_up_failed_deployment]
+        logger.info "Deleting host #{name} because of non zero exit code of deployment script."


Nitpick, if you could include the name of the script that'd help users when they debug what's going on.

dLobatog · 2015-10-02T08:37:10Z

@machalekj Nice fix, 👍 from my side. Could you write a test to make sure this feature still works in the future? It's already mostly untested and changes could break it without us noticing as it is right now.

Thank you!

machalekj · 2015-10-05T11:52:26Z

@elobato Hi, I am sorry I won't be able (at least without some hints). I am not yet familiar with Rails and simulating SSH service is a bit too much to start learning it. Or is there any test that creates real host which I could use as an example?

domcleal · 2015-10-12T13:27:59Z

app/models/concerns/orchestration/ssh_provision.rb

+      if Setting[:clean_up_failed_deployment]
+        logger.info "Deleting host #{name} because of non zero exit code of deployment script."
+        Host.find(id).destroy
+      end
      raise ::Foreman::Exception.new(N_("Provision script had a non zero exit, removing instance"))


The "removing instance" comment here is peculiar, I wonder if it changed in the past. Could you remove that part? We should to rely on the log message you added to say whether it did, or didn't remove the VM.

domcleal · 2015-10-13T08:30:55Z

[test]

theforeman-bot · 2015-10-13T08:57:28Z

There were the following issues with the commit message:

ae3afa2 must be in the format fixes #redmine_number - brief description.

If you don't have a ticket number, please create an issue in Redmine, selecting the appropriate project.

More guidelines are available on the Foreman wiki.

This message was auto-generated by Foreman's prprocessor

domcleal · 2015-10-13T08:58:29Z

Having the merge commit is probably what the bot's complaining about - but I can probably remove that on merge if you're having trouble with it, don't worry.

[test]

machalekj · 2015-10-13T09:16:17Z

Thanks, I just fixed it by removing the merge. That's what I indeed intended.

machalekj · 2015-10-13T13:04:58Z

@domcleal Can you please re-run tests? The revision which was actually used for test was wrong one (without my patch), which I immediately replaced by correct one.

domcleal · 2015-10-13T13:05:14Z

[test]

machalekj · 2015-10-13T14:06:07Z

@domcleal Hm, it's still testing old revision (b6b977a), instead of commit 52d67e3. Should I make some 'dummy' commit/rebuild just to enforce Jenkinks to use new correct revision?

domcleal · 2015-10-13T14:10:50Z

You'll have to ignore that, it's not really accurate since it's reporting the commit on develop that it's testing rather than the commit from your branch - it merges your branch in during the test. It looks like the test I wrote fails on MySQL. I'll have a closer look!

domcleal · 2015-10-13T14:50:58Z

test/fixtures/settings.yml

+attributes57:
+  name: clean_up_failed_deployment
+  category: Setting::Provisioning
+  default: false


I think I remember now. This should have been quoted, "false" for it to pass on MySQL.

This commit fixes checking exit codes returned from provisioning templates, which were ignored because of sending results through pipe to command tee. Also adds optional host deleting if deployment fails because of non zero exit code. This can be configured by setting clean_up_failed_deployment (default true).

… setting

machalekj · 2015-10-14T06:27:32Z

@domcleal ok, I fixed the fixture (and updated the its default value). Please test it.

domcleal · 2015-10-14T06:30:43Z

[test]

dLobatog · 2015-10-16T07:52:06Z

I'll squash this on merge. Thanks @machalekj @domcleal !

dLobatog · 2015-10-16T07:59:04Z

Merged as c919008, thanks again @machalekj @domcleal !

theforeman-bot added Waiting on contributor Not yet reviewed Needs testing labels Sep 25, 2015

machalekj changed the title ~~Fix bash command masking deployment script exit code~~ fixes #11987 - fix checking exit code of deployment scripts Sep 29, 2015

domcleal removed the Not yet reviewed label Sep 29, 2015

theforeman-bot added Needs re-review Needs testing Waiting on contributor and removed Waiting on contributor labels Sep 29, 2015

machalekj force-pushed the develop branch from 7adb469 to 6954b37 Compare September 29, 2015 13:23

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Sep 29, 2015

dLobatog reviewed Oct 2, 2015
View reviewed changes

dLobatog added Waiting on contributor and removed Needs re-review Needs testing labels Oct 2, 2015

domcleal added Needs re-review and removed Waiting on contributor labels Oct 6, 2015

domcleal reviewed Oct 12, 2015
View reviewed changes

theforeman-bot added Needs re-review Needs testing Waiting on contributor labels Oct 13, 2015

machalekj force-pushed the develop branch from 60b3a82 to ae3afa2 Compare October 13, 2015 08:57

theforeman-bot added Needs re-review Needs testing Waiting on contributor and removed Waiting on contributor labels Oct 13, 2015

machalekj force-pushed the develop branch from ae3afa2 to 52d67e3 Compare October 13, 2015 09:03

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Oct 13, 2015

domcleal reviewed Oct 13, 2015
View reviewed changes

machalekj and others added 3 commits October 14, 2015 07:52

refs #11987 - tests for SSH provisioning host destroy action

f9f0a97

refs #11987 - fix typo in test fixture for clean_up_failed_deployment…

a3acb9f

… setting

machalekj force-pushed the develop branch from 52d67e3 to a3acb9f Compare October 14, 2015 05:53

dLobatog closed this Oct 16, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixes #11987 - fix checking exit code of deployment scripts #2760

fixes #11987 - fix checking exit code of deployment scripts #2760

machalekj commented Sep 25, 2015

theforeman-bot commented Sep 25, 2015

domcleal commented Sep 25, 2015

machalekj commented Sep 29, 2015

domcleal commented Sep 29, 2015

theforeman-bot commented Sep 29, 2015

machalekj commented Sep 29, 2015

domcleal commented Sep 30, 2015

dLobatog Oct 2, 2015

dLobatog commented Oct 2, 2015

machalekj commented Oct 5, 2015

domcleal Oct 12, 2015

domcleal commented Oct 13, 2015

theforeman-bot commented Oct 13, 2015

domcleal commented Oct 13, 2015

machalekj commented Oct 13, 2015

machalekj commented Oct 13, 2015

domcleal commented Oct 13, 2015

machalekj commented Oct 13, 2015

domcleal commented Oct 13, 2015

domcleal Oct 13, 2015

machalekj commented Oct 14, 2015

domcleal commented Oct 14, 2015

dLobatog commented Oct 16, 2015

dLobatog commented Oct 16, 2015

fixes #11987 - fix checking exit code of deployment scripts #2760

fixes #11987 - fix checking exit code of deployment scripts #2760

Conversation

machalekj commented Sep 25, 2015

theforeman-bot commented Sep 25, 2015

domcleal commented Sep 25, 2015

machalekj commented Sep 29, 2015

domcleal commented Sep 29, 2015

theforeman-bot commented Sep 29, 2015

machalekj commented Sep 29, 2015

domcleal commented Sep 30, 2015

dLobatog Oct 2, 2015

Choose a reason for hiding this comment

dLobatog commented Oct 2, 2015

machalekj commented Oct 5, 2015

domcleal Oct 12, 2015

Choose a reason for hiding this comment

domcleal commented Oct 13, 2015

theforeman-bot commented Oct 13, 2015

domcleal commented Oct 13, 2015

machalekj commented Oct 13, 2015

machalekj commented Oct 13, 2015

domcleal commented Oct 13, 2015

machalekj commented Oct 13, 2015

domcleal commented Oct 13, 2015

domcleal Oct 13, 2015

Choose a reason for hiding this comment

machalekj commented Oct 14, 2015

domcleal commented Oct 14, 2015

dLobatog commented Oct 16, 2015

dLobatog commented Oct 16, 2015